/review-cli

Review CLI

Review of the interface a command-line tool presents, not the language it is written in. The checks below are the same for a Python CLI and a Rust one; only the fixes differ, and §8 carries those.

Complements review-<language> skills (which review the code) and review-interfaces (which reviews function and module signatures, not the CLI surface). Don't repeat either.

Relationship to the toolchain (read first)

There is essentially no tooling for this layer. That is not an assumption — a search across four phrasings for a CLI-design linter turned up clig.dev itself, generic static-analysis round-ups, and no tool. ShellCheck analyses shell scripts, i.e. programs written in bash; it says nothing about the interface a compiled or Python program presents. Neither ruff nor clippy ships a CLI-conventions rule set.

Two narrow exceptions worth knowing:

• Rust can mechanize the stream discipline in §2. clippy::print_stdout and clippy::print_stderr are restriction lints — off by default, but a crate can turn them on and #[allow] the handful of genuine output sites. When reviewing a Rust CLI, recommend enabling the lint rather than listing instances one by one.
• clap::Command::debug_assert() catches duplicate short flags and conflicting requirements — definition consistency, not design.

So this skill carries more weight than a language review does, and its findings must be earned by evidence, since nothing else will catch a mistake here and nothing else will catch a false positive either.

What this skill checks

A CLI's design is decided once; its behaviour regresses continuously. So the default review looks only at what the diff introduces — a new subcommand, a new output path, a new destructive operation, a new prompt. One-time properties (does --version exist, does --help carry descriptions, are config roots XDG-correct) belong to the project-level set in §P, which runs at --all and nowhere else. Re-checking those on every diff is noise, and it buries the findings that matter.

Step 0: probe what the diff touches

Most CLI behaviour is invisible in the source, so run the thing rather than reading it — but scope the probing to the change:

• Any scope: if the diff adds or changes an output path, pipe it into head and check stderr is empty. If it adds a destructive operation, run it with < /dev/null. If it adds --json output, pipe it through jq.
• --all only: the full probe table in §P.

The pipe probe needs output larger than the pipe buffer (~64 KB) or it silently passes. Use the largest output the changed command can produce.

A probe you could not run (no build, no fixture data) is reported as not run, never as passing.

Usage

/review-cli                  # Review context-related code
/review-cli --staged         # Review staged changes
/review-cli --unpushed       # Review files changed across all unpushed commits
/review-cli --changed        # Review unstaged changes
/review-cli --all            # Full CLI surface audit (the natural scope for this skill)
/review-cli --multi          # Also get external advisor opinions

Scope

Only --all runs §P. At every other scope the review covers §1–§8 against what the diff introduces, and a §P finding is out of scope even if you notice it — note it in one line at the end if it is severe, but never as a review finding.

Workflow

1. Determine scope and locate the entry point and argument definitions.

2. Run the Step 0 probes. Record results.

3. Read AGENTS.md / CLAUDE.md / README — the README matters unusually much here, because a gap between what it documents and what --help says is itself a finding (§6).

4. Review against §1–§7. Load references/conventions.md for the normative sources and the contested list; load references/language-notes.md when writing a fix.

5. Parallelize if the CLI has many subcommands: one sub-agent per category, merge and dedupe.

6. External opinions (if --multi): invoke second-opinion --quick with this prompt:

   Read-only review of a command-line tool's interface, not its implementation language. No
   linter covers this layer, so nothing is out of scope for being mechanical. Focus on: does a
   destructive command have a gate, and is whatever justifies skipping it available on every code
   path that reaches it? Does the tool refuse rather than assume when stdin is not a terminal?
   Does it ever report an action that did not happen? Do exit codes distinguish failure classes
   without colliding? Is primary output on stdout and every diagnostic on stderr? Does a dry-run
   flag reach the lowest write primitive or just the command handler? If it dies halfway, what is
   left behind and is a re-run safe? Do errors say what to do next? 300 words or less.

   Wait for all external results before proceeding.

7. Classify severity and report, grouped by severity.

1. Exit codes & signals

• Broken pipe is the single most commonly missed defect. A tool piped into head keeps writing after the reader exits. If SIGPIPE is ignored (which both Python and Rust do by default), the write fails and surfaces as a traceback or panic on stderr plus a wrong exit code. Check every CLI for this; the fix is language-specific (§8). The symptom differs: Python prints a BrokenPipeError traceback and exits 1; Rust prints a panic and exits 101, which is indistinguishable from a genuine bug — a Rust CLI reporting 101 in a bug report should make you suspect this first.
• Exit codes must distinguish failure classes, and no two classes may collide. The reviewable property is consistency and documentation, not which scheme. A CLI that returns 2 for both "you typed it wrong" (the parser's convention) and "this build doesn't do that" has made those indistinguishable to a caller. Do not prescribe sysexits.h — see the contested list.
• Zero on failure is a defect unless it is a documented inversion. A hook or shim that must never break its host can legitimately always exit 0, provided failures are routed to a channel the user can actually see (a health log with a doctor command, say). Check that the compensating channel exists before calling it a bug — and check it exists before calling it fine.
• Signal handling. If the tool catches SIGINT for cleanup, it should still terminate promptly and bound the cleanup; a second Ctrl-C must escape. Exiting 130 imitates what the shell reports for an uncaught SIGINT (128+2) and is the usual convention, though no primary source states it directly.
• A signal-terminated child's status is not an exit code. Passing a negative or raw wait status straight to the process exit turns Ctrl-C in a child into a nonsense shell status.

2. Streams & machine-readable output

• Primary output and anything machine-readable on stdout; every log, error and progress message on stderr. The tell is a diagnostic on stdout right next to a mechanism that gets it right — a prompt preamble printed to stdout while the prompt itself goes to stderr, or a line saying "see the warning above" when the warning went to stderr and the user redirected stdout to a file.
• Help is not an error. Requested help → stdout, exit 0. Usage error → stderr, non-zero. A tool that prints --help to stderr is wrong, and so is one that prints a usage error to stdout. This one has a definite answer; see the contested list for why the folklore says otherwise.
• A machine-readable format is what earns the freedom to change human output. A tool with no --json/--plain has implicitly made its pretty output an API. Check that machine output carries a schema version the consumer can branch on, and that nothing else is interleaved on stdout when it is active.
• Column widths computed from the result set are fine for humans and a trap for scripts — a note rather than a defect when a stable format is offered alongside.
• Colour off when not a TTY, and when NO_COLOR is set and non-empty regardless of its value. Check stdout and stderr independently: piping stdout does not mean stderr colour is unwanted. If the tool emits no colour at all, this whole item is moot — don't invent it.

3. Interactivity, TTY & destructive actions

• The gate must exist on every path that reaches the command. The highest-value check in this skill. When a destructive command has no confirmation because something else makes it recoverable — an undo, a trash, a backup — verify that thing is available on every backend, platform and configuration the command runs under. A safety net that exists for one of two storage backends does not gate the command; it gates half of it.
• Non-TTY must refuse, not assume. Silence is not consent and EOF is not yes. The correct shape is: explicit --yes/--force → proceed; interactive terminal → prompt; otherwise → refuse with a message naming the flag that would have worked. Checking that both stdin and stderr are terminals is stricter than the common stdin-only check and is right.
• Never require a prompt. Every prompted value needs a flag or argument equivalent, or the command is unscriptable. Prompts also belong at the CLI edge, not in the library layer where every caller — tests included — has to arrange a TTY.
• Unguarded input()/read crashes under < /dev/null and in CI. Check every prompt, not the first one; these are usually copy-pasted, and the guard is usually on some of the copies.
• Match the gate to the blast radius. Deleting one named thing the user just named may need no prompt. A bulk or remote deletion needs one. Something irreversible and large should be hard to confirm by accident — requiring the name to be typed, with a flag form like --confirm=<name> so it stays scriptable, is the design that satisfies both.
• Watch for non-obvious destruction — a config value changed from 10 to 1 that implicitly deletes nine things is a severe action wearing an edit's clothing.

4. Dry-run, partial failure & idempotency

• A dry-run flag must be threaded to the lowest write primitive, not checked at the command handler. An if not dry_run: at each call site is a promise a future caller will forget. In a typed language, ask for the stronger form: pass an effect enum or a writer into the backend so a path that writes without consulting it fails to compile.
• If it dies halfway, what is on disk, and is a re-run safe? Trace a mid-loop failure explicitly. Idempotent-by-accident (the re-run happens to work) is worth crediting and worth naming as accidental — the reviewer's question is whether the recovery path was designed or merely available today.
• Batch commands: validate everything, then apply everything. Resolving all inputs before the first mutation is the pattern; the alternative needs a documented partial-result contract and an exit code that expresses it. There is no established convention for what a bulk command should exit when 7 of 10 items succeed — so the finding is that the tool must decide and document it, not that it picked wrong.
• Never report an action that did not happen. Idempotency at the library layer is correct; narrating it as though it occurred is the bug. "removed X" with exit 0 when X was never present is a defect in any language — and it is worse when a typo'd argument triggers a teardown path that then reports success.
• Atomic writes and durability — temp file in the same directory, then rename. Whether the contents are fsynced before the rename decides whether a power cut yields a truncated file. Check especially where the tool rewrites files it does not own.
• Cap unbounded stdin if the tool reads it.
• Parse arguments before loading config, so --help still works on a machine with a broken config file. Easy to get backwards, trivial to check.

5. Config precedence

Per-change: a new setting must join the existing precedence chain, not start a second one. If the diff reads an environment variable or a config key directly instead of going through the project's one resolution function, that is the finding — a second copy of the order is how precedence drifts. Whether the order itself is right, and whether config lands in the correct XDG root, is §P.

Also per-change: a new secret read from a flag (it leaks into ps and shell history) or from an environment variable. A --password-file or stdin is the shape.

6. Flags & subcommands

Per-change, all of these are about the new surface agreeing with the surface it joins:

• A new flag or subcommand inconsistent with its siblings — the same concept spelled differently, a short alias where siblings have none, --json on three subcommands and not the fourth. A consistency promise already made is a defect to break; one never made is a scope decision.
• A new flag whose help text is missing while its siblings have one.
• A required thing modeled as optional — a flag declared optional but handled only in its present form, producing a confusing fallthrough instead of the parser's "required argument not provided". Let the parser express the constraint.
• A new conflict hand-checked instead of declared to the parser, so it neither appears in help nor fails uniformly.
• Validation for the new flag living in a second layer — the same mistake reachable two ways should produce the same message and the same exit code and envelope. A shared validator reached through two layers often doesn't.
• A changed flag, output shape, or subcommand name. These are interfaces you have committed to; a rename breaks scripts in the wild. Watch for a new catch-all subcommand or newly-allowed abbreviation, both of which make it impossible to add names later.

7. Errors & messages

• An error should name the next action. "unknown project nope — configured project(s): demo" is a finding-free error. A bare string with no remedy is the defect. The fuller shape worth aiming at: what failed, why, and how to fix it, plus a code or URL when there is one.
• Don't lose the cause at a boundary. Stringifying an I/O error into a message discards the path, the operation and the error kind, so no caller can branch on not-found versus permission-denied and the user sees "No such file or directory" with no indication which file. This costs more in languages where the chain is not printed for free (§8).
• A pointer to a message on another stream is not a message. See §2.
• The worst message is a success message for something that did not happen. See §4.

P. Project-level checks — --all only

These are decided once and almost never regress. Run them on a full audit; never report them on a diff review, where they are noise that buries the real findings.

The full probe table:

Plus, read once:

• Does --help carry what the README carries? A CLI whose subcommands have no descriptions while the README documents all of them has put its documentation where the user isn't.
• Is SIGPIPE handled at all, anywhere — one fix in main covers every write site (§8).
• Is precedence implemented exactly once, in one function every consumer calls, including any which/debug command that reports the resolution?
• Are config, data, state and cache in the right XDG roots? The one people get wrong is state: per the spec, $XDG_STATE_HOME holds data that persists between restarts but is "not important or portable enough to the user that it should be stored in $XDG_DATA_HOME" — history, logs, last-used selections. Credentials and user-authored content are data. The test: would the user want this back on a new machine?
• Does a machine-readable format exist, and does it declare a schema version a consumer can branch on? A tool with none has implicitly made its pretty output an API.
• Is there a --dry-run where the destructive surface warrants one?

8. What differs by language

The checks above are identical everywhere. These fixes are not — load references/language-notes.md before writing one. The three that most often change the finding itself:

The reference also covers stream discipline (Rust can turn this into a compile gate; Python can't), dry-run enforcement, atomic writes, env precedence, and file locking.

Do NOT flag these

• Anything in §P, on a diff review. Missing --version, thin --help, XDG placement and precedence structure are project properties; they have not changed because this diff didn't touch them, and reporting them every time trains the reader to skim.
• Missing --json on a tool with no machine consumers, or NO_COLOR handling on a tool that emits no colour. Both are real rules with a precondition; check the precondition.
• Deviations from POSIX Utility Syntax Guidelines 3, 7 and 9. Guideline 3 permits only single-character options, so --long has no POSIX basis at all; 7 forbids optional option-arguments; 9 requires options before operands. Git, docker and cargo all violate these deliberately. Guidelines 4, 10 and 13 are the durable ones.
• Not adopting sysexits.h — deprecated by the BSD that originated it, documented neutrally by Linux, still recommended by the Rust CLI Book. No consensus; don't manufacture one.
• Prescribing a specific exit number beyond 0/non-zero and the 2-for-usage convention.
• A deliberate always-exit-0 inversion with a working diagnostic channel (§1).
• Idempotent library behaviour — the bug is narrating it as an action, not the idempotency.
• Anything the project's own enabled lints already gate, including a Rust crate that has turned on the print lints.

Severity

• Critical: destroys data with no gate on a reachable path; reports success for a destructive action that did not happen, or for one that happened differently than stated; exits 0 on a failure the caller must detect, with no compensating channel.
• High: broken-pipe traceback or panic; colliding exit codes; a dry-run that doesn't reach the write; non-TTY assuming yes; a prompt with no flag equivalent; partial failure leaving state a re-run can't recover; precedence implemented twice.
• Medium: stream leaks; missing --version; empty --help; missing machine-readable format or schema version; a required flag modeled as optional; stringified errors losing the cause; config in the wrong XDG root.
• Suggestion: examples in help, short aliases, colour/TTY refinements, column-width notes.

Output Format

## CLI Review: {scope}

### Baseline
{Probes you ran and what happened, including any you could not run and why. Language and argument
library. **At `--all` only:** the §P results. On a diff review, omit this block unless a probe you
ran actually failed.}

### Critical (data loss / false success / undetectable failure)
- {file}:{line} — {category}: {description}
  **Probe:** {the command and its actual output, where a probe found it}
  **Impact:** {what breaks, for whom}
  **Fix:** {concrete change — with code, in this project's language}

### High (behaviour that will bite users or scripts)
- {file}:{line} — {category}: {description}
  **Fix:** {solution}

### Medium (interface and message quality)
- {file}:{line} — {category}: {description} — {suggested change}

### Suggestions
- {opportunities}

If --multi was used, append one subsection per advisor that responded (titled with the advisor's name as reported by second-opinion), then a Cross-Model Agreement subsection.

Examples

A gate that only covers half the paths:

/review-cli --all

rm has no confirmation, no --yes and no --force. The justification is an undo command — but undo is only wired for one of two storage backends, and on the other it returns "this backend does not support undo". Reports Critical: on that backend rm is unprompted and unrecoverable. The finding needs reading the gate, the thing that justifies its absence, and every construction path of that thing — three places, none of which look wrong alone.

Broken pipe, found by probe not by reading:

/review-cli

cmd ls | head -3 prints three rows and then panics — failed printing to stdout: Broken pipe, exit 101. Nothing in the source mentions SIGPIPE, and nothing looks wrong: it is a plain print in a loop. Reports High with the one-line fix for the language, and notes that exit 101 will be misread as a genuine bug in any report.

Success message for a no-op:

/review-cli --changed

uninstall --consumer <typo> removes nothing, then reports "removed consumer <typo> (last consumer; hooks and active binary removed)" and exits 0 — having actually torn down the shared hooks, because zero registered consumers remain. The idempotent remove is correct; the message is the defect. Reports Critical: a misspelled argument performs a full teardown and reports success.

Troubleshooting

Can't build or install the tool, so Step 0 is impossible

Solution: Review §2–§7 from the source and say in the Baseline that probes were not run. Do not infer probe results from reading — the whole point of Step 0 is that these behaviours don't appear in the source. Ask the user for a build command or a fixture project rather than guessing.

The pipe probe passes but the tool looks unprotected

Solution: The output fit in the pipe buffer (~64 KB), so the writer never saw the closed pipe. Re-run against the largest output the tool can produce, or a project with real data in it. A tool can be exposed by construction and shielded only by volume — that is still a finding, because the volume is the user's data, not a property of the tool.

Unsure whether a missing affordance is a defect or a scope decision

Solution: Check whether the tool's own conventions imply it. A CLI that offers --json on three subcommands and not the fourth has made a consistency promise; one that offers it nowhere has made a scope decision. Same for --dry-run, short flags, and confirmation prompts. Report the first as a defect and the second as a suggestion, if at all.

Notes

• Probe first, read second. The probes cost a minute and find the defects that reading misses.
• Respect project conventions in AGENTS.md / CLAUDE.md, and read the README — a gap between it and --help is a finding in its own right.
• Don't be dogmatic. The most-cited source in this area says of itself: "When following convention would compromise a program's usability, it might be time to break with it—but such a decision should be made with care." Look for the decision, not just the deviation.
• Credit what's right. A CLI that refuses on a non-TTY with a message naming both escapes, or that re-parses its own rendering before writing a file it doesn't own, is doing something most don't — and saying so keeps the review honest about what the remaining findings cost.
• Sources: Command Line Interface Guidelines (clig.dev); POSIX Utility Syntax Guidelines and <signal.h>; the GNU Coding Standards on --help and --version; the XDG Base Directory Specification; no-color.org; the GNU Bash manual on exit status; the Python signal docs' note on SIGPIPE; the Rust CLI Book and the -Zon-broken-pipe documentation; 12 Factor CLI Apps; git's --porcelain stability wording. Quoted lines and the contested list are in references/conventions.md.