From bddda4fd7e927c93d787c59f25ba206efb541378 Mon Sep 17 00:00:00 2001 From: "anika.maskara" Date: Sun, 16 Aug 2026 23:46:14 -0400 Subject: [PATCH] fix(main): surface invalid-subcommand hint in agent --help MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In agent mode, `--help` is intercepted before clap to emit a JSON schema. The intercept resolved the requested subcommand and fell through a catch-all to the generic root schema (exit 0) whenever it didn't resolve — swallowing clap's "did you mean" suggestion. So `pup monitor list --help --agent` was less helpful than the same command without `--help`. Only emit a schema when the request resolves to a real command (or no subcommand was given); for an unknown subcommand, fall through to clap so it reports the typo with a suggestion. - Make `find_subcommand` alias-aware so valid aliases (e.g. `audit`) still return the scoped JSON schema instead of clap text help. - Extract `top_level_subcommand` to skip values of value-taking global flags (`--org`, `-o/--output`, `--jq`), so `--org x monitors --help` scopes to `monitors`, not the flag value. - Add unit tests for subcommand resolution, alias handling, and top-level extraction. Co-Authored-By: Claude Opus 4.8 --- src/main.rs | 74 +++++++++++++++++------ src/test_commands.rs | 137 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 192 insertions(+), 19 deletions(-) diff --git a/src/main.rs b/src/main.rs index b244814f..13c9c610 100644 --- a/src/main.rs +++ b/src/main.rs @@ -11332,11 +11332,40 @@ enum AuthActions { // ---- Agent-mode JSON schema for --help ---- +/// Extract the top-level subcommand token from raw CLI args (the value passed +/// to `pup`, including the binary name at index 0). Used by the agent-mode +/// `--help` intercept, which runs before clap parses. +/// +/// Skips the binary name, flags, `--help`/`-h`, and any value belonging to a +/// value-taking global flag — so `--org myorg logs` yields `logs`, not `myorg`. +/// The `--flag=value` form is a single `-`-prefixed token and needs no lookahead. +fn top_level_subcommand(args: &[String]) -> Option<&str> { + // Global flags that consume the following token as their value. + const VALUE_GLOBALS: &[&str] = &["-o", "--output", "--org", "--jq"]; + let mut prev_consumes_value = false; + for arg in args.iter().skip(1) { + if prev_consumes_value { + prev_consumes_value = false; + continue; + } + if arg.starts_with('-') { + prev_consumes_value = VALUE_GLOBALS.contains(&arg.as_str()); + continue; + } + return Some(arg.as_str()); + } + None +} + /// Walk the clap command tree to find the subcommand matching the given path. fn find_subcommand<'a>(cmd: &'a clap::Command, path: &[&str]) -> Option<&'a clap::Command> { let mut current = cmd; for name in path { - current = current.get_subcommands().find(|s| s.get_name() == *name)?; + // Match canonical names and aliases so `audit` resolves the same way + // clap would resolve it to `audit-logs`. + current = current + .get_subcommands() + .find(|s| s.get_name() == *name || s.get_all_aliases().any(|a| a == *name))?; } if path.is_empty() { None @@ -11345,6 +11374,27 @@ fn find_subcommand<'a>(cmd: &'a clap::Command, path: &[&str]) -> Option<&'a clap } } +/// Return the agent-help schema for a valid command, or `None` when clap should +/// handle an unknown command or invalid nested subcommand normally. +fn agent_help_schema(cmd: &clap::Command, args: &[String]) -> Option { + let top_level: Vec<&str> = top_level_subcommand(args).into_iter().collect(); + let target_cmd = find_subcommand(cmd, &top_level); + let has_invalid_subcommand = target_cmd.is_some() + && cmd + .clone() + .try_get_matches_from(args) + .is_err_and(|error| error.kind() == clap::error::ErrorKind::InvalidSubcommand); + + match target_cmd { + Some(target) if !has_invalid_subcommand => { + Some(build_agent_schema_scoped(cmd, target, &top_level)) + } + Some(_) => None, + None if top_level.is_empty() => Some(build_agent_schema(cmd)), + None => None, + } +} + /// Guidance returned in the agent schema for LLMs that author shell scripts /// or runbooks the user will execute outside the agent session. Agent mode /// wraps responses in a `{status, data, metadata}` envelope; outside agent @@ -12796,24 +12846,10 @@ async fn main_inner() -> anyhow::Result<()> { let has_no_agent_flag = args.iter().any(|a| a == "--no-agent"); if has_help && !has_no_agent_flag && (useragent::is_agent_mode() || has_agent_flag) { let cmd = Cli::command(); - // Collect subcommand path from args (skip binary name, flags, and --help/-h) - let sub_path: Vec<&str> = args - .iter() - .skip(1) - .filter(|a| *a != "--help" && *a != "-h" && !a.starts_with('-')) - .map(|s| s.as_str()) - .collect(); - // Always scope to the top-level subcommand (e.g., "logs" even if "logs search") - let top_level: Vec<&str> = sub_path.iter().take(1).copied().collect(); - let target_cmd = find_subcommand(&cmd, &top_level); - let schema = match target_cmd { - Some(target) if !top_level.is_empty() => { - build_agent_schema_scoped(&cmd, target, &top_level) - } - _ => build_agent_schema(&cmd), - }; - println!("{}", serde_json::to_string_pretty(&schema).unwrap()); - return Ok(()); + if let Some(schema) = agent_help_schema(&cmd, &args) { + println!("{}", serde_json::to_string_pretty(&schema).unwrap()); + return Ok(()); + } } // --- Extension interception (before clap parsing) --- diff --git a/src/test_commands.rs b/src/test_commands.rs index 591f9296..9ba102be 100644 --- a/src/test_commands.rs +++ b/src/test_commands.rs @@ -1328,3 +1328,140 @@ fn test_saved_widgets_get_parses() { _ => panic!("expected Commands::SavedWidgets"), } } + +// ------------------------------------------------------------------------- +// Agent-mode --help intercept: subcommand resolution +// +// The `--help` intercept in `main_inner` emits a JSON schema only when the +// requested command resolves. `find_subcommand` drives that decision: +// Some(_) -> scoped schema +// None (empty) -> root schema +// None (non-empty) -> fall through to clap, which reports the typo +// ------------------------------------------------------------------------- + +#[test] +fn test_find_subcommand_resolves_when_name_valid() { + let cmd = crate::Cli::command(); + let found = crate::find_subcommand(&cmd, &["monitors"]); + assert_eq!( + found.map(|c| c.get_name()), + Some("monitors"), + "a valid top-level subcommand should resolve to itself" + ); +} + +#[test] +fn test_find_subcommand_returns_none_when_name_is_typo() { + let cmd = crate::Cli::command(); + // `monitor` (singular) is a typo for `monitors`; it must not resolve so the + // intercept falls through to clap's "did you mean" suggestion. + assert!( + crate::find_subcommand(&cmd, &["monitor"]).is_none(), + "an unknown subcommand must not resolve" + ); +} + +#[test] +fn test_find_subcommand_returns_none_when_path_empty() { + let cmd = crate::Cli::command(); + // No subcommand given -> root schema branch, not scoped. + assert!( + crate::find_subcommand(&cmd, &[]).is_none(), + "an empty path must not resolve to any subcommand" + ); +} + +#[test] +fn test_find_subcommand_resolves_when_alias_used() { + let cmd = crate::Cli::command(); + // `audit` is a visible alias of `audit-logs`; it must resolve so agents + // still get the scoped JSON schema rather than clap's plain-text help. + let found = crate::find_subcommand(&cmd, &["audit"]); + assert_eq!( + found.map(|c| c.get_name()), + Some("audit-logs"), + "a visible alias should resolve to its canonical command" + ); +} + +#[test] +fn test_clap_reports_invalid_nested_subcommand_with_suggestion() { + let result = crate::Cli::command() + .try_get_matches_from(["pup", "monitors", "lits", "--help", "--agent"]); + let err = result.expect_err("clap should reject an unknown subcommand"); + assert_eq!( + err.kind(), + clap::error::ErrorKind::InvalidSubcommand, + "unknown subcommand should surface as InvalidSubcommand" + ); + let rendered = err.to_string(); + assert!(rendered.contains("unrecognized subcommand 'lits'")); + assert!(rendered.contains("a similar subcommand exists: 'list'")); +} + +#[test] +fn test_find_subcommand_resolves_nested_path() { + let cmd = crate::Cli::command(); + // A valid two-level path resolves to the leaf command. + let found = crate::find_subcommand(&cmd, &["monitors", "list"]); + assert_eq!( + found.map(|c| c.get_name()), + Some("list"), + "a valid nested path should resolve to the leaf subcommand" + ); +} + +fn owned(args: &[&str]) -> Vec { + args.iter().map(|s| s.to_string()).collect() +} + +#[test] +fn test_top_level_subcommand_returns_first_positional() { + let args = owned(&["pup", "monitors", "list", "--help", "--agent"]); + assert_eq!(crate::top_level_subcommand(&args), Some("monitors")); +} + +#[test] +fn test_top_level_subcommand_skips_value_global_before_subcommand() { + // The value of `--org` must not be mistaken for the subcommand. + let args = owned(&["pup", "--org", "myorg", "monitors", "--help", "--agent"]); + assert_eq!(crate::top_level_subcommand(&args), Some("monitors")); +} + +#[test] +fn test_top_level_subcommand_skips_short_value_global() { + let args = owned(&["pup", "-o", "table", "logs", "--help", "--agent"]); + assert_eq!(crate::top_level_subcommand(&args), Some("logs")); +} + +#[test] +fn test_top_level_subcommand_handles_attached_value_form() { + // `--output=table` is one token and consumes no following token. + let args = owned(&["pup", "--output=table", "logs", "--help"]); + assert_eq!(crate::top_level_subcommand(&args), Some("logs")); +} + +#[test] +fn test_top_level_subcommand_returns_none_when_flags_only() { + let args = owned(&["pup", "--agent", "--help"]); + assert_eq!(crate::top_level_subcommand(&args), None); +} + +#[test] +fn test_agent_help_schema_for_valid_nested_subcommand() { + let cmd = crate::Cli::command(); + let args = owned(&["pup", "monitors", "list", "--help", "--agent"]); + + let schema = crate::agent_help_schema(&cmd, &args) + .expect("valid nested agent help should return a schema"); + + assert_eq!(schema["description"], "Manage monitors"); +} + +#[test] +fn test_agent_help_falls_through_for_invalid_nested_subcommand() { + let cmd = crate::Cli::command(); + let args = owned(&["pup", "monitors", "lits", "--help", "--agent"]); + + assert!(crate::agent_help_schema(&cmd, &args).is_none()); +}