Skip to content

Commit 8ac69d1

Browse files
amaskara-ddclaude
andcommitted
fix(main): surface invalid-subcommand hint in agent --help
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 <noreply@anthropic.com>
1 parent 490c829 commit 8ac69d1

2 files changed

Lines changed: 166 additions & 17 deletions

File tree

src/main.rs

Lines changed: 50 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -10896,10 +10896,39 @@ enum AuthActions {
1089610896
// ---- Agent-mode JSON schema for --help ----
1089710897

1089810898
/// Walk the clap command tree to find the subcommand matching the given path.
10899+
/// Extract the top-level subcommand token from raw CLI args (the value passed
10900+
/// to `pup`, including the binary name at index 0). Used by the agent-mode
10901+
/// `--help` intercept, which runs before clap parses.
10902+
///
10903+
/// Skips the binary name, flags, `--help`/`-h`, and any value belonging to a
10904+
/// value-taking global flag — so `--org myorg logs` yields `logs`, not `myorg`.
10905+
/// The `--flag=value` form is a single `-`-prefixed token and needs no lookahead.
10906+
fn top_level_subcommand(args: &[String]) -> Option<&str> {
10907+
// Global flags that consume the following token as their value.
10908+
const VALUE_GLOBALS: &[&str] = &["-o", "--output", "--org", "--jq"];
10909+
let mut prev_consumes_value = false;
10910+
for arg in args.iter().skip(1) {
10911+
if prev_consumes_value {
10912+
prev_consumes_value = false;
10913+
continue;
10914+
}
10915+
if arg.starts_with('-') {
10916+
prev_consumes_value = VALUE_GLOBALS.contains(&arg.as_str());
10917+
continue;
10918+
}
10919+
return Some(arg.as_str());
10920+
}
10921+
None
10922+
}
10923+
1089910924
fn find_subcommand<'a>(cmd: &'a clap::Command, path: &[&str]) -> Option<&'a clap::Command> {
1090010925
let mut current = cmd;
1090110926
for name in path {
10902-
current = current.get_subcommands().find(|s| s.get_name() == *name)?;
10927+
// Match canonical names and aliases so `audit` resolves the same way
10928+
// clap would resolve it to `audit-logs`.
10929+
current = current
10930+
.get_subcommands()
10931+
.find(|s| s.get_name() == *name || s.get_all_aliases().any(|a| a == *name))?;
1090310932
}
1090410933
if path.is_empty() {
1090510934
None
@@ -12283,24 +12312,28 @@ async fn main_inner() -> anyhow::Result<()> {
1228312312
let has_no_agent_flag = args.iter().any(|a| a == "--no-agent");
1228412313
if has_help && !has_no_agent_flag && (useragent::is_agent_mode() || has_agent_flag) {
1228512314
let cmd = Cli::command();
12286-
// Collect subcommand path from args (skip binary name, flags, and --help/-h)
12287-
let sub_path: Vec<&str> = args
12288-
.iter()
12289-
.skip(1)
12290-
.filter(|a| *a != "--help" && *a != "-h" && !a.starts_with('-'))
12291-
.map(|s| s.as_str())
12292-
.collect();
12293-
// Always scope to the top-level subcommand (e.g., "logs" even if "logs search")
12294-
let top_level: Vec<&str> = sub_path.iter().take(1).copied().collect();
12315+
// Scope to the top-level subcommand (e.g. "logs" even for "logs search").
12316+
let top_level: Vec<&str> = top_level_subcommand(&args).into_iter().collect();
1229512317
let target_cmd = find_subcommand(&cmd, &top_level);
12296-
let schema = match target_cmd {
12297-
Some(target) if !top_level.is_empty() => {
12298-
build_agent_schema_scoped(&cmd, target, &top_level)
12318+
// Only emit a schema when the request resolves to a real command:
12319+
// either the top-level subcommand exists, or none was given at all.
12320+
// If a subcommand name was given but doesn't resolve (e.g. a typo like
12321+
// `monitor` for `monitors`), fall through to clap so it reports the
12322+
// invalid subcommand with its "did you mean" suggestion.
12323+
match target_cmd {
12324+
Some(target) => {
12325+
let schema = build_agent_schema_scoped(&cmd, target, &top_level);
12326+
println!("{}", serde_json::to_string_pretty(&schema).unwrap());
12327+
return Ok(());
1229912328
}
12300-
_ => build_agent_schema(&cmd),
12301-
};
12302-
println!("{}", serde_json::to_string_pretty(&schema).unwrap());
12303-
return Ok(());
12329+
None if top_level.is_empty() => {
12330+
let schema = build_agent_schema(&cmd);
12331+
println!("{}", serde_json::to_string_pretty(&schema).unwrap());
12332+
return Ok(());
12333+
}
12334+
// Unknown subcommand: don't intercept — let clap emit the suggestion.
12335+
None => {}
12336+
}
1230412337
}
1230512338

1230612339
// --- Extension interception (before clap parsing) ---

src/test_commands.rs

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1165,3 +1165,119 @@ fn test_saved_widgets_get_parses() {
11651165
_ => panic!("expected Commands::SavedWidgets"),
11661166
}
11671167
}
1168+
1169+
// -------------------------------------------------------------------------
1170+
// Agent-mode --help intercept: subcommand resolution
1171+
//
1172+
// The `--help` intercept in `main_inner` emits a JSON schema only when the
1173+
// requested command resolves. `find_subcommand` drives that decision:
1174+
// Some(_) -> scoped schema
1175+
// None (empty) -> root schema
1176+
// None (non-empty) -> fall through to clap, which reports the typo
1177+
// -------------------------------------------------------------------------
1178+
1179+
#[test]
1180+
fn test_find_subcommand_resolves_when_name_valid() {
1181+
let cmd = crate::Cli::command();
1182+
let found = crate::find_subcommand(&cmd, &["monitors"]);
1183+
assert_eq!(
1184+
found.map(|c| c.get_name()),
1185+
Some("monitors"),
1186+
"a valid top-level subcommand should resolve to itself"
1187+
);
1188+
}
1189+
1190+
#[test]
1191+
fn test_find_subcommand_returns_none_when_name_is_typo() {
1192+
let cmd = crate::Cli::command();
1193+
// `monitor` (singular) is a typo for `monitors`; it must not resolve so the
1194+
// intercept falls through to clap's "did you mean" suggestion.
1195+
assert!(
1196+
crate::find_subcommand(&cmd, &["monitor"]).is_none(),
1197+
"an unknown subcommand must not resolve"
1198+
);
1199+
}
1200+
1201+
#[test]
1202+
fn test_find_subcommand_returns_none_when_path_empty() {
1203+
let cmd = crate::Cli::command();
1204+
// No subcommand given -> root schema branch, not scoped.
1205+
assert!(
1206+
crate::find_subcommand(&cmd, &[]).is_none(),
1207+
"an empty path must not resolve to any subcommand"
1208+
);
1209+
}
1210+
1211+
#[test]
1212+
fn test_find_subcommand_resolves_when_alias_used() {
1213+
let cmd = crate::Cli::command();
1214+
// `audit` is a visible alias of `audit-logs`; it must resolve so agents
1215+
// still get the scoped JSON schema rather than clap's plain-text help.
1216+
let found = crate::find_subcommand(&cmd, &["audit"]);
1217+
assert_eq!(
1218+
found.map(|c| c.get_name()),
1219+
Some("audit-logs"),
1220+
"a visible alias should resolve to its canonical command"
1221+
);
1222+
}
1223+
1224+
#[test]
1225+
fn test_clap_reports_invalid_subcommand_when_name_is_typo() {
1226+
// Confirms the fall-through target: clap rejects the typo (rather than
1227+
// silently accepting it), which is what produces the helpful suggestion.
1228+
let result = crate::Cli::command().try_get_matches_from(["pup", "monitor", "list"]);
1229+
let err = result.expect_err("clap should reject an unknown subcommand");
1230+
assert_eq!(
1231+
err.kind(),
1232+
clap::error::ErrorKind::InvalidSubcommand,
1233+
"unknown subcommand should surface as InvalidSubcommand"
1234+
);
1235+
}
1236+
1237+
#[test]
1238+
fn test_find_subcommand_resolves_nested_path() {
1239+
let cmd = crate::Cli::command();
1240+
// A valid two-level path resolves to the leaf command.
1241+
let found = crate::find_subcommand(&cmd, &["monitors", "list"]);
1242+
assert_eq!(
1243+
found.map(|c| c.get_name()),
1244+
Some("list"),
1245+
"a valid nested path should resolve to the leaf subcommand"
1246+
);
1247+
}
1248+
1249+
fn owned(args: &[&str]) -> Vec<String> {
1250+
args.iter().map(|s| s.to_string()).collect()
1251+
}
1252+
1253+
#[test]
1254+
fn test_top_level_subcommand_returns_first_positional() {
1255+
let args = owned(&["pup", "monitors", "list", "--help", "--agent"]);
1256+
assert_eq!(crate::top_level_subcommand(&args), Some("monitors"));
1257+
}
1258+
1259+
#[test]
1260+
fn test_top_level_subcommand_skips_value_global_before_subcommand() {
1261+
// The value of `--org` must not be mistaken for the subcommand.
1262+
let args = owned(&["pup", "--org", "myorg", "monitors", "--help", "--agent"]);
1263+
assert_eq!(crate::top_level_subcommand(&args), Some("monitors"));
1264+
}
1265+
1266+
#[test]
1267+
fn test_top_level_subcommand_skips_short_value_global() {
1268+
let args = owned(&["pup", "-o", "table", "logs", "--help", "--agent"]);
1269+
assert_eq!(crate::top_level_subcommand(&args), Some("logs"));
1270+
}
1271+
1272+
#[test]
1273+
fn test_top_level_subcommand_handles_attached_value_form() {
1274+
// `--output=table` is one token and consumes no following token.
1275+
let args = owned(&["pup", "--output=table", "logs", "--help"]);
1276+
assert_eq!(crate::top_level_subcommand(&args), Some("logs"));
1277+
}
1278+
1279+
#[test]
1280+
fn test_top_level_subcommand_returns_none_when_flags_only() {
1281+
let args = owned(&["pup", "--agent", "--help"]);
1282+
assert_eq!(crate::top_level_subcommand(&args), None);
1283+
}

0 commit comments

Comments
 (0)