Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions docs/spec/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,44 @@ The priority over which is used (CLI flag, env var, config file, default) is the
are defined,
so in this example it will be "CLI flag > env var > config file > default".

## Command effects

A command can declare what running it does to the world:

```kdl
cmd "ls" effect="read" help="List installed tools"
cmd "use" effect="write" help="Install a tool and add it to the config"
cmd "uninstall" effect="destructive" help="Remove a tool"
```

| Effect | Meaning |
| -------------- | ------------------------------------------------------------------------------------ |
| `read` | Only inspects state. Running it twice is the same as running it once. |
| `write` | Creates or modifies state, but removes nothing the user cannot recreate. |
| `destructive` | May delete or irreversibly overwrite something. Deserves a confirmation prompt. |

This is a coarse classification, not a permission model. It exists because
several consumers keep reinventing the same distinction:

- generated documentation and `--help` can mark destructive commands
- a wrapper script can require confirmation before running one
- an AI coding agent can be handed an allowlist of read-only commands instead of
prompting on every invocation

`effect` is **not inherited by subcommands**. `git remote` and
`git remote remove` do different things, and quietly inheriting a parent's
effect would make the least safe reading of a spec the default one. A command
with no `effect` is unknown, not safe — consumers should treat the absence of a
value as "ask".

It can also be written as a child node, which is easier to generate:

```kdl
cmd "uninstall" {
effect "destructive"
}
```

## Compatibility

Usage is not designed to model every possible CLI. It's generally designed for CLIs that follow
Expand Down
3 changes: 3 additions & 0 deletions lib/src/docs/markdown/templates/cmd_template.md.tera
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
{%- if cmd.aliases %}
- **Aliases**: `{{ cmd.aliases | join(sep="`, `") }}`
{%- endif %}
{%- if cmd.effect %}
- **Effect**: {% if cmd.effect == "read" %}read-only{% elif cmd.effect == "destructive" %}destructive — may delete or irreversibly overwrite{% else %}modifies state{% endif %}
{%- endif %}
Comment on lines +15 to +17

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Effect rendering lacks snapshot coverage

The new template branches for read, write, and destructive are not exercised by the existing Markdown snapshots because the shared test spec declares no effects. Add cases for all three values so changes to enum serialization or template comparisons cannot silently produce incorrect labels.

Knowledge Base Used: Docs generation: rendering a Spec into markdown, manpages, and CLI help

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Claude Code

{%- if source_code_link %}
- **Source code**: {{ source_code_link }}
{%- endif %}
Expand Down
3 changes: 3 additions & 0 deletions lib/src/docs/models.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use crate::docs::markdown::MarkdownRenderer;
use crate::spec::effect::SpecCommandEffect;
use crate::SpecChoices;
use indexmap::IndexMap;
use serde::Serialize;
Expand Down Expand Up @@ -37,6 +38,7 @@ pub struct SpecCommand {
pub flags: Vec<SpecFlag>,
// pub mounts: Vec<SpecMount>,
pub deprecated: Option<String>,
pub effect: Option<SpecCommandEffect>,
pub hide: bool,
pub subcommand_required: bool,
pub help: Option<String>,
Expand Down Expand Up @@ -214,6 +216,7 @@ impl From<&crate::SpecCommand> for SpecCommand {
flags,
// mounts: cmd.mounts.iter().map(SpecMount::from).collect(),
deprecated: cmd.deprecated.clone(),
effect: cmd.effect,
hide: cmd.hide,
subcommand_required: cmd.subcommand_required,
help: cmd.help.clone(),
Expand Down
1 change: 1 addition & 0 deletions lib/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ pub use crate::spec::builder::{SpecArgBuilder, SpecCommandBuilder, SpecFlagBuild
pub use crate::spec::choices::SpecChoices;
pub use crate::spec::cmd::SpecCommand;
pub use crate::spec::complete::SpecComplete;
pub use crate::spec::effect::SpecCommandEffect;
pub use crate::spec::flag::SpecFlag;
pub use crate::spec::mount::SpecMount;
pub use crate::spec::Spec;
Expand Down
7 changes: 7 additions & 0 deletions lib/src/spec/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
//! ```

use crate::spec::cmd::SpecExample;
use crate::spec::effect::SpecCommandEffect;
use crate::{spec::arg::SpecDoubleDashChoices, SpecArg, SpecChoices, SpecCommand, SpecFlag};

/// Builder for SpecFlag
Expand Down Expand Up @@ -464,6 +465,12 @@ impl SpecCommandBuilder {
self
}

/// Set what running this command does to the world
pub fn effect(mut self, effect: SpecCommandEffect) -> Self {
self.inner.effect = Some(effect);
self
}

/// Set deprecated message
pub fn deprecated(mut self, msg: impl Into<String>) -> Self {
self.inner.deprecated = Some(msg.into());
Expand Down
126 changes: 126 additions & 0 deletions lib/src/spec/cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use crate::error::UsageErr;
use crate::sh::sh;
use crate::spec::builder::SpecCommandBuilder;
use crate::spec::context::ParsingContext;
use crate::spec::effect::{SpecCommandEffect, EFFECT_VALUES};
use crate::spec::helpers::{string_entry, NodeHelper};
use crate::spec::is_false;
use crate::spec::mount::SpecMount;
Expand Down Expand Up @@ -49,6 +50,10 @@ pub struct SpecCommand {
/// Deprecation message if this command is deprecated
#[serde(skip_serializing_if = "Option::is_none")]
pub deprecated: Option<String>,
/// What running this command does to the world: read, write or destructive.
/// Not inherited by subcommands.
#[serde(skip_serializing_if = "Option::is_none")]
pub effect: Option<SpecCommandEffect>,
Comment thread
greptile-apps[bot] marked this conversation as resolved.
/// Whether to hide this command from help output
pub hide: bool,
/// True when this command came from a [`SpecMount`], i.e. it describes another
Expand Down Expand Up @@ -131,6 +136,7 @@ impl Default for SpecCommand {
flags: vec![],
mounts: vec![],
deprecated: None,
effect: None,
hide: false,
mounted: false,
flags_from_mount: false,
Expand Down Expand Up @@ -221,6 +227,17 @@ impl SpecCommand {
"after_help_md" => cmd.after_help_md = Some(v.ensure_string()?),
"subcommand_required" => cmd.subcommand_required = v.ensure_bool()?,
"hide" => cmd.hide = v.ensure_bool()?,
"effect" => {
let raw = v.ensure_string()?;
match raw.parse() {
Ok(effect) => cmd.effect = Some(effect),
Err(_) => bail_parse!(
ctx,
v.entry.span(),
"unsupported effect {raw}, expected one of: {EFFECT_VALUES}"
),
}
}
"restart_token" => cmd.restart_token = Some(v.ensure_string()?),
"deprecated" => {
cmd.deprecated = match v.value.as_bool() {
Expand Down Expand Up @@ -294,6 +311,18 @@ impl SpecCommand {
cmd.subcommand_required = child.ensure_arg_len(1..=1)?.arg(0)?.ensure_bool()?
}
"hide" => cmd.hide = child.ensure_arg_len(1..=1)?.arg(0)?.ensure_bool()?,
"effect" => {
let arg = child.ensure_arg_len(1..=1)?.arg(0)?;
let raw = arg.ensure_string()?;
match raw.parse() {
Ok(effect) => cmd.effect = Some(effect),
Err(_) => bail_parse!(
ctx,
arg.entry.span(),
"unsupported effect {raw}, expected one of: {EFFECT_VALUES}"
),
}
}
"restart_token" => {
cmd.restart_token = Some(child.ensure_arg_len(1..=1)?.arg(0)?.ensure_string()?)
}
Expand Down Expand Up @@ -574,6 +603,10 @@ impl From<&SpecCommand> for KdlNode {
node.entries_mut()
.push(string_entry(Some("deprecated"), deprecated));
}
if let Some(effect) = &cmd.effect {
node.entries_mut()
.push(string_entry(Some("effect"), effect.as_str()));
}
for flag in &cmd.flags {
let children = node.children_mut().get_or_insert_with(KdlDocument::new);
children.nodes_mut().push(flag.into());
Expand Down Expand Up @@ -644,3 +677,96 @@ impl From<clap::Command> for Spec {
(&cmd).into()
}
}

#[cfg(test)]
mod tests {
use crate::spec::effect::SpecCommandEffect;
use crate::Spec;
use insta::assert_snapshot;

#[test]
fn test_effect_prop_and_child_node() {
let spec = Spec::parse(
&Default::default(),
r#"
bin "mise"
cmd "ls" effect="read"
cmd "use" effect="write"
cmd "uninstall" {
effect "destructive"
}
cmd "version"
"#,
)
.unwrap();

let cmds = &spec.cmd.subcommands;
assert_eq!(cmds["ls"].effect, Some(SpecCommandEffect::Read));
assert_eq!(cmds["use"].effect, Some(SpecCommandEffect::Write));
assert_eq!(
cmds["uninstall"].effect,
Some(SpecCommandEffect::Destructive)
);
// Unspecified stays unknown rather than defaulting to anything.
assert_eq!(cmds["version"].effect, None);
}

#[test]
fn test_effect_is_not_inherited_by_subcommands() {
let spec = Spec::parse(
&Default::default(),
r#"
bin "git"
cmd "remote" effect="read" {
cmd "add" effect="write"
cmd "show"
}
"#,
)
.unwrap();

let remote = &spec.cmd.subcommands["remote"];
assert_eq!(remote.effect, Some(SpecCommandEffect::Read));
assert_eq!(
remote.subcommands["add"].effect,
Some(SpecCommandEffect::Write)
);
assert_eq!(remote.subcommands["show"].effect, None);
}

#[test]
fn test_effect_roundtrips_through_kdl() {
let spec = Spec::parse(
&Default::default(),
r#"
bin "mise"
cmd "ls" effect="read"
cmd "uninstall" effect="destructive"
"#,
)
.unwrap();

assert_snapshot!(spec, @r#"
name mise
bin mise
cmd ls effect=read
cmd uninstall effect=destructive
"#);
}

#[test]
fn test_unknown_effect_is_an_error() {
let err = Spec::parse(
&Default::default(),
r#"
bin "mise"
cmd "ls" effect="readonly"
"#,
)
.unwrap_err();
assert!(
err.to_string().contains("Invalid usage config"),
"unexpected error: {err}"
);
}
}
103 changes: 103 additions & 0 deletions lib/src/spec/effect.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
use std::fmt::{self, Display};

use serde::Serialize;
use strum::{Display as StrumDisplay, EnumString};

/// What running a command does to the world.
///
/// This is a coarse, three-way classification rather than a permission model.
/// It exists so a spec can distinguish "safe to run to find something out" from
/// "changes state" from "may destroy something", which is the distinction
/// consumers keep reinventing:
///
/// - documentation and `--help` can mark destructive commands
/// - a shell wrapper can require confirmation
/// - an AI coding agent can be given an allowlist of read-only commands rather
/// than asking about every invocation
///
/// It is deliberately not inherited by subcommands. `git remote` and
/// `git remote remove` do different things, and silently inheriting an effect
/// from a parent would make the strictest reading of a spec the wrong one.
#[derive(Debug, Copy, Clone, PartialEq, Eq, EnumString, StrumDisplay, Serialize)]
#[strum(serialize_all = "snake_case")]
#[serde(rename_all = "snake_case")]
pub enum SpecCommandEffect {
/// Only inspects state. Running it twice is the same as running it once,
/// and not running it changes nothing.
Read,
/// Creates or modifies state, but does not remove anything the user cannot
/// recreate by running another command.
Write,
/// May delete or irreversibly overwrite something. Deserves a confirmation
/// prompt.
Destructive,
}

impl SpecCommandEffect {
pub fn as_str(&self) -> &'static str {
match self {
Self::Read => "read",
Self::Write => "write",
Self::Destructive => "destructive",
}
}

/// Human-readable label used in generated documentation.
pub fn label(&self) -> &'static str {
match self {
Self::Read => "read-only",
Self::Write => "modifies state",
Self::Destructive => "destructive",
}
}
}

/// The set of values accepted by `effect=`, for error messages.
pub(crate) const EFFECT_VALUES: &str = "read, write, destructive";

#[derive(Debug)]
pub struct ParseEffectError(pub String);

impl Display for ParseEffectError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(
f,
"unknown effect {:?}, expected one of: {EFFECT_VALUES}",
self.0
)
}
Comment thread
cursor[bot] marked this conversation as resolved.
Outdated
}

#[cfg(test)]
mod tests {
use super::*;
use std::str::FromStr;

#[test]
fn test_parse() {
assert_eq!(
SpecCommandEffect::from_str("read").unwrap(),
SpecCommandEffect::Read
);
assert_eq!(
SpecCommandEffect::from_str("destructive").unwrap(),
SpecCommandEffect::Destructive
);
assert!(SpecCommandEffect::from_str("readonly").is_err());
}

#[test]
fn test_display_roundtrips() {
for effect in [
SpecCommandEffect::Read,
SpecCommandEffect::Write,
SpecCommandEffect::Destructive,
] {
assert_eq!(
SpecCommandEffect::from_str(&effect.to_string()).unwrap(),
effect
);
assert_eq!(effect.to_string(), effect.as_str());
}
}
}
1 change: 1 addition & 0 deletions lib/src/spec/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ pub mod complete;
pub mod config;
mod context;
pub mod data_types;
pub mod effect;
pub mod flag;
pub mod helpers;
pub mod mount;
Expand Down
Loading