diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e44b0b..f4d2686 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,114 @@ All notable changes to this project are documented here. The format follows ## [Unreleased] +## [0.21.0] - 2026-08-27 + +What you already told another agent tool, brought across once — and two surfaces +that finally do what they have been described as doing. + +**`/import` brings your setup over from the tool you were using before.** io looks +for `~/.claude` and `~/.claude.json`, `~/.codex`, `~/.gemini`, and a `.cursorrules` +or `CONVENTIONS.md` in the repository, and offers four things out of whatever it +finds: the standing instructions you wrote, which are appended whole to the memory +file for the scope you pick rather than shredded into a bullet per line; the MCP +servers you configured, translated into io-harness's `[[mcp]]` spelling; the skills +you collected, written into your skills directory; and the model you settled on. It +is offered once on a first run — io records that it asked, so it never asks twice — +and one key declines and carries straight on into the session. `/import` opens the +same thing whenever you want it. **The whole plan is on screen before a single byte +is written**, one row per thing found with where it came from and where it would go, +and you accept item by item. Declining writes nothing, and a cancelled import is not +a partial one. A tool whose files are all empty is a distinct row and says so, which +matters more than it sounds: on a good many machines every Gemini file exists and +every one is zero bytes, and an import of nothing that then reports success is the +failure you cannot see. + +**No credential is read or copied, and the code is what enforces that rather than +the intention behind it.** `~/.codex/auth.json` is not in the list of files this +program can open, so no path through it reaches one. A server's environment values +are parsed and discarded without ever being assembled into a string — only the +variable *name* is held — and what gets written is `${env:NAME}`, the name pointing +at itself, which io-harness resolves from your own environment when a run needs it. +`~/.claude.json` is a whole application's state with OAuth material in it and is +read through narrow structs, so every field io does not name is skipped by the +parser instead of being loaded and then politely ignored. + +**An allowlist is read, shown, and deliberately not translated.** Codex spells a +permission as `prefix_rule(pattern=["bun","install"], …)` and Claude as +`Bash(cargo yank *)`; both match a command line, and io-harness's `Act::Exec` +matches a binary name and nothing else. The nearest faithful import of `bun install` +is therefore a blanket allow on `bun` — a wider permission than you ever granted, +written by a tool you were trusting to be careful. io says what it found and says it +cannot express it, and produces no rule, no `[policy]` table and no policy layer. A +boundary half imported is worse than one left alone. A model id is carried rather +than written for a smaller version of the same reason: `[[provider]]` needs a vendor +and `gpt-5` does not name one, so the entry is built once you have chosen. + +**Skills are counted before any are written.** A name your directory already answers +to is refused on its own row and the rest of the import goes through; a set that +would cross io-harness's 64-skill ceiling refuses **every** skill instead, because +the harness rejects a whole directory rather than the excess and the alternative is +a session in which every turn dies at run start with nothing visible to blame. + +**A capability bundle's skills are on a surface at last.** 0.20.0 let a bundle +contribute skills and every one of them reached the model, under no row in `/skills` +and no entry in the `/` palette — a list that disagreed with the catalogue the turn +was handed. Both now carry them, spelled `__`, which is io-harness's +own namespacing and the string a refusal or a tool call will name, with the bundle +shown as the origin. Enabling or disabling one is **refused**: turning a skill off +is moving its file into a `disabled/` directory, and for a bundle skill that means +io-cli creating a directory inside somebody else's bundle and moving their file into +it. Stop the bundle with `/plugin` instead. + +**And 0.20.0 shipped a session-killer behind that omission.** `Plugin::skills_dir` +is the manifest's word joined onto the bundle root with no existence check, and the +walk that discovers skills fails the run with `?` before the first completion — so a +bundle declaring a skills directory that is not on disk ended **every turn of that +session**, with nothing anywhere naming the cause. `/skills` and `/plugin` now name +the bundle and say what it costs, one row per bundle, so a second broken bundle does +not hide behind the first and a broken one does not take down the surface that +explains it. + +**`/mcp` and `/provider` write.** Both could only list. The writer functions existed +and were tested and were called from nothing, while three places in the code and the +documentation said the two panels "add, edit, disable and remove entries" — a +sentence that has been describing an intention since 0.19.0. They now genuinely add, +edit and remove through the same staged write `/config` uses, read back by io-harness +and rolled back whole on a refusal. `/provider` also arranges the chain: promote, +demote, or add an entry, which is the fallback order io-harness has read since its +0.27.0 and that this interface has drawn an event for without ever being able to +cause one. Reordering moves an entry with its own comments and its own keys rather +than rebuilding the array. *Disable* is gone from that sentence rather than +implemented: `McpServer` has no key for it, and an `enabled = false` invented here +would be accepted by the file and ignored by the harness, so the server would start +anyway. + +**Seven defects were fixed in those writers on the way**, and one of them is the +reason `[[mcp]]` is the section to be careful in: it is one of only two io-harness +exempts from `deny_unknown_fields`, so a misspelled key spliced into an entry was +accepted by the parser, reported as written, and ignored by every turn afterwards — +the server running with the setting the operator thought they had changed still at +its default. `/mcp` refuses a key it does not know instead of writing it, and the +round-trip assertions deserialise into `McpServer` rather than looking for a string +in a file. Neither panel takes a row number either: an entry is addressed by finding +it in the file's own bytes, because a row on screen and a position in a file's array +stop agreeing the moment anything sorts or filters, and that failure is silent — it +removes a server nobody named, or bills the next turn to a vendor nobody chose. + +**`/import` is the twenty-eighth command**, and it joins the **configure** group +because it writes files, which is what that group means and what **inspect** +promises it never does. It is last in the group because it is the one command there +an operator uses once; the others are returned to for the life of the install. +Configure goes to eight. + +**One documentation correction that had nothing to do with any of the above.** +`src/configure.rs` said io-harness substitutes `${env:...}` and `${file:...}` "and +nothing else". There are three: `${cmd:...}` is the third, and it is refused in a +project-scoped file. The README's `skills` row said the same thing and has been +corrected too. `${cmd:}` is still not passed through by the credential redaction — +it is a command line rather than a name — but that is now written down as a choice +instead of resting on a false premise. + ## [0.20.0] - 2026-08-27 A directory somebody else wrote can add to your session, and you can see exactly diff --git a/Cargo.lock b/Cargo.lock index 0a9c1a9..be28b56 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1866,7 +1866,7 @@ dependencies = [ [[package]] name = "io-cli" -version = "0.20.0" +version = "0.21.0" dependencies = [ "clap", "crossterm 0.28.1", diff --git a/Cargo.toml b/Cargo.toml index 3a71ad9..1f5e156 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "io-cli" -version = "0.20.0" +version = "0.21.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 036c6b9..f5004ba 100644 --- a/README.md +++ b/README.md @@ -18,7 +18,7 @@ permission boundary and the session store are all reimplemented here. A test asserts that: `tests/dependencies.rs` fails the build if this crate ever grows an HTTP client, a TLS stack, a database or a sandbox. -- [Install](#install) · [First run](#first-run) · [While it works](#while-it-works) +- [Install](#install) · [First run](#first-run) · [Bringing your setup across](#bringing-your-setup-across) · [While it works](#while-it-works) - [Keys](#keys) · [Commands](#commands) · [Configuration](#configuration) - [Capability bundles](#capability-bundles) · [Hooks](#hooks) · [The fleet](#the-fleet) - [Pictures](#pictures) · [Documents](#documents) · [Background jobs](#background-jobs) @@ -46,6 +46,7 @@ on one row and the keys and the posture on the next.](docs/screenshot.png) | **Readable without seeing it** | `--plain` animates nothing and commits every state change as text, for a screen reader, a braille display or a log | | **Markdown, rendered** | Headings, bullets, code and emphasis drawn as themselves rather than printed as notation | | **Documents, read and written** | Spreadsheets, Word, slide decks, PDFs and barcodes through io-harness's own tools — twelve of them, six of which write, every one under the same gate as any other read or write | +| **Your setup, brought across** | `/import` finds the agent tools already on this machine and offers their instructions, MCP servers, skills and model, item by item, with the whole plan shown before a byte is written and no credential read at any point | ## It never takes your terminal @@ -115,6 +116,76 @@ Your key never appears on screen, in the scrollback, or in a log line. If the provider's environment variable is already set, the wizard offers to use it and writes no key to disk at all. +## Bringing your setup across + +**You have almost certainly used another agent tool first, and `/import` brings +what you told it.** Four things carry over: the standing instructions you wrote, +the MCP servers you configured, the skills you collected, and the model you +settled on. Nothing else, and nothing without you saying so. + +It is offered **once**, on a first run, and io records that it asked so it never +asks again. One key declines and the session carries straight on — declining +writes nothing at all, and there is no reminder later. `/import` opens the same +thing at any time. + +**What it looks at:** + +| Where | What is read | +| --- | --- | +| `~/.claude/` | `CLAUDE.md`, `settings.json`, and the skills under `skills/` and `plugins/` | +| `~/.claude.json` | the MCP servers, which live here and not beside `settings.json` | +| `~/.codex/` | `AGENTS.md`, `memories/MEMORY.md`, `config.toml`, `rules/default.rules` | +| `~/.gemini/` | `GEMINI.md` and `antigravity/mcp_config.json` | +| the repository | `.cursorrules` or `CONVENTIONS.md`, if either is there | + +A tool that is not installed simply is not offered. A tool whose files are all +**empty** is a different row and says so — on a good many machines all three +Gemini files exist and every one is zero bytes, and an import of nothing that +then reports success is the failure you cannot see. + +**Where it writes:** instructions are appended to the memory file for the scope +you pick — one block per source file, with a line of provenance above the +original text, kept whole rather than shredded into a bullet per line. MCP +servers become `[[mcp]]` entries in io-harness's own spelling. Skills become +directories under `~/.io-cli/skills`. The model is *carried*, not written: a +`[[provider]]` entry needs a vendor and a foreign tool's model string does not +name one — `gpt-5` could be OpenAI or any of the twenty-one presets pointed at a +compatible endpoint — so io hands you the id and the entry is built once you have +chosen the vendor. + +**The whole plan is on screen before a single byte is written.** One row per +thing found, saying where it came from and where it would go, and you accept them +item by item. What you did not accept is not written. A cancelled import is not a +partial one. + +**No credential is ever read or copied, and that is enforced by the code rather +than promised by it.** `~/.codex/auth.json` is not in the list of files this +program can open, so no path through it reaches one. A server's `env` values are +parsed and thrown away without ever being assembled into a string — only the +variable *name* is ever held — and what gets written is `${env:NAME}`, the name +pointing at itself, which io-harness resolves out of your own environment at the +moment a run needs it. Your shell has to have those variables set, and the import +says which. `~/.claude.json` is an entire application's state with OAuth material +in it and is read through narrow structs, so every field io does not name is +skipped by the parser instead of being loaded and then politely ignored. + +**An allowlist is read, shown, and deliberately not translated.** Codex's +`prefix_rule(pattern=["bun","install"], …)` and Claude's `Bash(cargo yank *)` both +match a *command line*. io-harness's `Act::Exec` matches a **binary name and +nothing else** — it has no argument matching at all. So the closest faithful +import of `bun install` is a blanket allow on `bun`, which is a wider permission +than you ever granted, written by a tool you were trusting to be careful. io says +what it found and says it cannot express it, and produces no rule, no `[policy]` +table and no policy layer. A boundary half imported is worse than one left alone. + +**Two skills of one name kills a session, so an import counts before it writes.** +A name already answered to in your skills directory is refused on its own row and +the rest of the import still goes through. Going over io-harness's ceiling refuses +**every** skill instead: the harness rejects a whole directory rather than the +excess, so an operator at 63 skills who imported three more would get a session in +which every turn dies at run start with nothing visible to blame. See +[Skills](#skills). + ## While it works Two rows sit above the composer for exactly as long as a turn is in flight, and @@ -393,16 +464,24 @@ above a row that ranked there for reasons having nothing to do with it. | `/mcp` | the MCP servers configured, and what this session has seen of each | | `/provider` | the providers configured, in the order a turn tries them | | `/plugin` | the capability bundles loaded, what each contributed, and the ones that failed | +| `/import` | bring instructions, MCP servers, skills and a model across from another agent | `/plugin` is new in 0.20.0 and sits beside those two because it is the third surface of the same kind: something a configuration file declares by name, whose effect on the session is otherwise invisible. See [Capability bundles](#capability-bundles). +`/import` is new in 0.21.0 and is last in the group because it is the one command +here you use once: the others are returned to for the life of the install. It +writes files, which is the whole reason it is under **configure**. See [Bringing +your setup across](#bringing-your-setup-across). + `/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 +that list to add, edit and remove entries in the configuration file, which is the +one thing **inspect** promises it never does. That second half was a promise +rather than a fact until 0.21.0 — the writers existed and nothing called them, so +both panels could only read. They write now. `/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. @@ -542,7 +621,40 @@ with the file you wrote. 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. +many it installed and how many it withheld. `/import` counts against the same +ceiling before it writes a byte, and refuses the whole import rather than leave +you over it. + +**That ceiling is per directory, and the directories are not bounded together.** +Every skills directory is discovered on its own — yours, and one more for each +capability bundle that declares any — so six bundles can put far more than 64 +names in front of the model with nothing failing anywhere and nothing said about +it. What the limit protects is one directory's discovery, not the size of the +catalogue a turn is handed. If the palette has grown longer than you can read, +that is why, and `/skills` is where you see which directory each name came from. + +**A bundle's skills are listed too, under the name the model actually uses.** +Until 0.21.0 they reached the model and appeared on no surface that lists a skill, +so `/skills` and the `/` palette were lists that disagreed with the catalogue the +turn was handed. They are in both now, spelled `__` — io-harness's +own namespacing, and the string a refusal or a tool call will name — with the +bundle named as where the row came from. + +**Turning a bundle's skill on or off is refused, and the refusal is the honest +answer.** Turning a skill off is moving its file into a `disabled/` directory +beside it. For a bundle skill that would mean io-cli creating a directory inside +somebody else's bundle and moving their file into it — a directory io-cli did not +install, does not own and cannot put back. Stop the bundle instead: `/plugin` +removes its `[[plugin]]` entry and everything it contributed goes with it. + +**And a bundle naming a skills directory that is not on disk killed every turn of +that session, silently, in 0.20.0.** io-harness joins the manifest's word onto the +bundle root with no existence check at all, and the walk that discovers skills +fails the run before the first completion — so a typo in a `plugin.toml` somebody +else wrote reads as io being broken. `/skills` and `/plugin` name the bundle and +say what it costs, one row per bundle: a second broken bundle does not hide behind +the first, and a broken one no longer takes the surface that could explain it down +as well. ## Capability bundles @@ -1034,13 +1146,36 @@ rules: it writes, asks io-harness to read the file back, and restores it exactly when the answer is no. **`/mcp`** shows what is configured, which servers answered this session, how -many distinct tools each answered, and the last failure. A server the session has -not reached says so and is not shown as broken. +many tools each announced, how many distinct ones this session has asked for, and +the last failure. A server the session has not reached says so and is not shown as +broken. From 0.21.0 it also **adds, edits and removes** `[[mcp]]` entries, through +the same write `/config` uses: staged, read back by io-harness, and rolled back +whole when the answer is no. It offers no *disable*, because `McpServer` has no +key for one — an `enabled = false` invented here would be accepted by the file and +ignored by the harness, and a panel saying "disabled" over a running server is +worse than a panel with one fewer verb. Nor a *reconnect*: servers are attached +per turn, so the next turn is what picks up your edit. **`/provider`** shows the `[[provider]]` array as what it is: the order a turn -tries them. Reorder it and you have arranged the fallback chain io-harness has -supported since its 0.27.0. The twenty-one presets it reaches through one +tries them. From 0.21.0 you can **arrange** it — add an entry, promote one, +demote one, remove one — which is the fallback chain io-harness has supported +since its 0.27.0 and that this interface has drawn an event for without ever +being able to cause one. The twenty-one presets it reaches through one `Compatible` provider are offered by name with the endpoint each resolves to. +Reordering moves an entry with its own comments and its own keys rather than +rebuilding the array, because a chain rebuilt from io-cli's model would silently +drop whatever io-cli does not model. + +**Both of those panels could only list until 0.21.0.** The writers were there and +tested and called from nothing, while three places in this documentation said they +"add, edit, disable and remove". They genuinely write now, and the word *disable* +is gone from that sentence because `/mcp` does not offer it. + +Neither verb takes a row number. An entry is addressed by finding its id in the +file's own bytes, because a row on screen and a position in a file's array are +different numbers the moment anything sorts or filters, and getting that wrong +does not fail loudly — it removes a server you never named, or bills the next turn +to a vendor you did not choose. **`/plugin`** shows the capability bundles a `[[plugin]]` entry declared: what each one contributed, by name, and every bundle that was declared and dropped @@ -1058,7 +1193,7 @@ Eight keys live there, and five tables: | `diff` | `unified` — the default, and what an absent key means — or `minimal`, the changed lines and the `@@` header without the context, for reviewing by file rather than by hunk. | | `glyphs` | `unicode` or `ascii`. Absent asks the locale. | | `plain` | `true` runs every session in plain mode. The same switch as `--plain`, which wins over it. | -| `skills` | a directory of skills for the agent. They appear in the `/` palette by name, and the agent reads them itself. Absent, it is `~/.io-cli/skills`. A leading `~` is your home directory — io-cli expands it before io-harness sees the path, because io-harness substitutes `${env:…}` and `${file:…}` and nothing else. | +| `skills` | a directory of skills for the agent. They appear in the `/` palette by name, and the agent reads them itself. Absent, it is `~/.io-cli/skills`. A leading `~` is your home directory — io-cli expands it before io-harness sees the path, because io-harness substitutes `${env:…}`, `${file:…}` and `${cmd:…}`, and a tilde is none of the three. | | `max_parallel_reads` | how many read-only tool calls one turn may run at once. Absent, it is io-harness's own 10; `0` is clamped to 1 rather than meaning none. A `TaskContract` field with no io-harness configuration key of its own, which is why it is named here. | | `spawn_background_after_secs` | how long a spawned child may run before it is backgrounded. Absent, a child is waited for however long it takes. | | `detached_spawns` | whether a spawn may detach at all. Absent, it may. `false` buys a trace with every child's whole life in it, which a detached child gives up. | @@ -1094,6 +1229,11 @@ 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. +io also records in the home that it has offered to bring your setup across from +another agent tool, so that offer is made once on a first run and never again +however many times you start a session. Opening it deliberately is `/import` — +see [Bringing your setup across](#bringing-your-setup-across). + 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 @@ -1199,6 +1339,14 @@ giving a session verification gates needs its own surface; and `run.templates` i the thirteenth `[run]` key, reachable only through its own accessor. None of them is a silent omission — this is where they are named. +**`/import` does not bring a permission boundary across, and never will.** Another +tool spells a permission as a command line; io-harness matches a binary name. The +nearest honest translation is wider than what you granted, so io reports the +allowlist it found and writes no rule at all. Setting the boundary here is +`Shift+Tab`, `/config`, or the `io-permissions` skill — three surfaces where you +can see what you are granting. See [Bringing your setup +across](#bringing-your-setup-across). + **Sixel is still absent**, because encoding it means palette quantisation and another dependency, for terminals that either speak one of the two protocols already here or draw half blocks correctly. The Kitty path covers PNG rather than diff --git a/docs/config.example.toml b/docs/config.example.toml index a56fe06..9dfe5ce 100644 --- a/docs/config.example.toml +++ b/docs/config.example.toml @@ -164,6 +164,49 @@ net = "deny" # max_chars = 200000 # the total size of them # max_entry_chars = 4000 # the size of any one of them +# --- MCP servers ------------------------------------------------------------ +# +# The shape `/mcp` writes, and the shape `/import` translates another tool's +# servers into. io-cli defines no schema for one — this is io-harness's own, with +# the TRANSPORT AS A TAG beside the other fields rather than as a nested table. +# +# id what the server is called +# transport "stdio" for a program on this machine, "http" for a URL +# command the program, for a stdio server +# args its arguments, as an ARRAY — never a shell string +# env variables handed to that program +# +# A CREDENTIAL IS A REFERENCE AND NOT A LITERAL. io-harness resolves ${env:NAME} +# from the environment at the moment it reads this file, so the secret stays in +# your shell and never lands here. A substitution RESOLVES OR FAILS and never +# empties: a variable that is not set is an error naming the key, rather than a +# server that starts holding an empty token and fails somewhere less obvious. +# `/import` writes exactly this form — it reads the NAMES of another tool's +# environment variables and never their values — and tells you which variables +# your shell has to have. +# +# ${file:PATH} and ${cmd:PROGRAM ARGS} are the other two forms. ${cmd:} is +# REFUSED in a project-scoped io.toml, because that file arrives with a +# `git clone`. +# +# NO TILDE, HERE OR ANYWHERE IN THIS FILE. io-harness expands none: those three +# substitutions are the whole of what it rewrites, so a leading `~` in a value is +# a directory literally named `~`. Write the path out, or use ${env:HOME}. The +# one exception is io-cli's own `skills` key further down, which io-cli expands +# itself before io-harness ever sees it. +# +# [[mcp]] +# id = "github" +# transport = "stdio" +# command = "npx" +# args = ["-y", "@modelcontextprotocol/server-github"] +# env = { GITHUB_TOKEN = "${env:GITHUB_TOKEN}" } +# +# [[mcp]] +# id = "issues" +# transport = "http" +# url = "https://mcp.example.com/v1" + # --- Capability bundles (0.20.0) -------------------------------------------- # # A bundle is a directory with a `plugin.toml` in it, and it is loaded because @@ -386,9 +429,18 @@ fleet = "ctrl+f" # # Absent, this is `~/.io-cli/skills`, so writing the key is how you point at a # directory somewhere else rather than how you turn skills on. A leading `~` is -# your home directory: io-harness substitutes `${env:...}` and `${file:...}` and -# nothing else, so io-cli expands the tilde itself before handing the path over. -# A `~` anywhere but the front is a directory literally named `~`. +# your home directory: io-harness substitutes `${env:...}`, `${file:...}` and +# `${cmd:...}`, and a tilde is none of the three — so io-cli expands it itself +# before handing the path over. A `~` anywhere but the front is a directory +# literally named `~`. +# +# THE CEILING IS PER DIRECTORY. io-harness accepts at most 64 skills in one and +# rejects the whole directory rather than the excess — but every capability +# bundle a [[plugin]] entry declares brings a skills directory of its own, and +# the totals are not bounded together. +# Six bundles can put far more than 64 names in front of the model with nothing +# failing anywhere. `/skills` lists every one of them, bundle skills included, +# under the `__` the model actually addresses. # # skills = "~/.io-cli/skills" diff --git a/src/commands.rs b/src/commands.rs index 809f864..f8c6b1a 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -210,6 +210,10 @@ pub const COMMANDS: &[(&str, &str)] = &[ "/plugin", "the capability bundles loaded, what each contributed, and the ones that failed", ), + ( + "/import", + "bring instructions, MCP servers, skills and a model across from another agent", + ), ( "/profile", "switch to a named profile from the configuration, for this session", @@ -324,7 +328,7 @@ pub const GROUPS: &[(Group, &[&str])] = &[ ), // **`/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 + // both go on from that screen to add, edit 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 @@ -342,6 +346,11 @@ pub const GROUPS: &[(Group, &[&str])] = &[ "/mcp", "/provider", "/plugin", + // **`/import` writes files, so `Configure` is the only group it can + // be in** — and `Inspect` is full at nine besides. It is last because + // it is the one command here an operator uses once: the others are + // returned to for the life of the install. + "/import", ], ), ]; @@ -492,7 +501,7 @@ pub const SKILL: &str = "skill: "; /// A skill's rows come last, after the commands and the templates, and a session /// that configured no skills directory contributes none — the same "not /// configured" shape the templates have. -pub fn palette(templates: &Templates, skills: &io_harness::Skills) -> Vec { +pub fn palette(templates: &Templates, skills: &[crate::skillview::Listed]) -> Vec { entries(templates, skills) .into_iter() .map(|entry| entry.row) @@ -515,7 +524,7 @@ struct Entry { /// written next to each other; a grouped list with headings in it makes that /// agreement impossible to keep by inspection, because the rows and the things /// they stand for are no longer the same length. -fn entries(templates: &Templates, skills: &io_harness::Skills) -> Vec { +fn entries(templates: &Templates, skills: &[crate::skillview::Listed]) -> Vec { let mut out: Vec = Vec::new(); // Commands, under their group headings. The headings are drawn while the @@ -570,7 +579,20 @@ fn entries(templates: &Templates, skills: &io_harness::Skills) -> Vec { row: Row::marked( SKILL_MARK, skill.name.clone(), - format!("{SKILL}{}", skill.description), + // **The bundle is named in the detail, and the name carries + // the real signal.** A narrow terminal drops the detail column + // first, which is the 0.16.0 lesson about marks — so the + // origin must never be the only place the provenance lives. + // It is not: a bundle's skill is listed under the namespaced + // `__` the model actually addresses, and that prefix + // is in the label, which is the column that survives and the + // one `crate::fuzzy` ranks. + match &skill.origin { + crate::skillview::Origin::Bundle(id) => { + format!("{SKILL}{} · from the {id} bundle", skill.description) + } + _ => format!("{SKILL}{}", skill.description), + }, ), chosen: Some(Chosen::Skill(skill.name.clone())), }); @@ -609,7 +631,7 @@ fn entries(templates: &Templates, skills: &io_harness::Skills) -> Vec { /// in the prompt rather than something it guessed at. pub fn palette_pick( templates: &Templates, - skills: &io_harness::Skills, + skills: &[crate::skillview::Listed], index: usize, ) -> Option { entries(templates, skills) @@ -709,19 +731,57 @@ pub fn templates(config: &Config) -> (Templates, Option) { /// Discovered once, when the session starts — the same walk the contract's /// `skills` field will do for the run, done here so the palette can list what the /// agent will be told about without walking the directory on every keystroke. -pub fn skills(dir: Option<&std::path::Path>) -> (io_harness::Skills, Option) { - let Some(dir) = dir else { - return (io_harness::Skills::none(), None); +/// **Bundles are why this is no longer an `io_harness::Skills`.** A capability +/// bundle contributes its own directory, the harness merges every one of them +/// before the model sees the catalogue, and `Skills` has a private field with no +/// public constructor and a `pub(crate)` `merged` — so io-cli cannot build the +/// value that would describe what the run is actually offered. It carries the +/// rows instead, which it can build, and which carry the origin the palette needs +/// anyway. +/// +/// The walk itself belongs to [`crate::skillview`] and is not repeated here. +/// `/skills` and the palette must never disagree about what the model was +/// offered, and the cheapest way to guarantee that is for both to come out of one +/// function. This one drops the disabled rows, because a disabled skill is +/// precisely one the model is not offered and the palette exists to say what it +/// is. +pub fn skills( + home: &std::path::Path, + dir: Option<&std::path::Path>, + bundles: &[(String, std::path::PathBuf)], +) -> (Vec, Option) { + let view = match dir { + Some(dir) => crate::skillview::view(home, dir, bundles), + // Still the bundles. A home with no `skills/` of its own is the ordinary + // fresh install, and it is exactly when everything on offer came from a + // bundle. + None => crate::skillview::view_of_bundles(bundles), }; - match io_harness::Skills::discover(dir) { - Ok(found) => (found, None), - Err(error) => ( - io_harness::Skills::none(), - Some(format!( - "{error}; this session lists no skills until that is fixed" - )), - ), + // **Both failures, not just the operator's own directory.** + // + // A bundle whose declared skills directory is not on disk ends every turn of + // the session before the first completion, and this is the only call made at + // startup — so dropping `bundles_failed` here meant the one class of failure + // that kills the session said nothing at the one moment it was cheapest to + // say it, leaving the operator to guess that `/skills` or `/plugin` is where + // the reason lives. + let mut sentences: Vec = Vec::new(); + if let Some(error) = view.failed { + sentences.push(format!( + "{error}; this session lists no skills until that is fixed" + )); + } + for (id, error) in view.bundles_failed { + sentences.push(format!( + "the {id} bundle contributes no skills: {error}. Every turn of this session ends on \ + that error until the directory exists or the bundle is removed with `/plugin`." + )); } + let sentence = (!sentences.is_empty()).then(|| sentences.join(" ")); + ( + view.skills.into_iter().filter(|row| row.enabled).collect(), + sentence, + ) } /// What the driver should do about a slash command. @@ -891,6 +951,25 @@ pub enum Action { /// refused **whole**, and the sentence is what names the two files it could /// move to instead. Plugin, + /// Bring an operator's work across from another agent they already use. + /// + /// **Everything is shown before anything is written, and declining is the + /// default.** The surface lists one item per thing found — an instructions + /// file, an MCP server, a skill, a model — with where it came from and where + /// it would go, and writes only what the operator accepted. A cancelled + /// import is not a partial one. + /// + /// **An allowlist is read and deliberately not translated.** Another tool + /// spells a permission as a command and its arguments; io-harness's + /// `Act::Exec` matches a binary name and nothing else. What can be said + /// honestly is what was found and that it does not carry over, so that is + /// what is said — a boundary half imported is worse than one left alone. + /// + /// **No credential is read, at any point.** A server's environment values are + /// discarded without being constructed and the variable *name* is what gets + /// recorded. A key that was never on disk must not reach disk as a side + /// effect of trying a new program. + Import, } /// What `/copy` was asked for. @@ -1473,6 +1552,11 @@ pub fn parse(input: &str, keys: &Keys, theme: &Theme) -> Action { // are: the thing being listed is plural, so the plural is what a hand // reaches for, and refusing it teaches nothing. "plugin" | "plugins" => Action::Plugin, + // `/migrate` is admitted because it is the other word for this act, and + // an operator arriving from another tool is by definition someone with no + // idea what this one calls things. Neither spelling is an alias for a + // second screen: both open the one surface. + "import" | "migrate" => Action::Import, // **An alias earns no row of its own.** `/usage` is what an operator // coming from another agent types for "what is this costing me", and the // answer is `/status` — which already commits the spend, the budgets and diff --git a/src/configure.rs b/src/configure.rs index c4e68b1..b9d3713 100644 --- a/src/configure.rs +++ b/src/configure.rs @@ -152,10 +152,23 @@ fn is_credential(path: &str) -> bool { /// A value as it is safe to show. /// /// A `${env:VAR}` or `${file:PATH}` reference is shown **as written**: the -/// variable's name is the information an operator needs and its contents are not, -/// and io-harness substitutes those two forms and nothing else. Anything else in -/// a credential key is reduced to its last four characters, which is enough to -/// tell two keys apart and not enough to use. +/// variable's name is the information an operator needs and its contents are not. +/// +/// **There are three substitution forms and not two.** io-harness resolves +/// `${env:...}`, `${file:...}` **and** `${cmd:...}` +/// (`io-harness-0.69.0/src/config.rs:1909`); this comment claimed two until +/// 0.21.0, and the sentence it claimed it in was the argument for which forms +/// pass through here. The third is deliberately not one of them: a `${env:}` or +/// `${file:}` reference is a *name*, and the name is the whole of what an +/// operator needs to identify the credential. `${cmd:}` is a program and its +/// arguments — the arguments are values rather than names, and printing them +/// whole under a key this function was called on precisely because it holds a +/// secret is the one thing it exists to avoid. So it is reduced like any other +/// value. Separately, io-harness refuses `${cmd:}` outright in a project-scoped +/// file, so the form reaches this function only from a file the operator owns. +/// +/// Anything else in a credential key is reduced to its last four characters, +/// which is enough to tell two keys apart and not enough to use. pub fn redact(path: &str, value: &str) -> String { if !is_credential(path) { return value.to_string(); diff --git a/src/context.rs b/src/context.rs index e130515..be60724 100644 --- a/src/context.rs +++ b/src/context.rs @@ -25,7 +25,7 @@ //! nothing an operator can act on: the repository's instructions are *inside* the //! system block (`compose` frames them as a `repository_guidance` section), the //! MCP tools are *inside* the tool catalogue (they are the members of -//! `request.tools` carrying [`MCP_TOOL_PREFIX`](io_harness::MCP_TOOL_PREFIX)), +//! `request.tools` carrying [`io_harness::MCP_TOOL_PREFIX`]), //! and the recalled memory is *inside* the conversation (`assemble` pushes the //! `[memory]` block onto the front of the assembled text, which the user turn //! carries). Reporting each whole would count some bytes three times and produce @@ -56,7 +56,7 @@ //! measured in different units — the page would be arithmetically consistent with //! itself and wrong about the only comparison it exists to make. //! -//! The denominator is [`ContextBudget::effective_tokens`], read off the +//! The denominator is `ContextBudget::effective_tokens`, read off the //! *contract* rather than off `ContextBudget::default()`, because the operator's //! `[context]` configuration is already resolved onto the contract io-cli builds //! and reading the file a second time here would be a second answer to a settled diff --git a/src/home.rs b/src/home.rs index 79fecf2..05df373 100644 --- a/src/home.rs +++ b/src/home.rs @@ -191,7 +191,7 @@ fn chosen() -> Option { /// What decided the directory in force, without changing anything. /// -/// This cannot simply be [`chosen`], and the difference is the whole point: after +/// This cannot simply be `chosen`, and the difference is the whole point: after /// [`adopt`] runs there **is** an `IO_CONFIG_HOME` in the environment, because /// io-cli put it there — so a status row reading the raw variable would credit the /// operator for this crate's own default. The rule instead is that a variable @@ -341,6 +341,44 @@ pub fn adopt() -> Option { /// two answers to what mode io-cli's home has, one of them from a module that /// has no business deciding. The mode applies only to directories this call /// actually creates, so passing an existing one changes nothing. +/// The file recording that the import offer has been made. +/// +/// **A file rather than a key in `io.toml`.** Declining an import is not a +/// configuration choice the operator should find in their own file later; it is +/// io-cli's bookkeeping about a question it has already asked. Putting it in the +/// configuration would also mean the offer could not be made to an operator whose +/// file will not parse — which is exactly the operator most likely to want it. +const OFFERED: &str = ".import-offered"; + +/// Whether the import offer has already been made. +/// +/// **Absent means "not yet offered", which is the right reading for every install +/// that predates 0.21.0.** That is what makes this need no migration: an existing +/// operator has no marker, so they are offered once, which is the whole intent. +/// A home io-cli cannot locate answers `true` — there is nowhere to record the +/// answer, and an offer that cannot be remembered would be made on every launch. +#[must_use] +pub fn import_offered() -> bool { + match path() { + Some(home) => home.join(OFFERED).exists(), + None => true, + } +} + +/// Record that the import offer has been made, whatever the operator said to it. +/// +/// **Written when the offer is *made*, not when it is accepted.** An operator who +/// declines has answered the question, and asking again on the next launch would +/// make declining meaningless. `/import` ignores this entirely, so the choice is +/// never lost — it is only stopped from being asked unprompted a second time. +pub fn mark_import_offered() -> std::io::Result<()> { + let Some(home) = path() else { + return Ok(()); + }; + create(&home)?; + std::fs::write(home.join(OFFERED), []) +} + pub(crate) fn create(home: &Path) -> std::io::Result<()> { #[cfg(unix)] { diff --git a/src/import.rs b/src/import.rs new file mode 100644 index 0000000..5e52bac --- /dev/null +++ b/src/import.rs @@ -0,0 +1,1346 @@ +//! What the operator already told another agent tool, brought across once. +//! +//! An operator arriving at io has usually been somewhere else first, and that +//! somewhere holds four things they would otherwise retype: standing +//! instructions, MCP servers, skills, and a model id. This module finds them, +//! translates the three that have an honest translation, and writes only what it +//! was handed back. +//! +//! # The promise, and it is the only reason to trust this module at all +//! +//! **It never opens a credential file and never reads a credential value.** That +//! is not a convention kept by care; it is kept by the types. `~/.codex/auth.json` +//! is not in [`files`], so no code path here can reach it. And a server's `env` +//! map is deserialised as [`IgnoredAny`] values — serde walks each value and +//! throws it away without ever building a `String` from it — so the only thing +//! this process ever holds is the KEY. What gets written is `${env:NAME}`, the +//! name pointing at itself, which is what io-harness resolves from the operator's +//! own environment at the moment a run needs it. +//! +//! The same shape covers everything else in those files. `~/.claude.json` is a +//! whole application's state — OAuth material included — and it is read through +//! narrow structs rather than a `serde_json::Value`, so every field this module +//! does not name is skipped by the parser instead of being materialised and then +//! politely ignored. +//! +//! # Two things this module refuses to translate +//! +//! **An allowlist is read, described, and translated into nothing.** Codex's +//! `~/.codex/rules/default.rules` says `prefix_rule(pattern=["bun","install"], +//! decision="allow")` and Claude's `permissions.ask` says `Bash(cargo yank *)` — +//! both of which match a *command line*. io-harness's `Act::Exec` matches a +//! **binary name and nothing else**; it has no argument matching at all +//! (`io-harness-0.69.0/src/policy.rs:62`). So the nearest thing to a faithful +//! import of `bun install` is a blanket allow on `bun`, which is a wider +//! permission than the operator ever granted, written by a tool they were +//! trusting to be careful. A boundary half-imported is worse than one left alone, +//! so [`Kind::Allowlist`] produces a sentence and never a `Rule`, never a +//! `[policy]` table, and never a `[[policy.layers]]` entry. +//! +//! **A model id is carried, not written.** `[[provider]]` needs a vendor and a +//! foreign tool's model string does not name one — `gpt-5` could be OpenAI or any +//! of the twenty-one presets in [`crate::providers::PRESETS`] pointed at a +//! compatible endpoint. So the plan records the id and [`Item::provider_edit`] +//! hands the caller the [`crate::edit::Edit`] once a vendor has been chosen, +//! built by [`crate::providers::add`] rather than by a second speller here. +//! +//! # The ceiling is the feature +//! +//! `Skills::discover` rejects a directory holding more than +//! [`io_harness::skills::MAX_SKILLS`] — **the whole set, not the excess** — and +//! `TaskContract::discover_skills` propagates that with `?` before the first +//! completion. So an operator sitting at 63 skills who imports three more does +//! not get 64 skills and a warning; they get a dead session, on their next turn, +//! with no visible cause. Counting before writing is therefore the point of the +//! skills half rather than a nicety, and it is why an import that would go over +//! writes **nothing** and says so: an import is a set the operator accepted as a +//! set, and two thirds of one is a state they cannot reason about. +//! +//! # Why this module parses no TOML +//! +//! `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. Codex's `config.toml` is read through +//! [`crate::edit::value_at`], which quotes a named value's own bytes and decides +//! nothing, and [`crate::edit::sections`], which lists header paths. The one +//! thing that machinery cannot do is enumerate the KEYS of a table — and that is +//! the one place a line scan is used, for `[mcp_servers..env]`, where key +//! names are all this module is allowed to want anyway. +//! +//! # No driver in here +//! +//! No ratatui, no `App`, no keyboard. [`detect`] reports what is on the machine, +//! [`plan`] builds a `Vec` **whole, before anything is written**, and +//! [`apply`] writes the items it is handed. The overlay that draws the plan and +//! collects an operator's yes is a different file's job, and this one is testable +//! without a terminal because of it. + +use std::collections::{BTreeMap, BTreeSet}; +use std::path::{Path, PathBuf}; + +use io_harness::config::Scope; +use io_harness::{McpServer, McpTransport}; +use serde::de::IgnoredAny; + +/// Claude's directory under the operator's home. +const CLAUDE_DIR: &str = ".claude"; + +/// Claude's state file, which is **not** under [`CLAUDE_DIR`] and is where the +/// MCP servers actually live — under `projects[""].mcpServers` +/// and a top-level `mcpServers`. Looking for them beside `settings.json`, which +/// is where they read as though they ought to be, finds nothing at all. +const CLAUDE_STATE: &str = ".claude.json"; + +/// Codex's directory. `auth.json` sits in it and is deliberately not named +/// anywhere in this file. +const CODEX_DIR: &str = ".codex"; + +/// Gemini's directory. +const GEMINI_DIR: &str = ".gemini"; + +/// The file that makes a directory a skill, for every tool involved and for +/// io-harness too. +const SKILL_FILE: &str = "SKILL.md"; + +/// How far the skill walk descends. +/// +/// Claude's plugins nest five and six levels deep, so a shallow walk finds +/// nothing. The cap is here because the walk follows directory symlinks — which +/// it must, since `~/.claude/skills/*` entries are usually links to somewhere +/// else — and a link pointing at an ancestor is an infinite descent. +/// +// ponytail: a depth cap rather than a visited-inode set. Eight is comfortably +// past the deepest real layout and the failure mode is "a skill nobody found" +// rather than a hang. Swap in a `BTreeSet<(dev, ino)>` if a real tree ever +// needs more than eight. +const MAX_DEPTH: usize = 8; + +// --------------------------------------------------------------------------- +// A. What is on the machine +// --------------------------------------------------------------------------- + +/// A tool io-cli knows how to read. +/// +/// Two of these are repository files rather than tools, and they are here rather +/// than folded into one "workspace" variant because an operator recognises the +/// name of the thing they wrote: `.cursorrules` and `CONVENTIONS.md` are +/// different documents with different audiences, and a row saying "workspace" +/// would make them guess which one io-cli found. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +pub enum Source { + /// Claude Code: `~/.claude` plus `~/.claude.json`. + Claude, + /// Codex: `~/.codex`. + Codex, + /// Gemini: `~/.gemini`. + Gemini, + /// `.cursorrules` at the workspace root. + Cursor, + /// `CONVENTIONS.md` at the workspace root. + Conventions, +} + +impl Source { + /// Every source, in the order a surface lists them. + pub const ALL: [Source; 5] = [ + Source::Claude, + Source::Codex, + Source::Gemini, + Source::Cursor, + Source::Conventions, + ]; + + /// What this source is called, to an operator. + #[must_use] + pub fn word(self) -> &'static str { + match self { + Source::Claude => "Claude", + Source::Codex => "Codex", + Source::Gemini => "Gemini", + Source::Cursor => ".cursorrules", + Source::Conventions => "CONVENTIONS.md", + } + } +} + +/// A source that is on this machine, and every file this module would read of it. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Found { + /// Which tool. + pub source: Source, + /// The files, in the order [`files`] names them. Never empty — a source with + /// no files is not a [`Found`] at all. + pub paths: Vec, + /// What they hold, in bytes, added up. + pub bytes: u64, +} + +impl Found { + /// **The files are there and there is nothing in them.** + /// + /// A distinct state from absent, and the common one: on the machine this + /// module was written against, all three of Gemini's files exist and every + /// one is zero bytes. Collapsing the two would offer an operator an import of + /// nothing and then report success, which is the failure they cannot see from + /// a surface. + #[must_use] + pub fn empty(&self) -> bool { + self.bytes == 0 + } + + /// One line naming what was found. + #[must_use] + pub fn says(&self) -> String { + if self.empty() { + return format!( + "{}: {} file(s), all empty — nothing to bring across", + self.source.word(), + self.paths.len() + ); + } + format!( + "{}: {} file(s), {} bytes", + self.source.word(), + self.paths.len(), + self.bytes + ) + } +} + +/// Every file this module would read from `source`, that is actually there. +/// +/// **The roots are parameters and `$HOME` is never read here.** [`crate::home`] +/// resolves the operator's home for the driver; this module takes what it is +/// given, so a fixture directory drives it exactly as the real machine does and +/// a test can never touch the operator's own tools. +/// +/// `~/.codex/auth.json` is not in this list and must never be added to it. See +/// the module note. +#[must_use] +pub fn files(source: Source, home_root: &Path, workspace: &Path) -> Vec { + let mut out = Vec::new(); + match source { + Source::Claude => { + let root = home_root.join(CLAUDE_DIR); + push_file(&mut out, root.join("CLAUDE.md")); + push_file(&mut out, root.join("settings.json")); + push_file(&mut out, home_root.join(CLAUDE_STATE)); + skill_files(&root.join("skills"), 0, &mut out); + skill_files(&root.join("plugins"), 0, &mut out); + } + Source::Codex => { + let root = home_root.join(CODEX_DIR); + push_file(&mut out, root.join("AGENTS.md")); + push_file(&mut out, root.join("memories").join("MEMORY.md")); + push_file(&mut out, root.join("config.toml")); + push_file(&mut out, root.join("rules").join("default.rules")); + } + Source::Gemini => { + let root = home_root.join(GEMINI_DIR); + push_file(&mut out, root.join("GEMINI.md")); + push_file(&mut out, root.join("antigravity").join("mcp_config.json")); + } + Source::Cursor => push_file(&mut out, workspace.join(".cursorrules")), + Source::Conventions => push_file(&mut out, workspace.join("CONVENTIONS.md")), + } + out +} + +fn push_file(out: &mut Vec, path: PathBuf) { + if path.is_file() { + out.push(path); + } +} + +/// Every skill file under `root`, flattened. +/// +/// Two rules, and the second is the one that keeps a plugin tree from becoming a +/// hundred skills: +/// +/// 1. A directory holding a `SKILL.md` **is** one skill, and the walk does not +/// descend past it. Whatever else is in there is that skill's own material. +/// 2. A loose `*.md` counts only at the immediate top of a skills root, which is +/// where the tools that use that spelling put them. Deeper down, a `.md` file +/// is a README or a reference and importing it would offer the model a +/// document that was never a skill. +/// +/// `is_dir` and `is_file` both follow symlinks, which is required rather than +/// incidental: `~/.claude/skills/` is usually a link to a checkout +/// somewhere else, and a walk that used `symlink_metadata` would find an empty +/// directory. +fn skill_files(root: &Path, depth: usize, out: &mut Vec) { + if depth > MAX_DEPTH { + return; + } + let manifest = root.join(SKILL_FILE); + if manifest.is_file() { + out.push(manifest); + return; + } + let Ok(entries) = std::fs::read_dir(root) else { + return; + }; + // Sorted, because `read_dir` order is the filesystem's and a plan that came + // out in a different order on two machines would be a plan nobody could + // review against a previous run. + let mut found: Vec = entries.filter_map(Result::ok).map(|e| e.path()).collect(); + found.sort(); + for path in found { + if path.is_dir() { + skill_files(&path, depth + 1, out); + } else if depth == 0 && is_markdown(&path) { + out.push(path); + } + } +} + +fn is_markdown(path: &Path) -> bool { + path.extension() + .is_some_and(|ext| ext.eq_ignore_ascii_case("md")) +} + +/// Which sources are on this machine. +/// +/// Absent sources are simply not in the answer; a source that is there and holds +/// nothing is a [`Found`] whose [`Found::empty`] is true. See that method for why +/// the two are kept apart. +#[must_use] +pub fn detect(home_root: &Path, workspace: &Path) -> Vec { + Source::ALL + .into_iter() + .filter_map(|source| { + let paths = files(source, home_root, workspace); + if paths.is_empty() { + return None; + } + let bytes = paths + .iter() + .filter_map(|path| std::fs::metadata(path).ok()) + .map(|meta| meta.len()) + .sum(); + Some(Found { + source, + paths, + bytes, + }) + }) + .collect() +} + +// --------------------------------------------------------------------------- +// B. The plan +// --------------------------------------------------------------------------- + +/// What one item brings across. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Kind { + /// Standing guidance, appended to an instructions file. + Instructions, + /// One MCP server, as a `[[mcp]]` entry. + Mcp, + /// One skill, materialised flat under `/skills`. + Skill, + /// A model id. Carried for [`crate::providers`], never written here. + Model, + /// A command allowlist. **Reported and never translated** — see the module + /// note. + Allowlist, +} + +impl Kind { + /// The word a surface labels the row with. + #[must_use] + pub fn word(self) -> &'static str { + match self { + Kind::Instructions => "instructions", + Kind::Mcp => "mcp", + Kind::Skill => "skill", + Kind::Model => "model", + Kind::Allowlist => "allowlist", + } + } + + /// Whether accepting this item causes [`apply`] to write anything. + /// + /// `false` for the two kinds this module deliberately does not write, so a + /// surface can draw them as an account of what was found rather than as a + /// checkbox that does nothing when ticked. + #[must_use] + pub fn writes(self) -> bool { + !matches!(self, Kind::Model | Kind::Allowlist) + } +} + +/// Where an accepted item lands. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Destination { + /// Appended to the instructions file for this scope, through + /// [`crate::memory::remember`]. + Instructions(Scope), + /// A `[[mcp]]` entry in the configuration file for this scope, through + /// [`crate::configure::write`]. + Config(Scope), + /// A file written at exactly this path. + File(PathBuf), + /// **Nothing is written.** An allowlist, a model, or an item this module + /// refused at plan time — a name already taken, a set over the ceiling. The + /// reason is in [`Item::says`]. + Nowhere, +} + +impl Destination { + /// The file this lands in, for a session rooted at `root`. + /// + /// `None` for [`Destination::Nowhere`], and for a scope that has no path on + /// this machine — the same answer [`crate::memory::path`] gives, for the same + /// reason. + #[must_use] + pub fn path(&self, root: &Path) -> Option { + match self { + Destination::Instructions(scope) => crate::memory::path(root, *scope), + Destination::Config(scope) => crate::configure::scope_path(root, *scope), + Destination::File(path) => Some(path.clone()), + Destination::Nowhere => None, + } + } +} + +/// One thing an operator accepts or declines. +/// +/// The translated form is carried in the item rather than recomputed at write +/// time, and that is what makes a plan a plan: everything that could fail to +/// *read* has already happened, so accepting an item is a write and nothing else. +/// It is also why a skill's whole text is here — the symlink was followed while +/// the plan was built, so what gets written is a real file whatever happened to +/// the link since. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Item { + /// Which tool this came from. + pub source: Source, + /// What it is. + pub kind: Kind, + /// The file it came out of. + pub from: PathBuf, + /// One sentence, for an operator deciding. + pub says: String, + /// Where it goes. + pub to: Destination, + /// The exact bytes that will be written, where anything is: the TOML body of + /// a `[[mcp]]` entry, the text of an instruction, a skill's whole file, or a + /// model id. `None` for an item that writes nothing at all. + pub form: Option, +} + +impl Item { + /// The model id, for a [`Kind::Model`] item. + #[must_use] + pub fn model(&self) -> Option<&str> { + if self.kind != Kind::Model { + return None; + } + self.form.as_deref() + } + + /// The `[[provider]]` edit for this model, once a caller has decided which + /// vendor serves it. + /// + /// **The vendor is the caller's to supply and cannot be inferred here.** A + /// foreign tool's model string names a model, not a provider: `gpt-5` is + /// OpenAI's own name for it and equally what a `compatible` endpoint in front + /// of it would be asked for. Guessing would produce a `[[provider]]` that + /// resolves, authenticates against the wrong account, and fails on the first + /// turn — so the guess is the caller's, made in front of the operator. + /// + /// Built by [`crate::providers::add`] rather than spelled here, so there is + /// one place in this crate that knows what a provider entry looks like. + #[must_use] + /// **The endpoint is the caller's, and it cannot be spelled wrongly.** A + /// foreign model id names a model and never a vendor, so which endpoint + /// answers for it is a guess — and `crate::providers::Endpoint` is what makes + /// the guess unable to produce an entry io-harness refuses. The credential is + /// `None` here on purpose: a key is never carried across, and where one is + /// wanted the caller passes the variable name it already lives behind. + pub fn provider_edit( + &self, + endpoint: crate::providers::Endpoint<'_>, + ) -> Option { + Some(crate::providers::add(endpoint, self.model()?, None)) + } +} + +/// Build the whole plan. +/// +/// **Nothing is written and nothing can be.** Every failure available to this +/// function is a file that would not read or would not decode, and the answer to +/// each is the same: no item. An operator seeing a shorter list than they +/// expected can look at [`detect`]'s output to see what was there; an operator +/// seeing a plan that half-applied could not. +/// +/// `scope` is where instructions and configuration go — [`Scope::Project`] is the +/// answer for a repository the operator is standardising, [`Scope::User`] for a +/// machine they are moving onto. +#[must_use] +pub fn plan(found: &[Found], home: &Path, scope: Scope) -> Vec { + let mut items: Vec = Vec::new(); + // Gathered across every source and planned in one go, because the ceiling is + // a property of the destination directory rather than of any one tool: three + // skills from Claude and one from a plugin tree are four skills to + // `Skills::discover`. + let mut skills: Vec<(Source, PathBuf)> = Vec::new(); + + for one in found { + for path in &one.paths { + let name = path + .file_name() + .map(|name| name.to_string_lossy().to_string()) + .unwrap_or_default(); + match (one.source, name.as_str()) { + ( + _, + "CLAUDE.md" | "AGENTS.md" | "MEMORY.md" | "GEMINI.md" | ".cursorrules" + | "CONVENTIONS.md", + ) => items.extend(instructions(one.source, path, scope)), + (Source::Claude, "settings.json") => items.extend(claude_settings(path)), + (Source::Claude, ".claude.json") => { + items.extend(foreign_mcp(one.source, path, scope)); + } + (Source::Codex, "config.toml") => items.extend(codex_config(path, scope)), + (Source::Codex, "default.rules") => items.extend(codex_rules(path)), + (Source::Gemini, "mcp_config.json") => { + items.extend(foreign_mcp(one.source, path, scope)); + } + // Everything [`files`] can produce is named above except the + // skill files, so this arm is exactly those. Keep it that way: + // a new anchor file added to `files` without a case here would + // silently arrive as a skill. + _ => skills.push((one.source, path.clone())), + } + } + } + + items.extend(skill_plan(&skills, home)); + items +} + +// --------------------------------------------------------------------------- +// C. Translation +// --------------------------------------------------------------------------- + +/// One instructions file, as a single appended block. +/// +/// **The whole file goes through one [`crate::memory::remember`] call, not one +/// per line.** `remember` writes a markdown bullet, and a bullet per line would +/// turn the operator's headings, code fences and nested lists into a flat list of +/// fragments — the document would still be read by the model and would no longer +/// mean what it said. So the bullet is one sentence of provenance and the +/// original text follows it verbatim, at column zero, which is a lazy +/// continuation in markdown and therefore still inside the list item. +/// +/// A file that is nothing but whitespace produces no item. `remember` refuses a +/// blank line anyway; refusing it here means the operator is never offered a row +/// that cannot do anything. +fn instructions(source: Source, path: &Path, scope: Scope) -> Vec { + let Ok(text) = std::fs::read_to_string(path) else { + return Vec::new(); + }; + if text.trim().is_empty() { + return Vec::new(); + } + let lines = text.lines().count(); + vec![Item { + source, + kind: Kind::Instructions, + from: path.to_path_buf(), + says: format!( + "{lines} lines of {} instructions, appended to {}", + source.word(), + crate::memory::file_name(scope) + ), + to: Destination::Instructions(scope), + form: Some(format!( + "imported from {}:\n\n{}", + path.display(), + text.trim_end() + )), + }] +} + +/// One MCP server, as a foreign tool spells it. +/// +/// `env` is a map of **names to nothing**: [`IgnoredAny`] means serde parses each +/// value and discards it without ever constructing it. Reading a value here would +/// be the one thing this module promises not to do, and the promise is kept by the +/// type rather than by remembering. +#[derive(serde::Deserialize)] +struct ForeignServer { + #[serde(default)] + command: Option, + #[serde(default)] + args: Vec, + #[serde(default)] + env: BTreeMap, +} + +/// A JSON file that carries MCP servers. +/// +/// Narrow on purpose: `~/.claude.json` is a whole application's state and holds +/// material this module must not touch. Every field not named here is skipped by +/// serde rather than being deserialised into a `Value` and then ignored. +#[derive(serde::Deserialize, Default)] +#[serde(rename_all = "camelCase")] +struct ForeignJson { + #[serde(default)] + mcp_servers: BTreeMap, + /// Claude keys servers per absolute workspace path, and this is where they + /// actually are — the top-level map is usually empty. + #[serde(default)] + projects: BTreeMap, +} + +#[derive(serde::Deserialize, Default)] +#[serde(rename_all = "camelCase")] +struct ForeignProject { + #[serde(default)] + mcp_servers: BTreeMap, +} + +/// Every stdio MCP server in a JSON file, translated. +/// +/// The top-level map first, then each project's, and **the first entry to claim a +/// name wins**. Two `[[mcp]]` entries with one `id` is a configuration +/// io-harness reads as two servers under one name, and every tool call after that +/// is ambiguous — so the duplicate is dropped here rather than written and +/// discovered later. +fn foreign_mcp(source: Source, path: &Path, scope: Scope) -> Vec { + let Ok(text) = std::fs::read_to_string(path) else { + return Vec::new(); + }; + let Ok(file) = serde_json::from_str::(&text) else { + return Vec::new(); + }; + + let mut seen: BTreeSet = BTreeSet::new(); + let mut items = Vec::new(); + let every = file.mcp_servers.into_iter().chain( + file.projects + .into_values() + .flat_map(|project| project.mcp_servers.into_iter()), + ); + for (id, server) in every { + // No command is an HTTP server, or an entry shaped some other way. Left + // alone rather than guessed at: `McpTransport::Http` wants a URL this + // struct deliberately does not read, and inventing a `command` from a + // URL would write a `[[mcp]]` entry that spawns nothing. + let Some(command) = server.command else { + continue; + }; + if !seen.insert(id.clone()) { + continue; + } + let names: Vec = server.env.into_keys().collect(); + items.push(mcp_item( + source, + path, + scope, + &id, + &command, + server.args, + &names, + )); + } + items +} + +/// Codex's `config.toml`: the model, and every `[mcp_servers.]` table. +/// +/// Read through [`crate::edit::value_at`] and [`crate::edit::sections`] rather +/// than by parsing — see the module note on why this file may not hold a second +/// TOML reader. +fn codex_config(path: &Path, scope: Scope) -> Vec { + let Ok(text) = std::fs::read_to_string(path) else { + return Vec::new(); + }; + let mut items = Vec::new(); + + if let Some(model) = crate::edit::value_at(&text, "model") + .map(|value| unquoted(&value)) + .filter(|model| !model.is_empty()) + { + items.push(model_item(Source::Codex, path, &model)); + } + + for name in crate::edit::sections(&text) + .into_iter() + .filter(|section| section.len() == 2 && section[0] == "mcp_servers") + .map(|section| section[1].clone()) + { + let Some(command) = crate::edit::value_at(&text, &format!("mcp_servers.{name}.command")) + .map(|v| unquoted(&v)) + else { + continue; + }; + let args = crate::edit::value_at(&text, &format!("mcp_servers.{name}.args")) + .map(|value| strings(&value)) + .unwrap_or_default(); + let names = env_names(&text, &name); + items.push(mcp_item( + Source::Codex, + path, + scope, + &name, + &command, + args, + &names, + )); + } + + items +} + +/// The KEY NAMES of `[mcp_servers..env]`, and nothing else. +/// +/// **The one line scan in this module, and it exists because no value is allowed +/// to be read.** [`crate::edit::value_at`] answers "what is at this path" and +/// there is no public way to ask "what paths are in this table"; a scan that took +/// values would have been a reason to want one. This takes the identifier to the +/// left of the first `=` on each line and stops at the next header. +/// +// ponytail: a header inside a multi-line string would end the scan early, which +// `crate::edit::regions` handles properly with a character state machine. A +// config.toml with a `"""` block above its env table is not a thing that exists; +// if one turns up, expose `regions` from `edit` and use it here. +fn env_names(text: &str, server: &str) -> Vec { + let header = format!("[mcp_servers.{server}.env]"); + let mut names = Vec::new(); + let mut inside = false; + for line in text.lines() { + let line = line.trim(); + if line.starts_with('[') { + inside = line == header; + continue; + } + if !inside { + continue; + } + let Some((key, _)) = line.split_once('=') else { + continue; + }; + let key = key.trim().trim_matches('"').trim_matches('\'').trim(); + if !key.is_empty() && !key.starts_with('#') { + names.push(key.to_string()); + } + } + names +} + +/// One translated server, as the `[[mcp]]` body io-harness will read back. +/// +/// **The bytes come out of the real type through serde, not out of a format +/// string.** [`McpServer`] is `#[serde(flatten)]` over a +/// `#[serde(tag = "transport")]` enum, so the discriminant sits flat beside `id` +/// and a hand-written body that forgot it would load with `missing field +/// transport`. Serialising the value that io-harness itself deserialises is +/// what makes "what was written" and "what will be read" the same question. +/// +/// Each key is rendered with `toml::Value`'s own inline form, which is why `env` +/// comes out as an inline table: an appended `[[mcp]]` block cannot carry a +/// `[mcp.env]` header after it without that header attaching to the wrong entry. +fn mcp_item( + source: Source, + from: &Path, + scope: Scope, + id: &str, + command: &str, + args: Vec, + env: &[String], +) -> Item { + let mut server = McpServer::stdio(id, command).with_args(args); + if let McpTransport::Stdio { env: into, .. } = &mut server.transport { + // **The name pointing at itself.** io-harness resolves `${env:NAME}` from + // the operator's own environment when it reads the file, so the secret + // stays where it already was and this process never holds it. + *into = env + .iter() + .map(|name| (name.clone(), format!("${{env:{name}}}"))) + .collect(); + } + + let form = body(&server); + let note = if env.is_empty() { + String::new() + } else { + format!( + "; {} kept as ${{env:…}} — set {} in your shell or io-harness refuses the file", + many(env.len(), "secret"), + env.join(", ") + ) + }; + + Item { + source, + kind: Kind::Mcp, + from: from.to_path_buf(), + says: format!( + "{} MCP server `{id}` running `{command}`{note}", + source.word() + ), + to: if form.is_some() { + Destination::Config(scope) + } else { + Destination::Nowhere + }, + form, + } +} + +/// The `key = value` lines of one `[[mcp]]` entry. +/// +/// `None` where the value will not serialise, which is a shape io-harness could +/// not have produced and this module will not invent a spelling for. +fn body(server: &McpServer) -> Option { + let value = toml::Value::try_from(server).ok()?; + let table = value.as_table()?; + Some( + table + .iter() + .map(|(key, value)| format!("{key} = {value}")) + .collect::>() + .join("\n"), + ) +} + +/// Claude's `settings.json`: a model, and an allowlist that is described and +/// never translated. +#[derive(serde::Deserialize, Default)] +struct ClaudeSettings { + #[serde(default)] + model: Option, + #[serde(default)] + permissions: ClaudePermissions, +} + +#[derive(serde::Deserialize, Default)] +struct ClaudePermissions { + /// Patterns like `Bash(cargo yank *)`. **Counted, never decoded.** + #[serde(default)] + ask: Vec, +} + +fn claude_settings(path: &Path) -> Vec { + let Ok(text) = std::fs::read_to_string(path) else { + return Vec::new(); + }; + let Ok(settings) = serde_json::from_str::(&text) else { + return Vec::new(); + }; + + let mut items = Vec::new(); + if let Some(model) = settings.model.filter(|model| !model.trim().is_empty()) { + items.push(model_item(Source::Claude, path, &model)); + } + if !settings.permissions.ask.is_empty() { + items.push(allowlist_item( + Source::Claude, + path, + settings.permissions.ask.len(), + )); + } + items +} + +/// Codex's `~/.codex/rules/default.rules`. +/// +/// A Python-call DSL — `prefix_rule(pattern=["bun","install"], decision="allow")` +/// — and this function counts its rules and stops. See the module note: there is +/// no honest translation of an argument-matching allowlist into a policy that +/// matches binary names. +fn codex_rules(path: &Path) -> Vec { + let Ok(text) = std::fs::read_to_string(path) else { + return Vec::new(); + }; + let rules = text + .lines() + .map(str::trim) + .filter(|line| !line.is_empty() && !line.starts_with('#')) + .filter(|line| line.contains("decision")) + .count(); + if rules == 0 { + return Vec::new(); + } + vec![allowlist_item(Source::Codex, path, rules)] +} + +/// The one sentence an allowlist produces. +/// +/// `Destination::Nowhere` and `form: None`, both load-bearing: an item with a +/// destination is an item [`apply`] can write, and this one must never be +/// writable however a surface treats it. +fn allowlist_item(source: Source, path: &Path, rules: usize) -> Item { + Item { + source, + kind: Kind::Allowlist, + from: path.to_path_buf(), + says: format!( + "{} {} in {} — reported, not imported. io-harness matches an exec rule on the \ + BINARY NAME alone and does no argument matching, so `bun install` could only \ + become a blanket allow on `bun`. Review them and write the ones you want with \ + /permissions.", + many(rules, "rule"), + source.word(), + path.display() + ), + to: Destination::Nowhere, + form: None, + } +} + +/// The one sentence a model produces, and the id itself in `form`. +fn model_item(source: Source, path: &Path, model: &str) -> Item { + Item { + source, + kind: Kind::Model, + from: path.to_path_buf(), + says: format!( + "{} is set to `{model}` — carried, not written: a `[[provider]]` entry needs a \ + vendor, and a model id does not name one. Pick one with /provider.", + source.word() + ), + to: Destination::Nowhere, + form: Some(model.to_string()), + } +} + +/// Every skill, flattened, name-checked, and counted against the ceiling. +/// +/// The order of the three checks is the order that costs the operator least: a +/// name already claimed is refused individually, because the rest of the import +/// is still good; a set that would go over the ceiling refuses **every** skill, +/// because `Skills::discover` rejects a directory rather than truncating it and +/// a partial import is a state nobody can reason about. +/// +/// The oracle for what is already there is [`io_harness::Skills::discover`], the +/// same walk the run will do — not a `read_dir` of io-cli's own, which would +/// disagree with the run in exactly the case the check exists for: a file called +/// anything at all whose frontmatter claims the name. +fn skill_plan(candidates: &[(Source, PathBuf)], home: &Path) -> Vec { + if candidates.is_empty() { + return Vec::new(); + } + let dir = crate::skills::dir(home); + // A directory that does not discover is one with nothing in it to collide + // with. `discover` errors on a directory that is not there at all, which is + // an install whose home has not been adopted yet. + let existing = io_harness::Skills::discover(&dir).ok(); + let held = existing.as_ref().map_or(0, |skills| skills.len()); + + let mut taken: BTreeSet = BTreeSet::new(); + let mut items: Vec = Vec::new(); + let mut incoming = 0usize; + + for (source, path) in candidates { + let name = skill_name(path); + // **A frontmatter `name:` is somebody else's text and it is about to + // become a path component.** `skill_name` answers with the declared name + // because that is the name a skill is *addressed* by — but `Path::join` + // treats `../../../x` as an escape and an absolute path as a replacement + // for everything to its left, so the declared name reaching `join` + // unchecked lets a third-party bundle write a file anywhere the process + // can reach. Nothing else on the path catches it: the collision guard + // asks whether the target exists, and creating a *new* file somewhere + // else answers that happily. + // + // Refused rather than sanitised. A name is how the model addresses a + // skill, so quietly rewriting it would install a skill under a name the + // operator's other tool does not use — and a skill whose name cannot be a + // directory is a skill io-harness could not have discovered in the source + // tree either. Saying so names the file, which is the thing worth knowing. + if !one_path_component(&name) { + items.push(Item { + source: *source, + kind: Kind::Skill, + from: path.clone(), + says: format!( + "skill `{name}` is not imported: a skill's name becomes a directory here, \ + and that one is not a single path component. Rename it in {} and import \ + again.", + path.display() + ), + to: Destination::Nowhere, + form: None, + }); + continue; + } + let target = dir.join(&name); + let claimed = existing + .as_ref() + .is_some_and(|skills| skills.get(&name).is_some()) + || target.exists() + || dir.join(format!("{name}.md")).exists() + || !taken.insert(name.clone()); + if claimed { + items.push(Item { + source: *source, + kind: Kind::Skill, + from: path.clone(), + says: format!( + "skill `{name}` is not imported: {} already answers to that name. \ + Two skills with one name is `Error::Config` at run start, so this one \ + is left where it is.", + dir.display() + ), + to: Destination::Nowhere, + form: None, + }); + continue; + } + // Read here rather than at write time: this is where a symlink is + // followed, so what gets written is a real file whatever becomes of the + // link afterwards. + let Ok(text) = std::fs::read_to_string(path) else { + continue; + }; + incoming += 1; + items.push(Item { + source: *source, + kind: Kind::Skill, + from: path.clone(), + says: format!("skill `{name}` from {}", source.word()), + to: Destination::File(target.join(SKILL_FILE)), + form: Some(text), + }); + } + + if held + incoming > io_harness::skills::MAX_SKILLS { + let sentence = format!( + "no skill is imported: {} already holds {held} and {} more would be {}, over the \ + {} io-harness allows in one directory. Over the ceiling it rejects the WHOLE \ + directory rather than the excess, so every skill you have would stop loading. \ + Remove some first.", + dir.display(), + incoming, + held + incoming, + io_harness::skills::MAX_SKILLS, + ); + for item in &mut items { + if matches!(item.to, Destination::File(_)) { + item.to = Destination::Nowhere; + item.form = None; + item.says = sentence.clone(); + } + } + } + + items +} + +/// What a skill file resolves to, the way `Skills::discover` will resolve it. +/// +/// [`crate::skillview::describe`] reads the frontmatter `name:` and falls back to +/// the file stem — which is right for a loose `foo.md` and wrong for +/// `foo/SKILL.md`, whose stem is the literal word `SKILL`. io-harness's own +/// `default_name` special-cases exactly that (`skills.rs:345-356`), so this does +/// too; a plan that named three imported skills `SKILL` would collide all three +/// against each other. +/// Whether `name` is exactly one ordinary path component. +/// +/// The question is asked of a name that came out of somebody else's file and is +/// about to be joined onto io's own skills directory. It is answered by asking +/// `Path` itself rather than by scanning for `/` and `..`, because the separators +/// and the meaning of a prefix are the platform's business — `C:` and a backslash +/// are components on Windows and are not here, and a hand-rolled check would be +/// right on the machine it was written on. +/// +/// Empty is refused, `.` and `..` are refused, an absolute or prefixed path is +/// refused, and anything with more than one component is refused. A NUL is +/// refused too: it cannot reach the filesystem and would otherwise fail far from +/// here with an error naming neither the skill nor the file it came from. +fn one_path_component(name: &str) -> bool { + if name.is_empty() || name.contains('\0') { + return false; + } + let mut parts = Path::new(name).components(); + matches!(parts.next(), Some(std::path::Component::Normal(_))) && parts.next().is_none() +} + +fn skill_name(path: &Path) -> String { + let (name, _) = crate::skillview::describe(path); + if name.eq_ignore_ascii_case("SKILL") { + if let Some(parent) = path.parent().and_then(Path::file_name) { + return parent.to_string_lossy().to_string(); + } + } + name +} + +// --------------------------------------------------------------------------- +// E. Applying +// --------------------------------------------------------------------------- + +/// What [`apply`] did, in the order it did it. +/// +/// The shape [`crate::home::Report`] uses, for the reason that one uses it: the +/// report is owed until somebody delivers it, so every failure is collected and +/// the rest of the plan carries on. An import that stopped at the first bad item +/// would leave the operator with a half-applied plan and no list of which half. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct Report { + /// Every destination written, and what went there. **This is the undo + /// list** — an operator who wants the import back out needs the file and the + /// entry, and one line saying "wrote io.toml" three times is not that. + pub written: Vec<(PathBuf, String)>, + /// Every item that was accepted and could not be written, with the reason. + pub refused: Vec, + /// Every item the plan carries that this module never writes: a model + /// waiting on a vendor, an allowlist reported and not translated, a skill + /// refused at plan time. + pub carried: Vec, +} + +impl Report { + /// One line per outcome, written first, then what was carried, then what + /// failed. + #[must_use] + pub fn lines(&self) -> Vec { + let mut out = + Vec::with_capacity(self.written.len() + self.carried.len() + self.refused.len() + 1); + for (path, what) in &self.written { + out.push(format!("wrote {what} into {}", path.display())); + } + out.extend(self.carried.iter().cloned()); + out.extend(self.refused.iter().cloned()); + if out.is_empty() { + out.push("nothing was imported".to_string()); + } + out + } +} + +/// Write the accepted items, and say what happened to every one of them. +/// +/// `plan` here is the **accepted** subset — a surface filters the plan and hands +/// back what the operator said yes to. Nothing is read from a source in this +/// function: every byte that will be written was translated in [`plan`], so a +/// tool the operator uninstalled between the two calls cannot change what lands. +/// +/// **Every failure is collected.** A `[[mcp]]` entry io-harness refuses does not +/// stop the instructions from being appended, and it does not leave the +/// configuration file changed either — [`crate::configure::write`] round-trips +/// through `Config::discover` and rolls the file back when the harness says no. +/// That is what catches the one trap in this whole module: `${env:NAME}` for a +/// variable that is not set is a **hard parse error**, so a server whose secret +/// the operator has not exported yet is refused loudly at import time rather than +/// killing their next session. +/// **No `home` argument, and that is the design rather than an omission.** Every +/// destination is resolved when the plan is *built*, so the absolute path an +/// operator read on the review surface is the byte-for-byte path written here. +/// Re-deriving one from the home at write time would open a gap between what was +/// shown and what happens, on the one surface where that gap is the whole risk. +/// +/// A `Kind::Model` item is reported as **carried, not written**: a foreign model +/// id names a model and never a vendor, and `[[provider]]` needs both. The +/// endpoint is the caller's to ask for, in front of the operator, and the write +/// goes through [`Item::provider_edit`]. +pub fn apply(plan: &[Item], root: &Path) -> Report { + let mut report = Report::default(); + // `[instructions] files` is only worth writing for the scopes that need it. + // `AGENTS.md` is io-harness's `DEFAULT_INSTRUCTIONS` and is read by a project + // with no configuration at all; the other two are unreachable without the + // list, which is what `memory::install` writes. + let mut needs_install = false; + + for item in plan { + match (item.kind, &item.to) { + (_, Destination::Nowhere) | (Kind::Model, _) | (Kind::Allowlist, _) => { + report.carried.push(item.says.clone()); + } + (Kind::Instructions, Destination::Instructions(scope)) => { + let Some(form) = item.form.as_deref() else { + report.refused.push(nothing_to_write(item)); + continue; + }; + match crate::memory::remember(root, *scope, form) { + Ok(path) => { + if *scope != Scope::Project { + needs_install = true; + } + report.written.push((path, item.says.clone())); + } + Err(error) => report.refused.push(failed(item, &error)), + } + } + (Kind::Mcp, Destination::Config(scope)) => { + let Some(form) = item.form.as_deref() else { + report.refused.push(nothing_to_write(item)); + continue; + }; + // **Asked of the file, once per server, immediately before the + // append.** The dedupe inside the readers is per-source, so the + // same server declared by two tools — `context7` and `playwright` + // are in both Claude's and Gemini's files on a real machine — + // arrived twice; and nothing at all compared the plan against what + // `io.toml` already declared, so a second `/import`, or the + // first-run offer followed by `/import` later, appended a second + // entry under an id that was already there. io-harness validates + // no such thing, so `configure::write`'s round trip accepts it and + // rolls nothing back. + // + // Checking here rather than at plan time is what makes it cover + // both: writes happen one server at a time, so the first of a + // within-plan pair is on disk by the time the second is asked + // about. `servers::declared_in` was written for this caller. + let id = crate::edit::value_at(form, "id") + .map(|value| value.trim().trim_matches('"').to_string()); + if let Some(id) = id.filter(|id| crate::servers::declared_in(root, id).is_some()) { + report.refused.push(format!( + "MCP server `{id}` is not imported: a server of that id is already \ + declared. Nothing was changed — `/mcp` shows the one in force." + )); + continue; + } + // One write per server, deliberately. A batch would put every + // entry behind one `Config::discover`, so a single server naming + // an unset variable would take the whole import down with it. + let edits = [crate::edit::Edit::append("mcp", form)]; + match crate::configure::write(root, *scope, &edits) { + Ok(()) => { + let at = crate::configure::scope_path(root, *scope) + .unwrap_or_else(|| PathBuf::from("io.toml")); + report.written.push((at, item.says.clone())); + } + Err(error) => report.refused.push(failed(item, &error)), + } + } + (Kind::Skill, Destination::File(path)) => { + let Some(form) = item.form.as_deref() else { + report.refused.push(nothing_to_write(item)); + continue; + }; + // Asked again here and not only at plan time. The plan may be + // minutes old, and overwriting somebody's skill is the one + // outcome this half of the module exists to prevent. + if path.exists() { + report + .refused + .push(failed(item, "there is already a file there")); + continue; + } + let made = path.parent().map_or(Ok(()), crate::home::create); + if let Err(error) = made.and_then(|()| std::fs::write(path, form)) { + report.refused.push(failed(item, &error.to_string())); + continue; + } + report.written.push((path.clone(), item.says.clone())); + } + // A kind and a destination that do not go together. Reported rather + // than written somewhere plausible: an item this function does not + // understand is one it must not guess about. + _ => report.refused.push(failed( + item, + "this item's destination does not match what it is", + )), + } + } + + if needs_install { + // Best effort and never fatal: the guidance is already on disk, and what + // a failure here costs is io-harness reading it. Reported so the operator + // knows which of the two happened. + if let Err(error) = crate::memory::install(root) { + report.refused.push(format!( + "the instructions were written but `[instructions] files` was not updated, \ + so io-harness may not read them: {error}" + )); + } + } + + report +} + +fn failed(item: &Item, error: &str) -> String { + format!( + "could not import {} from {}: {error}", + item.kind.word(), + item.from.display() + ) +} + +fn nothing_to_write(item: &Item) -> String { + format!( + "{} from {} carries nothing to write", + item.kind.word(), + item.from.display() + ) +} + +// --------------------------------------------------------------------------- +// Small readers +// --------------------------------------------------------------------------- + +/// `1 rule` or `3 rules`. +fn many(n: usize, noun: &str) -> String { + if n == 1 { + format!("1 {noun}") + } else { + format!("{n} {noun}s") + } +} + +/// Every string in the source text of a TOML array. +/// +/// **The source text, because that is all [`crate::edit::value_at`] returns** — +/// it quotes a value's own bytes and says nothing about what they mean, which is +/// the line `tests/dependencies.rs` draws around TOML in this crate. Decoding a +/// list of strings out of those bytes is the same kind of act as +/// [`crate::providers`] taking the quotes off one. +/// +/// Both quote styles, because TOML has both and Codex's own file uses basic +/// strings for commands and literal strings for Windows paths. +/// +// ponytail: no `\uXXXX`, no `\U`, no multi-line `"""` element. An MCP argument +// list is program arguments — flags and paths — and none of the three has ever +// appeared in one. Reach for a real decoder the first time a bug report shows an +// escape this drops. +fn strings(source: &str) -> Vec { + let mut out = Vec::new(); + let mut chars = source.chars(); + loop { + let Some(open) = chars.next() else { + return out; + }; + if open != '"' && open != '\'' { + continue; + } + let mut item = String::new(); + while let Some(c) = chars.next() { + if c == open { + break; + } + // A literal string has no escapes at all — that is what makes it + // literal, and treating `\` as one there would eat a Windows path + // separator. + if c == '\\' && open == '"' { + let Some(escaped) = chars.next() else { break }; + item.push(match escaped { + 'n' => '\n', + 't' => '\t', + 'r' => '\r', + other => other, + }); + continue; + } + item.push(c); + } + out.push(item); + } +} + +/// One TOML string value, with its quotes taken off. +/// +/// Falls back to the trimmed source for a value that is not a string at all, so +/// a `model = 3` in somebody's file reads as `3` rather than as nothing. +fn unquoted(source: &str) -> String { + strings(source) + .into_iter() + .next() + .unwrap_or_else(|| source.trim().to_string()) +} diff --git a/src/lib.rs b/src/lib.rs index 783b203..1717955 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -52,6 +52,7 @@ pub mod fleet; pub mod fuzzy; pub mod glyphs; pub mod home; +pub mod import; pub mod intent; pub mod keys; pub mod markdown; diff --git a/src/main.rs b/src/main.rs index 63d6532..0becfde 100644 --- a/src/main.rs +++ b/src/main.rs @@ -361,7 +361,19 @@ async fn drive( // directory the turn hands the agent, or a skill the model can use is one the // operator cannot see in `/`. let skills_dir = io_cli::contract::skills_dir(&config, &capabilities, root.to_path_buf()); - let (skills, complaint) = commands::skills(skills_dir.as_deref()); + // **The bundles are part of the inventory, not an addition to it.** io-harness + // merges every declared bundle's skills into the catalogue the model is given, + // so a palette listing only the operator's own directory lists less than the + // model was offered — which is the gap 0.20.0 shipped. + // + // A home io-cli cannot locate costs provenance and nothing else: `home` is + // only ever asked whether io-cli's own manifest wrote a file, so an empty one + // makes every row read as the operator's, which is the safe direction. It + // never reaches a path that is joined to, so this cannot resolve anything + // against the working directory. + let home = io_cli::home::path().unwrap_or_default(); + let bundles = bundle_skills(&config); + let (skills, complaint) = commands::skills(&home, skills_dir.as_deref(), &bundles); if let Some(complaint) = complaint { notices.push(complaint); } @@ -448,10 +460,183 @@ fn skills_view( let Some(home) = io_cli::home::path() else { return io_cli::skillview::View::default(); }; + let bundles = bundle_skills(config); 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(), + Some(dir) => io_cli::skillview::view(&home, &dir, &bundles), + // **Still the bundles, with no directory of the operator's own.** A home + // that has never made `skills/` is the ordinary case, and it is exactly + // the case where every skill in front of the model came from a bundle — + // so returning an empty view here would blank the surface precisely when + // it is the only listing there is. + None => io_cli::skillview::view_of_bundles(&bundles), + } +} + +/// The import plan as picker rows: one per item, then the row that writes. +/// +/// **The accepted mark rides the LABEL, never the detail.** A narrow terminal +/// drops the detail column first — the 0.16.0 lesson — and an operator who cannot +/// see which items are switched on is an operator about to write something they +/// did not choose. On this surface that is the whole safety property, so it goes +/// in the column that survives. +/// +/// The last row is the only one that writes anything, and it says how many items +/// it will write. At zero accepted it still reads honestly rather than being +/// hidden: a row that says it will write nothing is how an operator confirms they +/// meant to decline everything. +/// Write an accepted import plan, and the model with it when an endpoint was +/// chosen. Answers with the lines the surface should commit, in order. +/// +/// **A free function returning lines, rather than an arm that records as it +/// goes.** The write happens from two places — straight off the review surface, +/// and after the endpoint question a model item forces — and the one thing that +/// must not differ between them is what gets written. Returning the lines instead +/// of holding `App` keeps both callers on one implementation. +/// +/// The model is written last and separately because it is the only item whose +/// destination the plan could not resolve: `[[provider]]` names a vendor and a +/// model, a foreign configuration names only the model, so the vendor is a +/// question and never an inference. +fn import_written( + chosen: &[io_cli::import::Item], + root: &std::path::Path, + endpoint: Option>, +) -> Vec<(Tone, String)> { + let mut lines: Vec<(Tone, String)> = io_cli::import::apply(chosen, root) + .lines() + .into_iter() + .map(|line| (Tone::Muted, line)) + .collect(); + let Some(endpoint) = endpoint else { + return lines; + }; + let Some(item) = chosen + .iter() + .find(|item| item.kind == io_cli::import::Kind::Model) + else { + return lines; + }; + match item.provider_edit(endpoint) { + None => lines.push(( + Tone::Refused, + "the model could not be written: the item names none".to_string(), + )), + Some(edit) => { + // The user scope, the same file the rest of the import wrote to, so a + // provider and the servers it will talk to do not end up in two files + // with different lifetimes. + match io_cli::configure::write(root, io_harness::config::Scope::User, &[edit]) { + // **A fallback, and the sentence says so.** `providers::add` + // appends, and the front of a `[[provider]]` array is the entry a + // run uses — so on any configuration that already had a provider, + // which is every operator this feature was written for, the + // imported model is the LAST link and does not answer anything + // until the ones before it fail. Saying "in force" would be true + // only on an empty chain, which is the one case the fixtures + // build. The operator is told where it landed and what makes it + // answer. + Ok(()) => lines.push(( + Tone::Success, + format!( + "{} joined the provider chain as its last fallback; `/provider` promotes \ + it if you want it to answer", + item.model().unwrap_or("the imported model"), + ), + )), + Err(error) => { + lines.push((Tone::Refused, format!("the model was not written: {error}"))) + } + } + } } + lines +} + +/// Whether accepting this item causes anything to be written. +/// +/// **Not `Kind::writes`, and the difference is a real one.** That answers for +/// `import::apply`, which reports a model as carried because a foreign +/// configuration names a model and never a vendor. The driver goes on to ask for +/// the vendor and write the entry, so from this surface's seat a model item does +/// write. Everything else that writes nothing has `Destination::Nowhere` — an +/// allowlist, a name already claimed, a set over the ceiling — and those are +/// findings io is reporting rather than choices it is offering. +fn import_writes(item: &io_cli::import::Item) -> bool { + item.kind == io_cli::import::Kind::Model + || !matches!(item.to, io_cli::import::Destination::Nowhere) +} + +fn import_rows( + items: &[io_cli::import::Item], + accepted: &[bool], + root: &std::path::Path, +) -> Vec { + let mut rows: Vec = items + .iter() + .zip(accepted) + .map(|(item, on)| { + io_cli::picker::Row::marked( + // **A row that can never write is not drawn as a checkbox.** An + // allowlist io cannot express, a skill whose name is already + // claimed, a set over the ceiling — these are things io found and + // is telling the operator about. Giving them a box to tick invites + // ticking one and being told "write the 1 item switched on above" + // by a surface that then writes nothing. + if !import_writes(item) { + "(i)" + } else if *on { + "[x]" + } else { + "[ ]" + }, + format!("{} · {}", item.kind.word(), item.says), + // **Both ends, because this surface promises both.** The source + // alone left the destination the one fact never on screen — on the + // one surface whose whole safety property is that nothing is + // written the operator has not read first. + match item.to.path(root) { + Some(to) => format!("{} → {}", item.from.display(), to.display()), + None => item.from.display().to_string(), + }, + ) + }) + .collect(); + let count = items + .iter() + .zip(accepted) + .filter(|(item, on)| **on && import_writes(item)) + .count(); + rows.push(io_cli::picker::Row::new(match count { + 0 => "write nothing and close".to_string(), + 1 => "write the 1 item switched on above".to_string(), + many => format!("write the {many} items switched on above"), + })); + rows +} + +/// The `(id, directory)` pair for every loaded bundle that declares skills. +/// +/// **Read off `pluginview` rather than off `Plugins`, and that is not a +/// shortcut.** `Plugins::skill_dirs` is `pub(crate)` in io-harness, but +/// `Plugin::id` and `Plugin::skills_dir` are both public and `pluginview::view` +/// already folds them into exactly this pair for the `/plugin` surface. Building +/// it a second time from the plugin list would be a second answer to one +/// question, and the two could drift — which is the whole reason `/plugin` and +/// `/skills` must agree about which bundle contributed what. +/// +/// **The order is the declaration order, and it matters.** It is the order +/// `TaskContract::discover_skills` folds the directories in, so a surface listing +/// them in this order lists them the way the model will be offered them. +/// +/// A bundle that declares no skills directory is absent rather than present and +/// empty: it contributed nothing here, and a row saying so would be a row about +/// the absence of a thing the operator never asked for. +fn bundle_skills(config: &Config) -> Vec<(String, std::path::PathBuf)> { + io_cli::pluginview::view(config) + .plugins + .into_iter() + .filter_map(|listed| listed.skills.map(|dir| (listed.id, dir))) + .collect() } /// The interactive session, as something [`provider::build`] can run. @@ -499,7 +684,14 @@ struct Interactive<'a, 'b> { /// What io-harness discovered in the configured skills directory, walked once /// at startup. Empty when nothing is configured and empty when the walk /// failed — the notice above is what tells those two apart. - skills: io_harness::Skills, + /// + /// **Rows rather than an `io_harness::Skills`, because bundles.** The harness + /// merges every declared bundle's directory into the catalogue the model gets, + /// and `Skills` has a private field, no public constructor and a `pub(crate)` + /// `merged` — so the value describing what the run is actually offered is one + /// io-cli cannot build. It carries the rows it can build instead, which also + /// carry the origin the palette draws. + skills: Vec, /// The named profile in force, or `None`. Carried as a name because the /// turn-boundary reload discovers the file afresh and would otherwise drop /// the overlay it produced. @@ -575,7 +767,7 @@ async fn loop_over Result>( // 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, + mut skills: Vec, // 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. @@ -710,6 +902,67 @@ async fn loop_over Result>( // `22m12s` beside a turn six seconds old. Each turn is handed its own. let mut picker: Option<(Picker, Pick)> = None; + // **The import offer, made once to everybody and never twice.** + // + // The gate is a marker in io's own home and NOT `provider_spec().is_none()`, + // which is what the wizard uses: that condition is only ever true for an + // operator who has configured nothing, so an offer behind it would reach + // nobody who upgraded into this release — every existing operator would be + // excluded from the one feature written for them. + // + // The marker is written here, as the offer is *made*, so declining costs one + // keystroke and is remembered. `Esc` closes the picker into the session rather + // than out of the program, which is the other half of the same promise: the + // wizard's cancel returns `None` and `main` turns that into an exit, and an + // operator who did not want to import has not said they did not want to run. + // + // Nothing is drawn when nothing was found, because a surface that opens to say + // "no other agent is installed" is a surface charging every first run for a + // question that had no answer. + // **Painted first, and this ordering is load-bearing rather than tidy.** + // Detection reads and decodes `~/.claude.json`, which is tens of megabytes on + // a real install, and walks `~/.claude/plugins` to depth eight following + // symlinks. Doing that above the first paint would hold a blank terminal for + // the length of that work on every operator's first launch after upgrading — + // the one launch where a session that looks hung is least explicable. + paint(screen, &mut app)?; + + if !io_cli::home::import_offered() { + if let Some(home) = io_cli::home::path() { + let found = io_cli::import::detect( + &io_cli::home::expand(std::path::Path::new("~")), + session.root(), + ); + let items = io_cli::import::plan(&found, &home, io_harness::config::Scope::User); + if !items.is_empty() { + for source in &found { + app.record(Tone::Muted, source.says()); + } + app.record( + Tone::Muted, + "io found work you have already done in another agent. Nothing is written \ + until you switch an item on and choose the last row; `Esc` leaves it all \ + alone and `/import` opens this again." + .to_string(), + ); + let accepted = vec![false; items.len()]; + picker = Some(( + Picker::new("Import", import_rows(&items, &accepted, session.root())), + Pick::Import { items, accepted }, + )); + } + // Written whatever happened above, including when nothing was found: + // the question has been asked and answered, and a machine that grows a + // `~/.claude` next week should not be interrupted for it mid-session. + if let Err(error) = io_cli::home::mark_import_offered() { + app.record( + Tone::Muted, + format!("io could not record that it offered to import: {error}"), + ); + } + } + } + paint(screen, &mut app)?; loop { @@ -903,10 +1156,157 @@ async fn loop_over Result>( // about to change, types the value themselves, and // presses Enter. A picker that wrote on a keystroke would // change a file on the way past. - // Choosing a server says what it is, in the scrollback. - // There is nothing to open: the write verbs go through - // `/config`, and a panel that opened a second editor - // here would be a third way to write one file. + // **Choosing an item switches it on; only the last row + // writes.** That separation is the release's central + // promise and it is enforced here rather than in + // `import::apply`: an operator who backs out with `Esc` + // at any point has changed no file, and one who reaches + // the write row has seen every destination first. + // + // Rebuilt through `descended` rather than `picker`, for + // the reason the `Pick::SkillToggle` note above gives — + // the end of this match closes the surface + // unconditionally, so a picker assigned here would be + // built and thrown away. `selecting` puts the cursor back + // on the row just toggled, so a run of items can be + // switched on without hunting for the place each time. + Pick::Import { items, accepted } => { + if index < items.len() { + // A row that can never write is a finding io is + // reporting, not a choice it is offering, so a + // press on it changes nothing rather than arming a + // box that means nothing. The surface draws those + // rows with `(i)` instead of a checkbox. + if import_writes(&items[index]) { + accepted[index] = !accepted[index]; + } + descended = Some(( + Picker::new( + "Import", + import_rows(items, accepted, session.root()), + ) + .selecting(index), + Pick::Import { + items: items.clone(), + accepted: accepted.clone(), + }, + )); + } else { + let chosen: Vec = items + .iter() + .zip(accepted.iter()) + .filter(|(_, on)| **on) + .map(|(item, _)| item.clone()) + .collect(); + match (chosen.is_empty(), io_cli::home::path()) { + (true, _) => app.record( + Tone::Muted, + "nothing was imported and nothing was written; \ + `/import` offers the same list again" + .to_string(), + ), + (false, None) => app.record( + Tone::Refused, + "io has no home directory of its own, so there is \ + nowhere to import into" + .to_string(), + ), + (false, Some(_)) => { + // **A model forces one more question + // before anything is written.** A foreign + // configuration names a model and never a + // vendor, and `[[provider]]` needs both — + // so which endpoint answers for it is the + // operator's to say, not io-cli's to + // guess. Guessing would write a provider + // that resolves, authenticates against the + // wrong account and fails on the first + // turn. + if chosen + .iter() + .any(|item| item.kind == io_cli::import::Kind::Model) + { + descended = Some(( + Picker::new( + "Which provider answers for that model?", + vec![ + Row::new("OpenRouter"), + Row::new("Anthropic"), + Row::new("OpenAI"), + Row::new( + "write everything else and leave \ + the model alone", + ), + ], + ), + Pick::ImportModel(chosen), + )); + } else { + // Every destination, named. That list + // is what makes an import undoable by + // hand, which is the only undo there + // is. + for (tone, line) in + import_written(&chosen, session.root(), None) + { + app.record(tone, line); + } + } + // The file on disk has moved under the + // session, so the session reads it again + // rather than trusting what it just wrote + // — the same round trip every other writer + // here makes, and the place a refusal by + // io-harness would finally show. + match io_cli::configure::reload(session.root()) { + Ok((fresh, stored)) => { + capabilities = + io_cli::contract::Capabilities::stored( + stored.as_ref(), + ); + config = fresh; + } + Err(error) => app.record( + Tone::Error, + format!( + "the import was written but the \ + configuration would not read back: {error}" + ), + ), + } + } + } + } + } + // The endpoint question, answered. The last row declines + // it, and declining writes everything else rather than + // abandoning the import — the model was one item among + // several and the operator already accepted the others. + Pick::ImportModel(chosen) => { + let endpoint = match index { + 0 => Some(io_cli::providers::Endpoint::OpenRouter), + 1 => Some(io_cli::providers::Endpoint::Anthropic), + 2 => Some(io_cli::providers::Endpoint::OpenAi), + _ => None, + }; + for (tone, line) in import_written(chosen, session.root(), endpoint) { + app.record(tone, line); + } + match io_cli::configure::reload(session.root()) { + Ok((fresh, stored)) => { + capabilities = + io_cli::contract::Capabilities::stored(stored.as_ref()); + config = fresh; + } + Err(error) => app.record( + Tone::Error, + format!( + "the import was written but the configuration would \ + not read back: {error}" + ), + ), + } + } Pick::Mcp => { let list = io_cli::servers::servers(&config, &app.servers); if let Some(server) = list.get(index) { @@ -921,6 +1321,72 @@ async fn loop_over Result>( server.decided.word(), ), ); + // **The position is read out of the file's own + // bytes, never from this row's index.** A row here + // is a merged, filtered view across three scopes; + // the `[[mcp]]` array it would be spliced into is + // a different list entirely. Handing one list's + // index to the other is precisely the silent wrong + // delete 0.20.0 shipped in `pluginview::rows`, and + // `servers::At` exists so it cannot be spelled. + match io_cli::servers::declared_at(server) { + None => app.record( + Tone::Muted, + "no configuration file in force declares this server, \ + so there is nothing here to change" + .to_string(), + ), + Some(at) => { + descended = Some(( + Picker::new( + format!("{}?", server.id), + vec![ + Row::new("leave it as it is"), + Row::new("remove this server"), + ], + ), + Pick::McpRemove { + id: server.id.clone(), + at, + }, + )); + } + } + } + } + // Row 0 is "leave it", the default that does nothing — + // the shape `/skills` and `/plugin` both already use. + Pick::McpRemove { id, at } => { + if index == 1 { + let edit = io_cli::servers::remove(at); + match io_cli::configure::write(session.root(), at.scope, &[edit]) { + Ok(()) => { + match io_cli::configure::reload(session.root()) { + Ok((fresh, stored)) => { + capabilities = + io_cli::contract::Capabilities::stored( + stored.as_ref(), + ); + config = fresh; + } + Err(error) => app.record( + Tone::Error, + format!( + "{id} was removed but the configuration \ + would not read back: {error}" + ), + ), + } + app.record( + Tone::Success, + format!( + "{id} is no longer configured; the next turn \ + talks to it no more", + ), + ); + } + Err(refusal) => app.record(Tone::Refused, refusal), + } } } // Says what the skill is and offers the one change there @@ -1157,10 +1623,20 @@ async fn loop_over Result>( if index != 0 { app.record(Tone::Muted, format!("{name} is unchanged")); } else { + // **The bundle list goes to the move, not just to + // the listing.** A row drawn from a bundle is + // refused inside `disable`/`enable` rather than + // being filtered out here, because a guard at the + // call site only guards the call site: the + // destination is computed from the file's own + // parent, so a bundle path reaching the move + // creates `disabled/` inside somebody else's + // bundle and takes their file into it. + let bundles = bundle_skills(&config); let moved = if *enabled { - io_cli::skillview::disable(path) + io_cli::skillview::disable(path, &bundles) } else { - io_cli::skillview::enable(path) + io_cli::skillview::enable(path, &bundles) }; match moved { Ok(to) => { @@ -1173,12 +1649,21 @@ async fn loop_over Result>( // harness refuses by name — or hides one // that has just come back. skills = io_cli::commands::skills( + &io_cli::home::path().unwrap_or_default(), io_cli::contract::skills_dir( &config, &capabilities, session.root().to_path_buf(), ) .as_deref(), + // Re-read rather than carried: the + // operator may have added or removed a + // `[[plugin]]` entry through `/plugin` + // since this session started, and a + // carried list would go on offering a + // bundle's skills after the bundle was + // removed. + &bundle_skills(&config), ) .0; app.record( @@ -1222,6 +1707,90 @@ async fn loop_over Result>( .unwrap_or_default(), ), ); + // The chain's order IS which model answers — the + // first `[[provider]]` entry is the provider and + // the rest are its fallbacks — so promoting is not + // cosmetic reordering, it is the switch. Read from + // the file rather than from `entry.index`, which + // counts the merged chain. + match io_cli::providers::declared_at(&config, entry) { + None => app.record( + Tone::Muted, + "no configuration file in force declares this link, so \ + there is nothing here to change" + .to_string(), + ), + Some(at) => { + let first = entry.index == 0; + let mut rows = vec![Row::new("leave it as it is")]; + if !first { + rows.push(Row::new("make this the provider in force")); + } + rows.push(Row::new("remove this link from the chain")); + descended = Some(( + Picker::new(format!("{}?", entry.model), rows), + Pick::ProviderVerb { + label: entry.model.clone(), + at, + first, + }, + )); + } + } + } + } + // Rows are "leave it", then promote where it is offered, + // then remove — so the index of `remove` moves with + // `first`. Computed from the same flag the rows were built + // from rather than hard-coded, because those two drifting + // apart is how a list removes the thing it offered to + // promote. + Pick::ProviderVerb { label, at, first } => { + let promote = if *first { usize::MAX } else { 1 }; + let remove = if *first { 1 } else { 2 }; + let edit = if index == promote { + io_cli::providers::promote(at) + } else if index == remove { + Some(io_cli::providers::remove(at)) + } else { + None + }; + if let Some(edit) = edit { + match io_cli::configure::write(session.root(), at.scope, &[edit]) { + Ok(()) => { + match io_cli::configure::reload(session.root()) { + Ok((fresh, stored)) => { + capabilities = + io_cli::contract::Capabilities::stored( + stored.as_ref(), + ); + config = fresh; + } + Err(error) => app.record( + Tone::Error, + format!( + "the chain was written but the configuration \ + would not read back: {error}" + ), + ), + } + app.record( + Tone::Success, + if index == promote { + format!( + "{label} answers from the next turn; what \ + was in force is now its fallback" + ) + } else { + format!( + "{label} is out of the chain from the next \ + turn" + ) + }, + ); + } + Err(refusal) => app.record(Tone::Refused, refusal), + } } } Pick::Profile(names) => { @@ -1662,6 +2231,29 @@ async fn loop_over Result>( if let Some(sentence) = &view.failed { app.record(Tone::Refused, sentence.clone()); } + // **This is the loudest thing this surface says, and it is + // the only place it can be said.** A bundle that declares a + // skills directory which is not on disk is not a cosmetic + // problem: `Plugin::skills_dir` does no existence check, the + // harness's `discover_skills` walks it with `?` before the + // first completion, and every turn of the session therefore + // ends with an error naming a path the operator never chose + // and never typed. Nothing else in io-cli connects that error + // to the bundle that caused it. So the sentence names the + // bundle, carries the harness's own words for the failure, + // and says plainly what it costs — and it is drawn even when + // the rows below it are fine, because the rows being fine is + // exactly what makes the dead session baffling. + for (id, sentence) in &view.bundles_failed { + app.record( + Tone::Refused, + format!( + "the {id} bundle contributes no skills: {sentence}. Every turn of \ + this session ends on that error until the directory exists or the \ + bundle's `[[plugin]]` entry is removed — `/plugin` removes it.", + ), + ); + } if view.skills.is_empty() { if view.failed.is_none() { // **Not "on the next start".** `install` already ran @@ -1712,6 +2304,27 @@ async fn loop_over Result>( // reload — and a bundle they have just broken shows up as a // refusal in the same breath. let view = io_cli::pluginview::view(&config); + // **The same call `/skills` makes, deliberately.** A bundle + // whose declared skills directory is absent ends every turn of + // the session, and both surfaces have to say so — but saying + // it twice from two implementations is how two surfaces come + // to disagree about one bundle. `skillview` owns the question + // "what did this bundle actually contribute", so `/plugin` + // asks it rather than answering it, and the two can only ever + // agree. `Plugins::dropped` cannot cover this: the bundle + // loaded fine, and it is the directory it names that is gone. + for (id, sentence) in + io_cli::skillview::view_of_bundles(&bundle_skills(&config)).bundles_failed + { + app.record( + Tone::Refused, + format!( + "the {id} bundle contributes no skills: {sentence}. Every turn of \ + this session ends on that error until the directory exists or \ + this bundle is removed below.", + ), + ); + } if view.is_empty() { // Naming the file rather than the concept: an operator who // has never declared a bundle needs to know where the @@ -1733,6 +2346,56 @@ async fn loop_over Result>( )); } } + // **Everything is shown before anything is written, and the + // default for every item is no.** The plan is built whole here — + // detection, translation and destination — and drawn into the + // scrollback before the picker opens, so an operator who presses + // `Esc` has still *seen* what io found. The picker then turns + // items on one at a time; nothing is written until the last row is + // chosen, which is why a cancelled import is never a partial one. + Action::Import => match io_cli::home::path() { + None => app.record( + Tone::Refused, + "io has no home directory of its own, so there is nowhere to import into" + .to_string(), + ), + Some(home) => { + // The operator's home, not io's: `~/.claude` and + // `~/.codex` sit beside `~/.io-cli`, not inside it. + let found = io_cli::import::detect( + &io_cli::home::expand(std::path::Path::new("~")), + session.root(), + ); + for source in &found { + app.record(Tone::Muted, source.says()); + } + // **The user scope, and it is not a default chosen for + // convenience.** It is the one configuration file that is + // never committed, and an import writes absolute paths and + // an operator's accumulated instructions — neither of + // which belongs in a checkout somebody else clones. + let items = + io_cli::import::plan(&found, &home, io_harness::config::Scope::User); + if items.is_empty() { + app.record( + Tone::Muted, + "nothing to import: no other agent's configuration was found. \ + `AGENTS.md` in a repository needs no import — io-harness has \ + discovered it with no configuration at all since its 0.45.0." + .to_string(), + ); + } else { + let accepted = vec![false; items.len()]; + picker = Some(( + Picker::new( + "Import", + import_rows(&items, &accepted, session.root()), + ), + Pick::Import { items, accepted }, + )); + } + } + }, Action::Provider => { let chain = io_cli::providers::chain(&config); if chain.is_empty() { @@ -2382,6 +3045,37 @@ async fn loop_over Result>( // a profile body can carry `[app.io-cli]` keys too. let (in_force, _) = settings::stored(&config); capabilities = io_cli::contract::Capabilities::stored(in_force.as_ref()); + // **The palette's inventory is re-walked here, and before + // 0.21.0 nothing needed it to be.** The list was read once at + // startup and reassigned in exactly one place, the `/skills` + // toggle — which was complete while the only skills in it came + // from one directory the operator changed only through that + // toggle. Bundles broke that: `/plugin` can stop loading a + // bundle and `/import` can add skills, both mid-session, and + // neither goes anywhere near that arm. Left alone, `/` went on + // offering a name whose file the turn's catalogue no longer + // has, and the model is asked for a skill it will be refused. + // + // Done at the turn boundary rather than at each writer, + // because this is where `config` is already fresh and it + // bounds the staleness for every way the set can change, + // including an operator editing `io.toml` in another window. + // The sentence is dropped on purpose: it was said once at + // startup and both `/skills` and `/plugin` say it on demand. + // Committing it again every turn is the shape 0.19.0 already + // rejected once — a line that repeats for the life of the + // file it is about stops being read. + skills = commands::skills( + &io_cli::home::path().unwrap_or_default(), + io_cli::contract::skills_dir( + &config, + &capabilities, + session.root().to_path_buf(), + ) + .as_deref(), + &bundle_skills(&config), + ) + .0; let effective = approval::session_policy(&policy, app.posture(), app.remembered()); let stopped = turn( @@ -3637,6 +4331,44 @@ enum Pick { /// index reads straight back through `commands::palette_pick` — no list is /// carried here because the rows already address the thing they stand for. Palette, + /// The import plan, and which of its items are switched on. + /// + /// **The two vectors are the same length and are indexed together**, which is + /// the arrangement this codebase has already been bitten by once: in 0.20.0 an + /// index from one list was handed to a function expecting another's and + /// removed the wrong entry. They are safe here for a reason that must hold + /// whenever this arm is edited — `import_rows` builds one row per item in + /// order and appends exactly one row after them, so a chosen index below + /// `items.len()` addresses the item at that index and the single index equal + /// to it is the write row. Nothing filters, so no row index is ever a + /// different list's position. + /// + /// Carried rather than recomputed between keystrokes, because recomputing the + /// plan would re-read the filesystem underneath the operator and could change + /// the row count while they are choosing. + Import { + items: Vec, + accepted: Vec, + }, + /// The accepted items, held while the operator says which endpoint answers + /// for an imported model. Nothing has been written when this is open: the + /// question comes before the write, so declining it still writes the rest. + ImportModel(Vec), + /// One MCP server, and where in which file it is declared. The position is a + /// `servers::At`, read from that file's own bytes — never this surface's row + /// index, which addresses a merged view across three scopes. + McpRemove { + id: String, + at: io_cli::servers::At, + }, + /// One link of the provider chain, and where it is declared. `first` decides + /// whether promoting it is offered at all: a control that is drawn and does + /// nothing is worse than one that is absent. + ProviderVerb { + label: String, + at: io_cli::providers::At, + first: bool, + }, /// One directory of the workspace, in the order `list_dir` sorted it, so a /// chosen index reads straight back through `complete::pick`. The rows are /// last components rather than paths — see `complete::rows` for why — which diff --git a/src/picker.rs b/src/picker.rs index 58072bd..7f35891 100644 --- a/src/picker.rs +++ b/src/picker.rs @@ -68,7 +68,7 @@ pub struct Row { /// **A heading is drawn while the list is being browsed and disappears the /// moment a character is typed**, which is not decoration: a ranked list with /// headings interleaved puts a heading above a row that ranked there for - /// reasons having nothing to do with it. So [`Picker::refilter`] admits + /// reasons having nothing to do with it. So `Picker::refilter` admits /// headings only for an empty query, and nothing can be chosen while one is /// under the marker. pub heading: bool, diff --git a/src/providers.rs b/src/providers.rs index 1c0ab43..6729807 100644 --- a/src/providers.rs +++ b/src/providers.rs @@ -25,12 +25,42 @@ //! the real one back out of that error, so a gate can prove the two agree. **A //! list nothing has to agree with is decoration**, and this one would drift into //! an operator selecting a vendor and being told "unknown provider preset". +//! +//! # Arranging the chain, and the two numbers that must not be confused +//! +//! [`add`], [`promote`], [`demote`] and [`remove`] are the verbs, and three of +//! them address an entry by **position in a file's `[[provider]]` array**. That +//! is a different number from a row on screen the moment anything filters or +//! reorders the view, and getting it wrong here does not fail loudly: it moves a +//! provider an operator did not name to the front of the chain, and the next turn +//! bills to a vendor they did not choose. So those three take an [`At`], which +//! only [`At::of`] builds — by counting entries in the file's own bytes. +//! +//! `[[provider]]` is deliberately **not** one of io-harness's appending keys +//! (`config.rs:2052`): the winning scope replaces the chain whole, "because a +//! half-appended fallback chain is not a chain". So exactly one file decides the +//! whole array, and [`declared_at`] finds that file through [`decided`] — the +//! same origin [`chain`] already reads the credentials out of, so the position +//! and the text cannot come from two different files. +//! +//! # Exactly one of `preset` and `base_url` +//! +//! io-harness refuses a `compatible` entry that names both or neither, by index, +//! at load (`config.rs:456`). [`crate::configure::write`] would catch it on the +//! round trip and roll back, which is a good failure — but [`add`] takes an +//! [`Endpoint`] whose two `compatible` shapes are separate variants, so the entry +//! that fails cannot be constructed at all. -use io_harness::config::Config; +use io_harness::config::{Config, Scope}; use io_harness::{Compatible, ProviderSpec}; use crate::configure::Decided; use crate::edit::Edit; +// **The twin this module used to keep its own copy of.** Both spelled a value +// the same way and neither escaped a control character; a model id is tame, but +// an `api_key` and a `base_url` are pasted text, and a raw newline inside a +// basic string is a parse error rather than a value. One copy, fixed once. +use crate::servers::quoted; /// Every vendor preset io-harness reaches through `Compatible`. /// @@ -332,32 +362,190 @@ pub fn rows(chain: &[Entry]) -> Vec { .collect() } +/// One entry's position in one file's `[[provider]]` array, and how long that +/// array is. +/// +/// **The length is carried rather than passed.** [`demote`] used to take it as a +/// second argument, which meant the bound on "is there anywhere to move to" came +/// from whatever the caller happened to be counting — a filtered view, a chain +/// rendered from a different `Config`, or the same number typed twice. Here both +/// numbers are read from the same file in the same pass, so they cannot disagree +/// about the array they describe. +/// +/// It carries the scope too, because a caller needs it and it comes from the +/// same lookup: [`crate::configure::write`] takes a scope, and an index means +/// nothing without the file it counts in. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct At { + /// The scope whose file carries the chain — the scope a write must go to. + pub scope: Scope, + /// Private so nothing outside this module can spell one from a row number. + index: usize, + /// How many `[[provider]]` entries that file declares. + len: usize, +} + +impl At { + /// The `index`-th `[[provider]]` entry of `text`. + /// + /// `text` must be the bytes of the file `scope` names. Entries are counted by + /// walking `provider[n].kind` — the one key `#[serde(tag = "kind")]` makes + /// required on every variant — until the first gap, which is the end of a + /// contiguous array of tables. + /// + /// `None` when the file declares no entry at that position, so a move or a + /// removal aimed past the end is refused here rather than turning into + /// io-harness's "there is no `provider[n]` in this file" further down. + pub fn of(scope: Scope, text: &str, index: usize) -> Option { + let mut len = 0usize; + while crate::edit::value_at(text, &format!("provider[{len}].kind")).is_some() { + len += 1; + } + (index < len).then_some(At { scope, index, len }) + } + + /// The position, for a caller that has a sentence to write about it. + /// + /// ponytail: no accessor for the length beside it. Nothing needs to draw + /// "3 of 4" yet, and the one thing the length is *for* — the bound on + /// [`demote`] — is answered inside this module. Add one when a row wants it. + pub fn index(&self) -> usize { + self.index + } +} + +/// Where the file that decided the chain declares `entry`. +/// +/// It takes the [`Entry`] rather than a number so there is nothing for a caller +/// to get wrong, and it reads the file [`decided`] names — which is the file +/// [`chain`] itself read, so the position it returns is a position in the array +/// the operator is looking at. +/// +/// `None` where no file declares a chain at all, and where the deciding file no +/// longer carries that position — an operator editing `io.toml` under the +/// session is exactly what the second looks like. +pub fn declared_at(config: &Config, entry: &Entry) -> Option { + let Decided::File { scope, path } = decided(config) else { + return None; + }; + let text = std::fs::read_to_string(path).ok()?; + let at = At::of(scope, &text, entry.index)?; + // **The position is confirmed against the entry's own content, and without + // this the newtype is a wrapper rather than a guard.** + // + // `At::of` counts `provider[n].kind` in the file's TOP-LEVEL array and + // bounds-checks the number it was handed. [`chain`] builds its rows from the + // RESOLVED configuration. Those are the same list right up until a profile is + // in force: `provider` is not an appending key, so `[[profile.fast.provider]]` + // *replaces* the top-level array, and io-harness then rewrites the origins to + // `provider.*` — so `decided` still names this file and every positional check + // still passes while the rows on screen describe entries that are not at those + // positions. Removing "the only link" would have deleted the operator's real + // primary provider, which was never on screen. + // + // So the row and the file have to agree about *what is there*, not merely that + // something is. Disagreement means the array being counted is not the array + // being looked at, and the honest answer is that no file in force declares + // this link — which the caller already renders as a refusal. + let model = crate::edit::value_at(&text, &format!("provider[{}].model", entry.index))?; + let kind = crate::edit::value_at(&text, &format!("provider[{}].kind", entry.index))?; + (unquoted(&model) == entry.model && unquoted(&kind) == entry.kind).then_some(at) +} + +/// What a new `[[provider]]` entry reaches. +/// +/// **The two `compatible` shapes are two variants and not two `Option`s.** +/// io-harness takes exactly one of `preset` and `base_url` and refuses both and +/// neither by index at load; a pair of options is a signature in which three of +/// four combinations are wrong. The `kind` string is derived here rather than +/// taken, for the same reason: `deny_unknown_fields` is on the `[[provider]]` +/// variants, so `kind = "openai"` beside a `preset` is refused too, and a free +/// string is a way to ask for that. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Endpoint<'a> { + /// `kind = "openrouter"`. An absent key means `OPENROUTER_API_KEY`. + OpenRouter, + /// `kind = "anthropic"`. An absent key means `ANTHROPIC_API_KEY`. + Anthropic, + /// `kind = "openai"`. An absent key means `OPENAI_API_KEY`. + OpenAi, + /// A `compatible` entry reached through one of [`PRESETS`]. + /// + /// A name io-harness does not know is refused on the round trip, naming every + /// preset that exists — which is a better sentence than io-cli would write, + /// so this does not check the name itself. + Preset(&'a str), + /// A `compatible` entry reached at a base URL the operator gives, with + /// `/chat/completions` appended to it by the harness. + BaseUrl(&'a str), +} + +impl Endpoint<'_> { + /// The `kind` this writes, which is the tag io-harness dispatches on. + fn kind(&self) -> &'static str { + match self { + Endpoint::OpenRouter => "openrouter", + Endpoint::Anthropic => "anthropic", + Endpoint::OpenAi => "openai", + Endpoint::Preset(_) | Endpoint::BaseUrl(_) => "compatible", + } + } +} + /// The edit that appends a provider to the end of the chain. -pub fn add(kind: &str, model: &str, preset: Option<&str>) -> Edit { - let mut body = format!("kind = {}\nmodel = {}", quoted(kind), quoted(model)); - if let Some(preset) = preset { - body.push_str(&format!("\npreset = {}", quoted(preset))); +/// +/// Last, because the end of an array of tables is the end of the chain and the +/// front of it is the provider a run uses: an entry inserted anywhere else would +/// change which vendor the next turn bills to, which is not what "add" says. +/// +/// `api_key` is written as given, so `Some("${env:GROQ_API_KEY}")` is how a +/// credential is named without being copied into the file. **io-harness resolves +/// that at parse time and fails outright when the variable is unset**, so a +/// reference to a name that does not exist is refused by +/// [`crate::configure::write`]'s round trip and rolled back — the file is never +/// left naming a variable that stops the session from starting. +pub fn add(endpoint: Endpoint<'_>, model: &str, api_key: Option<&str>) -> Edit { + let mut body = format!("kind = {}", quoted(endpoint.kind())); + // Exhaustive, with no wildcard: a variant added later must be given its own + // second line here rather than falling through to a `compatible` entry that + // names neither base and is refused at load. + match endpoint { + Endpoint::OpenRouter | Endpoint::Anthropic | Endpoint::OpenAi => {} + Endpoint::Preset(name) => body.push_str(&format!("\npreset = {}", quoted(name))), + Endpoint::BaseUrl(url) => body.push_str(&format!("\nbase_url = {}", quoted(url))), + } + body.push_str(&format!("\nmodel = {}", quoted(model))); + if let Some(key) = api_key { + body.push_str(&format!("\napi_key = {}", quoted(key))); } Edit::append("provider", body) } -/// The edit that moves an entry one place towards the front. -pub fn promote(index: usize) -> Option { - (index > 0).then(|| Edit::move_entry("provider", index, index - 1)) +/// The edit that moves an entry one place towards the front of the chain. +/// +/// **Which is a change to what the next turn runs on, not a cosmetic one**: the +/// first entry is the provider in force. Promoting the second entry makes it the +/// provider and demotes the one that was. +/// +/// `None` for the entry that is already first, so a caller cannot draw a control +/// that does nothing. +pub fn promote(at: &At) -> Option { + (at.index > 0).then(|| Edit::move_entry("provider", at.index, at.index - 1)) } -/// The edit that moves an entry one place towards the back. -pub fn demote(index: usize, len: usize) -> Option { - (index + 1 < len).then(|| Edit::move_entry("provider", index, index + 1)) +/// The edit that moves an entry one place towards the back of the chain. +/// +/// `None` for the last entry, bounded by the length [`At`] read from the file +/// rather than by a count the caller supplied. +pub fn demote(at: &At) -> Option { + (at.index + 1 < at.len).then(|| Edit::move_entry("provider", at.index, at.index + 1)) } /// The edit that removes an entry. -pub fn remove(index: usize) -> Edit { - Edit::remove(format!("provider[{index}]")) -} - -/// A TOML basic string, escaped. -fn quoted(text: &str) -> String { - let escaped = text.replace('\\', "\\\\").replace('"', "\\\""); - format!("\"{escaped}\"") +/// +/// Removing the first one promotes the second, because the chain is the array's +/// order and nothing else — worth saying out loud, since it is the one removal +/// on this surface that changes which provider a run uses. +pub fn remove(at: &At) -> Edit { + Edit::remove(format!("provider[{}]", at.index)) } diff --git a/src/servers.rs b/src/servers.rs index 9f922e3..9787da3 100644 --- a/src/servers.rs +++ b/src/servers.rs @@ -47,11 +47,36 @@ //! **Reconnect**, because there is nothing to cycle: servers are attached per //! turn through `TaskContract::with_mcp`. What an operator means by it is "pick //! up the edit I just made", and that is what the next turn does. +//! +//! # The write half, and the one number it must not be handed +//! +//! [`add`], [`edit`] and [`remove`] are the three verbs, and two of them address +//! an entry by **position in a file's `[[mcp]]` array**. That number is not a row +//! number, and this crate has already paid once for the difference: 0.20.0's +//! `pluginview::rows` drew two lists in an order no file shares, and handing one +//! of its row numbers to a remover would have deleted a bundle nobody named. So +//! neither verb takes a `usize` here — they take an [`At`], which only +//! [`At::of`] builds, by finding the id in the file's own bytes. +//! +//! **`[[mcp]]` is one of only two sections io-harness exempts from +//! `deny_unknown_fields`** (`config.rs:86`), which is what makes a wrong write +//! here quieter than anywhere else in the file: a misspelled key is accepted by +//! the parser and ignored by the harness, so the server starts with the setting +//! the operator thought they changed still at its default. That is why [`edit`] +//! refuses a key that is not one of [`KEYS`] instead of writing it, and why the +//! round-trip assertions in `tests/servers.rs` deserialise into [`McpServer`] +//! rather than looking for a string in the file. +//! +//! **A whole server is one edit and therefore one write.** [`crate::configure::write`] +//! splices, re-discovers and rolls back on refusal; a server that took two calls +//! would be two of those round trips with a half-written entry on disk between +//! them, and the second call is the one that can fail. use std::collections::{BTreeMap, BTreeSet}; +use std::path::Path; -use io_harness::config::Config; -use io_harness::EventKind; +use io_harness::config::{Config, Scope}; +use io_harness::{EventKind, McpServer, McpTransport}; use crate::configure::Decided; @@ -265,32 +290,204 @@ pub fn rows(servers: &[Server]) -> Vec { .collect() } -/// The edit that adds a server. +/// One entry's position in one file's `[[mcp]]` array. +/// +/// **The type exists so that a row number cannot be spelled as a position.** The +/// two are the same integer often enough to look interchangeable — `[[mcp]]` is +/// *not* one of io-harness's appending keys (`config.rs:2052`), so the winning +/// scope replaces the array whole and [`servers`] does list it in file order — +/// and that is exactly the shape of a bug that survives every test written on a +/// one-scope fixture. `pluginview` learned it the expensive way in 0.20.0. Here +/// the id is looked up in the file's own bytes instead, so the two lists never +/// have to agree. +/// +/// It carries the scope as well as the index, because a caller needs both and +/// they come from the same lookup: the index means nothing without the file it +/// counts in, and [`crate::configure::write`] takes the scope. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct At { + /// The scope whose file carries the entry — the scope a write must go to. + pub scope: Scope, + /// Private so nothing outside this module can spell one. [`At::of`] is the + /// only way to obtain one, and it reads a file to answer. + index: usize, +} + +impl At { + /// Where `text` declares the `[[mcp]]` entry whose `id` is `id`. + /// + /// `text` must be the bytes of the file `scope` names — this counts entries + /// in what it is given, and given the wrong file it would answer about the + /// wrong array. + /// + /// The comparison is made twice, against the source spelling and against the + /// unquoted text, because `id = "we\"ird"` is one id written two ways and + /// this module may not parse TOML to settle it (`tests/dependencies.rs`). + /// + /// `None` when no entry carries that id, and a caller must say so rather + /// than write to a position it guessed. + pub fn of(scope: Scope, text: &str, id: &str) -> Option { + let wanted = quoted(id); + // The array is walked until a gap, which is what `value_at` reports by + // returning `None` — an array of tables is contiguous, and `id` is a + // required field of `McpServer`, so the first miss is the end of it. + for index in 0.. { + let Some(raw) = crate::edit::value_at(text, &format!("mcp[{index}].id")) else { + break; + }; + let raw = raw.trim(); + if raw == wanted || unquoted(raw) == id { + return Some(At { scope, index }); + } + } + None + } + + /// The position, for a caller that has a sentence to write about it. + pub fn index(&self) -> usize { + self.index + } +} + +/// Where the file that configured `server` declares it. +/// +/// The bridge between a row on screen and an entry in a file, and it takes the +/// row rather than a number so there is nothing for a caller to get wrong. The +/// file is the one [`Server::decided`] already named — the same origin the panel +/// drew — so this cannot disagree with the column an operator is looking at. +/// +/// `None` for a server io-harness's own default supplied (there is no file to +/// edit) and for one whose deciding file no longer names it, which is what an +/// operator editing the file under the session looks like. +pub fn declared_at(server: &Server) -> Option { + let Decided::File { scope, path } = &server.decided else { + return None; + }; + let text = std::fs::read_to_string(path).ok()?; + At::of(*scope, &text, &server.id) +} + +/// Where a scope's file declares the server called `id`, read from disk. +/// +/// The form for a caller holding a root rather than a drawn row — an import, or +/// a command naming a server the panel is not showing. Searched in precedence +/// order and **stopped at the first scope that declares any `[[mcp]]` at all**, +/// which is the part that is not obvious: the array is replaced whole rather +/// than appended across scopes, so a lower file's entry is not merely +/// lower-priority, it is *not in force*. Editing it would change a file and +/// nothing else. +pub fn declared_in(root: &Path, id: &str) -> Option { + for scope in [Scope::Local, Scope::Project, Scope::User] { + let Some(path) = crate::configure::scope_path(root, scope) else { + continue; + }; + let Ok(text) = std::fs::read_to_string(&path) else { + continue; + }; + if crate::edit::value_at(&text, "mcp[0].id").is_none() { + continue; + } + // This scope decides the whole array, so the answer is here or nowhere. + return At::of(scope, &text, id); + } + None +} + +/// The keys an `[[mcp]]` entry may carry, which are [`McpServer`]'s own. +/// +/// A list rather than a free string because of the exemption in the module docs: +/// `[[mcp]]` is not held to `deny_unknown_fields`, so `comand = "…"` is written, +/// accepted, and ignored. The one write in this crate whose failure says nothing +/// at all is the one worth spending a `const` on. +pub const KEYS: &[&str] = &[ + "id", + "transport", + // Stdio. + "command", + "args", + "env", + // Http. + "url", + "headers", + "timeout_secs", +]; + +/// The edit that adds a whole server. /// /// A whole `[[mcp]]` entry, because an array of tables grows by gaining a block /// and [`crate::edit::Edit::set`] can only reach inside one that exists. -pub fn add(id: &str, command: &str) -> crate::edit::Edit { - crate::edit::Edit::append( - "mcp", - // `transport` is required: `McpTransport` is `#[serde(tag = "transport")]`, - // so the discriminant sits flat beside `id` rather than nesting, and an - // entry without it fails to load with `missing field \`transport\``. - format!( - "id = {}\ntransport = \"stdio\"\ncommand = {}", - quoted(id), - quoted(command) - ), - ) +/// +/// **Every field in the one edit, and that is the point.** A server whose `args` +/// and `env` arrived in a second [`crate::configure::write`] would be a second +/// discover-and-roll-back round trip over a file that, in between, held a server +/// declared without the arguments that make it work — and if the second call is +/// the one io-harness refuses, that is the state it stays in. +/// +/// It takes io-harness's own [`McpServer`] rather than a widening list of +/// parameters: that type is the authority on what an entry may carry, so a field +/// added to it is a compile error here rather than a key io-cli quietly stops +/// writing. The transport match below is exhaustive with no wildcard for the +/// reason the panel's own reader is: `McpTransport` is not `#[non_exhaustive]`, +/// so a variant a later io-harness adds breaks this build rather than being +/// written as a server that cannot be reached. +/// +/// `args`, `env` and `headers` are omitted when empty and `timeout_secs` when it +/// is the harness's own default — all four are `#[serde(default)]`, so what is +/// read back is the same server, and a file gains no line stating a default. +pub fn add(server: &McpServer) -> crate::edit::Edit { + // `transport` is required: `McpTransport` is `#[serde(tag = "transport")]`, + // so the discriminant sits flat beside `id` rather than nesting, and an + // entry without it fails to load with `missing field \`transport\``. + let mut body = format!("id = {}\n", quoted(&server.id)); + match &server.transport { + McpTransport::Stdio { command, args, env } => { + body.push_str("transport = \"stdio\"\n"); + body.push_str(&format!("command = {}\n", quoted(command))); + if !args.is_empty() { + let items: Vec<&str> = args.iter().map(String::as_str).collect(); + body.push_str(&format!("args = {}\n", crate::edit::array(&items))); + } + if !env.is_empty() { + body.push_str(&format!("env = {}\n", inline_table(env))); + } + } + McpTransport::Http { url, headers } => { + body.push_str("transport = \"http\"\n"); + body.push_str(&format!("url = {}\n", quoted(url))); + if !headers.is_empty() { + body.push_str(&format!("headers = {}\n", inline_table(headers))); + } + } + } + // Asked of the harness rather than written as `60` here: the default is + // io-harness's, `default_timeout_secs` is private, and a literal copied into + // io-cli is a number that goes stale in silence. + if server.timeout_secs != McpServer::stdio("", "").timeout_secs { + body.push_str(&format!("timeout_secs = {}\n", server.timeout_secs)); + } + crate::edit::Edit::append("mcp", body) } -/// The edit that changes one key of the `index`-th server. -pub fn edit(index: usize, key: &str, value: &str) -> crate::edit::Edit { - crate::edit::Edit::set(format!("mcp[{index}].{key}"), value.to_string()) +/// The edit that changes one key of the entry at `at`. +/// +/// `value` is TOML **source**, the way [`crate::edit::Edit::set`] takes it — +/// `"\"mcp-find\""`, `"[\"--verbose\"]"`, `"30"`. Build the first two with +/// [`quoted`] and [`crate::edit::array`] rather than a format string. +/// +/// `None` for a key that is not one of [`KEYS`]. **That refusal is the whole +/// reason this returns an `Option`**: an unknown key inside `[[mcp]]` is not +/// rejected by io-harness, so `configure::write`'s round trip would accept it +/// and an operator would be told their change landed while the server ran on the +/// old value. +pub fn edit(at: &At, key: &str, value: &str) -> Option { + let path = format!("mcp[{}].{key}", at.index); + KEYS.contains(&key) + .then(|| crate::edit::Edit::set(path, value.to_string())) } -/// The edit that removes the `index`-th server whole. -pub fn remove(index: usize) -> crate::edit::Edit { - crate::edit::Edit::remove(format!("mcp[{index}]")) +/// The edit that removes the entry at `at`, whole. +pub fn remove(at: &At) -> crate::edit::Edit { + crate::edit::Edit::remove(format!("mcp[{}]", at.index)) } /// A TOML basic string, escaped. @@ -298,7 +495,63 @@ pub fn remove(index: usize) -> crate::edit::Edit { /// Here rather than through `toml::to_string`, which would need a document to /// serialise: this crate writes VALUES, and a value is the one thing the /// serialiser cannot be asked for on its own. -fn quoted(text: &str) -> String { - let escaped = text.replace('\\', "\\\\").replace('"', "\\\""); - format!("\"{escaped}\"") +/// +/// **Public, because [`edit`] takes TOML source and a caller has a Rust string.** +/// The alternative every call site reaches for is `format!("\"{value}\"")`, which +/// is either a parse error or a different value the moment the text carries a +/// quote or a backslash — a Windows command path is full of the second. +/// +/// **Every escape TOML defines and not just the two that are obvious.** A quote +/// and a backslash are the pair a reader thinks of; a newline inside a basic +/// string is a parse error, and an MCP `env` value is whatever an imported +/// server definition put there. Getting it wrong is a refusal rather than a +/// corruption — [`crate::edit::apply`] reads its own result back — but a +/// refusal an operator cannot act on is still a verb that does not work. +pub fn quoted(text: &str) -> String { + let mut out = String::with_capacity(text.len() + 2); + out.push('"'); + for c in text.chars() { + match c { + '\\' => out.push_str("\\\\"), + '"' => out.push_str("\\\""), + '\n' => out.push_str("\\n"), + '\r' => out.push_str("\\r"), + '\t' => out.push_str("\\t"), + '\u{8}' => out.push_str("\\b"), + '\u{c}' => out.push_str("\\f"), + // The rest of C0, and DEL, which TOML forbids raw in a basic string. + c if (c as u32) < 0x20 || c as u32 == 0x7f => { + out.push_str(&format!("\\u{:04X}", c as u32)); + } + c => out.push(c), + } + } + out.push('"'); + out +} + +/// A TOML basic string with its quotes taken off, for comparing an id back. +fn unquoted(text: &str) -> String { + text.trim() + .trim_matches(|c: char| c == '"' || c == '\'') + .to_string() +} + +/// A map spelled as a TOML inline table. +/// +/// **Inline rather than a `[mcp.env]` sub-header**, because [`crate::edit`] cuts +/// a document at every line-leading `[`: a header inside an appended entry would +/// end that entry's region early, so `mcp[i].timeout_secs` written after it +/// would be looked for in a region that no longer contains it. An inline table +/// is one value, and one value is what this module writes. +/// +/// ponytail: only strings, because `env` and `headers` are the only two maps in +/// `[[mcp]]` and both are `BTreeMap`. A map of anything else +/// would need the value spelled by kind, and there is none to spell. +fn inline_table(pairs: &BTreeMap) -> String { + let body: Vec = pairs + .iter() + .map(|(key, value)| format!("{} = {}", quoted(key), quoted(value))) + .collect(); + format!("{{ {} }}", body.join(", ")) } diff --git a/src/skillview.rs b/src/skillview.rs index 39d8723..43c853b 100644 --- a/src/skillview.rs +++ b/src/skillview.rs @@ -45,6 +45,28 @@ //! 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. //! +//! # A bundle's skills belong here too, and one of them can kill the session +//! +//! 0.20.0 gave a `[[plugin]]` bundle a skills directory, and every skill in one +//! reaches the model — namespaced `bundle__skill`, the form io-harness itself +//! builds out of [`io_harness::NAMESPACE`]. None of them appeared on any surface +//! that lists a skill, so an operator reading `/skills` was reading a list that +//! disagreed with the catalogue the turn was handed. [`view`] takes the bundles +//! and lists them under exactly the name the model addresses. +//! +//! The other half is worse than a missing row. `TaskContract::discover_skills` +//! walks every bundle's declared directory with the same `Skills::discover` this +//! module calls, and every caller of it uses `?` — while `Plugin::skills_dir` +//! does no existence check whatever, it is the manifest's word joined onto the +//! root. So a bundle naming a `skills` directory that is not on disk is a session +//! in which **every turn dies at run start, before the first completion**, and +//! io-cli has had nowhere that says so. [`View::bundles_failed`] is that place. +//! That is why the error may neither be swallowed nor propagated: swallowed, the +//! one surface that could explain the dead session draws nothing; propagated, it +//! takes the surface down with it and the operator is left with the same silence, +//! having also lost every row that was fine. Each bundle costs exactly itself, +//! which is what [`crate::pluginview`] already promises about the same bundles. +//! //! # Disabling is a rename and never a copy //! //! A copy leaves the skill in both directories, which is one resolved name @@ -57,6 +79,13 @@ //! be dead code whose only reachable behaviour is the failure it is meant to //! avoid. //! +//! And neither lever is offered a bundle's file at all. [`disable`] would compute +//! its destination as the file's own parent joined to `disabled/`, which for a +//! bundle skill means io-cli creating a directory inside somebody else's bundle +//! and moving their file into it — a bundle io-cli does not own, did not install +//! and cannot put back. Both levers refuse, and they refuse *inside themselves* +//! rather than at the call site; see [`disable`]. +//! //! 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 @@ -92,13 +121,26 @@ const NO_DESCRIPTION: &str = "(no description)"; const PATH_FLOOR: usize = 20; /// Who wrote a skill. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +/// +/// **No longer `Copy` from 0.21.0**, because [`Origin::Bundle`] carries the +/// bundle's own name and that name is the only honest word for a row that is +/// neither io-cli's nor the operator's. [`Origin::word`] argues the trade; the +/// cost falls entirely on call sites that already hold a `&Listed`. +#[derive(Debug, Clone, 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, + /// A `[[plugin]]` bundle's, named by the manifest `name` that `/plugin` lists + /// it under and that its skills are namespaced by. + /// + /// **A third answer rather than folding into [`Origin::Yours`].** The + /// operator owns neither the file nor the directory holding it, and a row + /// saying `yours` would be telling them io-cli may move it — which is the one + /// thing [`disable`] and [`enable`] refuse to do. + Bundle(String), } impl Origin { @@ -111,11 +153,26 @@ impl Origin { /// 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. + /// + /// # Borrowed rather than `&'static str`, so a bundle draws its own name + /// + /// The alternative considered was the fixed word `"bundle"` with the id + /// carried in a second field, and it fails the argument above in exactly the + /// way the filled dot does: two bundles each contributing a skill would draw + /// the identical word, and an operator reading it would have no way to know + /// which of the two directories to go and edit. The name is the useful word, + /// so the name is the word, and the return type widens to `&str` to say it. + /// + /// It does repeat what the row's name already carries, since a bundle skill + /// is listed namespaced. Deliberately: the origin column is where an operator + /// reads *whose* a row is, and it has to answer that for every row or it is + /// not a column, only a column that is sometimes filled in. #[must_use] - pub fn word(self) -> &'static str { + pub fn word(&self) -> &str { match self { Origin::IoCli => "io-cli", Origin::Yours => "yours", + Origin::Bundle(id) => id, } } } @@ -155,6 +212,24 @@ pub struct View { /// 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, + /// The bundles whose skills directory would not discover: the bundle's id and + /// io-harness's own sentence, one pair per bundle, in the order they were + /// walked. + /// + /// **A separate field rather than folding into [`View::failed`], because they + /// are separate failures with separate consequences.** `failed` is the + /// operator's own directory not discovering, which costs the enabled list and + /// nothing else — the session still runs. A bundle's directory not discovering + /// costs *the session*: `TaskContract::discover_skills` walks the same + /// directory with the same call and propagates the error at run start, so + /// every turn dies before the first completion. One `Option` holding + /// either would make the surface unable to say which of the two happened, on + /// the one question where the difference is everything. + /// + /// A `Vec` and not an `Option` for the same reason [`crate::pluginview::View`] + /// keeps a list: two broken bundles are two facts, and reporting the first + /// would send an operator to fix one directory and meet the same dead session. + pub bundles_failed: Vec<(String, String)>, } /// Every skill in `dir` and in `dir/disabled`. @@ -168,10 +243,27 @@ pub struct View { /// /// `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. +/// +/// # `bundles` is a slice of plain pairs, and that is a deliberate refusal +/// +/// Each pair is a bundle's id and the skills directory it declared — precisely +/// what `crate::pluginview::Listed` already carries in its `id` and `skills` +/// fields, and precisely nothing else. It is **not** an [`io_harness::Config`] +/// and **not** a `pluginview::View`, because either one would tie this module to +/// the configuration types, and every test in this file would then have to build +/// a configuration to ask a question about a directory. The driver is already the +/// thing that holds a `pluginview::View`; turning it into pairs is one `map` at +/// the one call site, and it keeps the whole of this surface answerable from a +/// fixture directory. +/// +/// The slice is walked **in the order given**, which the driver takes from +/// `Config::plugins()` — the same order `TaskContract::discover_skills` folds +/// the directories in, so a name that collides collides the same way here. #[must_use] -pub fn view(home: &Path, dir: &Path) -> View { +pub fn view(home: &Path, dir: &Path, bundles: &[(String, PathBuf)]) -> View { let mut listed = Vec::new(); let mut failed = None; + let mut bundles_failed = Vec::new(); match io_harness::Skills::discover(dir) { Ok(found) => listed.extend(found.iter().map(|skill| Listed { @@ -184,6 +276,10 @@ pub fn view(home: &Path, dir: &Path) -> View { Err(error) => failed = Some(error.to_string()), } + let (from_bundles, failures) = bundle_rows(bundles); + listed.extend(from_bundles); + bundles_failed.extend(failures); + for path in disabled_files(dir) { let (name, description) = describe(&path); listed.push(Listed { @@ -195,14 +291,85 @@ pub fn view(home: &Path, dir: &Path) -> View { }); } - // 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. + sorted(listed, failed, bundles_failed) +} + +/// Every bundle's skills, for a home that has no skills directory of its own. +/// +/// **A separate entry point rather than an `Option<&Path>` on [`view`].** The +/// caller either has a directory the run reads or it does not, and threading an +/// `Option` through would put the question in every one of this module's tests +/// rather than in the one place that can answer it. There is no `home` argument +/// because there is nothing to ask the manifest about: every row here is +/// [`Origin::Bundle`] by construction. +/// +/// This is not a rare path. `crate::contract::skills_dir` answers `None` whenever +/// the operator has never made `~/.io-cli/skills`, which is the ordinary state of +/// a fresh install — and it is exactly the state in which every skill the model +/// is offered came from a bundle. Returning an empty view here would blank the +/// surface precisely when it is the only listing there is. +#[must_use] +pub fn view_of_bundles(bundles: &[(String, PathBuf)]) -> View { + let (listed, bundles_failed) = bundle_rows(bundles); + sorted(listed, None, bundles_failed) +} + +/// One row per skill each bundle contributes, and one sentence per bundle that +/// would not discover. Shared by [`view`] and [`view_of_bundles`] so the two can +/// never disagree about what a bundle contributed. +fn bundle_rows(bundles: &[(String, PathBuf)]) -> (Vec, Vec<(String, String)>) { + let mut listed = Vec::new(); + let mut bundles_failed = Vec::new(); + for (id, bundle) in bundles { + match io_harness::Skills::discover(bundle) { + // **The namespaced name, rebuilt rather than borrowed.** `Skills` + // does namespace its own — `merged` and `namespaced` are both + // `pub(crate)` in io-harness and the type has no public constructor — + // so the one honest thing io-cli can do is reproduce the format the + // harness uses, out of the harness's own constant. A row listed under + // the bare `skill` would be listed under a name the model cannot + // address, on the surface that exists to say what the model is offered. + Ok(found) => listed.extend(found.iter().map(|skill| Listed { + name: format!("{id}{}{}", io_harness::NAMESPACE, skill.name), + description: skill.description.clone(), + // Never the manifest's answer. `crate::skills::wrote` asks whether + // io-cli last wrote these bytes, and for a file in somebody else's + // bundle the answer is no in a way that means something different + // from the operator having written it. + origin: Origin::Bundle(id.clone()), + // No lever reaches it, so there is no state but on. `disabled/` + // is io-cli's mechanism inside io-cli's own directory. + enabled: true, + path: skill.path.clone(), + })), + // Recorded and walked past, never `?`. See [`View::bundles_failed`]: + // this is the session-killing failure, and it is the only failure on + // this surface where taking the surface down would take away the one + // report that explains it. + Err(error) => bundles_failed.push((id.clone(), error.to_string())), + } + } + (listed, bundles_failed) +} + +/// The rows in the order the surface draws them, wrapped in a [`View`]. +/// +/// By name, then by path. The name is what an operator is looking for and is +/// what discovery sorts by, so the sources interleave rather than sitting in +/// blocks; the path breaks the tie a name claimed in two directories would +/// otherwise leave to `read_dir` order. A bundle's rows sort under their +/// namespaced names, so a bundle's skills land together without this sort +/// needing to know a bundle exists. +fn sorted( + mut listed: Vec, + failed: Option, + bundles_failed: Vec<(String, String)>, +) -> View { listed.sort_by(|left, right| (&left.name, &left.path).cmp(&(&right.name, &right.path))); View { skills: listed, failed, + bundles_failed, } } @@ -415,12 +582,35 @@ pub fn rows(skills: &[Listed], width: u16, glyphs: &Glyphs) -> Vec { /// 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. /// +/// # Not a `[[plugin]]` bundle's file, and the guard is in here rather than up there +/// +/// The destination below is `path.parent().join(DISABLED)`. For a bundle's skill +/// that parent is the *bundle's* skills directory, so io-cli would create a +/// `disabled/` inside a directory it does not own and move somebody else's file +/// into it — where the bundle's next update will not find it, `/skills` will not +/// list it, and only the operator's memory says where it went. +/// +/// `is_bundle` cannot see this: it decides a `SKILL.md` folder *shape* and knows +/// nothing about whose directory a path is in. Two different senses of one word, +/// and two guards. +/// +/// **`bundles` is taken here, and by [`enable`], rather than the call site testing +/// [`Origin::Bundle`] before it calls.** A guard at the call site is a guard the +/// next call site does not have, and this one protects a stranger's files from an +/// operation that cannot be undone by the surface that performed it. Taking the +/// slice costs both callers an argument they already hold — it is the same slice +/// [`view`] took to produce the row being acted on — and buys that the move is +/// unreachable with a bundle path from anywhere. +/// /// 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 { +pub fn disable(path: &Path, bundles: &[(String, PathBuf)]) -> Result { + if let Some(refusal) = refuse_bundle(path, bundles) { + return Err(refusal); + } if is_bundle(path) { return Err(format!( "{} is a skill folder rather than a single file; move the folder into the disabled \ @@ -446,7 +636,15 @@ pub fn disable(path: &Path) -> Result { /// /// The inverse of [`disable`] byte for byte: neither reads the file, so neither /// can change it. -pub fn enable(path: &Path) -> Result { +/// +/// It refuses a bundle's file for [`disable`]'s reason and by the same guard, +/// even though no bundle file can ever be in a `disabled/` this surface made. The +/// pair is symmetric on purpose: a lever that refuses in one direction and not +/// the other is a lever whose rule a reader has to look up. +pub fn enable(path: &Path, bundles: &[(String, PathBuf)]) -> Result { + if let Some(refusal) = refuse_bundle(path, bundles) { + return Err(refusal); + } // 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 @@ -486,7 +684,38 @@ pub fn enable(path: &Path) -> Result { relocate(path, dir) } +/// The sentence for a path that lives inside a `[[plugin]]` bundle, if it does. +/// +/// **Containment, not equality.** io-harness walks a skills directory one level +/// down for `/SKILL.md` as well as reading the loose files in it, so a +/// bundle skill's path is not always a direct child of the declared directory. +/// +/// Both sides are canonicalised before they are compared, because the two come +/// from different places: a path off [`view`] came through `Skills::discover`, +/// which canonicalises, and a declared directory is `Plugin::skills_dir` — the +/// bundle root joined to whatever the manifest wrote, canonical or not. On macOS +/// a temporary directory is a symlink, which is enough on its own to make an +/// uncanonicalised comparison answer "not a bundle" about a bundle. A path that +/// will not canonicalise is compared as it stands: the file being gone is +/// [`relocate`]'s error to report, not a reason to relax this. +fn refuse_bundle(path: &Path, bundles: &[(String, PathBuf)]) -> Option { + let resolved = std::fs::canonicalize(path).unwrap_or_else(|_| path.to_path_buf()); + let (id, dir) = bundles.iter().find(|(_, dir)| { + resolved.starts_with(std::fs::canonicalize(dir).unwrap_or_else(|_| dir.clone())) + })?; + Some(format!( + "{} belongs to the plugin `{id}`, so io-cli did not move it — a bundle's \ + skills are turned off in the bundle's own directory, {}", + path.display(), + dir.display(), + )) +} + /// Whether a path is a `SKILL.md` inside its own directory. +/// +/// A different sense of the word from [`refuse_bundle`]'s: this one is a *file +/// shape* — one skill spread over a directory — and says nothing about whose +/// directory it is in. Both are checked, and neither substitutes for the other. fn is_bundle(path: &Path) -> bool { path.file_name() .is_some_and(|name| name.eq_ignore_ascii_case("SKILL.md")) diff --git a/tests/commands.rs b/tests/commands.rs index 223b05a..cf693d4 100644 --- a/tests/commands.rs +++ b/tests/commands.rs @@ -69,6 +69,10 @@ fn the_commands_are_the_commands() { // can hand over skills, templates, agents, servers, hooks and policy // at once. That breadth is the argument for it being visible at all. "/plugin", + // 0.21.0 — the operator's own work, carried across from whatever + // agent they were using before this one. It is the only command here + // whose subject is a tool that is not io. + "/import", "/profile", "/contain", // 0.12.0 — the planning phase stopped being something @@ -425,13 +429,13 @@ fn f9_exit_is_listed_and_its_palette_row_leaves() { // Found by name rather than by the inventory's own index: since 0.16.0 the // palette is grouped, so its rows are the commands in GROUP order with a // heading before each group, and a position in `COMMANDS` addresses neither. - let rows = palette(&Templates::none(), &io_harness::Skills::none()); + let rows = palette(&Templates::none(), &[]); let index = rows .iter() .position(|row| row.label == "exit") .expect("`/exit` has a row"); assert_eq!( - palette_pick(&Templates::none(), &io_harness::Skills::none(), index), + palette_pick(&Templates::none(), &[], index), Some(Chosen::Command("/exit")), "the row is advertised and inert", ); @@ -645,7 +649,7 @@ fn remember_keeps_the_whole_line_and_memory_takes_no_argument() { #[test] fn the_memory_commands_are_configure_commands_with_working_palette_rows() { use io_cli::commands::{group_of, palette, palette_pick, Chosen, Group}; - use io_harness::{Skills, Templates}; + use io_harness::Templates; for name in ["/remember", "/memory"] { assert!( @@ -662,14 +666,14 @@ fn the_memory_commands_are_configure_commands_with_working_palette_rows() { "{name} writes a file the operator keeps, so it belongs with the group that writes", ); - let rows = palette(&Templates::none(), &Skills::none()); + let rows = palette(&Templates::none(), &[]); let bare = name.strip_prefix('/').expect("a command carries its slash"); let index = rows .iter() .position(|row| row.label == bare) .unwrap_or_else(|| panic!("{name} has no palette row")); assert_eq!( - palette_pick(&Templates::none(), &Skills::none(), index), + palette_pick(&Templates::none(), &[], index), Some(Chosen::Command(name)), "{name}'s row is advertised and inert", ); @@ -695,7 +699,7 @@ fn the_memory_commands_are_configure_commands_with_working_palette_rows() { #[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}; + use io_harness::Templates; for name in ["/mcp", "/provider"] { assert_eq!( @@ -730,13 +734,13 @@ fn the_writers_are_configure_commands_and_skills_is_the_inspection() { "Inspect holds {inspect}; the point of the move was to leave room", ); - let rows = palette(&Templates::none(), &Skills::none()); + let rows = palette(&Templates::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), + palette_pick(&Templates::none(), &[], index), Some(Chosen::Command("/skills")), "`/skills`'s row is advertised and inert", ); @@ -1085,7 +1089,7 @@ fn f17_usage_answers_and_is_never_listed() { for (_, names) in GROUPS { assert!(!names.contains(&"/usage")); } - let rows = palette(&io_harness::Templates::none(), &io_harness::Skills::none()); + let rows = palette(&io_harness::Templates::none(), &[]); assert!( !rows.iter().any(|row| row.label == "usage"), "a second row for one screen reads as a second screen", diff --git a/tests/cursor.rs b/tests/cursor.rs index 8c67207..950b913 100644 --- a/tests/cursor.rs +++ b/tests/cursor.rs @@ -476,7 +476,7 @@ fn f2_the_composer_frame_sets_a_cursor_even_when_it_is_too_narrow_to_draw() { fn palette() -> Picker { Picker::new( "Which command?", - io_cli::commands::palette(&io_harness::Templates::none(), &io_harness::Skills::none()), + io_cli::commands::palette(&io_harness::Templates::none(), &[]), ) } diff --git a/tests/import.rs b/tests/import.rs new file mode 100644 index 0000000..15ab26d --- /dev/null +++ b/tests/import.rs @@ -0,0 +1,870 @@ +//! `/import` — what another agent tool already knows, brought across once. +//! +//! Every fixture here is a temporary directory standing in for an operator's +//! home, so nothing in this file can read the machine it runs on. That is the +//! same property `src/import.rs` is built for — the roots are parameters, `$HOME` +//! is never consulted — and it is what makes the credential assertions below mean +//! anything: a test that reached the real `~/.codex` could not prove a secret was +//! never copied, because there would be a second copy of it already. +//! +//! No clocks and no sleeps. Every assertion is about bytes on disk or a value +//! parsed out of them. + +use std::path::{Path, PathBuf}; + +use io_cli::import::{apply, detect, files, plan, Destination, Kind, Source}; +use io_harness::config::Scope; + +/// The value that must never reach a file io-cli writes. +const SECRET: &str = "sk-live-this-must-never-be-copied"; + +/// A variable no machine sets, so `${env:…}` for it is unresolvable. +const UNSET: &str = "IO_CLI_IMPORT_TEST_TOKEN"; + +fn put(path: &Path, text: &str) { + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent).expect("a fixture directory"); + } + std::fs::write(path, text).expect("a fixture file"); +} + +/// Every file under `dir`, as `(relative path, bytes)`, sorted. +fn contents(dir: &Path) -> Vec<(PathBuf, Vec)> { + fn walk(dir: &Path, base: &Path, out: &mut Vec<(PathBuf, Vec)>) { + let Ok(entries) = std::fs::read_dir(dir) else { + return; + }; + for entry in entries.flatten() { + let path = entry.path(); + if path.is_dir() { + walk(&path, base, out); + } else if let Ok(bytes) = std::fs::read(&path) { + let relative = path.strip_prefix(base).unwrap_or(&path).to_path_buf(); + out.push((relative, bytes)); + } + } + } + let mut out = Vec::new(); + walk(dir, dir, &mut out); + out.sort(); + out +} + +/// One number for a whole directory tree, through the crate's own digest. +fn fingerprint(dir: &Path) -> u64 { + let mut bytes = Vec::new(); + for (path, content) in contents(dir) { + bytes.extend_from_slice(path.to_string_lossy().as_bytes()); + bytes.push(0); + bytes.extend_from_slice(&content); + bytes.push(0); + } + io_cli::skills::digest(&bytes) +} + +/// Whether any file under `dir` holds `needle`. +fn holds(dir: &Path, needle: &str) -> Option { + contents(dir).into_iter().find_map(|(path, bytes)| { + String::from_utf8_lossy(&bytes) + .contains(needle) + .then_some(path) + }) +} + +/// The `[[mcp]]` entries of a written configuration, as io-harness's own type. +/// +/// **Deserialised, never string-matched.** The question this file has to answer +/// is "will io-harness read back what io-cli wrote", and a substring assertion +/// answers a different one — it would pass on a body that spelled every key right +/// and left off `transport`, which is the one field `#[serde(flatten)]` makes +/// mandatory and easy to forget. +fn written_servers(path: &Path) -> Vec { + let text = std::fs::read_to_string(path).expect("the configuration was written"); + let document: toml::Value = toml::from_str(&text).expect("it parses"); + let Some(entries) = document.get("mcp").and_then(toml::Value::as_array) else { + return Vec::new(); + }; + entries + .iter() + .map(|entry| -> io_harness::McpServer { + entry + .clone() + .try_into() + .expect("every entry is an `io_harness::McpServer`") + }) + .collect() +} + +fn stdio(server: &io_harness::McpServer) -> (String, Vec, Vec) { + match &server.transport { + io_harness::McpTransport::Stdio { command, args, env } => ( + command.clone(), + args.clone(), + env.keys().cloned().collect::>(), + ), + other => panic!("expected a stdio server, got {other:?}"), + } +} + +// --------------------------------------------------------------------------- +// A. Detection +// --------------------------------------------------------------------------- + +#[test] +fn every_source_on_the_machine_is_found_and_named() { + let home_root = tempfile::tempdir().expect("a fake home"); + let workspace = tempfile::tempdir().expect("a workspace"); + + put( + &home_root.path().join(".claude/CLAUDE.md"), + "- be careful\n", + ); + put(&home_root.path().join(".claude.json"), "{}"); + put(&home_root.path().join(".codex/AGENTS.md"), "- be brief\n"); + put(&home_root.path().join(".gemini/GEMINI.md"), "- hello\n"); + put(&workspace.path().join(".cursorrules"), "no tabs\n"); + put(&workspace.path().join("CONVENTIONS.md"), "# conventions\n"); + + let found = detect(home_root.path(), workspace.path()); + let sources: Vec = found.iter().map(|one| one.source).collect(); + assert_eq!( + sources, + vec![ + Source::Claude, + Source::Codex, + Source::Gemini, + Source::Cursor, + Source::Conventions, + ], + "every source with a file on disk is reported, in the order a surface lists them", + ); + assert!( + found.iter().all(|one| !one.empty()), + "none of these fixtures is empty: {:?}", + found.iter().map(|one| one.bytes).collect::>(), + ); +} + +#[test] +fn a_source_with_nothing_in_its_files_is_found_and_reported_empty() { + let home_root = tempfile::tempdir().expect("a fake home"); + let workspace = tempfile::tempdir().expect("a workspace"); + + // The state Gemini is actually in on the machine this module was written + // against: three files, all of them zero bytes. + put(&home_root.path().join(".gemini/GEMINI.md"), ""); + put( + &home_root.path().join(".gemini/antigravity/mcp_config.json"), + "", + ); + + let found = detect(home_root.path(), workspace.path()); + assert_eq!(found.len(), 1, "Gemini is there: {found:?}"); + assert_eq!(found[0].source, Source::Gemini); + assert_eq!(found[0].paths.len(), 2, "both files are named"); + assert!( + found[0].empty(), + "found-but-empty is a state of its own, not the same answer as absent", + ); + assert!( + found[0].says().contains("nothing to bring across"), + "the sentence says so: {}", + found[0].says(), + ); + + assert!( + plan(&found, home_root.path(), Scope::Project).is_empty(), + "an empty file offers the operator nothing to accept", + ); +} + +#[test] +fn a_source_that_is_not_installed_is_not_found_at_all() { + let home_root = tempfile::tempdir().expect("a fake home"); + let workspace = tempfile::tempdir().expect("a workspace"); + put(&home_root.path().join(".codex/AGENTS.md"), "- be brief\n"); + + let found = detect(home_root.path(), workspace.path()); + assert_eq!( + found.iter().map(|one| one.source).collect::>(), + vec![Source::Codex], + "a source with no files is absent from the answer rather than present and empty", + ); +} + +// --------------------------------------------------------------------------- +// The plan is a plan +// --------------------------------------------------------------------------- + +#[test] +fn a_plan_that_is_declined_writes_nothing_and_leaves_the_home_byte_identical() { + let home_root = tempfile::tempdir().expect("a fake home"); + let workspace = tempfile::tempdir().expect("a workspace"); + let home = tempfile::tempdir().expect("io's home"); + + put( + &home_root.path().join(".claude/CLAUDE.md"), + "- be careful\n", + ); + put( + &home_root.path().join(".claude.json"), + r#"{"mcpServers":{"semlith":{"command":"semlith","args":["mcp"]}}}"#, + ); + put( + &home_root.path().join(".claude/skills/thing/SKILL.md"), + "# thing\n", + ); + put(&home.path().join("io.toml"), ""); + put(&home.path().join("skills/.gitkeep"), ""); + + let found = detect(home_root.path(), workspace.path()); + let items = plan(&found, home.path(), Scope::Project); + assert!( + items.len() >= 3, + "there is a real plan to decline: {items:?}", + ); + + let before = (fingerprint(home.path()), fingerprint(workspace.path())); + + // The operator said no to all of it. What a surface hands back is the + // ACCEPTED subset, and that subset is empty. + let report = apply(&[], workspace.path()); + + assert!(report.written.is_empty(), "{report:?}"); + assert_eq!(report.lines(), vec!["nothing was imported".to_string()]); + assert_eq!( + (fingerprint(home.path()), fingerprint(workspace.path())), + before, + "building a plan is a read; declining it must not have moved a byte", + ); +} + +// --------------------------------------------------------------------------- +// C. MCP round trips +// --------------------------------------------------------------------------- + +#[test] +fn a_claude_mcp_entry_round_trips_into_an_mcp_server() { + let home_root = tempfile::tempdir().expect("a fake home"); + let workspace = tempfile::tempdir().expect("a workspace"); + let home = tempfile::tempdir().expect("io's home"); + + // Where they actually are: under `projects[""]`, not beside + // `settings.json` and not under `~/.claude/` at all. + put( + &home_root.path().join(".claude.json"), + r#"{ + "mcpServers": {}, + "projects": { + "/somewhere/else": { + "mcpServers": { + "semlith": { + "type": "stdio", + "command": "semlith", + "args": ["--store", "/s", "mcp"] + } + } + } + } + }"#, + ); + + let found = detect(home_root.path(), workspace.path()); + let items: Vec<_> = plan(&found, home.path(), Scope::Project) + .into_iter() + .filter(|item| item.kind == Kind::Mcp) + .collect(); + assert_eq!(items.len(), 1, "{items:?}"); + + let report = apply(&items, workspace.path()); + assert!(report.refused.is_empty(), "{report:?}"); + assert_eq!(report.written.len(), 1, "{report:?}"); + + let servers = written_servers(&workspace.path().join("io.toml")); + assert_eq!(servers.len(), 1); + assert_eq!(servers[0].id, "semlith"); + let (command, args, env) = stdio(&servers[0]); + assert_eq!(command, "semlith"); + assert_eq!(args, vec!["--store", "/s", "mcp"]); + assert!(env.is_empty(), "this entry declared no environment"); + assert!( + servers[0].timeout_secs > 0, + "the timeout is io-harness's own default, not a zero this crate invented", + ); +} + +#[test] +fn a_codex_mcp_table_round_trips_into_an_mcp_server() { + let home_root = tempfile::tempdir().expect("a fake home"); + let workspace = tempfile::tempdir().expect("a workspace"); + let home = tempfile::tempdir().expect("io's home"); + + put( + &home_root.path().join(".codex/config.toml"), + "model = \"gpt-5\"\n\ + \n\ + [mcp_servers.semlith]\n\ + command = \"semlith\"\n\ + args = [\"--store\", \"/s\", \"mcp\"]\n\ + startup_timeout_sec = 30\n", + ); + + let found = detect(home_root.path(), workspace.path()); + let all = plan(&found, home.path(), Scope::Project); + + let model: Vec<_> = all.iter().filter(|item| item.kind == Kind::Model).collect(); + assert_eq!(model.len(), 1, "{all:?}"); + assert_eq!(model[0].model(), Some("gpt-5")); + assert_eq!( + model[0].to, + Destination::Nowhere, + "a model id does not name a vendor, so nothing is written for it here", + ); + assert!( + model[0] + .provider_edit(io_cli::providers::Endpoint::OpenAi) + .is_some(), + "the caller that HAS chosen a vendor gets the edit", + ); + + let items: Vec<_> = all + .into_iter() + .filter(|item| item.kind == Kind::Mcp) + .collect(); + assert_eq!(items.len(), 1); + + let report = apply(&items, workspace.path()); + assert!(report.refused.is_empty(), "{report:?}"); + + let servers = written_servers(&workspace.path().join("io.toml")); + assert_eq!(servers.len(), 1); + assert_eq!(servers[0].id, "semlith"); + let (command, args, env) = stdio(&servers[0]); + assert_eq!(command, "semlith"); + assert_eq!( + args, + vec!["--store", "/s", "mcp"], + "the argument list survives the TOML source it was quoted out of", + ); + assert!(env.is_empty()); +} + +// --------------------------------------------------------------------------- +// D. Credentials +// --------------------------------------------------------------------------- + +#[test] +fn a_secret_in_a_servers_environment_reaches_no_file_io_cli_writes() { + let home_root = tempfile::tempdir().expect("a fake home"); + let workspace = tempfile::tempdir().expect("a workspace"); + let home = tempfile::tempdir().expect("io's home"); + + put( + &home_root.path().join(".codex/config.toml"), + &format!( + "[mcp_servers.paid]\n\ + command = \"paid-mcp\"\n\ + \n\ + [mcp_servers.paid.env]\n\ + {UNSET} = \"{SECRET}\"\n" + ), + ); + put( + &home_root.path().join(".claude.json"), + &format!( + r#"{{"mcpServers":{{"other":{{"command":"other-mcp","env":{{"{UNSET}":"{SECRET}"}}}}}}}}"# + ), + ); + + let found = detect(home_root.path(), workspace.path()); + let items = plan(&found, home.path(), Scope::Project); + let mcp: Vec<_> = items + .iter() + .filter(|item| item.kind == Kind::Mcp) + .cloned() + .collect(); + assert_eq!(mcp.len(), 2, "{items:?}"); + + for item in &mcp { + let form = item.form.as_deref().expect("a translated body"); + assert!( + form.contains(&format!("${{env:{UNSET}}}")), + "the NAME points at itself: {form}", + ); + assert!( + !form.contains(SECRET), + "the value was read and written: {form}", + ); + assert!( + !item.says.contains(SECRET), + "the value reached the sentence an operator sees: {}", + item.says, + ); + } + + let report = apply(&mcp, workspace.path()); + + assert_eq!( + holds(workspace.path(), SECRET), + None, + "the secret reached the workspace", + ); + assert_eq!( + holds(home.path(), SECRET), + None, + "the secret reached io's home", + ); + + // **And the unresolvable reference is refused at import time rather than at + // the operator's next session.** io-harness treats an unset `${env:…}` as a + // hard parse error, and `configure::write` round-trips through + // `Config::discover` and rolls the file back when it does — so a server whose + // secret has not been exported yet costs a sentence here instead of a dead + // session later. Guarded on the variable genuinely being unset, because that + // is the premise rather than the claim. + if std::env::var_os(UNSET).is_none() { + assert!( + report.written.is_empty(), + "an unresolvable `${{env:}}` must not be left in the file: {report:?}", + ); + assert_eq!(report.refused.len(), 2, "{report:?}"); + let at = workspace.path().join("io.toml"); + assert!( + !at.exists() || written_servers(&at).is_empty(), + "the rollback left a `[[mcp]]` entry behind", + ); + } +} + +#[test] +fn the_codex_credential_file_is_never_listed_and_never_read() { + let home_root = tempfile::tempdir().expect("a fake home"); + let workspace = tempfile::tempdir().expect("a workspace"); + let home = tempfile::tempdir().expect("io's home"); + + put( + &home_root.path().join(".codex/auth.json"), + &format!(r#"{{"OPENAI_API_KEY":"{SECRET}"}}"#), + ); + put(&home_root.path().join(".codex/AGENTS.md"), "- be brief\n"); + + let listed = files(Source::Codex, home_root.path(), workspace.path()); + assert!( + !listed.iter().any(|path| path.ends_with("auth.json")), + "`auth.json` is in the list of files this module reads: {listed:?}", + ); + + let found = detect(home_root.path(), workspace.path()); + let items = plan(&found, home.path(), Scope::Project); + assert!( + items.iter().all(|item| !item.says.contains(SECRET) + && item + .form + .as_deref() + .is_none_or(|form| !form.contains(SECRET))), + "a credential reached the plan: {items:?}", + ); + + apply(&items, workspace.path()); + assert_eq!(holds(workspace.path(), SECRET), None); + assert_eq!(holds(home.path(), SECRET), None); +} + +// --------------------------------------------------------------------------- +// Instructions +// --------------------------------------------------------------------------- + +#[test] +fn instructions_land_somewhere_config_discover_reports() { + let home_root = tempfile::tempdir().expect("a fake home"); + let workspace = tempfile::tempdir().expect("a workspace"); + let home = tempfile::tempdir().expect("io's home"); + + put( + &home_root.path().join(".claude/CLAUDE.md"), + "# rules\n\nAlways run the linter before committing.\n", + ); + + let found = detect(home_root.path(), workspace.path()); + let items: Vec<_> = plan(&found, home.path(), Scope::Project) + .into_iter() + .filter(|item| item.kind == Kind::Instructions) + .collect(); + assert_eq!(items.len(), 1, "{items:?}"); + assert_eq!( + items[0].to, + Destination::Instructions(Scope::Project), + "the scope the caller asked for", + ); + + let report = apply(&items, workspace.path()); + assert!(report.refused.is_empty(), "{report:?}"); + assert_eq!( + report.written.len(), + 1, + "the report names the destination, which is what makes this undoable by hand: {report:?}", + ); + assert_eq!(report.written[0].0, workspace.path().join("AGENTS.md")); + + // The oracle is io-harness's own discovery, not the file being on disk: + // `AGENTS.md` is `DEFAULT_INSTRUCTIONS`, so a project with no configuration + // at all reads it — and this asserts that it did. + let config = io_harness::config::Config::discover(workspace.path()) + .expect("the workspace configuration is readable"); + let read = config.instructions().join("\n"); + assert!( + read.contains("Always run the linter before committing."), + "io-harness did not read the imported instructions back: {read}", + ); + assert!( + read.contains("imported from"), + "the provenance line went with it: {read}", + ); +} + +// --------------------------------------------------------------------------- +// Skills +// --------------------------------------------------------------------------- + +#[test] +fn a_skill_nested_five_deep_arrives_flat_and_is_found_by_discovery() { + let home_root = tempfile::tempdir().expect("a fake home"); + let workspace = tempfile::tempdir().expect("a workspace"); + let home = tempfile::tempdir().expect("io's home"); + std::fs::create_dir_all(home.path().join("skills")).expect("a skills directory"); + + put( + &home_root + .path() + .join(".claude/plugins/repo/marketplace/pack/skills/deepone/SKILL.md"), + "# deepone\n\nDoes the deep thing.\n", + ); + + let found = detect(home_root.path(), workspace.path()); + let items = plan(&found, home.path(), Scope::Project); + assert_eq!(items.len(), 1, "{items:?}"); + assert_eq!(items[0].kind, Kind::Skill); + assert_eq!( + items[0].to, + Destination::File(home.path().join("skills/deepone/SKILL.md")), + "five levels down at the source, one level down at the destination", + ); + + let report = apply(&items, workspace.path()); + assert!(report.refused.is_empty(), "{report:?}"); + + let discovered = + io_harness::Skills::discover(home.path().join("skills")).expect("the directory discovers"); + assert!( + discovered.get("deepone").is_some(), + "the flattened skill is not in io-harness's own catalogue", + ); +} + +/// **A skill's declared name becomes a directory here, and it is somebody else's +/// text.** +/// +/// `skill_name` answers with the frontmatter `name:` because that is the name a +/// skill is addressed by — and `Path::join` treats `../..` as an escape and an +/// absolute path as a replacement for everything to its left. A third-party +/// marketplace plugin declaring either one could put a file anywhere the process +/// can reach, and the collision guard would not stop it: that asks whether the +/// TARGET exists, and creating a new file somewhere else answers happily. +/// +/// The destination was also the one fact the review surface never drew, so this +/// could not have been caught by reading the screen either. +#[test] +fn a_skill_whose_declared_name_is_not_a_path_component_is_refused() { + let home_root = tempfile::tempdir().expect("a fake home"); + let workspace = tempfile::tempdir().expect("a workspace"); + let home = tempfile::tempdir().expect("io's home"); + std::fs::create_dir_all(home.path().join("skills")).expect("a skills directory"); + + // The escape, and the absolute replacement, and a plain innocent one beside + // them so the refusal is shown to be about the name rather than about there + // being a skill at all. + put( + &home_root.path().join(".claude/skills/escape/SKILL.md"), + "---\nname: ../../../pwned\ndescription: climbs out\n---\n\nbody\n", + ); + // A name nothing else on the machine will have, so the assertion below is + // about this test and can never be satisfied or spoiled by another process. + let absolute = std::env::temp_dir().join("io-cli-0210-traversal-probe"); + let _ = std::fs::remove_dir_all(&absolute); + put( + &home_root.path().join(".claude/skills/absolute/SKILL.md"), + &format!( + "---\nname: {}\ndescription: starts again\n---\n\nbody\n", + absolute.display() + ), + ); + put( + &home_root.path().join(".claude/skills/ordinary/SKILL.md"), + "---\nname: ordinary\ndescription: behaves\n---\n\nbody\n", + ); + + let found = detect(home_root.path(), workspace.path()); + let items = plan(&found, home.path(), Scope::Project); + + let skills: Vec<_> = items.iter().filter(|i| i.kind == Kind::Skill).collect(); + assert_eq!(skills.len(), 3, "{skills:?}"); + + for item in &skills { + let ordinary = item.says.contains("`ordinary`"); + if ordinary { + assert_eq!( + item.to, + Destination::File(home.path().join("skills/ordinary/SKILL.md")), + "the well-behaved skill still imports", + ); + } else { + assert_eq!( + item.to, + Destination::Nowhere, + "a name that is not one path component has nowhere to go: {item:?}", + ); + assert!(item.form.is_none(), "and nothing to write: {item:?}"); + } + } + + let before = fingerprint(home.path()); + let report = apply(&items, workspace.path()); + + // The absolute name's target, which `Path::join` would have made the whole + // destination — asserted by the exact path this fixture named, so nothing + // else on the machine can satisfy or spoil it. + assert!( + !absolute.exists(), + "an absolute declared name replaced the join and wrote outside io's home", + ); + // And the escape was refused rather than quietly flattened into the skills + // directory, which would install a skill under a name the operator's other + // tool does not use. + assert!( + !home.path().join("skills/pwned").exists(), + "the escaping name was sanitised into the directory instead of refused", + ); + + // Exactly one skill landed, and the home changed only by that much. + let discovered = + io_harness::Skills::discover(home.path().join("skills")).expect("the directory discovers"); + assert_eq!(discovered.len(), 1, "only the ordinary skill is installed"); + assert!(discovered.get("ordinary").is_some()); + assert_ne!(before, fingerprint(home.path()), "the ordinary one wrote"); + assert_eq!( + report.written.len(), + 1, + "one write, and it is the ordinary skill: {report:?}", + ); +} + +#[cfg(unix)] +#[test] +fn a_symlinked_skill_arrives_as_a_real_file() { + let home_root = tempfile::tempdir().expect("a fake home"); + let workspace = tempfile::tempdir().expect("a workspace"); + let home = tempfile::tempdir().expect("io's home"); + let elsewhere = tempfile::tempdir().expect("the checkout the link points at"); + std::fs::create_dir_all(home.path().join("skills")).expect("a skills directory"); + + put( + &elsewhere.path().join("graphify/SKILL.md"), + "# graphify\n\nBuilds a graph.\n", + ); + std::fs::create_dir_all(home_root.path().join(".claude/skills")).expect("a skills directory"); + std::os::unix::fs::symlink( + elsewhere.path().join("graphify"), + home_root.path().join(".claude/skills/graphify"), + ) + .expect("a symlink"); + + let found = detect(home_root.path(), workspace.path()); + let items = plan(&found, home.path(), Scope::Project); + assert_eq!(items.len(), 1, "the walk followed the link: {items:?}"); + + let report = apply(&items, workspace.path()); + assert!(report.refused.is_empty(), "{report:?}"); + + let landed = home.path().join("skills/graphify/SKILL.md"); + let kind = std::fs::symlink_metadata(&landed).expect("it is there"); + assert!( + !kind.is_symlink(), + "the import made a link rather than materialising the file, so deleting the \ + operator's checkout would empty their skills directory", + ); + assert_eq!( + std::fs::read_to_string(&landed).expect("readable"), + "# graphify\n\nBuilds a graph.\n", + ); + assert!(io_harness::Skills::discover(home.path().join("skills")) + .expect("it discovers") + .get("graphify") + .is_some()); +} + +#[test] +fn a_skill_name_that_is_already_taken_is_refused_rather_than_overwritten() { + let home_root = tempfile::tempdir().expect("a fake home"); + let workspace = tempfile::tempdir().expect("a workspace"); + let home = tempfile::tempdir().expect("io's home"); + + let mine = home.path().join("skills/mine/SKILL.md"); + put(&mine, "# mine\n\nThe operator's own.\n"); + put( + &home_root.path().join(".claude/skills/mine/SKILL.md"), + "# mine\n\nSomebody else's.\n", + ); + + let found = detect(home_root.path(), workspace.path()); + let items = plan(&found, home.path(), Scope::Project); + assert_eq!(items.len(), 1, "{items:?}"); + assert_eq!(items[0].kind, Kind::Skill); + assert_eq!( + items[0].to, + Destination::Nowhere, + "a claimed name is refused in the plan, before anything can be written", + ); + assert!(items[0].form.is_none(), "and it carries nothing to write"); + assert!( + items[0].says.contains("already answers"), + "the operator is told why: {}", + items[0].says, + ); + + let report = apply(&items, workspace.path()); + assert!(report.written.is_empty(), "{report:?}"); + assert_eq!( + std::fs::read_to_string(&mine).expect("still there"), + "# mine\n\nThe operator's own.\n", + "the operator's skill was overwritten", + ); +} + +#[test] +fn sixty_three_present_and_three_incoming_is_reported_and_nothing_is_written() { + let home_root = tempfile::tempdir().expect("a fake home"); + let workspace = tempfile::tempdir().expect("a workspace"); + let home = tempfile::tempdir().expect("io's home"); + + for n in 0..63 { + put( + &home.path().join(format!("skills/held{n:02}.md")), + &format!("# held{n:02}\n"), + ); + } + for name in ["alpha", "beta", "gamma"] { + put( + &home_root + .path() + .join(format!(".claude/skills/{name}/SKILL.md")), + &format!("# {name}\n"), + ); + } + + let before = fingerprint(home.path()); + let found = detect(home_root.path(), workspace.path()); + let items = plan(&found, home.path(), Scope::Project); + assert_eq!(items.len(), 3, "{items:?}"); + for item in &items { + assert_eq!( + item.to, + Destination::Nowhere, + "63 + 3 is 66, and the ceiling is {}", + io_harness::skills::MAX_SKILLS, + ); + assert!( + item.says.contains("no skill is imported") && item.says.contains("63"), + "the numbers are in the sentence: {}", + item.says, + ); + } + + let report = apply(&items, workspace.path()); + assert!(report.written.is_empty(), "{report:?}"); + assert_eq!( + fingerprint(home.path()), + before, + "going over the ceiling rejects the WHOLE directory at run start, so an import \ + that would cross it writes nothing at all", + ); + assert_eq!( + io_harness::Skills::discover(home.path().join("skills")) + .expect("it still discovers") + .len(), + 63, + "and the operator's session still starts", + ); +} + +// --------------------------------------------------------------------------- +// The allowlist, which is described and never translated +// --------------------------------------------------------------------------- + +#[test] +fn an_allowlist_is_reported_and_no_policy_is_written() { + let home_root = tempfile::tempdir().expect("a fake home"); + let workspace = tempfile::tempdir().expect("a workspace"); + let home = tempfile::tempdir().expect("io's home"); + + put( + &home_root.path().join(".codex/rules/default.rules"), + "# the operator's own allowlist\n\ + prefix_rule(pattern=[\"bun\", \"install\"], decision=\"allow\")\n\ + prefix_rule(pattern=[\"cargo\", \"test\"], decision=\"allow\")\n", + ); + put( + &home_root.path().join(".claude/settings.json"), + r#"{"model":"opusplan","permissions":{"ask":["Bash(cargo yank *)"]}}"#, + ); + // One server, so there IS a configuration file to assert the absence in. + put( + &home_root.path().join(".claude.json"), + r#"{"mcpServers":{"semlith":{"command":"semlith","args":["mcp"]}}}"#, + ); + + let found = detect(home_root.path(), workspace.path()); + let items = plan(&found, home.path(), Scope::Project); + + let allow: Vec<_> = items + .iter() + .filter(|item| item.kind == Kind::Allowlist) + .collect(); + assert_eq!( + allow.len(), + 2, + "one per source that declares one: {items:?}" + ); + for item in &allow { + assert_eq!(item.to, Destination::Nowhere); + assert!(item.form.is_none(), "there is no translated form to accept"); + assert!( + item.says.contains("BINARY NAME"), + "the sentence states why it cannot be translated: {}", + item.says, + ); + assert!(!item.kind.writes(), "and a surface can draw it as such"); + } + + let report = apply(&items, workspace.path()); + assert!(report.refused.is_empty(), "{report:?}"); + assert_eq!( + report.carried.len(), + 3, + "two allowlists and a model: {report:?}" + ); + + // Asserted on the PARSED result, not on a substring: a `[policy]` written as + // a dotted key would be invisible to a `contains("[policy]")`. + let at = workspace.path().join("io.toml"); + let text = std::fs::read_to_string(&at).expect("the MCP entry made a file"); + let document: toml::Value = toml::from_str(&text).expect("it parses"); + assert!( + document.get("policy").is_none(), + "an allowlist was translated into a policy: {text}", + ); + assert_eq!( + written_servers(&at).len(), + 1, + "and the rest of the import still landed", + ); +} diff --git a/tests/narrow.rs b/tests/narrow.rs index a4ac052..594d461 100644 --- a/tests/narrow.rs +++ b/tests/narrow.rs @@ -956,10 +956,7 @@ fn f12_the_slash_palette_fits_eighty_columns_in_both_glyph_sets() { let set = theme.glyphs.name; let mark = theme.glyphs.ellipsis; let palette = |query: &str| { - let mut picker = Picker::new( - "Which command?", - commands::palette(&templates, &io_harness::Skills::none()), - ); + let mut picker = Picker::new("Which command?", commands::palette(&templates, &[])); type_at(&mut picker, query); picker }; @@ -1061,7 +1058,7 @@ fn f12_the_pickers_query_line_and_its_no_match_line_fit_eighty_columns() { let mark = theme.glyphs.ellipsis; let mut picker = Picker::new( "Which command?", - io_cli::commands::palette(&io_harness::Templates::none(), &io_harness::Skills::none()), + io_cli::commands::palette(&io_harness::Templates::none(), &[]), ); type_at(&mut picker, TYPED); assert_eq!( diff --git a/tests/palette.rs b/tests/palette.rs index 2da5f0c..a1d6fbe 100644 --- a/tests/palette.rs +++ b/tests/palette.rs @@ -47,10 +47,7 @@ fn palette() -> Picker { /// The palette as the driver opens it over a set of templates. fn palette_over(templates: &Templates) -> Picker { - Picker::new( - "Which command?", - commands::palette(templates, &io_harness::Skills::none()), - ) + Picker::new("Which command?", commands::palette(templates, &[])) } /// A templates directory with two templates in it: one that renders as it @@ -166,7 +163,7 @@ fn choosable(rows: &[io_cli::picker::Row]) -> usize { /// Where a command's row sits, by name, since the order is now grouped. fn row_of(name: &str) -> usize { let bare = name.strip_prefix('/').unwrap_or(name); - commands::palette(&Templates::none(), &io_harness::Skills::none()) + commands::palette(&Templates::none(), &[]) .iter() .position(|row| row.label == bare) .unwrap_or_else(|| panic!("{name} has no row")) @@ -174,7 +171,7 @@ fn row_of(name: &str) -> usize { #[test] fn f1_the_palette_opens_on_the_rows_the_viewport_has() { - let rows = commands::palette(&Templates::none(), &io_harness::Skills::none()); + let rows = commands::palette(&Templates::none(), &[]); assert_eq!(choosable(&rows), COMMANDS.len()); let picks: Vec<&io_cli::picker::Row> = rows.iter().filter(|row| !row.heading).collect(); // Zipped against the GROUPED order rather than `COMMANDS`' own, because the @@ -279,7 +276,7 @@ fn f1_fk_selects_fork_though_no_command_begins_with_it() { panic!("Enter on the one matched row must choose it"); }; assert_eq!( - commands::palette_pick(&Templates::none(), &io_harness::Skills::none(), index), + commands::palette_pick(&Templates::none(), &[], index), Some(Chosen::Command("/fork")), ); } @@ -377,8 +374,7 @@ fn f1_enter_puts_the_chosen_command_in_the_composer_rather_than_running_it() { let Outcome::Chosen(index) = picker.key(key(KeyCode::Enter)) else { panic!("Enter must choose"); }; - let Some(Chosen::Command(command)) = - commands::palette_pick(&Templates::none(), &io_harness::Skills::none(), index) + let Some(Chosen::Command(command)) = commands::palette_pick(&Templates::none(), &[], index) else { panic!("a chosen row in the command half is a command"); }; @@ -402,20 +398,16 @@ fn f1_a_palette_row_addresses_the_command_it_was_built_from() { for (name, _) in COMMANDS.iter() { let index = row_of(name); assert_eq!( - commands::palette_pick(&none, &io_harness::Skills::none(), index), + commands::palette_pick(&none, &[], index), Some(Chosen::Command(name)) ); assert_eq!( - commands::palette(&none, &io_harness::Skills::none())[index].label, + commands::palette(&none, &[])[index].label, name.strip_prefix('/').expect("a command"), ); } assert_eq!( - commands::palette_pick( - &none, - &io_harness::Skills::none(), - commands::palette(&none, &io_harness::Skills::none()).len(), - ), + commands::palette_pick(&none, &[], commands::palette(&none, &[]).len()), None ); } @@ -439,7 +431,7 @@ fn f2_no_templates_configured_is_an_empty_section_and_not_an_error() { "a configuration that never mentioned templates has nothing to complain about", ); assert_eq!( - choosable(&commands::palette(&found, &io_harness::Skills::none())), + choosable(&commands::palette(&found, &[])), COMMANDS.len(), "an empty section contributes no rows", ); @@ -452,7 +444,7 @@ fn f2_every_template_is_a_row_carrying_its_name_and_its_description() { assert_eq!(complaint, None, "this directory is exactly what it says"); assert_eq!(found.len(), 2); - let rows = commands::palette(&found, &io_harness::Skills::none()); + let rows = commands::palette(&found, &[]); assert_eq!(choosable(&rows), COMMANDS.len() + found.len()); for template in found.iter() { let row = rows @@ -518,7 +510,7 @@ fn f2_a_configured_directory_that_cannot_be_walked_is_disclosed_with_the_harness let (found, complaint) = commands::templates(&configured(&missing)); assert!(found.is_empty(), "nothing was discovered, which is true"); assert_eq!( - choosable(&commands::palette(&found, &io_harness::Skills::none())), + choosable(&commands::palette(&found, &[])), COMMANDS.len(), "and the palette therefore looks exactly like the unconfigured one", ); @@ -559,9 +551,7 @@ fn f2_choosing_a_template_puts_the_rendered_text_in_the_composer_rather_than_sen let Outcome::Chosen(index) = picker.key(key(KeyCode::Enter)) else { panic!("Enter must choose"); }; - let Some(Chosen::Template(name)) = - commands::palette_pick(&found, &io_harness::Skills::none(), index) - else { + let Some(Chosen::Template(name)) = commands::palette_pick(&found, &[], index) else { panic!("the row under the marker is a template"); }; assert_eq!(name, "review"); @@ -622,7 +612,7 @@ fn f2_a_row_addresses_the_command_or_the_template_it_was_built_from() { // one under the marker, and nothing on screen would say so. let dir = written(); let (found, _) = commands::templates(&configured(dir.path())); - let rows = commands::palette(&found, &io_harness::Skills::none()); + let rows = commands::palette(&found, &[]); for (name, _) in COMMANDS.iter() { let index = rows @@ -630,7 +620,7 @@ fn f2_a_row_addresses_the_command_or_the_template_it_was_built_from() { .position(|row| row.label == name.strip_prefix('/').expect("a command")) .expect("every command has a row"); assert_eq!( - commands::palette_pick(&found, &io_harness::Skills::none(), index), + commands::palette_pick(&found, &[], index), Some(Chosen::Command(name)), ); } @@ -642,14 +632,11 @@ fn f2_a_row_addresses_the_command_or_the_template_it_was_built_from() { let _ = offset; assert_eq!(rows[index].label, template.name); assert_eq!( - commands::palette_pick(&found, &io_harness::Skills::none(), index), + commands::palette_pick(&found, &[], index), Some(Chosen::Template(template.name.clone())), ); } - assert_eq!( - commands::palette_pick(&found, &io_harness::Skills::none(), rows.len()), - None - ); + assert_eq!(commands::palette_pick(&found, &[], rows.len()), None); } /// **F5 — the palette lists what the workspace actually taught the agent.** @@ -667,6 +654,11 @@ fn f2_a_row_addresses_the_command_or_the_template_it_was_built_from() { #[test] fn f5_every_discovered_skill_is_a_row_after_the_templates() { let dir = tempfile::tempdir().expect("a directory"); + // The home is where the manifest lives, and it is a different question from + // the directory the run reads — so it is a directory of its own here, empty, + // which makes every row the operator's own. What this test is about is the + // rows, not whose they are; `tests/skillview.rs` owns provenance. + let home = tempfile::tempdir().expect("a home"); std::fs::write( dir.path().join("migrations.md"), "---\nname: migrations\ndescription: how this repo changes a schema\n---\nbody\n", @@ -679,7 +671,7 @@ fn f5_every_discovered_skill_is_a_row_after_the_templates() { ) .expect("write"); - let (skills, complaint) = commands::skills(Some(dir.path())); + let (skills, complaint) = commands::skills(home.path(), Some(dir.path()), &[]); assert_eq!(complaint, None, "this directory is exactly what it says"); assert_eq!(skills.len(), 2, "both layouts are discovered"); @@ -730,15 +722,16 @@ fn f5_choosing_a_skill_puts_its_name_in_the_prompt_and_not_its_body() { /// product has already paid for twice. #[test] fn f5_a_skills_directory_that_will_not_walk_says_so() { + let home = tempfile::tempdir().expect("a home"); assert_eq!( - commands::skills(None).1, + commands::skills(home.path(), None, &[]).1, None, "a session that configured none has nothing to complain about", ); - assert!(commands::skills(None).0.is_empty()); + assert!(commands::skills(home.path(), None, &[]).0.is_empty()); let missing = std::path::Path::new("/tmp/io-cli-no-such-skills-dir"); - let (skills, complaint) = commands::skills(Some(missing)); + let (skills, complaint) = commands::skills(home.path(), Some(missing), &[]); assert!(skills.is_empty()); let complaint = complaint.expect("a path that is not there is a sentence, not silence"); assert!( @@ -747,6 +740,199 @@ fn f5_a_skills_directory_that_will_not_walk_says_so() { ); } +// --------------------------------------------------------------------------- +// 0.21.0 — a capability bundle's skills are in the palette, and a disabled one +// is in nothing. +// +// The gap 0.20.0 shipped with: a bundle's skills are folded into the turn's +// catalogue and namespaced, and no surface in io-cli offered one — so an +// operator scanning the palette was reading a list that disagreed with what the +// model had. `tests/skillview.rs` owns the walk; what is asserted here is the +// half that is the palette's, which is what an operator can choose and what +// choosing it stands for. +// --------------------------------------------------------------------------- + +/// A skill file, in the two keys io-harness reads. +fn skill_file(name: &str, description: &str) -> String { + format!("---\nname: {name}\ndescription: {description}\n---\n\nDo the thing.\n") +} + +/// The namespaced name io-harness itself builds, out of the harness's own +/// constant rather than a `"__"` literal. The separator is io-harness's to +/// change, and a literal here would go on passing while the palette offered a +/// name the model cannot address — which is the whole defect these tests are for. +fn namespaced(id: &str, name: &str) -> String { + format!("{id}{}{name}", io_harness::NAMESPACE) +} + +/// The heading a skill row sits under, by index. +fn skills_heading(rows: &[io_cli::picker::Row]) -> usize { + rows.iter() + .position(|row| row.heading && row.label == "skills") + .expect("the palette drew no skills heading") +} + +#[test] +fn f5_a_bundle_skill_is_offered_under_the_name_the_model_addresses() { + let root = tempfile::tempdir().expect("a directory"); + let home = root.path().join("home"); + let dir = io_cli::skills::dir(&home); + std::fs::create_dir_all(&dir).expect("the skills directory"); + std::fs::write( + dir.join("mine.md"), + skill_file("mine", "The operator's own."), + ) + .expect("write"); + + let contributed = root.path().join("acme").join("skills"); + std::fs::create_dir_all(&contributed).expect("the bundle's skills directory"); + std::fs::write( + contributed.join("helper.md"), + skill_file("helper", "Something contributed."), + ) + .expect("write"); + let bundles = [("acme".to_string(), contributed)]; + + let (skills, complaint) = commands::skills(&home, Some(dir.as_path()), &bundles); + assert_eq!( + complaint, None, + "both directories are what they say they are" + ); + + let none = Templates::none(); + let rows = commands::palette(&none, &skills); + assert_eq!( + choosable(&rows), + COMMANDS.len() + 2, + "the operator's own skill and the bundle's are both choosable", + ); + + let label = namespaced("acme", "helper"); + let index = rows + .iter() + .position(|row| row.label == label) + .unwrap_or_else(|| { + let offered: Vec<&str> = rows.iter().map(|row| row.label.as_str()).collect(); + panic!("`{label}` is not offered; the palette holds {offered:?}") + }); + assert!( + !rows[index].heading, + "a bundle's skill is a row an operator can land on, not furniture", + ); + assert!( + index > skills_heading(&rows), + "the row sits under the skills heading, not among the commands", + ); + assert_eq!( + rows[index].mark, + Some(commands::SKILL_MARK), + "it is marked a skill, in the column the narrow terminal keeps", + ); + + // **The absence that is the point.** The bare name is what `Skills::discover` + // resolved out of the bundle's directory and it is NOT what the model + // addresses — offering it would put a name in the prompt that resolves to + // nothing, on the surface that exists to say what the model is offered. + assert!( + !rows.iter().any(|row| row.label == "helper"), + "the bare name is offered, and the model cannot address it", + ); + + // The provenance is in the label AND in the detail, because the detail is the + // column a narrow terminal drops first. + let detail = rows[index] + .detail + .as_deref() + .expect("a skill row carries its description"); + assert!( + detail.starts_with(SKILL) + && detail.contains("Something contributed.") + && detail.contains("acme"), + "the detail says what it is and whose it is: {detail:?}", + ); + + assert_eq!( + commands::palette_pick(&none, &skills, index), + Some(Chosen::Skill(label.clone())), + "choosing it stands for the namespaced name and not the bare one", + ); + + // And through the picker, as the driver reaches it: typed for by the name the + // model uses, chosen with `Enter`, resolved back through `palette_pick`. + let mut picker = Picker::new("Which command?", rows.clone()); + type_at(&mut picker, &label); + assert_eq!( + marked(&picker), + label, + "typing the namespaced name finds it" + ); + let Outcome::Chosen(chosen) = picker.key(key(KeyCode::Enter)) else { + panic!("Enter on the marked row must choose it"); + }; + assert_eq!( + commands::palette_pick(&none, &skills, chosen), + Some(Chosen::Skill(label)), + ); +} + +/// **A disabled skill is offered nowhere.** `commands::skills` drops it, so the +/// palette never draws it and no index resolves to it — which is the property +/// that keeps `/skills` and the palette from disagreeing: a disabled skill is +/// precisely one the model is not offered. +/// +/// Sabotage: drop the `.filter(|row| row.enabled)` in `commands::skills`. Under +/// it only this fails, and it fails by putting a row in the palette whose choice +/// names a skill the turn was never handed. +#[test] +fn f5_a_disabled_skill_is_not_offered_in_the_palette_at_all() { + let root = tempfile::tempdir().expect("a directory"); + let home = root.path().join("home"); + let dir = io_cli::skills::dir(&home); + std::fs::create_dir_all(io_cli::skills::disabled_dir(&home)).expect("the disabled directory"); + std::fs::write(dir.join("on.md"), skill_file("on", "Offered.")).expect("write"); + std::fs::write( + io_cli::skills::disabled_dir(&home).join("off.md"), + skill_file("off", "Turned off."), + ) + .expect("write"); + + // The premise, so this cannot pass because the fixture was never read: the + // walk DOES see the disabled file and classifies it as disabled. + let view = io_cli::skillview::view(&home, &dir, &[]); + assert!( + view.skills + .iter() + .any(|row| row.name == "off" && !row.enabled), + "the fixture is not a disabled skill, so the absence below proves nothing", + ); + + let (skills, complaint) = commands::skills(&home, Some(dir.as_path()), &[]); + assert_eq!(complaint, None, "this directory is exactly what it says"); + assert_eq!( + skills.len(), + 1, + "the palette's inventory is what the model was offered", + ); + + let none = Templates::none(); + let rows = commands::palette(&none, &skills); + assert_eq!(choosable(&rows), COMMANDS.len() + 1); + assert!(rows.iter().any(|row| row.label == "on")); + assert!( + !rows.iter().any(|row| row.label == "off"), + "a disabled skill has a row, and choosing it would name a skill the turn \ + was never handed", + ); + + // And no index anywhere in the list resolves to it, including past the end. + for index in 0..=rows.len() { + assert_ne!( + commands::palette_pick(&none, &skills, index), + Some(Chosen::Skill("off".to_string())), + ); + } +} + /// **0.13.0 F6 — opening the palette re-places no viewport.** /// /// The decision is in `src/main.rs`, which nothing links, and `Screen::replace` @@ -809,7 +995,7 @@ fn f6_the_palette_path_in_the_driver_replaces_no_viewport() { fn f7_every_row_below_the_fold_is_still_reachable() { let dir = written(); let (templates, _) = commands::templates(&configured(dir.path())); - let rows = commands::palette(&templates, &io_harness::Skills::none()); + let rows = commands::palette(&templates, &[]); let height = io_cli::term::VIEWPORT_HEIGHT; assert!( rows.len() > usize::from(height), @@ -961,7 +1147,7 @@ fn f14_a_heading_can_never_be_chosen() { fn f16_every_kind_of_row_carries_a_mark_and_it_is_not_in_the_detail() { let dir = written(); let (templates, _) = commands::templates(&configured(dir.path())); - let rows = commands::palette(&templates, &io_harness::Skills::none()); + let rows = commands::palette(&templates, &[]); let marks: std::collections::BTreeSet<&str> = rows .iter() @@ -994,10 +1180,8 @@ fn f16_the_mark_survives_the_width_that_drops_the_detail() { // width where a row is hardest to read, and a command carried no mark at all. let dir = written(); let (templates, _) = commands::templates(&configured(dir.path())); - let mut picker = io_cli::picker::Picker::new( - "Which command?", - commands::palette(&templates, &io_harness::Skills::none()), - ); + let mut picker = + io_cli::picker::Picker::new("Which command?", commands::palette(&templates, &[])); let (mut screen, _recorder) = support::screen(28, 24); screen diff --git a/tests/providers.rs b/tests/providers.rs index d45ca35..c479d5d 100644 --- a/tests/providers.rs +++ b/tests/providers.rs @@ -2,8 +2,8 @@ use std::sync::{Mutex, MutexGuard}; -use io_cli::providers::{self, Credential}; -use io_harness::config::Config; +use io_cli::providers::{self, At, Credential, Endpoint}; +use io_harness::config::{Config, Scope}; const THREE: &str = "\ [[provider]] @@ -80,7 +80,8 @@ fn f7_the_chain_is_the_file_s_order_head_and_tail_together() { #[test] fn f7_reordering_the_panel_reorders_the_file_and_therefore_the_chain() { - let edit = providers::promote(1).expect("an entry that is not already first can be promoted"); + let at = At::of(Scope::User, THREE, 1).expect("the file declares three entries"); + let edit = providers::promote(&at).expect("an entry that is not already first can be promoted"); let after = io_cli::edit::apply(THREE, &[edit]).unwrap(); let moved = config(&after); @@ -115,17 +116,30 @@ fn f7_reordering_the_panel_reorders_the_file_and_therefore_the_chain() { fn f7_the_first_entry_cannot_be_promoted_and_the_last_cannot_be_demoted() { // Expressed as `None` rather than as a no-op edit, so a caller cannot draw a // control that does nothing. - assert!(providers::promote(0).is_none()); - assert!(providers::demote(2, 3).is_none()); - assert!(providers::promote(2).is_some()); - assert!(providers::demote(0, 3).is_some()); + let at = |index| At::of(Scope::User, THREE, index).expect("a declared position"); + assert!(providers::promote(&at(0)).is_none()); + assert!(providers::demote(&at(2)).is_none()); + assert!(providers::promote(&at(2)).is_some()); + assert!(providers::demote(&at(0)).is_some()); + + // **The bound is read from the file, not passed in.** `demote` used to take + // the length as a second argument, so the answer to "is there anywhere to + // move to" came from whatever the caller happened to be counting — a + // filtered view, or a chain rendered from a different `Config`. A position + // past the end is now refused where it is built, once, rather than becoming + // an `Edit` that fails somewhere further down. + assert!( + At::of(Scope::User, THREE, 3).is_none(), + "a position past the end of the array was accepted", + ); + assert!(At::of(Scope::User, "", 0).is_none()); } #[test] fn f7_an_added_provider_goes_last_because_that_is_the_end_of_the_chain() { let after = io_cli::edit::apply( THREE, - &[providers::add("compatible", "qwen-max", Some("qwen"))], + &[providers::add(Endpoint::Preset("qwen"), "qwen-max", None)], ) .unwrap(); let chain = providers::chain(&config(&after).config); @@ -141,13 +155,191 @@ fn f7_an_added_provider_goes_last_because_that_is_the_end_of_the_chain() { #[test] fn f7_removing_an_entry_leaves_the_rest_in_order() { - let after = io_cli::edit::apply(THREE, &[providers::remove(1)]).unwrap(); + let at = At::of(Scope::User, THREE, 1).expect("the middle link"); + let after = io_cli::edit::apply(THREE, &[providers::remove(&at)]).unwrap(); let chain = providers::chain(&config(&after).config); let kinds: Vec<&str> = chain.iter().map(|e| e.kind.as_str()).collect(); assert_eq!(kinds, vec!["openrouter", "ollama"]); } +#[test] +fn f7_demoting_the_head_hands_the_run_to_the_next_link() { + // The other direction of the same claim, and it is the one with something at + // stake: the FIRST entry is the provider in force, so demoting it changes + // which vendor the next turn bills to. Asserted by reading back which entry + // is first — through io-harness's own `provider_spec`, not by looking at + // where the text moved to. + let at = At::of(Scope::User, THREE, 0).expect("the head"); + let edit = providers::demote(&at).expect("a head with a chain behind it can be demoted"); + let after = io_cli::edit::apply(THREE, &[edit]).unwrap(); + + let moved = config(&after); + let chain = providers::chain(&moved.config); + assert_eq!(chain.len(), 3, "a link was lost in the move"); + assert_eq!( + chain[0].kind, "groq", + "the provider in force did not change" + ); + assert_eq!(chain[1].kind, "openrouter"); + assert_eq!(chain[2].kind, "ollama"); + + let head = moved.config.provider_spec().expect("a head"); + assert!( + matches!(head, io_harness::ProviderSpec::Compatible { preset, .. } + if preset.as_deref() == Some("groq")), + "io-harness does not agree about which provider a run would use", + ); +} + +#[test] +fn f7_removing_the_head_promotes_the_next_link_and_removing_the_only_one_leaves_none() { + // Removing the first entry is the removal that changes what a run uses, and + // it is not spelled differently from any other — the chain IS the array's + // order, so the second link becomes the provider by arithmetic. + let at = At::of(Scope::User, THREE, 0).expect("the head"); + let after = io_cli::edit::apply(THREE, &[providers::remove(&at)]).unwrap(); + let chain = providers::chain(&config(&after).config); + assert_eq!(chain[0].kind, "groq"); + assert_eq!(chain.len(), 2); + assert!( + !after.contains("openrouter"), + "the removed entry left bytes behind", + ); + + // The last of several: the tail comes out and the head is untouched. + let at = At::of(Scope::User, THREE, 2).expect("the tail"); + let after = io_cli::edit::apply(THREE, &[providers::remove(&at)]).unwrap(); + let chain = providers::chain(&config(&after).config); + let kinds: Vec<&str> = chain.iter().map(|e| e.kind.as_str()).collect(); + assert_eq!(kinds, vec!["openrouter", "groq"]); + + // The only one: a configuration with no `[[provider]]` has NO head, rather + // than a defaulted vendor the operator never named — io-harness says so and + // this surface must not paper over it. + const ONLY: &str = "[[provider]]\nkind = \"openai\"\nmodel = \"gpt-4o\"\n"; + let at = At::of(Scope::User, ONLY, 0).expect("the only entry"); + assert!( + At::of(Scope::User, ONLY, 1).is_none(), + "a second position was invented for a one-entry chain", + ); + let after = io_cli::edit::apply(ONLY, &[providers::remove(&at)]).unwrap(); + assert_eq!(after, "", "the last entry left bytes behind"); + assert!(Config::from_toml(&after) + .expect("a file with nothing in it is a configuration") + .provider_spec() + .is_none()); +} + +#[test] +fn f7_a_compatible_entry_naming_both_bases_or_neither_cannot_be_written() { + // io-harness takes EXACTLY ONE of `preset` and `base_url` and refuses both + // and neither, by index, at load (`config.rs:456`). `configure::write`'s + // round trip would catch it and roll back, which is a good failure — but the + // pair of `Option`s that made it expressible is gone: `Endpoint` has one + // variant per shape, so three of the four combinations cannot be spelled. + // + // The refusals are written by hand here, because that is the only way left + // to produce them — and without them this test would be asserting that a + // constraint io-harness might have dropped is still being satisfied. + let neither = "[[provider]]\nkind = \"compatible\"\nmodel = \"m\"\n"; + let both = "[[provider]]\nkind = \"compatible\"\nmodel = \"m\"\n\ + preset = \"groq\"\nbase_url = \"https://example.com/v1\"\n"; + for bad in [neither, both] { + assert!( + Config::from_toml(bad).is_err(), + "io-harness accepted a `compatible` entry that names both bases or \ + neither, so `Endpoint`'s split is no longer load-bearing: {bad}", + ); + } + + // And every shape `Endpoint` CAN build is one io-harness loads. + for endpoint in [ + Endpoint::OpenRouter, + Endpoint::Anthropic, + Endpoint::OpenAi, + Endpoint::Preset("groq"), + Endpoint::BaseUrl("https://example.com/v1"), + ] { + let written = + io_cli::edit::apply("", &[providers::add(endpoint, "a-model", None)]).unwrap(); + assert!( + Config::from_toml(&written).is_ok(), + "{endpoint:?} produced an entry io-harness refuses:\n{written}", + ); + } +} + +#[test] +fn f7_an_added_provider_can_name_its_credential_without_carrying_it() { + // The gap that made `add` unusable for every hosted vendor: there was no way + // to write `api_key` at all, so an operator adding one had to open the file + // by hand — which is the thing this surface exists not to make them do. + // + // Written as the indirection rather than the key, which is the form the + // panel then shows back: the variable's NAME is the information. + let after = io_cli::edit::apply( + "", + &[providers::add( + Endpoint::Preset("groq"), + "llama-3.3-70b-versatile", + Some("${env:IO_CLI_TEST_GROQ_KEY}"), + )], + ) + .unwrap(); + + let chain = providers::chain(&config(&after).config); + assert_eq!( + chain[0].credential, + Credential::Indirect("${env:IO_CLI_TEST_GROQ_KEY}".into()), + ); + assert_eq!(chain[0].kind, "groq"); + assert_eq!(chain[0].model, "llama-3.3-70b-versatile"); + + // And an entry with no key states none, rather than an empty one — which is + // a different configuration: for `compatible` there is no environment + // variable to fall back to, and `api_key = ""` is a key that is set. + let bare = io_cli::edit::apply( + "", + &[providers::add(Endpoint::Preset("ollama"), "llama3.2", None)], + ) + .unwrap(); + assert!( + !bare.contains("api_key"), + "an empty credential was written: {bare}" + ); + assert_eq!( + providers::chain(&config(&bare).config)[0].credential, + Credential::NotNeeded, + ); +} + +#[test] +fn f7_a_pasted_credential_is_escaped_rather_than_breaking_the_file() { + // `api_key` and `base_url` are pasted text. A `format!("\"{}\"")` would turn + // a backslash into an escape and a quote into the end of the value — the + // second is a way to write keys the operator did not ask for. + let after = io_cli::edit::apply( + "", + &[providers::add( + Endpoint::OpenAi, + "gpt-4o", + Some("sk-a\\b\"c\nmodel = \"smuggled\""), + )], + ) + .unwrap(); + + let config = Config::from_toml(&after).expect("the written file still parses"); + let Some(io_harness::ProviderSpec::OpenAi { model, api_key }) = config.provider_spec() else { + panic!("the file named an openai provider"); + }; + assert_eq!( + model, "gpt-4o", + "a second `model` was smuggled into the entry" + ); + assert_eq!(api_key.as_deref(), Some("sk-a\\b\"c\nmodel = \"smuggled\"")); +} + #[test] fn f8_the_preset_list_is_the_harness_s_own() { // io-cli carries its own list because `preset_names()` is `pub(crate)`. This @@ -279,3 +471,70 @@ fn f9_the_verifier_is_the_wizard_s_and_there_is_only_one() { ); } } + +/// A file whose top-level chain and whose profile's chain are DIFFERENT lists. +/// +/// The profile declares one entry; the top level declares three. io-harness does +/// not append a profile's `provider` onto the top-level array, it replaces it — +/// and then rewrites the origins so the overlaid config still names this file. +const PROFILE_REPLACES_THE_CHAIN: &str = r#" +[[provider]] +kind = "openrouter" +model = "the-operators-real-primary" + +[[provider]] +kind = "anthropic" +model = "the-operators-first-fallback" + +[[provider]] +kind = "openai" +model = "the-operators-second-fallback" + +[[profile.fast.provider]] +kind = "openrouter" +model = "something-small-and-quick" +"#; + +/// **A position is confirmed against the entry it is supposed to name.** +/// +/// Under a profile the rows on screen come from the resolved chain while the +/// array a write would splice is the file's top-level one, and those are not the +/// same list. Every positional check still passes — `decided` names this file, +/// the index is in bounds — so nothing but the content can tell the two apart. +/// +/// Without the check this is a silent wrong delete: the profile shows one link, +/// `remove` is aimed at `provider[0]`, and `provider[0]` is the operator's real +/// primary provider, which was never on screen. +#[test] +fn f7_a_profiles_link_is_never_addressed_by_a_position_in_the_top_level_array() { + let fixture = config(PROFILE_REPLACES_THE_CHAIN); + let overlaid = io_cli::configure::with_profile(&fixture.config, "fast") + .expect("the fixture declares the profile"); + + let chain = providers::chain(&overlaid); + assert_eq!( + chain.len(), + 1, + "the profile replaces the chain rather than extending it; if this is 4 the \ + premise of this test is gone and the guard below is testing nothing", + ); + assert_eq!(chain[0].model, "something-small-and-quick"); + + assert!( + providers::declared_at(&overlaid, &chain[0]).is_none(), + "the profile's link is not the entry at that position in the top-level array, \ + so there is no position to hand a writer — answering with one would aim a \ + removal at `the-operators-real-primary`, which is not on screen", + ); + + // The control: with no profile in force the two lists ARE the same list, and + // every link still answers with its own position. A guard that refused + // everything would pass the assertion above and break the surface. + let plain = providers::chain(&fixture.config); + assert_eq!(plain.len(), 3); + for entry in &plain { + let at = providers::declared_at(&fixture.config, entry) + .expect("a top-level link is addressable"); + assert_eq!(at.index(), entry.index); + } +} diff --git a/tests/servers.rs b/tests/servers.rs index e9b1573..63ae346 100644 --- a/tests/servers.rs +++ b/tests/servers.rs @@ -1,9 +1,12 @@ //! F5, F6 and F9 — the `/mcp` panel's three states, the writes it makes, and the //! two counts it keeps apart. -use io_cli::servers::{self, Observed, Reached}; -use io_harness::config::Config; -use io_harness::{EventKind, McpTransport}; +use std::collections::BTreeMap; + +use io_cli::configure::Decided; +use io_cli::servers::{self, At, Observed, Reached, Server}; +use io_harness::config::{Config, Scope}; +use io_harness::{EventKind, McpServer, McpTransport}; /// A configuration naming two servers, neither of them reached yet. fn configured() -> Config { @@ -343,7 +346,11 @@ command = \"mcp-docs\" [instructions] files = [\"AGENTS.md\"] "; - let after = io_cli::edit::apply(OPERATORS, &[servers::add("search", "mcp-search")]).unwrap(); + let after = io_cli::edit::apply( + OPERATORS, + &[servers::add(&McpServer::stdio("search", "mcp-search"))], + ) + .unwrap(); assert!(after.contains("id = \"search\"")); assert!(after.contains("command = \"mcp-search\"")); @@ -370,7 +377,14 @@ id = \"search\" transport = \"stdio\" command = \"mcp-search\" "; - let after = io_cli::edit::apply(TWO, &[servers::edit(1, "command", "\"mcp-find\"")]).unwrap(); + // The position is looked up by id rather than typed, which is the whole of + // the `At` contract: the second entry is `search`, and nothing here spells a + // `1` that could have been a row number instead. + let at = At::of(Scope::User, TWO, "search").expect("the fixture names `search`"); + assert_eq!(at.index(), 1); + let edit = servers::edit(&at, "command", &servers::quoted("mcp-find")) + .expect("`command` is a key an `[[mcp]]` entry carries"); + let after = io_cli::edit::apply(TWO, &[edit]).unwrap(); assert!(after.contains("command = \"mcp-find\"")); assert!( @@ -397,7 +411,8 @@ command = \"mcp-search\" [run] max_steps = 30 "; - let after = io_cli::edit::apply(TWO, &[servers::remove(0)]).unwrap(); + let at = At::of(Scope::User, TWO, "docs").expect("the fixture names `docs`"); + let after = io_cli::edit::apply(TWO, &[servers::remove(&at)]).unwrap(); let config = Config::from_toml(&after).expect("the written file loads"); let left: Vec<&str> = config.mcp_servers().iter().map(|s| s.id.as_str()).collect(); @@ -422,8 +437,9 @@ command = \"mcp-docs\" env = { TOKEN = \"abc\" } args = [\"--verbose\"] "; - let after = - io_cli::edit::apply(EXTRA, &[servers::edit(0, "command", "\"mcp-docs-2\"")]).unwrap(); + let at = At::of(Scope::User, EXTRA, "docs").expect("the fixture names `docs`"); + let edit = servers::edit(&at, "command", &servers::quoted("mcp-docs-2")).expect("a real key"); + let after = io_cli::edit::apply(EXTRA, &[edit]).unwrap(); assert!( after.contains("env = { TOKEN = \"abc\" }"), @@ -438,7 +454,8 @@ args = [\"--verbose\"] #[test] fn f6_a_server_id_with_a_quote_in_it_is_escaped_rather_than_breaking_the_file() { - let after = io_cli::edit::apply("", &[servers::add("we\"ird", "cmd")]).unwrap(); + let after = + io_cli::edit::apply("", &[servers::add(&McpServer::stdio("we\"ird", "cmd"))]).unwrap(); let config = Config::from_toml(&after).expect("the written file still parses"); assert_eq!(config.mcp_servers()[0].id, "we\"ird"); } @@ -472,3 +489,398 @@ url = \"https://example.com/mcp\" McpTransport::Stdio { .. } )); } + +// --- F6: the write half, complete and addressable ------------------------------ + +/// Two servers and nothing else, for the position tests. +const TWO_SERVERS: &str = r#"[[mcp]] +id = "docs" +transport = "stdio" +command = "mcp-docs" + +[[mcp]] +id = "search" +transport = "stdio" +command = "mcp-search" +"#; + +/// A file an operator wrote: a comment above the array, an inline comment on a +/// value, a blank-line rhythm, an array value, a section io-cli has no type for +/// (`[[agent]]`) and another (`[instructions]`). +const HAND_WRITTEN: &str = r#"# The servers this workspace talks to. + +[[mcp]] +id = "docs" +transport = "stdio" +command = "mcp-docs" + +[[mcp]] +id = "search" +transport = "stdio" +command = "mcp-search" # the fast one +args = ["--index", "./idx"] + +# The roster, which io-cli has no type for anywhere. +[[agent]] +name = "scout" +role = "find the file and say where it is" +model = "anthropic/claude-haiku-4" + +[instructions] +files = ["AGENTS.md"] +"#; + +/// Exactly the bytes the first `[[mcp]]` entry occupies, its trailing blank line +/// included — an entry owns the whitespace down to the next header. +const DOCS_ENTRY: &str = r#"[[mcp]] +id = "docs" +transport = "stdio" +command = "mcp-docs" + +"#; + +#[test] +fn f6_add_edit_and_remove_change_one_entry_and_no_other_byte() { + // F1 as a BYTE property, on a file an operator wrote. A writer that + // re-serialised the array from io-cli's model would still produce a file + // that parses and still carry the value it was asked for, which is what + // makes that mistake invisible to any test that only re-parses. So this + // compares bytes, three times. + + // Adding appends, so the whole original is still the prefix, untouched. + let added = io_cli::edit::apply( + HAND_WRITTEN, + &[servers::add(&McpServer::stdio("notes", "mcp-notes"))], + ) + .unwrap(); + assert_eq!( + &added[..HAND_WRITTEN.len()], + HAND_WRITTEN, + "adding a server rewrote bytes above it", + ); + assert_eq!( + &added[HAND_WRITTEN.len()..], + "\n[[mcp]]\nid = \"notes\"\ntransport = \"stdio\"\ncommand = \"mcp-notes\"\n", + ); + + // Editing replaces the value's own bytes — the inline comment sharing that + // line included, which is the span property `edit.rs` measurement 1 states. + let at = At::of(Scope::User, HAND_WRITTEN, "search").expect("the fixture names `search`"); + let edited = io_cli::edit::apply( + HAND_WRITTEN, + &[servers::edit(&at, "command", &servers::quoted("mcp-find")).expect("a real key")], + ) + .unwrap(); + assert_eq!( + edited, + HAND_WRITTEN.replace("\"mcp-search\"", "\"mcp-find\""), + "an edit reached past the value it named", + ); + assert!( + edited.contains("command = \"mcp-find\" # the fast one"), + "the inline comment on the edited line was eaten", + ); + + // Removing takes that entry's block and leaves every other byte in place. + let at = At::of(Scope::User, HAND_WRITTEN, "docs").expect("the fixture names `docs`"); + let removed = io_cli::edit::apply(HAND_WRITTEN, &[servers::remove(&at)]).unwrap(); + assert_eq!(removed, HAND_WRITTEN.replace(DOCS_ENTRY, "")); + assert!(!removed.contains("mcp-docs"), "the entry left bytes behind"); + assert!( + removed.contains("# The servers this workspace talks to."), + "the comment above the array went with the entry below it", + ); + + // And all three results are still the harness's schema, roster and + // instructions intact. + for text in [&added, &edited, &removed] { + Config::from_toml(text).expect("the written file loads"); + assert!(text.contains("name = \"scout\""), "the roster was dropped"); + assert!( + text.contains("files = [\"AGENTS.md\"]"), + "a section io-cli has no type for was dropped", + ); + } +} + +#[test] +fn f6_a_server_with_args_and_env_round_trips_as_the_harness_s_own_type() { + // **Asserted by deserialising, never by looking for a string.** `[[mcp]]` is + // one of only two sections io-harness exempts from `deny_unknown_fields` + // (`config.rs:86`), so a key this writer got wrong is accepted by the parser + // and ignored by the harness: a test that found `args = [...]` somewhere in + // the file would pass over a write the server never sees. Reading it back as + // an `McpServer` and comparing values is the only assertion that cannot. + let mut env = BTreeMap::new(); + env.insert( + "GITHUB_TOKEN".to_string(), + "ghp-not-a-real-token".to_string(), + ); + // A key and a value a `format!("\"{}\"")` would spell wrong: the backslashes + // in a Windows path open escapes, and `\U` is a parse error rather than a + // different value. + env.insert("WIN\"PATH".to_string(), "C:\\Users\\me".to_string()); + let server = McpServer { + id: "github".into(), + transport: McpTransport::Stdio { + command: "github-mcp-server".into(), + args: vec!["stdio".into(), "--read-only".into()], + env, + }, + timeout_secs: 30, + }; + + // ONE edit, therefore one `configure::write`: a server whose `args` arrived + // in a second call would be a second discover-and-roll-back round trip over + // a file that, in between, declared a server without the arguments that make + // it work. + let edits = [servers::add(&server)]; + assert_eq!(edits.len(), 1, "a whole server is one write"); + let after = io_cli::edit::apply("", &edits).unwrap(); + + let config = Config::from_toml(&after).expect("the written file loads"); + assert_eq!( + config.mcp_servers(), + [server].as_slice(), + "the server read back is not the server written", + ); +} + +#[test] +fn f6_an_http_server_round_trips_with_its_headers() { + let mut headers = BTreeMap::new(); + headers.insert( + "Authorization".to_string(), + "Bearer not-a-token".to_string(), + ); + let server = McpServer { + id: "remote".into(), + transport: McpTransport::Http { + url: "https://example.com/mcp".into(), + headers, + }, + timeout_secs: 90, + }; + + let after = io_cli::edit::apply("", &[servers::add(&server)]).unwrap(); + let config = Config::from_toml(&after).expect("the written file loads"); + assert_eq!(config.mcp_servers(), [server].as_slice()); + // The transport tag, not a `command` borrowed from the other arm. + assert!( + !after.contains("command"), + "an http entry named a command: {after}" + ); +} + +#[test] +fn f6_an_entry_states_nothing_that_is_already_the_default() { + // Every one of these is `#[serde(default)]`, so writing them changes nothing + // except how much of an operator's file is io-cli's opinion. `timeout_secs` + // is the one worth naming: the default is io-harness's, and a `60` written + // here is a number that stops tracking it the day it changes. + let plain = McpServer::stdio("plain", "cmd"); + let after = io_cli::edit::apply("", &[servers::add(&plain)]).unwrap(); + + for absent in ["args", "env", "timeout_secs", "url", "headers"] { + assert!( + !after.contains(absent), + "`{absent}` was written for a server that has none: {after}", + ); + } + // And the round trip is still exact, which is what makes the absence safe. + let config = Config::from_toml(&after).expect("the written file loads"); + assert_eq!(config.mcp_servers(), [plain].as_slice()); +} + +#[test] +fn f6_a_key_no_mcp_entry_carries_is_refused_rather_than_written() { + // THE SABOTAGE, and it is the quietest write in this crate. `comand = "…"` + // would be spliced in, accepted by io-harness on `configure::write`'s round + // trip, reported to the operator as written — and ignored by every turn + // after it. There is no later failure to catch it, so the refusal has to + // happen here. + let at = At::of(Scope::User, TWO_SERVERS, "docs").expect("the fixture names `docs`"); + assert!(servers::edit(&at, "comand", "\"mcp-find\"").is_none()); + assert!( + servers::edit(&at, "enabled", "false").is_none(), + "there is no `enabled` key; the module docs say why the verb is not offered", + ); + assert!(servers::edit(&at, "command", "\"mcp-find\"").is_some()); + for key in servers::KEYS { + assert!( + servers::edit(&at, key, "\"x\"").is_some(), + "`{key}` is listed and refused", + ); + } + + // The control: the file really would have taken the misspelling, so the + // refusal above is load-bearing rather than belt-and-braces. + let smuggled = io_cli::edit::apply( + TWO_SERVERS, + &[io_cli::edit::Edit::set("mcp[0].comand", "\"mcp-find\"")], + ) + .unwrap(); + assert!( + Config::from_toml(&smuggled).is_ok(), + "io-harness rejected a misspelled `[[mcp]]` key, so the exemption this \ + refusal exists for is gone and the const can go with it", + ); +} + +#[test] +fn f6_a_position_is_found_by_id_and_can_never_be_a_row_number() { + // The 0.20.0 defect, closed by construction: `pluginview::rows` drew two + // lists in an order no file shares, and a row number handed to a remover + // deletes whichever entry happens to sit there. Nothing outside the module + // can build an `At`, so there is no way left to spell that call. + assert_eq!( + At::of(Scope::User, TWO_SERVERS, "docs").map(|a| a.index()), + Some(0), + ); + assert_eq!( + At::of(Scope::User, TWO_SERVERS, "search").map(|a| a.index()), + Some(1), + ); + assert!( + At::of(Scope::User, TWO_SERVERS, "never-configured").is_none(), + "a position was invented for a server no file names", + ); + assert!(At::of(Scope::User, "", "docs").is_none()); + + // An id spelled with an escape is the same id, and the comparison is made + // against both spellings because this module may not parse TOML to settle it. + let escaped = io_cli::edit::apply("", &[servers::add(&McpServer::stdio("we\"ird", "cmd"))]) + .expect("the entry is written"); + assert_eq!( + At::of(Scope::User, &escaped, "we\"ird").map(|a| a.index()), + Some(0), + ); + + // The scope rides along, because an index means nothing without the file it + // counts in — and it is the scope `configure::write` has to be handed. + assert_eq!( + At::of(Scope::Local, TWO_SERVERS, "docs") + .expect("`docs`") + .scope, + Scope::Local, + ); +} + +#[test] +fn f6_the_only_entry_and_the_last_of_several_both_come_out_clean() { + const ONLY: &str = r#"[[mcp]] +id = "docs" +transport = "stdio" +command = "mcp-docs" +"#; + let at = At::of(Scope::User, ONLY, "docs").expect("the fixture names `docs`"); + let after = io_cli::edit::apply(ONLY, &[servers::remove(&at)]).unwrap(); + assert_eq!(after, "", "the last server left bytes behind"); + assert!(Config::from_toml(&after) + .expect("a file with nothing in it is a configuration") + .mcp_servers() + .is_empty(),); + + const THREE_AND_A_SECTION: &str = r#"[[mcp]] +id = "a" +transport = "stdio" +command = "mcp-a" + +[[mcp]] +id = "b" +transport = "stdio" +command = "mcp-b" + +[[mcp]] +id = "c" +transport = "stdio" +command = "mcp-c" + +[run] +max_steps = 30 +"#; + let at = At::of(Scope::User, THREE_AND_A_SECTION, "c").expect("the fixture names `c`"); + assert_eq!(at.index(), 2); + let after = io_cli::edit::apply(THREE_AND_A_SECTION, &[servers::remove(&at)]).unwrap(); + + let config = Config::from_toml(&after).expect("the written file loads"); + let left: Vec<&str> = config.mcp_servers().iter().map(|s| s.id.as_str()).collect(); + assert_eq!(left, vec!["a", "b"]); + assert!( + !after.contains("mcp-c"), + "the removed entry left bytes behind" + ); + assert!( + after.contains("max_steps = 30"), + "the section after the last entry was taken with it", + ); +} + +#[test] +fn f6_the_file_a_row_names_is_the_file_its_position_is_read_from() { + let dir = tempfile::tempdir().expect("a temporary directory"); + let path = dir.path().join(io_harness::config::PROJECT_FILE); + std::fs::write(&path, TWO_SERVERS).expect("the fixture is written"); + + // A row as the panel builds it, pointing at the file its origin column drew. + let row = Server { + id: "search".into(), + transport: "mcp-search".into(), + decided: Decided::File { + scope: Scope::Project, + path: path.clone(), + }, + state: Reached::NotYet, + }; + let at = servers::declared_at(&row).expect("the file names `search`"); + assert_eq!(at.index(), 1); + assert_eq!(at.scope, Scope::Project); + + // A server io-harness's own default supplied has no file to edit, and this + // says so rather than handing back position 0 of something. + let defaulted = Server { + decided: Decided::Default, + ..row.clone() + }; + assert!(servers::declared_at(&defaulted).is_none()); + + // A row whose file no longer names it — an operator editing `io.toml` under + // the session — is the same answer, and not a write to whatever is at 1 now. + let stale = Server { + id: "renamed-since".into(), + ..row + }; + assert!(servers::declared_at(&stale).is_none()); +} + +#[test] +fn f6_a_shadowed_server_is_not_offered_for_editing_because_it_is_not_running() { + // `[[mcp]]` is NOT one of io-harness's appending keys (`config.rs:2052`): + // the winning scope replaces the array WHOLE, "because a half-merged MCP + // server definition is not a server". So an entry named only in the project + // file, while a local file declares any `[[mcp]]` at all, is not + // lower-priority — it is not in force. Editing it would change a file and + // nothing an operator can observe, which is the worst kind of successful + // write. + let dir = tempfile::tempdir().expect("a temporary directory"); + std::fs::write( + dir.path().join(io_harness::config::PROJECT_FILE), + TWO_SERVERS, + ) + .expect("the project file"); + std::fs::write( + dir.path().join(io_harness::config::LOCAL_FILE), + "[[mcp]]\nid = \"local-only\"\ntransport = \"stdio\"\ncommand = \"mcp-local\"\n", + ) + .expect("the local file"); + + let at = servers::declared_in(dir.path(), "local-only").expect("the local file names it"); + assert_eq!(at.scope, Scope::Local); + assert_eq!(at.index(), 0); + + assert!( + servers::declared_in(dir.path(), "docs").is_none(), + "a shadowed entry was offered as editable: the local scope replaces the \ + array whole, so `docs` is not a server this workspace runs", + ); +} diff --git a/tests/skillview.rs b/tests/skillview.rs index adeda01..93cdfb4 100644 --- a/tests/skillview.rs +++ b/tests/skillview.rs @@ -9,6 +9,9 @@ //! //! 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. +//! The bundles are the same shape: plain `(id, directory)` pairs, so a question +//! about a `[[plugin]]` bundle's skills is still asked of a fixture directory and +//! never of a loaded configuration. mod support; @@ -111,7 +114,7 @@ fn f6_origin_is_decided_by_the_manifest_and_never_by_the_io_prefix() { // Only the un-prefixed one is recorded, so the prefix and the truth disagree. recorded(&home, &[("mine", ours.as_path())]); - let view = skillview::view(&home, &skills::dir(&home)); + let view = skillview::view(&home, &skills::dir(&home), &[]); assert_eq!(view.failed, None, "the fixture discovers"); assert_eq!(listed(&view, "mine").origin, Origin::IoCli); assert_eq!(listed(&view, "io-thing").origin, Origin::Yours); @@ -121,7 +124,7 @@ fn f6_origin_is_decided_by_the_manifest_and_never_by_the_io_prefix() { // edited is theirs. The bytes no longer hash to what io-cli recorded, which is // true, and is also what the next upgrade will decide about it. write(&ours, &skill("mine", "Edited by the operator.")); - let view = skillview::view(&home, &skills::dir(&home)); + let view = skillview::view(&home, &skills::dir(&home), &[]); assert_eq!( listed(&view, "mine").origin, Origin::Yours, @@ -136,7 +139,7 @@ fn f6_a_skill_carries_its_description_its_state_and_the_file_it_lives_in() { let path = dir.join("alpha.md"); write(&path, &skill("alpha", "Does the alpha thing.")); - let view = skillview::view(&home, &skills::dir(&home)); + let view = skillview::view(&home, &skills::dir(&home), &[]); let row = listed(&view, "alpha"); assert_eq!(row.description, "Does the alpha thing."); assert!(row.enabled, "a file in skills/ is offered to the model"); @@ -154,9 +157,9 @@ fn f6_a_disabled_skill_is_listed_as_disabled_with_its_path_inside_disabled() { let path = dir.join("alpha.md"); write(&path, &skill("alpha", "Does the alpha thing.")); - let moved = skillview::disable(&path).expect("the move"); + let moved = skillview::disable(&path, &[]).expect("the move"); - let view = skillview::view(&home, &skills::dir(&home)); + let view = skillview::view(&home, &skills::dir(&home), &[]); let row = listed(&view, "alpha"); assert!(!row.enabled, "a file under disabled/ is not offered"); // Canonicalised on both sides: the surface resolves a path the way @@ -193,7 +196,7 @@ fn f6_a_directory_that_will_not_discover_shows_the_harness_sentence() { &skill("parked", "Turned off earlier."), ); - let view = skillview::view(&home, &skills::dir(&home)); + let view = skillview::view(&home, &skills::dir(&home), &[]); let sentence = view .failed @@ -240,7 +243,7 @@ fn f7_disable_then_enable_returns_the_file_byte_for_byte_and_leaves_one_copy() { "disabled/ is created on demand" ); - let parked = skillview::disable(&path).expect("the move"); + 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")); @@ -254,7 +257,7 @@ fn f7_disable_then_enable_returns_the_file_byte_for_byte_and_leaves_one_copy() { ); assert_eq!(read(&parked), before.as_bytes(), "a move rewrites no byte"); - let back = skillview::enable(&parked).expect("the move back"); + let back = skillview::enable(&parked, &[]).expect("the move back"); assert_eq!(back, path); assert!( !parked.exists(), @@ -275,7 +278,7 @@ fn f7_a_disabled_skill_is_gone_from_the_harnesss_own_discovery() { assert_eq!(discovered(&dir), vec!["alpha", "beta"]); - skillview::disable(&dir.join("alpha.md")).expect("the move"); + 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`. @@ -293,8 +296,8 @@ fn f7_neither_lever_writes_to_io_toml() { 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"); + 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"), @@ -315,7 +318,7 @@ fn f7_a_move_that_would_overwrite_a_file_is_a_sentence_and_not_a_loss() { 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"); + let error = skillview::enable(&parked, &[]).expect_err("it declines"); assert!( error.contains("alpha.md"), "the message names the file: {error}" @@ -341,7 +344,7 @@ fn f7_a_rename_that_fails_is_a_readable_sentence_and_never_a_panic() { 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"); + let error = skillview::disable(&path, &[]).expect_err("it fails rather than panicking"); assert!( error.contains("gone.md"), "the message names the file: {error}" @@ -388,7 +391,7 @@ fn drawable() -> (tempfile::TempDir, PathBuf) { #[test] fn n4_every_row_draws_in_ascii_with_its_meaning_intact_inside_eighty_columns() { let (_dir, home) = drawable(); - let view = skillview::view(&home, &skills::dir(&home)); + let view = skillview::view(&home, &skills::dir(&home), &[]); assert_eq!(view.failed, None); assert_eq!(view.skills.len(), 3); @@ -428,7 +431,7 @@ fn n4_every_row_draws_in_ascii_with_its_meaning_intact_inside_eighty_columns() { #[test] fn n4_the_path_is_what_gives_way_and_the_name_and_the_state_never_do() { let (_dir, home) = drawable(); - let view = skillview::view(&home, &skills::dir(&home)); + let view = skillview::view(&home, &skills::dir(&home), &[]); let ascii = io_cli::glyphs::ASCII; let glyphs = &ascii; @@ -509,14 +512,14 @@ fn enabling_never_puts_a_second_file_under_a_name_already_answered_to() { 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 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); + let back = skillview::enable(&parked, &[]); assert!( back.is_err(), "enabling moved a second `alpha` in beside the first; every turn of that \ @@ -550,7 +553,7 @@ fn install_skips_a_name_that_is_disabled_under_a_different_file_name() { let theirs = dir.join("mine.md"); write(&theirs, &skill("io-mcp", "The operator's own.")); - skillview::disable(&theirs).expect("the move out"); + skillview::disable(&theirs, &[]).expect("the move out"); skills::install(&home); @@ -566,3 +569,306 @@ fn install_skips_a_name_that_is_disabled_under_a_different_file_name() { expected.sort(); assert_eq!(discovered(&dir), expected); } + +// --------------------------------------------------------------------------- +// 0.21.0 — a `[[plugin]]` bundle's skills reach the model, so they belong on the +// surface that says what the model is offered; and the two levers may not touch +// a file in a directory io-cli does not own. +// --------------------------------------------------------------------------- + +/// A bundle's skills directory, and the `(id, dir)` pair [`skillview::view`] takes. +/// +/// Plain pairs rather than a configuration with `[[plugin]]` entries in it, which +/// is the whole point of the signature: this surface is answerable from a fixture +/// directory, and a test that had to write a manifest to ask about a directory +/// would be testing the loader instead. +fn bundle(root: &Path, id: &str, files: &[(&str, &str)]) -> (String, PathBuf) { + let dir = root.join(id).join("skills"); + for (name, description) in files { + write(&dir.join(format!("{name}.md")), &skill(name, description)); + } + (id.to_string(), dir) +} + +/// Every file under `dir`, with its bytes. +/// +/// Sorted, so two snapshots compare as values. Used to assert an **absence**: a +/// refused lever left the bundle exactly as it found it — no file moved, no file +/// rewritten, and no `disabled/` directory conjured inside somebody else's bundle. +fn snapshot(dir: &Path) -> Vec<(PathBuf, Vec)> { + let mut found = Vec::new(); + let Ok(entries) = std::fs::read_dir(dir) else { + return found; + }; + for entry in entries.filter_map(Result::ok) { + let path = entry.path(); + if path.is_dir() { + found.extend(snapshot(&path)); + } else { + let bytes = read(&path); + found.push((path, bytes)); + } + } + found.sort(); + found +} + +/// The namespaced name io-harness itself builds, spelled here the way the module +/// spells it, out of the harness's own constant rather than a literal `"__"`. +fn namespaced(id: &str, name: &str) -> String { + format!("{id}{}{name}", io_harness::NAMESPACE) +} + +#[test] +fn a_bundle_skill_is_listed_under_the_name_the_model_addresses() { + // The gap 0.20.0 shipped with: a bundle's skills are folded into the turn's + // catalogue, namespaced, and no surface in io-cli listed one. An operator + // reading `/skills` was reading a list that disagreed with what the model had. + let (dir, home) = home(); + let ours = skills::dir(&home).join("mine.md"); + write(&ours, &skill("mine", "The operator's own.")); + let plug = bundle(dir.path(), "acme", &[("helper", "Something contributed.")]); + + let view = skillview::view(&home, &skills::dir(&home), std::slice::from_ref(&plug)); + assert_eq!(view.failed, None); + assert!(view.bundles_failed.is_empty()); + + let row = listed(&view, &namespaced("acme", "helper")); + assert_eq!(row.description, "Something contributed."); + assert!(row.enabled, "a bundle's skill has no state but on"); + assert_eq!(row.origin, Origin::Bundle("acme".to_string())); + assert_eq!( + row.origin.word(), + "acme", + "the origin column names the bundle, which is where the operator has to go" + ); + assert!( + row.path.ends_with("helper.md"), + "the row says which file: {}", + row.path.display() + ); + + // **The absences.** A row under the bare name would be a row under a name the + // model cannot address; and the bundle's file is not the operator's, whatever + // the manifest does or does not say about it. + let names: Vec<&str> = view.skills.iter().map(|s| s.name.as_str()).collect(); + assert!( + !names.contains(&"helper"), + "listed un-namespaced, which is a name no turn resolves: {names:?}" + ); + assert_eq!(listed(&view, "mine").origin, Origin::Yours); +} + +#[test] +fn two_bundles_each_contributing_one_name_do_not_collide() { + // The reason io-harness namespaces at all, asserted from this side: two + // bundles may both ship a `helper`, and the surface has to draw two rows an + // operator can tell apart — including two different origin words. + let (dir, home) = home(); + std::fs::create_dir_all(skills::dir(&home)).expect("the skills directory"); + let bundles = [ + bundle(dir.path(), "acme", &[("helper", "Acme's.")]), + bundle(dir.path(), "widget", &[("helper", "Widget's.")]), + ]; + + let view = skillview::view(&home, &skills::dir(&home), &bundles); + + let acme = namespaced("acme", "helper"); + let widget = namespaced("widget", "helper"); + assert_eq!( + view.skills + .iter() + .map(|s| s.name.as_str()) + .collect::>(), + vec![acme.clone(), widget.clone()], + "two bundles, two rows, sorted by the names the model uses" + ); + assert_eq!(listed(&view, &acme).description, "Acme's."); + assert_eq!(listed(&view, &widget).description, "Widget's."); + assert_eq!(listed(&view, &acme).origin.word(), "acme"); + assert_eq!(listed(&view, &widget).origin.word(), "widget"); + assert_ne!( + listed(&view, &acme).path, + listed(&view, &widget).path, + "two rows standing for one file would be a surface that lost one of them" + ); +} + +#[test] +fn a_bundle_whose_directory_is_missing_is_named_and_costs_only_itself() { + // **The session-killer.** `Plugin::skills_dir` does no existence check, and + // `TaskContract::discover_skills` walks every declared directory with `?` at + // run start — so a bundle naming a `skills` directory that is not on disk is a + // session where every turn dies before the first completion. This surface is + // the only place in io-cli that can say so, which is why the error is neither + // swallowed nor propagated. + let (dir, home) = home(); + let ours = skills::dir(&home).join("mine.md"); + write(&ours, &skill("mine", "The operator's own.")); + write( + &skills::disabled_dir(&home).join("parked.md"), + &skill("parked", "Turned off earlier."), + ); + let missing = dir.path().join("broken").join("skills"); + let bundles = [ + ("broken".to_string(), missing.clone()), + bundle(dir.path(), "acme", &[("helper", "Acme's.")]), + ]; + + let view = skillview::view(&home, &skills::dir(&home), &bundles); + + let (id, sentence) = view + .bundles_failed + .first() + .expect("a bundle that would end every turn is a state, not silence"); + assert_eq!(id, "broken", "the report names which bundle to go and fix"); + let harness = io_harness::Skills::discover(&missing) + .expect_err("the fixture directory is not there") + .to_string(); + assert_eq!( + sentence, &harness, + "the surface carries the harness's own sentence verbatim; it names the directory" + ); + assert_eq!(view.bundles_failed.len(), 1, "one broken bundle, one entry"); + + // **And it cost exactly itself.** The operator's directory discovered, so + // `failed` stays empty — one field holding either failure could not say which + // of the two happened, on the question where the difference is everything. + assert_eq!(view.failed, None); + assert_eq!( + view.skills + .iter() + .map(|s| s.name.as_str()) + .collect::>(), + vec![ + namespaced("acme", "helper"), + "mine".to_string(), + "parked".to_string() + ], + "every other row still draws" + ); +} + +#[test] +fn the_operators_own_skills_survive_a_bundle_that_will_not_discover() { + // The same rule from the other side, and the one that would be lost to a `?`: + // a broken bundle must not take away the rows the operator came to read. The + // enabled set, the disabled set and their provenance are all still here. + let (dir, home) = home(); + let ours = skills::dir(&home).join("mine.md"); + write( + &ours, + &skill("mine", "A skill io-cli happens to have written."), + ); + write( + &skills::disabled_dir(&home).join("parked.md"), + &skill("parked", "Turned off earlier."), + ); + recorded(&home, &[("mine", ours.as_path())]); + let bundles = [("broken".to_string(), dir.path().join("nowhere"))]; + + let view = skillview::view(&home, &skills::dir(&home), &bundles); + + assert_eq!(view.bundles_failed.len(), 1); + assert_eq!(listed(&view, "mine").origin, Origin::IoCli); + assert!(listed(&view, "mine").enabled); + assert!(!listed(&view, "parked").enabled); + assert!( + view.skills + .iter() + .all(|s| !matches!(s.origin, Origin::Bundle(_))), + "a bundle that did not discover contributed no row" + ); +} + +#[test] +fn disable_is_refused_on_a_bundle_skill_and_leaves_the_bundle_untouched() { + // **`disable` computes its destination as the file's own parent joined to + // `disabled/`.** For a bundle's skill that parent is the bundle's directory, + // so without this guard io-cli would create a `disabled/` inside somebody + // else's bundle and move their file into it — where the bundle's next update + // will not find it and no surface will list it. `is_bundle` cannot see this: + // it decides a `SKILL.md` folder shape and knows nothing about whose directory + // a path is in. + // + // Sabotage: delete the `refuse_bundle` call in `skillview::disable`. Under it + // this test fails on the snapshot, with the bundle holding a `disabled/` it + // never had. + let (dir, home) = home(); + std::fs::create_dir_all(skills::dir(&home)).expect("the skills directory"); + let plug = bundle(dir.path(), "acme", &[("helper", "Acme's.")]); + let before = snapshot(&plug.1); + let names = discovered(&plug.1); + + let view = skillview::view(&home, &skills::dir(&home), std::slice::from_ref(&plug)); + let row = listed(&view, &namespaced("acme", "helper")); + + let error = skillview::disable(&row.path, std::slice::from_ref(&plug)) + .expect_err("io-cli does not move another product's files"); + assert!( + error.contains("acme"), + "the refusal names the bundle: {error}" + ); + assert!( + error.contains(&plug.1.display().to_string()), + "and names the directory the operator has to change it in: {error}" + ); + + // The absences, and they are the assertion: nothing moved, nothing was + // rewritten, and no `disabled/` appeared inside the bundle. + assert_eq!( + snapshot(&plug.1), + before, + "the refused lever touched the bundle" + ); + assert!( + !plug.1.join(skills::DISABLED).exists(), + "io-cli made a disabled/ inside a bundle it does not own" + ); + assert_eq!(discovered(&plug.1), names, "and the bundle still discovers"); +} + +#[test] +fn enable_is_refused_on_a_bundle_skill_and_leaves_the_bundle_untouched() { + // The same refusal in the other direction. No bundle file can be in a + // `disabled/` this surface made — that is the point of the test above — so + // this asserts the pair is symmetric rather than a second route to the same + // hole: a lever that refuses one way and not the other is a lever whose rule + // a reader has to look up. + // + // Sabotage: delete the `refuse_bundle` call in `skillview::enable`. Under it + // this test fails, and the file lands two levels up, outside the bundle's + // skills directory entirely. + let (dir, home) = home(); + std::fs::create_dir_all(skills::dir(&home)).expect("the skills directory"); + let plug = bundle(dir.path(), "acme", &[("helper", "Acme's.")]); + let before = snapshot(&plug.1); + + let view = skillview::view(&home, &skills::dir(&home), std::slice::from_ref(&plug)); + let row = listed(&view, &namespaced("acme", "helper")); + + let error = skillview::enable(&row.path, std::slice::from_ref(&plug)) + .expect_err("io-cli does not move another product's files"); + assert!( + error.contains("acme"), + "the refusal names the bundle: {error}" + ); + + assert_eq!( + snapshot(&plug.1), + before, + "the refused lever touched the bundle" + ); + assert!( + row.path.exists(), + "{} is gone, so the refusal lost the file it was protecting", + row.path.display() + ); + // And the guard did not spill over onto the operator's own directory: their + // levers still work while a bundle is in the list. + let ours = skills::dir(&home).join("mine.md"); + write(&ours, &skill("mine", "The operator's own.")); + let parked = skillview::disable(&ours, std::slice::from_ref(&plug)).expect("their own move"); + skillview::enable(&parked, std::slice::from_ref(&plug)).expect("and back again"); + assert!(ours.exists()); +}