Skip to content

fix(gh,glab): don't pre-reject view/checks/run subcommands missing the id #2123

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aeppling merged 1 commit into from
May 29, 2026
+193 −61
Merged

fix(gh,glab): don't pre-reject view/checks/run subcommands missing the id #2123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
161 changes: 120 additions & 41 deletions src/cmds/git/gh_cmd.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,16 @@ fn extract_identifier_and_extra_args(args: &[String]) -> Option<(String, Vec<Str
identifier.map(|id| (id, extra))
}

/// Like `extract_identifier_and_extra_args` but yields `(None, args.to_vec())` when no
/// positional identifier is present, so callers can defer the "id required" decision
/// to `gh` itself (e.g. `gh pr view` defaults to the current branch's PR).
fn parse_optional_identifier(args: &[String]) -> (Option<String>, Vec<String>) {
match extract_identifier_and_extra_args(args) {
Some((id, extra)) => (Some(id), extra),
None => (None, args.to_vec()),
}
}

fn run_gh_json<F>(cmd: Command, label: &str, filter_fn: F) -> Result<i32>
where
F: Fn(&Value) -> String,
Expand Down Expand Up @@ -319,27 +329,32 @@ fn pr_status_json_fields() -> &'static str {
}

fn view_pr(args: &[String], _verbose: u8, ultra_compact: bool) -> Result<i32> {
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 {
Expand Down Expand Up @@ -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<i32> {
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()
Expand Down Expand Up @@ -623,27 +643,29 @@ fn format_issue_list(json: &Value, ultra_compact: bool) -> String {
}

fn view_issue(args: &[String], _verbose: u8) -> Result<i32> {
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 {
Expand Down Expand Up @@ -753,23 +775,32 @@ fn should_passthrough_run_view(extra_args: &[String]) -> bool {
}

fn view_run(args: &[String], _verbose: u8) -> Result<i32> {
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()
Expand All @@ -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;
Expand Down Expand Up @@ -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<String> = 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<String> = 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<String> = vec!["123".into(), "--web".into()];
Expand Down Expand Up @@ -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()]));
Expand Down
93 changes: 73 additions & 20 deletions src/cmds/git/glab_cmd.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,16 @@ fn extract_identifier_and_extra_args(args: &[String]) -> Option<(String, Vec<Str
identifier.map(|id| (id, extra))
}

/// Like `extract_identifier_and_extra_args` but yields `(None, args.to_vec())` when no
/// positional identifier is present, so callers can defer the "id required" decision
/// to `glab` itself (e.g. `glab mr view` defaults to the current branch's MR).
fn parse_optional_identifier(args: &[String]) -> (Option<String>, Vec<String>) {
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 {
Expand Down Expand Up @@ -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<i32> {
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<i32> {
Expand Down Expand Up @@ -603,25 +621,31 @@ fn format_issue_view(json: &Value) -> String {
}

fn issue_view(args: &[String], _verbose: u8) -> Result<i32> {
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 ─────────────────────────────────────────────
Expand Down Expand Up @@ -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<String> = 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<String> = 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]
Expand Down
Loading