From 8ac69d120371c7df4bc3f308f78fdb93c2d056e2 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 | 67 ++++++++++++++++++------- src/test_commands.rs | 116 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 166 insertions(+), 17 deletions(-) diff --git a/src/main.rs b/src/main.rs index ab8fd3ea..482ce48e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -10896,10 +10896,39 @@ enum AuthActions { // ---- Agent-mode JSON schema for --help ---- /// Walk the clap command tree to find the subcommand matching the given path. +/// 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 +} + 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 @@ -12283,24 +12312,28 @@ 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(); + // Scope to the top-level subcommand (e.g. "logs" even for "logs search"). + let top_level: Vec<&str> = top_level_subcommand(&args).into_iter().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) + // Only emit a schema when the request resolves to a real command: + // either the top-level subcommand exists, or none was given at all. + // If a subcommand name was given but doesn't resolve (e.g. a typo like + // `monitor` for `monitors`), fall through to clap so it reports the + // invalid subcommand with its "did you mean" suggestion. + match target_cmd { + Some(target) => { + let schema = build_agent_schema_scoped(&cmd, target, &top_level); + println!("{}", serde_json::to_string_pretty(&schema).unwrap()); + return Ok(()); } - _ => build_agent_schema(&cmd), - }; - println!("{}", serde_json::to_string_pretty(&schema).unwrap()); - return Ok(()); + None if top_level.is_empty() => { + let schema = build_agent_schema(&cmd); + println!("{}", serde_json::to_string_pretty(&schema).unwrap()); + return Ok(()); + } + // Unknown subcommand: don't intercept — let clap emit the suggestion. + None => {} + } } // --- Extension interception (before clap parsing) --- diff --git a/src/test_commands.rs b/src/test_commands.rs index 0db52d14..26d7bd62 100644 --- a/src/test_commands.rs +++ b/src/test_commands.rs @@ -1165,3 +1165,119 @@ 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_subcommand_when_name_is_typo() { + // Confirms the fall-through target: clap rejects the typo (rather than + // silently accepting it), which is what produces the helpful suggestion. + let result = crate::Cli::command().try_get_matches_from(["pup", "monitor", "list"]); + 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" + ); +} + +#[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); +}