Point the agent skill at --help instead of a second copy of the docs - #152
Point the agent skill at --help instead of a second copy of the docs#152leenk7991 wants to merge 6 commits into
Conversation
An agent reading the skill from a branch or a registry can be told about flags the installed CLI does not accept. Compiling skills/corgea/SKILL.md into the binary makes the reference pinned to the version being driven, by construction. corgea skill show prints it verbatim. It is dispatched without the token check that guards install, because reading a compiled-in string cannot need a login, and the point is that it still answers when the registry does not. install --local writes the same content, refusing a name other than corgea or an explicit version since neither is something the binary can honour. The registry path is unchanged and still serves company-authored skills. Co-authored-by: Cursor <cursoragent@cursor.com>
Config::load() creates ~/.corgea/config.toml and panics when it cannot, so `corgea skill show` exited 101 with a backtrace in a sandbox with a read-only home. That is the environment the command is most useful in, and printing a string compiled into the binary should not depend on the filesystem, so it is now dispatched before the config loads. Writing via write_all also lets `corgea skill show | head` exit quietly. Rust ignores SIGPIPE, so print! would have panicked once the skill grew past the pipe buffer. Also anchors the frontmatter test to the frontmatter block rather than matching anywhere in the file. Co-authored-by: Cursor <cursoragent@cursor.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review risk: 3/5.
The embedded display path is robust, but local installation still unnecessarily depends on a writable HOME. Process-level guarantees also lack integration coverage.
Critical or high-priority changes must be addressed.
Automatic approval was not submitted: automated review found critical or high-priority findings.
The early return added for `skill show` only covered that one command, so `skill install corgea --local --dir <writable-dir>` still reached the unconditional `Config::load()` and panicked creating ~/.corgea/config.toml. The embedded path needs neither a registry nor a token, so the local installer was unusable in the same sandbox `skill show` was fixed for. Replace the special case with one rule: `Config::load_or_defaults()` falls back to in-memory defaults, and `tolerates_unusable_home` decides which commands get it. Only the two that serve the compiled-in skill qualify; everything else still fails loudly, because it needs a token or somewhere to save one. The existing tests only read the embedded constant, so they survived every regression this PR guards against. Add binary-level tests that drive the real executable with no token under an unusable HOME and compare the bytes it prints and writes against skills/corgea/SKILL.md. Reverting the config guard fails four of them; removing the --local auth bypass fails five.
There was a problem hiding this comment.
Automated review risk: 3/5.
The offline commands are well tested for an uncreatable HOME, but config loading still panics on malformed configuration and mutates fresh homes. Both contradict the core no-config/no-persistence behavior.
Critical or high-priority changes must be addressed.
Automatic approval was not submitted: automated review found critical or high-priority findings.
`load_or_defaults` called `load()`, which creates ~/.corgea and writes config.toml as a side effect of resolving its path. So `skill show` began persisting state on a writable home -- a regression against the early return it replaced, and a contradiction of its own comment. It also inherited the `.expect()` on the TOML parse, so a config the user had merely mistyped took down the commands that exist to work when nothing else does. Read the file directly instead, through a `config_path_readonly` that creates nothing, falling back to the defaults when it is absent, unreadable or unparseable. `load()` now returns a parse failure as an `io::Error` naming the file rather than panicking, and `main` prints that and exits 1: a file the user can edit is their problem to fix, not a crash with a backtrace note. The tolerance is scoped to the embedded path. A command that needs the config still refuses to run on one it cannot parse.
There was a problem hiding this comment.
is this something a client ran into @leenk7991 @juangaitanv ? if not I'm not sure about embedding the skill file into the binary maybe we just add a block to the skill to ask the agent to just update the CLI "If a command or argument isn’t found in the CLI tool, it may need to be updated. Check which package manager was used to install it and update the package to the latest version"
Another option will be to just show a warning when someone tries to install the corgea skill with an old version of the CLI
Dropped the embedded skill, `corgea skill show` and `--local`. The problem they solved is real but unreported: no client has hit version skew, and `corgea --help` is already a version-pinned reference compiled into the binary by clap, for free. The skill now says so. It opens with `corgea --version`, names `--help` as authoritative wherever the two disagree, and tells the agent to report an outdated CLI with the upgrade for how it was installed rather than upgrading unprompted, since CI runners and self-hosted installs are often pinned. It also names the case a "command not found" rule misses: a changed default or output shape does not error, so a surprising result on an older CLI is a version difference before it is a bug. Kept from the earlier approach: a malformed `config.toml` used to panic with a backtrace note, because `load` called `.expect()` on the parse and `main` called `.expect()` on the result. It is a file the user can edit, so it now returns an `io::Error` naming the file and exits 1. Co-authored-by: Cursor <cursoragent@cursor.com>
leenk7991
changed the title
not something a client ran into. took your first suggestion, now the agent reports outdated cli |
There was a problem hiding this comment.
Automated review risk: 1/5.
Low-risk refactor: malformed configuration now produces a clear error instead of panicking, with appropriate integration coverage. Earlier review concerns target code removed from the supplied diff.
No critical or high-priority changes were found.
Automatic approval was not submitted: changes have been requested.
`--help` exits during clap parsing, so the assertion held whether or not `Config::load` ever ran. Use `ls`, the same command the malformed-config test uses, so the two differ only in whether the file parses, and assert on reaching the auth gate — which is only possible once the config has been read. Co-authored-by: Cursor <cursoragent@cursor.com>
4 participants
Summary
An agent reading this skill can be told about commands the installed CLI does not have.
corgea --helpis compiled into the binary by clap, so it already describes the exact version on the machine — the skill now defers to it rather than asking to be trusted on its own.The skill opens with
corgea --version, names--helpauthoritative wherever the two disagree, and tells the agent to confirm any unfamiliar command against it before running.On a mismatch it tells the agent to report the outdated CLI, with the upgrade for the package manager it was installed with, rather than upgrading unprompted. CI runners and self-hosted installs are often pinned deliberately, and a security tool that upgrades itself mid-run is a bad default.
It also names the case a plain "command not found" rule misses. A removed flag errors and is easy to catch; a flag whose default or output shape changed does not error at all. The skill tells the agent to read a surprising result on an older CLI as a version difference before treating it as a bug.
A malformed config no longer panics
Separate fix in the same PR.
Config::loadcalled.expect()on the TOML parse andmaincalled.expect()on the result, so a mistyped~/.corgea/config.tomlgave every command a Rust panic and aRUST_BACKTRACEnote.It is a file the user can edit, so it is now an ordinary error:
loadreturns anio::Errornaming the file and the parse position, andmainprints that and exits 1. Commands that need the config still refuse to run on one they cannot parse.Related
Corgea/skills#7 slims the public
corgea-scanskill against the same principle — it keeps install, auth and the core commands and defers the flag matrix to--helpand the docs, so the two copies have little overlap left to drift.Test plan
cargo fmt --checkandcargo clippy --all-targets -- -D warningscleancargo testsuite greenconfig.tomlexits 1 with a message naming the file, and no panic or backtracetests/cli_deps_skill.rsstill passes, so the generated deps catalog block stays current