diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e8f02d..5b038bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,106 @@ All notable changes to this project are documented here. The format follows ## [Unreleased] +## [0.34.1] - 2026-09-01 + +A patch about honesty rather than capability. This product machine-enforces its own +marketing — a test fails if a HTTP client, a TLS stack, a database or a sandbox appears +in the crate, and another fails if a scripted session writes an alternate-screen or +mouse-capture sequence — and four published claims got past all of it, because no test +decided any of them. One of the four was a real defect that a script hits silently and +never investigates. + +### Changed + +**A usage error exits `1`. It used to exit `2`, and `2` means something else.** +`Cli::parse()` exits with clap's usage code, which is `2` — the number +`docs/CONTRACT.md` publishes as `REFUSED`, "denied, refused, or the plan was +rejected", since 0.5.0. So a mistyped flag and a run the policy stopped produced the +same status at the one surface this product offers to be branched on, and a CI job +asking "did the boundary stop this?" was told yes by every typo. io-cli's own +argument refusals already exited `1`, so the collision was not even consistent with +the rest of the binary. A parse failure is now `FAILED` (`1`), which is what the +contract always said it was; `--help` and `--version` are not failures and still +exit `0`. clap 4 publishes no builder knob for this, so the exit is taken from +`Cli::try_parse` and split on clap's own `use_stderr`. + +**`install.ps1` no longer writes the user PATH.** It prints the line to add, which is +what `install.sh` has always done and what the README has always said *both* scripts +do. A user PATH write is not a shell profile — the README's original wording was +literally true and its meaning was not — but it is still a change to the machine that +outlives the install, made by a script piped into a shell, in a place nobody has a +reason to look. Nothing is un-written: an install that already added +`%LOCALAPPDATA%\io\bin` keeps that entry, because the script cannot tell its own old +edit from one made on purpose. + +### Fixed + +**Exit `4` no longer promises an invocation for the pause that does not have one.** +The contract said exit `4` names the `question_id`, `plan_id` or `attempt_id` that +`io resume` needs. That is true of three of the four pauses. An approval is answered +by the person the run asked, at the terminal it asked from, and io-harness publishes +no resume entry point that takes one — the binary has always said so and only the +documentation over-claimed, so a script written from the page went looking for a +command that does not exist, in the one case where somebody is already waiting. + +**The headless refusal claim carves out the provider's own endpoint.** "Every +approval in a headless run is declined, and the refusal is fed back to the agent as +an observation it can adapt to" is true of every approval a tool call raises, and +false of one. The provider's endpoint is authorized once, before the run's first +step, so an `ask` verdict on that host ends the run with no turn to tell about it and +nothing to adapt around. Reachable only through an explicit `act = "net", effect = +"ask"` rule matching that host — the harness contributes an allow for it from a layer +of its own, so an ordinary `net`-denying posture still reaches the model — and +nothing warns first, because the notice an asking posture earns reads `write` and +`exec` and never `net`. + +**Drift tests now decide all four claims.** The exit-code table in `docs/CONTRACT.md` +is read as data and held against the constants row by row, with a pairwise +distinctness check, so a later release cannot trade one collision for a neighbour +without a test naming which row moved. The two corrected sentences are bound to the +functions they describe rather than to themselves. The installer and the README are +bound to each other, so a future edit that re-adds the PATH write fails rather than +quietly making the README a lie. The exit codes are asserted by driving the built +binary, because the decision lives in `src/main.rs` and nothing under `tests/` links +it. + +### Known limitations + +**A boundary refusal on the provider endpoint exits `1`, not `2`.** The case +documented above ends the run through io-harness's typed `Error::Refused`, and +`src/exec.rs` flattens that to a message before an exit code is chosen — so a real +policy refusal is reported as a failure, which is the same class of defect as the one +this release fixes, in the other direction. Correcting it means re-typing the error +seam and auditing which of the harness's other errors are refusals; that is more than +a patch, it is named in the documentation rather than left to be discovered, and it +is 0.35.0's. + +### Dependencies + +**Unchanged.** The io-harness pin stays at 0.73.0 where 0.34.0 put it, and the direct +dependency set is the same ten names. A patch that moved a pin across a minor with declared +breaks would not be a patch. + +### Upgrading + +**Read this if a script branches on any `io` command's exit status**, not only `io exec`. +The change is at the top-level parser, so `io resume`, `io mcp`, `io plugin`, `io config` +and `io skill` are affected the same way. `2` no longer means "refused or a usage error"; +it means refused. A usage error is `1`. A script that tested for `2` to detect a mistyped +flag now sees `1`, and one that tested for `2` to detect a policy refusal now gets a `2` +that only ever means that. Nothing else moved: `0`, `3`, `4`, `5` and `6` mean what they +have meant since 0.5.0 and 0.24.0. + +**On Windows, a fresh install no longer edits your PATH.** The installer prints the +line instead. An existing install keeps the PATH entry it already has. + +**And the printed line reaches less than the write did — say so rather than let it be +found.** A user PATH write was visible to cmd.exe, Explorer and every GUI-launched +program; `$env:Path += …` in a PowerShell profile is PowerShell's alone. The installer +now prints that distinction and points at System > Environment Variables for the rest. +`install.sh` has always had the same shell-specific limit, so the two are symmetric — but +on Windows this is a reduction in reach and not only a removal of a write. + ## [0.34.0] - 2026-08-31 0.32.0 made io-harness's `__` a wire detail: a bundle's contribution is drawn with @@ -3075,7 +3175,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.34.0...HEAD +[Unreleased]: https://github.com/initorigin/io-cli/compare/v0.34.1...HEAD +[0.34.1]: https://github.com/initorigin/io-cli/compare/v0.34.0...v0.34.1 [0.34.0]: https://github.com/initorigin/io-cli/compare/v0.33.0...v0.34.0 [0.33.0]: https://github.com/initorigin/io-cli/compare/v0.32.0...v0.33.0 [0.32.0]: https://github.com/initorigin/io-cli/compare/v0.31.0...v0.32.0 diff --git a/Cargo.lock b/Cargo.lock index dfd795d..1c519f4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1850,7 +1850,7 @@ dependencies = [ [[package]] name = "io-cli" -version = "0.34.0" +version = "0.34.1" dependencies = [ "clap", "crossterm", diff --git a/Cargo.toml b/Cargo.toml index 8d22a6e..b4b5c05 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "io-cli" -version = "0.34.0" +version = "0.34.1" 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 e979e66..2c42fa6 100644 --- a/README.md +++ b/README.md @@ -98,8 +98,8 @@ irm https://raw.githubusercontent.com/initorigin/io-cli/main/install.ps1 | iex 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: if the directory is -not on your `PATH`, the script prints the line to add. +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 diff --git a/docs/CONTRACT.md b/docs/CONTRACT.md index 026c666..9f4fa91 100644 --- a/docs/CONTRACT.md +++ b/docs/CONTRACT.md @@ -48,14 +48,28 @@ to the existing `3` would have moved exactly the runs `6` was invented for. | Code | Name | Means | | --- | --- | --- | | `0` | OK | The run finished | -| `1` | FAILED | The run failed | +| `1` | FAILED | The run failed, or the command line was not understood | | `2` | REFUSED | Denied, refused, or the plan was rejected | | `3` | CEILING | A step, time, cost or budget ceiling was reached, and nothing judged the work | -| `4` | PAUSED | The run is waiting on a question, a plan, an approval or an interrupted call — resumable with `io resume` | +| `4` | PAUSED | The run is waiting on a question, a plan, an approval or an interrupted call — resumable with `io resume`, except an approval | | `5` | UNFINISHED | The run ended in a state this table does not name | | `6` | UNVERIFIED | The work was judged by a verification gate and did not hold up | -Exit `4` names the `question_id`, `plan_id` or `attempt_id` that `io resume` needs. +Exit `4` names the `question_id`, `plan_id` or `attempt_id` that `io resume` needs — for three of +the four pauses. **An approval is the fourth and it names no invocation**, because there is none: +io-harness publishes no resume entry point that takes an approval, and one is answered by the +person the run asked, at the terminal it asked from. That pause names its `request_id` and says +that instead, rather than printing a command a script would find does not exist. **`io` does not +produce that pause today at all** — it declines every approval rather than deferring one, in a +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. + +**One boundary refusal does not reach `2` today, and this is a known defect rather than the +contract.** A headless run whose provider endpoint lands in the `ask` tier is refused by io-harness +before the run's first step; that typed refusal is flattened to a message before an exit code is +chosen, so it exits `1`. It is reachable only through an explicit `act = "net", effect = "ask"` +rule matching the provider host — see [the headless guide](guide/headless.md), which describes it +where an operator meets it. Every other refusal exits `2`. ## Configuration diff --git a/docs/guide/headless.md b/docs/guide/headless.md index 97a346a..8f57a59 100644 --- a/docs/guide/headless.md +++ b/docs/guide/headless.md @@ -25,10 +25,22 @@ permits the agent to try. **`--policy ask-writes` is refused.** Nothing in a headless run can answer an approval, so honouring it would turn *ask before writes* into *deny writes* -without saying so. Every approval in a headless run is declined, and the +without saying so. Every approval a *tool call* raises is declined, and the refusal is fed back to the agent as an observation it can adapt to — which is what it already does with a policy refusal. +**The provider's own endpoint is the exception, and it is not adaptable.** That +host is authorized once, before the run's first step, so a policy that puts it in +the `ask` tier refuses the run there — there is no turn yet to tell about it, and +nothing to adapt. Only an explicit `act = "net", effect = "ask"` rule matching the +provider host reaches this: the harness contributes an allow for that endpoint +from a layer of its own, so an ordinary `net`-denying posture still gets to the +model, and `io setup` writes no such rule. Worth knowing because nothing warns +first — the notice above reads `write` and `exec` and never `net`. **A run refused +this way exits `1` and not the `2` the table below gives for a boundary**: the +typed refusal is flattened to a message before an exit code is chosen. That is a +known defect rather than the contract. + ### Exit status | Code | Means | @@ -67,10 +79,10 @@ alone now has a sixth answer to handle, which is the point — before this relea there was no status a gated run could return that said the work was not good enough, because there were no gates. -**Exit `4` names the pause from 0.23.0, and the invocation that answers it.** -The closing line used to name the run id and nothing else, which addressed -none of the four pauses; it now names the question, plan or call the run stopped -on and the `io resume` that decides it. That release renumbered nothing and added +**Exit `4` names the pause from 0.23.0, and — for three of the four — the +invocation that answers it.** The closing line used to name the run id and +nothing else, which addressed none of the four pauses; it now names the question, +plan or call the run stopped on and the `io resume` that decides it. That release renumbered nothing and added nothing: `4` had been given to a pause that could not yet be answered for exactly that release. diff --git a/install.ps1 b/install.ps1 index f7dd252..1adbc4f 100644 --- a/install.ps1 +++ b/install.ps1 @@ -8,8 +8,9 @@ Downloads the artifact for this machine and the SHA256SUMS beside it, VERIFIES THE ARTIFACT BEFORE EXPANDING IT, and installs into %LOCALAPPDATA%\io\bin — a directory the current user owns. No administrator - rights, nothing written outside the user's own profile, and nothing left - behind if anything fails. + rights, nothing written outside that directory and a temporary one it removes + on the way out — not your PATH either, which is printed for you to set rather + than set for you — and nothing left behind if anything fails. The checksum defends against a truncated download and a tampered asset. It does not defend against a compromised repository; piping a script from the @@ -140,21 +141,38 @@ try { Copy-Item -Path $binary -Destination $installed -Force Write-Host "installed $installed" - # The USER PATH, never the machine PATH: the machine one needs administrator - # rights and would change the environment for everybody on the box. - $userPath = [Environment]::GetEnvironmentVariable('Path', 'User') + # The PATH line is printed rather than written. A user PATH write is not a + # shell profile — it goes to the user's own environment rather than a file + # they keep — but it is still a change to the machine that the operator did + # not ask for, made by a script they piped into a shell, and it outlives the + # install in a place they have no reason to look. The line below is one they + # can read before they run it. Nothing is un-written either, so an install + # from before this changed keeps whatever user PATH entry it added — this + # script cannot tell its own old edit from one the operator made on purpose, + # so it takes neither away. + # + # What is read is the PROCESS PATH, and that is not interchangeable with the + # user PATH in the registry now that nothing is written. $env:Path is what + # the advice below actually changes, and it is what the machine, user and + # session entries all resolve into — so an entry made any of those ways is + # seen here on the next run. Reading the registry value instead would be + # reading the one place the printed line never reaches: an operator who did + # exactly as they were told would be told to do it again on every update, + # forever, and the other branch would become unreachable for everybody who + # complied. install.sh reads $PATH for the same reason. $entries = @() - if ($userPath) { $entries = $userPath -split ';' | Where-Object { $_ } } + if ($env:Path) { $entries = $env:Path -split ';' | Where-Object { $_ } } if ($entries -notcontains $InstallDir) { - [Environment]::SetEnvironmentVariable('Path', (($entries + $InstallDir) -join ';'), 'User') Write-Host '' - Write-Host "$InstallDir has been added to your user PATH." - Write-Host 'Open a NEW terminal and run: io' + Write-Host "$InstallDir is not on your PATH. Add this to your PowerShell profile:" Write-Host '' - Write-Host 'If you would rather set it yourself, this is the line:' Write-Host " `$env:Path += ';$InstallDir'" + Write-Host '' + Write-Host 'then open a new terminal and run: io' + Write-Host 'That line reaches PowerShell. For cmd.exe, Explorer and GUI' + Write-Host 'programs, add it through System > Environment Variables.' } else { - Write-Host "$InstallDir is on your user PATH; run: io" + Write-Host "$InstallDir is on your PATH; run: io" } # The last line of the narration is the binary's own, not this script's: the diff --git a/src/exec.rs b/src/exec.rs index 517d29f..aa1fd64 100644 --- a/src/exec.rs +++ b/src/exec.rs @@ -392,7 +392,14 @@ pub fn asks_nobody_can_answer(policy: &Policy) -> Option { } /// The extra line a paused run gets, naming the handle that addresses the pause -/// and the invocation that acts on it. +/// and — for three of the four — the invocation that acts on it. +/// +/// The fourth is an approval, which no `io resume` entry point takes, so that arm +/// names the request and says so rather than offering a command that does not +/// exist. Written out here as well as in the arm because the unqualified version +/// of this sentence is what `docs/CONTRACT.md` and the headless guide both +/// carried until 0.34.1, and a summary that over-claims is how it got into two +/// pages in the first place. /// /// **The run id is not that handle, and through 0.22.0 this line printed only the /// run id.** Every resume entry point in io-harness takes a second number — the diff --git a/src/main.rs b/src/main.rs index 93d71b9..e579c0d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -62,7 +62,31 @@ fn main() -> ExitCode { } async fn run(report: &mut Vec) -> Result { - let cli = Cli::parse(); + // **A parse failure is `FAILED`, not `REFUSED`, and clap's default was the + // second one.** `Cli::parse()` exits with clap's usage code, which is 2 — + // the same 2 `exec::REFUSED` has meant since 0.5.0. `docs/CONTRACT.md` + // publishes those codes as a contract a CI job branches on, so the default + // told every such job that a mistyped flag was a boundary refusal, at the + // one surface this product offers to be scripted against. io-cli's own + // argument refusals — `--policy ask-writes` and the rest — already come back + // as `Err(String)` and exit 1, so the collision was not even consistent with + // the crate's own behaviour. + // + // Taken here rather than on the builder because clap 4 publishes no knob for + // it: there is no `error_exit_code` anywhere in `clap_builder` 4.6.6. The + // split is clap's own `use_stderr` — `--help` and `--version` are writes to + // stdout that are not failures and keep their 0, and everything clap sends + // to stderr is a usage error, which is what `FAILED` means. + let cli = Cli::try_parse().unwrap_or_else(|error| { + // Swallowing the print error is clap's own behaviour in `Error::exit`: + // a broken pipe on the way out is not worth a second failure. + let _ = error.print(); + std::process::exit(i32::from(if error.use_stderr() { + io_cli::exec::FAILED + } else { + 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/tests/docs.rs b/tests/docs.rs index e4e5425..e6e80d0 100644 --- a/tests/docs.rs +++ b/tests/docs.rs @@ -2311,3 +2311,174 @@ fn the_guide_says_a_skill_installs_under_its_own_name_and_a_bundle_removes_by_na ); } } + +/// Prose with its line breaks taken out, so an assertion is about the sentence +/// rather than about where the paragraph happened to wrap. +/// +/// This also normalises the CRLF working copy git hands Windows, which is how +/// 0.33.0 shipped a docs gate that read a whole document as one paragraph on one +/// platform and nowhere else. +fn unwrapped_prose(text: &str) -> String { + text.split_whitespace().collect::>().join(" ") +} + +/// **0.34.1 F6 — the exit-`4` documentation names the pause `io resume` cannot take.** +/// +/// The sentence said exit `4` names the id "that `io resume` needs", full stop. +/// That is true of three of the four pauses. An approval names no invocation, +/// because io-harness publishes no resume entry point that takes one — so a +/// script written from the old sentence went looking for a command that does not +/// exist, in the one case where a person is already waiting. +/// +/// Bound to the code and not to itself: [`io_cli::exec::parked`] is asked what it +/// really prints for each pause, so the page cannot be made true by editing the +/// page. Sabotage: drop the carve-out clause from either surface, or teach +/// `parked` to offer an `io resume` for an approval. Each fails this. +#[test] +fn f6_the_exit_four_documentation_names_the_approval_carve_out() { + let approval = io_cli::exec::parked( + &io_harness::RunOutcome::AwaitingApproval { + request_id: 7, + steps: 2, + }, + 41, + ) + .expect("an approval is a pause, and `parked` names every pause"); + + // The premise the documentation rests on. If this stops holding, the + // carve-out below is stale prose and should fail here rather than quietly + // keep describing behaviour that changed. + // + // The needle is the *invocation*, not the words `io resume` — this line + // names `io resume` in order to say it is not the answer, which is the whole + // point of it and the trap a `!contains("io resume")` walks into. + assert!( + !approval.contains("io resume 41"), + "`parked` now offers an `io resume` invocation for an approval, so the \ + carve-out written into the documentation is out of date: {approval}", + ); + assert!( + approval.contains("not by `io resume`"), + "the approval pause stopped saying what does not answer it, so an \ + operator is left to infer it from the absence of a command: {approval}", + ); + + for (pause, id) in [ + ( + io_harness::RunOutcome::AwaitingAnswer { + question_id: 7, + steps: 2, + }, + "question", + ), + ( + io_harness::RunOutcome::AwaitingPlan { + plan_id: 7, + steps: 2, + }, + "plan", + ), + ( + io_harness::RunOutcome::AwaitingRecovery { + attempt_id: 7, + steps: 2, + }, + "call", + ), + ] { + let line = io_cli::exec::parked(&pause, 41).expect("a pause names itself"); + assert!( + line.contains("io resume 41"), + "the {id} pause is one of the three that DO name an invocation, and it \ + stopped naming one — the documentation now over-carves rather than \ + over-claims: {line}", + ); + } + + let contract = unwrapped_prose(&read("docs/CONTRACT.md")); + for said in [ + // The table's own cell, so a reader who only skims the table is not told + // that every pause resumes. + "resumable with `io resume`, except an approval", + "for three of the four pauses", + "An approval is the fourth and it names no invocation", + ] { + assert!( + contract.contains(said), + "`docs/CONTRACT.md` no longer says {said:?}, so the exit-`4` contract is \ + back to promising an invocation for a pause that has none", + ); + } + + assert!( + unwrapped_prose(&guide("headless")).contains("for three of the four"), + "the headless guide's exit-`4` claim is unqualified again", + ); +} + +/// **0.34.1 F6, the second sentence — the headless guide carves out the `net` gate.** +/// +/// "Every approval in a headless run is declined, and the refusal is fed back to +/// the agent as an observation it can adapt to" was true of every approval a tool +/// call raises and false of one: the provider's own endpoint is authorized once, +/// before the run's first step, so an `ask` verdict there ends the run with no +/// turn to tell about it and nothing to adapt. +/// +/// The non-vacuity is the second half of the guide's claim — that nothing warns +/// beforehand — asserted against [`io_cli::exec::asks_nobody_can_answer`], which +/// reads `write` and `exec` and never `net`. A page can be edited; that function +/// cannot be edited by editing the page. +#[test] +fn f6_the_headless_guide_carves_out_the_provider_endpoint() { + // The notice an asking posture earns. `net` is deliberately not one of the + // tiers it inspects, which is exactly why the carve-out needs writing down. + // + // Every tier is set here rather than only `net`: io-harness's own default + // policy already asks about writes and commands, so a policy built by + // changing one field would earn the notice for the two tiers this test is + // not about and prove nothing. + let mut asking_net = io_harness::Policy::default(); + asking_net.defaults.write = io_harness::Effect::Deny; + asking_net.defaults.exec = io_harness::Effect::Deny; + asking_net.defaults.net = io_harness::Effect::Ask; + assert!( + io_cli::exec::asks_nobody_can_answer(&asking_net).is_none(), + "`asks_nobody_can_answer` now warns about a `net` tier that asks, so the \ + guide's \"nothing warns first\" is no longer true and the paragraph needs \ + rewriting rather than re-asserting", + ); + + let guide = unwrapped_prose(&guide("headless")); + for said in [ + // The scope of the general claim, narrowed to what is actually true. + "Every approval a *tool call* raises is declined", + "The provider's own endpoint is the exception, and it is not adaptable.", + // The one configuration that reaches it, so the carve-out is actionable + // rather than a warning about nothing. + r#"`act = "net", effect = "ask"`"#, + // And the exit code it produces today, which is not the one the table + // below it gives for a boundary. + "exits `1` and not the `2`", + ] { + assert!( + guide.contains(said), + "`docs/guide/headless.md` no longer says {said:?}, so the headless \ + refusal claim is back to covering a case it does not cover", + ); + } + + // And the contract page says it too. + // + // The first draft of this release admitted the wrong exit code in the guide + // and left `docs/CONTRACT.md` publishing `2` for every refusal, so the two + // pages contradicted each other about the number a CI job branches on — with + // the contradiction on the page headed "the contract a CI job depends on". + // An operator who reads one page and not the other is the whole audience for + // this carve-out, so it is asserted on both. + let contract = unwrapped_prose(&read("docs/CONTRACT.md")); + assert!( + contract.contains("One boundary refusal does not reach `2` today"), + "`docs/CONTRACT.md` no longer carries the exit-`2` carve-out, so it now \ + disagrees with the headless guide about the code a refused run exits", + ); +} diff --git a/tests/exec.rs b/tests/exec.rs index e3ce6e9..ffe4f1f 100644 --- a/tests/exec.rs +++ b/tests/exec.rs @@ -11,6 +11,10 @@ //! N1 — no interface code on the headless path. //! N6 — the plain output is not composed to a width. //! +//! 0.34.1 F1 — a clap parse failure exits 1, and `--help`/`--version` still 0. +//! 0.34.1 F2 — `docs/CONTRACT.md`'s exit-code table is the constants, row by row. +//! 0.34.1 F3 — io-cli's own argument refusals exit 1, on the same door. +//! //! 0.23.0 F8 — `io resume`: the parked runs are listable, one is resumable by id, //! the parked line names the id that addresses the pause, and the two pauses that //! cannot be carried on are refused in words rather than by a failed drive. @@ -2129,3 +2133,233 @@ fn o15_none_of_this_releases_interface_reaches_the_headless_path() { "the NDJSON line is io-harness's `RunEvent`, serialized whole", ); } + +/// The argv the door is driven with, and what the process said back. +/// +/// Every exit-code criterion in 0.34.1 is about a decision taken in +/// `src/main.rs`, which nothing under `tests/` links — so the only instrument +/// that can see it is the built binary. `-C` and the environment are not set +/// here on purpose: a clap parse failure happens before io-cli reads anything at +/// all, and a test that isolates a home it never reaches would be describing a +/// run that does not exist. +fn door(argv: &[&str]) -> std::process::Output { + std::process::Command::new(env!("CARGO_BIN_EXE_io")) + .args(argv) + .output() + .unwrap_or_else(|error| panic!("the built binary runs: {error}")) +} + +/// **0.34.1 F1 — a mistyped flag is `FAILED`, and it used to be `REFUSED`.** +/// +/// `Cli::parse()` exits with clap's usage code, which is `2`, and `exec::REFUSED` +/// has been `2` since 0.5.0. `docs/CONTRACT.md` publishes those codes as a +/// contract a CI job branches on, so every typo told such a job that a boundary +/// had said no — at the one surface this product offers to be scripted against, +/// and while io-cli's own argument refusals correctly exited `1`. +/// +/// Sabotage: put `Cli::parse()` back in `src/main.rs`. Only this fails. +#[test] +fn f1_an_unknown_flag_exits_failed_rather_than_refused() { + for argv in [ + // The global door, before any subcommand is chosen. + &["--nonexistent-flag"][..], + // And inside the subcommand, which is a different clap parser and the + // one a CI job actually invokes. + &["exec", "--nonexistent-flag"][..], + &["exec", "--nonexistent-flag", "a goal"][..], + // A flag that exists carrying a value it does not accept: still a usage + // error, and the shape a script is likeliest to produce by hand. + &["exec", "--policy", "not-a-posture", "a goal"][..], + ] { + let run = door(argv); + + assert_eq!( + run.status.code(), + Some(i32::from(io_cli::exec::FAILED)), + "`io {}` did not exit FAILED ({}). clap's own default is {}, which is \ + what REFUSED means, and that collision is what this release removes. \ + stderr was:\n{}", + argv.join(" "), + io_cli::exec::FAILED, + io_cli::exec::REFUSED, + String::from_utf8_lossy(&run.stderr), + ); + } +} + +/// **0.34.1 F1, the other half — `--help` and `--version` are not failures.** +/// +/// clap reports both as errors and separates them from real ones by the stream +/// it writes to, so a fix that reads "any clap error exits 1" would make `io +/// --help` a failing command. That is the neighbour this release must not shift, +/// and it is one a script notices immediately. +#[test] +fn f1_help_and_version_are_still_not_failures() { + for argv in [ + &["--help"][..], + &["-h"][..], + &["--version"][..], + &["exec", "--help"][..], + ] { + let run = door(argv); + + assert_eq!( + run.status.code(), + Some(i32::from(io_cli::exec::OK)), + "`io {}` is a request, not a failure, and it exited non-zero. \ + stderr was:\n{}", + argv.join(" "), + String::from_utf8_lossy(&run.stderr), + ); + assert!( + !run.stdout.is_empty(), + "`io {}` exited 0 without writing anything, so the code is right for \ + the wrong reason", + argv.join(" "), + ); + } +} + +/// **0.34.1 F3 — io-cli's own argument refusals exit `FAILED` too.** +/// +/// They already did; the assertion exists so the two paths are proven to agree +/// rather than assumed to. `--policy ask-writes` is refused by +/// [`io_cli::exec::posture_for`] before a store is opened, a session is created +/// or a provider is built, so this drives the real binary with no credential and +/// reaches no network. +/// +/// The home is isolated because this argv gets past clap and into `run`, which +/// adopts a home and discovers a configuration — pointed at a fixture so the +/// test cannot read, move or create anything in the developer's own `~/.io-cli`. +#[test] +fn f3_io_clis_own_argument_refusal_exits_failed_too() { + let fixture = tempfile::tempdir().expect("a fixture"); + let root = fixture.path(); + + let mut command = std::process::Command::new(env!("CARGO_BIN_EXE_io")); + command + .arg("-C") + .arg(root) + .arg("exec") + .arg("--policy") + .arg("ask-writes") + .arg("say nothing") + .env_remove(io_harness::config::CONFIG_VAR) + .env_remove(io_harness::config::CONFIG_HOME_VAR); + #[cfg(windows)] + command + .env("USERPROFILE", root) + .env("APPDATA", root.join("AppData").join("Roaming")); + #[cfg(not(windows))] + command.env("HOME", root).env_remove("XDG_CONFIG_HOME"); + + let run = command.output().expect("the built binary runs"); + let stderr = String::from_utf8_lossy(&run.stderr).into_owned(); + + assert_eq!( + run.status.code(), + Some(i32::from(io_cli::exec::FAILED)), + "`io exec --policy ask-writes` is io-cli's own refusal of an argument, \ + which is FAILED and not REFUSED — the boundary never got a chance to \ + say anything. stderr was:\n{stderr}", + ); + assert!( + stderr.contains("cannot be honoured without a terminal"), + "the refusal did not come from `posture_for`, so this test exited 1 for \ + some other reason and proves nothing. stderr was:\n{stderr}", + ); +} + +/// **0.34.1 F2 — the documented table is the constants, row by row.** +/// +/// The fix changes what clap does for one class of failure, and clap governs +/// more paths than the one it has in mind. A sentence asserting "the codes are +/// unchanged" is satisfiable by any table; this reads `docs/CONTRACT.md`'s rows +/// as data and holds each one against the constant of its own name, so a release +/// that traded one collision for a neighbour would have to say which row moved. +/// +/// The distinctness assertion is the property the whole release is about: two +/// documented codes sharing a number is exactly the defect being removed, and it +/// is invisible to a per-row check. +#[test] +fn f2_the_documented_exit_code_table_is_the_constants() { + // git hands Windows a CRLF working copy, and a section split on "\n## " + // would find no boundary there and take the rest of the document. + let page = std::fs::read_to_string( + std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("docs/CONTRACT.md"), + ) + .expect("the contract page") + .replace("\r\n", "\n"); + + let section = page + .split("\n## Exit codes\n") + .nth(1) + .expect("`docs/CONTRACT.md` no longer has an `Exit codes` section") + .split("\n## ") + .next() + .expect("the section's own text"); + + // `| `0` | OK | The run finished |` — the code, then the name. The header and + // the separator carry no number and drop out here rather than by position. + let documented: Vec<(u8, String)> = section + .lines() + .filter_map(|line| { + let mut cells = line.trim().strip_prefix('|')?.split('|').map(str::trim); + let code = cells.next()?.trim_matches('`').parse::().ok()?; + Some((code, cells.next()?.to_string())) + }) + .collect(); + + let constants = [ + ("OK", io_cli::exec::OK), + ("FAILED", io_cli::exec::FAILED), + ("REFUSED", io_cli::exec::REFUSED), + ("CEILING", io_cli::exec::CEILING), + ("PAUSED", io_cli::exec::PAUSED), + ("UNFINISHED", io_cli::exec::UNFINISHED), + ("UNVERIFIED", io_cli::exec::UNVERIFIED), + ]; + + assert_eq!( + documented.len(), + constants.len(), + "the page documents {} codes and this crate defines {}; whichever moved, \ + the contract and the binary now disagree. Parsed:\n{documented:?}", + documented.len(), + constants.len(), + ); + + for (name, value) in constants { + let (code, _) = documented + .iter() + .find(|(_, documented)| documented == name) + .unwrap_or_else(|| { + panic!("`{name}` is a code this crate exits with and the page does not name it") + }); + assert_eq!( + *code, value, + "the page documents `{name}` as {code} and `io_cli::exec::{name}` is {value}", + ); + } + + // And the property the release is actually about, asserted over the CONSTANTS + // rather than over the table. + // + // Over the table it would have no arm at all: the length check and the + // per-row check between them make a duplicate documented code unreachable — + // whichever row you move, one of those two fires first. Over the constants it + // has one, and it is the exact regression 0.34.1 removed: define two codes as + // the same number, document them both that way honestly, and every other + // assertion here passes while a script loses the ability to tell two endings + // apart. Found by running the sabotage arm and getting nothing. + for (index, (name, code)) in constants.iter().enumerate() { + for (other_name, other) in constants.iter().skip(index + 1) { + assert_ne!( + code, other, + "`exec::{name}` and `exec::{other_name}` are both {code}. A script \ + branching on that number cannot tell the two endings apart, which \ + is the collision this release exists to remove", + ); + } + } +} diff --git a/tests/installers.rs b/tests/installers.rs index 41e8cee..7c47472 100644 --- a/tests/installers.rs +++ b/tests/installers.rs @@ -248,10 +248,11 @@ fn n8_neither_installer_asks_for_administrator_rights() { // the user's own profile. Neither is a system path. assert!(shell.contains("$HOME/.local/bin"), "{shell}"); assert!(powershell.contains("LOCALAPPDATA"), "{powershell}"); - assert!( - powershell.contains("'User'"), - "install.ps1 must set the USER PATH; the machine PATH needs administrator rights", - ); + // `'User'` used to be asserted here, because the script wrote the user PATH + // and the property was that it wrote the USER one and never the machine one. + // It writes neither now, so the needle would only have pinned a spelling + // nothing depends on. What survives is the half that is still a permission + // claim. assert!( !powershell.contains("'Machine'"), "install.ps1 must not touch the machine PATH", @@ -317,6 +318,69 @@ fn the_artifact_names_match_what_the_release_workflow_builds() { assert!(powershell.contains("x86_64-pc-windows-msvc")); } +/// The README's promise that neither installer writes your environment. +/// +/// The paragraph and the two scripts are one fact kept in three files, and this +/// is the direction that already went wrong: `install.ps1` set the user PATH +/// while the README told people neither script edited anything, and nothing +/// failed, because no test read the two together. The failure this exists to +/// catch is a future edit that re-adds the write for the operator's convenience +/// — it now has to change the README in the same commit, instead of leaving it +/// quietly lying about what the script does to the machine. +#[test] +fn the_readme_promise_that_neither_installer_writes_the_environment_holds() { + let readme = std::fs::read_to_string(repo().join("README.md")).expect("README.md"); + // git hands Windows a CRLF working copy, and the fragments below would then + // be found at the wrong offsets or not at all. + let powershell = std::fs::read_to_string(repo().join("install.ps1")) + .expect("install.ps1") + .replace("\r\n", "\n"); + let shell = std::fs::read_to_string(repo().join("install.sh")).expect("install.sh"); + + // Half one: the claim is still being made, in the words 0.34.1 gave it. + // + // The needle is the WHOLE clause including `or your `PATH``. A needle of + // `neither edits your shell profile` alone is a prefix of the sentence as it + // read *before* this release, so reverting the README half would have left + // this test green — the one fragment added to guard the promise being the one + // fragment the guard could not see. + assert!( + readme.contains("neither edits your shell profile or your `PATH`"), + "the README no longer promises what this test holds the scripts to", + ); + assert!( + readme.contains("the script prints the line to add"), + "the README no longer says the PATH line is printed rather than set", + ); + + // Half two: neither script writes it. Both are scanned, because the README + // says NEITHER — the shell script cannot call these, and that is the point: + // the promise is about the pair, not about the one that broke it. + for (name, text) in [("install.sh", &shell), ("install.ps1", &powershell)] { + for written in ["SetEnvironmentVariable", "setx "] { + assert!( + !text.contains(written), + "{name} contains {written:?}; the README says neither installer writes your \ + environment, so this is a lie in one file or the other", + ); + } + } + + // Half three, and the one that keeps halves one and two from being satisfied + // by a script that does nothing at all. The README does not only promise an + // absence: it promises the line is PRINTED. Both scripts are held to actually + // printing it, in the spelling each one really uses, so deleting the advice + // along with the write would fail here rather than read as compliance. + assert!( + powershell.contains(r#"Write-Host " `$env:Path += ';$InstallDir'""#), + "install.ps1 no longer prints the PATH line the README says it prints", + ); + assert!( + shell.contains(r#"say " export PATH=\"$dest:\$PATH\"""#), + "install.sh no longer prints the PATH line the README says it prints", + ); +} + /// F10 — `install.sh` prints the whole install, in order, with the values in it. /// /// The archive's real checksum is computed here and matched against BOTH the @@ -523,7 +587,19 @@ fn f10_the_powershell_installer_prints_the_same_ordered_sequence() { "Write-Host 'checksum ok'".to_string(), r#"Write-Host "unpacked $archive""#.to_string(), r#"Write-Host "installed $installed""#.to_string(), - "your user PATH".to_string(), + // The PATH advice, in the order the two branches are written: the + // one that prints the line to add comes first, and the line itself + // is matched by its text rather than by the branch around it — + // print a different line, or stop printing one, and this fails. + r#"Write-Host "$InstallDir is not on your PATH. Add this to your PowerShell profile:""# + .to_string(), + r#"Write-Host " `$env:Path += ';$InstallDir'""#.to_string(), + // What that line does and does not reach. A profile edit is + // PowerShell's, and the write it replaces was every process on the + // machine — saying so is the difference between a smaller promise and + // a silent regression in reach. + "For cmd.exe, Explorer and GUI".to_string(), + r#"Write-Host "$InstallDir is on your PATH; run: io""#.to_string(), "& $installed --version".to_string(), ], );