From c25ee9fec37b33ce701705d58842ff202ccc64aa Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Wed, 26 Aug 2026 21:53:46 +0530 Subject: [PATCH 01/10] docs: the lru alert closed as fixed, not dismissed The comment said alert #1 was dismissed as not-reachable. It was not: it closed as `fixed` at 2026-08-26T14:57:27Z, when the merge put `lru 0.18.2` in Cargo.lock, before any dismissal was issued. The plan to dismiss was overtaken by the dependency change that answered it. The unreachability evidence stays, because it is why this was never urgent and why the removal could be planned rather than rushed. What keeps the tree clean from here is the lockfile gate, not that judgement. 0.18.0's sealed record carries the same wording. It is immutable and pinned and the merge to main has happened, so that correction is stated in the release report rather than amended in. --- .github/dependabot.yml | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/.github/dependabot.yml b/.github/dependabot.yml index 6df81b4..ca961f1 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -41,12 +41,22 @@ updates: # graph and closed nothing. So the dependency was removed rather than # bumped: the editing model is `src/editor.rs` now, and ratatui is on 0.30. # - # Alert #1 was dismissed as not-reachable rather than fixed by a version, - # and the evidence is this: in ratatui 0.29 `lru` backed the layout cache - # only (`src/layout/layout.rs:9` and `:31`), and `iter_mut` appears nowhere - # under `ratatui-0.29.0/src/layout/` — so `lru::IterMut`, which is the - # entire subject of the advisory, was never constructed on any path this - # binary could reach. The tree is clean by version now in any case: + # **Alert #1 closed as `fixed`, not dismissed**, at 2026-08-26T14:57:27Z — + # the moment the merge put `lru 0.18.2` in `Cargo.lock`. The plan had been + # to dismiss it as not-reachable, and the plan was overtaken: a dependency + # change that moves the version out of range closes the alert on its own, + # and a dismissal on top would have recorded a waiver for something that was + # actually repaired. Check an alert's state after the merge before + # dismissing anything. + # + # The unreachability evidence is kept because it is why this was never + # urgent, and why the release could be planned around the migration rather + # than rushed: in ratatui 0.29 `lru` backed the layout cache only + # (`src/layout/layout.rs:9` and `:31`), and `iter_mut` appears nowhere under + # `ratatui-0.29.0/src/layout/` — so `lru::IterMut`, the entire subject of the + # advisory, was never constructed on any path this binary could reach. + # + # What holds the tree clean from here is a gate rather than a judgement: # `ratatui-core` 0.30 takes a current `lru`, and # `tests/dependencies.rs::n1_the_composer_is_free_and_no_lru_is_in_the_advisorys_range` # fails the build if any `lru` in the lockfile falls inside the range. From 7b0609393753187d6df8faa88574e2139c0ce9d8 Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Thu, 27 Aug 2026 00:27:02 +0530 Subject: [PATCH 02/10] feat(commands): move /mcp and /provider to the configure group and add /skills MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both add, edit, disable and remove entries in the configuration file, which is what Group::Configure means, and neither is an inspection — Group::Inspect's own doc says none of it changes what a turn does. The move is a correction, and the room it leaves is what /skills takes. Session 5, Turn 6, Inspect 9, Configure 6 — twenty-six commands, no group above ten. --- src/commands.rs | 43 +++++++++++++++++++++++---- tests/commands.rs | 75 +++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 111 insertions(+), 7 deletions(-) diff --git a/src/commands.rs b/src/commands.rs index 6fa77f0..7b94229 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -175,6 +175,16 @@ pub const COMMANDS: &[(&str, &str)] = &[ "/memory", "what io remembers: the instruction files, and the agent's own notes", ), + // Beside `/memory` because the two answer the halves of one question: that + // one says what io was *told*, this one says what it was *taught*. A skill is + // the only thing in either list io-cli did not write and does not parse — the + // rows come from io-harness's own discovery — so an operator who cannot see + // which ones are on, and out of which file, has no way to account for a turn + // that read one. + ( + "/skills", + "every skill, shipped or yours: what it is for, whether it is on, and its file", + ), ( "/mcp", "the MCP servers configured, and what this session has seen of each", @@ -207,11 +217,11 @@ pub const COMMANDS: &[(&str, &str)] = &[ /// /// **Grouped by the operator's intent rather than by which part of the harness /// answers**, because the second is an implementation detail and the first is -/// the only thing somebody scanning a list of twenty-five is holding in their +/// the only thing somebody scanning a list of twenty-six is holding in their /// head. /// /// Four groups and none longer than ten, which is the bound `tests/commands.rs` -/// asserts. A flat list of twenty-five is a list nobody reads; 0.16.0 is the +/// asserts. A flat list of twenty-six is a list nobody reads; 0.16.0 is the /// release that grouped them, at twenty, and every release since has added to a /// group rather than to a list. **The count in this paragraph is the one number /// here that goes stale on its own** — it said twenty through 0.17.0, which had @@ -226,6 +236,13 @@ pub enum Group { /// opens a list; none of it changes what a turn does. Inspect, /// Change the configuration file. + /// + /// **A surface that lists the file before it writes to it is still here**, and + /// `/mcp` and `/provider` are the two — they were under `Inspect` through + /// 0.18.0 on the strength of their first screen. Both add, edit, disable and + /// remove entries, so an operator who opened one to look is one keystroke from + /// changing what the next turn talks to; that is the sentence `Inspect` + /// promises it will never say. Configure, } @@ -282,16 +299,32 @@ pub const GROUPS: &[(Group, &[&str])] = &[ "/context", "/expand", "/fleet", - "/mcp", - "/provider", + "/skills", "/image", "/copy", "/copy diff", ], ), + // **`/mcp` and `/provider` moved here in 0.19.0, and it is a correction rather + // than a way of making room.** Both were grouped by the screen they open, and + // both go on from that screen to add, edit, disable and remove entries in the + // configuration file — which is what this group means and what `Inspect` + // promises it does not do. It takes `Inspect` to eight and `/skills` puts it + // back to nine, and the order of those two sentences is worth keeping: the + // room was made by filing two commands where they belong, not by filing one + // where there was space. A reader who finds `/mcp` under "configure" should be + // able to work out why from the panel it opens, without knowing what the + // release before it looked like. ( Group::Configure, - &["/config", "/theme", "/remember", "/memory"], + &[ + "/config", + "/theme", + "/remember", + "/memory", + "/mcp", + "/provider", + ], ), ]; diff --git a/tests/commands.rs b/tests/commands.rs index 9c6efb4..cfc116c 100644 --- a/tests/commands.rs +++ b/tests/commands.rs @@ -59,6 +59,9 @@ fn the_commands_are_the_commands() { // files, and which one is half of every decision made here. "/remember", "/memory", + // 0.19.0 — the other half of `/memory`'s question: that one is what io + // was told, this one is what it was taught. + "/skills", "/mcp", "/provider", "/profile", @@ -611,8 +614,10 @@ fn the_memory_commands_are_configure_commands_with_working_palette_rows() { COMMANDS.iter().any(|(command, _)| *command == name), "{name} is not in the inventory, so nobody is told it exists", ); - // Configure, and not Inspect: both of these write, and Inspect already - // holds ten, which is the bound `f13_no_group_is_longer_than_ten` asserts. + // Configure, and not Inspect: both of these write. That was already the + // reason in 0.18.0, when Inspect was also full; it is the whole reason + // now, because 0.19.0 moved `/mcp` and `/provider` out on the same + // argument and left room behind them. assert_eq!( group_of(name), Some(Group::Configure), @@ -633,6 +638,72 @@ fn the_memory_commands_are_configure_commands_with_working_palette_rows() { } } +// --- 0.19.0: `/skills`, and the two commands that stopped being inspections ---- + +/// `/mcp` and `/provider` are `Configure` commands, and `/skills` is the +/// `Inspect` command that took their place. +/// +/// Two claims, and the first is the one with a decision behind it. Both of those +/// commands add, edit, disable and remove entries in the configuration file, so +/// filing them under a group whose documentation says "none of it changes what a +/// turn does" was a sentence the surface did not keep. The move is asserted by +/// name rather than by counting, because the count is the *consequence* — a later +/// release that grew `Inspect` back to ten must not be able to quietly file a +/// writer under it again to balance the tables. +/// +/// The sabotage arm for the second claim is listing `/skills` without wiring its +/// row: `palette_pick` is what the driver resolves a chosen row through, so a row +/// that comes back as anything but the command whole is advertised and inert. +#[test] +fn the_writers_are_configure_commands_and_skills_is_the_inspection() { + use io_cli::commands::{group_of, palette, palette_pick, Chosen, Group, GROUPS}; + use io_harness::{Skills, Templates}; + + for name in ["/mcp", "/provider"] { + assert_eq!( + group_of(name), + Some(Group::Configure), + "{name} adds, edits, disables and removes entries in the file the \ + operator keeps, so it belongs with the group that writes", + ); + } + + assert!( + COMMANDS.iter().any(|(command, _)| *command == "/skills"), + "a surface nobody is told about is a surface nobody uses", + ); + assert_eq!( + group_of("/skills"), + Some(Group::Inspect), + "`/skills` opens a list and writes nothing, which is what Inspect means", + ); + + // The room the move made is real, not an accounting trick: Inspect is under + // the bound with a command to spare. `f13_no_group_is_longer_than_ten` is + // still the gate; this only says the release did not spend every seat it + // freed. + let inspect = GROUPS + .iter() + .find(|(group, _)| *group == Group::Inspect) + .map(|(_, names)| names.len()) + .expect("Inspect is a group with commands in it"); + assert!( + inspect < 10, + "Inspect holds {inspect}; the point of the move was to leave room", + ); + + let rows = palette(&Templates::none(), &Skills::none()); + let index = rows + .iter() + .position(|row| row.label == "skills") + .expect("`/skills` has no palette row"); + assert_eq!( + palette_pick(&Templates::none(), &Skills::none(), index), + Some(Chosen::Command("/skills")), + "`/skills`'s row is advertised and inert", + ); +} + /// `Forgotten::Refused` is reported as a refusal that names why, never as a /// removal. /// From f415f50897b61104ece88b388953a6ea80c879bc Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Thu, 27 Aug 2026 00:56:48 +0530 Subject: [PATCH 03/10] feat(skills): ship five io-* skills and the installer that guards them The five bodies ride include_str! and are written into /skills on start. Three guards, and each exists because Skills::discover fails a whole run rather than degrading: - a name the operator's own skills already resolve to is never written over, because two skills answering to one name is Error::Config propagated at run start and every turn of that session dies; - the 64-skill ceiling is counted before the write, because discover rejects the whole set rather than trimming it; - refresh is decided against a manifest io-cli writes, never against this release's own text, so a skill unchanged between two releases does not read as edited and stop being refreshed forever. The manifest is FNV-1a written here: no dependency is available for it, and no clock is either. It lives in the home rather than in skills/, because every .md in that directory is offered to the model. --- skills/io-mcp.md | 62 ++++ skills/io-permissions.md | 63 +++++ skills/io-provider.md | 66 +++++ skills/io-remember.md | 54 ++++ skills/io-update.md | 65 +++++ src/skills.rs | 431 ++++++++++++++++++++++++++++ tests/skills.rs | 591 +++++++++++++++++++++++++++++++++++++++ 7 files changed, 1332 insertions(+) create mode 100644 skills/io-mcp.md create mode 100644 skills/io-permissions.md create mode 100644 skills/io-provider.md create mode 100644 skills/io-remember.md create mode 100644 skills/io-update.md create mode 100644 src/skills.rs create mode 100644 tests/skills.rs diff --git a/skills/io-mcp.md b/skills/io-mcp.md new file mode 100644 index 0000000..ee866d9 --- /dev/null +++ b/skills/io-mcp.md @@ -0,0 +1,62 @@ +--- +name: io-mcp +description: Add, change or remove an MCP server for io by proposing an edit to the [[mcp]] array in io.toml, in the scope that suits it. +--- + +Use this when the operator wants a tool server: "add the GitHub MCP server", +"point io at our internal docs server", "drop the one that keeps failing", +"why is the issues server not answering". + +## What one looks like + +`[[mcp]]` is an array of tables. A stdio server is a program io launches and +talks to over a pipe; an HTTP server is a URL it dials. + +```toml +[[mcp]] +id = "docs" +transport = "stdio" +command = "mcp-server-docs" +args = [] + +[[mcp]] +id = "issues" +transport = "http" +url = "https://mcp.example.com/v1" +``` + +`id` is how everything else refers to the server, so it has to be unique. There +is a second array, `[[app.io-cli.mcp]]`, which is merged with the top-level one +and wins a collision of ids — use it only when the operator wants a server for +the terminal session and not for `io exec`, and say so if you do. + +## Which file + +The user file `~/.io-cli/io.toml` for a server that belongs to this machine; the +project's `io.toml` for one the whole team should have. **A credential never goes +into a committed file**: write `${env:GITHUB_TOKEN}` and let the environment +carry the secret. io-harness substitutes `${env:…}` and `${file:…}` and nothing +else. + +## The surface + +`/mcp` shows what is configured, which servers this session actually reached, how +many distinct tools each answered with, and the last failure. A server the +session has not tried yet says so rather than showing as broken — worth checking +before proposing to remove one. The same surface adds, edits and removes entries, +so the operator may prefer to be sent there. + +A server's tools reach a turn through io-harness. Its network traffic is the +server's own and is not what the `net` rule in `[policy.defaults]` governs, so do +not tell the operator that a policy change will contain it. + +## What to do + +Name the server, the transport, the id, and the file you would put it in. Then +either send the operator to `/mcp`, or propose the edit — writing `io.toml` is an +ordinary write, shown as a diff and gated by the session's policy, and it is the +operator who approves it. + +The server is not configured because you described it. Say what you are +proposing; do not say it is connected, and do not say it works until `/mcp` shows +it answering. diff --git a/skills/io-permissions.md b/skills/io-permissions.md new file mode 100644 index 0000000..406e018 --- /dev/null +++ b/skills/io-permissions.md @@ -0,0 +1,63 @@ +--- +name: io-permissions +description: Change what io asks about before it acts, by proposing an edit to the policy defaults in io.toml in the scope the operator means. +--- + +Use this when the operator asks for the approval posture to change: "stop asking +me before every write in this repository", "always ask before you run anything", +"let it read whatever it likes here", "I want to approve network calls". + +## What decides it + +`[policy.defaults]` in `io.toml`, four keys and no others: + +```toml +[policy.defaults] +read = "allow" +write = "allow" +exec = "allow" +net = "deny" +``` + +Each is `allow`, `ask` or `deny`. `read` and `write` are separate acts — there is +no single `fs` key, and inventing one is a file io-harness refuses to parse. + +Narrower rules go in `[[policy.layers]]`, which are evaluated after the defaults +and may add capability but may never re-allow something an earlier layer denied. +A request like "never touch `.env`" is a layer, not a default: + +```toml +[[policy.layers]] +name = "no-secrets" +rules = [{ act = "read", effect = "deny", pattern = ".env" }] +``` + +## Which file, which is half the answer + +Three files, and the later one wins: `~/.io-cli/io.toml` (this operator, every +project on this machine), the project's own `io.toml` (committed, everyone who +clones it), and `io.local.toml` beside it (this checkout, not committed). + +"In this repository" means the project file or the local one, never the user +file. io-harness **refuses** a project-scoped change that widens the boundary, +in its own words, and accepts the same value in `io.local.toml` — the rule is +about which file, not which value. So an uncommitted widening belongs in +`io.local.toml` and there is no argument to have about it. + +## The surface + +`/config` lists every key with the value in force and the file that decided it. +`/config policy.defaults.write allow` asks which of the three files to write, and +only that choice writes. `Shift+Tab` cycles the posture for the session alone and +writes nothing, which is the right answer to "just for now". + +## What to do + +Say which key, which value and which file, and why that file rather than another. +Then either give the operator the exact `/config` line to type, or make the edit +yourself — an edit to `io.toml` is an ordinary write, shown as a diff and gated +by the same policy as any other write, and the operator approves it or does not. + +Do not report the boundary as moved. Until the write is approved and the next +turn begins, the file on disk still says what it said before, and a session that +claims otherwise is a session the operator cannot trust about anything else. diff --git a/skills/io-provider.md b/skills/io-provider.md new file mode 100644 index 0000000..0e77fb4 --- /dev/null +++ b/skills/io-provider.md @@ -0,0 +1,66 @@ +--- +name: io-provider +description: Switch io to another provider or a local model by proposing an edit to the [[provider]] array, whose order is the order a turn tries them. +--- + +Use this when the operator wants a different model behind the session: "point +this at a local model instead", "use my own OpenAI key", "fall back to something +cheaper when the first one is rate limited", "try Ollama". + +## The array is the fallback chain + +`[[provider]]` is an array of tables and **its order is the order a turn tries +them**. The first entry is what runs; the ones after it are what a turn falls +back to when an earlier one fails. Reordering is therefore a real change, not +tidying, and adding an entry at the end changes nothing until the first one +fails. + +```toml +[[provider]] +kind = "openrouter" +model = "anthropic/claude-sonnet-4" +api_key = "${env:OPENROUTER_API_KEY}" +``` + +Any OpenAI-shaped endpoint — a proxy, a gateway, a local runtime — is the +`compatible` kind, with **exactly one** of `preset` and `base_url`: + +```toml +[[provider]] +kind = "compatible" +base_url = "http://localhost:11434/v1" +model = "llama3.2" +auth = "none" +``` + +A local runtime is that shape: its own `base_url`, the model name it serves, and +`auth = "none"` because there is no key to send. The endpoint has to be one this +machine can actually reach, and nothing here starts it. + +## Credentials + +`api_key = "${env:…}"` rather than the key itself, always in a project file and +preferably everywhere. io-harness substitutes `${env:…}` and `${file:…}` and +nothing else, and an unset variable is a hard parse error rather than a silent +empty string — so name a variable the operator has, or say which one they need to +export. + +## The surface + +`/provider` shows the chain as what it is: the order a turn tries them. It adds +entries, removes them, and promotes or demotes one to reorder the chain. It also +offers the presets io-harness reaches through the one `compatible` provider, by +name, with the endpoint each resolves to — worth looking at before writing a +`base_url` by hand. + +## What to do + +Say which entry you would add, change or move, where in the order it would sit, +and what that means for which provider actually runs. Then either send the +operator to `/provider`, or propose the edit to `io.toml` — an ordinary write, +shown as a diff, gated by the session's policy, and approved by the operator or +not. + +Do not say the session has switched. The provider in force is the one the +configuration named when the turn started; a change reaches the next turn, and +only after the operator has approved the write. diff --git a/skills/io-remember.md b/skills/io-remember.md new file mode 100644 index 0000000..8f4d787 --- /dev/null +++ b/skills/io-remember.md @@ -0,0 +1,54 @@ +--- +name: io-remember +description: Write something down where the next session reads it, choosing between AGENTS.md, AGENTS.local.md and ~/.io-cli/IO.md by who else sees it. +--- + +Use this when the operator wants a thing to stick: "remember that we use pnpm +here", "always run the formatter before you commit", "stop suggesting that +crate", "note that the staging database is read-only". + +## Three files, and the difference is the audience + +| File | Who reads it | +| --- | --- | +| `AGENTS.md` | Everyone who clones this repository. It is committed. | +| `AGENTS.local.md` | This checkout alone. It is not committed and nobody else sees it. | +| `~/.io-cli/IO.md` | This operator, in every project on this machine. | + +That is the whole decision, and getting it wrong is how a private note about a +colleague ends up in a pull request. A rule about the project's code belongs in +`AGENTS.md`. A rule about this machine, this branch or this person's habits +belongs in one of the other two. If the operator has not said which and the line +could embarrass them, ask before proposing the committed one. + +## What actually gets read back + +io-harness reads `AGENTS.md` with no configuration at all. The other two are read +only where `[instructions] files` in `io.toml` names them, and that list +**replaces** the default rather than adding to it — so a list that names +`AGENTS.local.md` and `IO.md` and stops there silently stops the repository's own +`AGENTS.md` being read. `/remember` writes the complete list; do not hand-edit +`[instructions] files` to add one name. + +`/memory` shows which of the three exist, how many lines each holds, and which +ones io-harness is really reading — the answer to "I wrote it down and it did not +work". + +## The surface + +`/remember` asks which of the three scopes at the moment it writes, appends the +line as a markdown bullet, and creates the file with a header saying who else +reads it if it was not there. It appends: every byte already in the file stays +where it is. + +## What to do + +Propose the line in the words it should be written in — short, specific, and +useful to a reader who has none of this conversation — and name the scope you +would put it in and why. Then either send the operator to `/remember`, or append +it yourself: writing one of these files is an ordinary write, shown as a diff and +gated by the session's policy. + +Do not say it has been remembered. Nothing is written until the operator approves +the write, and a session that reports a note it never made is one whose next +session behaves as though the operator never typed it. diff --git a/skills/io-update.md b/skills/io-update.md new file mode 100644 index 0000000..5d97b69 --- /dev/null +++ b/skills/io-update.md @@ -0,0 +1,65 @@ +--- +name: io-update +description: Find out whether a newer io has been released and propose the exact install.sh command for the operator to approve, never running an update behind them. +--- + +Use this when the operator asks about the version: "update io", "am I on the +latest?", "is there a newer version", "how do I upgrade this". + +## What is running now + +`io --version` says it, and so does the splash at the top of the session. Use +what the session already told you before proposing a command that finds out. + +## What has been published + +The newest release is whatever +`https://github.com/initorigin/io-cli/releases/latest` redirects to — the tag in +that redirect's URL is the version, which is why it can be read without a token +and without parsing JSON. + +Reaching it is a network act and `net` is `deny` by default in +`[policy.defaults]`, so it may be refused. **A refusal is an answer, not an +obstacle**: say the check could not be made from inside this session and give the +operator the release page to look at, or the `io-permissions` skill if they want +to widen the rule. Do not work around a refusal. + +## The command to propose + +`install.sh` in this repository is the installer, and it is the only installer. +It resolves the target for this machine, downloads the artifact and the +`SHA256SUMS` beside it, **verifies the checksum before unpacking**, and moves the +binary into a directory the operator already owns — no `sudo`, nothing written +outside their own directories, nothing left behind if any step fails. + +```sh +curl -fsSL https://raw.githubusercontent.com/initorigin/io-cli/main/install.sh | sh +``` + +Three environment variables change what it does, and nothing else does: + +- `IO_VERSION` — install this version instead of the latest, e.g. `IO_VERSION=0.18.0`. +- `IO_INSTALL_DIR` — install here instead of `~/.local/bin`. +- `IO_BASE_URL` — download from here instead of the GitHub Release. + +So a pinned install into a chosen directory is one line: + +```sh +IO_VERSION=0.19.0 IO_INSTALL_DIR="$HOME/bin" sh -c 'curl -fsSL https://raw.githubusercontent.com/initorigin/io-cli/main/install.sh | sh' +``` + +**Do not re-implement any of that.** Downloading the tarball yourself, unpacking +it, computing a checksum by hand or moving a binary into place is a second +installer with none of the guarantees of the first, written in a session nobody +will read back. Propose the script. + +## What to do + +Say what is running, say what has been published if you could find out, and then +propose the command. Running it is a gated command: the operator sees the exact +argv before anything runs and approves it or does not. + +Then stop. **Do not say io has been updated.** The binary in flight is the one +this process started with; a new one takes effect when the operator starts a new +session, and until they have approved the command and seen it finish, nothing has +changed at all. diff --git a/src/skills.rs b/src/skills.rs new file mode 100644 index 0000000..75d7bd1 --- /dev/null +++ b/src/skills.rs @@ -0,0 +1,431 @@ +//! The five skills io-cli ships, and how they get onto disk without breaking a +//! directory the operator already owns. +//! +//! A skill is a markdown file with two lines of frontmatter. io-harness discovers +//! every `*.md` in the skills directory, puts each one's `name` and `description` +//! into the system prompt on every turn, and lets the model read the body through +//! its own `read_skill` tool. That is the whole mechanism — there is no registry, +//! no index and no format of io-cli's own — so shipping a skill is writing a file +//! into `~/.io-cli/skills` and nothing else. +//! +//! Writing five files is not, however, three lines of [`std::fs::write`], and the +//! reason is that `Skills::discover` **rejects a set rather than repairing one**. +//! Two files resolving to one name is `Error::Config`, propagated with `?` at run +//! start by every io-harness entry point, which is every turn of that session +//! dead before the first completion. More than [`io_harness::skills::MAX_SKILLS`] in one +//! directory is the same error, and it rejects the whole set rather than the +//! excess — so an operator sitting at 62 skills who upgrades into five more would +//! get a dead session as their upgrade experience. The harness's own anti-collision +//! device, `Skills::namespaced` + `Skills::merged`, is `pub(crate)`, and +//! `TaskContract::with_skills` takes one directory and assigns it. io-cli cannot +//! reach any of it, so the guards are here or nowhere. +//! +//! Hence three rules, and every one of them is a thing this module refuses to do: +//! +//! 1. **It never takes a name the operator already claims.** The directory is read +//! first, through the same [`io_harness::Skills::discover`] the run will use — +//! not through io-cli's own idea of what a skill file is, which could disagree +//! with the oracle in exactly the case that matters. +//! 2. **It counts the ceiling before it writes**, and stops short of it, saying how +//! many it withheld. +//! 3. **It never resurrects a disabled skill.** `skills/disabled/io-mcp.md` is the +//! operator having turned that one off; writing a fresh copy beside it on the +//! next launch would make disabling a thing that undoes itself every time the +//! product starts. `disabled/` is invisible to discovery for free, because the +//! walk admits a subdirectory only when it holds a `SKILL.md`. +//! +//! # Refresh, and why it is decided from a manifest +//! +//! An upgrade should bring a shipped skill forward where nobody has touched it and +//! leave an edited one exactly as it is. The only honest question is **are these +//! the bytes io-cli last wrote**, and answering it needs a record of what was +//! written — [`MANIFEST`], one line per skill, in the home and deliberately *not* +//! in the skills directory, because discovery admits every `.md` in there and a +//! state file offered to the model as a skill called `manifest` is a worse bug +//! than the one it would be solving. +//! +//! Two roads not taken, both closed by a gate rather than by taste. Comparing the +//! bytes on disk against **this release's** shipped text instead would fail on the +//! second upgrade, not the first: every skill whose text did not change between +//! two releases would read as edited and stop being refreshed forever. And a +//! timestamp is unavailable — `tests/timing.rs` bans `Instant::now`, +//! `SystemTime::now` and `.elapsed()` in every file under `src/` but the driver, +//! so freshness here is decided by bytes and never by a clock, which is also the +//! only answer that survives a checkout whose mtimes are all identical. +//! +//! The digest is FNV-1a, six lines, written below. `tests/dependencies.rs` pins +//! the direct dependency set in both directions, so `sha2` and `blake3` are gate +//! failures rather than choices, and nothing here is a security boundary: the +//! question is whether a file the operator may have opened is byte-identical to +//! one io-cli wrote, and an adversary who can write into this directory has +//! already won by writing a skill of their own. +//! +//! # Nothing here ever fails a run +//! +//! [`install`] returns report lines, never an error. A read-only skills directory +//! is an operator with no shipped skills, which is the product they had before this +//! release; a session that refuses to start because of a directory they have never +//! heard of is not. + +use std::path::{Path, PathBuf}; + +/// The skills directory, under io-cli's home. The same name +/// [`crate::home::adopt`] creates with the home and +/// [`crate::contract::skills_dir`] resolves to when no key names another. +const DIR: &str = "skills"; + +/// The subdirectory a disabled skill is moved into. +/// +/// Invisible to `Skills::discover` for nothing: the walk admits a subdirectory +/// only when it contains a `SKILL.md` and otherwise skips it, so a folder of +/// loose `.md` files is not a skill, not in the catalogue, and not readable +/// through `read_skill`. That is why disabling is a rename and not a list. +pub const DISABLED: &str = "disabled"; + +/// What io-cli last wrote, one line per skill: `namehex-hash`. +/// +/// **In the home, not in the skills directory.** Every `*.md` in `skills/` is +/// offered to the model, so a manifest kept there would be a skill whose +/// description is a hash table. The leading dot is courtesy on top of that. +/// +/// Plain text and not TOML on purpose: `tests/dependencies.rs` permits +/// `toml::from_str` in `src/edit.rs` alone, because a second module that parses a +/// configuration file is a second opinion about what one means. Two fields keyed +/// by a name is not a format that needs a parser. +pub const MANIFEST: &str = ".skills-manifest"; + +/// One skill io-cli ships: the name it resolves to, and its whole text. +/// +/// The text is `include_str!`, so the five files are in the binary and there is +/// no install-time source to be missing, no path to resolve and nothing to +/// download. The name is stated here rather than derived from the file, because +/// the file's own `name:` frontmatter is what discovery will resolve and the two +/// must agree — `tests/skills.rs` asserts they do. +pub struct Shipped { + /// The resolved name: what the model addresses the skill by, what the + /// installed file is named after, and the manifest's key. + pub name: &'static str, + /// The file, frontmatter and all. + pub text: &'static str, +} + +/// The five, in name order, which is the order `Skills::discover` sorts into and +/// therefore the order a report reads in. +pub const SHIPPED: [Shipped; 5] = [ + Shipped { + name: "io-mcp", + text: include_str!("../skills/io-mcp.md"), + }, + Shipped { + name: "io-permissions", + text: include_str!("../skills/io-permissions.md"), + }, + Shipped { + name: "io-provider", + text: include_str!("../skills/io-provider.md"), + }, + Shipped { + name: "io-remember", + text: include_str!("../skills/io-remember.md"), + }, + Shipped { + name: "io-update", + text: include_str!("../skills/io-update.md"), + }, +]; + +/// `/skills`. +#[must_use] +pub fn dir(home: &Path) -> PathBuf { + home.join(DIR) +} + +/// `/skills/disabled`, whether or not it is there. +/// +/// Not created here. It is made by the surface that first moves a file into it, +/// because an empty `disabled/` is a directory the operator has to wonder about. +#[must_use] +pub fn disabled_dir(home: &Path) -> PathBuf { + dir(home).join(DISABLED) +} + +/// `/.skills-manifest`. +#[must_use] +pub fn manifest_path(home: &Path) -> PathBuf { + home.join(MANIFEST) +} + +/// The file a shipped skill is installed as, enabled or disabled. +fn file_name(name: &str) -> String { + format!("{name}.md") +} + +/// FNV-1a, 64-bit, over the file's bytes. +/// +/// Not a security primitive and not asked to be one — see the module note. It is +/// here rather than in a crate because the dependency set is pinned in both +/// directions by a gate, and because six lines with no state is smaller than the +/// sentence justifying a dependency for it. +#[must_use] +pub fn digest(bytes: &[u8]) -> u64 { + let mut hash: u64 = 0xcbf2_9ce4_8422_2325; + for byte in bytes { + hash ^= u64::from(*byte); + hash = hash.wrapping_mul(0x0000_0100_0000_01b3); + } + hash +} + +/// What the manifest says io-cli last wrote, as `(name, hash)` pairs. +/// +/// **A manifest that is absent, unreadable or malformed reads as empty**, and the +/// direction of that failure is the whole point: an empty manifest means every +/// file already on disk is treated as the operator's and left alone. Degrading +/// the other way — "no record, so it must be ours" — would let a corrupt state +/// file authorise overwriting somebody's edits. +/// +/// A `Vec` and a linear scan over five entries. An index here would be a data +/// structure nobody could justify at the next release. +fn recorded(home: &Path) -> Vec<(String, u64)> { + let Ok(text) = std::fs::read_to_string(manifest_path(home)) else { + return Vec::new(); + }; + text.lines() + .filter_map(|line| { + let (name, hash) = line.split_once('\t')?; + let hash = u64::from_str_radix(hash.trim(), 16).ok()?; + Some((name.to_string(), hash)) + }) + .collect() +} + +/// The hash recorded for one name, if there is one. +fn recorded_hash(recorded: &[(String, u64)], name: &str) -> Option { + recorded + .iter() + .find(|(recorded, _)| recorded == name) + .map(|(_, hash)| *hash) +} + +/// Record what was just written, replacing any earlier line for that name. +fn record(recorded: &mut Vec<(String, u64)>, name: &str, hash: u64) { + match recorded.iter_mut().find(|(had, _)| had == name) { + Some(entry) => entry.1 = hash, + None => recorded.push((name.to_string(), hash)), + } +} + +/// Write the manifest out. +/// +/// A plain write rather than the staged rename [`crate::configure`] uses for +/// `io.toml`, because the failure modes are not comparable: a truncated manifest +/// degrades to "every file is the operator's", which costs a refresh and destroys +/// nothing, while a truncated configuration is a session that will not start. +fn write_manifest(home: &Path, recorded: &[(String, u64)]) -> std::io::Result<()> { + let mut text = String::with_capacity(recorded.len() * 32); + for (name, hash) in recorded { + text.push_str(&format!("{name}\t{hash:016x}\n")); + } + std::fs::write(manifest_path(home), text) +} + +/// Whether the file at `path` is one io-cli wrote and nobody has edited since. +/// +/// The provenance question `/skills` has to answer, and it is answered from the +/// manifest rather than from the `io-` prefix. That difference is the surface's +/// entire job: an operator who writes their own `io-thing.md` is told it is +/// theirs, and an operator who has edited a shipped skill is told the same — +/// which is true, and is also exactly what [`install`] will do with it. +#[must_use] +pub fn wrote(home: &Path, name: &str, path: &Path) -> bool { + let Some(hash) = recorded_hash(&recorded(home), name) else { + return false; + }; + std::fs::read(path).is_ok_and(|bytes| digest(&bytes) == hash) +} + +/// `n skills`, or `1 skill`. +fn many(n: usize) -> String { + if n == 1 { + "1 skill".to_string() + } else { + format!("{n} skills") + } +} + +/// Put the five shipped skills in `/skills`, and say what happened. +/// +/// Call it on every run, before the contract is built: a skill written after the +/// contract is a skill the first session of a new install is not offered, which is +/// exactly the session in which an operator is most likely to ask for help. +/// +/// **Never returns an error, and a run that changed nothing returns no lines.** +/// Every failure — a directory that will not create, a file that will not write, a +/// directory that will not discover — becomes a line in the report and the session +/// carries on with whatever installed. The lines are what `main` already owns and +/// drains into the scrollback, the same `Vec` shape +/// [`crate::home::Report::lines`] produces. +/// +/// One skill gets one of six outcomes, decided in this order: +/// +/// | If | Then | +/// | --- | --- | +/// | it is in `skills/disabled/` | nothing at all, silently: the operator turned it off | +/// | no file of ours, and another file resolves to the name | withheld, claimant named | +/// | no file, and the directory is at [`io_harness::skills::MAX_SKILLS`] | withheld, and counted | +/// | no file | written, and the manifest records its hash | +/// | a file whose bytes match the manifest | replaced with the new text, hash updated | +/// | a file whose bytes do not, or with no entry at all | left byte for byte, named as kept | +pub fn install(home: &Path) -> Vec { + let dir = dir(home); + // The same `0700` [`crate::home::adopt`] gives the home, and a no-op on the + // directory it already made — an existing directory's mode is not touched. + if let Err(error) = crate::home::create(&dir) { + return vec![format!("could not create {}: {error}", dir.display())]; + } + + // **The oracle is the harness's own walk, not a `read_dir` of io-cli's.** A + // resolved name comes from frontmatter where there is one, so a file called + // anything at all can claim `io-mcp`, and a second opinion about what this + // directory holds would disagree with the run in precisely the case the + // collision guard exists for. A directory that will not discover is already + // broken; io-cli reports the harness's own sentence and adds nothing to it, + // because a write into a set that is already ambiguous can only make the + // session harder to fix. + let found = match io_harness::Skills::discover(&dir) { + Ok(found) => found, + Err(error) => { + return vec![format!( + "{error}; io-cli installed none of its own skills into it" + )]; + } + }; + + let disabled = disabled_dir(home); + let mut manifest = recorded(home); + // What the directory holds now, kept in step with every write below so the + // ceiling is counted against the set as it grows rather than as it was. + let mut held = found.len(); + let mut installed = 0usize; + let mut updated = 0usize; + let mut withheld = 0usize; + let mut touched_manifest = false; + // Lines about one named file, kept apart from the summary lines so the report + // reads counts first and exceptions after. + let mut notes: Vec = Vec::new(); + + for skill in &SHIPPED { + if disabled.join(file_name(skill.name)).exists() { + continue; + } + + let target = dir.join(file_name(skill.name)); + let existing = match std::fs::read(&target) { + Ok(bytes) => Some(bytes), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => None, + // There is a file there and this process cannot read it. It is not + // io-cli's to overwrite on the strength of not knowing what it says. + Err(error) => { + notes.push(format!("kept {}: {error}", target.display())); + continue; + } + }; + + let Some(bytes) = existing else { + // Nothing of ours at that path — but the *name* may still be taken, + // and taking it anyway is `Error::Config` at run start rather than a + // listing quirk. `target` does not exist, so anything discovery + // resolved to this name is necessarily a different file. + if let Some(claimant) = found.get(skill.name) { + notes.push(format!( + "kept your {}, which is already named `{}`; io-cli did not install its own", + claimant.path.display(), + skill.name, + )); + continue; + } + // Counted before the write, not discovered after it. `discover` + // rejects the whole set above the ceiling rather than truncating it, + // so one file too many costs the operator every skill they had. + if held >= io_harness::skills::MAX_SKILLS { + withheld += 1; + continue; + } + if let Err(error) = std::fs::write(&target, skill.text) { + notes.push(format!("could not write {}: {error}", target.display())); + continue; + } + record(&mut manifest, skill.name, digest(skill.text.as_bytes())); + touched_manifest = true; + held += 1; + installed += 1; + continue; + }; + + // **Against the manifest, never against the shipped text.** See the + // module note: comparing with what this release ships reads every skill + // unchanged between two releases as edited, and stops refreshing it + // forever. No entry at all means io-cli has no record of writing this + // file, which is the operator's file however it is spelled. + if recorded_hash(&manifest, skill.name) != Some(digest(&bytes)) { + notes.push(format!( + "kept {}, which has been edited; io-cli's own `{}` was not written over it", + target.display(), + skill.name, + )); + continue; + } + // Untouched and already the current text: no write, so no churned mtime + // for a backup to notice and no line about a thing that did not happen. + if bytes == skill.text.as_bytes() { + continue; + } + if let Err(error) = std::fs::write(&target, skill.text) { + notes.push(format!("could not write {}: {error}", target.display())); + continue; + } + record(&mut manifest, skill.name, digest(skill.text.as_bytes())); + touched_manifest = true; + updated += 1; + } + + let mut report = Vec::with_capacity(notes.len() + 3); + if installed > 0 { + report.push(format!( + "installed {} into {}", + many(installed), + dir.display() + )); + } + if updated > 0 { + report.push(format!( + "brought {} in {} up to date", + many(updated), + dir.display() + )); + } + if withheld > 0 { + report.push(format!( + "withheld {}: {} already holds {} of the {} io-harness allows in one directory", + many(withheld), + dir.display(), + held, + io_harness::skills::MAX_SKILLS, + )); + } + report.extend(notes); + + if touched_manifest { + if let Err(error) = write_manifest(home, &manifest) { + // The skills are on disk and the session is fine; what is lost is the + // record that io-cli wrote them, so the next upgrade will treat them + // as the operator's and refuse to refresh them. Worth a line. + report.push(format!( + "could not write {}: {error}", + manifest_path(home).display() + )); + } + } + + report +} diff --git a/tests/skills.rs b/tests/skills.rs new file mode 100644 index 0000000..6988557 --- /dev/null +++ b/tests/skills.rs @@ -0,0 +1,591 @@ +//! The shipped skills and the installer that puts them on disk. +//! +//! Every assertion about what a skills directory *holds* goes through +//! [`io_harness::Skills::discover`] rather than through a `read_dir` of this +//! file's own. That is not fastidiousness: the resolved name of a skill comes +//! from its frontmatter where there is one, so a file called anything at all can +//! answer to `io-mcp`, and a test that counted `io-*.md` files would agree with +//! io-cli and disagree with the run. The harness's walk is the only oracle whose +//! verdict is the one every turn of the session will get. +//! +//! Nothing here touches the environment. [`skills::install`] takes the home as an +//! argument, so each test gets a temporary directory of its own and they are free +//! to run in parallel — unlike `tests/memory.rs`, whose subject reads +//! `IO_CONFIG_HOME` at call time and therefore has to serialise. + +use std::path::{Path, PathBuf}; + +use io_cli::skills; + +/// A home with nothing in it. `install` makes the directories it needs. +fn home() -> (tempfile::TempDir, PathBuf) { + let dir = tempfile::tempdir().expect("a temporary directory"); + let home = dir.path().join("home"); + (dir, home) +} + +fn read(path: &Path) -> String { + std::fs::read_to_string(path).unwrap_or_else(|error| panic!("{}: {error}", path.display())) +} + +/// The names io-harness resolves out of a directory, sorted as it sorts them. +/// +/// Panics on a directory that will not discover, which is the point: a criterion +/// that says "and the session still runs" is asserting exactly that this call +/// returns `Ok`, and an `unwrap_or_default` here would turn the session-killer +/// into a green test with an empty list. +fn discovered(dir: &Path) -> Vec { + io_harness::Skills::discover(dir) + .unwrap_or_else(|error| { + panic!( + "{} does not discover, so every turn of that session would fail at run start: \ + {error}", + dir.display() + ) + }) + .iter() + .map(|skill| skill.name.clone()) + .collect() +} + +/// The five shipped names, in the order `SHIPPED` declares them. +fn shipped_names() -> Vec { + skills::SHIPPED + .iter() + .map(|skill| skill.name.to_string()) + .collect() +} + +/// The manifest as pairs, parsed the way the module documents its format. +/// +/// Written out here rather than read through a helper, so that a release which +/// quietly changed the format — to TOML, to JSON, to the skills directory — turns +/// this red instead of passing through an accessor that changed with it. +fn manifest(home: &Path) -> Vec<(String, u64)> { + let path = home.join(".skills-manifest"); + let Ok(text) = std::fs::read_to_string(&path) else { + return Vec::new(); + }; + text.lines() + .map(|line| { + let (name, hash) = line + .split_once('\t') + .unwrap_or_else(|| panic!("`{line}` is not `namehex-hash`")); + ( + name.to_string(), + u64::from_str_radix(hash, 16) + .unwrap_or_else(|error| panic!("`{hash}` is not hex: {error}")), + ) + }) + .collect() +} + +fn hash_of(manifest: &[(String, u64)], name: &str) -> Option { + manifest + .iter() + .find(|(had, _)| had == name) + .map(|(_, hash)| *hash) +} + +/// Put `count` skills of the operator's own in the directory, none of them named +/// anything io-cli ships. +fn operator_skills(dir: &Path, count: usize) { + std::fs::create_dir_all(dir).expect("the skills directory"); + for index in 0..count { + std::fs::write( + dir.join(format!("mine-{index:02}.md")), + format!("A skill the operator wrote, number {index}.\n"), + ) + .expect("an operator skill"); + } +} + +// --------------------------------------------------------------------------- +// The five files themselves +// --------------------------------------------------------------------------- + +/// Each shipped file declares the name the installer files it under. +/// +/// The two are stated in different places — `name:` in the markdown, `name` in +/// `SHIPPED` — and if they disagree the installed file is `io-mcp.md` answering +/// to something else, which makes every collision check in this module look at +/// the wrong name. +#[test] +fn every_shipped_file_declares_the_name_it_is_installed_under() { + for skill in &skills::SHIPPED { + let declared = skill + .text + .lines() + .find_map(|line| line.strip_prefix("name:")) + .map(str::trim) + .unwrap_or_else(|| panic!("{} has no `name:` in its frontmatter", skill.name)); + assert_eq!( + declared, skill.name, + "the file's frontmatter and SHIPPED disagree about what this skill is called", + ); + assert!( + skill.text.starts_with("---\n"), + "{}'s frontmatter is not a fence io-harness will read; without one the name \ + falls back to the file stem and the description to the first prose line", + skill.name, + ); + } +} + +/// A description is one line, and a short one. +/// +/// Every skill's description is in the system prompt on **every turn**, and +/// io-harness clamps at 240 characters — so an essay here is an essay paid for +/// per turn, silently truncated where it matters most. +#[test] +fn every_description_is_one_short_sentence() { + for skill in &skills::SHIPPED { + let description = skill + .text + .lines() + .find_map(|line| line.strip_prefix("description:")) + .map(str::trim) + .unwrap_or_else(|| panic!("{} has no `description:`", skill.name)); + + assert!( + !description.is_empty(), + "{}'s description is empty, so the catalogue says nothing about it", + skill.name, + ); + assert!( + description.len() < 200, + "{}'s description is {} characters; it is sent on every turn and io-harness \ + clamps it at 240", + skill.name, + description.len(), + ); + } +} + +/// **F8, the half a test can hold.** Every skill tells the model to *propose*. +/// +/// The criterion is about prose and is finally judged by reading it, but the +/// sabotage it names — a skill that tells the model to report the setting as +/// changed — starts by deleting the instruction to propose. This catches that. +#[test] +fn every_shipped_skill_ends_in_a_proposal_rather_than_a_claim() { + for skill in &skills::SHIPPED { + assert!( + skill.text.contains("propos"), + "{} never tells the model to propose anything, so nothing in it ends at a \ + change the operator approves", + skill.name, + ); + } +} + +// --------------------------------------------------------------------------- +// The install +// --------------------------------------------------------------------------- + +/// A home with nothing in it gets all five, and io-harness sees all five. +#[test] +fn a_fresh_home_gets_the_five_and_the_harness_discovers_them() { + let (_dir, home) = home(); + + let report = skills::install(&home); + let dir = skills::dir(&home); + + assert_eq!( + discovered(&dir), + shipped_names(), + "the directory io-harness walks does not hold exactly the five io-cli ships", + ); + for skill in &skills::SHIPPED { + assert_eq!( + read(&dir.join(format!("{}.md", skill.name))), + skill.text, + "{} on disk is not the text in the binary", + skill.name, + ); + } + assert!( + report + .iter() + .any(|line| line.contains("installed 5 skills")), + "the report does not say what it did: {report:?}", + ); +} + +/// Running it again changes nothing and says nothing. +/// +/// A line per run about a thing that did not happen is a report an operator stops +/// reading, which costs the lines that *do* matter — the withheld skill, the kept +/// edit — their only chance of being seen. +#[test] +fn a_second_install_with_nothing_to_do_writes_nothing_and_reports_nothing() { + let (_dir, home) = home(); + skills::install(&home); + + let before = manifest(&home); + let report = skills::install(&home); + + assert!(report.is_empty(), "the second run reported {report:?}"); + assert_eq!(manifest(&home), before, "the manifest was rewritten anyway"); +} + +/// **F2.** A name the operator already claims is never overwritten, and the +/// session still runs. +/// +/// The sabotage is installing unconditionally. Under it this is the only test +/// that fails, and it fails the way the field would: two files answering to +/// `io-mcp`, `Skills::discover` returning `Error::Config`, and — because +/// `TaskContract::discover_skills` propagates that at run start — every turn of +/// that session dead before the first completion, with the palette empty and +/// nothing on screen connecting it to an upgrade. +#[test] +fn f2_a_name_the_operator_already_claims_is_withheld_and_the_session_still_runs() { + let (_dir, home) = home(); + let dir = skills::dir(&home); + std::fs::create_dir_all(&dir).expect("the skills directory"); + + let mine = dir.join("mine.md"); + let theirs = "---\nname: io-mcp\ndescription: My own take on the MCP servers.\n---\n\nMine.\n"; + std::fs::write(&mine, theirs).expect("the operator's skill"); + + let report = skills::install(&home); + + assert!( + !dir.join("io-mcp.md").exists(), + "io-cli wrote io-mcp.md over a name the operator already claims", + ); + assert_eq!(read(&mine), theirs, "the operator's file was touched"); + + // Four written, not five, and the fifth name still resolves to their file. + let mut names = discovered(&dir); + names.sort(); + assert_eq!( + names, + shipped_names(), + "the resolved set is not the five names, one of which is now the operator's file", + ); + assert_eq!( + std::fs::read_dir(&dir) + .expect("the directory") + .filter(|entry| entry + .as_ref() + .is_ok_and(|entry| entry.file_name().to_string_lossy().starts_with("io-"))) + .count(), + 4, + "io-cli installed a fifth file of its own", + ); + + let said = report.join("\n"); + assert!( + said.contains("io-mcp") && said.contains("mine.md"), + "the report names neither the withheld skill nor the file that claimed it: {report:?}", + ); +} + +/// **F3.** The shared ceiling is respected before the write, not discovered after +/// it. +/// +/// The sabotage is installing all five and letting discovery complain. Under it +/// only this test fails, and it fails with the harness rejecting the **entire +/// set** rather than the excess — so an operator with a working 62-skill +/// directory has none at all after upgrading. +#[test] +fn f3_the_ceiling_is_counted_before_the_write_and_the_session_still_runs() { + let (_dir, home) = home(); + let dir = skills::dir(&home); + operator_skills(&dir, 62); + + let report = skills::install(&home); + + let names = discovered(&dir); + assert_eq!( + names.len(), + io_harness::skills::MAX_SKILLS, + "62 of the operator's plus at most two of io-cli's is the ceiling exactly", + ); + let shipped = shipped_names(); + let ours = names + .iter() + .filter(|name| shipped.iter().any(|theirs| theirs == *name)) + .count(); + assert_eq!( + ours, 2, + "io-cli added {ours} skills into a directory with two places left", + ); + + let said = report.join("\n"); + assert!( + said.contains("withheld 3 skills"), + "the report does not say how many were withheld: {report:?}", + ); + assert!( + said.contains("64"), + "the report does not say why they were withheld: {report:?}", + ); +} + +/// **F4.** An untouched skill is brought forward; a modified one is left alone; +/// one io-cli has no record of writing is the operator's. +/// +/// The three cases are set up by hand, because they are what the *second* upgrade +/// looks like and a test that only installs twice cannot reach them: a previous +/// release's text with its own hash recorded, an operator edit, and a file with +/// no manifest entry at all. +/// +/// The sabotage is comparing the bytes on disk against the **shipped** text +/// instead of against the manifest. Under it only this test fails, and it fails +/// on the second upgrade rather than the first: every skill unchanged between two +/// releases would read as modified and stop being refreshed forever. +#[test] +fn f4_an_untouched_skill_is_refreshed_and_an_edited_one_is_left_byte_for_byte() { + let (_dir, home) = home(); + let dir = skills::dir(&home); + skills::install(&home); + + // 1. A previous release's io-mcp, recorded as io-cli's own. + let older = "---\nname: io-mcp\ndescription: What the last release said.\n---\n\nOld.\n"; + let mcp = dir.join("io-mcp.md"); + std::fs::write(&mcp, older).expect("the older text"); + + // 2. An edit the operator made to io-provider, which io-cli must not touch. + let provider = dir.join("io-provider.md"); + let edited = format!("{}\n\nAnd my own note at the end.\n", read(&provider)); + std::fs::write(&provider, &edited).expect("the operator's edit"); + + // 3. An io-remember with no manifest entry at all — a file io-cli has no + // record of writing, whatever it is named, is the operator's. + let remember = dir.join("io-remember.md"); + let theirs = "---\nname: io-remember\ndescription: Mine, not io-cli's.\n---\n\nMine.\n"; + std::fs::write(&remember, theirs).expect("the operator's own io-remember"); + + // The manifest as those three edits leave it: io-mcp recorded at the older + // text's hash, io-provider still at what io-cli wrote, io-remember absent. + let recorded: Vec = manifest(&home) + .into_iter() + .filter(|(name, _)| name != "io-remember") + .map(|(name, hash)| { + let hash = if name == "io-mcp" { + skills::digest(older.as_bytes()) + } else { + hash + }; + format!("{name}\t{hash:016x}") + }) + .collect(); + std::fs::write( + home.join(".skills-manifest"), + format!("{}\n", recorded.join("\n")), + ) + .expect("the manifest"); + let before = manifest(&home); + + let report = skills::install(&home); + + // 1. Untouched: replaced with the new text, and the hash brought forward. + let shipped = skills::SHIPPED + .iter() + .find(|skill| skill.name == "io-mcp") + .expect("io-mcp is shipped"); + assert_eq!( + read(&mcp), + shipped.text, + "a file whose bytes still hash to the manifest is io-cli's and was not refreshed", + ); + assert_eq!( + hash_of(&manifest(&home), "io-mcp"), + Some(skills::digest(shipped.text.as_bytes())), + "the manifest still records the older text io-cli no longer ships", + ); + + // 2. Edited: byte for byte as it was, and its manifest entry untouched. + assert_eq!( + read(&provider), + edited, + "io-cli overwrote an edit the operator made", + ); + assert_eq!( + hash_of(&manifest(&home), "io-provider"), + hash_of(&before, "io-provider"), + "the manifest entry for a file io-cli did not write was rewritten", + ); + + // 3. Unmanifested: the operator's, whatever it is called. + assert_eq!( + read(&remember), + theirs, + "a file io-cli has no record of writing was overwritten anyway", + ); + + let said = report.join("\n"); + for kept in ["io-provider.md", "io-remember.md"] { + assert!( + said.contains(kept), + "the report does not name {kept} as kept: {report:?}", + ); + } + assert!( + !said.contains("io-mcp.md"), + "io-mcp was refreshed, not kept, and the report says otherwise: {report:?}", + ); +} + +/// **F5.** A disabled skill stays disabled across a restart and an upgrade. +/// +/// The sabotage is keying the skip on the enabled path alone — on "is there an +/// `io-provider.md` in `skills/`". Under it only this test fails, and disabling +/// becomes a thing that undoes itself on the next launch, which is worse than not +/// offering it. +#[test] +fn f5_a_disabled_skill_is_not_resurrected_by_the_next_run() { + let (_dir, home) = home(); + let dir = skills::dir(&home); + skills::install(&home); + + // Disabling is a move, which is what `/skills` does with a keystroke. + let disabled = skills::disabled_dir(&home); + std::fs::create_dir_all(&disabled).expect("the disabled directory"); + let was = dir.join("io-provider.md"); + let now = disabled.join("io-provider.md"); + let text = read(&was); + std::fs::rename(&was, &now).expect("the move"); + + // Twice: a restart, and then an upgrade that has something else to do. + skills::install(&home); + std::fs::remove_file(dir.join("io-update.md")).expect("something for the second run to do"); + skills::install(&home); + + assert!( + !was.exists(), + "io-cli wrote a fresh io-provider.md beside the one the operator disabled", + ); + assert_eq!(read(&now), text, "the disabled file was touched"); + + let mut expected: Vec = shipped_names() + .into_iter() + .filter(|name| name != "io-provider") + .collect(); + expected.sort(); + assert_eq!( + discovered(&dir), + expected, + "the catalogue io-harness composes names the skill the operator turned off", + ); +} + +/// **N3.** The manifest is not a skill and the disabled folder is not a skill. +/// +/// Asserted against the real `Skills::discover`, not against io-cli's idea of the +/// directory. The sabotage is putting the manifest in `skills/` as `manifest.md` +/// — under which only this test fails, and it fails by offering the model a skill +/// called `manifest` whose description is a hash table. +#[test] +fn n3_neither_the_manifest_nor_the_disabled_folder_is_offered_as_a_skill() { + let (_dir, home) = home(); + let dir = skills::dir(&home); + skills::install(&home); + + // A disabled skill, and a second loose file in there — the folder holds no + // `SKILL.md`, which is the whole reason discovery walks past it. + let disabled = skills::disabled_dir(&home); + std::fs::create_dir_all(&disabled).expect("the disabled directory"); + std::fs::rename(dir.join("io-update.md"), disabled.join("io-update.md")).expect("the move"); + std::fs::write(disabled.join("something-else.md"), "Turned off too.\n").expect("a second one"); + + let names = discovered(&dir); + let mut expected: Vec = shipped_names() + .into_iter() + .filter(|name| name != "io-update") + .collect(); + expected.sort(); + assert_eq!( + names, expected, + "discovery over an installed home does not return exactly the enabled set", + ); + + // The manifest exists, is in the home, and is not in the walk. + let path = skills::manifest_path(&home); + assert!(path.is_file(), "no manifest was written at all"); + assert_eq!( + path.parent(), + Some(home.as_path()), + "the manifest is inside the directory every `.md` of which is offered to the model", + ); + assert!( + !dir.join(skills::MANIFEST).exists(), + "there is a manifest in the skills directory, every `.md` of which is a skill", + ); + for name in &names { + assert!( + !name.contains("manifest") && name != "disabled", + "`{name}` is in the catalogue, and it is not a skill", + ); + } +} + +/// A directory that will not discover is reported, not added to. +/// +/// The set is already ambiguous, so every turn of that session is going to fail +/// until somebody moves a file. Writing five more into it can only make that +/// harder — and io-cli's own report is the one place the operator can be told +/// the harness's own sentence, because the palette shows them an empty list. +#[test] +fn a_directory_that_will_not_discover_is_reported_and_left_alone() { + let (_dir, home) = home(); + let dir = skills::dir(&home); + std::fs::create_dir_all(&dir).expect("the skills directory"); + for name in ["one.md", "two.md"] { + std::fs::write(dir.join(name), "---\nname: same\n---\n\nAmbiguous.\n").expect("a skill"); + } + + let report = skills::install(&home); + + assert_eq!( + std::fs::read_dir(&dir).expect("the directory").count(), + 2, + "io-cli wrote into a directory that was already broken", + ); + assert!( + !skills::manifest_path(&home).exists(), + "a manifest was written for skills that were never installed", + ); + assert_eq!( + report.len(), + 1, + "one line, and it is the harness's: {report:?}" + ); + assert!( + report[0].contains("same"), + "the report does not carry io-harness's own message: {report:?}", + ); +} + +/// `wrote` answers provenance from the manifest, not from the `io-` prefix. +/// +/// This is what `/skills` will decide origin on, and the prefix is a courtesy +/// rather than a guarantee: an operator's own `io-thing.md` is theirs, and a +/// shipped skill they have edited is theirs too. +#[test] +fn provenance_comes_from_the_manifest_and_never_from_the_name() { + let (_dir, home) = home(); + let dir = skills::dir(&home); + skills::install(&home); + + let mcp = dir.join("io-mcp.md"); + assert!( + skills::wrote(&home, "io-mcp", &mcp), + "a file io-cli just wrote does not read as io-cli's", + ); + + let theirs = dir.join("io-thing.md"); + std::fs::write(&theirs, "A skill the operator wrote with an io- name.\n").expect("their file"); + assert!( + !skills::wrote(&home, "io-thing", &theirs), + "a file the operator wrote reads as io-cli's because of its name", + ); + + std::fs::write(&mcp, format!("{}\nEdited.\n", read(&mcp))).expect("their edit"); + assert!( + !skills::wrote(&home, "io-mcp", &mcp), + "a shipped skill the operator has edited still reads as io-cli's", + ); +} From f1a528ff39db3f9d1f146779b26fa60453fbd55d Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Thu, 27 Aug 2026 00:56:48 +0530 Subject: [PATCH 04/10] feat(skills): install on start, and give /skills an arm to land on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The install runs immediately after the home is adopted and long before any contract is built: a skill written after the contract is one the first session of a new install cannot be offered. It is gated on adopt having adopted, because creating ~/.io-cli/skills for an operator who chose their own config home would silently attach a skills directory to a run they pointed elsewhere. /skills had a row in the table, a row in the palette and no arm in parse, so it resolved to the help listing — advertised and inert. Nothing asserted otherwise, because every gate here derives from COMMANDS and a row satisfies those by existing. The new gate states the property over the whole inventory instead: exactly one listed command may resolve to the help listing, and it is /help. --- src/commands.rs | 11 +++ src/lib.rs | 2 + src/main.rs | 159 ++++++++++++++++++++++++++++++++- tests/commands.rs | 33 +++++++ tests/install.rs | 221 ++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 425 insertions(+), 1 deletion(-) create mode 100644 tests/install.rs diff --git a/src/commands.rs b/src/commands.rs index 7b94229..bd7f5f9 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -842,6 +842,16 @@ pub enum Action { Memory, /// Show the configured MCP servers and what the session has seen of them. Mcp, + /// List every skill — the five io-cli ships and the operator's own — with + /// what it is for, whose it is, whether it is on, and the file it lives in. + /// + /// **The enabled set is read through `io_harness::Skills::discover`**, the + /// same call the run makes, so what this lists is what the model is actually + /// offered rather than io-cli's account of a directory. The disabled set is + /// read by io-cli itself, because `skills/disabled/` holds no `SKILL.md` and + /// is therefore invisible to that call by design — which is the whole + /// mechanism, not a gap in it. + Skills, /// Show the provider chain, in the order a turn tries it. Provider, /// List the named profiles, and switch to one for this session. @@ -1421,6 +1431,7 @@ pub fn parse(input: &str, keys: &Keys, theme: &Theme) -> Action { // already follow. "memory" => Action::Memory, "mcp" | "servers" => Action::Mcp, + "skills" => Action::Skills, "provider" | "providers" => Action::Provider, "profile" | "profiles" => Action::Profile, // **An alias earns no row of its own.** `/usage` is what an operator diff --git a/src/lib.rs b/src/lib.rs index e828f4f..05f3cd0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -68,6 +68,8 @@ pub mod servers; pub mod sessions; pub mod settings; pub mod shell; +pub mod skills; +pub mod skillview; pub mod splash; pub mod status; pub mod stdin; diff --git a/src/main.rs b/src/main.rs index 9c784b4..e0de4ed 100644 --- a/src/main.rs +++ b/src/main.rs @@ -77,7 +77,32 @@ async fn run(report: &mut Vec) -> Result { // visible symptom is a `/resume` that silently finds nothing. Empty when the // operator named a location themselves, which is `adopt` refusing to move // anybody who has already chosen. - *report = io_cli::home::adopt().map_or_else(Vec::new, |report| report.lines()); + let adopted = io_cli::home::adopt(); + *report = adopted + .as_ref() + .map_or_else(Vec::new, io_cli::home::Report::lines); + // **The skills io-cli ships go in immediately after the home is adopted and + // long before any contract is built**, because a skill written after the + // contract is a skill the run cannot be offered — and the run that would miss + // them is the first one of a new install, which is exactly the session in + // which somebody is most likely to ask for help. + // + // Gated on `adopt` having actually adopted, which is what the `Some` above + // means. An operator who set `IO_CONFIG` or `IO_CONFIG_HOME` themselves has + // chosen a home, `adopt` stands aside for them, and creating `~/.io-cli/skills` + // anyway would do something worse than nothing: `contract::default_skills` + // takes that directory the moment it exists, so io-cli would silently attach a + // skills directory to a run whose operator had pointed everything else + // somewhere else. + // + // It cannot fail the run. `install` returns report lines and never an error + // for exactly that reason — a read-only directory nobody has heard of is not + // a reason to refuse to start. + if adopted.is_some() { + if let Some(home) = io_cli::home::path() { + report.extend(io_cli::skills::install(&home)); + } + } let config = Config::discover(&root).map_err(|error| error.to_string())?; // **A profile, if one was asked for, before anything reads the configuration.** // Applied here rather than per arm so a session and an `io exec` run get the @@ -833,6 +858,84 @@ async fn loop_over Result>( ); } } + // Says what the skill is and offers the one change there + // is. The view is read again rather than carried: between + // the row being drawn and this keystroke the operator may + // have moved the file themselves, and a carried row would + // then name a path that is no longer there. + Pick::Skills => { + let view = io_cli::home::path() + .map(|home| io_cli::skillview::view(&home)) + .unwrap_or_default(); + if let Some(skill) = view.skills.get(index) { + app.record( + Tone::Muted, + format!( + "{} — {} · {} · {} · {}", + skill.name, + skill.description, + skill.origin.word(), + if skill.enabled { "enabled" } else { "disabled" }, + skill.path.display(), + ), + ); + let verb = if skill.enabled { + "turn it off" + } else { + "turn it back on" + }; + // `descended`, not `picker`: the assignment at + // the end of this match is unconditional, so a + // second surface opened here would be built and + // then thrown away. The same replace-in-place + // `Pick::Complete` and `Pick::Remembered` use. + descended = Some(( + Picker::new( + format!("{}?", skill.name), + vec![Row::new(verb), Row::new("leave it as it is")], + ), + Pick::SkillToggle { + name: skill.name.clone(), + path: skill.path.clone(), + enabled: skill.enabled, + }, + )); + } + } + // **A rename, and only ever a rename.** A copy would leave + // one name resolving in both directories, and two skills + // answering to one name is an `Err` from `Skills::discover` + // that io-harness propagates at run start — every turn of + // the session dead. So the failure of a move is said, and + // nothing is written twice. + Pick::SkillToggle { + name, + path, + enabled, + } => { + if index != 0 { + app.record(Tone::Muted, format!("{name} is unchanged")); + } else { + let moved = if *enabled { + io_cli::skillview::disable(path) + } else { + io_cli::skillview::enable(path) + }; + match moved { + Ok(to) => app.record( + Tone::Muted, + format!( + "{name} is now {} — {}. The next turn is composed \ + from the directory as it is then, so this is in \ + force immediately.", + if *enabled { "off" } else { "on" }, + to.display(), + ), + ), + Err(why) => app.record(Tone::Refused, why), + } + } + } // Says what the link is, and where it points. The chain // is arranged through `/config`, which is the one writer // this release gives the file. @@ -1282,6 +1385,44 @@ async fn loop_over Result>( )); } } + // **Both directories, and a failed discovery is drawn rather than + // swallowed.** `view` reads the enabled set through the same + // `Skills::discover` the run makes, so what this lists is what the + // model is offered — and when that call fails the harness's own + // sentence goes into the scrollback, which is more than io-cli + // knows how to say: on a duplicate name it names both files. The + // rows still open afterwards, because the disabled set comes from + // a directory discovery never looks at and is exactly where the + // operator's next move probably is. + Action::Skills => { + let view = io_cli::home::path() + .map(|home| io_cli::skillview::view(&home)) + .unwrap_or_default(); + if let Some(sentence) = &view.failed { + app.record(Tone::Refused, sentence.clone()); + } + if view.skills.is_empty() { + if view.failed.is_none() { + app.record( + Tone::Muted, + "no skills are installed; `io` writes its own into \ + the home on the next start", + ); + } + } else { + picker = Some(( + Picker::new( + "Skills", + io_cli::skillview::rows( + &view.skills, + screen.width(), + &app.theme.glyphs, + ), + ), + Pick::Skills, + )); + } + } Action::Provider => { let chain = io_cli::providers::chain(&config); if chain.is_empty() { @@ -3086,6 +3227,22 @@ enum Pick { /// The provider chain, in the order `providers::rows` drew it — which is /// the order a turn tries it. Provider, + /// Every skill, in the order `skillview::rows` drew them. No list is carried: + /// the view is read again when a row is chosen, because between the two + /// keystrokes the operator may have edited the directory in another pane and + /// a stale row would name a file that has moved. + Skills, + /// One skill, and the move that decides whether the model is offered it. + /// + /// **Two steps rather than one, and for the reason [`Pick::ConfigScope`] + /// gives**: this is a picker that would otherwise change a file on the way + /// past. `/mcp`'s own arm says the same thing about why it opens no editor. + /// So the list says what a skill is, and the change is a second, named answer. + SkillToggle { + name: String, + path: std::path::PathBuf, + enabled: bool, + }, /// The named profiles a file declares, in the order `configure::profiles` /// sorted them. Profile(Vec), diff --git a/tests/commands.rs b/tests/commands.rs index cfc116c..1c74fba 100644 --- a/tests/commands.rs +++ b/tests/commands.rs @@ -83,6 +83,39 @@ fn the_commands_are_the_commands() { } } +/// **A listed command that `parse` has no arm for is advertised and inert**, and +/// until 0.19.0 nothing in this file could see that: every gate here derives from +/// `COMMANDS` and `GROUPS`, which a row satisfies merely by existing. `/skills` +/// was added to the table and to the palette and resolved, silently, to the help +/// listing — the same answer `/models` gets, which is the arm that exists to tell +/// somebody they typed a command that is not real. +/// +/// So the property is stated once, over the whole inventory, rather than one +/// `assert_eq!` per command in `each_command_resolves` below — a list like that +/// stops covering the next row the moment somebody forgets to extend it, which is +/// exactly the failure this test exists for. Exactly ONE listed command may +/// resolve to the help listing, and it is `/help`. +#[test] +fn no_listed_command_falls_through_to_the_help_listing() { + let listing = commands::parse("help", &defaults(), &DARK); + let inert: Vec<&str> = commands::COMMANDS + .iter() + .map(|(name, _)| *name) + .filter(|name| { + let typed = name.strip_prefix('/').unwrap_or(name); + commands::parse(typed, &defaults(), &DARK) == listing + }) + .collect(); + assert_eq!( + inert, + vec!["/help"], + "these commands are listed but resolve to the help listing, which is what \ + an unknown command resolves to — they are advertised and inert. Give each \ + one an arm in `commands::parse` and an arm in the driver, or take the row \ + out of `COMMANDS`." + ); +} + #[test] fn each_command_resolves() { assert!(matches!( diff --git a/tests/install.rs b/tests/install.rs new file mode 100644 index 0000000..bfe9864 --- /dev/null +++ b/tests/install.rs @@ -0,0 +1,221 @@ +//! F1 and O2 — the skills io-cli ships reach the contract the run is built from, +//! and an install that cannot write does not take the session down with it. +//! +//! **What this file can prove and what it cannot.** io-harness composes the +//! prompt catalogue itself — `with_skill_catalog` and the composer around it are +//! private, and `PromptComposed` carries a byte count rather than the text — so +//! the deterministic half of F1 is that the contract the turn is built from names +//! a directory in which `Skills::discover` finds exactly the five, which is the +//! input the catalogue is composed from and the only thing io-cli decides. The +//! other half, that the five names are on the wire, is a live claim and belongs +//! to the pty capture, which reads it off the real `CompletionRequest`. +//! +//! **The wiring itself is in `src/main.rs` and nothing under `tests/` links the +//! binary**, so the ordering F1's sabotage names — installing after the contract +//! is built rather than before — cannot be driven from here. What is asserted +//! instead is the property that ordering exists to produce: after `install`, the +//! contract built from that home carries a directory whose discovery names all +//! five. A driver that installed too late would leave the first session's +//! directory empty, and `f1_the_contract_a_turn_is_built_from_names_all_five` +//! would then be describing a state the product never reaches. + +use std::path::{Path, PathBuf}; +use std::sync::{Mutex, MutexGuard, OnceLock}; + +/// `HOME` and `USERPROFILE` are process-wide and every test here rewrites them. +fn env_lock() -> MutexGuard<'static, ()> { + static LOCK: OnceLock> = OnceLock::new(); + LOCK.get_or_init(|| Mutex::new(())) + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) +} + +/// A home of this test's own, put back when it drops. +struct HomeFixture { + dir: tempfile::TempDir, + previous: [(&'static str, Option); 2], +} + +impl HomeFixture { + fn new() -> Self { + let dir = tempfile::tempdir().expect("a home directory"); + let previous = ["HOME", "USERPROFILE"].map(|var| (var, std::env::var_os(var))); + for (var, _) in &previous { + std::env::set_var(var, dir.path()); + } + Self { dir, previous } + } + + fn home(&self) -> PathBuf { + self.dir.path().join(".io-cli") + } +} + +impl Drop for HomeFixture { + fn drop(&mut self) { + for (var, value) in &self.previous { + match value { + Some(value) => std::env::set_var(var, value), + None => std::env::remove_var(var), + } + } + } +} + +/// A configuration discovered from an empty workspace, which is what a first run +/// of a new install actually has. +fn discovered() -> (tempfile::TempDir, io_harness::Config) { + let dir = tempfile::tempdir().expect("a workspace"); + let config = io_harness::Config::discover(dir.path()).expect("an empty workspace discovers"); + (dir, config) +} + +fn names(dir: &Path) -> Vec { + io_harness::Skills::discover(dir) + .expect("the installed directory discovers") + .iter() + .map(|skill| skill.name.clone()) + .collect() +} + +/// **F1 — the contract a turn is built from names all five.** +/// +/// Both arms, because `contract::configured` is the half `io exec` and a session +/// share and a skill that reached only the terminal would be the 0.14.0 asymmetry +/// this product deleted once already. +#[test] +fn f1_the_contract_a_turn_is_built_from_names_all_five() { + let _guard = env_lock(); + let fixture = HomeFixture::new(); + let home = fixture.home(); + + let report = io_cli::skills::install(&home); + assert!( + report.iter().any(|line| line.contains("installed 5")), + "a first install into an empty home says what it did: {report:?}", + ); + + let dir = io_cli::skills::dir(&home); + let mut found = names(&dir); + found.sort(); + let mut shipped: Vec = io_cli::skills::SHIPPED + .iter() + .map(|skill| skill.name.to_string()) + .collect(); + shipped.sort(); + assert_eq!( + found, shipped, + "the directory the contract will name does not resolve to the five io-cli ships", + ); + + let (workspace, config) = discovered(); + let headless = + io_cli::contract::configured("read the notes", workspace.path().to_path_buf(), &config); + assert_eq!( + headless.skills.as_deref(), + Some(dir.as_path()), + "`io exec` is built from a contract that names the installed directory", + ); +} + +/// **F1's other half, stated as the absence it really is.** +/// +/// A name is offered to the model because a file resolving to it is in the +/// directory the contract names. Assert that relationship rather than a count: +/// a count is satisfied by five files of any kind, and what the catalogue carries +/// is names. +#[test] +fn f1_every_shipped_name_is_resolvable_in_the_directory_the_contract_names() { + let _guard = env_lock(); + let fixture = HomeFixture::new(); + let home = fixture.home(); + io_cli::skills::install(&home); + + let found = names(&io_cli::skills::dir(&home)); + for skill in &io_cli::skills::SHIPPED { + assert!( + found.iter().any(|name| name == skill.name), + "`{}` is shipped and is not resolvable in the installed directory: {found:?}", + skill.name, + ); + } +} + +/// **O2 — an install that cannot write is reported and never fatal.** +/// +/// The sabotage the criterion names is propagating the error out of `run`. It +/// cannot be written as a type error, because `install` returns no `Result` at +/// all — which is the point: the signature is what makes the criterion structural +/// rather than a promise about a call site. What is asserted here is the +/// behaviour that signature buys — a home that cannot be written into yields +/// report lines and a return, and the caller has nothing to propagate. +#[test] +#[cfg(unix)] +fn o2_a_home_that_cannot_be_written_into_is_reported_and_not_fatal() { + use std::os::unix::fs::PermissionsExt; + + let _guard = env_lock(); + let fixture = HomeFixture::new(); + let home = fixture.home(); + std::fs::create_dir_all(&home).expect("the home"); + let mode = std::fs::metadata(&home) + .expect("the home is there") + .permissions(); + std::fs::set_permissions(&home, std::fs::Permissions::from_mode(0o500)) + .expect("a read-only home"); + + let report = io_cli::skills::install(&home); + + // Restored before any assertion, so a failing assertion cannot leave a + // directory the temporary-directory cleanup is unable to remove. + std::fs::set_permissions(&home, mode).expect("the mode goes back"); + + assert!( + !report.is_empty(), + "a home that could not be written into said nothing at all", + ); + assert!( + report.iter().any(|line| line.contains( + io_cli::skills::dir(&home) + .file_name() + .and_then(|name| name.to_str()) + .unwrap_or("skills") + )), + "the report does not name what it could not write: {report:?}", + ); +} + +/// **Running it twice changes nothing, which is what makes it safe on every +/// start.** The installer is on the startup path, so it runs on every launch and +/// not once — an install that was not idempotent would rewrite five files, and +/// therefore five mtimes, every time the product opened. +#[test] +fn installing_twice_writes_nothing_the_second_time() { + let _guard = env_lock(); + let fixture = HomeFixture::new(); + let home = fixture.home(); + + io_cli::skills::install(&home); + let before: Vec> = io_cli::skills::SHIPPED + .iter() + .map(|skill| { + std::fs::read(io_cli::skills::dir(&home).join(format!("{}.md", skill.name))) + .expect("an installed skill") + }) + .collect(); + + let again = io_cli::skills::install(&home); + assert!( + again.is_empty(), + "a second install had something to say about a directory it did not change: {again:?}", + ); + + let after: Vec> = io_cli::skills::SHIPPED + .iter() + .map(|skill| { + std::fs::read(io_cli::skills::dir(&home).join(format!("{}.md", skill.name))) + .expect("an installed skill") + }) + .collect(); + assert_eq!(before, after, "a second install rewrote the files"); +} From 8fd928a76cf68bed58705707539846601039f8e6 Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Thu, 27 Aug 2026 00:56:48 +0530 Subject: [PATCH 05/10] feat(skills): /skills, and enable and disable as renames MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Origin comes from the manifest and never from the io- prefix, so a file the operator wrote called io-thing.md reads as theirs and an edited shipped skill reads as theirs too — which is what install will treat it as. The enabled set is read through the harness's own discover, so the list is what the model is offered; the disabled set is read here, because a directory holding no SKILL.md is invisible to that call, which is the whole mechanism. A rename and never a copy: a copy leaves one name resolving in both directories, which is the session-killer this release exists to avoid. No EXDEV fallback either — skills/ and skills/disabled/ cannot straddle filesystems, and the fallback would be the copy by another route. --- src/skillview.rs | 483 +++++++++++++++++++++++++++++++++++++++++++++ tests/skillview.rs | 469 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 952 insertions(+) create mode 100644 src/skillview.rs create mode 100644 tests/skillview.rs diff --git a/src/skillview.rs b/src/skillview.rs new file mode 100644 index 0000000..499d5cf --- /dev/null +++ b/src/skillview.rs @@ -0,0 +1,483 @@ +//! `/skills` — what each skill is for, whose it is, whether it is on, and the +//! file it lives in; with the two levers that turn one off and back on. +//! +//! The palette has listed skills since 0.10.0 and that is a launcher: it puts a +//! name into the composer. This is the management surface, and the four facts it +//! draws are four facts the palette has never carried. **Provenance is the one it +//! exists for.** An operator looking at `io-thing.md` cannot tell from the screen +//! whether io-cli wrote it or they did, and the answer decides whether an upgrade +//! is going to overwrite it. +//! +//! # Origin comes from the manifest, never from the `io-` prefix +//! +//! The prefix is a courtesy. Nothing stops an operator naming a file `io-thing.md` +//! and nothing stops a file called anything at all from declaring `name: io-mcp` +//! in its frontmatter, which is the name discovery resolves. So origin is +//! [`crate::skills::wrote`] — *are these the bytes io-cli last wrote* — and that +//! is F6's whole sabotage arm: deciding it from the prefix tells an operator that +//! a file they wrote themselves came from io-cli, on the surface whose entire job +//! is provenance. It also has the property the prefix cannot have: a shipped skill +//! the operator has since edited reads as **theirs**, which is true, and is +//! exactly what [`crate::skills::install`] will do with it on the next upgrade. +//! +//! # Two sets, read two different ways, and only one of them has an oracle +//! +//! The enabled set is [`io_harness::Skills::discover`] over +//! [`crate::skills::dir`] — the same call the run makes, so what this surface +//! lists is what the model is offered. A `read_dir` of io-cli's own would be a +//! second opinion about what a skill file is, and it would disagree with the run +//! in precisely the case that matters. +//! +//! The disabled set has no oracle by construction. `skills/disabled/` is +//! invisible to discovery — the walk admits a subdirectory only when it holds a +//! `SKILL.md` — which is the entire mechanism by which disabling works, and it +//! means this module has to read that directory itself and resolve names itself. +//! `split_front_matter` is `pub(crate)` in the harness, so `front_matter` below is +//! io-cli's own smaller reader over the same two keys. +//! +//! # A directory that will not discover is a state, not an empty list +//! +//! `Skills::discover` returns `Err` on a missing directory, a directory that is a +//! file, more than `MAX_SKILLS` entries and — the one that matters — two skills +//! resolving to one name, which is every turn of that session dying at run start. +//! Today the operator sees an empty palette and no reason. [`View::failed`] is the +//! harness's own sentence, carried verbatim, and the disabled set is still listed +//! beside it: a broken enabled set says nothing about what is in `disabled/`, and +//! the file the operator needs to move back may well be in there. +//! +//! # Disabling is a rename and never a copy +//! +//! A copy leaves the skill in both directories, which is one resolved name +//! claimed twice, which is `Error::Config` at run start — F2's session-killer +//! arriving through io-cli's own keystroke. So there is exactly one +//! [`std::fs::rename`] here and **no `EXDEV` copy fallback**, deliberately unlike +//! [`crate::home`]'s move, which crosses filesystems because it moves a home +//! between two places an operator chose. These two directories are a parent and +//! its own child; they cannot be on different filesystems, so the fallback would +//! be dead code whose only reachable behaviour is the failure it is meant to +//! avoid. +//! +//! Neither lever rewrites a byte of the file and neither touches `io.toml`. There +//! is no `enabled` concept in the harness and no key for one in the configuration, +//! so a flag would be io-cli's alone and a second list disagreeing with the +//! filesystem is how a product grows two sources of truth. +//! +//! # No terminal I/O and no keys +//! +//! A data model and pure functions, as [`crate::servers`] is. The driver in +//! `src/main.rs` owns the keyboard and performs what [`disable`] and [`enable`] +//! return. + +use std::path::{Path, PathBuf}; + +use crate::glyphs::Glyphs; +use crate::picker::{fit, fit_left, Row}; +use crate::skills; + +/// What a skill with no description at all is listed as. +/// +/// The harness's own words for the same gap, matched on purpose: a file with no +/// frontmatter and no prose reads identically whether it is enabled — where the +/// string comes from `Skills::discover` — or disabled, where it comes from here. +const NO_DESCRIPTION: &str = "(no description)"; + +/// The narrowest a path may be drawn at, before the separator in front of it. +/// +/// Twenty cells is `...cli/skills/io-mcp.md` — the last two segments and the mark +/// saying the front went, which is what identifies a file on a machine where every +/// skill shares the first several segments of its path. Below that what survives is +/// an extension and an ellipsis, which says nothing at all about where the file is, +/// so the path is dropped whole instead. A row is allowed to lose a fact; it is not +/// allowed to draw one that cannot be read. +const PATH_FLOOR: usize = 20; + +/// Who wrote a skill. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Origin { + /// io-cli shipped it, and the bytes on disk are still the ones it wrote. + IoCli, + /// The operator's: a file io-cli never wrote, or one it wrote and they have + /// since edited. Both are theirs, and both are left alone by an upgrade. + Yours, +} + +impl Origin { + /// The word this origin draws as. + /// + /// **A word rather than a mark, and that is the ASCII form N4 asks for.** + /// [`crate::glyphs`] gives every mark a Unicode form and an ASCII one because + /// a mark that cannot be drawn has lost its meaning — but the degradation + /// only works where the ASCII substitute still says the same thing. A filled + /// dot against a hollow one is a pair `*` and `o` cannot carry: an operator + /// reading `*` has no way to know which of the two origins it stands for. + /// So there is one form here, it is already ASCII, and it needs no set. + #[must_use] + pub fn word(self) -> &'static str { + match self { + Origin::IoCli => "io-cli", + Origin::Yours => "yours", + } + } +} + +/// One skill, as `/skills` lists it. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Listed { + /// The name the model addresses it by: the frontmatter `name` where there is + /// one, else the file stem. **Not the file name** — the two differ exactly + /// when a collision is possible, which is the case this surface is for. + pub name: String, + /// The one line that goes into the prompt catalogue. + pub description: String, + /// Whose it is, decided from the manifest. See the module note. + pub origin: Origin, + /// Whether the model is offered it. A disabled skill is one whose file has + /// been moved into `skills/disabled/`, and nothing else. + pub enabled: bool, + /// The file it lives in, which is also what [`disable`] and [`enable`] take. + pub path: PathBuf, +} + +/// Everything `/skills` draws. +/// +/// Two fields rather than a `Result`, because they are not alternatives: the +/// disabled set is read out of a directory discovery never looks at, so it is +/// still listable when the enabled set has failed — and on the failure that +/// matters most, a duplicate name, the operator's next move is very likely to be +/// in the list that survived. +#[derive(Debug, Clone, Default)] +pub struct View { + /// Both directories, sorted by name the way discovery sorts. + pub skills: Vec, + /// The harness's own sentence, when the enabled set would not discover. + /// + /// **Verbatim.** It names the directory, and on a duplicate it names both + /// files — which is the whole of what the operator needs and more than io-cli + /// knows how to say. The driver draws it above the rows. + pub failed: Option, +} + +/// Every skill in `/skills` and `/skills/disabled`. +#[must_use] +pub fn view(home: &Path) -> View { + let mut listed = Vec::new(); + let mut failed = None; + + match io_harness::Skills::discover(skills::dir(home)) { + Ok(found) => listed.extend(found.iter().map(|skill| Listed { + origin: origin(home, &skill.name, &skill.path), + name: skill.name.clone(), + description: skill.description.clone(), + enabled: true, + path: skill.path.clone(), + })), + Err(error) => failed = Some(error.to_string()), + } + + for path in disabled_files(home) { + let (name, description) = describe(&path); + listed.push(Listed { + origin: origin(home, &name, &path), + name, + description, + enabled: false, + path, + }); + } + + // By name, then by path. The name is what an operator is looking for and is + // what discovery sorts by, so the two halves interleave rather than sitting + // in two blocks; the path breaks the tie a name claimed in both directories + // would otherwise leave to `read_dir` order. + listed.sort_by(|left, right| (&left.name, &left.path).cmp(&(&right.name, &right.path))); + View { + skills: listed, + failed, + } +} + +/// Whose a file is: the manifest's answer, never the file name's. +fn origin(home: &Path, name: &str, path: &Path) -> Origin { + if skills::wrote(home, name, path) { + Origin::IoCli + } else { + Origin::Yours + } +} + +/// The `*.md` files sitting in `skills/disabled`, sorted, absolute. +/// +/// Top-level files only, which is what [`disable`] ever puts there — see the note +/// on it about the bundle it declines to move. A directory that is not there at +/// all is no disabled skills, which is the ordinary case: `disabled/` is created +/// by the first disable and never before, so that an operator who has turned +/// nothing off has no directory to wonder about. +/// +/// Canonicalised the way `Skills::discover` canonicalises, so a path off this +/// surface is comparable with a path off that one and neither depends on the +/// process's working directory. +fn disabled_files(home: &Path) -> Vec { + let Ok(entries) = std::fs::read_dir(skills::disabled_dir(home)) else { + return Vec::new(); + }; + let mut files: Vec = entries + .filter_map(|entry| entry.ok()) + .map(|entry| entry.path()) + .filter(|path| { + path.is_file() + && path + .extension() + .is_some_and(|extension| extension.eq_ignore_ascii_case("md")) + }) + .map(|path| std::fs::canonicalize(&path).unwrap_or(path)) + .collect(); + files.sort(); + files +} + +/// The name and description a disabled file declares. +/// +/// The same fallbacks the harness uses, because a file that is disabled today was +/// enabled yesterday and will be enabled again tomorrow: a skill that changed its +/// name by being turned off would be a different skill on the way back. +fn describe(path: &Path) -> (String, String) { + let stem = path + .file_stem() + .map(|stem| stem.to_string_lossy().to_string()) + .unwrap_or_default(); + let Ok(text) = std::fs::read_to_string(path) else { + // Unreadable, and this surface still lists it: a file the operator cannot + // read is one they especially need to be shown the path of. + return (stem, NO_DESCRIPTION.to_string()); + }; + let (name, description, body) = front_matter(&text); + ( + name.unwrap_or(stem), + description + .or_else(|| first_prose_line(body)) + .unwrap_or_else(|| NO_DESCRIPTION.to_string()), + ) +} + +/// `name` and `description` out of a leading `---` fence, and the body after it. +/// +/// io-cli's own, because the harness's `split_front_matter` is `pub(crate)`. Two +/// scalar keys and a fence; an unterminated fence is a file with no frontmatter, +/// which is what the harness decides too — guessing where the operator meant it to +/// close is worse than falling back to the filename. +/// +/// ponytail: `description: >` and `description: |` are read as absent, so a block +/// scalar falls through to the first prose line rather than to the block. Reading +/// the block needs the harness's continuation state machine; the day a shipped +/// skill needs one, this grows the same `open`/`buffer` pair it has. +fn front_matter(text: &str) -> (Option, Option, &str) { + let Some(rest) = text + .strip_prefix("---\n") + .or_else(|| text.strip_prefix("---\r\n")) + else { + return (None, None, text); + }; + + // Byte offsets, so the body comes back borrowed out of the original. + let mut offset = 0usize; + let mut fence = None; + for line in rest.split_inclusive('\n') { + if line.trim_end_matches(['\r', '\n']).trim() == "---" { + fence = Some((offset, offset + line.len())); + break; + } + offset += line.len(); + } + let Some((front_len, body_start)) = fence else { + return (None, None, text); + }; + + let mut name = None; + let mut description = None; + for line in rest[..front_len].lines() { + let Some((key, value)) = line.split_once(':') else { + continue; + }; + let value = value.trim(); + if value.is_empty() { + continue; + } + match key.trim() { + "name" => name = Some(value.to_string()), + "description" => description = Some(value.to_string()), + _ => {} + } + } + (name, description, &rest[body_start..]) +} + +/// The first line of a body that reads as prose, heading and quote marks stripped. +fn first_prose_line(body: &str) -> Option { + body.lines() + .map(|line| line.trim().trim_start_matches(['#', '>']).trim()) + .find(|line| !line.is_empty()) + .map(str::to_string) +} + +/// The picker rows, fitted for a terminal this wide. +/// +/// The name is the label, because content precedes metadata and the name is what +/// the model addresses. The detail carries the other three facts in the order an +/// operator needs them, **composed to a budget rather than assembled and trimmed** +/// — the shape `crate::sessions::rows` was rewritten into after a row was cut at +/// the picker's edge and left a turn count amputated to a single digit. +/// +/// # The narrow form, and what gives way in it +/// +/// N4 fixes the rule: **the path gives way, and the name and the state never do.** +/// So the origin and the state are unconditional — two short words that together +/// cost about fifteen cells — the description takes what is left after them, and +/// the path is appended only if what remains after *that* still holds a shortened +/// form of it. At eighty columns a row is therefore the name, the origin, the +/// state and as much of the description as fits, with no path at all; widen the +/// terminal and the path arrives. A state word cut in half would be a lie about +/// whether the model is being offered the skill, and a path cut below `PATH_FLOOR` +/// is not where a file is — so the path is the field that is *dropped*, never the +/// field that is drawn illegibly. +/// +/// The path is shortened from the **left**, since every skill on one machine +/// shares the first several segments of its path and the end is what identifies it. +pub fn rows(skills: &[Listed], width: u16, glyphs: &Glyphs) -> Vec { + let separator = glyphs.separator; + let separator_width = separator.chars().count(); + // A field is worth appending only if there is room for more than the mark that + // would say it had been shortened. Measured off the set rather than assumed: + // the ellipsis is one cell in Unicode and three in ASCII. + let floor = separator_width + glyphs.ellipsis.chars().count(); + + skills + .iter() + .map(|skill| { + let state = if skill.enabled { "enabled" } else { "disabled" }; + let mut detail = format!("{}{separator}{state}", skill.origin.word()); + + // The picker's own arithmetic, mirrored: two cells of marker, the + // label, two cells of gap. Mirrored rather than guessed, because a + // budget one cell out is how an ellipsis ends up on the floor. Four in + // either glyph set — the marker is two cells in both. + let mut left = (width as usize) + .saturating_sub(4) + .saturating_sub(skill.name.chars().count()) + .saturating_sub(detail.chars().count()); + + if left > floor { + let described = fit(&skill.description, left - separator_width, glyphs); + left -= separator_width + described.chars().count(); + detail.push_str(separator); + detail.push_str(&described); + } + if left >= separator_width + PATH_FLOOR { + detail.push_str(separator); + detail.push_str(&fit_left( + &skill.path.display().to_string(), + left - separator_width, + glyphs, + )); + } + + Row::with_detail(skill.name.clone(), detail) + }) + .collect() +} + +/// Move a skill's file into `skills/disabled/`, creating that directory if it is +/// not there. Answers with where the file now is. +/// +/// The next turn stops offering it, because the skills directory is resolved per +/// turn and `disabled/` is invisible to the walk that reads it. +/// +/// **One rename, no copy, no partial state.** See the module note: a copy is the +/// same file under one name in two places, which is `Error::Config` at run start +/// and every turn of that session dead. A rename either happened or did not. +/// +/// The failure that is not hypothetical is Windows refusing to rename a file +/// another process holds open — a second `io` running, an editor with the skill +/// open — which is how 0.15.0's post-seal defect was found. It comes back as a +/// sentence naming the file, never as a panic. +/// +/// ponytail: declines a `SKILL.md` bundle rather than moving its directory. Moving +/// the file alone would leave an empty directory behind and land as `disabled/SKILL.md`, +/// which collides with the next bundle disabled and resolves to the name `SKILL`; +/// moving the directory is the correct answer and is what this grows if an operator +/// asks for it. Declining is the one option that cannot lose a file. +pub fn disable(path: &Path) -> Result { + if is_bundle(path) { + return Err(format!( + "{} is a skill folder rather than a single file; move the folder into the disabled \ + one by hand to turn it off", + path.display() + )); + } + let Some(dir) = path.parent() else { + return Err(format!("{} is not in a skills directory", path.display())); + }; + let disabled = dir.join(skills::DISABLED); + // Made here and nowhere earlier: the surface that first moves a file in is + // what creates it. `crate::home::create` rather than a bare `create_dir_all`, + // so it is `0700` like everything else under the home. + if let Err(error) = crate::home::create(&disabled) { + return Err(format!("could not create {}: {error}", disabled.display())); + } + relocate(path, &disabled) +} + +/// Move a disabled skill's file back out of `skills/disabled/`. Answers with +/// where the file now is. +/// +/// The inverse of [`disable`] byte for byte: neither reads the file, so neither +/// can change it. +pub fn enable(path: &Path) -> Result { + // Out of `disabled/` and into its parent, which is the skills directory. Two + // levels up from the file rather than a path recomputed from the home, + // because the caller has a path off [`view`] and the home it came from is not + // on this call — and a file that is not two levels deep is not one this + // surface put there. + let Some(dir) = path.parent().and_then(Path::parent) else { + return Err(format!( + "{} is not in a disabled skills directory", + path.display() + )); + }; + relocate(path, dir) +} + +/// Whether a path is a `SKILL.md` inside its own directory. +fn is_bundle(path: &Path) -> bool { + path.file_name() + .is_some_and(|name| name.eq_ignore_ascii_case("SKILL.md")) +} + +/// Move one file into `into`, keeping its name. +/// +/// **Refuses to move over a file that is already there.** `std::fs::rename` +/// replaces the destination silently on unix, so enabling `io-mcp` while the +/// operator has written their own `io-mcp.md` in the meantime would destroy +/// theirs — and this surface's one promise is that neither lever changes a file's +/// contents. The state it declines is one an operator made by hand, so the +/// sentence names both paths and lets them decide. +fn relocate(from: &Path, into: &Path) -> Result { + let Some(name) = from.file_name() else { + return Err(format!("{} has no file name", from.display())); + }; + let to = into.join(name); + if to.exists() { + return Err(format!( + "{} is already there, so io-cli did not move {} over it", + to.display(), + from.display() + )); + } + match std::fs::rename(from, &to) { + Ok(()) => Ok(to), + Err(error) => Err(format!( + "could not move {} to {}: {error}", + from.display(), + to.display() + )), + } +} diff --git a/tests/skillview.rs b/tests/skillview.rs new file mode 100644 index 0000000..7b82ef4 --- /dev/null +++ b/tests/skillview.rs @@ -0,0 +1,469 @@ +//! F6, F7 and N4 — the `/skills` surface: provenance, the two levers, and what +//! the rows look like on a terminal that can draw nothing but ASCII. +//! +//! Every claim about what the *enabled* set holds goes through +//! [`io_harness::Skills::discover`], for the reason `tests/skills.rs` opens with: +//! a resolved name comes from frontmatter where there is one, so a test that +//! counted `io-*.md` files would agree with io-cli and disagree with the run. The +//! harness's walk is the only oracle whose verdict is the one every turn gets. +//! +//! Nothing here touches the environment — [`skillview::view`] takes the home as an +//! argument — so each test owns a temporary directory and they run in parallel. + +mod support; + +use std::path::{Path, PathBuf}; + +use io_cli::skills; +use io_cli::skillview::{self, Listed, Origin, View}; + +/// A home with nothing in it. +fn home() -> (tempfile::TempDir, PathBuf) { + let dir = tempfile::tempdir().expect("a temporary directory"); + let home = dir.path().join("home"); + (dir, home) +} + +fn write(path: &Path, text: &str) { + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent).expect("the parent directory"); + } + std::fs::write(path, text).unwrap_or_else(|error| panic!("{}: {error}", path.display())); +} + +fn read(path: &Path) -> Vec { + std::fs::read(path).unwrap_or_else(|error| panic!("{}: {error}", path.display())) +} + +/// A skill file: the two keys the harness reads, and a line of body. +fn skill(name: &str, description: &str) -> String { + format!("---\nname: {name}\ndescription: {description}\n---\n\nDo the thing.\n") +} + +/// Record the files at `paths` in the manifest as io-cli's own. +/// +/// Written out here in the format the module documents rather than through an +/// io-cli helper, so a release that quietly changed where or how provenance is +/// recorded turns this red instead of passing through an accessor that changed +/// with it. This is the fixture the whole of F6 turns on: without it every file +/// on disk is the operator's, which is exactly the direction +/// [`skills::wrote`] degrades in. +fn recorded(home: &Path, entries: &[(&str, &Path)]) { + let mut text = String::new(); + for (name, path) in entries { + text.push_str(&format!("{name}\t{:016x}\n", skills::digest(&read(path)))); + } + write(&home.join(".skills-manifest"), &text); +} + +/// The row for one name, or a panic naming what was listed instead. +fn listed<'a>(view: &'a View, name: &str) -> &'a Listed { + view.skills + .iter() + .find(|skill| skill.name == name) + .unwrap_or_else(|| { + let names: Vec<&str> = view.skills.iter().map(|s| s.name.as_str()).collect(); + panic!("`{name}` is not listed; the surface shows {names:?}") + }) +} + +/// The names io-harness resolves out of a directory, sorted as it sorts them. +/// +/// Panics on a directory that will not discover, which is the point: a criterion +/// asserting the session still runs is asserting exactly that this returns `Ok`. +fn discovered(dir: &Path) -> Vec { + io_harness::Skills::discover(dir) + .unwrap_or_else(|error| { + panic!( + "{} does not discover, so every turn of that session would fail at run \ + start: {error}", + dir.display() + ) + }) + .iter() + .map(|skill| skill.name.clone()) + .collect() +} + +// --------------------------------------------------------------------------- +// F6 — what it is, whose it is, whether it is on, and where it lives. +// --------------------------------------------------------------------------- + +#[test] +fn f6_origin_is_decided_by_the_manifest_and_never_by_the_io_prefix() { + // **The sabotage arm.** `io-thing.md` carries the prefix io-cli ships under and + // is the operator's; `mine.md` carries no prefix and is io-cli's, because the + // manifest says io-cli wrote those bytes. A surface that read the file name + // would get both of them backwards, and it would be telling an operator that a + // file they wrote themselves came from io-cli — on the surface whose entire job + // is provenance. + let (_dir, home) = home(); + let dir = skills::dir(&home); + + let ours = dir.join("mine.md"); + write( + &ours, + &skill("mine", "A skill io-cli happens to have written."), + ); + let theirs = dir.join("io-thing.md"); + write(&theirs, &skill("io-thing", "A skill the operator wrote.")); + + // Only the un-prefixed one is recorded, so the prefix and the truth disagree. + recorded(&home, &[("mine", ours.as_path())]); + + let view = skillview::view(&home); + assert_eq!(view.failed, None, "the fixture discovers"); + assert_eq!(listed(&view, "mine").origin, Origin::IoCli); + assert_eq!(listed(&view, "io-thing").origin, Origin::Yours); + assert_eq!(listed(&view, "io-thing").origin.word(), "yours"); + + // And the second half of the same rule: a shipped skill the operator has since + // edited is theirs. The bytes no longer hash to what io-cli recorded, which is + // true, and is also what the next upgrade will decide about it. + write(&ours, &skill("mine", "Edited by the operator.")); + let view = skillview::view(&home); + assert_eq!( + listed(&view, "mine").origin, + Origin::Yours, + "an edited shipped skill belongs to whoever edited it" + ); +} + +#[test] +fn f6_a_skill_carries_its_description_its_state_and_the_file_it_lives_in() { + let (_dir, home) = home(); + let dir = skills::dir(&home); + let path = dir.join("alpha.md"); + write(&path, &skill("alpha", "Does the alpha thing.")); + + let view = skillview::view(&home); + let row = listed(&view, "alpha"); + assert_eq!(row.description, "Does the alpha thing."); + assert!(row.enabled, "a file in skills/ is offered to the model"); + assert!( + row.path.ends_with("alpha.md"), + "the surface says which file, not just which name: {}", + row.path.display() + ); +} + +#[test] +fn f6_a_disabled_skill_is_listed_as_disabled_with_its_path_inside_disabled() { + let (_dir, home) = home(); + let dir = skills::dir(&home); + let path = dir.join("alpha.md"); + write(&path, &skill("alpha", "Does the alpha thing.")); + + let moved = skillview::disable(&path).expect("the move"); + + let view = skillview::view(&home); + let row = listed(&view, "alpha"); + assert!(!row.enabled, "a file under disabled/ is not offered"); + // Canonicalised on both sides: the surface resolves a path the way + // `Skills::discover` does, so a row off this list and a row off that one are + // comparable — and on macOS a temporary directory is a symlink, which is the + // difference that would otherwise make this assertion about the platform. + assert_eq!( + row.path, + std::fs::canonicalize(&moved).expect("the moved file") + ); + assert_eq!( + row.path.parent().and_then(Path::file_name), + Some(std::ffi::OsStr::new(skills::DISABLED)), + "the path says where it went: {}", + row.path.display() + ); + // Still named, still described. A disabled skill that lost its description + // would be a row an operator cannot decide about. + assert_eq!(row.description, "Does the alpha thing."); +} + +#[test] +fn f6_a_directory_that_will_not_discover_shows_the_harness_sentence() { + // Two files resolving to one name: `Error::Config` at run start, and every turn + // of that session dead before the first completion. Today the operator sees an + // empty palette and no reason, which is the hole F6 names. + let (_dir, home) = home(); + let dir = skills::dir(&home); + write(&dir.join("a.md"), &skill("dup", "One of two.")); + write(&dir.join("b.md"), &skill("dup", "The other of two.")); + // And one skill already turned off, in the directory discovery never looks at. + write( + &skills::disabled_dir(&home).join("parked.md"), + &skill("parked", "Turned off earlier."), + ); + + let view = skillview::view(&home); + + let sentence = view + .failed + .clone() + .expect("a failed discovery is a state, not silence"); + let harness = io_harness::Skills::discover(&dir) + .expect_err("the fixture is ambiguous") + .to_string(); + assert_eq!( + sentence, harness, + "the surface carries the harness's own sentence verbatim; it names both files" + ); + assert!(sentence.contains("dup"), "the sentence names the collision"); + + // Not an empty list. The disabled set is read out of a directory discovery + // never walks, so it survives the failure — and on this failure in particular + // the operator's next move is very likely to be in it. + assert_eq!( + view.skills + .iter() + .map(|s| s.name.as_str()) + .collect::>(), + vec!["parked"], + ); + assert!(!listed(&view, "parked").enabled); +} + +// --------------------------------------------------------------------------- +// F7 — enable and disable are renames, and the next turn sees them. +// --------------------------------------------------------------------------- + +#[test] +fn f7_disable_then_enable_returns_the_file_byte_for_byte_and_leaves_one_copy() { + let (_dir, home) = home(); + let dir = skills::dir(&home); + let path = dir.join("alpha.md"); + let before = skill("alpha", "Does the alpha thing."); + write(&path, &before); + + // `disabled/` is made by the first disable and never before it, so an operator + // who has turned nothing off has no directory to wonder about. + assert!( + !skills::disabled_dir(&home).exists(), + "disabled/ is created on demand" + ); + + let parked = skillview::disable(&path).expect("the move"); + assert!(skills::disabled_dir(&home).is_dir()); + assert_eq!(parked, skills::disabled_dir(&home).join("alpha.md")); + + // **The absence is the assertion.** A copy would leave the file in both + // directories, which is one resolved name claimed twice — F2's session-killer + // arriving through io-cli's own keystroke. + assert!( + !path.exists(), + "{} is still there, so the skill is in two directories at once", + path.display() + ); + assert_eq!(read(&parked), before.as_bytes(), "a move rewrites no byte"); + + let back = skillview::enable(&parked).expect("the move back"); + assert_eq!(back, path); + assert!( + !parked.exists(), + "{} is still there after enabling it", + parked.display() + ); + assert_eq!(read(&path), before.as_bytes(), "byte for byte, both ways"); +} + +#[test] +fn f7_a_disabled_skill_is_gone_from_the_harnesss_own_discovery() { + // The oracle, not io-cli's opinion: what the run will be offered on the next + // turn, since the skills directory is resolved per turn. + let (_dir, home) = home(); + let dir = skills::dir(&home); + write(&dir.join("alpha.md"), &skill("alpha", "One.")); + write(&dir.join("beta.md"), &skill("beta", "Two.")); + + assert_eq!(discovered(&dir), vec!["alpha", "beta"]); + + skillview::disable(&dir.join("alpha.md")).expect("the move"); + + // Still `Ok` — `disabled/` holds no `SKILL.md`, so the walk skips it rather + // than reading the loose file inside as a second `alpha`. + assert_eq!(discovered(&dir), vec!["beta"]); +} + +#[test] +fn f7_neither_lever_writes_to_io_toml() { + // There is no `enabled` key in the harness and none in io-cli's configuration. + // A flag would be a second list disagreeing with the filesystem. + let (_dir, home) = home(); + let dir = skills::dir(&home); + let path = dir.join("alpha.md"); + write(&path, &skill("alpha", "One.")); + let config = home.join("io.toml"); + write(&config, "model = \"a-model\"\n"); + + let parked = skillview::disable(&path).expect("the move"); + skillview::enable(&parked).expect("the move back"); + + assert_eq!( + std::fs::read_to_string(&config).expect("io.toml"), + "model = \"a-model\"\n" + ); +} + +#[test] +fn f7_a_move_that_would_overwrite_a_file_is_a_sentence_and_not_a_loss() { + // The state Windows reports as an error and unix performs silently: enabling + // `alpha` while the operator has written their own `alpha.md` in the meantime. + // A rename here would destroy theirs, and this surface's one promise is that + // neither lever changes a file's contents. + let (_dir, home) = home(); + let dir = skills::dir(&home); + let path = dir.join("alpha.md"); + write(&path, &skill("alpha", "Theirs.")); + let parked = skills::disabled_dir(&home).join("alpha.md"); + write(&parked, &skill("alpha", "Turned off earlier.")); + + let error = skillview::enable(&parked).expect_err("it declines"); + assert!( + error.contains("alpha.md"), + "the message names the file: {error}" + ); + assert_eq!( + read(&path), + skill("alpha", "Theirs.").as_bytes(), + "the operator's file is untouched" + ); + assert!( + parked.exists(), + "and nothing was lost from disabled/ either" + ); +} + +#[test] +fn f7_a_rename_that_fails_is_a_readable_sentence_and_never_a_panic() { + // Windows refuses to rename a file another process holds open, which is how + // 0.15.0's post-seal defect was found. A missing file is the portable stand-in + // for the same shape: the call answers, the message names the file, and there + // is no half-move to clean up. + let (_dir, home) = home(); + let path = skills::dir(&home).join("gone.md"); + std::fs::create_dir_all(skills::dir(&home)).expect("the directory"); + + let error = skillview::disable(&path).expect_err("it fails rather than panicking"); + assert!( + error.contains("gone.md"), + "the message names the file: {error}" + ); + assert!(!skills::disabled_dir(&home).join("gone.md").exists()); +} + +// --------------------------------------------------------------------------- +// N4 — ASCII, eighty columns, and the field that gives way. +// --------------------------------------------------------------------------- + +/// A home holding one io-cli skill, one of the operator's, and one turned off. +/// +/// Every byte of it is ASCII on purpose, so the render below can be asserted whole +/// rather than field by field: anything non-ASCII on the screen came from this +/// surface's own marks, which is exactly N4's sabotage. +fn drawable() -> (tempfile::TempDir, PathBuf) { + let (dir, home) = home(); + let skills_dir = skills::dir(&home); + + let ours = skills_dir.join("io-mcp.md"); + write( + &ours, + &skill( + "io-mcp", + "Add, change or remove an MCP server by proposing an edit to io.toml.", + ), + ); + write( + &skills_dir.join("mine.md"), + &skill("mine", "Something the operator wrote for this machine."), + ); + write( + &skills::disabled_dir(&home).join("parked.md"), + &skill( + "parked", + "Turned off by the operator, and still listed here.", + ), + ); + recorded(&home, &[("io-mcp", ours.as_path())]); + (dir, home) +} + +#[test] +fn n4_every_row_draws_in_ascii_with_its_meaning_intact_inside_eighty_columns() { + let (_dir, home) = drawable(); + let view = skillview::view(&home); + assert_eq!(view.failed, None); + assert_eq!(view.skills.len(), 3); + + let theme = io_cli::theme::DARK.with_glyphs(io_cli::glyphs::ASCII); + let rows = skillview::rows(&view.skills, 80, &theme.glyphs); + let mut picker = io_cli::picker::Picker::new("skills", rows); + + let (mut screen, _recorder) = support::screen(80, 24); + screen + .draw(|frame| picker.render(frame, frame.area(), &theme)) + .expect("frame"); + let drawn = screen.viewport_text().to_string(); + + for line in drawn.lines() { + assert!( + line.is_ascii(), + "a row drew a character the ASCII terminal cannot: {line:?}" + ); + assert!( + line.chars().count() <= 80, + "a row overflowed eighty columns: {line:?}" + ); + } + + // The meaning, not merely the bytes: the two markers are words, so they say + // which origin and which state without a legend and without a glyph set. + for word in [ + "io-mcp", "mine", "parked", "io-cli", "yours", "enabled", "disabled", + ] { + assert!( + drawn.contains(word), + "`{word}` is not on the screen:\n{drawn}" + ); + } +} + +#[test] +fn n4_the_path_is_what_gives_way_and_the_name_and_the_state_never_do() { + let (_dir, home) = drawable(); + let view = skillview::view(&home); + let ascii = io_cli::glyphs::ASCII; + let glyphs = &ascii; + + // Eighty columns: the description takes what the two markers leave, and the + // path is dropped whole rather than shortened past legibility. + for row in skillview::rows(&view.skills, 80, glyphs) { + let detail = row.detail.expect("every row has a detail"); + assert!( + !detail.contains(".md"), + "the path should have given way at eighty columns: {detail:?}" + ); + assert!( + detail.contains("enabled") || detail.contains("disabled"), + "the state never gives way: {detail:?}" + ); + assert!( + detail.starts_with("io-cli") || detail.starts_with("yours"), + "the origin never gives way: {detail:?}" + ); + assert!(!row.label.ends_with(glyphs.ellipsis), "the name is not cut"); + } + + // Widen the terminal and the path arrives, shortened from the LEFT, because + // every skill on one machine shares the first segments of its path. + let wide = skillview::rows(&view.skills, 240, glyphs); + let details: Vec = wide + .into_iter() + .map(|row| row.detail.expect("a detail")) + .collect(); + assert!( + details.iter().all(|detail| detail.contains(".md")), + "the path is on a wide terminal: {details:?}" + ); + assert!( + details.iter().any(|detail| detail.contains("parked.md")), + "and the end of the path is what survives, since the front is shared: {details:?}" + ); +} From e1599a3d38c03d97687c7e352b3a84cb33c87b66 Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Thu, 27 Aug 2026 00:56:48 +0530 Subject: [PATCH 06/10] docs: the shipped skills, where they live, and what can go wrong The command table is rebuilt from GROUPS rather than edited, which also clears drift nobody could see: /steer and /compact had been listed under inspect since 0.17.0 while the code filed them under Turn, and the gate checks presence and row count but never which heading a row sits under. Also fixes a claim this release makes false. 'io-cli parses no skill file and keeps no copy of one' was true until it embedded five bodies; the true half stays. --- CHANGELOG.md | 44 ++++++++++++++++++++ README.md | 104 +++++++++++++++++++++++++++++++++++++++++++--- tests/docs.rs | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 256 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6370ac5..b9e345a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,50 @@ All notable changes to this project are documented here. The format follows ## [Unreleased] +## [0.19.0] - 2026-08-27 + +Ask for a thing io can do, in your own words, and have it happen. + +**Five skills ship with io and answer the asks nobody should have to learn a +command for.** "Stop asking me before every write in this repository" is +`io-permissions`; "add the GitHub MCP server" is `io-mcp`; "point this at a +local model instead" is `io-provider`; "remember that we use pnpm here" is +`io-remember`; "update io" is `io-update`, which checks the published Release +and proposes the exact `install.sh` line rather than replacing the binary behind +you. Every one of them ends in a change you see before it lands — a proposed +edit, shown as a diff, gated by the same policy as anything else. + +**They are ordinary files in `~/.io-cli/skills`, beside your own.** `SKILL.md` +bodies and nothing more clever than that: open one, edit it, fork it, delete it. +The model is offered each skill's name and description on every turn and reads +the body through io-harness's own `read_skill` tool, under this session's +policy. An upgrade brings forward the ones nobody has touched, leaves a file you +edited exactly as it is and says so, and never resurrects one you turned off. + +**`/skills` is where all of it is visible**, and turning a skill off is moving +its file into `skills/disabled/` — a directory io-harness's discovery walks past +because it holds no `SKILL.md`. `/skills` does the move with a keystroke, it +takes effect on the next turn, and it works on your own skills too. There is no +`enabled` flag anywhere, because a second list disagreeing with the filesystem +is how a product grows two sources of truth. + +**`/mcp` and `/provider` moved from the inspect group to the configure group**, +which matters to anyone who finds a command by scanning `/help` rather than by +typing it. Both open with a list and both go on to add, edit, disable and remove +entries in the configuration file, which is what configure means and what +inspect promises it does not do. The commands, their keys and their screens are +unchanged; only where they are listed is. Inspect goes to nine and configure to +six, with `/skills` the twenty-sixth command. + +**Two guards you can watch work.** A skill of yours already answering to a +shipped name is never overwritten: io-cli installs four files instead of five +and names the one it withheld and the file that claimed the name — because +io-harness addresses a skill by name and two of one name ends every turn of the +session before the first completion. And io-harness accepts at most 64 skills in +a directory, rejecting the whole set rather than trimming it, so io-cli counts +before it writes, stops short of the ceiling and says how many it withheld. An +install that cannot write at all is reported and the session runs anyway. + ## [0.18.0] - 2026-08-26 Tell it to remember something, and it is still remembered tomorrow. diff --git a/README.md b/README.md index 30988c7..812be5b 100644 --- a/README.md +++ b/README.md @@ -363,6 +363,8 @@ above a row that ranked there for reasons having nothing to do with it. | `/contain` | run turns contained, so the agent can fan out: on, off, or ask | | `/plan` | make turns propose a plan before they work: on, off, or ask | | `/profile` | switch to a named profile from the configuration, for this session | +| `/steer` | send what is queued into the turn that is already running | +| `/compact` | fold this conversation into a summary, at the next step | **inspect** @@ -371,12 +373,9 @@ above a row that ranked there for reasons having nothing to do with it. | `/help` | this table | | `/status` | commit the whole session state into the scrollback | | `/context` | what is in the model's window, read from the request that carried the turn | -| `/steer` | send what is queued into the turn that is already running | -| `/compact` | fold this conversation into a summary, at the next step | | `/expand` | commit the last step's full detail into the scrollback | | `/fleet` | show the children this turn has spawned | -| `/mcp` | the MCP servers configured, and what this session has seen of each | -| `/provider` | the providers configured, in the order a turn tries them | +| `/skills` | every skill, shipped or yours: what it is for, whether it is on, and its file | | `/image` | draw an attached image again: /image 1 | | `/copy` | put the last answer on the system clipboard | | `/copy diff` | put the whole run's patch on the system clipboard | @@ -389,6 +388,16 @@ above a row that ranked there for reasons having nothing to do with it. | `/theme` | change the theme for this session | | `/remember` | remember a line of guidance, in the scope you choose | | `/memory` | what io remembers: the instruction files, and the agent's own notes | +| `/mcp` | the MCP servers configured, and what this session has seen of each | +| `/provider` | the providers configured, in the order a turn tries them | + +`/mcp` and `/provider` sit under **configure** from 0.19.0, and it is a +correction rather than a promotion: both open with a list, and both go on from +that list to add, edit, disable and remove entries in the configuration file, +which is the one thing **inspect** promises it never does. `/steer` and +`/compact` are listed under **this turn**, which is the group the code has +always filed them in — the table above said otherwise until this release, and +nothing but a reader was misled by it. `/usage` answers what `/status` answers and is deliberately not listed above: an alias earns no row of its own, because a second row for one screen reads as a @@ -445,6 +454,81 @@ refuse a large payload without saying so. `/theme` and `/model` change this session only and say so. Making a choice permanent is `io setup`. +## Skills + +**Five of the things io can do have a plain-language door.** Say what you want +in your own words — "stop asking me before every write in this repository", "add +the GitHub MCP server", "point this at a local model instead", "remember that we +use pnpm here", "update io" — and the model reaches for the skill that answers +it, instead of you reaching for the command that does. + +| Skill | For | +| --- | --- | +| `io-permissions` | changing what io asks about before it acts | +| `io-mcp` | adding, changing or removing an MCP server | +| `io-provider` | switching provider, or pointing the session at a local model | +| `io-remember` | writing something down where the next session reads it | +| `io-update` | finding out whether a newer io has been released, and proposing the installer line for it | + +**They are files, and nothing more clever than that.** Five ordinary `SKILL.md` +bodies written into `~/.io-cli/skills`, beside whatever skills you keep there +yourself. Open one, read it, edit it, copy it into a skill of your own, delete +it — the same things you would do to any other markdown file in a directory you +own. There is no registry, no index and no remote source; the five are carried +in the binary and written out on the first run that finds them missing. + +**Each of them ends in a change you see before it lands.** A skill instructs the +model in what io can already do and which surface does it, so what comes back is +a proposed edit to `io.toml`, or to a memory file, or a command to run — shown +as a diff or as a command, gated by exactly the policy everything else is gated +by, and refusable. A skill can no more move the permission boundary behind your +back than the agent can, because moving it *is* a write, and a write is a thing +you approve. + +**The model is offered a name and a description, and reads the rest only when it +matters.** Every turn's system prompt carries the catalogue — five names and +five short descriptions — and the body of a skill reaches the model through +io-harness's own `read_skill` tool, under this session's policy, like any other +read. So a skill costs the prompt one line until it is relevant, and it is +subject to the same boundary as everything else in the session. + +**An upgrade refreshes what nobody touched and leaves the rest alone.** io-cli +records the bytes it last wrote for each shipped skill; a file that still +matches gets the new text, and a file you have edited is kept exactly as it is +and named on screen as kept. A skill with no record at all is treated as yours. +The bias is deliberate: there is no restore point behind these files, so the +failure mode of a lost or unreadable record is that io-cli stops refreshing, +never that it writes over something you wrote. + +**Turning a skill off is moving its file into `~/.io-cli/skills/disabled/`.** +io-harness admits a subdirectory only when it holds a `SKILL.md`, so a folder of +loose `.md` files is invisible to discovery, to the catalogue and to +`read_skill` — which makes a directory the whole mechanism, with no second list +to disagree with the filesystem. It works on your own skills too, and it +survives an upgrade: a shipped skill sitting in `disabled/` is not written back +into `skills/` on the next launch, because a switch that turns itself on again +every morning is not a switch. `/skills` does the move for you and shows, for +every skill in both directories, what it is for, whether it came from io-cli or +from you, whether it is on, and the file it lives in. + +**Two names resolving to one skill is fatal, which is why io-cli withholds +rather than overwrites.** io-harness addresses a skill by name, and a directory +holding two of the same name fails discovery outright — not as a listing quirk, +but as an error raised at the start of a run, so *every turn of that session* +dies before the first completion. The resolved name is the `name:` in a file's +frontmatter where there is one, not the filename, so a file of yours called +anything at all can claim `io-mcp`. io-cli therefore reads the directory before +it writes to it, and never installs a shipped skill over a name your own files +already claim: it installs four instead of five, and says which one it withheld +and which file claimed the name. Rename yours, or leave it — the choice stays +with the file you wrote. + +**And there is a ceiling: io-harness accepts at most 64 skills in a directory.** +It rejects the whole set rather than trimming it, so an operator sitting near +the limit who gains five more would otherwise get no skills at all as their +upgrade. io-cli counts first, installs up to the ceiling and stops, and says how +many it installed and how many it withheld. + ## The fleet An agent can break a task into sub-agents and run them over the same workspace. @@ -774,6 +858,14 @@ and the skills directory, which is `~/.io-cli/skills` when `skills` names none. `/remember` writes when you pick that scope. That is one directory to copy to a new machine, and one path to put in a bug report. +Two more paths arrive with the shipped skills. `~/.io-cli/skills/disabled/` holds +the ones that are turned off, which is a directory rather than a setting — see +[Skills](#skills). And `~/.io-cli/.skills-manifest` is where io-cli records the +bytes it last wrote for each shipped skill, so an upgrade can tell an untouched +file from one you edited. It sits in the home and deliberately *not* in the +skills directory, because every markdown file in there is offered to the model +and a state file is not a skill. + The file is found in this order, which is io-harness's and is unchanged: `$IO_CONFIG`, else `$IO_CONFIG_HOME/io.toml`, else `$XDG_CONFIG_HOME/io/io.toml` or `~/.config/io/io.toml`, and `%APPDATA%\io\io.toml` on Windows. What 0.15.0 @@ -917,7 +1009,9 @@ nothing reaches this program to draw. into prompt text, so nothing but this program is involved; a skill is read by the *model*, through a tool, under the run's own policy. Choosing one from the palette puts `use the skill: ` in your prompt and stops there. io-cli -parses no skill file and keeps no copy of one. +parses no skill file: the five it ships from 0.19.0 are bodies it carries and +writes to disk, and after that they are read the same way yours are — by the +model, through the tool, under the policy. **`io exec` runs one goal and stops, and a run that pauses stays paused.** An agent that asks a question about what you meant, or proposes a plan, ends the run diff --git a/tests/docs.rs b/tests/docs.rs index 2f2eea2..058dc20 100644 --- a/tests/docs.rs +++ b/tests/docs.rs @@ -125,6 +125,78 @@ fn the_readme_command_table_is_the_command_table() { assert_eq!(rows, COMMANDS.len()); } +/// The README's account of the skills this crate ships. +/// +/// The names are read out of `skills/` rather than listed here, so a sixth +/// shipped skill fails this test instead of arriving undocumented — which is the +/// shape of drift the command table already has a gate for and this section did +/// not. The resolved name is the frontmatter `name:` and not the filename, +/// because that is the name io-harness addresses a skill by and therefore the +/// name an operator has to avoid claiming. +/// +/// The two rules asserted under it are the ones an operator can only learn from +/// prose: a duplicate name is fatal at the harness level, and there is a ceiling +/// that rejects the whole set rather than the excess. Both are asserted on a +/// short distinctive phrase rather than on a sentence, because a sentence gets +/// reworded and a gate that fails on an improvement teaches people to delete +/// gates. +#[test] +fn the_readme_documents_every_skill_this_crate_ships() { + let readme = read("README.md"); + let (_, section) = readme + .split_once("## Skills") + .expect("the README should have a section about the shipped skills"); + let section = section.split("\n## ").next().unwrap_or(section); + + let mut shipped = 0; + for entry in std::fs::read_dir(repo().join("skills")).expect("the shipped skills directory") { + let path = entry.expect("a directory entry").path(); + if path.extension().and_then(|end| end.to_str()) != Some("md") { + continue; + } + let body = std::fs::read_to_string(&path).expect("a skill body"); + let name = body + .lines() + .find_map(|line| line.strip_prefix("name:")) + .unwrap_or_else(|| panic!("{} declares no name in its frontmatter", path.display())) + .trim(); + assert!( + section.contains(&format!("`{name}`")), + "the README should name the {name} skill and say what it is for", + ); + shipped += 1; + } + assert!(shipped > 0, "there should be skills to document"); + + // The collision rule, in both halves: what it costs, and what io-cli does + // about it. Half of it is worse than none — "io-cli withholds a colliding + // skill" without the consequence reads as tidiness rather than as the + // session-killer it avoids. + assert!( + section.contains("every turn of that session"), + "the README should say what a duplicate skill name costs", + ); + assert!( + section.contains("frontmatter"), + "the README should say the claimed name is the frontmatter name, not the filename", + ); + assert!( + section.contains("withheld"), + "the README should say io-cli withholds rather than overwrites a claimed name", + ); + + // And the ceiling, with the number, because "a limit on skills" is not + // actionable and 64 is. + assert!( + section.contains("64 skills"), + "the README should name the 64-skill ceiling", + ); + assert!( + section.contains("rejects the whole set"), + "the README should say the ceiling rejects the set rather than trimming it", + ); +} + /// The `[app.io-cli]` table in the README, sliced by the prose either side of it. /// /// It has no `` markers because it is not generated — the descriptions are @@ -523,6 +595,40 @@ fn the_changelog_has_a_section_for_this_version() { ); } +/// The 0.19.0 section, and the one thing in it a reader cannot find out any +/// other way. +/// +/// A command that changes group changes where an operator finds it: nothing +/// about `/mcp` or `/provider` was rewritten, so the only trace of the move is +/// that `/help` lists them somewhere else than it did yesterday. A release note +/// that leaves that out has documented the features and hidden the one change +/// that will make somebody think a command was removed. Pinned to `0.19.0` +/// rather than to `CARGO_PKG_VERSION`, because this is a fact about one release +/// and not a rule about every one. +#[test] +fn the_changelog_says_which_commands_changed_group() { + let changelog = read("CHANGELOG.md"); + let start = changelog + .find("## [0.19.0]") + .expect("CHANGELOG.md should have a 0.19.0 section"); + let rest = &changelog[start..]; + let body = rest + .split_once("\n## [") + .map(|(body, _)| body) + .unwrap_or(rest); + + for moved in ["`/mcp`", "`/provider`"] { + assert!( + body.contains(moved), + "the 0.19.0 section should say {moved} changed group", + ); + } + assert!( + body.contains("configure"), + "the 0.19.0 section should say which group they moved to", + ); +} + #[test] fn the_readme_states_what_the_checksum_does_not_defend_against() { // N7's own words: the README says this rather than implying more. A `curl | @@ -558,10 +664,17 @@ fn the_readme_says_where_io_keeps_its_files() { .expect("the home is a directory under the operator's own") .to_string_lossy() .into_owned(); + // The last two arrived with 0.19.0, and they are here rather than in the + // skills section's own gate because this is the paragraph an operator reads + // when they are asking what io put on their disk. `disabled/` is a directory + // that changes behaviour, and the manifest is a dotfile they did not create; + // both are things to find named somewhere before finding them by accident. for named in [ format!("`~/{dir}`"), format!("`%USERPROFILE%\\{dir}`"), format!("`~/{dir}/skills`"), + format!("`~/{dir}/skills/disabled/`"), + format!("`~/{dir}/.skills-manifest`"), ] { assert!( section.contains(&named), From ec0b6473c96139a9896490b83db317ca34eec0ea Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Thu, 27 Aug 2026 00:56:48 +0530 Subject: [PATCH 07/10] test: two gates read code rather than prose MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both refused src/skills.rs for explaining itself. The module says it writes a plain-text manifest rather than TOML, and that it decides freshness by bytes rather than by a clock — and saying either means naming the call it is deliberately not making. dependencies.rs already stripped comments for src/edit.rs, for exactly this reason, and timing.rs already built its own needles with format! so that it would not match itself. Each had solved the problem for one file and not for the files it scans. Line-oriented, so a // inside a string literal still counts as code: the stripping can only make either gate stricter. --- tests/dependencies.rs | 32 ++++++++++++++++++++++++-------- tests/timing.rs | 14 ++++++++++++++ 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/tests/dependencies.rs b/tests/dependencies.rs index a4b29e8..1e8aaca 100644 --- a/tests/dependencies.rs +++ b/tests/dependencies.rs @@ -567,6 +567,19 @@ fn f5_the_spawn_is_unreachable_from_the_event_path() { ); } +/// A source file with every comment line taken out. +/// +/// Line-oriented and deliberately not a parser: a `//` inside a string literal +/// survives, which is the safe direction for a gate — it can only make the check +/// stricter, never blinder. +fn code_of(text: &str) -> String { + text.lines() + .map(str::trim_start) + .filter(|line| !line.starts_with("//")) + .collect::>() + .join("\n") +} + #[test] fn f7_the_configuration_is_read_through_the_harness_and_never_parsed_here() { // io-harness owns discovery, layering and validation. This crate serializes @@ -590,16 +603,24 @@ fn f7_the_configuration_is_read_through_the_harness_and_never_parsed_here() { // it is asserted rather than promised in a comment. let editor = PathBuf::from("src/edit.rs"); for (path, text) in sources() { + // **Comments stripped for the sweep too, and 0.19.0 is why.** The rule + // below already strips them for `src/edit.rs`, for the reason written + // there — a gate that reads prose forbids a file from explaining itself. + // The same thing is true of every other file, and `src/skills.rs` found + // it: it writes a plain-text manifest and says so, and saying so means + // naming the call it is deliberately not making. The property is about + // what the code does, so the check reads code. + let code = code_of(&text); let permitted = path.ends_with(&editor); if !permitted { assert!( - !text.contains("toml::from_str"), + !code.contains("toml::from_str"), "{} parses TOML itself; configuration is io-harness's", path.display(), ); } assert!( - !text.contains("toml::de::"), + !code.contains("toml::de::"), "{} reaches into TOML deserialization", path.display(), ); @@ -616,12 +637,7 @@ fn f7_the_configuration_is_read_through_the_harness_and_never_parsed_here() { // may not reach for a configuration type, which means naming several of // them. A gate that read prose would forbid the file from explaining itself, // and the property is about what the code does. - let editor_code: String = editor_text - .lines() - .map(|line| line.trim_start()) - .filter(|line| !line.starts_with("//")) - .collect::>() - .join("\n"); + let editor_code = code_of(&editor_text); for named in [ "io_harness::Config", diff --git a/tests/timing.rs b/tests/timing.rs index cde17fb..f63cadf 100644 --- a/tests/timing.rs +++ b/tests/timing.rs @@ -234,6 +234,20 @@ fn n1_the_driver_is_the_only_thing_that_reads_a_clock() { } for (needle, why) in &needles { for (number, line) in source.lines().enumerate() { + // **A comment is not a clock read, and 0.19.0 is why this line + // is here.** `clock_reads` already builds its own needles with + // `format!` so that *this* file can name them without matching + // itself — the same problem, solved for the scanner and not for + // the scanned. `src/skills.rs` explains that it decides freshness + // by bytes rather than by a clock, which means naming the three + // calls it is deliberately not making, and a gate that read prose + // would forbid a module from documenting the rule it obeys. + // + // Line-oriented on purpose: a `//` inside a string literal still + // counts as code here, which can only make the gate stricter. + if line.trim_start().starts_with("//") { + continue; + } if line.contains(needle.as_str()) { readers.push(format!( "{}:{}: {needle} — {why}", From d2042fc1a8cb33b6099ccdfc424cb2eb4c4dda9e Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Thu, 27 Aug 2026 00:56:48 +0530 Subject: [PATCH 08/10] chore: bump to 0.19.0 --- Cargo.lock | 2 +- Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ef205eb..1f50557 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1261,7 +1261,7 @@ dependencies = [ [[package]] name = "io-cli" -version = "0.18.0" +version = "0.19.0" dependencies = [ "clap", "crossterm 0.28.1", diff --git a/Cargo.toml b/Cargo.toml index 9280faa..e5c2bb4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "io-cli" -version = "0.18.0" +version = "0.19.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 From 07971c72974d5a7f2f8ef9e63cdee12ed5e88348 Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Thu, 27 Aug 2026 01:21:50 +0530 Subject: [PATCH 09/10] fix(skills): four defects an adversarial review found behind a green suite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every one of them is the shape that pass is briefed to hunt: the tests exercise each feature once, and the defect appears the second time. A session-killer first. relocate guards its destination by FILE NAME while a skill is addressed by its RESOLVED name, so a file moved back out of disabled/ could land beside a different file already answering to its name — and two skills of one name is an Err from Skills::discover that io-harness propagates at run start, killing every turn before the first completion. /skills could not report it either, because its list comes from the call that just failed. Two of the operator's own files are enough. enable now asks the question the run asks. The same asymmetry one door earlier: install skipped a disabled skill by testing disabled/.md, which misses an operator file whose frontmatter claims the name. A deleted skill came back on the next start, while the release record and the README both told the operator that rm was how to be rid of one. The manifest already knew io-cli had written that name; now it is asked. The / palette's skill list is walked once at startup. Nothing changed that directory from inside a session until this release, so it now re-walks after a toggle rather than offering a skill the model's catalogue no longer has. And /skills read the home rather than the directory contract::skills_dir resolves, so with [run] skills configured it would have listed five files nothing reads and hidden the ones in force. Three controls added, each verified to fail alone under its own sabotage. --- README.md | 9 ++- src/main.rs | 165 +++++++++++++++++++++++++++++++++++++-------- src/skills.rs | 54 +++++++++++++-- src/skillview.rs | 54 +++++++++++++-- tests/skills.rs | 102 +++++++++++++++++++++++++++- tests/skillview.rs | 113 +++++++++++++++++++++++++++++-- 6 files changed, 446 insertions(+), 51 deletions(-) diff --git a/README.md b/README.md index 812be5b..3c49b24 100644 --- a/README.md +++ b/README.md @@ -475,7 +475,14 @@ bodies written into `~/.io-cli/skills`, beside whatever skills you keep there yourself. Open one, read it, edit it, copy it into a skill of your own, delete it — the same things you would do to any other markdown file in a directory you own. There is no registry, no index and no remote source; the five are carried -in the binary and written out on the first run that finds them missing. +in the binary and written out the first time io has a home to put them in. + +**Delete one and it stays deleted.** `rm ~/.io-cli/skills/io-mcp.md` is the way +to be rid of a shipped skill for good: io remembers that it wrote that name, so +it does not put the file back on the next start. A skill added in a *later* +version has no such record, so upgrading still brings you the new ones. If you +only want one out of the way for now, turn it off instead — that is reversible +and `/skills` does it for you. **Each of them ends in a change you see before it lands.** A skill instructs the model in what io can already do and which surface does it, so what comes back is diff --git a/src/main.rs b/src/main.rs index e0de4ed..8216ebe 100644 --- a/src/main.rs +++ b/src/main.rs @@ -98,6 +98,14 @@ async fn run(report: &mut Vec) -> Result { // It cannot fail the run. `install` returns report lines and never an error // for exactly that reason — a read-only directory nobody has heard of is not // a reason to refuse to start. + // + // **Into io-cli's own home, and NOT into a directory the operator chose.** + // `[run] skills` and `[app.io-cli] skills` both beat the default, and writing + // five files into whatever a team pointed at — a checked-out repository of + // shared skills, say — is not something a version bump should do quietly. The + // consequence is stated rather than hidden: where the two differ, the report + // says so below, and `/skills` lists the directory in force rather than this + // one. if adopted.is_some() { if let Some(home) = io_cli::home::path() { report.extend(io_cli::skills::install(&home)); @@ -342,6 +350,20 @@ async fn drive( if let Some(complaint) = complaint { notices.push(complaint); } + // **The one case where io-cli wrote its skills somewhere the run will not + // read**, said once, here, where the resolved directory is finally known. + // Silence would leave an operator with a startup line saying five skills were + // installed and a model that has never heard of them. + if let (Some(home), Some(in_force)) = (io_cli::home::path(), skills_dir.as_deref()) { + let ours = io_cli::skills::dir(&home); + if ours.is_dir() && ours != in_force { + notices.push(format!( + "the skills in force are in {} — io's own are in {} and are not being read", + in_force.display(), + ours.display(), + )); + } + } // A server named in both `[[mcp]]` and `[[app.io-cli.mcp]]` is reconciled on // every turn and reported here, once. The file does not change while the // session runs, so the sentence is the same on the fiftieth turn as on the @@ -395,6 +417,28 @@ async fn drive( .await? } +/// The `/skills` view over the directory the RUN reads. +/// +/// Resolved through `contract::skills_dir` on every call rather than held: it is +/// derived from the configuration, the configuration is re-discovered at each +/// turn boundary, and a surface holding a directory decided at startup would go +/// on listing one an operator has since pointed away from. The home is a separate +/// argument because the manifest — which is what decides whose a file is — lives +/// there and not in the skills directory. +fn skills_view( + config: &Config, + capabilities: &io_cli::contract::Capabilities, + root: &std::path::Path, +) -> io_cli::skillview::View { + let Some(home) = io_cli::home::path() else { + return io_cli::skillview::View::default(); + }; + match io_cli::contract::skills_dir(config, capabilities, root.to_path_buf()) { + Some(dir) => io_cli::skillview::view(&home, &dir), + None => io_cli::skillview::View::default(), + } +} + /// The interactive session, as something [`provider::build`] can run. struct Interactive<'a, 'b> { screen: &'a mut Screen>, @@ -510,7 +554,13 @@ async fn loop_over Result>( // `Config` would leave every `[app.io-cli]` answer stale while the rest of // the session moved on. mut capabilities: io_cli::contract::Capabilities, - skills: io_harness::Skills, + // Mutable from 0.19.0, and for a reason that did not exist before it: this is + // the list the `/` palette offers, walked once at startup, and until this + // release the directory behind it only ever changed out of band. `/skills` + // now turns one off and on from inside the session, so a list that stayed put + // would go on offering a skill the model's catalogue no longer has — and + // `read_skill` would refuse it by name. + mut skills: io_harness::Skills, // The named profile in force. `--profile` seeds it and `/profile` replaces // it; it is re-applied after every turn-boundary reload, which goes back to // the file and knows nothing about either. @@ -863,11 +913,30 @@ async fn loop_over Result>( // the row being drawn and this keystroke the operator may // have moved the file themselves, and a carried row would // then name a path that is no longer there. - Pick::Skills => { - let view = io_cli::home::path() - .map(|home| io_cli::skillview::view(&home)) - .unwrap_or_default(); - if let Some(skill) = view.skills.get(index) { + Pick::Skills(drawn) => { + // **Located by what the row said, not by where it + // sat.** The list is read again — the operator may + // have moved a file in another pane between drawing + // the rows and choosing one — and an index carried + // across two different readings of a directory names + // whichever skill happens to be in that position now. + // Getting a *different* skill than the row you read, + // silently, is worse than being told it moved. + let drawn = drawn.get(index).cloned(); + let view = skills_view(&config, &capabilities, session.root()); + let found = drawn.as_ref().and_then(|(name, path)| { + view.skills + .iter() + .find(|skill| &skill.name == name && &skill.path == path) + }); + if found.is_none() && drawn.is_some() { + app.record( + Tone::Muted, + "that skill is not where it was a moment ago; \ + open `/skills` again", + ); + } + if let Some(skill) = found { app.record( Tone::Muted, format!( @@ -922,16 +991,35 @@ async fn loop_over Result>( io_cli::skillview::enable(path) }; match moved { - Ok(to) => app.record( - Tone::Muted, - format!( - "{name} is now {} — {}. The next turn is composed \ - from the directory as it is then, so this is in \ - force immediately.", - if *enabled { "off" } else { "on" }, - to.display(), - ), - ), + Ok(to) => { + // **The palette's list is walked once at + // startup and this is the only thing in + // the product that changes the directory + // under it.** Leave it and `/` goes on + // offering a skill the model's catalogue + // no longer has, whose `read_skill` the + // harness refuses by name — or hides one + // that has just come back. + skills = io_cli::commands::skills( + io_cli::contract::skills_dir( + &config, + &capabilities, + session.root().to_path_buf(), + ) + .as_deref(), + ) + .0; + app.record( + Tone::Muted, + format!( + "{name} is now {} — {}. The next turn is composed \ + from the directory as it is then, so this is in \ + force immediately.", + if *enabled { "off" } else { "on" }, + to.display(), + ), + ); + } Err(why) => app.record(Tone::Refused, why), } } @@ -1395,18 +1483,31 @@ async fn loop_over Result>( // a directory discovery never looks at and is exactly where the // operator's next move probably is. Action::Skills => { - let view = io_cli::home::path() - .map(|home| io_cli::skillview::view(&home)) - .unwrap_or_default(); + let view = skills_view(&config, &capabilities, session.root()); if let Some(sentence) = &view.failed { app.record(Tone::Refused, sentence.clone()); } if view.skills.is_empty() { if view.failed.is_none() { + // **Not "on the next start".** `install` already ran + // at the top of this one, so an empty list means it + // was skipped or could not write — and promising a + // fix on the next start promises a thing that will + // happen again exactly as it just did. Name the + // directory instead, which is the fact the operator + // can act on. app.record( Tone::Muted, - "no skills are installed; `io` writes its own into \ - the home on the next start", + match io_cli::contract::skills_dir( + &config, + &capabilities, + session.root().to_path_buf(), + ) { + Some(dir) => { + format!("no skills in {}", dir.display()) + } + None => "no skills directory is in force".to_string(), + }, ); } } else { @@ -1419,7 +1520,12 @@ async fn loop_over Result>( &app.theme.glyphs, ), ), - Pick::Skills, + Pick::Skills( + view.skills + .iter() + .map(|skill| (skill.name.clone(), skill.path.clone())) + .collect(), + ), )); } } @@ -3227,11 +3333,16 @@ enum Pick { /// The provider chain, in the order `providers::rows` drew it — which is /// the order a turn tries it. Provider, - /// Every skill, in the order `skillview::rows` drew them. No list is carried: - /// the view is read again when a row is chosen, because between the two - /// keystrokes the operator may have edited the directory in another pane and - /// a stale row would name a file that has moved. - Skills, + /// Every skill, in the order `skillview::rows` drew them, carried as the + /// `(name, path)` each row stood for. + /// + /// **Carried rather than re-derived by index**, which is the rule every other + /// list-bearing variant here follows. The view IS read again when a row is + /// chosen — the directory can change between two keystrokes — and an index + /// held across two readings of it addresses whichever skill is in that + /// position now. So the pair says which skill the operator actually read, and + /// a row that is no longer there is answered with a sentence. + Skills(Vec<(String, std::path::PathBuf)>), /// One skill, and the move that decides whether the model is offered it. /// /// **Two steps rather than one, and for the reason [`Pick::ConfigScope`] diff --git a/src/skills.rs b/src/skills.rs index 75d7bd1..81e3b91 100644 --- a/src/skills.rs +++ b/src/skills.rs @@ -244,6 +244,27 @@ pub fn wrote(home: &Path, name: &str, path: &Path) -> bool { std::fs::read(path).is_ok_and(|bytes| digest(&bytes) == hash) } +/// Every name the files in `disabled` resolve to. +/// +/// A directory that is not there is no disabled skills, which is the ordinary +/// case — `disabled/` is created by the first disable and never before. Resolved +/// through [`crate::skillview::describe`] rather than from the file stem, because +/// the stem and the name are exactly the two things this release keeps apart. +fn disabled_names(disabled: &Path) -> Vec { + let Ok(entries) = std::fs::read_dir(disabled) else { + return Vec::new(); + }; + entries + .filter_map(Result::ok) + .map(|entry| entry.path()) + .filter(|path| { + path.extension() + .is_some_and(|extension| extension.eq_ignore_ascii_case("md")) + }) + .map(|path| crate::skillview::describe(&path).0) + .collect() +} + /// `n skills`, or `1 skill`. fn many(n: usize) -> String { if n == 1 { @@ -314,8 +335,16 @@ pub fn install(home: &Path) -> Vec { // reads counts first and exceptions after. let mut notes: Vec = Vec::new(); + // **Every name the disabled directory answers to, not every file name in it.** + // A disabled file's resolved name comes from its frontmatter like any other, + // so testing `disabled/.md` alone would miss an operator's `mine.md` + // that declares `name: io-mcp` — and then install its own `io-mcp.md`, which + // turns the operator's next "turn it back on" into two files answering to one + // name and a session in which every turn dies. Read once, before the loop. + let turned_off = disabled_names(&disabled); + for skill in &SHIPPED { - if disabled.join(file_name(skill.name)).exists() { + if turned_off.iter().any(|name| name == skill.name) { continue; } @@ -332,6 +361,16 @@ pub fn install(home: &Path) -> Vec { }; let Some(bytes) = existing else { + // **A name io-cli has written before and whose file is now gone was + // deleted on purpose.** `rm ~/.io-cli/skills/io-mcp.md` is the + // documented way to be rid of one, and an installer that wrote it back + // on the next start would make deletion a thing that undoes itself — + // the same failure the disabled check above exists to prevent, through + // the other door. A name with no record at all is one this release + // never shipped, so a skill added in a later version still installs. + if recorded_hash(&manifest, skill.name).is_some() { + continue; + } // Nothing of ours at that path — but the *name* may still be taken, // and taking it anyway is `Error::Config` at run start rather than a // listing quirk. `target` does not exist, so anything discovery @@ -368,11 +407,14 @@ pub fn install(home: &Path) -> Vec { // forever. No entry at all means io-cli has no record of writing this // file, which is the operator's file however it is spelled. if recorded_hash(&manifest, skill.name) != Some(digest(&bytes)) { - notes.push(format!( - "kept {}, which has been edited; io-cli's own `{}` was not written over it", - target.display(), - skill.name, - )); + // **Left alone, and said nothing about.** A kept edit is a settled + // state, not an event: the operator changed the file deliberately and + // it is theirs from then on, so a line here would be printed at every + // start for the rest of that file's life — and on stderr at every + // `io exec`, which a scripted loop runs a great many times. The + // standing place for it is `/skills`, where the row already reads + // `yours`. A claimed NAME is different and is still reported below: + // that is an unresolved hazard rather than a settled state. continue; } // Untouched and already the current text: no write, so no churned mtime diff --git a/src/skillview.rs b/src/skillview.rs index 499d5cf..39d8723 100644 --- a/src/skillview.rs +++ b/src/skillview.rs @@ -157,13 +157,23 @@ pub struct View { pub failed: Option, } -/// Every skill in `/skills` and `/skills/disabled`. +/// Every skill in `dir` and in `dir/disabled`. +/// +/// **`dir` is the directory the RUN reads, and it is not always io-cli's own.** +/// `[run] skills` and `[app.io-cli] skills` both beat the `~/.io-cli/skills` +/// default, so a surface that walked the home would list five files the model is +/// never offered and hide the ones it is — which would make this module's whole +/// premise false. The caller resolves it through `crate::contract::skills_dir`, +/// the same call that decides what the turn is handed. +/// +/// `home` is still needed and is a different question: it is where the manifest +/// lives, and the manifest is what decides whose a file is. #[must_use] -pub fn view(home: &Path) -> View { +pub fn view(home: &Path, dir: &Path) -> View { let mut listed = Vec::new(); let mut failed = None; - match io_harness::Skills::discover(skills::dir(home)) { + match io_harness::Skills::discover(dir) { Ok(found) => listed.extend(found.iter().map(|skill| Listed { origin: origin(home, &skill.name, &skill.path), name: skill.name.clone(), @@ -174,7 +184,7 @@ pub fn view(home: &Path) -> View { Err(error) => failed = Some(error.to_string()), } - for path in disabled_files(home) { + for path in disabled_files(dir) { let (name, description) = describe(&path); listed.push(Listed { origin: origin(home, &name, &path), @@ -216,8 +226,8 @@ fn origin(home: &Path, name: &str, path: &Path) -> Origin { /// Canonicalised the way `Skills::discover` canonicalises, so a path off this /// surface is comparable with a path off that one and neither depends on the /// process's working directory. -fn disabled_files(home: &Path) -> Vec { - let Ok(entries) = std::fs::read_dir(skills::disabled_dir(home)) else { +fn disabled_files(dir: &Path) -> Vec { + let Ok(entries) = std::fs::read_dir(dir.join(skills::DISABLED)) else { return Vec::new(); }; let mut files: Vec = entries @@ -240,7 +250,12 @@ fn disabled_files(home: &Path) -> Vec { /// The same fallbacks the harness uses, because a file that is disabled today was /// enabled yesterday and will be enabled again tomorrow: a skill that changed its /// name by being turned off would be a different skill on the way back. -fn describe(path: &Path) -> (String, String) { +/// +/// `pub(crate)` because [`crate::skills::install`] asks the same question of the +/// same directory: it has to know whether a *disabled* file already answers to a +/// name before it writes its own file under that name, and answering it any other +/// way would be a second reading of what a skill is called. +pub(crate) fn describe(path: &Path) -> (String, String) { let stem = path .file_stem() .map(|stem| stem.to_string_lossy().to_string()) @@ -443,6 +458,31 @@ pub fn enable(path: &Path) -> Result { path.display() )); }; + // **The destination guard in `relocate` is by FILE NAME, and a skill is + // addressed by its RESOLVED name.** That asymmetry is the whole subject of + // this release, and it is reachable from here with two keystrokes: disable an + // operator's `mine.md` that declares `name: io-mcp`, restart — `install` no + // longer sees a claimant, because the claimant is in a directory discovery + // cannot look into, so it writes its own `io-mcp.md` — then enable `mine.md` + // again. `relocate` would find no `mine.md` in the way and move it, and the + // directory would hold two files answering to `io-mcp`. `Skills::discover` + // returns `Err` on that, io-harness propagates it at run start, and every turn + // of the session is dead before the first completion — with `/skills` itself + // unable to help, because its list comes from the call that just failed. + // + // So the question asked here is the one the run will ask: does anything in + // that directory already answer to this file's name? + let (name, _) = describe(path); + if let Ok(found) = io_harness::Skills::discover(dir) { + if let Some(claimant) = found.get(&name) { + return Err(format!( + "{} already answers to `{name}`, so io-cli did not move {} beside it — \ + two skills of one name end every turn of the session", + claimant.path.display(), + path.display(), + )); + } + } relocate(path, dir) } diff --git a/tests/skills.rs b/tests/skills.rs index 6988557..3bb3960 100644 --- a/tests/skills.rs +++ b/tests/skills.rs @@ -416,11 +416,22 @@ fn f4_an_untouched_skill_is_refreshed_and_an_edited_one_is_left_byte_for_byte() "a file io-cli has no record of writing was overwritten anyway", ); + // **And it says nothing about either of them.** A kept edit is a settled + // state, not an event: the operator changed the file on purpose and it is + // theirs from then on, so a line here would print at every start for the rest + // of that file's life — and on stderr at every `io exec`, which a script runs + // a great many times. `delivery.observability` in the release contract asks + // for silence when nothing changed, and a file that is being left alone did + // not change. `/skills` is the standing place for it, where the row reads + // `yours`. A claimed NAME is the opposite case and is still reported — see + // `f2_...`, which asserts that line — because that is an unresolved hazard + // rather than a settled state. let said = report.join("\n"); for kept in ["io-provider.md", "io-remember.md"] { assert!( - said.contains(kept), - "the report does not name {kept} as kept: {report:?}", + !said.contains(kept), + "the report names {kept} on a run that only left it alone, and will do so \ + again at every start from now on: {report:?}", ); } assert!( @@ -451,8 +462,32 @@ fn f5_a_disabled_skill_is_not_resurrected_by_the_next_run() { // Twice: a restart, and then an upgrade that has something else to do. skills::install(&home); - std::fs::remove_file(dir.join("io-update.md")).expect("something for the second run to do"); + // **The something else is a stale file, deliberately not a deleted one.** A + // deleted shipped skill is not reinstalled — `rm` is the documented way to be + // rid of one, and an installer that wrote it back would make deletion undo + // itself, which is this very test's complaint about disabling through the + // other door. So `io-update` is left on disk with older bytes and a manifest + // entry that matches them, which is exactly what an operator who has touched + // nothing looks like one release later. + let stale = dir.join("io-update.md"); + std::fs::write(&stale, "stale\n").expect("older bytes"); + let mut entries = manifest(&home); + for entry in &mut entries { + if entry.0 == "io-update" { + entry.1 = skills::digest(b"stale\n"); + } + } + let manifest_text: String = entries + .iter() + .map(|(name, hash)| format!("{name}\t{hash:016x}\n")) + .collect(); + std::fs::write(home.join(".skills-manifest"), manifest_text).expect("the manifest"); skills::install(&home); + assert_ne!( + read(&stale), + "stale\n", + "the second run had nothing to do, so this test proves nothing about an upgrade", + ); assert!( !was.exists(), @@ -589,3 +624,64 @@ fn provenance_comes_from_the_manifest_and_never_from_the_name() { "a shipped skill the operator has edited still reads as io-cli's", ); } + +/// **A shipped skill the operator deleted is not written back.** +/// +/// `rm ~/.io-cli/skills/io-mcp.md` is the documented way to be rid of one — the +/// release record says so in `delivery.migrations` and again in +/// `delivery.rollback`, and the README says it to the operator. An installer that +/// wrote the file back on the next start would make deletion a thing that undoes +/// itself, which is the same failure the disabled check prevents through the +/// other door, and the operator's only recourse would be to delete it again after +/// every launch. +/// +/// The record is the manifest: io-cli knows it wrote that name, and the file is +/// gone, so it was taken away on purpose. **A name with no record at all is a +/// skill this version ships and an earlier one did not**, which is what keeps an +/// upgrade able to deliver new skills — asserted here too, because a fix that +/// stopped installing anything would also pass the first half. +/// +/// Sabotage: drop the manifest check from the absent-file branch of `install`. +/// Under it only this test fails, on the second start rather than the first. +#[test] +fn a_deleted_shipped_skill_is_not_written_back_on_the_next_run() { + let (_dir, home) = home(); + let dir = skills::dir(&home); + skills::install(&home); + + let gone = dir.join("io-mcp.md"); + std::fs::remove_file(&gone).expect("the operator deletes one"); + + let report = skills::install(&home); + assert!( + !gone.exists(), + "io-cli wrote back a skill the operator deleted; deleting it is now a thing \ + they have to do at every start", + ); + assert!( + report.is_empty(), + "and it said something about a run in which it did nothing: {report:?}", + ); + + // The other half: a name io-cli has no record of is a skill a later version + // added, and it still installs. Simulated by taking `io-remember` out of the + // manifest while leaving no file — which is exactly the state an upgrade that + // ships a sixth skill is in for that sixth name. + let entries: Vec<(String, u64)> = manifest(&home) + .into_iter() + .filter(|(name, _)| name != "io-remember") + .collect(); + let text: String = entries + .iter() + .map(|(name, hash)| format!("{name}\t{hash:016x}\n")) + .collect(); + std::fs::write(home.join(".skills-manifest"), text).expect("the manifest"); + std::fs::remove_file(dir.join("io-remember.md")).expect("no file for that name either"); + + skills::install(&home); + assert!( + dir.join("io-remember.md").is_file(), + "a name io-cli has never recorded is one a later version added, and it must \ + still install — otherwise an upgrade can never deliver a new skill", + ); +} diff --git a/tests/skillview.rs b/tests/skillview.rs index 7b82ef4..adeda01 100644 --- a/tests/skillview.rs +++ b/tests/skillview.rs @@ -111,7 +111,7 @@ fn f6_origin_is_decided_by_the_manifest_and_never_by_the_io_prefix() { // Only the un-prefixed one is recorded, so the prefix and the truth disagree. recorded(&home, &[("mine", ours.as_path())]); - let view = skillview::view(&home); + let view = skillview::view(&home, &skills::dir(&home)); assert_eq!(view.failed, None, "the fixture discovers"); assert_eq!(listed(&view, "mine").origin, Origin::IoCli); assert_eq!(listed(&view, "io-thing").origin, Origin::Yours); @@ -121,7 +121,7 @@ fn f6_origin_is_decided_by_the_manifest_and_never_by_the_io_prefix() { // edited is theirs. The bytes no longer hash to what io-cli recorded, which is // true, and is also what the next upgrade will decide about it. write(&ours, &skill("mine", "Edited by the operator.")); - let view = skillview::view(&home); + let view = skillview::view(&home, &skills::dir(&home)); assert_eq!( listed(&view, "mine").origin, Origin::Yours, @@ -136,7 +136,7 @@ fn f6_a_skill_carries_its_description_its_state_and_the_file_it_lives_in() { let path = dir.join("alpha.md"); write(&path, &skill("alpha", "Does the alpha thing.")); - let view = skillview::view(&home); + let view = skillview::view(&home, &skills::dir(&home)); let row = listed(&view, "alpha"); assert_eq!(row.description, "Does the alpha thing."); assert!(row.enabled, "a file in skills/ is offered to the model"); @@ -156,7 +156,7 @@ fn f6_a_disabled_skill_is_listed_as_disabled_with_its_path_inside_disabled() { let moved = skillview::disable(&path).expect("the move"); - let view = skillview::view(&home); + let view = skillview::view(&home, &skills::dir(&home)); let row = listed(&view, "alpha"); assert!(!row.enabled, "a file under disabled/ is not offered"); // Canonicalised on both sides: the surface resolves a path the way @@ -193,7 +193,7 @@ fn f6_a_directory_that_will_not_discover_shows_the_harness_sentence() { &skill("parked", "Turned off earlier."), ); - let view = skillview::view(&home); + let view = skillview::view(&home, &skills::dir(&home)); let sentence = view .failed @@ -388,7 +388,7 @@ fn drawable() -> (tempfile::TempDir, PathBuf) { #[test] fn n4_every_row_draws_in_ascii_with_its_meaning_intact_inside_eighty_columns() { let (_dir, home) = drawable(); - let view = skillview::view(&home); + let view = skillview::view(&home, &skills::dir(&home)); assert_eq!(view.failed, None); assert_eq!(view.skills.len(), 3); @@ -428,7 +428,7 @@ fn n4_every_row_draws_in_ascii_with_its_meaning_intact_inside_eighty_columns() { #[test] fn n4_the_path_is_what_gives_way_and_the_name_and_the_state_never_do() { let (_dir, home) = drawable(); - let view = skillview::view(&home); + let view = skillview::view(&home, &skills::dir(&home)); let ascii = io_cli::glyphs::ASCII; let glyphs = &ascii; @@ -467,3 +467,102 @@ fn n4_the_path_is_what_gives_way_and_the_name_and_the_state_never_do() { "and the end of the path is what survives, since the front is shared: {details:?}" ); } + +// --------------------------------------------------------------------------- +// The adversarial review of 0.19.0 found the defect below behind a green suite +// of 1,020 tests. It is here as its own section because it is the one failure +// mode this whole release exists to prevent, reached through io-cli's own keys. +// --------------------------------------------------------------------------- + +/// **Enabling may not put a second file under a name already answered to.** +/// +/// The destination guard in `relocate` is by FILE NAME, and a skill is addressed +/// by its RESOLVED name — the asymmetry this whole release is built on. So a +/// file moving back out of `disabled/` can land beside a different file that +/// already answers to its name, with nothing in the way. `Skills::discover` then +/// returns `Err`, io-harness propagates it at run start, and every turn of the +/// session dies before the first completion. `/skills` cannot even report it: its +/// list comes from the call that just failed. +/// +/// The review that found this reached it through io-cli's own five — disable a +/// claimant, let the next start install io-cli's file into the freed name, enable +/// the claimant again. That route is now closed one step earlier, by +/// `install_skips_a_name_that_is_disabled_under_a_different_file_name` below, and +/// this test deliberately does **not** use it: a control that only fails when a +/// second guard is also removed is not a control. What is left is the route +/// neither installer guard can ever see, because io-cli never wrote either file. +/// +/// Sabotage: delete the resolved-name check in `skillview::enable`. Under it only +/// this test fails, and it fails with `discovered` panicking — which is the exact +/// sentence an operator would have got on every turn. +#[test] +fn enabling_never_puts_a_second_file_under_a_name_already_answered_to() { + let (_dir, home) = home(); + let dir = skills::dir(&home); + + // Two files of the OPERATOR'S OWN, which is where the hole survives both of + // the other guards: `install` only ever withholds files io-cli would write, + // so nothing in it looks at a pair like this one. The names are what collide; + // the file names never do, which is precisely why the destination check in + // `relocate` cannot see it. + let enabled = dir.join("a.md"); + write(&enabled, &skill("alpha", "The one they kept on.")); + let other = dir.join("b.md"); + write(&other, &skill("alpha", "The one they turned off.")); + let parked = skillview::disable(&other).expect("the move out"); + let kept = read(&parked); + + // One `alpha` in the directory, so the session is fine as it stands. + assert_eq!(discovered(&dir), vec!["alpha".to_string()]); + + // And this is the keystroke that used to end it. + let back = skillview::enable(&parked); + assert!( + back.is_err(), + "enabling moved a second `alpha` in beside the first; every turn of that \ + session is now dead at run start", + ); + let why = back.unwrap_err(); + assert!( + why.contains("alpha"), + "the refusal does not name the name that is taken: {why}", + ); + + // The operator's file is where it was, byte for byte. A refusal that also + // lost a file would be worse than the defect. + assert_eq!(read(&parked), kept, "the refused move touched the file"); + + // And the property the whole thing is about: the session still starts. + assert_eq!(discovered(&dir), vec!["alpha".to_string()]); +} + +/// **A disabled skill is skipped by the name it answers to, not by its file +/// name.** This is step 3 above, and closing it is what stops the sequence from +/// ever reaching step 4. +/// +/// Sabotage: test `disabled/.md` for existence instead of resolving the +/// names in that directory. Under it only this test fails, and io-cli writes an +/// `io-mcp.md` beside a disabled file that already answers to `io-mcp`. +#[test] +fn install_skips_a_name_that_is_disabled_under_a_different_file_name() { + let (_dir, home) = home(); + let dir = skills::dir(&home); + + let theirs = dir.join("mine.md"); + write(&theirs, &skill("io-mcp", "The operator's own.")); + skillview::disable(&theirs).expect("the move out"); + + skills::install(&home); + + assert!( + !dir.join("io-mcp.md").exists(), + "io-cli wrote its own `io-mcp` while a disabled file already answers to that name", + ); + let mut expected: Vec = skills::SHIPPED + .iter() + .map(|skill| skill.name.to_string()) + .filter(|name| name != "io-mcp") + .collect(); + expected.sort(); + assert_eq!(discovered(&dir), expected); +} From f53384cc06ec094bb291216421ffa52cc4e77c17 Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Thu, 27 Aug 2026 01:30:30 +0530 Subject: [PATCH 10/10] test: the frontmatter fence is either line ending, and the shipped bytes are LF Windows-only red, and the defect was in the test. io-harness's split_front_matter strips `---\n` and `---\r\n` alike and trims \r off every key, so a CRLF checkout reads exactly the same; the assertion took only the first form and was therefore stricter than the thing it protects. Green on two platforms and red on the third, which is the only place it could be seen. Separately, .gitattributes pins skills/*.md to LF. Those five files are include_str!'d into the binary and written to disk verbatim, so their bytes are a product artifact: without it one release ships two different sets of them and the manifest digest of a file io wrote depends on the host it was built on. Neither change stands in for the other. Remove the attribute and the assertion still passes, because the skill still works. --- .gitattributes | 10 ++++++++++ tests/skills.rs | 14 +++++++++++++- 2 files changed, 23 insertions(+), 1 deletion(-) create mode 100644 .gitattributes diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 0000000..646f262 --- /dev/null +++ b/.gitattributes @@ -0,0 +1,10 @@ +# The five skills io ships are `include_str!`'d into the binary and written to +# disk verbatim, so their bytes are a product artifact rather than a source file +# a developer reads. Without this, a Windows checkout produces CRLF, the same +# release ships two different sets of bytes, and the manifest digest of a file io +# wrote differs by the platform it was built on. +# +# io-harness reads either fence — `split_front_matter` strips `---\n` and +# `---\r\n` alike and trims `\r` off every key — so this is about the artifact +# being one artifact, not about the parser coping. +skills/*.md text eol=lf diff --git a/tests/skills.rs b/tests/skills.rs index 3bb3960..d566439 100644 --- a/tests/skills.rs +++ b/tests/skills.rs @@ -123,8 +123,20 @@ fn every_shipped_file_declares_the_name_it_is_installed_under() { declared, skill.name, "the file's frontmatter and SHIPPED disagree about what this skill is called", ); + // **Either line ending, because the property is what io-harness will + // read.** `split_front_matter` strips `---\n` and `---\r\n` alike and + // trims `\r` off every key, and the first form of this assertion took + // only the first — which is green on macOS and Linux and red on Windows, + // where the checkout is CRLF and `include_str!` gets it. That was a test + // stricter than the thing it protects, and only the matrix could see it. + // + // The bytes are also pinned to LF in `.gitattributes`, so one release + // ships one artifact rather than a different one per build host. That is + // a separate concern from this assertion and neither stands in for the + // other: remove the attribute and this still passes, which is correct, + // because the skill still works. assert!( - skill.text.starts_with("---\n"), + skill.text.starts_with("---\n") || skill.text.starts_with("---\r\n"), "{}'s frontmatter is not a fence io-harness will read; without one the name \ falls back to the file stem and the description to the first prose line", skill.name,