fix(cli): keep usage explain from running a spec's choices run= commands

[jdx]

https://entire.io/gh/jdx/usage/trails/51

usage explain answers "what would this command line bind to?" without running anything, and it promises that in the code: a spec file that arrived with a bug report shouldn't be able to run its commands on the machine of whoever explains a line against it. Mounts were already answered from injected outputs instead of being run. But choices run= (#1497) runs its command during parse-time validation, so usage explain ran it too:

bin "m"
arg "<svc>" {
  choices run="touch /tmp/ran; echo app"
}

Before: usage explain -f spec.kdl m nope created /tmp/ran. After: nothing runs, and the report shows <svc> = "nope" bound from argv with no error. The value is let through unchecked, because only the command could say whether it's valid, and explaining must not ask it. Values the declared choices list are still checked as usual.

Library: Parser::without_running_commands() is new. With it set, a choices run= command never starts during parse, explain or explain_refused. A value outside the declared choices is accepted unchecked, and a help page reached through the parse describes the command instead of listing its output. Parsing without it is unchanged. usage explain now uses it. Mounts are unaffected; they keep their existing injected-output handling. The MCP server only describes commands and never parses a command line, so it never ran these commands.

Tests: a_parser_told_not_to_run_commands_never_does (lib) and a_choices_command_is_never_run_to_answer_a_report (usage explain) check with a marker file that the command never ran. Both fail without the fix. Both are Unix-only, like the other command-running tests. cargo test --all --all-features and mise run lint pass.

This was raised by Greptile's review of #1498, which is where the merged #1497 code showed up in its view.

🤖 Generated with Claude Code

---

Note

Medium Risk
Changes parse-time validation semantics for callers using without_running_commands(); default parsing behavior is unchanged, but explain now may report values as bound that a real run would reject.

Overview
usage explain no longer executes choices run= shell commands when building a binding report. It now parses through Parser::without_running_commands(), matching the existing mount policy: inspect a line without side effects from untrusted specs.

The library adds Parser::without_running_commands() and a thread-local NoRunScope. While active, run_choices refuses to spawn processes; strict validation skips errors that would require command output, so argv values outside the static choice list are accepted unchecked (help text describes the command instead of listing its output). Declared static choices and mount injection behavior are unchanged when the flag is not set.

Tests (Unix): marker-file checks in explain and choices_run assert the run= command never runs under explain / no-run parsing.

^{Reviewed by Cursor Bugbot for commit eb0f6d1. Bugbot is set up for automated code reviews on this repo. Configure here.}

Summary by CodeRabbit

• New Features
  • The explain command can now report choice values without running choices run= commands, avoiding their side effects during explanations.
  • Parsing can also be configured not to execute these commands. In this mode, values are accepted when their validity cannot be checked because the command was not run; static choices are still validated.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Advanced

Run ID: 5dc9c4f5-0487-41cc-9a47-988ede38b0d4

📥 Commits

Reviewing files that changed from the base of the PR and between 80a2b07 and eb0f6d1.

📒 Files selected for processing (4)

• cli/src/cli/explain.rs
• lib/src/parse.rs
• lib/src/spec/choices.rs
• lib/tests/choices_run.rs

Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review.

---

📝 Walkthrough

Walkthrough

The parser now supports skipping choices run= command execution during parsing and help explanation. The explain command uses this option for parsers built for mount answers. Tests check that commands do not run and that parsing accepts the specified values.

Changes

Choice command execution

Layer / File(s)	Summary
Parser opt-out and choice handling lib/src/parse.rs, lib/src/spec/choices.rs, lib/tests/choices_run.rs	Parser::without_running_commands disables choices run= execution. When disabled, choice-check failures for these choices are ignored, and help explanation uses the same no-run scope. Tests check command-derived values, help explanation, and static choices.
Explain command integration cli/src/cli/explain.rs	Parsers built for mount answers disable command execution. A Unix-only test checks the reported value, empty errors, and that the command did not create its marker file.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Parser
  participant NoRunScope
  participant run_choices
  participant ChoiceValidation
  Parser->>NoRunScope: Enter scope when command execution is disabled
  Parser->>run_choices: Resolve run= choices
  run_choices->>NoRunScope: Check whether runs are disabled
  run_choices-->>ChoiceValidation: Return shell error without executing command
  ChoiceValidation-->>Parser: Ignore choice-check failure for run= choices

Loading

Merge Risk: ⚪ Minimal · up to eb0f6

The reviewed change is mergeable after normal checks; no actionable risk remains identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 68.75% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: preventing usage explanation from executing choices run= commands.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Confidence Score: 5/5

The PR appears safe to merge.

Summary

This PR makes usage explain avoid executing choices run= commands and adds a library parser option for the same inspection behavior.

• Command-dependent values remain bound without validation when execution is disabled; declared choices still validate.
• Unix-only marker tests cover the library and CLI paths.

_{Reviews (1) · Last reviewed commit: "fix(cli): keep usage explain from runnin..."}