From 21c06c00e63af121dbb5eb2440fb2193ef83569f Mon Sep 17 00:00:00 2001 From: Julien Danjou Date: Wed, 30 Sep 2026 16:39:53 +0200 Subject: [PATCH] fix(stack): discover a stack's pull requests without the Search API `stack push`, `list`, `sync` and `checkout` found a stack's pull requests with `GET /search/issues`. Environments that only allow repository-scoped API paths answer that with a 403, so every remote stack command failed there. Claude Code on the web's GitHub proxy is one of them. Discovery now uses only `/repos/{owner}/{repo}/...` endpoints: 1. `GET /git/matching-refs/heads//` lists every branch under the stack prefix in one call, with no pagination. I checked this against a repository holding 1,308 matching branches. Only branches one segment below the prefix whose name ends in a Change-Id are kept, which is the shape `stack push` creates. 2. For each local commit whose Change-Id is on no live branch, the branch it would be pushed to (`/`) is added. This is how a merged PR is still found after GitHub deleted its branch. `stack sync` needs that PR to drop the commit. The merged state only counts when the PR's `head.sha` equals the local commit, and an unchanged commit has an unchanged title, so its slug is the branch it was pushed under. 3. `GET /pulls?head=owner:&state=all` runs once per branch. That filter reads the PR's recorded head, so it also returns closed and merged PRs whose branch is gone. I checked this on a merged PR whose branch had been deleted. 4. Results are sorted by `updated_at`, newest first, the same order the search used, then grouped by Change-Id as before. The author filter runs locally on `user.login`, case-insensitively like `author:` was. Why not one paginated `GET /pulls` listing filtered by head prefix? Its cost grows with the number of open PRs in the repository, not with the size of the stack. It also misses merged PRs, and those would still need step 2. The approach above costs one call plus one per branch. The old one cost one search call plus one per PR. Search is not kept as a fallback. The only thing it could still find is a merged PR whose branch is deleted and whose Change-Id is no longer in the local stack. Nothing reads that PR: orphans are reported only when open. Search also has its own small rate limit and an index that lags behind new PRs, which the repository endpoints do not. The payloads now come from the list endpoint. It carries everything downstream reads, including the native `stack` object, except `mergeable`. `stack list` therefore fetches each unmerged PR itself for its `(conflicting)` marker, as part of its CI and review fan-out. `stack open` skips that, as it skips the fan-out. Fixes Mergifyio/mergify-cli#1861 Co-Authored-By: Claude Opus 5.5 Change-Id: Ibd22adea3084f027d64718be8fa6f331a7cb9b9b --- crates/mergify-cli/src/main.rs | 24 +- crates/mergify-cli/tests/stack_list.rs | 230 ++++++ .../tests/stack_push_github_native.rs | 97 +-- crates/mergify-cli/tests/stack_sync.rs | 88 ++- crates/mergify-stack/src/commands/checkout.rs | 162 ++--- crates/mergify-stack/src/commands/list.rs | 24 +- crates/mergify-stack/src/commands/push.rs | 33 +- crates/mergify-stack/src/commands/sync.rs | 6 +- crates/mergify-stack/src/remote_changes.rs | 670 +++++++++++++----- 9 files changed, 977 insertions(+), 357 deletions(-) create mode 100644 crates/mergify-cli/tests/stack_list.rs diff --git a/crates/mergify-cli/src/main.rs b/crates/mergify-cli/src/main.rs index a499dabe..c0c31b72 100644 --- a/crates/mergify-cli/src/main.rs +++ b/crates/mergify-cli/src/main.rs @@ -212,8 +212,8 @@ enum NativeCommand { InternalStackLocalCommits(InternalStackLocalCommitsOpts), /// `_internal stack-remote-changes --github-server URL --token T /// --user U --repo R --stack-prefix P --author A` — Python - /// migration helper. Searches GitHub for the open + merged PRs - /// belonging to the stack, groups them by Change-Id, prints a + /// migration helper. Looks up the open + merged PRs on the + /// stack's live branches, groups them by Change-Id, prints a /// JSON array of `{change_id, pull}` records. Wire format is /// not stable. InternalStackRemoteChanges(InternalStackRemoteChangesOpts), @@ -2712,7 +2712,7 @@ fn run_native(cmd: NativeCommand) -> ExitCode { Ok(mergify_core::ExitCode::Success) } NativeCommand::InternalStackRemoteChanges(opts) => { - // Search GitHub for PRs belonging to the stack and + // Look up the PRs on the stack's live branches and // group them by Change-Id, printed as a JSON array of // `{change_id, pull}` records for whoever invoked this // hidden command. @@ -2734,6 +2734,7 @@ fn run_native(cmd: NativeCommand) -> ExitCode { &opts.repo, &opts.stack_prefix, Some(opts.author.as_str()), + &[], ) .await?; let json = serde_json::to_string(&changes).map_err(|e| { @@ -3068,11 +3069,10 @@ enum InternalSubcommand { /// extraction. Not a stable user-facing surface. #[command(name = "stack-local-commits")] StackLocalCommits(InternalStackLocalCommitsArgs), - /// Search GitHub for the open + merged PRs belonging to a - /// stack and group them by `Change-Id`. Used by the Python - /// side of `mergify stack ` during migration to - /// centralise the GitHub search + per-PR fetch + change-id - /// regrouping. Not a stable user-facing surface. + /// Look up the open + merged PRs on a stack's live branches + /// and group them by `Change-Id`. A debugging aid: it has no + /// local stack, so a merged PR whose branch was deleted is not + /// found. Not a stable user-facing surface. #[command(name = "stack-remote-changes")] StackRemoteChanges(InternalStackRemoteChangesArgs), /// Self-invocation target for the rebase-family stack @@ -3821,12 +3821,12 @@ struct InternalStackRemoteChangesArgs { /// Repository name. #[arg(long)] repo: String, - /// Stack branch prefix (e.g. `stack/main` — the search query - /// becomes `head:/`). + /// Stack branch prefix (e.g. `stack/main` — the branches + /// looked up are `/--`). #[arg(long = "stack-prefix")] stack_prefix: String, - /// PR author to filter on. Limits the search to PRs the - /// current user owns — `mergify stack` only manages its own. + /// PR author to filter on. Keeps only PRs the current user + /// opened — `mergify stack` only manages its own. #[arg(long)] author: String, } diff --git a/crates/mergify-cli/tests/stack_list.rs b/crates/mergify-cli/tests/stack_list.rs new file mode 100644 index 00000000..3b797cfa --- /dev/null +++ b/crates/mergify-cli/tests/stack_list.rs @@ -0,0 +1,230 @@ +//! End-to-end tests for `mergify stack list`. Spawns the real binary +//! against a wiremock GitHub server and a real git repo. + +use std::path::{Path, PathBuf}; +use std::process::Command; + +use wiremock::matchers::{method, path, query_param}; +use wiremock::{Mock, MockServer, ResponseTemplate}; + +fn mergify_binary() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_mergify")) +} + +fn isolated_git() -> Command { + let mut cmd = Command::new("git"); + cmd.env("GIT_CONFIG_GLOBAL", "/dev/null"); + cmd.env("GIT_CONFIG_NOSYSTEM", "1"); + cmd +} + +fn run_in(dir: &Path, args: &[&str]) { + let ok = isolated_git() + .arg("-C") + .arg(dir) + .args(args) + .status() + .unwrap() + .success(); + assert!(ok, "git -C {}: {args:?} failed", dir.display()); +} + +fn capture(dir: &Path, args: &[&str]) -> String { + let out = isolated_git() + .arg("-C") + .arg(dir) + .args(args) + .output() + .unwrap(); + String::from_utf8(out.stdout).unwrap().trim().to_string() +} + +/// `feature` branch with `n_commits` commits on top of a pushed +/// `main`, each carrying a Change-Id distinct in its first 8 hex — +/// the part a stack branch name carries. +fn build_stack_repo(n_commits: usize) -> (tempfile::TempDir, Vec<(String, String)>) { + let workdir = tempfile::tempdir().unwrap(); + let upstream = workdir.path().join("up.git"); + isolated_git() + .args([ + "init", + "-q", + "--bare", + "-b", + "main", + upstream.to_str().unwrap(), + ]) + .status() + .unwrap(); + let local = workdir.path().join("local"); + std::fs::create_dir(&local).unwrap(); + for args in [ + &["init", "-q", "-b", "main"][..], + &["config", "user.email", "t@e.com"], + &["config", "user.name", "T"], + ] { + run_in(&local, args); + } + std::fs::write(local.join("root.txt"), "root").unwrap(); + run_in(&local, &["add", "root.txt"]); + run_in(&local, &["commit", "-q", "-m", "root"]); + run_in( + &local, + &["remote", "add", "origin", upstream.to_str().unwrap()], + ); + run_in(&local, &["push", "-q", "origin", "main"]); + run_in(&local, &["remote", "set-head", "origin", "main"]); + run_in(&local, &["checkout", "-q", "-b", "feature"]); + + let mut commits = Vec::new(); + for i in 0..n_commits { + let label = (b'A' + u8::try_from(i).expect("test stack stays under 26 commits")) as char; + let fname = format!("{}.txt", label.to_lowercase()); + std::fs::write(local.join(&fname), format!("content {label}")).unwrap(); + run_in(&local, &["add", &fname]); + let cid = format!("I{:08x}{}", i + 1, "0".repeat(32)); + let msg = format!("Commit {label}\n\nChange-Id: {cid}"); + run_in(&local, &["commit", "-q", "-m", &msg]); + commits.push((capture(&local, &["rev-parse", "HEAD"]), cid)); + } + (workdir, commits) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn list_reads_mergeable_from_the_pull_request_itself() { + // Stack discovery goes through the pull-request *list* endpoint, + // which never carries `mergeable`. `stack list` has to ask the PR + // for it, or it could no longer flag a conflicting one. + let (work, commits) = build_stack_repo(2); + let local = work.path().join("local"); + let head_ref = format!("stack/tester/feature/commit--{}", &commits[0].1[1..9]); + + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path( + "/repos/myorg/myrepo/git/matching-refs/heads/stack/tester/feature/", + )) + .respond_with( + ResponseTemplate::new(200) + .set_body_json(serde_json::json!([{"ref": format!("refs/heads/{head_ref}")}])), + ) + .expect(1) + .mount(&server) + .await; + let listed = serde_json::json!({ + "number": 101, + "state": "open", + "draft": false, + "merged_at": null, + "title": "Commit A", + "user": {"login": "tester"}, + "updated_at": "2026-01-01T00:00:00Z", + "head": {"ref": head_ref, "sha": commits[0].0}, + "base": {"ref": "main"}, + "html_url": "https://github.com/myorg/myrepo/pull/101", + }); + Mock::given(method("GET")) + .and(path("/repos/myorg/myrepo/pulls")) + .and(query_param("head", format!("myorg:{head_ref}"))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([listed]))) + .expect(1) + .mount(&server) + .await; + // Commit B has no PR yet: its branch is looked up and holds none. + Mock::given(method("GET")) + .and(path("/repos/myorg/myrepo/pulls")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([]))) + .with_priority(10) + .expect(1) + .mount(&server) + .await; + let mut full = listed.clone(); + full["mergeable"] = serde_json::json!(false); + Mock::given(method("GET")) + .and(path("/repos/myorg/myrepo/pulls/101")) + .respond_with(ResponseTemplate::new(200).set_body_json(full)) + .expect(1) + .mount(&server) + .await; + mount_no_checks_or_reviews(&server, &commits[0].0, 101).await; + + let output = run_list(&local, &server.uri()); + assert!( + output.status.success(), + "stdout: {}\nstderr: {}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); + + let payload: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap(); + let entries: Vec<_> = payload["entries"] + .as_array() + .unwrap() + .iter() + .map(|e| { + ( + e["pull_number"].clone(), + e["status"].clone(), + e["mergeable"].clone(), + ) + }) + .collect(); + assert_eq!( + entries, + vec![ + ( + serde_json::json!(101), + serde_json::json!("open"), + serde_json::json!(false) + ), + ( + serde_json::Value::Null, + serde_json::json!("no_pr"), + serde_json::Value::Null + ), + ], + ); +} + +fn run_list(local: &Path, server_uri: &str) -> std::process::Output { + Command::new(mergify_binary()) + .args([ + "stack", + "list", + "--json", + "--trunk", + "origin/main", + "--author", + "tester", + "--branch-prefix", + "stack/tester", + // Bypass slug discovery — `origin` is a local tempdir path. + "--repository", + "myorg/myrepo", + ]) + .current_dir(local) + .env("MERGIFY_TOKEN", "test-token") + .env("MERGIFY_GITHUB_SERVER", server_uri) + .env("GIT_CONFIG_GLOBAL", "/dev/null") + .env("GIT_CONFIG_NOSYSTEM", "1") + .output() + .unwrap() +} + +/// The CI and review columns: nothing to report. +async fn mount_no_checks_or_reviews(server: &MockServer, head_sha: &str, number: u64) { + Mock::given(method("GET")) + .and(path(format!( + "/repos/myorg/myrepo/commits/{head_sha}/check-runs" + ))) + .respond_with( + ResponseTemplate::new(200).set_body_json(serde_json::json!({"check_runs": []})), + ) + .mount(server) + .await; + Mock::given(method("GET")) + .and(path(format!("/repos/myorg/myrepo/pulls/{number}/reviews"))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([]))) + .mount(server) + .await; +} diff --git a/crates/mergify-cli/tests/stack_push_github_native.rs b/crates/mergify-cli/tests/stack_push_github_native.rs index 94323467..2f0feff2 100644 --- a/crates/mergify-cli/tests/stack_push_github_native.rs +++ b/crates/mergify-cli/tests/stack_push_github_native.rs @@ -25,7 +25,7 @@ use std::path::{Path, PathBuf}; use std::process::Command; -use wiremock::matchers::{method, path as wm_path, path_regex}; +use wiremock::matchers::{method, path as wm_path, path_regex, query_param}; use wiremock::{Mock, MockServer, ResponseTemplate}; fn mergify_binary() -> PathBuf { @@ -120,18 +120,49 @@ fn build_stack_repo(n_commits: usize) -> (tempfile::TempDir, Vec) { (workdir, change_ids) } +/// Mock the stack discovery a push starts with: `pulls` are the PRs +/// already open, each listed as a live branch under the stack prefix +/// and returned by the lookup on its own `head.ref`. Any other lookup +/// — a local commit with no PR yet — finds nothing. Each PR is given +/// the `user.login` the push filters on (`--author tester`). +async fn mount_discovery(server: &MockServer, pulls: &[serde_json::Value]) { + let head_ref = |pull: &serde_json::Value| pull["head"]["ref"].as_str().unwrap().to_string(); + let refs: Vec<_> = pulls + .iter() + .map(|pull| serde_json::json!({"ref": format!("refs/heads/{}", head_ref(pull))})) + .collect(); + Mock::given(method("GET")) + .and(wm_path( + "/repos/myorg/myrepo/git/matching-refs/heads/stack/tester/feature/", + )) + .respond_with(ResponseTemplate::new(200).set_body_json(refs)) + .mount(server) + .await; + for pull in pulls { + let mut listed = pull.clone(); + listed["user"] = serde_json::json!({"login": "tester"}); + Mock::given(method("GET")) + .and(wm_path("/repos/myorg/myrepo/pulls")) + .and(query_param("head", format!("myorg:{}", head_ref(pull)))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([listed]))) + .mount(server) + .await; + } + Mock::given(method("GET")) + .and(wm_path("/repos/myorg/myrepo/pulls")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([]))) + .with_priority(10) + .mount(server) + .await; +} + /// Mock GitHub for a push that creates `pr_numbers.len()` brand-new -/// PRs: an empty search (nothing on the remote yet), a POST that -/// hands out the numbers in order, and the comment endpoints the -/// stack comment needs. +/// PRs: nothing on the remote yet, a POST that hands out the numbers +/// in order, and the comment endpoints the stack comment needs. async fn mock_github_creating(pr_numbers: &[u64]) -> MockServer { let server = MockServer::start().await; - Mock::given(method("GET")) - .and(wm_path("/search/issues")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({"items": []}))) - .mount(&server) - .await; + mount_discovery(&server, &[]).await; // One POST mock per PR, mounted newest-first so wiremock's // last-mounted-wins ordering hands out the numbers bottom-to-top @@ -198,22 +229,11 @@ async fn mock_github_updating_a_stacked_pair( head_sha: &str, ) -> MockServer { let server = MockServer::start().await; - Mock::given(method("GET")) - .and(wm_path("/search/issues")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ - "items": [{"number": 101}, {"number": 102}], - }))) - .mount(&server) - .await; - for (i, number) in [101_u64, 102].iter().enumerate() { - let (head, base) = if i == 0 { - (bottom_ref, "main") - } else { - (top_ref, top_base) - }; - Mock::given(method("GET")) - .and(wm_path(format!("/repos/myorg/myrepo/pulls/{number}"))) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + let pulls: Vec<_> = [(101_u64, bottom_ref, "main"), (102, top_ref, top_base)] + .iter() + .enumerate() + .map(|(i, (number, head, base))| { + serde_json::json!({ "number": number, "state": "open", "merged_at": null, @@ -224,9 +244,11 @@ async fn mock_github_updating_a_stacked_pair( "base": {"ref": base}, "html_url": format!("https://github.com/myorg/myrepo/pull/{number}"), "stack": {"id": 162_170, "number": 7, "position": i + 1, "size": 2}, - }))) - .mount(&server) - .await; + }) + }) + .collect(); + mount_discovery(&server, &pulls).await; + for number in [101_u64, 102] { Mock::given(method("GET")) .and(wm_path(format!( "/repos/myorg/myrepo/pulls/{number}/reviews" @@ -561,16 +583,9 @@ async fn a_registered_stack_down_to_one_live_pull_request_still_loses_its_commen // the removal at all. let (_work, local, change_ids, remote_head) = repo_with_pushed_branches(1, 1); let server = MockServer::start().await; - Mock::given(method("GET")) - .and(wm_path("/search/issues")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ - "items": [{"number": 101}], - }))) - .mount(&server) - .await; - Mock::given(method("GET")) - .and(wm_path("/repos/myorg/myrepo/pulls/101")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + mount_discovery( + &server, + &[serde_json::json!({ "number": 101, "state": "open", "merged_at": null, @@ -583,9 +598,9 @@ async fn a_registered_stack_down_to_one_live_pull_request_still_loses_its_commen // What is left of a stack that was bigger: GitHub keeps // the registration and infers the merged prefix itself. "stack": {"id": 162_170, "number": 7, "position": 1, "size": 1}, - }))) - .mount(&server) - .await; + })], + ) + .await; Mock::given(method("GET")) .and(wm_path("/repos/myorg/myrepo/pulls/101/reviews")) .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([]))) diff --git a/crates/mergify-cli/tests/stack_sync.rs b/crates/mergify-cli/tests/stack_sync.rs index b50fb6c5..a83781cc 100644 --- a/crates/mergify-cli/tests/stack_sync.rs +++ b/crates/mergify-cli/tests/stack_sync.rs @@ -105,12 +105,44 @@ async fn start_mock_with_no_prs() -> wiremock::MockServer { }))) .mount(&server) .await; + mount_discovery(&server, None).await; + server +} + +/// Mock the stack discovery. `merged` is a PR merged from the bottom +/// commit, whose branch GitHub then deleted — the usual state after a +/// merge: no branch is left under the stack prefix, and the PR is +/// only found by the branch name its local commit implies +/// (`commit--` for "Commit A"). Every other lookup finds nothing. +async fn mount_discovery(server: &wiremock::MockServer, merged: Option) { + use wiremock::matchers::{method, path, query_param}; + use wiremock::{Mock, ResponseTemplate}; + Mock::given(method("GET")) - .and(path("/search/issues")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({"items": []}))) - .mount(&server) + .and(path( + "/repos/myorg/myrepo/git/matching-refs/heads/stack/tester/feature/", + )) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([]))) + .mount(server) + .await; + if let Some(mut pull) = merged { + let head = format!("myorg:{}", pull["head"]["ref"].as_str().unwrap()); + pull["user"] = serde_json::json!({"login": "tester"}); + Mock::given(method("GET")) + .and(path("/repos/myorg/myrepo/pulls")) + .and(query_param("head", head)) + .and(query_param("state", "all")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([pull]))) + .expect(1) + .mount(server) + .await; + } + Mock::given(method("GET")) + .and(path("/repos/myorg/myrepo/pulls")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([]))) + .with_priority(10) + .mount(server) .await; - server } fn run_mergify(local: &Path, server_uri: &str, args: &[&str]) -> std::process::Output { @@ -168,8 +200,7 @@ async fn sync_dry_run_reports_up_to_date_when_no_merged_prs() { #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn sync_dry_run_lists_merged_commits() { - use wiremock::matchers::{method, path}; - use wiremock::{Mock, MockServer, ResponseTemplate}; + use wiremock::MockServer; let (work, commits) = build_stack_repo(2); let local = work.path().join("local"); @@ -177,26 +208,19 @@ async fn sync_dry_run_lists_merged_commits() { let first_cid = commits[0].1.clone(); let server = MockServer::start().await; - Mock::given(method("GET")) - .and(path("/search/issues")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ - "items": [{"number": 42}], - }))) - .mount(&server) - .await; - Mock::given(method("GET")) - .and(path("/repos/myorg/myrepo/pulls/42")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + mount_discovery( + &server, + Some(serde_json::json!({ "number": 42, "title": "Commit A", "state": "closed", "merged_at": "2025-01-01T00:00:00Z", - "head": {"sha": first_sha, "ref": format!("stack/tester/feature/feat-a--{}", &first_cid[1..9])}, + "head": {"sha": first_sha, "ref": format!("stack/tester/feature/commit--{}", &first_cid[1..9])}, "base": {"ref": "main"}, "html_url": "https://github.com/myorg/myrepo/pull/42", - }))) - .mount(&server) - .await; + })), + ) + .await; let output = run_mergify( &local, @@ -233,8 +257,7 @@ async fn sync_dry_run_lists_merged_commits() { /// remove — the abort we guard against with `--reapply-cherry-picks`. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn sync_drops_squash_merged_commit_with_matching_patch() { - use wiremock::matchers::{method, path}; - use wiremock::{Mock, MockServer, ResponseTemplate}; + use wiremock::MockServer; let (work, commits) = build_stack_repo(2); let local = work.path().join("local"); @@ -253,26 +276,19 @@ async fn sync_drops_squash_merged_commit_with_matching_patch() { run_in(&local, &["checkout", "-q", "feature"]); let server = MockServer::start().await; - Mock::given(method("GET")) - .and(path("/search/issues")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ - "items": [{"number": 42}], - }))) - .mount(&server) - .await; - Mock::given(method("GET")) - .and(path("/repos/myorg/myrepo/pulls/42")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + mount_discovery( + &server, + Some(serde_json::json!({ "number": 42, "title": "Commit A", "state": "closed", "merged_at": "2025-01-01T00:00:00Z", - "head": {"sha": a_sha, "ref": format!("stack/tester/feature/feat-a--{}", &a_cid[1..9])}, + "head": {"sha": a_sha, "ref": format!("stack/tester/feature/commit--{}", &a_cid[1..9])}, "base": {"ref": "main"}, "html_url": "https://github.com/myorg/myrepo/pull/42", - }))) - .mount(&server) - .await; + })), + ) + .await; let output = run_mergify( &local, diff --git a/crates/mergify-stack/src/commands/checkout.rs b/crates/mergify-stack/src/commands/checkout.rs index 016d3df3..0e1f3c8e 100644 --- a/crates/mergify-stack/src/commands/checkout.rs +++ b/crates/mergify-stack/src/commands/checkout.rs @@ -20,9 +20,11 @@ //! //! 1. Resolve the target into a stack branch (+ `owner/repo`, //! which a PR URL carries). -//! 2. Search GitHub for the stack's PRs (via +//! 2. Look up the stack's PRs (via //! [`crate::remote_changes::get_remote_changes`], with no -//! `author:` filter — a branch name is unique within a repo). +//! author filter — a branch name is unique within a repo). Only +//! open PRs make the chain, and an open PR's branch always +//! exists, so no local commits are needed to find them. //! 3. Link open PRs into a single chain via their `head.ref` → //! `base.ref` pointers, find the root (the PR whose `base.ref` //! is *outside* the stack — i.e. doesn't start with the stack @@ -192,7 +194,8 @@ pub async fn run(opts: &Options<'_>) -> Result { }; let remote_changes = - remote_changes::get_remote_changes(opts.client, &owner, &repo, &stack_branch, None).await?; + remote_changes::get_remote_changes(opts.client, &owner, &repo, &stack_branch, None, &[]) + .await?; let chain = build_chain(&remote_changes, &stack_branch)?; if chain.is_empty() { @@ -458,17 +461,10 @@ mod tests { async fn run_no_stacked_prs_returns_no_stacked_prs() { use mergify_core::ApiFlavor; use url::Url; - use wiremock::matchers::{method, path}; - use wiremock::{Mock, MockServer, ResponseTemplate}; + use wiremock::MockServer; let server = MockServer::start().await; - Mock::given(method("GET")) - .and(path("/search/issues")) - .respond_with( - ResponseTemplate::new(200).set_body_json(serde_json::json!({"items": []})), - ) - .mount(&server) - .await; + mount_discovery(&server, "stack/author/my-branch", &[]).await; let client = HttpClient::new( Url::parse(&server.uri()).unwrap(), @@ -496,46 +492,37 @@ mod tests { async fn run_dry_run_returns_chain_without_touching_git() { use mergify_core::ApiFlavor; use url::Url; - use wiremock::matchers::{method, path}; - use wiremock::{Mock, MockServer, ResponseTemplate}; + use wiremock::MockServer; let server = MockServer::start().await; - Mock::given(method("GET")) - .and(path("/search/issues")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ - "items": [{"number": 1}, {"number": 2}], - }))) - .mount(&server) - .await; // Head refs use the new-format `--` shape so // `extract_from_branch_segment` accepts them and the // remote_changes pipeline doesn't filter them out. - Mock::given(method("GET")) - .and(path("/repos/user/repo/pulls/1")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ - "number": 1, - "title": "feat: A", - "html_url": "https://github.com/user/repo/pull/1", - "state": "open", - "base": {"ref": "main"}, - "head": {"ref": "stack/author/my-branch/feat-a--aaaaaaaa"}, - "merged_at": null, - }))) - .mount(&server) - .await; - Mock::given(method("GET")) - .and(path("/repos/user/repo/pulls/2")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ - "number": 2, - "title": "feat: B", - "html_url": "https://github.com/user/repo/pull/2", - "state": "open", - "base": {"ref": "stack/author/my-branch/feat-a--aaaaaaaa"}, - "head": {"ref": "stack/author/my-branch/feat-b--bbbbbbbb"}, - "merged_at": null, - }))) - .mount(&server) - .await; + mount_discovery( + &server, + "stack/author/my-branch", + &[ + serde_json::json!({ + "number": 1, + "title": "feat: A", + "html_url": "https://github.com/user/repo/pull/1", + "state": "open", + "base": {"ref": "main"}, + "head": {"ref": "stack/author/my-branch/feat-a--aaaaaaaa"}, + "merged_at": null, + }), + serde_json::json!({ + "number": 2, + "title": "feat: B", + "html_url": "https://github.com/user/repo/pull/2", + "state": "open", + "base": {"ref": "stack/author/my-branch/feat-a--aaaaaaaa"}, + "head": {"ref": "stack/author/my-branch/feat-b--bbbbbbbb"}, + "merged_at": null, + }), + ], + ) + .await; let client = HttpClient::new( Url::parse(&server.uri()).unwrap(), @@ -577,22 +564,14 @@ mod tests { #[tokio::test] async fn run_strips_changeid_suffix_from_a_pasted_leaf_ref() { // The user pastes a leaf branch ref verbatim, current - // `--<8 hex>` naming included. The search has to be - // issued against the stack stem, not the leaf. + // `--<8 hex>` naming included. The branches have to be + // listed under the stack stem, not the leaf. use mergify_core::ApiFlavor; use url::Url; - use wiremock::matchers::{method, path, query_param_contains}; - use wiremock::{Mock, MockServer, ResponseTemplate}; + use wiremock::MockServer; let server = MockServer::start().await; - Mock::given(method("GET")) - .and(path("/search/issues")) - .and(query_param_contains("q", "head:stack/author/my-branch/")) - .respond_with( - ResponseTemplate::new(200).set_body_json(serde_json::json!({"items": []})), - ) - .mount(&server) - .await; + mount_discovery(&server, "stack/author/my-branch", &[]).await; let client = HttpClient::new( Url::parse(&server.uri()).unwrap(), @@ -719,7 +698,7 @@ mod tests { // the full root→leaf chain, not just that PR and below. use mergify_core::ApiFlavor; use url::Url; - use wiremock::matchers::{method, path, query_param}; + use wiremock::matchers::{method, path}; use wiremock::{Mock, MockServer, ResponseTemplate}; fn pr_body(number: u64, base: &str, head: &str) -> serde_json::Value { @@ -731,6 +710,7 @@ mod tests { "base": {"ref": base}, "head": {"ref": head}, "merged_at": null, + "user": {"login": "someone-else"}, }) } @@ -747,18 +727,10 @@ mod tests { ]; let server = MockServer::start().await; - // No `author:` qualifier — checkout reaches anyone's stack. - Mock::given(method("GET")) - .and(path("/search/issues")) - .and(query_param( - "q", - "repo:user/repo is:pull-request head:stack/author/my-branch/", - )) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ - "items": [{"number": 1}, {"number": 2}, {"number": 3}], - }))) - .mount(&server) - .await; + // No author filter — checkout reaches anyone's stack, so PRs + // opened by someone else still make the chain. + mount_discovery(&server, stack, &bodies).await; + // The PR URL is resolved to its stack branch first. for (i, body) in bodies.iter().enumerate() { Mock::given(method("GET")) .and(path(format!("/repos/user/repo/pulls/{}", i + 1))) @@ -807,6 +779,42 @@ mod tests { } } + /// Mount the stack discovery for `stack_branch`: each of `pulls` + /// listed as a live branch under it, and returned by the lookup + /// on its own `head.ref`. The branches must be listed exactly + /// once, under the stack stem. + async fn mount_discovery( + server: &wiremock::MockServer, + stack_branch: &str, + pulls: &[serde_json::Value], + ) { + use wiremock::matchers::{method, path, query_param}; + use wiremock::{Mock, ResponseTemplate}; + + let head_ref = |pull: &serde_json::Value| pull["head"]["ref"].as_str().unwrap().to_string(); + let refs: Vec<_> = pulls + .iter() + .map(|pull| serde_json::json!({"ref": format!("refs/heads/{}", head_ref(pull))})) + .collect(); + Mock::given(method("GET")) + .and(path(format!( + "/repos/user/repo/git/matching-refs/heads/{stack_branch}/" + ))) + .respond_with(ResponseTemplate::new(200).set_body_json(refs)) + .expect(1) + .mount(server) + .await; + for pull in pulls { + Mock::given(method("GET")) + .and(path("/repos/user/repo/pulls")) + .and(query_param("head", format!("user:{}", head_ref(pull)))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([pull]))) + .expect(1) + .mount(server) + .await; + } + } + /// Mount a two-PR stack under `stack_branch` and return the /// client pointed at it. The server has to outlive the call, /// so it comes back with the client. @@ -834,13 +842,9 @@ mod tests { ]; let server = MockServer::start().await; - Mock::given(method("GET")) - .and(path("/search/issues")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ - "items": [{"number": 1}, {"number": 2}], - }))) - .mount(&server) - .await; + mount_discovery(&server, stack_branch, &bodies).await; + // A pull-request URL target is resolved to its stack branch + // through the PR itself. for (i, body) in bodies.iter().enumerate() { Mock::given(method("GET")) .and(path(format!("/repos/user/repo/pulls/{}", i + 1))) diff --git a/crates/mergify-stack/src/commands/list.rs b/crates/mergify-stack/src/commands/list.rs index e151d06f..4cfd28ad 100644 --- a/crates/mergify-stack/src/commands/list.rs +++ b/crates/mergify-stack/src/commands/list.rs @@ -107,21 +107,22 @@ pub async fn run(opts: &Options<'_>) -> Result { ))); } + let local = local_commits::read(&repo_dir, &base_commit_sha, "HEAD")?; let remote_changes = remote_changes::get_remote_changes( opts.client, opts.user, opts.repo, &stack_prefix, Some(opts.author), + &local, ) .await?; - let local = local_commits::read(&repo_dir, &base_commit_sha, "HEAD")?; let classified = changes::classify(&local, remote_changes)?; let mut entries = Vec::with_capacity(classified.locals.len()); for local in &classified.locals { - let (status, pull_number, pull_url, mergeable) = match &local.pull { - None => ("no_pr".to_string(), None, None, None), + let (status, pull_number, pull_url) = match &local.pull { + None => ("no_pr".to_string(), None, None), Some(pull) => { let merged = pull.get("merged_at").is_some_and(|v| !v.is_null()); let draft = pull.get("draft").and_then(Value::as_bool).unwrap_or(false); @@ -137,8 +138,7 @@ pub async fn run(opts: &Options<'_>) -> Result { .get("html_url") .and_then(Value::as_str) .map(str::to_owned); - let mergeable = pull.get("mergeable").and_then(Value::as_bool); - (status.to_string(), pull_number, pull_url, mergeable) + (status.to_string(), pull_number, pull_url) } }; entries.push(StackListEntry { @@ -152,7 +152,7 @@ pub async fn run(opts: &Options<'_>) -> Result { ci_checks: Vec::new(), review_status: "unknown".to_string(), reviews: Vec::new(), - mergeable, + mergeable: None, }); } @@ -176,7 +176,10 @@ pub async fn run(opts: &Options<'_>) -> Result { /// Per-PR fan-out: for each entry with a pull, fetch /// `/check-runs` + `/reviews` and fold them into the entry's -/// `ci_*` / `review_*` fields. Sequential rather than concurrent +/// `ci_*` / `review_*` fields, plus the PR itself for `mergeable` +/// unless it merged — the discovery payloads come from the +/// pull-request list endpoint, which never carries it (`None` while +/// GitHub is still computing it). Sequential rather than concurrent /// since GitHub's secondary rate limit kicks in around ~80 /// concurrent calls on the same endpoint pool — and a typical /// stack has well under that. @@ -215,6 +218,13 @@ async fn fetch_pr_details( let (review_status, review_list) = compute_review_status(&reviews); entries[i].review_status = review_status.to_string(); entries[i].reviews = review_list; + + if entries[i].status != "merged" { + let full: Value = client + .get(&format!("/repos/{user}/{repo}/pulls/{pull_number}")) + .await?; + entries[i].mergeable = full.get("mergeable").and_then(Value::as_bool); + } } Ok(()) } diff --git a/crates/mergify-stack/src/commands/push.rs b/crates/mergify-stack/src/commands/push.rs index 1a017ab3..8953f948 100644 --- a/crates/mergify-stack/src/commands/push.rs +++ b/crates/mergify-stack/src/commands/push.rs @@ -126,7 +126,7 @@ pub struct Options<'a> { /// `(remote, branch)` of the trunk. pub trunk: (&'a str, &'a str), /// GitHub login of the author whose PRs the stack belongs - /// to. Used both for the search filter in `remote_changes` + /// to. Used both for the author filter in `remote_changes` /// and for `branch_prefix` fallback resolution. pub author: &'a str, /// PR branch prefix (typically `stack/`); empty @@ -203,7 +203,7 @@ pub async fn run(opts: &Options<'_>) -> Result { format!("{prefix}/{dest_branch}", prefix = opts.branch_prefix) }; - // The pre-flight — trunk + notes fetch, the PR search, the rebase + // The pre-flight — trunk + notes fetch, the PR lookup, the rebase // decision — is several network round-trips with nothing to show, // the stretch that used to look frozen. Drive one spinner row // through it, resolving to a kept summary line so it never blinks @@ -232,10 +232,15 @@ pub async fn run(opts: &Options<'_>) -> Result { let trunk_ref = format!("{remote}/{base_branch}"); let base_commit_sha = compute_base_commit_sha(&repo_dir, &trunk_ref, &dest_branch)?; - // `get_remote_changes` fetches each PR sequentially. Publish the - // number it's on into a shared cell; the spinner's tick loop reads - // it each frame so the label tracks each pull (owner/repo#id) - // while the glyph keeps spinning smoothly. + // Read before the discovery: a merged PR whose branch was deleted + // is only found under the branch name a local commit implies. + let local = local_commits::read(&repo_dir, &base_commit_sha, "HEAD")?; + + // `get_remote_changes` looks each stack branch up sequentially. + // Publish how far it is into a shared cell (`done << 32 | total`, + // one atomic so the pair is never torn); the spinner's tick loop + // reads it each frame so the label tracks the lookups while the + // glyph keeps spinning smoothly. let reading = Arc::new(AtomicU64::new(0)); let reading_writer = Arc::clone(&reading); let fetch_pulls = remote_changes::get_remote_changes_reporting( @@ -244,16 +249,22 @@ pub async fn run(opts: &Options<'_>) -> Result { opts.repo, &stack_prefix, Some(opts.author), - move |number| reading_writer.store(number, Ordering::Relaxed), + &local, + move |done, total| { + let packed = + (u64::try_from(done).unwrap_or(0) << 32) | u64::try_from(total).unwrap_or(0); + reading_writer.store(packed, Ordering::Relaxed); + }, ); let (owner, repo_name) = (opts.user, opts.repo); let remote_changes_data = prog .run_reporting(pf, "reading pull requests", fetch_pulls, move || { - let n = reading.load(Ordering::Relaxed); - (n != 0).then(|| format!("reading {owner}/{repo_name}#{n}")) + let packed = reading.load(Ordering::Relaxed); + let (done, total) = (packed >> 32, packed & u64::from(u32::MAX)); + (total != 0) + .then(|| format!("reading {owner}/{repo_name} pull requests ({done}/{total})")) }) .await?; - let local = local_commits::read(&repo_dir, &base_commit_sha, "HEAD")?; let planner_opts = PlannerOpts { stack_prefix: &stack_prefix, @@ -326,7 +337,7 @@ pub async fn run(opts: &Options<'_>) -> Result { quiet: true, // The pre-flight already fetched the trunk and the remote // PRs; hand them through so the rebase doesn't repeat the - // search + per-PR GETs. + // per-branch PR lookups. prefetched_remote_changes: Some(remote_changes_data.clone()), skip_trunk_fetch: true, }; diff --git a/crates/mergify-stack/src/commands/sync.rs b/crates/mergify-stack/src/commands/sync.rs index 2dcf7424..9cff75c2 100644 --- a/crates/mergify-stack/src/commands/sync.rs +++ b/crates/mergify-stack/src/commands/sync.rs @@ -63,7 +63,7 @@ pub struct Options<'a> { pub quiet: bool, /// Remote changes already fetched by the caller (the `stack push` /// pre-flight). When `Some`, `run` reuses them instead of - /// re-running the search + per-PR GETs that `get_remote_changes` + /// re-running the per-branch PR lookups that `get_remote_changes` /// does. `None` for the standalone `stack sync` command. pub prefetched_remote_changes: Option>, /// Skip the `git fetch ` pre-rebase. Set by @@ -96,6 +96,7 @@ pub async fn run(opts: &Options<'_>) -> Result { let trunk_ref = format!("{remote}/{base_branch}"); let base_commit_sha = compute_base_commit_sha(&repo_dir, &trunk_ref, &dest_branch)?; + let local = local_commits::read(&repo_dir, &base_commit_sha, "HEAD")?; let remote_changes = match &opts.prefetched_remote_changes { Some(changes) => changes.clone(), None => { @@ -105,12 +106,11 @@ pub async fn run(opts: &Options<'_>) -> Result { opts.repo, &stack_prefix, Some(opts.author), + &local, ) .await? } }; - - let local = local_commits::read(&repo_dir, &base_commit_sha, "HEAD")?; let status = sync_status::classify( dest_branch.clone(), trunk_ref.clone(), diff --git a/crates/mergify-stack/src/remote_changes.rs b/crates/mergify-stack/src/remote_changes.rs index 39d1719b..bfe935e8 100644 --- a/crates/mergify-stack/src/remote_changes.rs +++ b/crates/mergify-stack/src/remote_changes.rs @@ -1,126 +1,234 @@ //! Discover the open + recently merged PRs belonging to a stack. //! //! Every `mergify stack ` that needs to know which PRs are -//! already on GitHub for the current branch runs this: +//! already on GitHub for the current branch runs this. It only +//! calls repository-scoped endpoints (`/repos/{owner}/{repo}/...`): +//! some environments proxy the GitHub API and refuse everything +//! else (Claude Code on the web answers `/search/issues` with a +//! 403, Mergifyio/mergify-cli#1861), and the Search API has its +//! own small rate limit on top. //! -//! 1. `GET /search/issues?q=repo:owner/repo author:author -//! is:pull-request head:prefix/` — GitHub search, sorted by -//! `updated`, capped at 100 results (the same `per_page` cap -//! the Python CLI used). Only one search call regardless of -//! stack depth. -//! 2. For each result, `GET /repos/owner/repo/pulls/` to -//! fetch the full PR payload (the search index returns a -//! pared-down shape; downstream code needs `head.sha`, -//! `merged_at`, `draft`, etc.). -//! 3. Group by [`change_id::extract_from_branch_segment`] applied -//! to the last segment of `head.ref`. Closed-but-not-merged -//! PRs are dropped. When two PRs share the same Change-Id, -//! open beats closed — and two open PRs on the same Change-Id -//! is a hard error (the user has a duplicate that the rest of -//! the orchestration can't reconcile). +//! 1. `GET /repos/owner/repo/git/matching-refs/heads//` — +//! every branch that exists under the stack prefix, in one +//! unpaginated call. Kept: branches one segment below the prefix +//! whose last segment carries a Change-Id, i.e. the shape +//! `stack push` creates. +//! 2. Add the branch each local commit would be pushed to +//! (`/`) when no live branch carries its +//! Change-Id. That is how a merged PR is still found after its +//! head branch was deleted: `SkipMerged` needs the PR's +//! `head.sha` to equal the local commit, and a commit that +//! unchanged has the same title, so the same slug. A commit +//! reworded since has a different SHA and is re-created +//! whether its merged PR is found or not. +//! 3. For each of those branches, +//! `GET /repos/owner/repo/pulls?head=owner:&state=all`. +//! That filter reads the PR's recorded head, so it answers for +//! closed and merged PRs whose branch is gone. +//! 4. Sort everything by `updated_at`, newest first, and group by +//! [`change_id::extract_from_branch_segment`] applied to the +//! last segment of `head.ref`. Closed-but-not-merged PRs are +//! dropped. When two PRs share the same Change-Id, open beats +//! closed — and two open PRs on the same Change-Id is a hard +//! error (the user has a duplicate that the rest of the +//! orchestration can't reconcile). //! -//! The output is order-preserving — GitHub's `sort=updated` -//! ordering carries through to downstream consumers (the orphan -//! list in `changes.py`'s `get_changes` iterates this map and the -//! order matters for how orphan PRs are presented). +//! The payloads are the pull-request *list* shape: everything the +//! single-PR endpoint returns except the computed fields +//! (`mergeable`, `mergeable_state`, `merged`, diff stats). A +//! consumer that needs one of those fetches the PR itself. //! -//! Exposed via the hidden `_internal stack-remote-changes` -//! subcommand on the `mergify` binary as a no-op-from-Python's- -//! perspective: the Rust impl is callable but `changes.py` still -//! does the HTTP work itself today. Migration of the Python call -//! site lands in a follow-up PR — that change requires updating -//! roughly 50 `respx`-mocked tests across the `mergify_cli/tests/stack/` -//! tree to drive the bridge stub instead, which is too much -//! churn to bundle here. +//! What the Search API found and this does not: a merged PR whose +//! branch was deleted and whose Change-Id is no longer in the local +//! stack. Consumers only report *open* orphans, so nothing reads it. +//! +//! The output is order-preserving — the `updated_at` ordering +//! carries through to downstream consumers (the orphan list is +//! presented in that order). use mergify_core::http::Client; use mergify_core::{ApiFlavor, CliError}; use serde::{Deserialize, Serialize}; use crate::change_id; +use crate::local_commits::LocalCommit; -/// One `{change_id, pull}` entry in the order GitHub returned it -/// from `/search/issues`. Emitted as a flat array (not a JSON -/// object) so consumers can rebuild an order-preserving dict -/// without relying on serde's optional `preserve_order` feature. +/// One `{change_id, pull}` entry, most recently updated first. +/// Emitted as a flat array (not a JSON object) so consumers can +/// rebuild an order-preserving dict without relying on serde's +/// optional `preserve_order` feature. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct RemoteChange { pub change_id: String, - /// Raw PR payload from `/repos/owner/repo/pulls/`. - /// Passed through as a typeless JSON value — the downstream - /// Python orchestrator consumes many fields (`head.ref`, - /// `head.sha`, `state`, `draft`, `merged_at`, `merge_commit_sha`, - /// `html_url`, …); typing them all here would be a - /// translation tax with no real correctness win. + /// Raw PR payload from `/repos/owner/repo/pulls?head=...` (the + /// list shape — no `mergeable`). Passed through as a typeless + /// JSON value — the downstream orchestrator consumes many + /// fields (`head.ref`, `head.sha`, `state`, `draft`, + /// `merged_at`, `merge_commit_sha`, `html_url`, `stack`, …); + /// typing them all here would be a translation tax with no + /// real correctness win. pub pull: serde_json::Value, } -/// Run the search → fetch → group pipeline. +/// Run the list-branches → look-up → group pipeline. /// -/// `user` / `repo` form the search scope (`repo:user/repo`). -/// `stack_prefix` filters the search to PRs whose head branch -/// starts with `prefix/`. `author` restricts the search to PRs -/// one user opened — `Some(me)` for the commands that only ever -/// manage the local user's own stack, `None` for `stack checkout`, -/// which has to reach anyone's stack. Branch names are unique -/// within a repo, so the branch filter alone already pins the -/// result set. +/// `user` / `repo` name the repository the stack lives in. +/// `stack_prefix` is the branch prefix the stack's PR branches sit +/// under. `author` keeps only PRs one user opened — `Some(me)` for +/// the commands that only ever manage the local user's own stack, +/// `None` for `stack checkout`, which has to reach anyone's stack. +/// `local_commits` supplies the branch names a merged PR may still +/// be recorded under after its branch was deleted; pass `&[]` when +/// there is no local stack (then only live branches are searched). pub async fn get_remote_changes( client: &Client, user: &str, repo: &str, stack_prefix: &str, author: Option<&str>, + local_commits: &[LocalCommit], ) -> Result, CliError> { - get_remote_changes_reporting(client, user, repo, stack_prefix, author, |_| {}).await + get_remote_changes_reporting( + client, + user, + repo, + stack_prefix, + author, + local_commits, + |_, _| {}, + ) + .await } -/// Same as [`get_remote_changes`], but calls `on_pull` with each -/// pull-request number just before fetching it — so a caller can -/// surface per-pull progress through the otherwise-silent -/// sequential fetch loop. +/// Same as [`get_remote_changes`], but calls `on_lookup(n, total)` +/// just before the `n`-th (1-based) of `total` per-branch lookups — +/// so a caller can surface progress through the otherwise-silent +/// sequential loop. pub async fn get_remote_changes_reporting( client: &Client, user: &str, repo: &str, stack_prefix: &str, author: Option<&str>, - mut on_pull: impl FnMut(u64), + local_commits: &[LocalCommit], + mut on_lookup: impl FnMut(usize, usize), ) -> Result, CliError> { - // Omitted entirely rather than left empty when there's no - // author — GitHub rejects a bare `author:` qualifier. - let author_filter = author.map_or_else(String::new, |a| format!("author:{a} ")); - let q = format!("repo:{user}/{repo} {author_filter}is:pull-request head:{stack_prefix}/"); - let search: SearchResponse = client - .get_with_query( - "/search/issues", - &[("q", &q), ("per_page", "100"), ("sort", "updated")], - ) - .await?; - - // Per-PR fetches run sequentially. The Python orchestrator - // uses `asyncio.gather` for fan-out, but the underlying - // reqwest client we wrap doesn't memoize connections by - // host the same way httpx does — and GitHub's secondary - // rate-limit kicks in around ~80 concurrent requests on the - // search-resource pool. Sequential keeps it under the - // threshold and the latency budget is small (~50 PRs × ~100ms - // each = a few seconds for the largest realistic stacks). - let mut pulls: Vec = Vec::with_capacity(search.items.len()); - for item in &search.items { - on_pull(item.number); - let path = format!("/repos/{user}/{repo}/pulls/{number}", number = item.number); - let pull: serde_json::Value = client.get(&path).await?; - pulls.push(pull); + let live = live_stack_branches(client, user, repo, stack_prefix).await?; + let branches = branches_to_look_up(live, stack_prefix, local_commits); + + // Sequential on purpose: GitHub's secondary rate limit kicks in + // on bursts of concurrent requests, and a stack is a handful of + // branches (~100ms each). + let pulls_path = format!("/repos/{user}/{repo}/pulls"); + let mut pulls: Vec = Vec::new(); + for (idx, branch) in branches.iter().enumerate() { + on_lookup(idx + 1, branches.len()); + let head = format!("{user}:{branch}"); + let found: Vec = client + .get_with_query( + &pulls_path, + &[("head", &head), ("state", "all"), ("per_page", "100")], + ) + .await?; + pulls.extend( + found + .into_iter() + .filter(|pull| author.is_none_or(|a| opened_by(pull, a))), + ); } + // ISO-8601 UTC timestamps sort chronologically as strings. A + // stable sort keeps lookup order between equal timestamps. + pulls.sort_by(|a, b| updated_at(b).cmp(updated_at(a))); group_by_change_id(pulls) } +/// Branches that exist right now one segment below `stack_prefix` +/// and end in a Change-Id — the branches `stack push` creates. +/// A nested stack (`//...`) or a hand-made branch +/// under the prefix is not this stack's. +async fn live_stack_branches( + client: &Client, + user: &str, + repo: &str, + stack_prefix: &str, +) -> Result, CliError> { + // The trailing slash matters: `matching-refs` is a plain string + // prefix match, so without it `stack/foo` would also list + // `stack/foobar/...`. + let path = format!("/repos/{user}/{repo}/git/matching-refs/heads/{stack_prefix}/"); + let refs: Vec = client.get(&path).await?; + let ref_prefix = format!("refs/heads/{stack_prefix}/"); + Ok(refs + .into_iter() + .filter_map(|r| { + let leaf = r.ref_name.strip_prefix(&ref_prefix)?; + let managed = + !leaf.contains('/') && change_id::extract_from_branch_segment(leaf).is_some(); + managed.then(|| format!("{stack_prefix}/{leaf}")) + }) + .collect()) +} + +/// `live`, plus the branch each local commit would be pushed to +/// when no live branch already carries its Change-Id. +fn branches_to_look_up( + live: Vec, + stack_prefix: &str, + local_commits: &[LocalCommit], +) -> Vec { + let live_change_ids: Vec = live + .iter() + .filter_map(|b| change_id::extract_from_branch_segment(last_segment(b))) + .map(str::to_owned) + .collect(); + let mut branches = live; + for local in local_commits { + if live_change_ids + .iter() + .any(|remote| same_change(remote, &local.change_id)) + { + continue; + } + let expected = format!("{stack_prefix}/{slug}", slug = local.slug); + if !branches.contains(&expected) { + branches.push(expected); + } + } + branches +} + +/// Whether a branch's Change-Id (the short 8-hex form or the full +/// `I` + 40 hex) and a local commit's full Change-Id name the same +/// change. The same prefix comparison the classifiers pair commits +/// with PRs by. +fn same_change(remote: &str, local: &str) -> bool { + let remote_hex = remote.strip_prefix('I').unwrap_or(remote); + let local_hex = local.strip_prefix('I').unwrap_or(local); + remote_hex.starts_with(local_hex) || local_hex.starts_with(remote_hex) +} + +fn last_segment(branch: &str) -> &str { + branch.rsplit('/').next().unwrap_or(branch) +} + +/// GitHub logins are case-insensitive, as the Search API's +/// `author:` qualifier this replaces was. +fn opened_by(pull: &serde_json::Value, author: &str) -> bool { + pull.pointer("/user/login") + .and_then(serde_json::Value::as_str) + .is_some_and(|login| login.eq_ignore_ascii_case(author)) +} + +fn updated_at(pull: &serde_json::Value) -> &str { + pull.get("updated_at") + .and_then(serde_json::Value::as_str) + .unwrap_or("") +} + /// Pure, network-free regrouping of the per-PR payloads. Exposed -/// so the search + fetch + group split can be unit-tested in -/// isolation without spinning up a mock server for every parser -/// edge case. +/// so the lookup + group split can be unit-tested in isolation +/// without spinning up a mock server for every parser edge case. fn group_by_change_id(pulls: Vec) -> Result, CliError> { let mut out: Vec = Vec::new(); for pull in pulls { @@ -139,8 +247,7 @@ fn group_by_change_id(pulls: Vec) -> Result .ok_or_else(|| { CliError::GitHubApi("PR payload missing required `head.ref` field".to_string()) })?; - let last_segment = head_ref.rsplit('/').next().unwrap_or(head_ref); - let Some(change_id) = change_id::extract_from_branch_segment(last_segment) else { + let Some(change_id) = change_id::extract_from_branch_segment(last_segment(head_ref)) else { continue; }; let change_id = change_id.to_string(); @@ -165,8 +272,8 @@ fn group_by_change_id(pulls: Vec) -> Result ))); } // Open-existing + closed-new, or both closed: - // keep the existing entry. GitHub returned the - // search sorted by `updated`, so the first-seen + // keep the existing entry. The input is sorted by + // `updated_at`, newest first, so the first-seen // one is the most recent. _ => {} } @@ -178,13 +285,9 @@ fn group_by_change_id(pulls: Vec) -> Result } #[derive(Debug, Deserialize)] -struct SearchResponse { - items: Vec, -} - -#[derive(Debug, Deserialize)] -struct SearchItem { - number: u64, +struct GitRef { + #[serde(rename = "ref")] + ref_name: String, } /// Build an [`ApiFlavor::GitHub`] client pre-configured for the @@ -198,6 +301,10 @@ pub fn default_client(github_server: url::Url, token: &str) -> Result) -> serde_json::Value { serde_json::json!({ @@ -257,9 +364,9 @@ mod tests { #[test] fn group_drops_pr_whose_branch_has_no_recognisable_change_id() { // Some user manually pushed a branch under the prefix - // without going through `mergify stack`. The search query - // can still surface it (the prefix substring matches); - // dropping it keeps the result hermetic to managed PRs. + // without going through `mergify stack`, and opened a PR + // on it. Dropping it keeps the result hermetic to managed + // PRs. let pulls = vec![pr(1, "open", "prefix/random-branch", None)]; let out = group_by_change_id(pulls).unwrap(); assert!(out.is_empty(), "got: {out:?}"); @@ -282,9 +389,9 @@ mod tests { #[test] fn group_preserves_first_seen_order_across_distinct_change_ids() { - // GitHub returns search results in `sort=updated` - // order; the orphan loop in `get_changes` observes that - // order. Confirm it's not silently re-sorted. + // The lookup sorts by `updated_at` before grouping, and + // the orphan list observes that order. Confirm grouping + // doesn't silently re-sort it. let pulls = vec![ pr(1, "open", "prefix/feat-a--aaaaaaaa", None), pr(2, "open", "prefix/feat-b--bbbbbbbb", None), @@ -337,104 +444,331 @@ mod tests { } } - #[tokio::test] - async fn get_remote_changes_searches_then_fetches_each_pull() { - // End-to-end smoke: the search query gets composed - // correctly, each result triggers a per-PR fetch, the - // grouped output matches. - use url::Url; - use wiremock::matchers::{method, path, query_param}; - use wiremock::{Mock, MockServer, ResponseTemplate}; + const ID_A: &str = "Iaaaaaaaa0000000000000000000000000000000a"; + const ID_B: &str = "Ibbbbbbbb0000000000000000000000000000000b"; + + fn local(change_id: &str, slug: &str) -> LocalCommit { + LocalCommit { + commit_sha: format!("sha-{slug}"), + title: slug.to_string(), + message: String::new(), + change_id: change_id.to_string(), + slug: slug.to_string(), + note: String::new(), + } + } - let server = MockServer::start().await; + /// A list-endpoint PR payload, with the fields the lookup + /// filters and sorts on. + fn listed(number: u64, state: &str, head_ref: &str, login: &str, updated: &str) -> Value { + let mut pull = pr(number, state, head_ref, None); + pull["user"] = json!({ "login": login }); + pull["updated_at"] = json!(updated); + pull + } + fn refs(names: &[&str]) -> Value { + Value::Array( + names + .iter() + .map(|n| json!({ "ref": format!("refs/heads/{n}"), "object": { "sha": "x" } })) + .collect(), + ) + } + + async fn mount_refs(server: &MockServer, prefix: &str, names: &[&str]) { Mock::given(method("GET")) - .and(path("/search/issues")) - .and(query_param( - "q", - "repo:user/repo author:author is:pull-request head:prefix/", - )) - .and(query_param("per_page", "100")) - .and(query_param("sort", "updated")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ - "items": [ - { "number": 11 }, - { "number": 22 }, - ], - }))) - .mount(&server) + .and(path(format!( + "/repos/user/repo/git/matching-refs/heads/{prefix}/" + ))) + .respond_with(ResponseTemplate::new(200).set_body_json(refs(names))) + .expect(1) + .mount(server) .await; + } + + async fn mount_lookup(server: &MockServer, branch: &str, pulls: Value) { Mock::given(method("GET")) - .and(path("/repos/user/repo/pulls/11")) - .respond_with(ResponseTemplate::new(200).set_body_json(pr( + .and(path("/repos/user/repo/pulls")) + .and(query_param("head", format!("user:{branch}"))) + .and(query_param("state", "all")) + .and(query_param("per_page", "100")) + .respond_with(ResponseTemplate::new(200).set_body_json(pulls)) + .expect(1) + .mount(server) + .await; + } + + /// The endpoint this module used to call. Mounted with + /// `expect(0)` so any regression back to it fails the test. + async fn forbid_search(server: &MockServer) { + Mock::given(path("/search/issues")) + .respond_with(ResponseTemplate::new(403)) + .expect(0) + .mount(server) + .await; + } + + fn client(server: &MockServer) -> Client { + Client::new( + Url::parse(&server.uri()).unwrap(), + "tok".to_string(), + ApiFlavor::GitHub, + ) + .unwrap() + } + + #[tokio::test] + async fn looks_up_each_live_stack_branch_without_the_search_api() { + let server = MockServer::start().await; + forbid_search(&server).await; + mount_refs( + &server, + "prefix", + &["prefix/feat-a--aaaaaaaa", "prefix/feat-b--bbbbbbbb"], + ) + .await; + mount_lookup( + &server, + "prefix/feat-a--aaaaaaaa", + json!([listed( 11, "open", "prefix/feat-a--aaaaaaaa", - None, - ))) - .mount(&server) - .await; - Mock::given(method("GET")) - .and(path("/repos/user/repo/pulls/22")) - .respond_with(ResponseTemplate::new(200).set_body_json(pr( + "author", + "2026-01-01T00:00:00Z" + )]), + ) + .await; + mount_lookup( + &server, + "prefix/feat-b--bbbbbbbb", + json!([listed( 22, "open", "prefix/feat-b--bbbbbbbb", - None, - ))) - .mount(&server) - .await; + "author", + "2026-01-02T00:00:00Z" + )]), + ) + .await; + + let mut progress = Vec::new(); + let got = get_remote_changes_reporting( + &client(&server), + "user", + "repo", + "prefix", + Some("author"), + &[], + |n, total| progress.push((n, total)), + ) + .await + .unwrap(); - let client = Client::new( - Url::parse(&server.uri()).unwrap(), - "tok".to_string(), - ApiFlavor::GitHub, + // Most recently updated first, as the Search API's + // `sort=updated` used to return them. + assert_eq!( + got.iter() + .map(|c| (c.change_id.as_str(), c.pull["number"].as_u64())) + .collect::>(), + vec![("bbbbbbbb", Some(22)), ("aaaaaaaa", Some(11))], + ); + assert_eq!(progress, vec![(1, 2), (2, 2)]); + } + + #[tokio::test] + async fn finds_a_merged_pull_whose_branch_was_deleted_through_the_local_slug() { + // The bottom PR merged and GitHub deleted its branch, so + // `matching-refs` no longer lists it. `stack sync` still + // has to see that PR as merged to drop the local commit. + let server = MockServer::start().await; + forbid_search(&server).await; + mount_refs(&server, "prefix", &["prefix/feat-b--bbbbbbbb"]).await; + mount_lookup( + &server, + "prefix/feat-b--bbbbbbbb", + json!([listed( + 22, + "open", + "prefix/feat-b--bbbbbbbb", + "author", + "2026-01-02T00:00:00Z" + )]), ) + .await; + let mut merged = listed( + 11, + "closed", + "prefix/feat-a--aaaaaaaa", + "author", + "2026-01-01T00:00:00Z", + ); + merged["merged_at"] = json!("2026-01-01T00:00:00Z"); + mount_lookup(&server, "prefix/feat-a--aaaaaaaa", json!([merged])).await; + + let locals = [ + local(ID_A, "feat-a--aaaaaaaa"), + local(ID_B, "feat-b--bbbbbbbb"), + ]; + let got = get_remote_changes( + &client(&server), + "user", + "repo", + "prefix", + Some("author"), + &locals, + ) + .await .unwrap(); - let got = get_remote_changes(&client, "user", "repo", "prefix", Some("author")) - .await - .unwrap(); + let merged = got + .iter() + .find(|c| c.change_id == "aaaaaaaa") + .expect("merged pull found"); + assert_eq!(merged.pull["number"], 11); + assert_eq!(merged.pull["merged_at"], "2026-01-01T00:00:00Z"); assert_eq!(got.len(), 2); - assert_eq!(got[0].change_id, "aaaaaaaa"); - assert_eq!(got[0].pull["number"], 11); - assert_eq!(got[1].change_id, "bbbbbbbb"); - assert_eq!(got[1].pull["number"], 22); } #[tokio::test] - async fn get_remote_changes_omits_author_filter_when_author_is_none() { - // `stack checkout` works on anyone's stack, so it searches - // the branch alone. The `author:` qualifier has to be - // absent entirely — an empty one (`author:`) is a search - // syntax error on GitHub's side. - use url::Url; - use wiremock::matchers::{method, path, query_param}; - use wiremock::{Mock, MockServer, ResponseTemplate}; - + async fn skips_the_slug_lookup_for_a_change_already_on_a_live_branch() { + // The commit was reworded after its PR opened: the PR keeps + // its original branch, the local slug now differs. The live + // branch already answers for that Change-Id, so the + // reworded slug is not looked up. let server = MockServer::start().await; + forbid_search(&server).await; + mount_refs(&server, "prefix", &["prefix/old-title--aaaaaaaa"]).await; + mount_lookup( + &server, + "prefix/old-title--aaaaaaaa", + json!([listed( + 11, + "open", + "prefix/old-title--aaaaaaaa", + "author", + "2026-01-01T00:00:00Z" + )]), + ) + .await; Mock::given(method("GET")) - .and(path("/search/issues")) - .and(query_param( - "q", - "repo:user/repo is:pull-request head:prefix/", - )) - .respond_with( - ResponseTemplate::new(200).set_body_json(serde_json::json!({"items": []})), - ) + .and(path("/repos/user/repo/pulls")) + .and(query_param("head", "user:prefix/new-title--aaaaaaaa")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!([]))) + .expect(0) .mount(&server) .await; - let client = Client::new( - Url::parse(&server.uri()).unwrap(), - "tok".to_string(), - ApiFlavor::GitHub, + let got = get_remote_changes( + &client(&server), + "user", + "repo", + "prefix", + Some("author"), + &[local(ID_A, "new-title--aaaaaaaa")], ) + .await .unwrap(); - let got = get_remote_changes(&client, "user", "repo", "prefix", None) + + assert_eq!(got.len(), 1); + assert_eq!(got[0].pull["number"], 11); + } + + #[tokio::test] + async fn ignores_nested_and_unmanaged_branches_under_the_prefix() { + // `prefix/sub/...` is another stack; `prefix/random` was + // never pushed by `mergify stack`. Neither is looked up. + let server = MockServer::start().await; + forbid_search(&server).await; + mount_refs( + &server, + "prefix", + &["prefix/sub/feat-x--cccccccc", "prefix/random-branch"], + ) + .await; + Mock::given(method("GET")) + .and(path("/repos/user/repo/pulls")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!([]))) + .expect(0) + .mount(&server) + .await; + + let got = get_remote_changes(&client(&server), "user", "repo", "prefix", None, &[]) + .await + .unwrap(); + + assert!(got.is_empty(), "got: {got:?}"); + } + + #[tokio::test] + async fn author_filter_is_case_insensitive_and_absent_without_an_author() { + // `stack checkout` passes no author and must see anyone's + // PR; the owner-only commands keep just their own, matching + // the login the way GitHub does (case-insensitively). + for (author, expected) in [ + (Some("AUTHOR"), vec![11]), + (Some("someone-else"), vec![]), + (None, vec![11]), + ] { + let server = MockServer::start().await; + mount_refs(&server, "prefix", &["prefix/feat-a--aaaaaaaa"]).await; + mount_lookup( + &server, + "prefix/feat-a--aaaaaaaa", + json!([listed( + 11, + "open", + "prefix/feat-a--aaaaaaaa", + "author", + "2026-01-01T00:00:00Z" + )]), + ) + .await; + + let got = get_remote_changes(&client(&server), "user", "repo", "prefix", author, &[]) + .await + .unwrap(); + + assert_eq!( + got.iter() + .filter_map(|c| c.pull["number"].as_u64()) + .collect::>(), + expected, + "author {author:?}", + ); + } + } + + #[tokio::test] + async fn keeps_the_most_recently_updated_of_two_merged_pulls_on_one_change() { + // Both merged, one Change-Id (re-opened and merged twice): + // the newest wins, whatever order the lookups ran in. + let server = MockServer::start().await; + mount_refs(&server, "prefix", &["prefix/feat-a--aaaaaaaa"]).await; + let mut older = listed( + 11, + "closed", + "prefix/feat-a--aaaaaaaa", + "author", + "2026-01-01T00:00:00Z", + ); + older["merged_at"] = json!("2026-01-01T00:00:00Z"); + let mut newer = listed( + 12, + "closed", + "prefix/feat-a--aaaaaaaa", + "author", + "2026-02-01T00:00:00Z", + ); + newer["merged_at"] = json!("2026-02-01T00:00:00Z"); + mount_lookup(&server, "prefix/feat-a--aaaaaaaa", json!([older, newer])).await; + + let got = get_remote_changes(&client(&server), "user", "repo", "prefix", None, &[]) .await .unwrap(); - assert!(got.is_empty()); + assert_eq!(got.len(), 1); + assert_eq!(got[0].pull["number"], 12); } }