feat(mcp): serve a usage spec to an agent over stdio - #746
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
📝 WalkthroughWalkthroughAdds a new ChangesMCP usage server
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant UsageMcp
participant SpecServer
participant UsageSpec
MCPClient->>UsageMcp: Send JSON-RPC over stdin
UsageMcp->>SpecServer: Start MCP stdio transport
SpecServer->>UsageSpec: Resolve commands and flags
SpecServer-->>MCPClient: Return tool response over stdout
Possibly related PRs
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryAdds a local stdio MCP server for querying usage specifications.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (5): Last reviewed commit: "fix(mcp): make the two tools agree on ho..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Switched the implementation to My original reason for hand-rolling the protocol — avoiding an async runtime in a synchronous CLI — was asserted rather than measured. Measured, the cost is binary size and nothing else:
The tokio runtime is built inside Two bugs surfaced while doing it:
Verified end to end against a real client handshake — This comment was generated by an AI coding assistant. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
|
Addressed the review: Stdin spec exhausts MCP stream (cursor, greptile) — fixed in 4d831d9. Inherited global flags are omitted (greptile) — correct and fixed in e9bc59f. usage resolves ancestors' Describe exposes hidden commands (cursor) — deliberate, now commented and tested rather than incidental. Verified end to end against a client handshake; This comment was generated by an AI coding assistant. |
My previous commit walked the ancestor chain itself and let a nearer declaration shadow a farther one. That is backwards. `merge_subcommand_flags` treats a non-global re-declaration sharing a global's long name as the *same* logical flag: the global's declaration survives and only the re-declaration's extra aliases are unioned in. So a subcommand re-declaring `-y --yes` without an effect was reported as `effect: null` when the flag actually carries the global's `write` — the exact failure this server exists to prevent, and worse than the omission it replaced. Rather than restate those rules a second time, `usage::available_flags(chain)` exposes the parser's own resolution and `describe_command` calls it. A test asserts it agrees with `parse_partial` for every command in a spec, so the two cannot drift. Two things fell out: - The merge can leave one logical flag under two `Arc`s — the merged declaration on the long key, the pre-merge one on the short — so `available_flags` collapses by name after the pointer dedup. Harmless for parsing, which looks up by key, visible to anything listing flags. - The `"inherited"` field is gone. Under these rules there is no clean local-vs-inherited split, and asserting one would be another small lie. `"global"` already says the flag is accepted everywhere. Reported by cursor and greptile on #746. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Rebased onto main (which now has #747) and fixed the flag resolution. cursor and greptile were both right, and my previous fix was worse than the bug it replaced. I walked the ancestor chain by hand and let a nearer declaration shadow a farther one. The fix isn't to restate those rules more carefully. Two things fell out:
Verified end to end: a spec with a long-only global This comment was generated by an AI coding assistant. |
`usage mcp -f mycli.usage.kdl` speaks the Model Context Protocol, so an
agent can ask what a command does before running it:
pitchfork logs effect=read
--clear effect=destructive Delete logs
pitchfork daemons remove effect=destructive
That is the payoff for `effect=`. Five CLIs now declare it across roughly
385 commands, and until now nothing could read any of it — the data existed
and no consumer did.
Local rather than hosted, on purpose. An agent doing real work is in a
project, in front of a CLI that is installed, so "what does this do" is a
local question. It costs nothing to run, has no abuse surface, and works for
private and internal CLIs a public service could never see. usage.sh stays
the right place for the other case: a CLI you do not have and want to ask
about.
Two tools. `list_commands` gives the tree with each command's effect;
`describe_command` gives one command's help, flags and arguments with the
effect of each. Effect is reported as an attribute alongside help and
aliases, not as a separate concept, and an unset one stays null rather than
defaulting to something reassuring. The server instructions spell out what
the three values mean and that a missing one means ask, since the client
sees them before it sees any command.
Hidden commands are excluded with their subtrees, since a visible child of a
hidden parent is not a documented path, and `include_hidden` opts back in.
The protocol is hand-written: JSON-RPC 2.0 over newline-delimited stdio, and
a read-only server needs `initialize`, `tools/list` and `tools/call`. An SDK
would have pulled an async runtime into a CLI that has no tokio and does not
need one. serde_json was already here.
13 tests cover the protocol edges as well as the data — notifications
getting no reply, unparseable input answering with a null id, an unknown
method being a protocol error while an unknown command is a tool error the
agent can recover from, and structured results also carrying their JSON as
text, which the spec asks for.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The first pass implemented the protocol by hand to avoid pulling an async
runtime into a synchronous CLI. That justification was asserted, not
measured, and it diverges from `mise mcp` and `fnox mcp`, which both use
rmcp — three servers in one author's tools should not each reimplement a
different subset of MCP.
Measured, the cost is size only:
binary 8,417,552 -> 11,365,072 bytes (+2.8 MiB)
complete-word (mise's 208 KB spec, 120 runs)
117.8 ms -> 118.1 ms median
The runtime is built inside `mcp run`, so nothing else pays for it. In
exchange the server gets version negotiation, pagination, cancellation,
`tools/list` schemas generated from the params structs, and correct
protocol-vs-tool error framing, and the module drops ~170 lines.
Two things the rewrite fixed on the way:
- `get_info` sets `server_info` explicitly. rmcp's default reads the
`CARGO_*` vars of its own crate, so a server that omits it introduces
itself to clients as "rmcp 2.2.0".
- `--file -` is now rejected. It reads stdin to EOF, which is the
transport the server then wants to serve on, so it could only ever
produce a session that ended before it began.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
usage resolves `global=#true` flags from ancestors while parsing an invocation rather than copying them onto each subcommand, so `describe_command` reporting only `cmd.flags` left them out entirely. `pitchfork daemons remove` accepts `--yes`, and that flag declares `effect="write"` — precisely the thing this server exists to surface. Inherited flags are appended after the command's own and marked `"inherited": true`. A nearer definition shadows a farther one, matching how the parser resolves the collision, so a subcommand that redefines a global without an effect is not reported as carrying the global's. Describing a hidden command stays allowed, now with a comment and a test saying so deliberately. `list_commands` omits them, but an agent that names one already knows it exists; refusing would only mean it runs the command without learning the effect. The response carries `"hidden": true` either way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
My previous commit walked the ancestor chain itself and let a nearer declaration shadow a farther one. That is backwards. `merge_subcommand_flags` treats a non-global re-declaration sharing a global's long name as the *same* logical flag: the global's declaration survives and only the re-declaration's extra aliases are unioned in. So a subcommand re-declaring `-y --yes` without an effect was reported as `effect: null` when the flag actually carries the global's `write` — the exact failure this server exists to prevent, and worse than the omission it replaced. Rather than restate those rules a second time, `usage::available_flags(chain)` exposes the parser's own resolution and `describe_command` calls it. A test asserts it agrees with `parse_partial` for every command in a spec, so the two cannot drift. Two things fell out: - The merge can leave one logical flag under two `Arc`s — the merged declaration on the long key, the pre-merge one on the short — so `available_flags` collapses by name after the pointer dedup. Harmless for parsing, which looks up by key, visible to anything listing flags. - The `"inherited"` field is gone. Under these rules there is no clean local-vs-inherited split, and asserting one would be another small lie. `"global"` already says the flag is accepted everywhere. Reported by cursor and greptile on #746. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`list_commands` emitted `"daemons remove"` while `describe_command` answered with `"pitchfork daemons remove"`. An agent doing the obvious thing — read the list, describe an entry, describe something the first response mentioned — got a tool error on a command that exists, because `find_chain` read the binary name as a subcommand. `describe_command` now reports the path alone and carries `bin` beside it, the way `list_commands` already does. A test walks every row of `list_commands` through `describe_command` and asserts the name comes back unchanged, so the two ends cannot drift apart again. A leading binary name is also accepted now, since an agent that has seen the CLI in a shell writes the whole line. Only skipped when the root has no subcommand by that name, so a CLI with a `usage usage` keeps resolving its own command rather than losing it to the prefix. Rebased onto main, which brought #751. Its `nothing_is_unclassified_by_accident` test failed immediately on `mcp`, which is what it is for; classified `read`, since every tool this serves only reads the spec it was handed. Unlike `mise mcp`, which stays unclassified because it serves a tool that runs tasks, nothing here can act on the CLI it describes. Reported by cursor on #746. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Rebased onto main (which brought #751) and fixed the path inconsistency. Command path format inconsistent (cursor) — correct.
A leading binary name is accepted too, since an agent that has seen the CLI in a shell will write the whole line. It's only skipped when the root has no subcommand by that name — a CLI with a One thing worth reporting: the rebase brought #751's Verified against This comment was generated by an AI coding assistant. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 259021a. Configure here.
| let mut available = gather_flags(root); | ||
| for cmd in rest { | ||
| merge_subcommand_flags(&mut available, gather_flags(cmd), false); | ||
| } |
There was a problem hiding this comment.
Mount crossing ignored in available_flags
Medium Severity
The new available_flags helper always calls merge_subcommand_flags with crossing_mount set to false. During real parsing, that flag is true when entering a subcommand whose mounted field is set while the parent is not, which changes how inherited globals collide with the mounted program’s flags. Flag lists from available_flags can then disagree with parse_partial for the same command chain, including what the MCP server reports via describe_command when the chain includes mounted commands.
Reviewed by Cursor Bugbot for commit 259021a. Configure here.
Instruction counts
2 benchmark(s) above the 1% gate: Only instruction counts gate. Wall clock is shown for context — on identical hardware it moves 4-20% run to run. Measured by tak — instruction-counted CLI benchmarks, stored in this repository's git notes.
|
|
The perf gate is red: Method: build
In absolute terms it's +56 µs. For the path that actually matters, Your call, and I'd rather you made it than have me pick:
Happy to do either. What I won't do is quietly widen the gate to make my own check green. This comment was generated by an AI coding assistant. |
1 participant
usage mcp -f mycli.usage.kdlspeaks the Model Context Protocol, so an agent can ask what a command does before running it. Against pitchfork's real spec:This is the payoff for
effect=. Five CLIs now declare it across roughly 385 commands — and until now nothing could read any of it. The data existed and no consumer did.Why local rather than usage.sh
An agent doing real work is in a project, in front of a CLI that is installed. "What does this do" is a local question. Answering it locally costs nothing to run, has no abuse surface, and works for private and internal CLIs a public service could never see.
usage.sh stays the right home for the other case — a CLI you don't have and want to ask about — which is also where the vendor skills live (jdx/usage-sh#8).
The tools
list_commandsdescribe_commandEffect is reported as an attribute beside
helpandaliases, not as a separate concept — per your call on #742. An unset effect staysnullrather than defaulting to something reassuring, and the serverinstructionsspell out what the three values mean and that a missing one means ask, since the client sees those before it sees any command.Hidden commands are excluded with their subtrees — a visible child of a hidden parent isn't a documented path, the same bug found in jdx/usage-sh#8 — with
include_hiddento opt back in.Hand-written protocol
JSON-RPC 2.0 over newline-delimited stdio. A read-only server needs
initialize,tools/listandtools/call; an SDK would have pulled an async runtime into a CLI that has no tokio and doesn't need one.serde_jsonwas already a dependency, so this adds none.Tests
13, covering the protocol edges as much as the data:
notifications/initialized; answering it is a violation)list_commandsservehandles a full session: two requests plus a notification produce exactly two responsesVerified end to end by piping a real session into the built binary against
pitchfork.usage.kdl, not only through unit tests.cargo test -p usage-cli,clippy --all-targets,fmt --checkclean.mise run renderregenerated the spec, man page, fig completions and CLI docs.Worth noting
usage's own spec declares no effects yet —
usage mcpcan describe every CLI except the one it ships in. Dogfooding that is a natural follow-up and I left it out to keep this reviewable.This PR was generated by an AI coding assistant.
Note
Medium Risk
New async MCP surface and dependency stack (tokio/rmcp); incorrect effect or flag metadata could mislead agents, though the server only reads the loaded spec.
Overview
Adds
usage mcp(aliasmcp-server), a local Model Context Protocol server on stdio viarmcpand a minimaltokioruntime. Clients load a spec with-f/-s;--file -is rejected because stdin is the transport.The server exposes
list_commandsanddescribe_command, returning help pluseffecton commands, flags, and args (unset staysnull). Server instructions explainread/write/destructive. Hidden commands are omitted from listings unless requested, but can still be described.usage-libgains publicavailable_flagsso flag reporting matches parser merge rules (globals, re-declarations).command_effectsclassifiesmcpas read-only. Generated CLI docs, man page, and Fig completions are updated.Reviewed by Cursor Bugbot for commit 259021a. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
usage mcpcommand (also available asmcp-server) to serve usage specifications through the Model Context Protocol.Documentation