From fcca1b6946e075c5fe8f445ec044f58bfb4e91b0 Mon Sep 17 00:00:00 2001 From: oliviasculley Date: Wed, 5 Aug 2026 18:03:57 +0000 Subject: [PATCH 1/3] feat(git): add `git review-url` to resolve an issue's Linear review URL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Linear's review page for a pull request (linear.app//review/) has no public lookup from a GitHub PR URL, and the slug appears nowhere on the issue or its attachments — `issue.attachments` and `attachmentsForURL` return the GitHub URL and GitHub metadata only. The slug lives on `PullRequest.slugId`, and the one path that reaches a `PullRequest` with a personal API key is the agent sessions attached to an issue (`Query.diff` is [Internal] and takes a `Diff` id nothing hands out). So `git review-url ` walks `issue.agentSessions.pullRequests`, pairs each `slugId` with `organization.urlKey`, and prints the review URL — one per line, or `-o json` for the PR number, state, title and GitHub URL alongside it. A pull request linked by more than one session is listed once. The limitation is inherent to the API rather than to this command, so it is stated in `--help`, in the README, and in the error raised when an issue resolves to no slug, which points at the GitHub PR URL instead of failing silently. --- README.md | 6 ++ src/commands/git.rs | 170 +++++++++++++++++++++++++++++++++++++++++++- src/main.rs | 2 +- 3 files changed, 175 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 22aed2b..787c478 100644 --- a/README.md +++ b/README.md @@ -360,8 +360,14 @@ linear-cli g branch LIN-123 # Show branch name linear-cli g create LIN-123 # Create branch (no checkout) linear-cli g commits # Commits with Linear trailers (jj) linear-cli g pr LIN-123 --draft # Create GitHub PR +linear-cli g review-url LIN-123 # Linear review URL for the issue's PR ``` +`review-url` resolves `https://linear.app//review/` from the +pull requests Linear has linked to the issue's agent sessions — the only place the +API exposes a PR's review slug. A pull request opened outside that flow has no +slug to resolve, so the command reports that and you use the GitHub PR URL. + ### Import / Export Round-trip CSV and JSON import/export with field resolution for status, assignee, and labels. diff --git a/src/commands/git.rs b/src/commands/git.rs index 9a23d81..948f5ed 100644 --- a/src/commands/git.rs +++ b/src/commands/git.rs @@ -1,12 +1,13 @@ use anyhow::Result; use clap::{Subcommand, ValueEnum}; use colored::Colorize; -use serde_json::json; +use serde_json::{json, Value}; use std::path::Path; use std::process::Command; use crate::api::LinearClient; use crate::display_options; +use crate::output::{print_json, OutputOptions}; use crate::text::truncate; use crate::vcs::{generate_branch_name, git_branch_exists, run_git_command, validate_branch_name}; @@ -82,6 +83,17 @@ pub enum GitCommands { #[arg(long, value_enum)] vcs: Option, }, + /// Show the Linear review URL for an issue's pull request(s) + #[command(after_help = r#"EXAMPLES: + linear git review-url LIN-123 # Print the review URL(s) + linear g review-url LIN-123 -o json # Include PR number, state, GitHub URL + +NOTE: Linear only exposes a pull request's review slug for PRs it has linked to +an agent session, so a PR opened outside that flow has no review URL to resolve."#)] + ReviewUrl { + /// Issue identifier (e.g., "LIN-123") or ID + issue: String, + }, /// Create a GitHub PR from a Linear issue #[command(after_help = r#"EXAMPLES: linear git pr LIN-123 # Create PR for issue @@ -140,8 +152,9 @@ fn get_vcs(vcs_flag: Option) -> Result { } } -pub async fn handle(cmd: GitCommands) -> Result<()> { +pub async fn handle(cmd: GitCommands, output: &OutputOptions) -> Result<()> { match cmd { + GitCommands::ReviewUrl { issue } => show_review_url(&issue, output).await, GitCommands::Checkout { issue, branch, vcs } => { let vcs = get_vcs(vcs)?; checkout_issue(&issue, branch, vcs).await @@ -167,6 +180,94 @@ pub async fn handle(cmd: GitCommands) -> Result<()> { } } +/// Build the review entries for an issue from a `review-url` query response. +/// +/// `PullRequest.slugId` is the only public field carrying the slug in a review +/// URL, and it is reachable only through the agent sessions attached to an issue, +/// so an issue can legitimately resolve to zero entries. One pull request can be +/// linked by more than one session, hence the de-duplication by slug. +fn review_entries(url_key: &str, issue: &Value) -> Vec { + let mut seen: Vec = Vec::new(); + let mut entries = Vec::new(); + + let sessions = issue["agentSessions"]["nodes"].as_array(); + for session in sessions.into_iter().flatten() { + let links = session["pullRequests"]["nodes"].as_array(); + for link in links.into_iter().flatten() { + let pr = &link["pullRequest"]; + let Some(slug) = pr["slugId"].as_str().filter(|s| !s.is_empty()) else { + continue; + }; + if seen.iter().any(|s| s == slug) { + continue; + } + seen.push(slug.to_string()); + entries.push(json!({ + "reviewUrl": format!("https://linear.app/{}/review/{}", url_key, slug), + "number": pr["number"], + "status": pr["status"], + "url": pr["url"], + "title": pr["title"], + })); + } + } + + entries +} + +async fn show_review_url(issue_id: &str, output: &OutputOptions) -> Result<()> { + let client = LinearClient::new()?; + + let query = r#" + query($id: String!) { + organization { urlKey } + issue(id: $id) { + identifier + agentSessions { + nodes { + pullRequests { + nodes { + pullRequest { slugId url number status title } + } + } + } + } + } + } + "#; + + let result = client.query(query, Some(json!({ "id": issue_id }))).await?; + let issue = &result["data"]["issue"]; + + if issue.is_null() { + anyhow::bail!("Issue not found: {}", issue_id); + } + + let url_key = result["data"]["organization"]["urlKey"] + .as_str() + .unwrap_or_default(); + let entries = review_entries(url_key, issue); + + if entries.is_empty() { + anyhow::bail!( + "No review URL for {}: Linear exposes a pull request's review slug only \ + for PRs linked to an agent session, and this issue has none. Use the \ + GitHub PR URL instead.", + issue["identifier"].as_str().unwrap_or(issue_id) + ); + } + + if output.is_json() || output.has_template() { + return print_json(&json!(entries), output); + } + + for entry in &entries { + println!("{}", entry["reviewUrl"].as_str().unwrap_or_default()); + } + + Ok(()) +} + async fn get_issue_info(issue_id: &str) -> Result<(String, String, String, String)> { let client = LinearClient::new()?; @@ -602,6 +703,71 @@ async fn create_pr(issue_id: &str, base: &str, draft: bool, web: bool) -> Result mod tests { use super::*; + fn issue_with_sessions(sessions: Value) -> Value { + json!({ "identifier": "LIN-123", "agentSessions": { "nodes": sessions } }) + } + + fn pr_link(slug: &str, number: u64) -> Value { + json!({ "pullRequest": { + "slugId": slug, + "number": number, + "status": "open", + "url": format!("https://github.com/acme/app/pull/{}", number), + "title": "Fix the thing" + }}) + } + + #[test] + fn test_review_entries_builds_review_url_from_slug() { + let issue = issue_with_sessions(json!([ + { "pullRequests": { "nodes": [pr_link("7ffd27854fd2", 183)] } } + ])); + + let entries = review_entries("acme", &issue); + + assert_eq!(entries.len(), 1); + assert_eq!( + entries[0]["reviewUrl"], + "https://linear.app/acme/review/7ffd27854fd2" + ); + assert_eq!(entries[0]["number"], 183); + assert_eq!(entries[0]["url"], "https://github.com/acme/app/pull/183"); + } + + #[test] + fn test_review_entries_dedupes_a_pr_linked_by_several_sessions() { + let issue = issue_with_sessions(json!([ + { "pullRequests": { "nodes": [pr_link("aaa111", 7)] } }, + { "pullRequests": { "nodes": [pr_link("aaa111", 7), pr_link("bbb222", 8)] } } + ])); + + let entries = review_entries("acme", &issue); + + assert_eq!(entries.len(), 2, "the repeated pull request is listed once"); + assert_eq!( + entries[0]["reviewUrl"], + "https://linear.app/acme/review/aaa111" + ); + assert_eq!( + entries[1]["reviewUrl"], + "https://linear.app/acme/review/bbb222" + ); + } + + #[test] + fn test_review_entries_empty_without_sessions_or_slug() { + assert!(review_entries("acme", &issue_with_sessions(json!([]))).is_empty()); + + // A session with no linked pull request, and a link whose slug is missing or + // blank: all unresolvable, and none of them may produce a bogus URL. + let unresolvable = issue_with_sessions(json!([ + { "pullRequests": { "nodes": [] } }, + { "pullRequests": { "nodes": [{ "pullRequest": { "number": 1 } }] } }, + { "pullRequests": { "nodes": [{ "pullRequest": { "slugId": "", "number": 2 } }] } } + ])); + assert!(review_entries("acme", &unresolvable).is_empty()); + } + #[test] fn test_generate_branch_name_simple() { assert_eq!( diff --git a/src/main.rs b/src/main.rs index 76ceab1..916026f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1160,7 +1160,7 @@ async fn run_command( Commands::Search { action } => search::handle(action, output).await?, Commands::Sync { action } => sync::handle(action, output).await?, Commands::Statuses { action } => statuses::handle(action, output).await?, - Commands::Git { action } => git::handle(action).await?, + Commands::Git { action } => git::handle(action, output).await?, Commands::Bulk { action } => bulk::handle(action, output).await?, Commands::Cache { action } => commands::cache::handle(action).await?, Commands::Notifications { action } => notifications::handle(action, output).await?, From e7623d21aab95b83d23defbcc60d560534a2538b Mon Sep 17 00:00:00 2001 From: oliviasculley Date: Wed, 5 Aug 2026 19:07:56 +0000 Subject: [PATCH 2/3] fix(git): resolve review-url from pull request notifications MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first pass claimed a review URL exists only for pull requests linked to an agent session. That is wrong: Linear creates a review page for any pull request it detects from a branch, and `PullRequestNotification` exposes it — `url` on the notification is the review page itself (`review/-`), alongside the `pullRequest` it belongs to. So `review-url` now matches the issue's `github` pull request attachments against the notification feed and returns that URL verbatim, which also preserves the human-readable title slug instead of dropping it by assembling `review/` by hand. A comment notification's `#comment-` anchor is trimmed so the result is the page, not a position in it. The agent-session path stays as the fallback for a pull request with no notifications, and results are merged per pull request so a PR reachable both ways is listed once. The feed has no server-side pull request filter, so it is walked newest-first for at most 5 pages; a pull request whose activity is older than that falls through to the fallback. What remains genuinely unresolvable is a pull request with no notification at all — typically one opened minutes ago with no CI result, comment, or review yet — and the error says so rather than emitting a URL that would 404. --- README.md | 10 +- src/commands/git.rs | 247 +++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 241 insertions(+), 16 deletions(-) diff --git a/README.md b/README.md index 787c478..3dad941 100644 --- a/README.md +++ b/README.md @@ -363,10 +363,12 @@ linear-cli g pr LIN-123 --draft # Create GitHub PR linear-cli g review-url LIN-123 # Linear review URL for the issue's PR ``` -`review-url` resolves `https://linear.app//review/` from the -pull requests Linear has linked to the issue's agent sessions — the only place the -API exposes a PR's review slug. A pull request opened outside that flow has no -slug to resolve, so the command reports that and you use the GitHub PR URL. +`review-url` reads the review URL from the issue's pull request notifications — +the one public place a pull request is paired with its review page — and falls +back to the pull requests linked to the issue's agent sessions. A pull request +that has produced neither (a brand-new PR with no CI result, comment, or review +activity yet) has nothing to resolve, and the command says so instead of guessing +a URL. ### Import / Export diff --git a/src/commands/git.rs b/src/commands/git.rs index 948f5ed..8df9f7f 100644 --- a/src/commands/git.rs +++ b/src/commands/git.rs @@ -88,8 +88,9 @@ pub enum GitCommands { linear git review-url LIN-123 # Print the review URL(s) linear g review-url LIN-123 -o json # Include PR number, state, GitHub URL -NOTE: Linear only exposes a pull request's review slug for PRs it has linked to -an agent session, so a PR opened outside that flow has no review URL to resolve."#)] +NOTE: The review URL is read from the issue's pull request notifications, falling +back to the pull requests linked to its agent sessions. A pull request that has +produced neither has no review URL to resolve."#)] ReviewUrl { /// Issue identifier (e.g., "LIN-123") or ID issue: String, @@ -180,12 +181,84 @@ pub async fn handle(cmd: GitCommands, output: &OutputOptions) -> Result<()> { } } -/// Build the review entries for an issue from a `review-url` query response. +/// GitHub pull request URLs attached to an issue. /// -/// `PullRequest.slugId` is the only public field carrying the slug in a review -/// URL, and it is reachable only through the agent sessions attached to an issue, -/// so an issue can legitimately resolve to zero entries. One pull request can be -/// linked by more than one session, hence the de-duplication by slug. +/// Linear links a pull request to an issue as a `github` attachment as soon as it +/// detects the branch, so this covers every pull request of the issue — but the +/// attachment carries no review slug, which is why the slug is looked up +/// separately. +fn attached_pr_urls(issue: &Value) -> Vec { + let mut urls = Vec::new(); + for node in issue["attachments"]["nodes"] + .as_array() + .into_iter() + .flatten() + { + if node["sourceType"].as_str() != Some("github") { + continue; + } + if let Some(url) = node["url"].as_str().filter(|u| u.contains("/pull/")) { + if !urls.iter().any(|existing| existing == url) { + urls.push(url.to_string()); + } + } + } + urls +} + +/// Pick out the review URLs for `pr_urls` from a page of notifications. +/// +/// A `PullRequestNotification` is the one public place that pairs a pull request +/// with its Linear review URL, and it carries that URL whole (`review/-<id>`) rather than requiring it to be assembled. +fn review_entries_from_notifications(nodes: &[Value], pr_urls: &[String]) -> Vec<Value> { + let mut entries = Vec::new(); + for node in nodes { + let pr = &node["pullRequest"]; + let Some(pr_url) = pr["url"].as_str() else { + continue; + }; + if !pr_urls.iter().any(|wanted| wanted == pr_url) { + continue; + } + let Some(review_url) = node["url"].as_str().filter(|u| !u.is_empty()) else { + continue; + }; + // A comment notification points at an anchor within the review page + // (`…#comment-<id>`); the page itself is what a caller wants. + let review_url = review_url.split('#').next().unwrap_or(review_url); + entries.push(json!({ + "reviewUrl": review_url, + "number": pr["number"], + "status": pr["status"], + "url": pr_url, + "title": pr["title"], + })); + } + entries +} + +/// Merge review entries, keeping the first entry seen per pull request URL. +fn merge_review_entries(entries: Vec<Value>) -> Vec<Value> { + let mut seen: Vec<String> = Vec::new(); + let mut merged = Vec::new(); + for entry in entries { + let key = entry["url"].as_str().unwrap_or_default().to_string(); + if !key.is_empty() && seen.contains(&key) { + continue; + } + seen.push(key); + merged.push(entry); + } + merged +} + +/// Build review entries from the agent sessions attached to an issue. +/// +/// This is the fallback for a pull request with no notification: an agent session +/// exposes `PullRequest.slugId` directly, from which the review URL can be +/// assembled. One pull request can be linked by several sessions, hence the +/// de-duplication by slug. fn review_entries(url_key: &str, issue: &Value) -> Vec<Value> { let mut seen: Vec<String> = Vec::new(); let mut entries = Vec::new(); @@ -215,6 +288,55 @@ fn review_entries(url_key: &str, issue: &Value) -> Vec<Value> { entries } +/// How many pages of notifications `review-url` will read before giving up. +/// +/// The notification feed has no server-side filter for pull requests, so it is +/// walked newest-first; a pull request whose last notification is older than this +/// falls through to the agent-session path. +const REVIEW_NOTIFICATION_PAGES: usize = 5; + +/// Look up review URLs for `pr_urls` by walking the notification feed. +async fn review_urls_via_notifications( + client: &LinearClient, + pr_urls: &[String], +) -> Result<Vec<Value>> { + let query = r#" + query($after: String) { + notifications(first: 100, after: $after, includeArchived: true) { + pageInfo { hasNextPage endCursor } + nodes { + __typename + ... on PullRequestNotification { + url + pullRequest { url number status title } + } + } + } + } + "#; + + let mut entries: Vec<Value> = Vec::new(); + let mut cursor: Option<String> = None; + + for _ in 0..REVIEW_NOTIFICATION_PAGES { + let result = client + .query(query, Some(json!({ "after": cursor }))) + .await?; + let page = &result["data"]["notifications"]; + let nodes = page["nodes"].as_array().cloned().unwrap_or_default(); + + entries.extend(review_entries_from_notifications(&nodes, pr_urls)); + entries = merge_review_entries(entries); + + if entries.len() == pr_urls.len() || page["pageInfo"]["hasNextPage"] != json!(true) { + break; + } + cursor = page["pageInfo"]["endCursor"].as_str().map(str::to_string); + } + + Ok(entries) +} + async fn show_review_url(issue_id: &str, output: &OutputOptions) -> Result<()> { let client = LinearClient::new()?; @@ -223,6 +345,7 @@ async fn show_review_url(issue_id: &str, output: &OutputOptions) -> Result<()> { organization { urlKey } issue(id: $id) { identifier + attachments { nodes { url sourceType } } agentSessions { nodes { pullRequests { @@ -246,14 +369,32 @@ async fn show_review_url(issue_id: &str, output: &OutputOptions) -> Result<()> { let url_key = result["data"]["organization"]["urlKey"] .as_str() .unwrap_or_default(); - let entries = review_entries(url_key, issue); + let pr_urls = attached_pr_urls(issue); + + // A notification carries the review URL whole; agent sessions only expose the + // slug to assemble one, so they are the fallback. + let mut entries = if pr_urls.is_empty() { + Vec::new() + } else { + review_urls_via_notifications(&client, &pr_urls).await? + }; + entries.extend(review_entries(url_key, issue)); + let entries = merge_review_entries(entries); if entries.is_empty() { anyhow::bail!( - "No review URL for {}: Linear exposes a pull request's review slug only \ - for PRs linked to an agent session, and this issue has none. Use the \ - GitHub PR URL instead.", - issue["identifier"].as_str().unwrap_or(issue_id) + "No review URL for {}: {}. Use the GitHub PR URL instead.", + issue["identifier"].as_str().unwrap_or(issue_id), + if pr_urls.is_empty() { + "no pull request is linked to this issue".to_string() + } else { + format!( + "Linear exposes a review URL through pull request notifications, and none \ + of the {} linked pull request(s) has one in the last {} pages of the feed", + pr_urls.len(), + REVIEW_NOTIFICATION_PAGES + ) + } ); } @@ -717,6 +858,88 @@ mod tests { }}) } + #[test] + fn test_attached_pr_urls_keeps_github_pull_requests_only() { + let issue = json!({ "attachments": { "nodes": [ + { "sourceType": "github", "url": "https://github.com/acme/app/pull/183" }, + { "sourceType": "github", "url": "https://github.com/acme/app/pull/183" }, + { "sourceType": "github", "url": "https://github.com/acme/app/issues/12" }, + { "sourceType": "sentry", "url": "https://sentry.io/acme/app/pull/1" }, + { "sourceType": "github", "url": "https://github.com/acme/app/pull/184" } + ]}}); + + assert_eq!( + attached_pr_urls(&issue), + vec![ + "https://github.com/acme/app/pull/183", + "https://github.com/acme/app/pull/184" + ] + ); + } + + #[test] + fn test_review_entries_from_notifications_uses_the_notification_url() { + let nodes = vec![ + json!({ + "__typename": "IssueNotification", + "url": "https://linear.app/acme/issue/LIN-1" + }), + json!({ + "__typename": "PullRequestNotification", + "url": "https://linear.app/acme/review/fix-the-thing-72e2bba2372a", + "pullRequest": { + "url": "https://github.com/acme/app/pull/183", + "number": 183, "status": "open", "title": "Fix the thing" + } + }), + json!({ + "__typename": "PullRequestNotification", + "url": "https://linear.app/acme/review/someone-elses-pr-aaaaaaaaaaaa", + "pullRequest": { "url": "https://github.com/acme/app/pull/999", "number": 999 } + }), + ]; + let wanted = vec!["https://github.com/acme/app/pull/183".to_string()]; + + let entries = review_entries_from_notifications(&nodes, &wanted); + + assert_eq!(entries.len(), 1, "only the requested pull request matches"); + assert_eq!( + entries[0]["reviewUrl"], "https://linear.app/acme/review/fix-the-thing-72e2bba2372a", + "the notification's URL is used verbatim, not reassembled from the slug" + ); + assert_eq!(entries[0]["number"], 183); + } + + #[test] + fn test_review_entries_from_notifications_drops_a_comment_anchor() { + let nodes = vec![json!({ + "__typename": "PullRequestNotification", + "url": "https://linear.app/acme/review/fix-the-thing-72e2bba2372a#comment-5f63aa7c", + "pullRequest": { "url": "https://github.com/acme/app/pull/183", "number": 183 } + })]; + let wanted = vec!["https://github.com/acme/app/pull/183".to_string()]; + + let entries = review_entries_from_notifications(&nodes, &wanted); + + assert_eq!( + entries[0]["reviewUrl"], "https://linear.app/acme/review/fix-the-thing-72e2bba2372a", + "a comment notification must still yield the review page URL" + ); + } + + #[test] + fn test_merge_review_entries_prefers_the_first_entry_per_pull_request() { + let merged = merge_review_entries(vec![ + json!({ "url": "https://github.com/acme/app/pull/1", "reviewUrl": "from-notification" }), + json!({ "url": "https://github.com/acme/app/pull/1", "reviewUrl": "from-agent-session" }), + json!({ "url": "https://github.com/acme/app/pull/2", "reviewUrl": "other" }), + ]); + + assert_eq!(merged.len(), 2); + assert_eq!(merged[0]["reviewUrl"], "from-notification"); + assert_eq!(merged[1]["reviewUrl"], "other"); + } + #[test] fn test_review_entries_builds_review_url_from_slug() { let issue = issue_with_sessions(json!([ From add69540af99aff15a53fb5593d88fb720930ad6 Mon Sep 17 00:00:00 2001 From: oliviasculley <olivia@sculley.dev> Date: Tue, 25 Aug 2026 02:34:37 +0000 Subject: [PATCH 3/3] refactor(git): model review-url resolution with typed entries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the review on #45. Move `review-url` out of `git.rs` into `src/commands/git/review_url.rs`, so `git.rs` holds the command variants and local VCS work again rather than GraphQL queries, feed pagination, and API decoding. `git.rs` returns to roughly its pre-feature size. Replace the raw-`Value` merge pipeline with typed deserialization and a serializable `ReviewEntry`. Sources merge into one map keyed by GitHub pull request URL, with the agent-session fallback inserted first so a notification result replaces it — precedence is now the merge's structure rather than a convention about ordering. Missing fields are `Option`s instead of silent nulls, and a pull request Linear returns without a URL yields no entry rather than one with a null identity. Resolution is modelled as resolved plus unresolved pull requests. Previously an issue with two attached pull requests where only one resolved printed that one URL and exited 0, saying nothing about the other. Unresolved pull requests are now part of the output contract: `-o json` returns `{"resolved": [...], "unresolved": [...]}` and the plain-text form names them on stderr. The command still fails only when it resolved nothing. Drop the private cursor state machine in favour of `paginate_until`, a short-circuiting paginator alongside `paginate_nodes` in `pagination.rs`. It follows the canonical cursor rules — including stopping when a connection claims `hasNextPage` without returning an `endCursor`, which the private loop would have answered by rereading the first page until its five-page cap. It stops as soon as every wanted pull request is found, so the common resolved-on-the-first-page case still costs one request rather than five. Also drop a trailing blank line in `initiatives.rs` that failed `cargo fmt --check` and so kept Clippy from running. --- README.md | 9 +- src/commands/git.rs | 388 +--------------- src/commands/git/review_url.rs | 788 +++++++++++++++++++++++++++++++++ src/commands/initiatives.rs | 1 - src/pagination.rs | 122 +++++ 5 files changed, 926 insertions(+), 382 deletions(-) create mode 100644 src/commands/git/review_url.rs diff --git a/README.md b/README.md index 3dad941..030aad3 100644 --- a/README.md +++ b/README.md @@ -367,8 +367,13 @@ linear-cli g review-url LIN-123 # Linear review URL for the iss the one public place a pull request is paired with its review page — and falls back to the pull requests linked to the issue's agent sessions. A pull request that has produced neither (a brand-new PR with no CI result, comment, or review -activity yet) has nothing to resolve, and the command says so instead of guessing -a URL. +activity yet) has nothing to resolve, and the command reports it instead of +guessing a URL. + +Unresolved pull requests are part of the output, not a silent omission: `-o json` +returns `{"resolved": [...], "unresolved": [...]}`, and the plain-text form prints +the review URLs on stdout while naming any unresolved pull request on stderr. The +command fails only when it resolved nothing at all. ### Import / Export diff --git a/src/commands/git.rs b/src/commands/git.rs index 8df9f7f..7799a27 100644 --- a/src/commands/git.rs +++ b/src/commands/git.rs @@ -1,16 +1,20 @@ use anyhow::Result; use clap::{Subcommand, ValueEnum}; use colored::Colorize; -use serde_json::{json, Value}; +use serde_json::json; use std::path::Path; use std::process::Command; use crate::api::LinearClient; use crate::display_options; -use crate::output::{print_json, OutputOptions}; +use crate::output::OutputOptions; use crate::text::truncate; use crate::vcs::{generate_branch_name, git_branch_exists, run_git_command, validate_branch_name}; +mod review_url; + +use review_url::show_review_url; + /// Version control system type #[derive(Clone, Copy, Debug, PartialEq, Eq, ValueEnum)] pub enum Vcs { @@ -86,11 +90,12 @@ pub enum GitCommands { /// Show the Linear review URL for an issue's pull request(s) #[command(after_help = r#"EXAMPLES: linear git review-url LIN-123 # Print the review URL(s) - linear g review-url LIN-123 -o json # Include PR number, state, GitHub URL + linear g review-url LIN-123 -o json # Resolved and unresolved PRs NOTE: The review URL is read from the issue's pull request notifications, falling back to the pull requests linked to its agent sessions. A pull request that has -produced neither has no review URL to resolve."#)] +produced neither has no review URL to resolve; it is listed as unresolved rather +than dropped, on stderr in plain text and under "unresolved" in JSON."#)] ReviewUrl { /// Issue identifier (e.g., "LIN-123") or ID issue: String, @@ -181,234 +186,6 @@ pub async fn handle(cmd: GitCommands, output: &OutputOptions) -> Result<()> { } } -/// GitHub pull request URLs attached to an issue. -/// -/// Linear links a pull request to an issue as a `github` attachment as soon as it -/// detects the branch, so this covers every pull request of the issue — but the -/// attachment carries no review slug, which is why the slug is looked up -/// separately. -fn attached_pr_urls(issue: &Value) -> Vec<String> { - let mut urls = Vec::new(); - for node in issue["attachments"]["nodes"] - .as_array() - .into_iter() - .flatten() - { - if node["sourceType"].as_str() != Some("github") { - continue; - } - if let Some(url) = node["url"].as_str().filter(|u| u.contains("/pull/")) { - if !urls.iter().any(|existing| existing == url) { - urls.push(url.to_string()); - } - } - } - urls -} - -/// Pick out the review URLs for `pr_urls` from a page of notifications. -/// -/// A `PullRequestNotification` is the one public place that pairs a pull request -/// with its Linear review URL, and it carries that URL whole (`review/<title -/// slug>-<id>`) rather than requiring it to be assembled. -fn review_entries_from_notifications(nodes: &[Value], pr_urls: &[String]) -> Vec<Value> { - let mut entries = Vec::new(); - for node in nodes { - let pr = &node["pullRequest"]; - let Some(pr_url) = pr["url"].as_str() else { - continue; - }; - if !pr_urls.iter().any(|wanted| wanted == pr_url) { - continue; - } - let Some(review_url) = node["url"].as_str().filter(|u| !u.is_empty()) else { - continue; - }; - // A comment notification points at an anchor within the review page - // (`…#comment-<id>`); the page itself is what a caller wants. - let review_url = review_url.split('#').next().unwrap_or(review_url); - entries.push(json!({ - "reviewUrl": review_url, - "number": pr["number"], - "status": pr["status"], - "url": pr_url, - "title": pr["title"], - })); - } - entries -} - -/// Merge review entries, keeping the first entry seen per pull request URL. -fn merge_review_entries(entries: Vec<Value>) -> Vec<Value> { - let mut seen: Vec<String> = Vec::new(); - let mut merged = Vec::new(); - for entry in entries { - let key = entry["url"].as_str().unwrap_or_default().to_string(); - if !key.is_empty() && seen.contains(&key) { - continue; - } - seen.push(key); - merged.push(entry); - } - merged -} - -/// Build review entries from the agent sessions attached to an issue. -/// -/// This is the fallback for a pull request with no notification: an agent session -/// exposes `PullRequest.slugId` directly, from which the review URL can be -/// assembled. One pull request can be linked by several sessions, hence the -/// de-duplication by slug. -fn review_entries(url_key: &str, issue: &Value) -> Vec<Value> { - let mut seen: Vec<String> = Vec::new(); - let mut entries = Vec::new(); - - let sessions = issue["agentSessions"]["nodes"].as_array(); - for session in sessions.into_iter().flatten() { - let links = session["pullRequests"]["nodes"].as_array(); - for link in links.into_iter().flatten() { - let pr = &link["pullRequest"]; - let Some(slug) = pr["slugId"].as_str().filter(|s| !s.is_empty()) else { - continue; - }; - if seen.iter().any(|s| s == slug) { - continue; - } - seen.push(slug.to_string()); - entries.push(json!({ - "reviewUrl": format!("https://linear.app/{}/review/{}", url_key, slug), - "number": pr["number"], - "status": pr["status"], - "url": pr["url"], - "title": pr["title"], - })); - } - } - - entries -} - -/// How many pages of notifications `review-url` will read before giving up. -/// -/// The notification feed has no server-side filter for pull requests, so it is -/// walked newest-first; a pull request whose last notification is older than this -/// falls through to the agent-session path. -const REVIEW_NOTIFICATION_PAGES: usize = 5; - -/// Look up review URLs for `pr_urls` by walking the notification feed. -async fn review_urls_via_notifications( - client: &LinearClient, - pr_urls: &[String], -) -> Result<Vec<Value>> { - let query = r#" - query($after: String) { - notifications(first: 100, after: $after, includeArchived: true) { - pageInfo { hasNextPage endCursor } - nodes { - __typename - ... on PullRequestNotification { - url - pullRequest { url number status title } - } - } - } - } - "#; - - let mut entries: Vec<Value> = Vec::new(); - let mut cursor: Option<String> = None; - - for _ in 0..REVIEW_NOTIFICATION_PAGES { - let result = client - .query(query, Some(json!({ "after": cursor }))) - .await?; - let page = &result["data"]["notifications"]; - let nodes = page["nodes"].as_array().cloned().unwrap_or_default(); - - entries.extend(review_entries_from_notifications(&nodes, pr_urls)); - entries = merge_review_entries(entries); - - if entries.len() == pr_urls.len() || page["pageInfo"]["hasNextPage"] != json!(true) { - break; - } - cursor = page["pageInfo"]["endCursor"].as_str().map(str::to_string); - } - - Ok(entries) -} - -async fn show_review_url(issue_id: &str, output: &OutputOptions) -> Result<()> { - let client = LinearClient::new()?; - - let query = r#" - query($id: String!) { - organization { urlKey } - issue(id: $id) { - identifier - attachments { nodes { url sourceType } } - agentSessions { - nodes { - pullRequests { - nodes { - pullRequest { slugId url number status title } - } - } - } - } - } - } - "#; - - let result = client.query(query, Some(json!({ "id": issue_id }))).await?; - let issue = &result["data"]["issue"]; - - if issue.is_null() { - anyhow::bail!("Issue not found: {}", issue_id); - } - - let url_key = result["data"]["organization"]["urlKey"] - .as_str() - .unwrap_or_default(); - let pr_urls = attached_pr_urls(issue); - - // A notification carries the review URL whole; agent sessions only expose the - // slug to assemble one, so they are the fallback. - let mut entries = if pr_urls.is_empty() { - Vec::new() - } else { - review_urls_via_notifications(&client, &pr_urls).await? - }; - entries.extend(review_entries(url_key, issue)); - let entries = merge_review_entries(entries); - - if entries.is_empty() { - anyhow::bail!( - "No review URL for {}: {}. Use the GitHub PR URL instead.", - issue["identifier"].as_str().unwrap_or(issue_id), - if pr_urls.is_empty() { - "no pull request is linked to this issue".to_string() - } else { - format!( - "Linear exposes a review URL through pull request notifications, and none \ - of the {} linked pull request(s) has one in the last {} pages of the feed", - pr_urls.len(), - REVIEW_NOTIFICATION_PAGES - ) - } - ); - } - - if output.is_json() || output.has_template() { - return print_json(&json!(entries), output); - } - - for entry in &entries { - println!("{}", entry["reviewUrl"].as_str().unwrap_or_default()); - } - - Ok(()) -} - async fn get_issue_info(issue_id: &str) -> Result<(String, String, String, String)> { let client = LinearClient::new()?; @@ -844,153 +621,6 @@ async fn create_pr(issue_id: &str, base: &str, draft: bool, web: bool) -> Result mod tests { use super::*; - fn issue_with_sessions(sessions: Value) -> Value { - json!({ "identifier": "LIN-123", "agentSessions": { "nodes": sessions } }) - } - - fn pr_link(slug: &str, number: u64) -> Value { - json!({ "pullRequest": { - "slugId": slug, - "number": number, - "status": "open", - "url": format!("https://github.com/acme/app/pull/{}", number), - "title": "Fix the thing" - }}) - } - - #[test] - fn test_attached_pr_urls_keeps_github_pull_requests_only() { - let issue = json!({ "attachments": { "nodes": [ - { "sourceType": "github", "url": "https://github.com/acme/app/pull/183" }, - { "sourceType": "github", "url": "https://github.com/acme/app/pull/183" }, - { "sourceType": "github", "url": "https://github.com/acme/app/issues/12" }, - { "sourceType": "sentry", "url": "https://sentry.io/acme/app/pull/1" }, - { "sourceType": "github", "url": "https://github.com/acme/app/pull/184" } - ]}}); - - assert_eq!( - attached_pr_urls(&issue), - vec![ - "https://github.com/acme/app/pull/183", - "https://github.com/acme/app/pull/184" - ] - ); - } - - #[test] - fn test_review_entries_from_notifications_uses_the_notification_url() { - let nodes = vec![ - json!({ - "__typename": "IssueNotification", - "url": "https://linear.app/acme/issue/LIN-1" - }), - json!({ - "__typename": "PullRequestNotification", - "url": "https://linear.app/acme/review/fix-the-thing-72e2bba2372a", - "pullRequest": { - "url": "https://github.com/acme/app/pull/183", - "number": 183, "status": "open", "title": "Fix the thing" - } - }), - json!({ - "__typename": "PullRequestNotification", - "url": "https://linear.app/acme/review/someone-elses-pr-aaaaaaaaaaaa", - "pullRequest": { "url": "https://github.com/acme/app/pull/999", "number": 999 } - }), - ]; - let wanted = vec!["https://github.com/acme/app/pull/183".to_string()]; - - let entries = review_entries_from_notifications(&nodes, &wanted); - - assert_eq!(entries.len(), 1, "only the requested pull request matches"); - assert_eq!( - entries[0]["reviewUrl"], "https://linear.app/acme/review/fix-the-thing-72e2bba2372a", - "the notification's URL is used verbatim, not reassembled from the slug" - ); - assert_eq!(entries[0]["number"], 183); - } - - #[test] - fn test_review_entries_from_notifications_drops_a_comment_anchor() { - let nodes = vec![json!({ - "__typename": "PullRequestNotification", - "url": "https://linear.app/acme/review/fix-the-thing-72e2bba2372a#comment-5f63aa7c", - "pullRequest": { "url": "https://github.com/acme/app/pull/183", "number": 183 } - })]; - let wanted = vec!["https://github.com/acme/app/pull/183".to_string()]; - - let entries = review_entries_from_notifications(&nodes, &wanted); - - assert_eq!( - entries[0]["reviewUrl"], "https://linear.app/acme/review/fix-the-thing-72e2bba2372a", - "a comment notification must still yield the review page URL" - ); - } - - #[test] - fn test_merge_review_entries_prefers_the_first_entry_per_pull_request() { - let merged = merge_review_entries(vec![ - json!({ "url": "https://github.com/acme/app/pull/1", "reviewUrl": "from-notification" }), - json!({ "url": "https://github.com/acme/app/pull/1", "reviewUrl": "from-agent-session" }), - json!({ "url": "https://github.com/acme/app/pull/2", "reviewUrl": "other" }), - ]); - - assert_eq!(merged.len(), 2); - assert_eq!(merged[0]["reviewUrl"], "from-notification"); - assert_eq!(merged[1]["reviewUrl"], "other"); - } - - #[test] - fn test_review_entries_builds_review_url_from_slug() { - let issue = issue_with_sessions(json!([ - { "pullRequests": { "nodes": [pr_link("7ffd27854fd2", 183)] } } - ])); - - let entries = review_entries("acme", &issue); - - assert_eq!(entries.len(), 1); - assert_eq!( - entries[0]["reviewUrl"], - "https://linear.app/acme/review/7ffd27854fd2" - ); - assert_eq!(entries[0]["number"], 183); - assert_eq!(entries[0]["url"], "https://github.com/acme/app/pull/183"); - } - - #[test] - fn test_review_entries_dedupes_a_pr_linked_by_several_sessions() { - let issue = issue_with_sessions(json!([ - { "pullRequests": { "nodes": [pr_link("aaa111", 7)] } }, - { "pullRequests": { "nodes": [pr_link("aaa111", 7), pr_link("bbb222", 8)] } } - ])); - - let entries = review_entries("acme", &issue); - - assert_eq!(entries.len(), 2, "the repeated pull request is listed once"); - assert_eq!( - entries[0]["reviewUrl"], - "https://linear.app/acme/review/aaa111" - ); - assert_eq!( - entries[1]["reviewUrl"], - "https://linear.app/acme/review/bbb222" - ); - } - - #[test] - fn test_review_entries_empty_without_sessions_or_slug() { - assert!(review_entries("acme", &issue_with_sessions(json!([]))).is_empty()); - - // A session with no linked pull request, and a link whose slug is missing or - // blank: all unresolvable, and none of them may produce a bogus URL. - let unresolvable = issue_with_sessions(json!([ - { "pullRequests": { "nodes": [] } }, - { "pullRequests": { "nodes": [{ "pullRequest": { "number": 1 } }] } }, - { "pullRequests": { "nodes": [{ "pullRequest": { "slugId": "", "number": 2 } }] } } - ])); - assert!(review_entries("acme", &unresolvable).is_empty()); - } - #[test] fn test_generate_branch_name_simple() { assert_eq!( diff --git a/src/commands/git/review_url.rs b/src/commands/git/review_url.rs new file mode 100644 index 0000000..9f4dffd --- /dev/null +++ b/src/commands/git/review_url.rs @@ -0,0 +1,788 @@ +//! `linear git review-url` — resolve the Linear review page for an issue's pull +//! requests. +//! +//! Linear exposes a pull request's review URL in two places, and neither covers +//! every pull request on its own: +//! +//! * a `PullRequestNotification` carries the review URL whole +//! (`review/<title-slug>-<id>`), but only exists once the pull request has +//! produced notification-worthy activity; +//! * an agent session's `PullRequest.slugId` is enough to assemble a review URL, +//! but only pull requests an agent worked on have a session. +//! +//! So notifications are the primary source, agent sessions the fallback, and a +//! pull request neither source resolves is reported as unresolved rather than +//! guessed at. + +use anyhow::Result; +use serde::{Deserialize, Serialize}; +use serde_json::{json, Map, Value}; +use std::collections::{BTreeMap, BTreeSet}; + +use crate::api::LinearClient; +use crate::output::{print_json, OutputOptions}; +use crate::pagination::{paginate_until, PageFlow, PaginationOptions}; + +/// How many notifications `review-url` reads before falling through to agent +/// sessions. +/// +/// The feed has no server-side filter for pull requests, so it is walked +/// newest-first; a pull request whose last notification is older than this is +/// left to the agent-session path. +const NOTIFICATION_LIMIT: usize = 500; + +/// Notifications per request while walking the feed. +const NOTIFICATION_PAGE_SIZE: usize = 100; + +const ISSUE_QUERY: &str = r#" + query($id: String!) { + organization { urlKey } + issue(id: $id) { + identifier + attachments { nodes { url sourceType } } + agentSessions { + nodes { + pullRequests { + nodes { + pullRequest { slugId url number status title } + } + } + } + } + } + } +"#; + +const NOTIFICATIONS_QUERY: &str = r#" + query($first: Int, $after: String) { + notifications(first: $first, after: $after, includeArchived: true) { + pageInfo { hasNextPage endCursor } + nodes { + __typename + ... on PullRequestNotification { + url + pullRequest { url number status title } + } + } + } + } +"#; + +/// A Linear GraphQL connection, reduced to the nodes callers care about. +#[derive(Debug, Deserialize)] +// `#[serde(default)]` on `nodes` would otherwise pull a `T: Default` bound into +// the generated impl; only `Deserialize` is actually needed. +#[serde(bound(deserialize = "T: Deserialize<'de>"))] +struct NodeList<T> { + #[serde(default)] + nodes: Vec<T>, +} + +impl<T> Default for NodeList<T> { + fn default() -> Self { + Self { nodes: Vec::new() } + } +} + +#[derive(Debug, Default, Deserialize)] +#[serde(rename_all = "camelCase")] +struct Issue { + #[serde(default)] + identifier: Option<String>, + #[serde(default)] + attachments: NodeList<Attachment>, + #[serde(default)] + agent_sessions: NodeList<AgentSession>, +} + +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +struct Attachment { + #[serde(default)] + source_type: Option<String>, + #[serde(default)] + url: Option<String>, +} + +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +struct AgentSession { + #[serde(default)] + pull_requests: NodeList<AgentSessionPullRequest>, +} + +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +struct AgentSessionPullRequest { + #[serde(default)] + pull_request: Option<PullRequest>, +} + +/// A pull request as Linear returns it, on either source. +#[derive(Debug, Clone, Deserialize)] +#[serde(rename_all = "camelCase")] +struct PullRequest { + #[serde(default)] + slug_id: Option<String>, + #[serde(default)] + url: Option<String>, + #[serde(default)] + number: Option<i64>, + #[serde(default)] + status: Option<String>, + #[serde(default)] + title: Option<String>, +} + +/// A node from the notification feed. The feed is heterogeneous, so everything +/// but `__typename` is optional and non-pull-request nodes are dropped. +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +struct Notification { + #[serde(rename = "__typename", default)] + typename: Option<String>, + #[serde(default)] + url: Option<String>, + #[serde(default)] + pull_request: Option<PullRequest>, +} + +/// One pull request that resolved to a review page. +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +#[serde(rename_all = "camelCase")] +struct ReviewEntry { + review_url: String, + number: Option<i64>, + status: Option<String>, + title: Option<String>, + /// The GitHub pull request URL; the identity a merge keys on. + url: String, +} + +impl ReviewEntry { + /// Build an entry for `pr` at an already-resolved `review_url`. + /// + /// A pull request Linear returned without a URL has no identity to merge or + /// report on, so it yields nothing rather than an entry with a null `url`. + fn new(review_url: String, pr: &PullRequest) -> Option<Self> { + let url = pr.url.clone().filter(|u| !u.is_empty())?; + Some(Self { + review_url, + number: pr.number, + status: pr.status.clone(), + title: pr.title.clone(), + url, + }) + } +} + +/// The outcome of resolving an issue's pull requests. +/// +/// Unresolved pull requests are part of the output contract, not a silent +/// omission: a caller that resolved two of three attached pull requests can see +/// which one it did not get. +#[derive(Debug, Default, PartialEq, Eq, Serialize)] +#[serde(rename_all = "camelCase")] +struct Resolution { + resolved: Vec<ReviewEntry>, + /// GitHub pull request URLs attached to the issue that neither source resolved. + unresolved: Vec<String>, +} + +impl Resolution { + /// Merge both sources into one entry per pull request. + /// + /// `fallback` (agent sessions) goes in first so a `primary` (notification) + /// result — which carries the review URL whole rather than assembling it — + /// replaces it. Entries are reported in `pr_urls` order, which is the order + /// Linear lists the issue's attachments; a pull request an agent session + /// linked without a corresponding attachment follows them. + fn merge(pr_urls: &[String], fallback: Vec<ReviewEntry>, primary: Vec<ReviewEntry>) -> Self { + let mut by_pr_url: BTreeMap<String, ReviewEntry> = BTreeMap::new(); + for entry in fallback.into_iter().chain(primary) { + by_pr_url.insert(entry.url.clone(), entry); + } + + let mut resolved = Vec::with_capacity(by_pr_url.len()); + let mut unresolved = Vec::new(); + for url in pr_urls { + match by_pr_url.remove(url) { + Some(entry) => resolved.push(entry), + None => unresolved.push(url.clone()), + } + } + resolved.extend(by_pr_url.into_values()); + + Self { + resolved, + unresolved, + } + } +} + +impl Issue { + /// GitHub pull request URLs attached to the issue. + /// + /// Linear links a pull request to an issue as a `github` attachment as soon + /// as it detects the branch, so this is the full set of the issue's pull + /// requests — but the attachment carries no review slug, which is why the + /// review URL is looked up separately. + fn attached_pr_urls(&self) -> Vec<String> { + let mut urls: Vec<String> = Vec::new(); + for attachment in &self.attachments.nodes { + if attachment.source_type.as_deref() != Some("github") { + continue; + } + let Some(url) = attachment.url.as_deref().filter(|u| u.contains("/pull/")) else { + continue; + }; + if !urls.iter().any(|existing| existing == url) { + urls.push(url.to_string()); + } + } + urls + } + + /// Review entries assembled from the issue's agent sessions. + /// + /// This is the fallback for a pull request with no notification: a session + /// exposes `PullRequest.slugId`, from which the review URL can be built. + fn agent_session_entries(&self, url_key: &str) -> Vec<ReviewEntry> { + let mut entries = Vec::new(); + for session in &self.agent_sessions.nodes { + for link in &session.pull_requests.nodes { + let Some(pr) = link.pull_request.as_ref() else { + continue; + }; + let Some(slug) = pr.slug_id.as_deref().filter(|s| !s.is_empty()) else { + continue; + }; + let review_url = format!("https://linear.app/{}/review/{}", url_key, slug); + if let Some(entry) = ReviewEntry::new(review_url, pr) { + entries.push(entry); + } + } + } + entries + } +} + +/// Pick out review entries for `pr_urls` from a page of notification nodes. +fn review_entries_from_notifications(nodes: &[Value], pr_urls: &[String]) -> Vec<ReviewEntry> { + let mut entries = Vec::new(); + for node in nodes { + let Ok(notification) = serde_json::from_value::<Notification>(node.clone()) else { + continue; + }; + if notification.typename.as_deref() != Some("PullRequestNotification") { + continue; + } + let Some(pr) = notification.pull_request.as_ref() else { + continue; + }; + if !pr + .url + .as_deref() + .is_some_and(|url| pr_urls.iter().any(|wanted| wanted == url)) + { + continue; + } + let Some(review_url) = notification.url.as_deref().filter(|u| !u.is_empty()) else { + continue; + }; + // A comment notification points at an anchor within the review page + // (`…#comment-<id>`); the page itself is what a caller wants. + let review_url = review_url.split('#').next().unwrap_or(review_url); + if let Some(entry) = ReviewEntry::new(review_url.to_string(), pr) { + entries.push(entry); + } + } + entries +} + +/// Accumulates review entries across pages of the notification feed. +/// +/// The feed is newest-first and a pull request can appear on it many times, so +/// the first entry seen for a pull request is its current one and later pages +/// must not displace it. Once every wanted pull request has an entry there is +/// nothing left to look for, which is what lets the walk stop early. +#[derive(Debug, Default)] +struct NotificationScan { + entries: Vec<ReviewEntry>, + seen_pr_urls: BTreeSet<String>, +} + +impl NotificationScan { + /// Take one page of notification nodes, and report whether to read another. + fn absorb(&mut self, nodes: &[Value], pr_urls: &[String]) -> PageFlow { + for entry in review_entries_from_notifications(nodes, pr_urls) { + if self.seen_pr_urls.insert(entry.url.clone()) { + self.entries.push(entry); + } + } + + if self.seen_pr_urls.len() >= pr_urls.len() { + PageFlow::Stop + } else { + PageFlow::Continue + } + } +} + +/// Walk the notification feed looking for the review URLs of `pr_urls`. +async fn notification_entries( + client: &LinearClient, + pr_urls: &[String], +) -> Result<Vec<ReviewEntry>> { + let options = PaginationOptions { + limit: Some(NOTIFICATION_LIMIT), + page_size: Some(NOTIFICATION_PAGE_SIZE), + ..Default::default() + }; + + let mut scan = NotificationScan::default(); + paginate_until( + client, + NOTIFICATIONS_QUERY, + Map::new(), + &["data", "notifications", "nodes"], + &["data", "notifications", "pageInfo"], + &options, + NOTIFICATION_PAGE_SIZE, + |nodes| scan.absorb(&nodes, pr_urls), + ) + .await?; + + Ok(scan.entries) +} + +/// Why `identifier` has no review URL, given the pull requests attached to it. +fn nothing_resolved_error(identifier: &str, pr_urls: &[String]) -> anyhow::Error { + let reason = if pr_urls.is_empty() { + "no pull request is linked to this issue".to_string() + } else { + format!( + "Linear exposes a review URL through pull request notifications, and none of the {} \ + linked pull request(s) has one in the last {} notifications", + pr_urls.len(), + NOTIFICATION_LIMIT + ) + }; + anyhow::anyhow!( + "No review URL for {}: {}. Use the GitHub PR URL instead.", + identifier, + reason + ) +} + +pub async fn show_review_url(issue_id: &str, output: &OutputOptions) -> Result<()> { + let client = LinearClient::new()?; + let result = client + .query(ISSUE_QUERY, Some(json!({ "id": issue_id }))) + .await?; + + if result["data"]["issue"].is_null() { + anyhow::bail!("Issue not found: {}", issue_id); + } + + let issue: Issue = serde_json::from_value(result["data"]["issue"].clone())?; + let url_key = result["data"]["organization"]["urlKey"] + .as_str() + .unwrap_or_default(); + + let pr_urls = issue.attached_pr_urls(); + let notifications = if pr_urls.is_empty() { + Vec::new() + } else { + notification_entries(&client, &pr_urls).await? + }; + + let resolution = Resolution::merge( + &pr_urls, + issue.agent_session_entries(url_key), + notifications, + ); + + if resolution.resolved.is_empty() { + let identifier = issue.identifier.as_deref().unwrap_or(issue_id); + return Err(nothing_resolved_error(identifier, &pr_urls)); + } + + if output.is_json() || output.has_template() { + return print_json(&json!(resolution), output); + } + + for entry in &resolution.resolved { + println!("{}", entry.review_url); + } + + // A partly resolved issue still prints what it has, but never silently: the + // pull requests it could not resolve are named on stderr so a caller reading + // stdout does not mistake the list for the whole set. + if !resolution.unresolved.is_empty() { + eprintln!( + "warning: no review URL for {} of the issue's pull request(s):", + resolution.unresolved.len() + ); + for url in &resolution.unresolved { + eprintln!(" {}", url); + } + } + + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn issue(value: Value) -> Issue { + serde_json::from_value(value).expect("issue fixture must deserialize") + } + + fn issue_with_sessions(sessions: Value) -> Issue { + issue(json!({ "identifier": "LIN-123", "agentSessions": { "nodes": sessions } })) + } + + fn pr_link(slug: &str, number: u64) -> Value { + json!({ "pullRequest": { + "slugId": slug, + "number": number, + "status": "open", + "url": format!("https://github.com/acme/app/pull/{}", number), + "title": "Fix the thing" + }}) + } + + fn entry(review_url: &str, pr_url: &str) -> ReviewEntry { + ReviewEntry { + review_url: review_url.to_string(), + number: None, + status: None, + title: None, + url: pr_url.to_string(), + } + } + + #[test] + fn test_attached_pr_urls_keeps_github_pull_requests_only() { + let issue = issue(json!({ "attachments": { "nodes": [ + { "sourceType": "github", "url": "https://github.com/acme/app/pull/183" }, + { "sourceType": "github", "url": "https://github.com/acme/app/pull/183" }, + { "sourceType": "github", "url": "https://github.com/acme/app/issues/12" }, + { "sourceType": "sentry", "url": "https://sentry.io/acme/app/pull/1" }, + { "sourceType": "github", "url": "https://github.com/acme/app/pull/184" } + ]}})); + + assert_eq!( + issue.attached_pr_urls(), + vec![ + "https://github.com/acme/app/pull/183", + "https://github.com/acme/app/pull/184" + ] + ); + } + + #[test] + fn test_review_entries_from_notifications_uses_the_notification_url() { + let nodes = vec![ + json!({ + "__typename": "IssueNotification", + "url": "https://linear.app/acme/issue/LIN-1" + }), + json!({ + "__typename": "PullRequestNotification", + "url": "https://linear.app/acme/review/fix-the-thing-72e2bba2372a", + "pullRequest": { + "url": "https://github.com/acme/app/pull/183", + "number": 183, "status": "open", "title": "Fix the thing" + } + }), + json!({ + "__typename": "PullRequestNotification", + "url": "https://linear.app/acme/review/someone-elses-pr-aaaaaaaaaaaa", + "pullRequest": { "url": "https://github.com/acme/app/pull/999", "number": 999 } + }), + ]; + let wanted = vec!["https://github.com/acme/app/pull/183".to_string()]; + + let entries = review_entries_from_notifications(&nodes, &wanted); + + assert_eq!(entries.len(), 1, "only the requested pull request matches"); + assert_eq!( + entries[0].review_url, "https://linear.app/acme/review/fix-the-thing-72e2bba2372a", + "the notification's URL is used verbatim, not reassembled from the slug" + ); + assert_eq!(entries[0].number, Some(183)); + } + + #[test] + fn test_review_entries_from_notifications_drops_a_comment_anchor() { + let nodes = vec![json!({ + "__typename": "PullRequestNotification", + "url": "https://linear.app/acme/review/fix-the-thing-72e2bba2372a#comment-5f63aa7c", + "pullRequest": { "url": "https://github.com/acme/app/pull/183", "number": 183 } + })]; + let wanted = vec!["https://github.com/acme/app/pull/183".to_string()]; + + let entries = review_entries_from_notifications(&nodes, &wanted); + + assert_eq!( + entries[0].review_url, "https://linear.app/acme/review/fix-the-thing-72e2bba2372a", + "a comment notification must still yield the review page URL" + ); + } + + #[test] + fn test_review_entries_from_notifications_skips_a_pull_request_without_a_url() { + // Nothing to key or report on, so it must not become an entry. + let nodes = vec![json!({ + "__typename": "PullRequestNotification", + "url": "https://linear.app/acme/review/fix-the-thing-72e2bba2372a", + "pullRequest": { "number": 183 } + })]; + let wanted = vec!["https://github.com/acme/app/pull/183".to_string()]; + + assert!(review_entries_from_notifications(&nodes, &wanted).is_empty()); + } + + #[test] + fn test_merge_prefers_the_notification_entry_per_pull_request() { + let pr = "https://github.com/acme/app/pull/1"; + let other = "https://github.com/acme/app/pull/2"; + + let merged = Resolution::merge( + &[pr.to_string(), other.to_string()], + vec![ + entry("from-agent-session", pr), + entry("other-from-agent-session", other), + ], + vec![entry("from-notification", pr)], + ); + + assert_eq!( + merged, + Resolution { + resolved: vec![ + entry("from-notification", pr), + entry("other-from-agent-session", other) + ], + unresolved: vec![], + } + ); + } + + #[test] + fn test_merge_reports_an_attached_pull_request_neither_source_resolved() { + let resolved_pr = "https://github.com/acme/app/pull/1"; + let unresolved_pr = "https://github.com/acme/app/pull/2"; + + let merged = Resolution::merge( + &[resolved_pr.to_string(), unresolved_pr.to_string()], + vec![], + vec![entry("from-notification", resolved_pr)], + ); + + assert_eq!( + merged, + Resolution { + resolved: vec![entry("from-notification", resolved_pr)], + unresolved: vec![unresolved_pr.to_string()], + }, + "a partial resolution must name the pull request it could not resolve" + ); + } + + #[test] + fn test_merge_reports_in_attachment_order_then_unattached_pull_requests() { + let second = "https://github.com/acme/app/pull/99"; + let first = "https://github.com/acme/app/pull/183"; + let unattached = "https://github.com/acme/app/pull/7"; + + let merged = Resolution::merge( + // Attachment order, which is not the order the entries arrive in and + // not lexicographic by URL. + &[first.to_string(), second.to_string()], + vec![entry("c", unattached)], + vec![entry("b", second), entry("a", first)], + ); + + assert_eq!( + merged.resolved, + vec![ + entry("a", first), + entry("b", second), + entry("c", unattached) + ] + ); + assert!(merged.unresolved.is_empty()); + } + + #[test] + fn test_agent_session_entries_builds_review_url_from_slug() { + let issue = issue_with_sessions(json!([ + { "pullRequests": { "nodes": [pr_link("7ffd27854fd2", 183)] } } + ])); + + let entries = issue.agent_session_entries("acme"); + + assert_eq!(entries.len(), 1); + assert_eq!( + entries[0].review_url, + "https://linear.app/acme/review/7ffd27854fd2" + ); + assert_eq!(entries[0].number, Some(183)); + assert_eq!(entries[0].url, "https://github.com/acme/app/pull/183"); + } + + #[test] + fn test_agent_session_entries_dedupe_a_pr_linked_by_several_sessions() { + let issue = issue_with_sessions(json!([ + { "pullRequests": { "nodes": [pr_link("aaa111", 7)] } }, + { "pullRequests": { "nodes": [pr_link("aaa111", 7), pr_link("bbb222", 8)] } } + ])); + let pr_urls = vec![ + "https://github.com/acme/app/pull/7".to_string(), + "https://github.com/acme/app/pull/8".to_string(), + ]; + + let merged = Resolution::merge(&pr_urls, issue.agent_session_entries("acme"), vec![]); + + assert_eq!( + merged.resolved.len(), + 2, + "the repeated pull request is listed once" + ); + assert_eq!( + merged.resolved[0].review_url, + "https://linear.app/acme/review/aaa111" + ); + assert_eq!( + merged.resolved[1].review_url, + "https://linear.app/acme/review/bbb222" + ); + } + + #[test] + fn test_agent_session_entries_empty_without_sessions_or_slug() { + assert!(issue_with_sessions(json!([])) + .agent_session_entries("acme") + .is_empty()); + + // A session with no linked pull request, and a link whose slug is missing + // or blank: all unresolvable, and none of them may produce a bogus URL. + let unresolvable = issue_with_sessions(json!([ + { "pullRequests": { "nodes": [] } }, + { "pullRequests": { "nodes": [{ "pullRequest": { "number": 1 } }] } }, + { "pullRequests": { "nodes": [{ "pullRequest": { "slugId": "", "number": 2 } }] } } + ])); + assert!(unresolvable.agent_session_entries("acme").is_empty()); + } + + #[test] + fn test_resolution_serializes_both_halves() { + let resolution = Resolution::merge( + &[ + "https://github.com/acme/app/pull/1".to_string(), + "https://github.com/acme/app/pull/2".to_string(), + ], + vec![], + vec![ReviewEntry { + review_url: "https://linear.app/acme/review/fix-the-thing-72e2bba2372a".to_string(), + number: Some(1), + status: Some("open".to_string()), + title: Some("Fix the thing".to_string()), + url: "https://github.com/acme/app/pull/1".to_string(), + }], + ); + + assert_eq!( + json!(resolution), + json!({ + "resolved": [{ + "reviewUrl": "https://linear.app/acme/review/fix-the-thing-72e2bba2372a", + "number": 1, + "status": "open", + "title": "Fix the thing", + "url": "https://github.com/acme/app/pull/1" + }], + "unresolved": ["https://github.com/acme/app/pull/2"] + }) + ); + } + + #[test] + fn test_nothing_resolved_error_distinguishes_no_pull_request_from_no_review_url() { + assert!(nothing_resolved_error("LIN-123", &[]) + .to_string() + .contains("no pull request is linked to this issue")); + + let attached = vec!["https://github.com/acme/app/pull/1".to_string()]; + let message = nothing_resolved_error("LIN-123", &attached).to_string(); + assert!(message.contains("1 linked pull request(s)")); + assert!(message.contains(&NOTIFICATION_LIMIT.to_string())); + } + + fn notification(review_url: &str, pr_url: &str) -> Value { + json!({ + "__typename": "PullRequestNotification", + "url": review_url, + "pullRequest": { "url": pr_url } + }) + } + + #[test] + fn test_scan_stops_once_every_wanted_pull_request_is_found() { + let first = "https://github.com/acme/app/pull/1"; + let second = "https://github.com/acme/app/pull/2"; + let wanted = vec![first.to_string(), second.to_string()]; + let mut scan = NotificationScan::default(); + + assert_eq!( + scan.absorb(&[notification("review-1", first)], &wanted), + PageFlow::Continue, + "one of two found: the rest of the feed is still worth reading" + ); + assert_eq!( + scan.absorb(&[notification("review-2", second)], &wanted), + PageFlow::Stop, + "both found: no further page may be requested" + ); + assert_eq!( + scan.entries, + vec![entry("review-1", first), entry("review-2", second)] + ); + } + + #[test] + fn test_scan_keeps_the_newest_notification_per_pull_request() { + let pr = "https://github.com/acme/app/pull/1"; + let other = "https://github.com/acme/app/pull/2"; + let wanted = vec![pr.to_string(), other.to_string()]; + let mut scan = NotificationScan::default(); + + // The feed is newest-first, so a later page's older notification for the + // same pull request must not displace the one already held. + scan.absorb(&[notification("newest-review", pr)], &wanted); + scan.absorb(&[notification("older-review", pr)], &wanted); + + assert_eq!(scan.entries, vec![entry("newest-review", pr)]); + } + + #[test] + fn test_scan_ignores_a_page_with_nothing_wanted_on_it() { + let wanted = vec!["https://github.com/acme/app/pull/1".to_string()]; + let mut scan = NotificationScan::default(); + + let flow = scan.absorb( + &[notification( + "review-x", + "https://github.com/acme/app/pull/999", + )], + &wanted, + ); + + assert_eq!(flow, PageFlow::Continue); + assert!(scan.entries.is_empty()); + } +} diff --git a/src/commands/initiatives.rs b/src/commands/initiatives.rs index f4888d8..9b54bea 100644 --- a/src/commands/initiatives.rs +++ b/src/commands/initiatives.rs @@ -396,4 +396,3 @@ async fn delete_initiative(id: &str, force: bool) -> Result<()> { Ok(()) } - diff --git a/src/pagination.rs b/src/pagination.rs index 73f4008..db0b6bc 100644 --- a/src/pagination.rs +++ b/src/pagination.rs @@ -150,6 +150,128 @@ pub async fn paginate_nodes( Ok(items) } +/// Whether a short-circuiting paginator should ask for another page. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum PageFlow { + /// Read the next page, if the connection has one and the limit allows. + Continue, + /// Everything the caller wanted is in hand; stop without another request. + Stop, +} + +/// Walk a connection forward, letting the caller stop as soon as it has enough. +/// +/// Same cursor rules as [`paginate_nodes`], but each page is handed to +/// `accumulate` before the next request goes out and `accumulate` owns whatever +/// it collects. A caller scanning an unfilterable feed for a known set of +/// records therefore pays for the pages it actually needs rather than +/// `limit / page_size` of them every time. +/// +/// # Example +/// +/// ```ignore +/// let mut found = Vec::new(); +/// paginate_until( +/// &client, +/// query, +/// Map::new(), +/// &["data", "notifications", "nodes"], +/// &["data", "notifications", "pageInfo"], +/// &options, +/// 100, +/// |nodes| { +/// found.extend(nodes.into_iter().filter(is_wanted)); +/// if found.len() == wanted { PageFlow::Stop } else { PageFlow::Continue } +/// }, +/// ) +/// .await?; +/// ``` +#[allow(clippy::too_many_arguments)] +pub async fn paginate_until<F>( + client: &LinearClient, + query: &str, + base_variables: Map<String, Value>, + nodes_path: &[&str], + page_info_path: &[&str], + options: &PaginationOptions, + default_page_size: usize, + mut accumulate: F, +) -> Result<()> +where + F: FnMut(Vec<Value>) -> PageFlow, +{ + let limit = if options.all { None } else { options.limit }; + let page_size = options.effective_page_size(default_page_size); + let mut after = options.after.clone(); + let mut read: usize = 0; + + loop { + let batch_size = limit + .map(|l| l.saturating_sub(read).min(page_size)) + .unwrap_or(page_size) + .max(1); + + let mut page_vars = Map::with_capacity(base_variables.len() + 2); + page_vars.insert( + "first".to_string(), + Value::Number(serde_json::Number::from(batch_size as u64)), + ); + if let Some(ref cursor) = after { + page_vars.insert("after".to_string(), Value::String(cursor.clone())); + } + for (k, v) in &base_variables { + page_vars.insert(k.clone(), v.clone()); + } + + let result = client.query(query, Some(Value::Object(page_vars))).await?; + + let mut nodes = get_path(&result, nodes_path) + .and_then(|v| v.as_array()) + .cloned() + .unwrap_or_default(); + if nodes.is_empty() { + break; + } + if let Some(l) = limit { + nodes.truncate(l.saturating_sub(read)); + } + read += nodes.len(); + + if accumulate(nodes) == PageFlow::Stop { + break; + } + + if limit.is_some_and(|l| read >= l) { + break; + } + + // An unbounded request (no `--limit`, no `--all`) is one page, as in + // `paginate_nodes`. + if !options.all && options.limit.is_none() { + break; + } + + let Some(page_info) = get_path(&result, page_info_path).and_then(|v| v.as_object()) else { + break; + }; + if !page_info + .get("hasNextPage") + .and_then(|v| v.as_bool()) + .unwrap_or(false) + { + break; + } + // A connection that claims another page without handing back a cursor + // would otherwise reread the first page until the limit ran out. + let Some(cursor) = page_info.get("endCursor").and_then(|v| v.as_str()) else { + break; + }; + after = Some(cursor.to_string()); + } + + Ok(()) +} + /// Stream paginated results, calling a handler for each batch of nodes. /// /// This is memory-efficient for large exports because it processes each page