From 936f2b02e07b56582434e27d50aa2913d82ec503 Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Fri, 7 Aug 2026 23:16:42 +0000 Subject: [PATCH] fix: address open CLI issues (#323, #326, #328, #329) - omit dangling em-dash in format_fix_pr when the fix PR URL is empty - strip jj's '(push: ...)' annotation when parsing the origin remote - clamp requested page to the filtered total in bugs/scans list - split HTTPS credentials on the last '@' so passwords may contain '@' Co-Authored-By: Sachin Iyer --- src/api/types.rs | 29 +++++++++++++++++++++++------ src/commands/bugs.rs | 7 ++++--- src/commands/scans.rs | 7 ++++--- src/output.rs | 30 ++++++++++++++++++++++++++++++ src/utils/vcs.rs | 35 ++++++++++++++++++++++++++++++++--- 5 files changed, 93 insertions(+), 15 deletions(-) diff --git a/src/api/types.rs b/src/api/types.rs index 762a91f..0edc096 100644 --- a/src/api/types.rs +++ b/src/api/types.rs @@ -82,13 +82,19 @@ pub fn format_linked_issue(issue: &LinkedIssue) -> String { } } -/// Format the Detail-generated fix PR (number, state, and URL) for the -/// detail/show view, rendered as `# () — `. +/// Format the Detail-generated fix PR for the detail/show view. +/// +/// Rendered as `# () — `. The URL and its separator are +/// omitted when the API returns an empty URL. pub fn format_fix_pr(fix_pr: &FixPr) -> String { - format!( - "#{} ({}) \u{2014} {}", - fix_pr.pr_number, fix_pr.state, fix_pr.url - ) + if fix_pr.url.is_empty() { + format!("#{} ({})", fix_pr.pr_number, fix_pr.state) + } else { + format!( + "#{} ({}) \u{2014} {}", + fix_pr.pr_number, fix_pr.state, fix_pr.url + ) + } } // ── clap::ValueEnum ────────────────────────────────────────────────── @@ -502,6 +508,17 @@ mod tests { ); } + #[test] + fn format_fix_pr_omits_separator_when_url_empty() { + let fix_pr: FixPr = serde_json::from_value(serde_json::json!({ + "prNumber": 42, + "url": "", + "state": "merged" + })) + .expect("valid FixPr JSON"); + assert_eq!(format_fix_pr(&fix_pr), "#42 (merged)"); + } + // ── format_introduced_in ───────────────────────────────────────── #[test] diff --git a/src/commands/bugs.rs b/src/commands/bugs.rs index 5c26271..2fd1d55 100644 --- a/src/commands/bugs.rs +++ b/src/commands/bugs.rs @@ -11,7 +11,7 @@ use crate::api::types::{ review_state_label, Bug, BugDismissalReason, BugId, BugReviewState, ListPublicBugsWorkflowRequestId, RepoId, }; -use crate::output::{output_list, SectionRenderer}; +use crate::output::{clamp_page, output_list, SectionRenderer}; use crate::utils::datetime::{format_datetime, parse_time_spec}; use crate::utils::pagination::page_to_offset; use crate::utils::repos::resolve_repo_id; @@ -574,8 +574,9 @@ pub async fn handle(command: &BugCommands, cli: &crate::Cli) -> Result<()> { let effective_limit = u32::try_from(total.max(1)).unwrap_or(u32::MAX); return output_list(&filtered, total, 1, effective_limit, format); } - let page_items = paginate_items(&filtered, *page, *limit); - output_list(&page_items, total, *page, *limit, format) + let page = clamp_page(*page, total, *limit); + let page_items = paginate_items(&filtered, page, *limit); + output_list(&page_items, total, page, *limit, format) } else if multi_status { // Multiple statuses but no client-side filters: fetch one // page per status and merge, avoiding a full exhaust. diff --git a/src/commands/scans.rs b/src/commands/scans.rs index a2081c2..57cee1f 100644 --- a/src/commands/scans.rs +++ b/src/commands/scans.rs @@ -3,7 +3,7 @@ use clap::Subcommand; use crate::api::client::ApiClient; use crate::api::types::{RepoId, Scan, ScanType, ScansResponse, WorkflowStatus}; -use crate::output::output_list; +use crate::output::{clamp_page, output_list}; use crate::utils::datetime::parse_time_spec; use crate::utils::pagination::page_to_offset; use crate::utils::repos::resolve_repo_id; @@ -157,8 +157,9 @@ pub async fn handle(command: &ScanCommands, cli: &crate::Cli) -> Result<()> { until_ms, ); let total = filtered.len(); - let page_items = paginate_items(&filtered, *page, *limit); - output_list(&page_items, total, *page, *limit, format) + let page = clamp_page(*page, total, *limit); + let page_items = paginate_items(&filtered, page, *limit); + output_list(&page_items, total, page, *limit, format) } else { let offset = page_to_offset(*page, *limit); let scans = client diff --git a/src/output.rs b/src/output.rs index 5f672d4..de52d84 100644 --- a/src/output.rs +++ b/src/output.rs @@ -118,6 +118,14 @@ fn total_pages(total: usize, limit: u32) -> u32 { .max(1) } +/// Clamp a page number to the last page that exists. +/// +/// Client-side filtering can shrink a result set below the requested page, +/// which would otherwise render an impossible `Page: 2 of 1`. +pub fn clamp_page(page: u32, total: usize, limit: u32) -> u32 { + page.clamp(1, total_pages(total, limit)) +} + /// Generic helper to output a list of items in the requested format pub fn output_list( items: &[T], @@ -197,6 +205,28 @@ mod tests { assert_eq!(total_pages(10, 0), 1); } + // ── clamp_page ─────────────────────────────────────────────────── + + #[test] + fn clamp_page_keeps_valid_page() { + assert_eq!(clamp_page(2, 100, 50), 2); + } + + #[test] + fn clamp_page_lowers_page_beyond_last() { + assert_eq!(clamp_page(5, 10, 50), 1); + } + + #[test] + fn clamp_page_empty_results_is_first_page() { + assert_eq!(clamp_page(3, 0, 50), 1); + } + + #[test] + fn clamp_page_raises_zero_to_first_page() { + assert_eq!(clamp_page(0, 100, 50), 1); + } + // ── SectionRenderer builder ────────────────────────────────────── #[test] diff --git a/src/utils/vcs.rs b/src/utils/vcs.rs index 2c02fb8..0f7399f 100644 --- a/src/utils/vcs.rs +++ b/src/utils/vcs.rs @@ -26,11 +26,18 @@ pub fn repo_root() -> Result { } /// Extract the `origin` remote's URL from `jj git remote list` output, -/// which prints one `name url` pair per line. +/// which prints one `name url` pair per line. When a remote's fetch and push +/// URLs differ, jj appends a ` (push: )` annotation that is dropped here +/// so only the fetch URL is returned. fn parse_jj_remote_list(list: &str) -> Option { list.lines() .find_map(|line| line.strip_prefix("origin ")) - .map(|url| url.trim().to_string()) + .map(|url| { + url.split_once(" (push:") + .map_or(url, |(fetch, _)| fetch) + .trim() + .to_string() + }) } /// Extract `owner/repo` from a GitHub remote URL. @@ -94,7 +101,9 @@ fn strip_http_credentials(url: &str) -> Option { // (or end of string). Anything after the authority — including a // `@` in the path — stays untouched. let (authority, path) = rest.split_once('/').unwrap_or((rest, "")); - if let Some((_, host)) = authority.split_once('@') { + // The credentials/host separator is the *last* `@`, since a + // password or token may itself contain `@`. + if let Some((_, host)) = authority.rsplit_once('@') { return Some(format!("{scheme}{host}/{path}")); } } @@ -205,6 +214,17 @@ mod tests { ); } + #[test] + fn jj_remote_list_strips_push_url_annotation() { + // jj appends `(push: )` when fetch and push URLs differ. + assert_eq!( + parse_jj_remote_list( + "origin https://github.com/usedetail/cli.git (push: https://other.com/cli.git)" + ), + Some("https://github.com/usedetail/cli.git".to_string()), + ); + } + // ── parse_github_remote_url ───────────────────────────────────── #[test] @@ -330,6 +350,15 @@ mod tests { ); } + #[test] + fn parses_https_with_at_sign_in_password() { + // The credentials/host separator is the last `@`, not the first. + assert_eq!( + parse_github_remote_url("https://user:p@ss@github.com/usedetail/cli.git"), + Some("usedetail/cli".to_string()), + ); + } + #[test] fn parses_http_with_credentials() { assert_eq!(