From e1bb17f85d387816621a6e77eca44f94458c4e81 Mon Sep 17 00:00:00 2001 From: Benoit Galati Date: Wed, 27 May 2026 17:47:32 +0200 Subject: [PATCH] fix(gh,glab): don't pre-reject view/checks/run subcommands missing the id gh and glab both accept several subcommands without an explicit id and resolve it from the current branch (gh pr view, gh pr checks, glab mr view) or open an interactive picker (gh run view). RTK was pre-rejecting with "X number required" before the underlying tool ever ran. Six call sites now treat the identifier as optional and forward the request unchanged, letting gh/glab decide: - gh pr view, gh pr checks, gh issue view, gh run view - glab mr view, glab issue view A new `parse_optional_identifier` helper in each file captures the "(Option, Vec)" parse so the no-id branch is unit-testable. format_run_view also drops the dangling `#` from its header when the run id is empty (interactive-picker passthrough). --- src/cmds/git/gh_cmd.rs | 161 +++++++++++++++++++++++++++++---------- src/cmds/git/glab_cmd.rs | 93 +++++++++++++++++----- 2 files changed, 193 insertions(+), 61 deletions(-) diff --git a/src/cmds/git/gh_cmd.rs b/src/cmds/git/gh_cmd.rs index c22c16b401..156b81e2a1 100644 --- a/src/cmds/git/gh_cmd.rs +++ b/src/cmds/git/gh_cmd.rs @@ -162,6 +162,16 @@ fn extract_identifier_and_extra_args(args: &[String]) -> Option<(String, Vec (Option, Vec) { + match extract_identifier_and_extra_args(args) { + Some((id, extra)) => (Some(id), extra), + None => (None, args.to_vec()), + } +} + fn run_gh_json(cmd: Command, label: &str, filter_fn: F) -> Result where F: Fn(&Value) -> String, @@ -319,27 +329,32 @@ fn pr_status_json_fields() -> &'static str { } fn view_pr(args: &[String], _verbose: u8, ultra_compact: bool) -> Result { - let (pr_number, extra_args) = match extract_identifier_and_extra_args(args) { - Some(result) => result, - None => return Err(anyhow::anyhow!("PR number required")), - }; + // `gh pr view` without an identifier defaults to the PR for the current branch. + let (pr_number_opt, extra_args) = parse_optional_identifier(args); if should_passthrough_pr_view(&extra_args) { - return run_passthrough_with_extra("gh", &["pr", "view", &pr_number], &extra_args); + let mut base: Vec<&str> = vec!["pr", "view"]; + if let Some(id) = pr_number_opt.as_deref() { + base.push(id); + } + return run_passthrough_with_extra("gh", &base, &extra_args); } let mut cmd = resolved_command("gh"); + cmd.args(["pr", "view"]); + if let Some(id) = pr_number_opt.as_deref() { + cmd.arg(id); + } cmd.args([ - "pr", - "view", - &pr_number, "--json", "number,title,state,author,body,url,mergeable,reviews,statusCheckRollup", ]); for arg in &extra_args { cmd.arg(arg); } - run_gh_json(cmd, &format!("pr view {}", pr_number), |json| { - format_pr_view(json, ultra_compact) - }) + let label = match pr_number_opt.as_deref() { + Some(id) => format!("pr view {}", id), + None => "pr view".to_string(), + }; + run_gh_json(cmd, &label, |json| format_pr_view(json, ultra_compact)) } fn format_pr_view(json: &Value, ultra_compact: bool) -> String { @@ -427,19 +442,24 @@ fn format_pr_view(json: &Value, ultra_compact: bool) -> String { } fn pr_checks(args: &[String], _verbose: u8, _ultra_compact: bool) -> Result { - let (pr_number, extra_args) = match extract_identifier_and_extra_args(args) { - Some(result) => result, - None => return Err(anyhow::anyhow!("PR number required")), - }; + // `gh pr checks` without an identifier defaults to the PR for the current branch. + let (pr_number_opt, extra_args) = parse_optional_identifier(args); let mut cmd = resolved_command("gh"); - cmd.args(["pr", "checks", &pr_number]); + cmd.args(["pr", "checks"]); + if let Some(id) = pr_number_opt.as_deref() { + cmd.arg(id); + } for arg in &extra_args { cmd.arg(arg); } + let label = match pr_number_opt.as_deref() { + Some(id) => format!("pr checks {}", id), + None => "pr checks".to_string(), + }; runner::run_filtered( cmd, "gh", - &format!("pr checks {}", pr_number), + &label, format_pr_checks, RunOptions::stdout_only() .early_exit_on_failure() @@ -623,27 +643,29 @@ fn format_issue_list(json: &Value, ultra_compact: bool) -> String { } fn view_issue(args: &[String], _verbose: u8) -> Result { - let (issue_number, extra_args) = match extract_identifier_and_extra_args(args) { - Some(result) => result, - None => return Err(anyhow::anyhow!("Issue number required")), - }; + // Let gh emit its own error message when the identifier is missing rather than pre-rejecting. + let (issue_number_opt, extra_args) = parse_optional_identifier(args); if should_passthrough_issue_view(&extra_args) { - return run_passthrough_with_extra("gh", &["issue", "view", &issue_number], &extra_args); + let mut base: Vec<&str> = vec!["issue", "view"]; + if let Some(id) = issue_number_opt.as_deref() { + base.push(id); + } + return run_passthrough_with_extra("gh", &base, &extra_args); } let mut cmd = resolved_command("gh"); - cmd.args([ - "issue", - "view", - &issue_number, - "--json", - "number,title,state,author,body,url", - ]); + cmd.args(["issue", "view"]); + if let Some(id) = issue_number_opt.as_deref() { + cmd.arg(id); + } + cmd.args(["--json", "number,title,state,author,body,url"]); for arg in &extra_args { cmd.arg(arg); } - run_gh_json(cmd, &format!("issue view {}", issue_number), |json| { - format_issue_view(json) - }) + let label = match issue_number_opt.as_deref() { + Some(id) => format!("issue view {}", id), + None => "issue view".to_string(), + }; + run_gh_json(cmd, &label, format_issue_view) } fn format_issue_view(json: &Value) -> String { @@ -753,23 +775,32 @@ fn should_passthrough_run_view(extra_args: &[String]) -> bool { } fn view_run(args: &[String], _verbose: u8) -> Result { - let (run_id, extra_args) = match extract_identifier_and_extra_args(args) { - Some(result) => result, - None => return Err(anyhow::anyhow!("Run ID required")), - }; + // `gh run view` without an identifier opens an interactive picker — defer to gh. + let (run_id_opt, extra_args) = parse_optional_identifier(args); if should_passthrough_run_view(&extra_args) { - return run_passthrough_with_extra("gh", &["run", "view", &run_id], &extra_args); + let mut base: Vec<&str> = vec!["run", "view"]; + if let Some(id) = run_id_opt.as_deref() { + base.push(id); + } + return run_passthrough_with_extra("gh", &base, &extra_args); } let mut cmd = resolved_command("gh"); - cmd.args(["run", "view", &run_id]); + cmd.args(["run", "view"]); + if let Some(id) = run_id_opt.as_deref() { + cmd.arg(id); + } for arg in &extra_args { cmd.arg(arg); } - let run_id_owned = run_id.clone(); + let label = match run_id_opt.as_deref() { + Some(id) => format!("run view {}", id), + None => "run view".to_string(), + }; + let run_id_owned = run_id_opt.unwrap_or_default(); runner::run_filtered( cmd, "gh", - &format!("run view {}", run_id), + &label, move |stdout| format_run_view(stdout, &run_id_owned), RunOptions::stdout_only() .early_exit_on_failure() @@ -781,7 +812,11 @@ fn format_run_view(stdout: &str, run_id: &str) -> String { let mut out = String::new(); let mut in_jobs = false; - out.push_str(&format!("Workflow Run #{}\n", run_id)); + if run_id.is_empty() { + out.push_str("Workflow Run\n"); + } else { + out.push_str(&format!("Workflow Run #{}\n", run_id)); + } for line in stdout.lines() { if line.contains("JOBS") { in_jobs = true; @@ -1077,6 +1112,35 @@ mod tests { assert!(extract_identifier_and_extra_args(&args).is_none()); } + // --- parse_optional_identifier tests --- + + #[test] + fn test_parse_optional_identifier_empty_yields_no_id() { + // `gh pr view` (no args) must surface as (None, []) so the caller + // hands the request to gh, which resolves the current branch's PR. + let (id, extra) = parse_optional_identifier(&[]); + assert!(id.is_none()); + assert!(extra.is_empty()); + } + + #[test] + fn test_parse_optional_identifier_only_flags_preserves_flags() { + // Regression: `gh pr view -R rtk-ai/rtk` previously triggered + // "PR number required". Now flags must round-trip into `extra`. + let args: Vec = vec!["-R".into(), "rtk-ai/rtk".into()]; + let (id, extra) = parse_optional_identifier(&args); + assert!(id.is_none()); + assert_eq!(extra, vec!["-R", "rtk-ai/rtk"]); + } + + #[test] + fn test_parse_optional_identifier_with_id_matches_extract() { + let args: Vec = vec!["-R".into(), "rtk-ai/rtk".into(), "42".into()]; + let (id, extra) = parse_optional_identifier(&args); + assert_eq!(id.as_deref(), Some("42")); + assert_eq!(extra, vec!["-R", "rtk-ai/rtk"]); + } + #[test] fn test_extract_identifier_with_web_flag() { let args: Vec = vec!["123".into(), "--web".into()]; @@ -1108,6 +1172,21 @@ mod tests { assert!(!should_passthrough_run_view(&[])); } + #[test] + fn test_format_run_view_with_id() { + let output = format_run_view("", "12345"); + assert!(output.starts_with("Workflow Run #12345\n")); + } + + #[test] + fn test_format_run_view_without_id() { + // `gh run view` with no arg opens an interactive picker — the captured run id + // is empty, so the header must not render an empty `#`. + let output = format_run_view("", ""); + assert!(output.starts_with("Workflow Run\n")); + assert!(!output.contains("#\n")); + } + #[test] fn test_run_view_no_passthrough_other_flags() { assert!(!should_passthrough_run_view(&["--web".into()])); diff --git a/src/cmds/git/glab_cmd.rs b/src/cmds/git/glab_cmd.rs index a0282a2210..56885f07cc 100644 --- a/src/cmds/git/glab_cmd.rs +++ b/src/cmds/git/glab_cmd.rs @@ -212,6 +212,16 @@ fn extract_identifier_and_extra_args(args: &[String]) -> Option<(String, Vec (Option, Vec) { + match extract_identifier_and_extra_args(args) { + Some((id, extra)) => (Some(id), extra), + None => (None, args.to_vec()), + } +} + /// Check if user explicitly requested JSON/custom output format. /// When present, passthrough to avoid double JSON injection. fn has_output_flag(args: &[String]) -> bool { @@ -409,24 +419,32 @@ fn format_mr_view(json: &Value, ultra_compact: bool) -> String { } fn mr_view(args: &[String], _verbose: u8, ultra_compact: bool) -> Result { - let (mr_number, extra_args) = match extract_identifier_and_extra_args(args) { - Some(pair) => pair, - None => return Err(anyhow::anyhow!("MR number required")), - }; + // `glab mr view` without an identifier defaults to the MR for the current branch. + let (mr_number_opt, extra_args) = parse_optional_identifier(args); // Passthrough for --web, --comments, or explicit output format if should_passthrough_view(&extra_args) { - return run_passthrough_with_extra("glab", &["mr", "view", &mr_number], &extra_args); + let mut base: Vec<&str> = vec!["mr", "view"]; + if let Some(id) = mr_number_opt.as_deref() { + base.push(id); + } + return run_passthrough_with_extra("glab", &base, &extra_args); } let mut cmd = resolved_command("glab"); - cmd.args(["mr", "view", &mr_number, "-F", "json"]); + cmd.args(["mr", "view"]); + if let Some(id) = mr_number_opt.as_deref() { + cmd.arg(id); + } + cmd.args(["-F", "json"]); for arg in &extra_args { cmd.arg(arg); } - run_glab_json(cmd, &format!("mr view {}", mr_number), |json| { - format_mr_view(json, ultra_compact) - }) + let label = match mr_number_opt.as_deref() { + Some(id) => format!("mr view {}", id), + None => "mr view".to_string(), + }; + run_glab_json(cmd, &label, |json| format_mr_view(json, ultra_compact)) } fn mr_create(args: &[String], _verbose: u8) -> Result { @@ -603,25 +621,31 @@ fn format_issue_view(json: &Value) -> String { } fn issue_view(args: &[String], _verbose: u8) -> Result { - let (issue_number, extra_args) = match extract_identifier_and_extra_args(args) { - Some(pair) => pair, - None => return Err(anyhow::anyhow!("Issue number required")), - }; + // Let glab emit its own error message when the identifier is missing rather than pre-rejecting. + let (issue_number_opt, extra_args) = parse_optional_identifier(args); if should_passthrough_view(&extra_args) { - return run_passthrough_with_extra("glab", &["issue", "view", &issue_number], &extra_args); + let mut base: Vec<&str> = vec!["issue", "view"]; + if let Some(id) = issue_number_opt.as_deref() { + base.push(id); + } + return run_passthrough_with_extra("glab", &base, &extra_args); } let mut cmd = resolved_command("glab"); - cmd.args(["issue", "view", &issue_number, "-F", "json"]); + cmd.args(["issue", "view"]); + if let Some(id) = issue_number_opt.as_deref() { + cmd.arg(id); + } + cmd.args(["-F", "json"]); for arg in &extra_args { cmd.arg(arg); } - run_glab_json( - cmd, - &format!("issue view {}", issue_number), - format_issue_view, - ) + let label = match issue_number_opt.as_deref() { + Some(id) => format!("issue view {}", id), + None => "issue view".to_string(), + }; + run_glab_json(cmd, &label, format_issue_view) } // ── CI/Pipeline subcommands ───────────────────────────────────────────── @@ -1235,6 +1259,35 @@ mod tests { assert!(extract_identifier_and_extra_args(&args).is_none()); } + // ── parse_optional_identifier tests ───────────────────────────────── + + #[test] + fn test_parse_optional_identifier_empty_yields_no_id() { + // `glab mr view` (no args) must surface as (None, []) so the caller + // hands the request to glab, which resolves the current branch's MR. + let (id, extra) = parse_optional_identifier(&[]); + assert!(id.is_none()); + assert!(extra.is_empty()); + } + + #[test] + fn test_parse_optional_identifier_only_flags_preserves_flags() { + // Regression: `glab mr view -R group/project` previously triggered + // "MR number required". Now flags must round-trip into `extra`. + let args: Vec = vec!["-R".into(), "group/project".into()]; + let (id, extra) = parse_optional_identifier(&args); + assert!(id.is_none()); + assert_eq!(extra, vec!["-R", "group/project"]); + } + + #[test] + fn test_parse_optional_identifier_with_id_matches_extract() { + let args: Vec = vec!["-R".into(), "group/project".into(), "42".into()]; + let (id, extra) = parse_optional_identifier(&args); + assert_eq!(id.as_deref(), Some("42")); + assert_eq!(extra, vec!["-R", "group/project"]); + } + // ── has_output_flag tests ─────────────────────────────────────────── #[test]