From b80f0d4d602f6a19fe0dbcc93f43d9186c9d53c4 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Thu, 30 Jul 2026 13:06:31 -0700 Subject: [PATCH 1/3] feat(cli)!: tidy --help output and make search a real command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `--fields` now shows its native clap default (like `--dry-run`'s `[default: false]`) instead of a hardcoded fallback; the per-command `Output fields:` table moved from the command description into `--fields`'s own help text, with each default field marked `(default)`. - `--filter`/`--expr` carry contextual usage examples on their own flags instead of a disconnected "Filter examples:"/"Expr examples:" section. - `--json`/`--toon`/`--human` shorthand flags are hidden from `--help` (still fully functional) and documented on `--output` instead, whose displayed default now reflects the actual TTY-based choice rather than a hardcoded `json`. - `--dry-run` is hidden from `--help` on commands that don't mutate, where it's a no-op. - Command-specific flags render before global flags in `--help`, in declaration order, and global flags render in the engine's own declared order — fixes clap's colliding per-`Command` auto display-order counters, which previously interleaved the two. - BREAKING CHANGE: `--search` is now a real `search [--scope ]` command instead of a raw-argv pre-parse bypass flag that silently discarded whatever command it was attached to (`some-command --search x` used to return empty results instead of running `some-command` or erroring). Removed the now-dead `GlobalFlags.search`, `Middleware.search`, and the public `extract_search_query` function. Test plan: - cargo fmt --all --check - cargo clippy --all-targets -- -D warnings - RUSTDOCFLAGS='-D warnings' cargo doc --no-deps - cargo rustdoc --lib -- -W missing-docs (zero) - cargo test --all-targets - cargo test --doc - Verified against a real consumer CLI binary patched to this branch Co-Authored-By: Claude Sonnet 5 --- docs/argv0-dispatch.md | 4 +- docs/concepts.md | 5 +- docs/design.md | 5 +- src/cli.rs | 256 +++++++++++++----- src/cli/builtins.rs | 80 ++++++ src/cli/help.rs | 22 +- src/command.rs | 9 +- src/flags.rs | 118 +++++--- src/lib.rs | 4 +- src/middleware.rs | 2 - src/output/schema.rs | 47 ++-- tests/argv0_dispatch.rs | 2 +- tests/consumer_cli.rs | 2 +- tests/exhaustive_public_api.rs | 3 - tests/foundation.rs | 473 ++++++++++++++++++++++++++------- 15 files changed, 783 insertions(+), 249 deletions(-) diff --git a/docs/argv0-dispatch.md b/docs/argv0-dispatch.md index 4d3d200..fc4d6e9 100644 --- a/docs/argv0-dispatch.md +++ b/docs/argv0-dispatch.md @@ -61,9 +61,7 @@ my-cli argv0 pl --team platform # dispatch as if invoked as `pl` my-cli argv0 legacy-tool ... # dispatch the `legacy-tool` personality ``` -It is recognized as the first argument after the program name, is never registered with `clap`, and -so never appears in `--help`, `tree`, or `--search`. It is active only when the application has -registered at least one route. +It is recognized as the first argument after the program name, is never registered with `clap`, and so never appears in `--help`, `tree`, or the `search` command. It is active only when the application has registered at least one route. Unlike the silent symlink fall-through, an **explicit** `argv0` invocation is strict: an unknown name, or a bare `argv0` with no name, exits non-zero with an error listing the known names. This diff --git a/docs/concepts.md b/docs/concepts.md index cdca0ff..4c6e252 100644 --- a/docs/concepts.md +++ b/docs/concepts.md @@ -272,7 +272,6 @@ populate middleware: | `--reason` | `reason` | empty | Reason passed to authorization, audit, and activity. Only registered when `CliConfig` has an `authz`, `auditor`, or `activity` hook configured directly (not via `init_deps`, which runs after flag registration) — apps with none of those simply don't have the flag. | | `--timeout` | `timeout` | `0s` | Command deadline (e.g. `60s`, `5m`); default `0s` = no timeout. | | `--debug` | `debug` | empty | Enables debug components (comma-separated patterns). Bare `--debug` enables all; a specific value uses the `=` form: `--debug=transport`, `--debug='*,-auth'`. `transport` dumps HTTP requests/responses to stderr. See [HTTP debug logging](#http-debug-logging). | -| `--search` | `search` | empty | Searches command and guide documentation before command execution. | Applications can add additional global flags through `CliConfig::register_flags` and copy parsed values into middleware through `CliConfig::apply_flags`. @@ -659,9 +658,7 @@ The renderer only recognizes `* `-prefixed bullets (with 0–3 leading spaces fo ## Search -`--search` searches command metadata, aliases, guides, and extra registered search documents. Search -short-circuits normal command execution so users and agents can find help without satisfying command -flags. +`search [--scope ]` is a built-in command (alongside `help`, `guide`, `tree`, and `completion`) that searches command metadata, aliases, guides, and extra registered search documents (`CliConfig::with_extra_search_docs`). `` accepts multiple words without quoting (`app search deploy pipeline`); quoting still works the same way (`app search "deploy pipeline"`) since both forms are just joined with spaces. `--scope` limits results to one command subtree using the same colon-separated path form as elsewhere (`--scope domain` or `--scope domain:list`), resolving aliases the same way a real command path would; an unresolvable scope falls back to an unscoped (root) search rather than erroring. ## Transport diff --git a/docs/design.md b/docs/design.md index cce15f4..9da2fd3 100644 --- a/docs/design.md +++ b/docs/design.md @@ -250,7 +250,6 @@ Framework global flags populate middleware and apply consistently to every comma | `--reason` | Reason passed to authorization, audit, and activity. Only registered when `CliConfig` has an `authz`, `auditor`, or `activity` hook configured directly (not via `init_deps`, which runs after flag registration). | | `--timeout` | Command deadline (e.g. `60s`, `5m`); default is no timeout (`0s`). | | `--debug` | Debug selector for integrations that use it. | -| `--search` | Searches command and guide documentation before command execution. | | `--version`, `-v` | Prints version/build metadata. | Applications can add their own global flags with `CliConfig::with_register_flags` and copy parsed @@ -377,9 +376,7 @@ available for simple or dynamic cases. Guides are markdown documents registered globally or by module. They can come from filesystem paths, embedded `(path, bytes)` pairs, or explicit `GuideEntry` values. -`--search` indexes command metadata, aliases, guide content, and extra registered search documents. -Search bypasses normal command execution so users and agents can discover commands without -satisfying required command flags. +`search [--scope ]` is a built-in command that indexes command metadata, aliases, guide content, and extra registered search documents. `` accepts multiple words without quoting. `--scope` limits results to one command subtree (e.g. `--scope domain:list`). ## Transport diff --git a/src/cli.rs b/src/cli.rs index df55b63..f2a2755 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -28,21 +28,21 @@ use crate::{ feature_flags::{FlagEntry, FlagPolicy, FlagRegistry, Stage}, flags::{ GlobalFlags, derive_bool_flags, derive_value_flags, extract_command_path, - extract_output_format, extract_search_query, global_flags_from_matches, - has_true_schema_flag, min_stage_env_var, output_env_var, register_global_flags, - register_reason_flag, resolve_default_output_format, + extract_output_format, global_flags_from_matches, has_true_schema_flag, min_stage_env_var, + output_env_var, register_global_flags, register_reason_flag, resolve_default_output_format, }, guide::{guide_content, render_guide_human}, module::{Module, ModuleContext}, output::{ - HumanViewDef, HumanViewRegistry, NextAction, SchemaRegistry, format_help_section, - global_human_view_registry_snapshot, global_schema_registry_snapshot, + FieldInfo, HumanViewDef, HumanViewRegistry, NextAction, SchemaRegistry, + format_help_section, global_human_view_registry_snapshot, global_schema_registry_snapshot, }, search::{SearchDocument, SearchIndex}, }; use builtins::{ completion_args, completion_command, guide_args, guide_command, help_args, help_command, + search_args, search_command, }; use help::{GROUP_HELP_TEMPLATE, ROOT_HELP_TEMPLATE}; pub use help::{ModuleHelpEntry, build_root_long, render_next_actions_human}; @@ -110,7 +110,7 @@ pub type PreRun = pub type ResolveMeta = Arc CommandMeta + Send + Sync>; /// Hook called after a CLI run completes. pub type OnShutdown = Arc; -/// Hook that contributes extra root-scope `--search` documents. +/// Hook that contributes extra root-scope `search` documents. pub type ExtraSearchDocs = Arc Vec + Send + Sync>; /// Hook that supplies the suggested next actions shown when the CLI is invoked /// with no subcommand (bare root). The same actions drive a human "Next actions" @@ -189,7 +189,8 @@ pub enum Argv0LinkMethod { /// Top-level subcommand names that are reserved by the engine and must not be /// used as module group names. [`Cli::add_module_group`] rejects a group whose /// name matches a reserved name so the engine's built-in command always wins. -pub(crate) const BUILTIN_COMMAND_NAMES: [&str; 4] = ["help", "guide", "tree", "completion"]; +pub(crate) const BUILTIN_COMMAND_NAMES: [&str; 5] = + ["help", "guide", "tree", "completion", "search"]; /// Declarative configuration for a CLI application. /// @@ -894,7 +895,8 @@ impl Cli { .subcommand(help_command()) .subcommand(guide_command()) .subcommand(Command::new("tree").about("Display full command tree")) - .subcommand(completion_command()); + .subcommand(completion_command()) + .subcommand(search_command()); if let Some(register_flags) = &config.register_flags { root = register_flags(root); } @@ -1309,7 +1311,7 @@ impl Cli { // The hidden `argv0` meta-command (` argv0 [args...]`) forces // a route without an actual symlink. It is recognized positionally as the // first argument after the program name and is never registered with clap, - // so it stays absent from `--help`, `tree`, and `--search`. + // so it stays absent from `--help`, `tree`, and the `search` command. let explicit = text_args.get(1).map(String::as_str) == Some("argv0"); let (name, rest) = if explicit { match text_args.get(2) { @@ -1550,9 +1552,6 @@ impl Cli { if let Some(output) = self.try_run_schema_bypass(&text_args) { return output; } - if let Some(output) = self.try_run_search_bypass(&text_args) { - return output; - } // Resolve the positional command path once and share it between the // group-help rewrite and the unknown-command check below. let bool_flags = derive_bool_flags(&self.root); @@ -1663,6 +1662,22 @@ impl Cli { } return self.finish_run(self.render_guide(&matches, &flags.output_format)); } + if command_path == "search" { + let args = search_args(&matches); + if let Err(err) = self.run_pre_run(&mut middleware, &command_path, &args) { + return self.finish_run(render_cli_error(&middleware, &err, &self.config.app_id)); + } + let query = args + .get("query") + .and_then(|v| v.as_str()) + .unwrap_or_default(); + let scope_path = args + .get("scope") + .and_then(|v| v.as_str()) + .unwrap_or_default(); + let scope = self.resolve_search_scope(scope_path); + return self.finish_run(self.render_search(query, &scope, &flags.output_format)); + } if command_path == "completion" { let args = completion_args(&matches); if let Err(err) = self.run_pre_run(&mut middleware, &command_path, &args) { @@ -1853,16 +1868,6 @@ impl Cli { } } - fn try_run_search_bypass(&self, args: &[String]) -> Option { - let query = extract_search_query(args); - if query.is_empty() { - return None; - } - let scope = self.search_scope(args); - let output_format = extract_output_format(args, &self.resolve_run_output_format()); - Some(self.render_search(&query, &scope, &output_format)) - } - fn try_run_schema_bypass(&self, args: &[String]) -> Option { if !has_true_schema_flag(args) { return None; @@ -2033,8 +2038,18 @@ impl Cli { docs } - fn search_scope(&self, args: &[String]) -> String { - let parts = extract_search_scope_parts(args); + /// Resolves `--scope`'s colon-separated path (e.g. `domain` or + /// `domain:list`) to the canonical scope string [`Self::search_documents`] + /// expects, matching aliases the same way a real command path would (via + /// [`canonical_path_from_parts`]'s `find_subcommand` walk). An empty or + /// unresolvable scope falls back to an unscoped (root) search rather than + /// erroring — `search` staying permissive here matches how a typo in a + /// search *query* just yields fewer results instead of a hard failure. + fn resolve_search_scope(&self, scope_path: &str) -> String { + if scope_path.is_empty() { + return String::new(); + } + let parts: Vec = scope_path.split(':').map(str::to_owned).collect(); canonical_path_from_parts(&self.root, &parts).unwrap_or_default() } @@ -2446,7 +2461,6 @@ fn apply_global_flags(middleware: &mut Middleware, flags: &GlobalFlags, timeout: middleware.schema = flags.schema; middleware.timeout = timeout; middleware.debug = flags.debug.clone(); - middleware.search = flags.search.clone(); } /// Builds the transport debug logger implied by a parsed `--debug` pattern, @@ -2675,28 +2689,6 @@ fn canonical_path_from_parts(root: &Command, parts: &[String]) -> Option Some(canonical.join(":")) } -fn extract_search_scope_parts(args: &[String]) -> Vec { - let mut parts = Vec::new(); - let mut index = 1; - while index < args.len() { - let arg = &args[index]; - if arg == "--search" || arg.starts_with("--search=") { - break; - } - if arg.starts_with('-') { - if !arg.contains('=') && index + 1 < args.len() && !args[index + 1].starts_with('-') { - index += 2; - } else { - index += 1; - } - continue; - } - parts.push(arg.clone()); - index += 1; - } - parts -} - fn collect_command_search_documents( command: &Command, prefix: &mut Vec, @@ -2763,6 +2755,7 @@ fn append_command_alias_terms(command: &Command, aliases: &mut Vec) { fn command_flag_text(command: &Command) -> String { command .get_arguments() + .filter(|arg| !arg.is_hide_set()) .filter_map(|arg| { let mut names = Vec::new(); if let Some(short) = arg.get_short() { @@ -3384,26 +3377,163 @@ fn command_clap_command_with_schema_help( schemas: &SchemaRegistry, ) -> Command { let mut command = spec.clap_command(); - let Some(schema) = schemas.get_by_path(command_path) else { + command = apply_dry_run_visibility(command, spec); + let schema = schemas.get_by_path(command_path); + let default_fields = default_field_names(spec); + command = apply_fields_arg( + command, + spec, + schema.as_ref().map(|schema| schema.fields.as_slice()), + &default_fields, + ); + let Some(schema) = schema else { return command; }; - let schema_help = format_help_section(&schema.fields); - if schema_help.is_empty() { + apply_filter_and_expr_examples(command, &schema.fields) +} + +/// Hides this command's inherited `--dry-run` flag when the command isn't +/// mutating (per [`CommandSpec::metadata`]'s `dry_run_prompt` — mirrored +/// here rather than reused, since that method returns the broader +/// [`CommandMeta`], not this one bool). `--dry-run` only ever does anything +/// for a command that opted in via `.mutates(true)`/`.with_tier(...)` (see +/// `Middleware::render_envelope`'s `meta.dry_run_prompt` gate), so showing +/// it on every other command is noise. The override still parses `--dry-run` +/// identically (same value parser, same defaults) in case a caller passes +/// it anyway — hidden only changes what `--help` shows, never behavior. +fn apply_dry_run_visibility(command: Command, spec: &CommandSpec) -> Command { + let mutates = spec.mutates || spec.tier.is_some_and(crate::Tier::is_mutating); + if mutates { return command; } - let base = spec - .long - .as_ref() - .filter(|long| !long.is_empty()) - .cloned() - .unwrap_or_else(|| spec.short.clone()); - let long = if base.is_empty() { - schema_help - } else { - format!("{base}\n\n{schema_help}") - }; - command = command.long_about(long); - command + command.arg( + clap::Arg::new("dry-run") + .long("dry-run") + .num_args(0..=1) + .require_equals(true) + .default_missing_value("true") + .default_value("false") + .value_parser(crate::flags::compat_bool_value_parser()) + .display_order(crate::flags::global_flag_order::DRY_RUN) + .hide(true) + .help("Preview mutations without executing"), + ) +} + +/// Splits a command's raw `default_fields` string into individual field +/// names, dropping the `all`/`*` sentinels that mean "every field" rather +/// than naming a real field. +fn default_field_names(spec: &CommandSpec) -> Vec<&str> { + spec.default_fields + .as_deref() + .map(|fields| { + fields + .split(',') + .map(str::trim) + .filter(|field| !field.is_empty() && *field != "all" && *field != "*") + .collect() + }) + .unwrap_or_default() +} + +/// Overrides this command's `--fields` flag with everything specific to this +/// command: its own `default_fields` as a native clap default value (so +/// `--help` shows `[default: ...]` on the flag itself, the same way +/// `--dry-run` shows `[default: false]`), and, when a schema is registered, +/// the output-field summary table appended to the flag's own help text +/// instead of the command's description — a long field table there used to +/// push `Usage:` far down the page. Global args apply to every subcommand, +/// but a subcommand-local arg of the same name takes precedence, so this +/// only affects the one command being built here. +fn apply_fields_arg( + command: Command, + spec: &CommandSpec, + schema_fields: Option<&[FieldInfo]>, + default_fields: &[&str], +) -> Command { + let default_value = spec + .default_fields + .as_deref() + .filter(|fields| !fields.is_empty()); + let table = schema_fields + .filter(|fields| !fields.is_empty()) + .map(|fields| format_help_section(fields, default_fields)); + if default_value.is_none() && table.is_none() { + return command; + } + + let mut help = String::from( + "Comma-separated fields to include in output (use 'all' or '*' for everything)", + ); + if let Some(table) = &table { + help.push_str("\n\n"); + help.push_str(table.trim_end()); + } + + let mut arg = clap::Arg::new("fields") + .long("fields") + .value_name("FIELDS") + // Must match `global_flag_order::FIELDS` — this re-registers the + // same flag with contextual help, not a new one, and needs to keep + // its place among the other global flags rather than falling back + // to this subcommand's own low, command-specific counter value. + .display_order(crate::flags::global_flag_order::FIELDS) + .help(help); + if let Some(default_value) = default_value { + arg = arg.default_value(default_value.to_owned()); + } + command.arg(arg) +} + +/// Overrides this command's `--filter` and `--expr` flags with help text +/// carrying usage examples built from its own output fields, so `--help` +/// shows them right under the flag instead of in a separate "Filter +/// examples:"/"Expr examples:" section disconnected from the flags they +/// demonstrate. Mirrors [`apply_fields_arg`]: a subcommand-local arg of the +/// same name shadows the framework's global one, and must carry the same +/// `global_flag_order` value as that global one for the same reason. +fn apply_filter_and_expr_examples(mut command: Command, fields: &[FieldInfo]) -> Command { + if fields.is_empty() { + return command; + } + let first_string = fields + .iter() + .find(|field| field.field_type == "string") + .map(|field| field.name.as_str()); + let first_bool = fields + .iter() + .find(|field| field.field_type == "bool") + .map(|field| field.name.as_str()); + + if first_string.is_some() || first_bool.is_some() { + let mut help = String::from("Per-item JMESPath predicate for list data"); + if let Some(name) = first_string { + help.push_str(&format!("\ne.g. --filter \"contains({name}, 'example')\"")); + } + if let Some(name) = first_bool { + help.push_str(&format!("\ne.g. --filter '{name}'")); + } + command = command.arg( + clap::Arg::new("filter") + .long("filter") + .value_name("EXPR") + .display_order(crate::flags::global_flag_order::FILTER) + .help(help), + ); + } + + let mut expr_help = String::from("JMESPath query applied to the whole result"); + expr_help.push_str("\ne.g. --expr 'length(@)'"); + if let Some(name) = first_string { + expr_help.push_str(&format!("\ne.g. --expr '[].{name}'")); + } + command.arg( + clap::Arg::new("expr") + .long("expr") + .value_name("EXPR") + .display_order(crate::flags::global_flag_order::EXPR) + .help(expr_help), + ) } fn process_exit_code(code: i32) -> ExitCode { diff --git a/src/cli/builtins.rs b/src/cli/builtins.rs index 0c261bd..9c2ea18 100644 --- a/src/cli/builtins.rs +++ b/src/cli/builtins.rs @@ -62,6 +62,38 @@ pub(crate) fn completion_args(matches: &ArgMatches) -> ValueMap { map } +pub(crate) fn search_command() -> Command { + Command::new("search") + .about("Search commands and guides by keyword") + .long_about("Searches command names, descriptions, aliases, and guide content for the given keyword(s). Narrow results to part of the command tree with --scope, e.g. --scope domain or --scope domain:list.") + .arg( + Arg::new("query") + .value_name("QUERY") + .num_args(1..) + .required(true) + .help("Keyword(s) to search for"), + ) + .arg( + Arg::new("scope") + .long("scope") + .value_name("PATH") + .help("Limit results to one command subtree, e.g. domain or domain:list"), + ) +} + +pub(crate) fn search_args(matches: &ArgMatches) -> ValueMap { + let leaf = leaf_matches(matches); + let query = leaf + .get_many::("query") + .map(|values| values.map(String::as_str).collect::>().join(" ")) + .unwrap_or_default(); + let mut map = value_map([("query", Value::String(query))]); + if let Some(scope) = leaf.get_one::("scope") { + map.insert("scope".to_owned(), Value::String(scope.clone())); + } + map +} + #[cfg(test)] #[allow(clippy::unwrap_used)] mod tests { @@ -96,4 +128,52 @@ mod tests { .is_err() ); } + + #[test] + fn search_args_joins_multi_word_query_with_spaces() { + let m = search_command() + .try_get_matches_from(["search", "deploy", "pipeline"]) + .unwrap(); + assert_eq!( + search_args(&m).get("query"), + Some(&Value::String("deploy pipeline".to_owned())) + ); + } + + #[test] + fn search_args_leaves_a_single_quoted_token_unchanged() { + let m = search_command() + .try_get_matches_from(["search", "deploy pipeline"]) + .unwrap(); + assert_eq!( + search_args(&m).get("query"), + Some(&Value::String("deploy pipeline".to_owned())) + ); + } + + #[test] + fn search_args_parses_scope() { + let m = search_command() + .try_get_matches_from(["search", "foo", "--scope", "domain:list"]) + .unwrap(); + let args = search_args(&m); + assert_eq!(args.get("query"), Some(&Value::String("foo".to_owned()))); + assert_eq!( + args.get("scope"), + Some(&Value::String("domain:list".to_owned())) + ); + } + + #[test] + fn search_args_omits_scope_when_absent() { + let m = search_command() + .try_get_matches_from(["search", "foo"]) + .unwrap(); + assert_eq!(search_args(&m).get("scope"), None); + } + + #[test] + fn search_command_requires_a_query() { + assert!(search_command().try_get_matches_from(["search"]).is_err()); + } } diff --git a/src/cli/help.rs b/src/cli/help.rs index b71f508..f7e1791 100644 --- a/src/cli/help.rs +++ b/src/cli/help.rs @@ -63,10 +63,26 @@ pub fn build_root_long(intro: &str, entries: &[ModuleHelpEntry], has_guide: bool } } out.push_str("\n\n Find Commands:"); - out.push_str("\n --search Search all commands and guides by keyword"); - out.push_str("\n tree Display full command tree"); + let mut find_commands: Vec<(&str, &str)> = vec![ + ( + "search ", + "Search all commands and guides by keyword", + ), + ("tree", "Display full command tree"), + ]; if has_guide { - out.push_str("\n guide Built-in guides for AI agents and developers"); + find_commands.push(("guide", "Built-in guides for AI agents and developers")); + } + let find_commands_width = find_commands + .iter() + .map(|(label, _)| label.len()) + .max() + .unwrap_or_default(); + for (label, description) in find_commands { + out.push_str(&format!( + "\n {:, } @@ -54,12 +52,51 @@ impl Default for GlobalFlags { reason: String::new(), timeout: "0s".to_owned(), debug: String::new(), - search: String::new(), credential_store: None, } } } +/// Explicit `--help` display-order values for the engine's own global flags, +/// numbered in the order they're registered below — which is meant to read +/// as their relative importance, most-used first. +/// +/// Without this, every global flag would collide with command-specific +/// ones: clap auto-assigns each unset `display_order` as "the Nth argument +/// added to this `Command`," starting the count over at 0 on every +/// `Command` it's called on — the root (where these are declared) and each +/// subcommand alike. A subcommand's own `CommandSpec::with_arg` args get +/// low counter values (0, 1, 2, ... in declaration order) from their own +/// `Command`; a global flag propagated onto that subcommand keeps the low +/// counter value it got on the *root*. Mix the two and `--help` interleaves +/// them instead of showing command-specific flags first, as a block, in the +/// order they were declared. Parking every global flag comfortably above +/// any realistic per-command arg count keeps that from happening. +/// +/// `FIELDS`, `FILTER`, and `EXPR` are `pub(crate)` because `cli.rs` +/// re-registers those three per-command (see `apply_fields_arg` and +/// `apply_filter_and_expr_examples`) with contextual help text; they must +/// reuse these same values or the override would drift out of position. +pub(crate) mod global_flag_order { + pub(crate) const HELP: usize = 1000; + pub(crate) const OUTPUT: usize = 1001; + pub(crate) const VERBOSE: usize = 1002; + pub(crate) const DRY_RUN: usize = 1003; + pub(crate) const FIELDS: usize = 1004; + pub(crate) const FILTER: usize = 1005; + pub(crate) const EXPR: usize = 1006; + pub(crate) const LIMIT: usize = 1007; + pub(crate) const OFFSET: usize = 1008; + pub(crate) const SCHEMA: usize = 1009; + pub(crate) const TIMEOUT: usize = 1010; + pub(crate) const DEBUG: usize = 1011; + pub(crate) const CREDENTIAL_STORE: usize = 1013; + pub(crate) const JSON: usize = 1014; + pub(crate) const TOON: usize = 1015; + pub(crate) const HUMAN: usize = 1016; + pub(crate) const REASON: usize = 1017; +} + /// Registers framework-global flags on a `clap` command. pub fn register_global_flags(command: Command) -> Command { command @@ -74,6 +111,7 @@ pub fn register_global_flags(command: Command) -> Command { .long("help") .action(ArgAction::HelpLong) .global(true) + .display_order(global_flag_order::HELP) .help("Print help"), ) .arg( @@ -81,13 +119,33 @@ pub fn register_global_flags(command: Command) -> Command { .long("output") .short('o') .global(true) + .display_order(global_flag_order::OUTPUT) .value_name("FORMAT") - .default_value("json") + // This default is cosmetic, not authoritative: it's never + // actually read as a value — `global_flags_from_matches` only + // consults this arg when it was given on the command line, + // falling back to `resolve_default_output_format`'s full + // env/config/TTY precedence the rest of the time. But since + // `--help` runs in this same process, this process's own + // stdout TTY-ness is already known and stable for the whole + // run, so mirroring that one signal here (skipping the + // env-var/config-file tiers, which aren't available until a + // command actually executes) keeps what `--help` shows honest + // in the common case instead of a hardcoded, often-wrong + // `[default: json]`. + .default_value(if std::io::stdout().is_terminal() { + "human" + } else { + "json" + }) // Only conflicts when *explicitly* given: clap's conflict // checks ignore an arg's default value, so a bare `--json` // with no `--output` at all is unaffected. .conflicts_with_all(["json", "toon", "human"]) - .help("Output format: toon|json|human"), + .help( + "Output format: toon|json|human (shorthand: --json, --toon, --human); \ + defaults to human in an interactive terminal, json otherwise", + ), ) .arg( Arg::new("verbose") @@ -96,6 +154,7 @@ pub fn register_global_flags(command: Command) -> Command { .num_args(0..=1) .default_missing_value("all") .value_name("FIELDS") + .display_order(global_flag_order::VERBOSE) .help("Include metadata in output (all, or comma-separated: system,duration,args,env,identity,command,effective_args,timestamp)"), ) .arg( @@ -107,6 +166,7 @@ pub fn register_global_flags(command: Command) -> Command { .default_missing_value("true") .default_value("false") .value_parser(compat_bool_value_parser()) + .display_order(global_flag_order::DRY_RUN) .help("Preview mutations without executing"), ) .arg( @@ -114,6 +174,7 @@ pub fn register_global_flags(command: Command) -> Command { .long("fields") .global(true) .value_name("FIELDS") + .display_order(global_flag_order::FIELDS) .help("Comma-separated fields to include in output (use 'all' or '*' for everything)"), ) .arg( @@ -121,6 +182,7 @@ pub fn register_global_flags(command: Command) -> Command { .long("filter") .global(true) .value_name("EXPR") + .display_order(global_flag_order::FILTER) .help("Per-item JMESPath predicate for list data"), ) .arg( @@ -128,6 +190,7 @@ pub fn register_global_flags(command: Command) -> Command { .long("expr") .global(true) .value_name("EXPR") + .display_order(global_flag_order::EXPR) .help("JMESPath query applied to the whole result"), ) .arg( @@ -137,6 +200,7 @@ pub fn register_global_flags(command: Command) -> Command { .value_parser(value_parser!(i64)) .allow_hyphen_values(true) .default_value("0") + .display_order(global_flag_order::LIMIT) .help("Max items to return (client-side, 0=all)"), ) .arg( @@ -146,6 +210,7 @@ pub fn register_global_flags(command: Command) -> Command { .value_parser(value_parser!(i64)) .allow_hyphen_values(true) .default_value("0") + .display_order(global_flag_order::OFFSET) .help("Skip N items before applying limit"), ) .arg( @@ -157,6 +222,7 @@ pub fn register_global_flags(command: Command) -> Command { .default_missing_value("true") .default_value("false") .value_parser(compat_bool_value_parser()) + .display_order(global_flag_order::SCHEMA) .help("Dump output field metadata instead of running the command"), ) .arg( @@ -166,6 +232,7 @@ pub fn register_global_flags(command: Command) -> Command { .allow_hyphen_values(true) .default_value("0s") .value_name("DURATION") + .display_order(global_flag_order::TIMEOUT) .help("Overall command timeout (e.g. 60s, 5m); default 0s = no timeout"), ) .arg( @@ -175,18 +242,13 @@ pub fn register_global_flags(command: Command) -> Command { .num_args(0..=1) .default_missing_value("*") .value_name("PATTERN") + .display_order(global_flag_order::DEBUG) .help("Enable debug logging (comma-separated component patterns, e.g. *, transport, *,-auth)"), ) - .arg( - Arg::new("search") - .long("search") - .global(true) - .value_name("KEYWORD") - .help("Search commands and guides by keyword"), - ) .arg( Arg::new("credential-store") .long("credential-store") + .display_order(global_flag_order::CREDENTIAL_STORE) .global(true) .value_name("MODE") .value_parser(|s: &str| s.parse::()) @@ -201,6 +263,10 @@ pub fn register_global_flags(command: Command) -> Command { // e.g. `--json --human` together is a usage error rather // than one silently overriding the other. .conflicts_with_all(["toon", "human"]) + // Documented on `--output` instead of taking their own line + // in every command's already-long options list. + .hide(true) + .display_order(global_flag_order::JSON) .help("Shorthand for --output json"), ) .arg( @@ -209,6 +275,8 @@ pub fn register_global_flags(command: Command) -> Command { .global(true) .action(ArgAction::SetTrue) .conflicts_with_all(["json", "human"]) + .hide(true) + .display_order(global_flag_order::TOON) .help("Shorthand for --output toon"), ) .arg( @@ -217,6 +285,8 @@ pub fn register_global_flags(command: Command) -> Command { .global(true) .action(ArgAction::SetTrue) .conflicts_with_all(["json", "toon"]) + .hide(true) + .display_order(global_flag_order::HUMAN) .help("Shorthand for --output human"), ) } @@ -239,6 +309,7 @@ pub fn register_reason_flag(command: Command) -> Command { .long("reason") .global(true) .value_name("TEXT") + .display_order(global_flag_order::REASON) .help("Short explanation of why this command is being run (forwarded to your authorizer, auditor, or activity emitter)"), ) } @@ -384,33 +455,12 @@ pub fn global_flags_from_matches(matches: &ArgMatches, default_format: &str) -> .get_one::("debug") .cloned() .unwrap_or_default(), - search: matches - .get_one::("search") - .cloned() - .unwrap_or_default(), credential_store: matches .get_one::("credential-store") .copied(), } } -#[must_use] -/// Extracts `--search` from raw args before normal parsing. -pub fn extract_search_query(args: &[impl AsRef]) -> String { - for index in 0..args.len() { - let arg = args[index].as_ref(); - if arg == "--search" { - return args - .get(index + 1) - .map_or_else(String::new, |value| value.as_ref().to_owned()); - } - if let Some(value) = arg.strip_prefix("--search=") { - return value.to_owned(); - } - } - String::new() -} - #[must_use] /// Extracts output format from raw args. /// @@ -492,7 +542,7 @@ pub fn has_true_schema_flag(args: &[impl AsRef]) -> bool { false } -fn compat_bool_value_parser() -> ValueParser { +pub(crate) fn compat_bool_value_parser() -> ValueParser { ValueParser::new(parse_compat_bool) } diff --git a/src/lib.rs b/src/lib.rs index a56ce22..8d477a7 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -136,8 +136,8 @@ pub use feature_flags::{FeatureFlag, FlagEntry, FlagPolicy, FlagRegistry, Stage} pub use flags::{ GlobalFlags, app_id_env_prefix, debug_component_enabled, default_output_format, derive_bool_flags, derive_value_flags, extract_command_path, extract_output_format, - extract_search_query, global_flags_from_matches, has_true_schema_flag, min_stage_env_var, - output_env_var, register_global_flags, register_reason_flag, resolve_default_output_format, + global_flags_from_matches, has_true_schema_flag, min_stage_env_var, output_env_var, + register_global_flags, register_reason_flag, resolve_default_output_format, }; pub use guide::{GuideEntry, parse_guides, parse_guides_from_markdown}; pub use middleware::{ diff --git a/src/middleware.rs b/src/middleware.rs index e41ac08..bb30c68 100644 --- a/src/middleware.rs +++ b/src/middleware.rs @@ -505,8 +505,6 @@ pub struct Middleware { pub timeout: Option, /// Debug selector, interpreted by applications. pub debug: String, - /// Search query, interpreted before command execution. - pub search: String, /// Output schema registry. pub schema_registry: SchemaRegistry, /// Human output view registry. diff --git a/src/output/schema.rs b/src/output/schema.rs index 505bea8..155eb4d 100644 --- a/src/output/schema.rs +++ b/src/output/schema.rs @@ -476,8 +476,21 @@ pub fn global_schema_registry_snapshot() -> SchemaRegistry { } /// Formats compact field summaries for command long help. +/// +/// `default_fields` lists the field names shown when `--fields` is absent +/// (see [`CommandSpec::with_default_fields`](crate::CommandSpec::with_default_fields)); +/// each one is marked `(default)` in the listing. The command's `--fields` +/// flag itself also carries this as a native clap `[default: ...]` (see +/// `command_clap_command_with_schema_help` in `cli.rs`), so this marker is a +/// per-field cross-reference rather than the only place the default appears. +/// Pass an empty slice when the command has no default projection. +/// +/// `--filter`/`--expr` usage examples used to live in a standalone section +/// appended here; they now live on the `--filter` and `--expr` flags +/// themselves (see `apply_filter_and_expr_examples` in `cli.rs`), the same +/// way `--fields` carries its default natively instead of in prose. #[must_use] -pub fn format_help_section(fields: &[FieldInfo]) -> String { +pub fn format_help_section(fields: &[FieldInfo], default_fields: &[&str]) -> String { if fields.is_empty() { return String::new(); } @@ -489,37 +502,19 @@ pub fn format_help_section(fields: &[FieldInfo]) -> String { let mut out = String::from("Output fields:\n"); for field in fields { let optional = if field.optional { " (optional)" } else { "" }; + let default_marker = if default_fields.contains(&field.name.as_str()) { + " (default)" + } else { + "" + }; out.push_str(&format!( - " {:")); } #[tokio::test] @@ -451,11 +450,11 @@ async fn cli_runtime_root_help_includes_find_commands_without_modules() { assert_eq!(output.exit_code, 0); assert!(output.rendered.contains("Developer tooling")); assert!(output.rendered.contains("Find Commands")); - assert!(output.rendered.contains("--search ")); + assert!(output.rendered.contains("search ")); assert!( output .rendered - .contains("tree Display full command tree") + .contains("tree Display full command tree") ); } @@ -1712,6 +1711,135 @@ async fn command_spec_output_schema_registers_schema_and_help_when_mounted() { assert!(help.rendered.contains("count int (optional)")); } +#[tokio::test] +async fn command_help_marks_default_fields_among_output_fields() { + #[derive(Debug)] + struct DefaultedThing; + + impl OutputSchema for DefaultedThing { + fn fields() -> &'static [OutputField] { + &[ + OutputField { + name: "id", + field_type: "string", + optional: false, + }, + OutputField { + name: "name", + field_type: "string", + optional: false, + }, + OutputField { + name: "internal_note", + field_type: "string", + optional: true, + }, + ] + } + } + + let mut cli = Cli::new(CliConfig { + name: "my-cli".to_owned(), + short: "Developer tooling".to_owned(), + app_id: "my-cli".to_owned(), + ..CliConfig::default() + }); + cli.add_module_group( + "Platform Systems", + RuntimeGroupSpec::new(GroupSpec::new( + "defaulted-things", + "Manage defaulted things", + )) + .with_command(RuntimeCommandSpec::new( + CommandSpec::new("list", "List defaulted things") + .no_auth(true) + .with_default_fields("id,name") + .with_output_schema::(), + async |_credential, _args| { + Ok(CommandResult::new(json!([ + {"id": "1", "name": "alpha", "internal_note": "x"} + ]))) + }, + )), + ); + + let help = cli + .run(["my-cli", "help", "defaulted-things", "list"]) + .await; + assert_eq!(help.exit_code, 0); + + let normalized: Vec = help + .rendered + .lines() + .map(|line| line.split_whitespace().collect::>().join(" ")) + .collect(); + assert!(normalized.contains(&"id string (default)".to_owned())); + assert!(normalized.contains(&"name string (default)".to_owned())); + assert!(normalized.contains(&"internal_note string (optional)".to_owned())); + // The `--fields` flag itself carries the same default natively, exactly + // like `--dry-run` shows `[default: false]`. + assert!(help.rendered.contains("--fields ")); + assert!(help.rendered.contains("[default: id,name]")); +} + +#[tokio::test] +async fn command_help_attaches_filter_and_expr_examples_to_their_own_flags() { + #[derive(Debug)] + struct Thing; + + impl OutputSchema for Thing { + fn fields() -> &'static [OutputField] { + &[ + OutputField { + name: "name", + field_type: "string", + optional: false, + }, + OutputField { + name: "enabled", + field_type: "bool", + optional: false, + }, + ] + } + } + + let mut cli = Cli::new(CliConfig { + name: "my-cli".to_owned(), + short: "Developer tooling".to_owned(), + app_id: "my-cli".to_owned(), + ..CliConfig::default() + }); + cli.add_module_group( + "Platform Systems", + RuntimeGroupSpec::new(GroupSpec::new("things", "Manage things")).with_command( + RuntimeCommandSpec::new( + CommandSpec::new("list", "List things") + .no_auth(true) + .with_output_schema::(), + async |_credential, _args| Ok(CommandResult::new(json!([]))), + ), + ), + ); + + let help = cli.run(["my-cli", "help", "things", "list"]).await; + assert_eq!(help.exit_code, 0); + + // Examples now live on the flags they demonstrate, not in a standalone + // "Filter examples:"/"Expr examples:" section. + assert!(!help.rendered.contains("Filter examples:")); + assert!(!help.rendered.contains("Expr examples:")); + assert!(help.rendered.contains("--filter ")); + assert!( + help.rendered + .contains("e.g. --filter \"contains(name, 'example')\"") + ); + assert!(help.rendered.contains("e.g. --filter 'enabled'")); + assert!(help.rendered.contains("--expr ")); + assert!(help.rendered.contains("e.g. --expr 'length(@)'")); + assert!(help.rendered.contains("e.g. --expr '[].name'")); +} + #[tokio::test] async fn command_spec_can_publish_rust_native_json_schema_with_field_summary() { #[derive(Debug, serde::Serialize, JsonSchema)] @@ -1779,6 +1907,178 @@ async fn command_spec_can_publish_rust_native_json_schema_with_field_summary() { ); } +#[tokio::test] +async fn output_format_shorthand_flags_are_hidden_from_help_but_still_parse() { + let mut cli = Cli::new(CliConfig { + name: "my-cli".to_owned(), + short: "Developer tooling".to_owned(), + app_id: "my-cli".to_owned(), + ..CliConfig::default() + }); + cli.add_command(RuntimeCommandSpec::new( + CommandSpec::new("things", "List things").no_auth(true), + async |_credential, _args| Ok(CommandResult::new(json!([{"name": "alpha"}]))), + )); + + let help = cli.run(["my-cli", "things", "--help"]).await; + assert_eq!(help.exit_code, 0); + // `--json`/`--toon`/`--human` no longer take their own line; they're + // folded into `--output`'s help text instead. Their old standalone help + // text is the unambiguous signal that a row would have rendered. + assert!(!help.rendered.contains("Shorthand for --output json")); + assert!(!help.rendered.contains("Shorthand for --output toon")); + assert!(!help.rendered.contains("Shorthand for --output human")); + assert!(help.rendered.contains("--output ")); + assert!(help.rendered.contains("shorthand: --json, --toon, --human")); + + // Still fully functional despite being hidden from help. + let output = cli.run(["my-cli", "things", "--json"]).await; + assert_eq!(output.exit_code, 0); + assert_eq!( + serde_json::from_str::(&output.rendered).expect("valid json")["data"], + json!([{"name": "alpha"}]) + ); +} + +#[tokio::test] +async fn output_flag_help_default_reflects_actual_tty_based_default() { + use std::io::IsTerminal; + + let mut cli = Cli::new(CliConfig { + name: "my-cli".to_owned(), + short: "Developer tooling".to_owned(), + app_id: "my-cli".to_owned(), + ..CliConfig::default() + }); + cli.add_command(RuntimeCommandSpec::new( + CommandSpec::new("things", "List things").no_auth(true), + async |_credential, _args| Ok(CommandResult::new(json!([{"name": "alpha"}]))), + )); + + let help = cli.run(["my-cli", "things", "--help"]).await; + assert_eq!(help.exit_code, 0); + assert!( + help.rendered + .contains("defaults to human in an interactive terminal, json otherwise") + ); + + // The hint clap prints must match what this process would actually get: + // both are driven by the same `stdout().is_terminal()` check, one at CLI + // construction time and one when a command actually runs (see the same + // TTY guard pattern used elsewhere in this suite, e.g. the guide-render + // test above — under `--nocapture` on a real terminal this flips to + // `[default: human]`, which is the point). + if std::io::stdout().is_terminal() { + assert!(help.rendered.contains("[default: human]")); + } else { + assert!(help.rendered.contains("[default: json]")); + } +} + +#[tokio::test] +async fn command_help_lists_command_specific_flags_before_global_ones_in_declared_order() { + let mut cli = Cli::new(CliConfig { + name: "my-cli".to_owned(), + short: "Developer tooling".to_owned(), + app_id: "my-cli".to_owned(), + ..CliConfig::default() + }); + cli.add_command(RuntimeCommandSpec::new( + // Declared out of alphabetical order (`zeta` before `alpha`) so the + // assertion below can only pass if declaration order — not clap's + // implicit per-`Command` counter, which would otherwise collide + // with the propagated global flags — is what's driving the sort. + CommandSpec::new("things", "List things") + .no_auth(true) + .with_arg(Arg::new("zeta").long("zeta").help("zeta flag")) + .with_arg(Arg::new("alpha").long("alpha").help("alpha flag")), + async |_credential, _args| Ok(CommandResult::new(json!([{"name": "x"}]))), + )); + + let help = cli.run(["my-cli", "things", "--help"]).await; + assert_eq!(help.exit_code, 0); + + let pos = |needle: &str| help.rendered.find(needle); + let zeta = pos("--zeta"); + let alpha = pos("--alpha"); + let help_flag = pos("-h, --help"); + let output = pos("--output "); + let verbose = pos("--verbose"); + + assert!( + zeta.is_some() + && alpha.is_some() + && help_flag.is_some() + && output.is_some() + && verbose.is_some(), + "expected all five flags in help text, got: {}", + help.rendered + ); + // Command-specific flags render first, as a block, in the order they + // were declared (not alphabetically, not interleaved with globals). + // `Option` compares `None < Some(_)`, so these also implicitly + // re-check presence, but the assert above gives a clearer failure. + assert!( + zeta < alpha, + "zeta ({zeta:?}) should precede alpha ({alpha:?})" + ); + assert!( + alpha < help_flag, + "alpha ({alpha:?}) should precede the first global flag, --help ({help_flag:?})" + ); + // Global flags follow, in the engine's own declared order. + assert!( + help_flag < output, + "--help ({help_flag:?}) should precede --output ({output:?})" + ); + assert!( + output < verbose, + "--output ({output:?}) should precede --verbose ({verbose:?})" + ); +} + +#[tokio::test] +async fn dry_run_flag_is_hidden_for_non_mutating_commands_but_still_parses() { + let mut cli = Cli::new(CliConfig { + name: "my-cli".to_owned(), + short: "Developer tooling".to_owned(), + app_id: "my-cli".to_owned(), + ..CliConfig::default() + }); + cli.add_command(RuntimeCommandSpec::new( + CommandSpec::new("things", "List things").no_auth(true), + async |_credential, _args| Ok(CommandResult::new(json!([{"name": "x"}]))), + )); + cli.add_command(RuntimeCommandSpec::new( + CommandSpec::new("delete-things", "Delete things") + .no_auth(true) + .with_tier(Tier::Destructive), + async |_credential, _args| Ok(CommandResult::new(json!({"deleted": true}))), + )); + + // Non-mutating: `--dry-run` would never short-circuit anything here, so + // it's noise — hidden from `--help`. + let read_help = cli.run(["my-cli", "things", "--help"]).await; + assert_eq!(read_help.exit_code, 0); + assert!(!read_help.rendered.contains("dry-run")); + + // Mutating: `--dry-run` is meaningful and stays visible. + let mutate_help = cli.run(["my-cli", "delete-things", "--help"]).await; + assert_eq!(mutate_help.exit_code, 0); + assert!(mutate_help.rendered.contains("--dry-run")); + + // Hidden only changes `--help`, never parsing: still accepted (as a + // harmless no-op) on the non-mutating command. + let output = cli + .run(["my-cli", "things", "--dry-run", "--output", "json"]) + .await; + assert_eq!(output.exit_code, 0); + assert_eq!( + serde_json::from_str::(&output.rendered).expect("valid json")["data"], + json!([{"name": "x"}]) + ); +} + #[tokio::test] async fn cli_config_extension_hooks_support_custom_flags_search_and_shutdown() { let shutdown_count = Arc::new(AtomicUsize::new(0)); @@ -1822,12 +2122,15 @@ async fn cli_config_extension_hooks_support_custom_flags_search_and_shutdown() { }); let search = cli - .run(["my-cli", "--search", "peering", "--output", "json"]) + .run(["my-cli", "search", "peering", "--output", "json"]) .await; assert_eq!(search.exit_code, 0); let rendered: serde_json::Value = serde_json::from_str(&search.rendered).expect("valid json"); assert_eq!(rendered["data"][0]["command"], "kb network"); - assert_eq!(shutdown_count.load(Ordering::SeqCst), 0); + // `search` goes through the same `finish_run` path as every other + // command now (it's a real builtin, not a pre-parse bypass anymore), so + // `on_shutdown` fires for it too. + assert_eq!(shutdown_count.load(Ordering::SeqCst), 1); let command = cli .run([ @@ -1844,7 +2147,7 @@ async fn cli_config_extension_hooks_support_custom_flags_search_and_shutdown() { assert_eq!(command.exit_code, 0); let rendered: serde_json::Value = serde_json::from_str(&command.rendered).expect("valid json"); assert_eq!(rendered["metadata"]["env"], "prod"); - assert_eq!(shutdown_count.load(Ordering::SeqCst), 1); + assert_eq!(shutdown_count.load(Ordering::SeqCst), 2); } #[tokio::test] @@ -1864,10 +2167,14 @@ async fn cli_config_pre_run_runs_after_init_for_real_commands_only() { })), pre_run: Some(Arc::new(move |middleware, command_path, args| { pre_run_count_for_closure.fetch_add(1, Ordering::SeqCst); - assert_eq!(command_path, "whoami"); - assert_eq!(args.get("name"), Some(&json!("tester"))); - assert_eq!(middleware.env, "init-env"); - middleware.reason = "pre-run reason".to_owned(); + // `search` also runs `pre_run` now (see the other builtins in + // `cli_config_pre_run_runs_for_builtins_without_init_deps_preserves_legacy`), + // so only assert the "whoami"-specific expectations for that call. + if command_path == "whoami" { + assert_eq!(args.get("name"), Some(&json!("tester"))); + assert_eq!(middleware.env, "init-env"); + middleware.reason = "pre-run reason".to_owned(); + } Ok(()) })), commands: vec![RuntimeCommandSpec::new( @@ -1880,11 +2187,13 @@ async fn cli_config_pre_run_runs_after_init_for_real_commands_only() { }); let search = cli - .run(["my-cli", "--search", "whoami", "--output", "json"]) + .run(["my-cli", "search", "whoami", "--output", "json"]) .await; assert_eq!(search.exit_code, 0); assert_eq!(init_count.load(Ordering::SeqCst), 0); - assert_eq!(pre_run_count.load(Ordering::SeqCst), 0); + // `search` is a builtin, like `help`/`guide`/`tree`/`completion`: it runs + // `pre_run` but never `init_deps`. + assert_eq!(pre_run_count.load(Ordering::SeqCst), 1); let output = cli .run([ @@ -1901,7 +2210,7 @@ async fn cli_config_pre_run_runs_after_init_for_real_commands_only() { assert_eq!(rendered["metadata"]["env"], "init-env"); assert_eq!(rendered["metadata"]["effective_args"]["name"], "tester"); assert_eq!(init_count.load(Ordering::SeqCst), 1); - assert_eq!(pre_run_count.load(Ordering::SeqCst), 1); + assert_eq!(pre_run_count.load(Ordering::SeqCst), 2); } #[tokio::test] @@ -1987,6 +2296,10 @@ async fn cli_config_pre_run_runs_for_builtins_without_init_deps_preserves_legacy assert_eq!(tree.exit_code, 0); let guide = cli.run(["my-cli", "guide", "deploy"]).await; assert_eq!(guide.exit_code, 0); + let search = cli + .run(["my-cli", "search", "guide", "--output", "json"]) + .await; + assert_eq!(search.exit_code, 0); assert_eq!(init_count.load(Ordering::SeqCst), 0); assert_eq!( @@ -1995,6 +2308,7 @@ async fn cli_config_pre_run_runs_for_builtins_without_init_deps_preserves_legacy ("help".to_owned(), json!({"command": "guide"})), ("tree".to_owned(), json!({})), ("guide".to_owned(), json!({"topic": "deploy"})), + ("search".to_owned(), json!({"query": "guide"})), ] ); } @@ -2096,7 +2410,7 @@ async fn cli_runtime_guide_command_rejects_extra_args_preserves_parser_maximum_o } #[tokio::test] -async fn cli_runtime_search_bypasses_required_command_flags() { +async fn cli_runtime_search_command_does_not_require_other_commands_flags() { let mut cli = Cli::new(CliConfig { name: "my-cli".to_owned(), short: "Developer tooling".to_owned(), @@ -2115,10 +2429,10 @@ async fn cli_runtime_search_bypasses_required_command_flags() { ), ); + // `search` is a structurally separate top-level command, so it never + // needs `project list`'s required `--project` flag to be satisfied. let output = cli - .run([ - "my-cli", "project", "list", "--search", "project", "--output", "json", - ]) + .run(["my-cli", "search", "project", "--output", "json"]) .await; assert_eq!(output.exit_code, 0); @@ -2128,7 +2442,7 @@ async fn cli_runtime_search_bypasses_required_command_flags() { } #[tokio::test] -async fn cli_runtime_search_scope_resolves_group_aliases_preserves_legacy() { +async fn cli_runtime_search_scope_flag_resolves_group_aliases() { let mut cli = Cli::new(CliConfig { name: "my-cli".to_owned(), short: "Developer tooling".to_owned(), @@ -2153,8 +2467,13 @@ async fn cli_runtime_search_scope_resolves_group_aliases_preserves_legacy() { ), ); + // `--scope p` resolves the group alias "p" to "project" the same way a + // real command path would, narrowing results to just "project list" and + // excluding "noise find". let output = cli - .run(["my-cli", "p", "--search", "projects", "--output", "json"]) + .run([ + "my-cli", "search", "projects", "--scope", "p", "--output", "json", + ]) .await; assert_eq!(output.exit_code, 0); @@ -2169,59 +2488,6 @@ async fn cli_runtime_search_scope_resolves_group_aliases_preserves_legacy() { ); } -#[tokio::test] -async fn cli_runtime_search_scope_preserves_legacy_no_opt_flag_consumption_quirk() { - let mut cli = Cli::new(CliConfig { - name: "my-cli".to_owned(), - short: "Developer tooling".to_owned(), - app_id: "my-cli".to_owned(), - ..CliConfig::default() - }); - cli.add_module_group( - "Platform Systems", - RuntimeGroupSpec::new(GroupSpec::new("project", "Manage projects")).with_command( - RuntimeCommandSpec::new( - CommandSpec::new("list", "List projects").no_auth(true), - async |_credential, _args| Ok(CommandResult::new(json!({}))), - ), - ), - ); - cli.add_module_group( - "Platform Systems", - RuntimeGroupSpec::new(GroupSpec::new("noise", "Noise")).with_command( - RuntimeCommandSpec::new( - CommandSpec::new("find", "Find projects elsewhere").no_auth(true), - async |_credential, _args| Ok(CommandResult::new(json!({}))), - ), - ), - ); - - let output = cli - .run([ - "my-cli", - "--verbose", - "project", - "--search", - "projects", - "--output", - "json", - ]) - .await; - - assert_eq!(output.exit_code, 0); - let rendered: serde_json::Value = serde_json::from_str(&output.rendered).expect("valid json"); - let commands = rendered["data"] - .as_array() - .expect("search results should be an array") - .iter() - .map(|result| result["command"].as_str().unwrap_or_default()) - .collect::>(); - assert!( - commands.contains(&"noise find"), - "Legacy scope resolution treats --verbose as consuming project before --search, so search falls back to root scope; got {commands:?}" - ); -} - #[tokio::test] async fn cli_runtime_search_indexes_group_command_and_flag_aliases() { let mut cli = Cli::new(CliConfig { @@ -2249,7 +2515,7 @@ async fn cli_runtime_search_indexes_group_command_and_flag_aliases() { for query in ["portfolio", "inventory", "domain"] { let output = cli - .run(["my-cli", "--search", query, "--output", "json"]) + .run(["my-cli", "search", query, "--output", "json"]) .await; assert_eq!(output.exit_code, 0); let rendered: serde_json::Value = @@ -2280,7 +2546,7 @@ async fn cli_runtime_hidden_commands_run_but_stay_out_of_discovery() { assert_eq!(rendered["data"], json!({"ok": true})); let search = cli - .run(["my-cli", "--search", "internal", "--output", "json"]) + .run(["my-cli", "search", "internal", "--output", "json"]) .await; assert_eq!(search.exit_code, 0); let rendered: serde_json::Value = serde_json::from_str(&search.rendered).expect("valid json"); @@ -2337,7 +2603,7 @@ async fn cli_runtime_hidden_groups_run_but_hide_their_subtree_from_discovery() { assert_eq!(rendered["data"], json!({"repaired": true})); let search = cli - .run(["my-cli", "--search", "repair", "--output", "json"]) + .run(["my-cli", "search", "repair", "--output", "json"]) .await; assert_eq!(search.exit_code, 0); let rendered: serde_json::Value = serde_json::from_str(&search.rendered).expect("valid json"); @@ -2409,7 +2675,7 @@ async fn cli_runtime_search_includes_guides_at_root_scope() { }]); let output = cli - .run(["my-cli", "--search", "rollout", "--output", "json"]) + .run(["my-cli", "search", "rollout", "--output", "json"]) .await; assert_eq!(output.exit_code, 0); @@ -3214,6 +3480,23 @@ async fn command_spec_system_and_default_fields_builders_drive_runtime_output() {"name": "beta"} ]) ); + + // `--fields` is registered per-command with `default_fields` as its own + // clap default value (so `--help` can show it natively); an explicit + // `--fields` on the command line must still take priority over that + // default rather than being shadowed by it. + let explicit = cli + .run(["my-cli", "things", "--output", "json", "--fields", "all"]) + .await; + assert_eq!(explicit.exit_code, 0); + let rendered: serde_json::Value = serde_json::from_str(&explicit.rendered).expect("valid json"); + assert_eq!( + rendered["data"], + json!([ + {"name": "alpha", "ignored": "x"}, + {"name": "beta", "ignored": "y"} + ]) + ); } #[tokio::test] @@ -3401,8 +3684,8 @@ fn root_long_groups_modules_and_builtin_command_hints() { assert!(rendered.contains("release")); assert!(rendered.contains("Deploy apps")); assert!(rendered.contains(" settings Manage settings")); - assert!(rendered.contains("--search ")); - assert!(rendered.contains("guide Built-in guides")); + assert!(rendered.contains("search Search all commands")); + assert!(rendered.contains("guide Built-in guides")); } #[test] @@ -3990,23 +4273,13 @@ async fn cli_runtime_command_args_preserve_common_clap_value_types() { } #[test] -fn raw_search_and_output_extraction_matches_legacy_bypass_helpers() { - assert_eq!( - extract_search_query(&["my-cli", "release", "--search", "promote"]), - "promote" - ); - assert_eq!( - extract_search_query(&["my-cli", "--search=deploy"]), - "deploy" - ); - assert_eq!(extract_search_query(&["my-cli", "--search"]), ""); - +fn raw_output_format_extraction_matches_legacy_bypass_helper() { assert_eq!( - extract_output_format(&["my-cli", "-o", "json", "--search", "foo"], "json"), + extract_output_format(&["my-cli", "-o", "json", "--reason", "foo"], "json"), "json" ); assert_eq!( - extract_output_format(&["my-cli", "--output=human", "--search", "foo"], "json"), + extract_output_format(&["my-cli", "--output=human", "--reason", "foo"], "json"), "human" ); assert_eq!( @@ -4014,7 +4287,7 @@ fn raw_search_and_output_extraction_matches_legacy_bypass_helpers() { "json" ); assert_eq!( - extract_output_format(&["my-cli", "--search", "foo"], "json"), + extract_output_format(&["my-cli", "--reason", "foo"], "json"), "json" ); assert_eq!( @@ -4039,7 +4312,7 @@ fn raw_search_and_output_extraction_matches_legacy_bypass_helpers() { // `--output` with no value behaves the same way. assert_eq!(extract_output_format(&["my-cli"], "human"), "human"); assert_eq!( - extract_output_format(&["my-cli", "--search", "foo"], "toon"), + extract_output_format(&["my-cli", "--reason", "foo"], "toon"), "toon" ); assert_eq!( @@ -4085,7 +4358,6 @@ fn global_flag_defaults_and_derived_flag_classes_cover_common_clap_actions() { reason: String::new(), timeout: "0s".to_owned(), debug: String::new(), - search: String::new(), credential_store: None, } ); @@ -9645,14 +9917,13 @@ fn schema_registry_returns_legacy_compatible_schema_shape_and_help_section() { assert_eq!(schema.fields[1].name, "enabled"); assert!(schema.fields[1].optional); - let help = format_help_section(&schema.fields); + let help = format_help_section(&schema.fields, &[]); assert!(help.contains("Output fields:")); assert!(help.contains("name string")); assert!(help.contains("enabled bool (optional)")); - assert!(help.contains("--filter \"contains(name, 'example')\"")); - assert!(help.contains("--filter 'enabled'")); - assert!(help.contains("--expr 'length(@)'")); - assert!(help.contains("--expr '[].name'")); + // `--filter`/`--expr` usage examples no longer live in this standalone + // section; they're attached to the flags themselves (see + // `command_help_attaches_filter_and_expr_examples_to_their_own_flags`). cli_engine::register_global_schema_fields( "manual:list", From 8fb6887e03462302a0473a4d701d1240db78267d Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Thu, 30 Jul 2026 13:23:31 -0700 Subject: [PATCH 2/3] fix(cli): give the conditional --env flag an explicit display_order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review on #68 caught that the engine-registered `--env` flag (added directly in `Cli::new`, conditionally, when `CliConfig.environments` is set — not via `register_global_flags`) had no `display_order`, so it fell back to clap's implicit low per-`Command` counter and collided with command-specific flags again, exactly the bug the rest of this PR fixes for every other global flag. Add `global_flag_order::ENV` and apply it. Regression test confirmed against the pre-fix code (fails: --env sorts before --help and the command's own flags; passes after). Co-Authored-By: Claude Sonnet 5 --- src/cli.rs | 1 + src/flags.rs | 8 ++++++++ tests/foundation.rs | 47 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 56 insertions(+) diff --git a/src/cli.rs b/src/cli.rs index f2a2755..c596931 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -917,6 +917,7 @@ impl Cli { .long("env") .global(true) .value_name("ENV") + .display_order(crate::flags::global_flag_order::ENV) .help("Override the active environment (see: env list)"), ); } diff --git a/src/flags.rs b/src/flags.rs index 4d12175..1795ef6 100644 --- a/src/flags.rs +++ b/src/flags.rs @@ -77,6 +77,13 @@ impl Default for GlobalFlags { /// re-registers those three per-command (see `apply_fields_arg` and /// `apply_filter_and_expr_examples`) with contextual help text; they must /// reuse these same values or the override would drift out of position. +/// +/// `REASON` and `ENV` cover the two global flags `Cli::new` registers +/// directly (conditionally, outside `register_global_flags`) rather than +/// this module's own function — `--reason` when an authorizer/auditor/ +/// activity emitter is configured, `--env` when `CliConfig.environments` is +/// set. Both are just as subject to the collision this module exists to +/// prevent, so both need an explicit value here too. pub(crate) mod global_flag_order { pub(crate) const HELP: usize = 1000; pub(crate) const OUTPUT: usize = 1001; @@ -95,6 +102,7 @@ pub(crate) mod global_flag_order { pub(crate) const TOON: usize = 1015; pub(crate) const HUMAN: usize = 1016; pub(crate) const REASON: usize = 1017; + pub(crate) const ENV: usize = 1018; } /// Registers framework-global flags on a `clap` command. diff --git a/tests/foundation.rs b/tests/foundation.rs index 80fe5f0..01c4508 100644 --- a/tests/foundation.rs +++ b/tests/foundation.rs @@ -10966,6 +10966,53 @@ async fn env_group_lists_gets_and_shows_info_for_active_environment() { ); } +#[tokio::test] +async fn command_help_lists_env_flag_after_command_specific_flags_when_environments_configured() { + use cli_engine::environments::{EnvironmentDef, Environments}; + + let mut cli = Cli::new( + CliConfig::new("my-cli", "Developer tooling", "my-cli").with_environments(Arc::new( + Environments::new("prod").with_environment( + "prod", + EnvironmentDef::new().with_field("api_url", "https://p"), + ), + )), + ); + cli.add_command(RuntimeCommandSpec::new( + CommandSpec::new("things", "List things") + .no_auth(true) + .with_arg(Arg::new("zeta").long("zeta").help("zeta flag")), + async |_credential, _args| Ok(CommandResult::new(json!([{"name": "x"}]))), + )); + + let help = cli.run(["my-cli", "things", "--help"]).await; + assert_eq!(help.exit_code, 0); + + let pos = |needle: &str| help.rendered.find(needle); + let zeta = pos("--zeta"); + let env_flag = pos("--env "); + let help_flag = pos("-h, --help"); + + assert!( + zeta.is_some() && env_flag.is_some() && help_flag.is_some(), + "expected all three flags in help text, got: {}", + help.rendered + ); + // `--env` is registered conditionally, directly in `Cli::new` (not via + // `register_global_flags`), when `CliConfig.environments` is set. It + // needs its own `global_flag_order` value just like every other global + // flag, or it falls back to clap's implicit low per-`Command` counter + // and collides with command-specific flags again. + assert!( + zeta < env_flag, + "zeta ({zeta:?}) should precede --env ({env_flag:?})" + ); + assert!( + help_flag < env_flag, + "--help ({help_flag:?}) should precede --env ({env_flag:?}), matching engine declaration order" + ); +} + #[derive(Debug)] struct RecordingEnvProvider { envs: Arc>>, From cfd24cadfe60bf60b5ee6e5291c9ea61218eb70a Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Thu, 30 Jul 2026 14:59:47 -0700 Subject: [PATCH 3/3] fix(cli): warn on stderr for an unresolvable --scope; close order-const gap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Qi's review feedback on #68: - An unresolvable `search --scope ` (typo, or a stale/removed command) used to fall back to a full-tree search with no indication anything was off. Now prints a best-effort stderr warning first ("warning: --scope ... did not match a known command path; searching everything instead") and proceeds with the same full-tree search as before — no behavior change on stdout, no error, just a heads-up. Written directly to a locked stderr handle, matching the transport module's `StderrTransportLogger` convention for this kind of best-effort side-channel diagnostic (outside `Cli::run`'s captured output, so not asserted on directly in tests — the functional fallback is). - Closed the numbering gap in `global_flag_order` left by removing `SEARCH` a few commits back (1012 was skipped). Values are compared only relatively, so the gap was cosmetic, but normalizing doesn't hurt. Co-Authored-By: Claude Sonnet 5 --- src/cli.rs | 31 ++++++++++++++++++++++- src/flags.rs | 12 ++++----- tests/foundation.rs | 60 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 96 insertions(+), 7 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index c596931..8626354 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -2046,12 +2046,21 @@ impl Cli { /// unresolvable scope falls back to an unscoped (root) search rather than /// erroring — `search` staying permissive here matches how a typo in a /// search *query* just yields fewer results instead of a hard failure. + /// An unresolvable (non-empty) scope prints a best-effort stderr hint + /// first, so a typo like `--scope doamin` doesn't silently widen the + /// search with no explanation for the extra results. fn resolve_search_scope(&self, scope_path: &str) -> String { if scope_path.is_empty() { return String::new(); } let parts: Vec = scope_path.split(':').map(str::to_owned).collect(); - canonical_path_from_parts(&self.root, &parts).unwrap_or_default() + match canonical_path_from_parts(&self.root, &parts) { + Some(scope) => scope, + None => { + warn_unresolvable_search_scope(scope_path); + String::new() + } + } } fn canonical_command_path(&self, command_path: &str) -> String { @@ -2690,6 +2699,26 @@ fn canonical_path_from_parts(root: &Command, parts: &[String]) -> Option Some(canonical.join(":")) } +/// Best-effort stderr hint for a `--scope` value that didn't resolve to a +/// known command path — `resolve_search_scope` still searches everything +/// (matching a bare `search` with no `--scope` at all), so this is the only +/// signal the user gets that their scope was ignored rather than applied. +/// Written directly to a locked stderr handle (not `eprintln!`), matching +/// the transport module's own `StderrTransportLogger` convention for this +/// kind of side-channel diagnostic: best-effort, so a write failure is +/// discarded rather than surfaced as a command error. +fn warn_unresolvable_search_scope(scope_path: &str) { + let mut stderr = std::io::stderr().lock(); + stderr + .write_all( + format!( + "warning: --scope {scope_path:?} did not match a known command path; searching everything instead\n" + ) + .as_bytes(), + ) + .ok(); +} + fn collect_command_search_documents( command: &Command, prefix: &mut Vec, diff --git a/src/flags.rs b/src/flags.rs index 1795ef6..d324780 100644 --- a/src/flags.rs +++ b/src/flags.rs @@ -97,12 +97,12 @@ pub(crate) mod global_flag_order { pub(crate) const SCHEMA: usize = 1009; pub(crate) const TIMEOUT: usize = 1010; pub(crate) const DEBUG: usize = 1011; - pub(crate) const CREDENTIAL_STORE: usize = 1013; - pub(crate) const JSON: usize = 1014; - pub(crate) const TOON: usize = 1015; - pub(crate) const HUMAN: usize = 1016; - pub(crate) const REASON: usize = 1017; - pub(crate) const ENV: usize = 1018; + pub(crate) const CREDENTIAL_STORE: usize = 1012; + pub(crate) const JSON: usize = 1013; + pub(crate) const TOON: usize = 1014; + pub(crate) const HUMAN: usize = 1015; + pub(crate) const REASON: usize = 1016; + pub(crate) const ENV: usize = 1017; } /// Registers framework-global flags on a `clap` command. diff --git a/tests/foundation.rs b/tests/foundation.rs index 01c4508..16f2b69 100644 --- a/tests/foundation.rs +++ b/tests/foundation.rs @@ -2488,6 +2488,66 @@ async fn cli_runtime_search_scope_flag_resolves_group_aliases() { ); } +#[tokio::test] +async fn cli_runtime_search_scope_typo_falls_back_to_full_tree_search() { + let mut cli = Cli::new(CliConfig { + name: "my-cli".to_owned(), + short: "Developer tooling".to_owned(), + app_id: "my-cli".to_owned(), + ..CliConfig::default() + }); + cli.add_module_group( + "Platform Systems", + RuntimeGroupSpec::new(GroupSpec::new("project", "Manage projects")).with_command( + RuntimeCommandSpec::new( + CommandSpec::new("list", "List projects").no_auth(true), + async |_credential, _args| Ok(CommandResult::new(json!({}))), + ), + ), + ); + cli.add_module_group( + "Platform Systems", + RuntimeGroupSpec::new(GroupSpec::new("noise", "Noise")).with_command( + RuntimeCommandSpec::new( + CommandSpec::new("find", "Find projects elsewhere").no_auth(true), + async |_credential, _args| Ok(CommandResult::new(json!({}))), + ), + ), + ); + + // A `--scope` that doesn't resolve to any real command path (a typo, or + // a stale/removed one) still searches everything — same as passing no + // `--scope` at all — rather than erroring or silently returning nothing. + // `resolve_search_scope` also prints a stderr hint in this case, but + // that's a best-effort side channel outside `Cli::run`'s captured + // output (see `warn_unresolvable_search_scope`), so it's not asserted + // on here; this test only covers the functional fallback. + let output = cli + .run([ + "my-cli", + "search", + "projects", + "--scope", + "not-a-real-group", + "--output", + "json", + ]) + .await; + + assert_eq!(output.exit_code, 0); + let rendered: serde_json::Value = serde_json::from_str(&output.rendered).expect("valid json"); + let commands = rendered["data"] + .as_array() + .expect("search results should be an array") + .iter() + .map(|result| result["command"].as_str().unwrap_or_default()) + .collect::>(); + assert!( + commands.contains(&"noise find"), + "an unresolvable --scope should fall back to a full-tree search; got {commands:?}" + ); +} + #[tokio::test] async fn cli_runtime_search_indexes_group_command_and_flag_aliases() { let mut cli = Cli::new(CliConfig {