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/.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. 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/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 diff --git a/README.md b/README.md index 30988c7..3c49b24 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,88 @@ 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 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 +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 +865,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 +1016,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/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/commands.rs b/src/commands.rs index 6fa77f0..bd7f5f9 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", + ], ), ]; @@ -809,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. @@ -1388,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..8216ebe 100644 --- a/src/main.rs +++ b/src/main.rs @@ -77,7 +77,40 @@ 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. + // + // **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)); + } + } 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 @@ -317,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 @@ -370,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>, @@ -485,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. @@ -833,6 +908,122 @@ 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(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!( + "{} — {} · {} · {} · {}", + 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) => { + // **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), + } + } + } // 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 +1473,62 @@ 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 = 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, + 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 { + picker = Some(( + Picker::new( + "Skills", + io_cli::skillview::rows( + &view.skills, + screen.width(), + &app.theme.glyphs, + ), + ), + Pick::Skills( + view.skills + .iter() + .map(|skill| (skill.name.clone(), skill.path.clone())) + .collect(), + ), + )); + } + } Action::Provider => { let chain = io_cli::providers::chain(&config); if chain.is_empty() { @@ -3086,6 +3333,27 @@ 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, 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`] + /// 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/src/skills.rs b/src/skills.rs new file mode 100644 index 0000000..81e3b91 --- /dev/null +++ b/src/skills.rs @@ -0,0 +1,473 @@ +//! 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) +} + +/// 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 { + "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(); + + // **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 turned_off.iter().any(|name| name == skill.name) { + 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 { + // **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 + // 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)) { + // **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 + // 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/src/skillview.rs b/src/skillview.rs new file mode 100644 index 0000000..39d8723 --- /dev/null +++ b/src/skillview.rs @@ -0,0 +1,523 @@ +//! `/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 `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, dir: &Path) -> View { + let mut listed = Vec::new(); + let mut failed = None; + + 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(), + description: skill.description.clone(), + enabled: true, + path: skill.path.clone(), + })), + Err(error) => failed = Some(error.to_string()), + } + + for path in disabled_files(dir) { + 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(dir: &Path) -> Vec { + let Ok(entries) = std::fs::read_dir(dir.join(skills::DISABLED)) 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. +/// +/// `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()) + .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() + )); + }; + // **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) +} + +/// 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/commands.rs b/tests/commands.rs index 9c6efb4..1c74fba 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", @@ -80,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!( @@ -611,8 +647,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 +671,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. /// 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/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), 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"); +} diff --git a/tests/skills.rs b/tests/skills.rs new file mode 100644 index 0000000..d566439 --- /dev/null +++ b/tests/skills.rs @@ -0,0 +1,699 @@ +//! 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", + ); + // **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("---\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, + ); + } +} + +/// 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", + ); + + // **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 names {kept} on a run that only left it alone, and will do so \ + again at every start from now on: {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); + // **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(), + "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", + ); +} + +/// **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 new file mode 100644 index 0000000..adeda01 --- /dev/null +++ b/tests/skillview.rs @@ -0,0 +1,568 @@ +//! 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, &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); + 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, &skills::dir(&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, &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"); + 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, &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 + // `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, &skills::dir(&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, &skills::dir(&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, &skills::dir(&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:?}" + ); +} + +// --------------------------------------------------------------------------- +// 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); +} 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}",