diff --git a/Cargo.lock b/Cargo.lock index c2e244ba..19e8924c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1479,6 +1479,7 @@ dependencies = [ "tempfile", "terminal_size", "tokio", + "tracing", "unicode-width", "url", "wiremock", diff --git a/crates/mergify-cli/src/main.rs b/crates/mergify-cli/src/main.rs index 2c0c9b9b..6bd160d3 100644 --- a/crates/mergify-cli/src/main.rs +++ b/crates/mergify-cli/src/main.rs @@ -375,6 +375,9 @@ struct StackPushOpts { /// `mergify-cli.stack-revision-history` at dispatch time. revision_history: Option, no_verify: bool, + /// `None` = fall back to git config + /// `mergify-cli.stack-github-native` at dispatch time. + github_native: Option, } struct StackListOpts { @@ -2244,6 +2247,9 @@ fn run_native(cmd: NativeCommand) -> ExitCode { let revision_history = opts.revision_history.unwrap_or_else(|| { mergify_stack::stack_context::resolve_default_revision_history(None) }); + let github_native = opts.github_native.unwrap_or_else(|| { + mergify_stack::stack_context::resolve_default_github_native(None) + }); let outcome = mergify_stack::commands::push::run( &mergify_stack::commands::push::Options { repo_dir: None, @@ -2264,6 +2270,7 @@ fn run_native(cmd: NativeCommand) -> ExitCode { only_update_existing_pulls: opts.only_update_existing_pulls, revision_history, no_verify: opts.no_verify, + github_native, }, ) .await?; @@ -3274,6 +3281,13 @@ struct StackPushCli { /// hooks). #[arg(long = "no-verify", action = clap::ArgAction::SetTrue)] no_verify: bool, + + /// Also register the stack with GitHub's native Stacks API, so + /// GitHub shows it as a stack. Experimental, and silently skipped + /// where the API isn't available. Default falls back to git config + /// `mergify-cli.stack-github-native` (`false` when unset). + #[arg(long = "github-native", num_args = 0, default_missing_value = "true")] + github_native: Option, } impl From for StackPushOpts { @@ -3293,6 +3307,7 @@ impl From for StackPushOpts { only_update_existing_pulls: cli.only_update_existing_pulls, revision_history: cli.revision_history, no_verify: cli.no_verify, + github_native: cli.github_native, } } } diff --git a/crates/mergify-cli/src/snapshots/mergify__tests__cli_schema_golden.snap b/crates/mergify-cli/src/snapshots/mergify__tests__cli_schema_golden.snap index 93330288..bffeaa66 100644 --- a/crates/mergify-cli/src/snapshots/mergify__tests__cli_schema_golden.snap +++ b/crates/mergify-cli/src/snapshots/mergify__tests__cli_schema_golden.snap @@ -3301,6 +3301,24 @@ expression: schema "short": null, "valueHint": null, "valueNames": [] + }, + { + "default": null, + "env": null, + "global": false, + "help": "Also register the stack with GitHub's native Stacks API, so GitHub shows it as a stack. Experimental, and silently skipped where the API isn't available. Default falls back to git config `mergify-cli.stack-github-native` (`false` when unset)", + "id": "github_native", + "kind": "option", + "long": "github-native", + "longHelp": "Also register the stack with GitHub's native Stacks API, so GitHub shows it as a stack. Experimental, and silently skipped where the API isn't available. Default falls back to git config `mergify-cli.stack-github-native` (`false` when unset)", + "numArgs": "0", + "possibleValues": [], + "required": false, + "short": null, + "valueHint": null, + "valueNames": [ + "GITHUB_NATIVE" + ] } ], "commands": [], diff --git a/crates/mergify-cli/tests/stack_push_github_native.rs b/crates/mergify-cli/tests/stack_push_github_native.rs new file mode 100644 index 00000000..9e321e7b --- /dev/null +++ b/crates/mergify-cli/tests/stack_push_github_native.rs @@ -0,0 +1,703 @@ +//! End-to-end tests for `mergify stack push --github-native`. +//! +//! Runs the real binary against a wiremock GitHub server and a real +//! git repo, and asserts on the *sequence* of requests it issued — +//! because the whole feature is a sequencing contract: +//! +//! - with the flag off, not a single `/stacks` request may be sent +//! (the flag-off path has to stay exactly what it was); +//! - a push that only refreshes commits must leave the registration +//! completely alone — no unstack, no re-registration, and no `base` +//! in the PATCH bodies, which is what makes that possible; +//! - a push that adds changes on top must *extend* the stack rather +//! than rebuild it; +//! - a push that retargets a pull request must `unstack` **before** +//! the first PR mutation and `POST /stacks` **after** the last one. +//! Getting that order wrong is what permanently closes a surviving +//! pull request (see `mergify_stack::native_stack`), and no unit +//! test on the module in isolation can catch it. + +use std::path::{Path, PathBuf}; +use std::process::Command; + +use wiremock::matchers::{method, path as wm_path, path_regex}; +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 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() +} + +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()); +} + +/// A `feature` branch with `n_commits` Change-Id-carrying commits on +/// top of a pushed `main`, plus a bare `origin` the push can reach. +fn build_stack_repo(n_commits: usize) -> (tempfile::TempDir, Vec) { + 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 change_ids = 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]); + // Distinct in the first 8 hex — that prefix is what a stack + // branch segment carries, and a collision would make the two + // commits look like one change. + let cid = format!("I{:08x}{}", i + 1, "0".repeat(32)); + run_in( + &local, + &[ + "commit", + "-q", + "-m", + &format!("Commit {label}\n\nChange-Id: {cid}"), + ], + ); + change_ids.push(cid); + } + (workdir, change_ids) +} + +/// 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. +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; + + // One POST mock per PR, mounted newest-first so wiremock's + // last-mounted-wins ordering hands out the numbers bottom-to-top + // as the sequential upsert loop asks for them. + for (i, number) in pr_numbers.iter().enumerate() { + Mock::given(method("POST")) + .and(wm_path("/repos/myorg/myrepo/pulls")) + .respond_with(ResponseTemplate::new(201).set_body_json(serde_json::json!({ + "number": number, + "state": "open", + "merged_at": null, + "title": format!("Commit {}", (b'A' + u8::try_from(i).unwrap()) as char), + "head": {"ref": format!("head-{number}"), "sha": "0".repeat(40)}, + "base": {"ref": "main"}, + "html_url": format!("https://github.com/myorg/myrepo/pull/{number}"), + }))) + .up_to_n_times(1) + .mount(&server) + .await; + } + + // Stack comments: none exist, so each PR gets a POST. + Mock::given(method("GET")) + .and(path_regex(r"^/repos/myorg/myrepo/issues/\d+/comments$")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([]))) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path_regex(r"^/repos/myorg/myrepo/issues/\d+/comments$")) + .respond_with(ResponseTemplate::new(201).set_body_json(serde_json::json!({"id": 1}))) + .mount(&server) + .await; + + server +} + +/// Mock GitHub for a push over two PRs that already exist **and are +/// already members of native stack #7**. Both PR payloads carry the +/// `stack` object exactly as GitHub puts it on the default API version. +/// +/// `top_base` is the base branch GitHub reports for the top PR: pass +/// `bottom_ref` for a stack that is correctly chained (the routine +/// push, which must not touch the registration) or anything else for +/// one the planner has to retarget (the case that needs the fence). +/// +/// The `/stacks` endpoints are deliberately *not* mounted here — every +/// test mounts the ones it expects, and asserts on the request log +/// rather than on mock absence: an unmounted `/stacks` call 404s, and +/// both `unstack` and `register` treat a 404 as a non-event by design. +async fn mock_github_updating_a_stacked_pair( + bottom_ref: &str, + top_ref: &str, + top_base: &str, + 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!({ + "number": number, + "state": "open", + "merged_at": null, + "draft": false, + "title": "existing", + "body": "existing", + "head": {"ref": head, "sha": head_sha}, + "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; + Mock::given(method("GET")) + .and(wm_path(format!( + "/repos/myorg/myrepo/pulls/{number}/reviews" + ))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([]))) + .mount(&server) + .await; + Mock::given(method("PATCH")) + .and(wm_path(format!("/repos/myorg/myrepo/pulls/{number}"))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({}))) + .mount(&server) + .await; + } + Mock::given(method("GET")) + .and(path_regex(r"^/repos/myorg/myrepo/issues/\d+/comments$")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([]))) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path_regex(r"^/repos/myorg/myrepo/issues/\d+/comments$")) + .respond_with(ResponseTemplate::new(201).set_body_json(serde_json::json!({"id": 1}))) + .mount(&server) + .await; + server +} + +fn run_push(local: &Path, server_uri: &str, extra: &[&str]) -> std::process::Output { + let mut args = vec![ + "stack", + "push", + "--trunk", + "origin/main", + "--author", + "tester", + "--branch-prefix", + "stack/tester", + // Bypass slug discovery — `origin` is a local tempdir path. + "--repo", + "myorg/myrepo", + // Keep the test hermetic: no rebase round-trips, no + // revision-history comments (Creates have no history anyway). + "--skip-rebase", + "--no-revision-history", + ]; + args.extend_from_slice(extra); + Command::new(mergify_binary()) + .args(&args) + .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() +} + +fn assert_success(output: &std::process::Output) { + assert!( + output.status.success(), + "push failed\nstdout: {}\nstderr: {}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); +} + +/// `METHOD /path` for each request the server saw, in order. +async fn request_log(server: &MockServer) -> Vec { + server + .received_requests() + .await + .unwrap() + .iter() + .map(|r| format!("{} {}", r.method.as_str(), r.url.path())) + .collect() +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn without_the_flag_no_stacks_request_is_ever_sent() { + // The load-bearing regression test for "default behaviour is + // byte-identical": the feature is invisible unless asked for. + let (work, _) = build_stack_repo(2); + let local = work.path().join("local"); + let server = mock_github_creating(&[101, 102]).await; + + assert_success(&run_push(&local, &server.uri(), &[])); + + let log = request_log(&server).await; + assert!( + log.iter().all(|r| !r.contains("/stacks")), + "flag off must not touch the Stacks API, got: {log:#?}", + ); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn the_flag_registers_the_stack_after_every_pull_request_is_upserted() { + let (work, _) = build_stack_repo(2); + let local = work.path().join("local"); + let server = mock_github_creating(&[101, 102]).await; + Mock::given(method("POST")) + .and(wm_path("/repos/myorg/myrepo/stacks")) + .respond_with(ResponseTemplate::new(201).set_body_json(serde_json::json!({"number": 12}))) + .expect(1) + .mount(&server) + .await; + + let output = run_push(&local, &server.uri(), &["--github-native"]); + assert_success(&output); + + let log = request_log(&server).await; + let register = log + .iter() + .position(|r| r == "POST /repos/myorg/myrepo/stacks") + .unwrap_or_else(|| panic!("no stack registration in {log:#?}")); + let last_create = log + .iter() + .rposition(|r| r == "POST /repos/myorg/myrepo/pulls") + .expect("PRs are created"); + assert!( + register > last_create, + "the stack must be registered only once every PR exists, got: {log:#?}", + ); + + // Members are sent bottom-to-top, as JSON integers. + let body: serde_json::Value = server + .received_requests() + .await + .unwrap() + .iter() + .find(|r| r.url.path() == "/repos/myorg/myrepo/stacks" && r.method.as_str() == "POST") + .map(|r| serde_json::from_slice(&r.body).unwrap()) + .unwrap(); + assert_eq!(body, serde_json::json!({"pull_requests": [101, 102]})); + + // And the user is told, without it looking like a failure. + let out = String::from_utf8_lossy(&output.stdout) + String::from_utf8_lossy(&output.stderr); + assert!(out.contains("GitHub stack #12"), "output was: {out}"); +} + +/// Stack-branch name `mergify stack push` derives for commit `i` of a +/// repo built by [`build_stack_repo`]. +fn head_ref(change_ids: &[String], i: usize) -> String { + format!( + "stack/tester/feature/commit-{}--{}", + (b'a' + u8::try_from(i).unwrap()) as char, + &change_ids[i][1..9], + ) +} + +/// A repo of `n_commits` whose first `n_existing` stack branches are +/// already on the remote, parked on trunk. +/// +/// Those commits then read as Updates of pull requests that already +/// exist — the branches must really be there at the SHA the mocked +/// payloads claim, or the push's force-with-lease rejects them, and +/// parking them on trunk (rather than at the local commit) is what +/// keeps them from planning as up-to-date and issuing no PATCH at all. +fn repo_with_pushed_branches( + n_commits: usize, + n_existing: usize, +) -> (tempfile::TempDir, PathBuf, Vec, String) { + let (work, change_ids) = build_stack_repo(n_commits); + let local = work.path().join("local"); + let remote_head = capture(&local, &["rev-parse", "origin/main"]); + for i in 0..n_existing { + run_in( + &local, + &[ + "push", + "-q", + "origin", + &format!("{remote_head}:refs/heads/{}", head_ref(&change_ids, i)), + ], + ); + } + (work, local, change_ids, remote_head) +} + +/// Bodies of every `PATCH /repos/myorg/myrepo/pulls/{n}` the server +/// saw, in order. +async fn pull_patch_bodies(server: &MockServer) -> Vec { + server + .received_requests() + .await + .unwrap() + .iter() + .filter(|r| { + r.method.as_str() == "PATCH" && r.url.path().starts_with("/repos/myorg/myrepo/pulls/") + }) + .map(|r| serde_json::from_slice(&r.body).unwrap()) + .collect() +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_routine_push_leaves_the_registration_completely_alone() { + // The answer to "does this churn the API on every push?": no. The + // stack lock is only about `base`, so a push that just refreshes + // commits sends no `base`, and therefore needs no unstack, no + // re-registration — no `/stacks` request at all. The PRs keep + // their stack number and watchers see no new `pull_request.stacked` + // events. + let (_work, local, change_ids, remote_head) = repo_with_pushed_branches(2, 2); + let server = mock_github_updating_a_stacked_pair( + &head_ref(&change_ids, 0), + &head_ref(&change_ids, 1), + // Already correctly chained: nothing is being retargeted. + &head_ref(&change_ids, 0), + &remote_head, + ) + .await; + + let output = run_push(&local, &server.uri(), &["--github-native"]); + assert_success(&output); + + let log = request_log(&server).await; + assert!( + log.iter().all(|r| !r.contains("/stacks")), + "a push that only refreshes commits must not touch the \ + registration, got: {log:#?}", + ); + // What makes that safe: no `base` key in the update bodies. + let bodies = pull_patch_bodies(&server).await; + assert_eq!(bodies.len(), 2, "both PRs are updated: {bodies:#?}"); + for body in &bodies { + assert!( + body.get("base").is_none(), + "an unchanged base must not be sent — GitHub 422s the whole \ + PATCH while the PR is stacked, got: {body}", + ); + } + let out = String::from_utf8_lossy(&output.stdout) + String::from_utf8_lossy(&output.stderr); + assert!(out.contains("GitHub stack #7 unchanged"), "output: {out}"); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_change_pushed_on_top_extends_the_stack_instead_of_rebuilding_it() { + // Appending is the other common stack operation, and GitHub has an + // endpoint for it. One `POST /stacks/7/add` keeps stack #7 — its + // number, its webhooks, its members' registration — where a + // dissolve + re-register would replace all of it. + let (_work, local, change_ids, remote_head) = repo_with_pushed_branches(3, 2); + let server = mock_github_updating_a_stacked_pair( + &head_ref(&change_ids, 0), + &head_ref(&change_ids, 1), + &head_ref(&change_ids, 0), + &remote_head, + ) + .await; + // The third commit has no PR yet. + Mock::given(method("POST")) + .and(wm_path("/repos/myorg/myrepo/pulls")) + .respond_with(ResponseTemplate::new(201).set_body_json(serde_json::json!({ + "number": 103, + "state": "open", + "merged_at": null, + "title": "Commit C", + "head": {"ref": head_ref(&change_ids, 2), "sha": "0".repeat(40)}, + "base": {"ref": head_ref(&change_ids, 1)}, + "html_url": "https://github.com/myorg/myrepo/pull/103", + }))) + .expect(1) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(wm_path("/repos/myorg/myrepo/stacks/7/add")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({"number": 7}))) + .expect(1) + .mount(&server) + .await; + + let output = run_push(&local, &server.uri(), &["--github-native"]); + assert_success(&output); + + let log = request_log(&server).await; + assert!( + !log.iter().any(|r| r.ends_with("/stacks/7/unstack")), + "extending must not dissolve the stack, got: {log:#?}", + ); + assert!( + !log.iter().any(|r| r == "POST /repos/myorg/myrepo/stacks"), + "extending must not re-register the stack, got: {log:#?}", + ); + // Only the new PR is appended, and only once every PR exists. + let add = log + .iter() + .position(|r| r == "POST /repos/myorg/myrepo/stacks/7/add") + .unwrap_or_else(|| panic!("no append in {log:#?}")); + let last_create = log + .iter() + .rposition(|r| r == "POST /repos/myorg/myrepo/pulls") + .expect("the new PR is created"); + assert!(add > last_create, "append after the create, got: {log:#?}"); + let body: serde_json::Value = server + .received_requests() + .await + .unwrap() + .iter() + .find(|r| r.url.path() == "/repos/myorg/myrepo/stacks/7/add") + .map(|r| serde_json::from_slice(&r.body).unwrap()) + .unwrap(); + assert_eq!(body, serde_json::json!({"pull_requests": [103]})); + let out = String::from_utf8_lossy(&output.stdout) + String::from_utf8_lossy(&output.stderr); + assert!(out.contains("added to GitHub stack #7"), "output: {out}"); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_stack_that_cannot_be_extended_is_rebuilt() { + // The append is best effort: a 422 (the new PR doesn't chain onto + // the current top) or a 404 (someone dissolved the stack in the + // meantime) must leave a registration that describes reality, not + // a stale one. Safe to repair here because the mutations are done. + let (_work, local, change_ids, remote_head) = repo_with_pushed_branches(3, 2); + let server = mock_github_updating_a_stacked_pair( + &head_ref(&change_ids, 0), + &head_ref(&change_ids, 1), + &head_ref(&change_ids, 0), + &remote_head, + ) + .await; + Mock::given(method("POST")) + .and(wm_path("/repos/myorg/myrepo/pulls")) + .respond_with(ResponseTemplate::new(201).set_body_json(serde_json::json!({ + "number": 103, + "state": "open", + "merged_at": null, + "title": "Commit C", + "head": {"ref": head_ref(&change_ids, 2), "sha": "0".repeat(40)}, + "base": {"ref": head_ref(&change_ids, 1)}, + "html_url": "https://github.com/myorg/myrepo/pull/103", + }))) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(wm_path("/repos/myorg/myrepo/stacks/7/add")) + .respond_with(ResponseTemplate::new(422).set_body_string("nope")) + .expect(1) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(wm_path("/repos/myorg/myrepo/stacks/7/unstack")) + .respond_with(ResponseTemplate::new(204)) + .expect(1) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(wm_path("/repos/myorg/myrepo/stacks")) + .respond_with(ResponseTemplate::new(201).set_body_json(serde_json::json!({"number": 14}))) + .expect(1) + .mount(&server) + .await; + + let output = run_push(&local, &server.uri(), &["--github-native"]); + assert_success(&output); + + let body: serde_json::Value = server + .received_requests() + .await + .unwrap() + .iter() + .find(|r| r.url.path() == "/repos/myorg/myrepo/stacks" && r.method.as_str() == "POST") + .map(|r| serde_json::from_slice(&r.body).unwrap()) + .unwrap(); + assert_eq!(body, serde_json::json!({"pull_requests": [101, 102, 103]})); + let out = String::from_utf8_lossy(&output.stdout) + String::from_utf8_lossy(&output.stderr); + assert!(out.contains("GitHub stack #14"), "output: {out}"); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_registered_stack_is_dissolved_before_a_pull_request_is_retargeted() { + // The fence. GitHub rejects `PATCH /pulls/{n}` carrying `base` + // while the PR is stacked — and the failed retarget is what lets + // the orphan teardown close a surviving PR for good. So when this + // push does move a base, the unstack has to precede every + // PR-mutating call, not just the `neutralize_stale_bases` one. + let (_work, local, change_ids, remote_head) = repo_with_pushed_branches(2, 2); + let server = mock_github_updating_a_stacked_pair( + &head_ref(&change_ids, 0), + &head_ref(&change_ids, 1), + // Top PR sits on a branch that is no longer its predecessor — + // the shape a reorder leaves behind. The push must retarget it. + "stack/tester/feature/commit-z--Ideadbee", + &remote_head, + ) + .await; + Mock::given(method("POST")) + .and(wm_path("/repos/myorg/myrepo/stacks/7/unstack")) + .respond_with(ResponseTemplate::new(204)) + .expect(1) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(wm_path("/repos/myorg/myrepo/stacks")) + .respond_with(ResponseTemplate::new(201).set_body_json(serde_json::json!({"number": 13}))) + .expect(1) + .mount(&server) + .await; + + assert_success(&run_push(&local, &server.uri(), &["--github-native"])); + + let log = request_log(&server).await; + let unstack = log + .iter() + .position(|r| r == "POST /repos/myorg/myrepo/stacks/7/unstack") + .unwrap_or_else(|| panic!("no unstack in {log:#?}")); + let first_mutation = log + .iter() + .position(|r| r.starts_with("PATCH /repos/myorg/myrepo/pulls/")) + .unwrap_or_else(|| panic!("no PR update in {log:#?}")); + let register = log + .iter() + .position(|r| r == "POST /repos/myorg/myrepo/stacks") + .unwrap_or_else(|| panic!("no re-registration in {log:#?}")); + assert!( + unstack < first_mutation, + "unstack must precede every PR mutation, got: {log:#?}", + ); + assert!( + register > first_mutation, + "re-registration must follow the mutations, got: {log:#?}", + ); + // And the retarget really is sent — otherwise this test would pass + // for the wrong reason. + let bodies = pull_patch_bodies(&server).await; + assert!( + bodies.iter().any(|b| b.get("base").is_some()), + "the moving PR must carry `base`, got: {bodies:#?}", + ); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_repository_without_the_stacks_api_still_pushes_cleanly() { + // Old GHES, or a repo where the feature isn't enabled: the PRs + // are all correct, so a 404 on registration must not colour the + // exit code or read as an error. + let (work, _) = build_stack_repo(2); + let local = work.path().join("local"); + let server = mock_github_creating(&[101, 102]).await; + Mock::given(method("POST")) + .and(wm_path("/repos/myorg/myrepo/stacks")) + .respond_with(ResponseTemplate::new(404).set_body_string("Not Found")) + .mount(&server) + .await; + + let output = run_push(&local, &server.uri(), &["--github-native"]); + assert_success(&output); + let out = String::from_utf8_lossy(&output.stdout) + String::from_utf8_lossy(&output.stderr); + assert!( + out.contains("not registered on GitHub"), + "the skip should be stated plainly, got: {out}", + ); + assert!( + !out.contains("mergify: "), + "degrading must not print an error, got: {out}", + ); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_one_change_stack_stays_a_plain_pull_request() { + // GitHub rejects a 1-PR stack with 422, so we don't even ask. + let (work, _) = build_stack_repo(1); + let local = work.path().join("local"); + let server = mock_github_creating(&[101]).await; + + assert_success(&run_push(&local, &server.uri(), &["--github-native"])); + + let log = request_log(&server).await; + assert!( + log.iter().all(|r| !r.contains("/stacks")), + "below the 2-PR floor nothing should be sent, got: {log:#?}", + ); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn git_config_can_turn_the_feature_on_without_the_flag() { + let (work, _) = build_stack_repo(2); + let local = work.path().join("local"); + run_in( + &local, + &["config", "mergify-cli.stack-github-native", "true"], + ); + let server = mock_github_creating(&[101, 102]).await; + Mock::given(method("POST")) + .and(wm_path("/repos/myorg/myrepo/stacks")) + .respond_with(ResponseTemplate::new(201).set_body_json(serde_json::json!({"number": 12}))) + .expect(1) + .mount(&server) + .await; + + assert_success(&run_push(&local, &server.uri(), &[])); +} diff --git a/crates/mergify-core/src/http.rs b/crates/mergify-core/src/http.rs index 6a437e2c..e0afc5d0 100644 --- a/crates/mergify-core/src/http.rs +++ b/crates/mergify-core/src/http.rs @@ -244,6 +244,26 @@ impl Client { .map(drop) } + /// POST to `path` with **no** request body, treating 404 as + /// success and discarding the response body. + /// + /// For "make sure this is off" endpoints that take no body and + /// answer with an empty 2xx — GitHub's `POST + /// /repos/{o}/{r}/stacks/{n}/unstack` is the first caller. A 404 + /// means the resource is already gone, which is exactly the + /// postcondition the caller wanted, so it is not an error (same + /// reasoning as [`Self::delete_if_exists`]). + /// + /// Distinct from [`Self::post_no_response`], which serializes a + /// JSON body and treats 404 as a failure: an endpoint documented + /// as taking no body should be sent none, not a JSON `null`. + pub async fn post_empty_if_exists(&self, path: &str) -> Result<(), CliError> { + let url = self.join(path)?; + self.execute_with_retry(self.inner.post(url), true, None) + .await + .map(drop) + } + /// PUT `body` as JSON to `path` and deserialize the JSON /// response as `T`. pub async fn put( @@ -708,6 +728,53 @@ mod tests { .unwrap(); } + #[tokio::test] + async fn post_empty_if_exists_sends_no_body_and_tolerates_404() { + // `POST .../unstack` is documented as taking no request body, + // so we must not send a JSON `null`; and a 404 (already + // dissolved) satisfies the caller's postcondition. + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/present")) + .respond_with(ResponseTemplate::new(204)) + .expect(1) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path("/absent")) + .respond_with(ResponseTemplate::new(404).set_body_string("gone")) + .expect(1) + .mount(&server) + .await; + + let client = fast_client(&server, ApiFlavor::GitHub); + client.post_empty_if_exists("/present").await.unwrap(); + client.post_empty_if_exists("/absent").await.unwrap(); + + let requests = server.received_requests().await.unwrap(); + assert!( + requests.iter().all(|r| r.body.is_empty()), + "no request body may be sent", + ); + } + + #[tokio::test] + async fn post_empty_if_exists_propagates_other_4xx() { + // Only 404 is "already done"; a 403 is a real failure the + // caller must see. + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/denied")) + .respond_with(ResponseTemplate::new(403).set_body_string("forbidden")) + .expect(1) + .mount(&server) + .await; + + let client = fast_client(&server, ApiFlavor::GitHub); + let err = client.post_empty_if_exists("/denied").await.unwrap_err(); + assert!(matches!(err, CliError::GitHubApi(_)), "got {err:?}"); + } + #[tokio::test] async fn post_no_response_propagates_4xx() { let server = MockServer::start().await; diff --git a/crates/mergify-stack/Cargo.toml b/crates/mergify-stack/Cargo.toml index de841a2c..87d56830 100644 --- a/crates/mergify-stack/Cargo.toml +++ b/crates/mergify-stack/Cargo.toml @@ -31,6 +31,10 @@ unicode-width = { workspace = true } # off the async executor so the `stack push` progress spinner keeps # ticking smoothly across them. tokio = { workspace = true } +# The native-stack fence swallows GitHub errors by design (an +# unavailable Stacks API must not fail a push); `-vv` is the only way +# to see why a registration was skipped. +tracing = { workspace = true } url = { workspace = true } [dev-dependencies] diff --git a/crates/mergify-stack/src/commands/push.rs b/crates/mergify-stack/src/commands/push.rs index ceeaedd6..18d6af0a 100644 --- a/crates/mergify-stack/src/commands/push.rs +++ b/crates/mergify-stack/src/commands/push.rs @@ -21,6 +21,20 @@ //! 10. Upsert stack comments, and render + upsert each prepared //! revision-history comment, per PR via [`crate::comment_upsert`]. //! 11. Tear down orphan branches. +//! 12. With `--github-native`, bring GitHub's native stack in line +//! with what was just pushed, via [`crate::native_stack`]. +//! +//! Step 12 has a conditional step 0. A registered stack blocks one +//! thing: changing a PR's base. So a push that retargets a PR, or +//! tears down an orphan branch (which is only survivable because the +//! survivors were retargeted first), dissolves the registration +//! *before* step 5 and rebuilds it in step 12 — the fence is why the +//! two steps are far apart. A push that only refreshes commits does +//! neither and leaves the registration alone; step 12 then extends it +//! if changes were added on top, and otherwise issues no request at +//! all. See [`crate::native_stack`] for the measured behaviour behind +//! each case. Between the two steps the flow is exactly what it is +//! with the flag off. //! //! PR upserts run sequentially. Async here is incidental — it comes //! from reqwest's async-only client, not from a need for concurrency: @@ -44,6 +58,7 @@ use crate::changes::Action; use crate::commands::sync as sync_cmd; use crate::comment_upsert; use crate::local_commits; +use crate::native_stack; use crate::notes_push::{self, NotesLease, PushEntry}; use crate::plan::{self, PlannedChange, PlannedChanges, PlannerOpts}; use crate::pr_upsert::{self, PrUpsertInput, StaleBase}; @@ -93,6 +108,11 @@ pub struct Options<'a> { pub only_update_existing_pulls: bool, pub revision_history: bool, pub no_verify: bool, + /// Opt-in: also register the pushed stack with GitHub's native + /// Stacks API. Off by default — when off, this flow issues no + /// stacks request at all and behaves exactly as it did before the + /// feature existed. See [`crate::native_stack`]. + pub github_native: bool, } /// Outcome of [`run`]. `DryRun` carries the plan + the rebase @@ -512,6 +532,58 @@ pub async fn run(opts: &Options<'_>) -> Result { } } + // Step 0 of the native-stack fence — but only for the pushes that + // need one. A live registration blocks exactly one thing: a `PATCH + // /pulls/{n}` whose body carries `base`. So the fence is needed + // when, and only when, this push + // + // * retargets a PR — either through `neutralize_stale_bases` + // below or through the upsert itself, which sends `base` only + // when it moves (see `pr_upsert::base_if_changed`); or + // * tears down an orphan branch, which closes every PR still + // based on it. That is survivable only because the survivors + // were retargeted first — the very PATCH the lock blocks — and + // it would anyway leave the dropped PR in the stack, closed. + // + // A push that merely refreshes commits does neither: its PATCHes + // carry no `base`, and the force-push leaves membership intact. It + // runs over the registered stack untouched. See + // [`crate::native_stack`] for the measurements. + // + // `registered_stack` is read from the PR payloads the pre-flight + // already fetched — GitHub puts the `stack` object on them — so + // discovering the current membership costs no extra request. + let registered_stack = opts + .github_native + .then(|| { + native_stack::registered_number( + planned.locals.iter().filter_map(|p| p.change.pull.as_ref()), + ) + }) + .flatten(); + let retargets_a_pull = planned + .locals + .iter() + .filter(|p| matches!(p.change.action, Action::Update)) + .any(|p| { + p.change.pull.as_ref().is_some_and(|pull| { + pull.pointer("/base/ref").and_then(Value::as_str) != Some(p.base_branch.as_str()) + }) + }); + let needs_fence = retargets_a_pull || !planned.orphans.is_empty(); + let dissolved = if let Some(number) = registered_stack + && needs_fence + { + // Fatal on failure: the whole point is to not proceed. Unlike + // the registration at the end, this one is load-bearing. + let unstacking = native_stack::unstack(opts.client, opts.user, opts.repo, number); + prog.run_optional(publishing, "unstacking on GitHub", unstacking) + .await?; + true + } else { + false + }; + // Before the force-push, repoint any Update PR whose base is // moving onto the trunk. A reorder can make a head branch // briefly an ancestor of its stale base once the atomic push @@ -786,6 +858,83 @@ pub async fn run(opts: &Options<'_>) -> Result { prog.resolve(oidx, Mark::Noop, Some("deleted")); } + // Step 12 — make GitHub's registration describe the stack this + // push produced. Last, after the orphan teardown, so the members + // are the ones that survived it: retargeted during the upsert loop, + // with the dropped changes' branches already gone. + // + // Three outcomes, cheapest first, because a re-registration is not + // free — it churns the stack number and re-emits + // `pull_request.stacked` for every member: + // + // - the registration already describes the stack → no request; + // - it is missing changes on top → one `append`, which preserves + // the stack and its members' registration; + // - anything else (a reorder, a drop, a change inserted in the + // middle, or a stack we dissolved up front) → register from + // scratch, which is only possible because nothing is registered + // at that point. + if opts.github_native { + // The stack still standing on GitHub, if any: the one we read + // before the mutations and did not dissolve. + let live_stack = if dissolved { None } else { registered_stack }; + // Bottom-to-top, open members only. Merged members are + // GitHub's to infer, and orphans aren't in `locals` at all. + let members: Vec = planned + .locals + .iter() + .filter_map(|p| { + let pull = p.change.pull.as_ref()?; + let number = native_stack::open_pull_number(Some(pull))?; + Some(native_stack::Member { + number, + registered: live_stack.is_some_and(|n| native_stack::is_member_of(pull, n)), + }) + }) + .collect(); + let numbers: Vec = members.iter().map(|m| m.number).collect(); + + // `Some(tail)` on a live stack is an append (possibly of + // nothing); everything else falls through to a full rebuild. + let tail = live_stack.zip(native_stack::appendable_tail(&members)); + match tail { + Some((number, tail)) if tail.is_empty() => { + prog.add_resolved(Mark::Noop, format!("GitHub stack #{number} unchanged")); + } + Some((number, tail)) => { + let sidx = prog.add("queued"); + let appended = prog + .run( + sidx, + "extending GitHub stack", + native_stack::append(opts.client, opts.user, opts.repo, number, &tail), + ) + .await; + if appended { + prog.resolve( + sidx, + Mark::Done, + Some(&format!("added to GitHub stack #{number}")), + ); + } else { + // The append is best-effort by design; when GitHub + // won't extend the stack (someone dissolved it, the + // chain doesn't line up) rebuild it rather than + // leaving a registration that describes a stack + // that no longer exists. The mutations are done, so + // a failing unstack is now harmless — it just means + // the registration stays as it was. + let _ = native_stack::unstack(opts.client, opts.user, opts.repo, number).await; + register_stack(opts, &mut prog, sidx, &numbers).await; + } + } + None => { + let sidx = prog.add("queued"); + register_stack(opts, &mut prog, sidx, &numbers).await; + } + } + } + // Warnings stashed during the live block (a mid-block print would // corrupt the in-place redraw) surface now, after the last row. for note in deferred_notes { @@ -808,6 +957,31 @@ pub async fn run(opts: &Options<'_>) -> Result { }) } +/// Register `numbers` (bottom-to-top) as a native stack and report the +/// outcome on progress row `idx`. +/// +/// Never fails the push: a repo without the Stacks API, a chain with a +/// hole in it, or a stack below GitHub's 2-PR floor all just leave the +/// PRs unregistered, which is the state the flag-off flow produces +/// anyway. See [`crate::native_stack::register`]. +async fn register_stack(opts: &Options<'_>, prog: &mut Progress, idx: usize, numbers: &[u64]) { + let registered = prog + .run( + idx, + "registering GitHub stack", + native_stack::register(opts.client, opts.user, opts.repo, numbers), + ) + .await; + match registered { + Some(number) => prog.resolve( + idx, + Mark::Done, + Some(&format!("registered as GitHub stack #{number}")), + ), + None => prog.resolve(idx, Mark::Noop, Some("not registered on GitHub")), + } +} + /// Append the "what will happen" preview lines (locals first, then /// orphans), in the would-be wording. Mirrors Python's /// `changes.display_plan`. diff --git a/crates/mergify-stack/src/lib.rs b/crates/mergify-stack/src/lib.rs index ccd39cb8..a00f8b8a 100644 --- a/crates/mergify-stack/src/lib.rs +++ b/crates/mergify-stack/src/lib.rs @@ -89,6 +89,7 @@ pub mod comment_upsert; pub mod git; pub mod local_commits; pub mod match_commit; +pub mod native_stack; pub mod notes_push; pub mod plan; pub mod plan_display; diff --git a/crates/mergify-stack/src/native_stack.rs b/crates/mergify-stack/src/native_stack.rs new file mode 100644 index 00000000..fbedb47b --- /dev/null +++ b/crates/mergify-stack/src/native_stack.rs @@ -0,0 +1,602 @@ +//! Opt-in registration of a pushed stack with GitHub's **native** +//! Stacks API (`stack push --github-native`). +//! +//! Native membership is *additive*: Change-Id identity, the branch +//! layout and the revision history stay ours. All this module does is +//! tell GitHub "these PRs, in this order, are one stack" so its UI and +//! its stack-aware merge path see what our stack comment already +//! describes. +//! +//! # What a live registration does and does not block +//! +//! Registration is not inert — it locks *one* thing. Measured against +//! the live API (2026-08-05); endpoint paths below are written relative +//! to `/repos/{owner}/{repo}`, as elsewhere in this crate: +//! +//! - `PATCH /pulls/{n}` fails with 422 *"Cannot change the base branch +//! because the pull request is part of a stack"* whenever the `base` +//! key is present **at all**, including when it is set to the value +//! the PR already has. That is the whole lock, and it is why +//! [`crate::pr_upsert::create_or_update_pr`] sends `base` only when +//! the PR is really being retargeted. The same PATCH without the key +//! — new `title`, new `body`, even a `head` that does not exist — +//! succeeds while stacked, and the stack survives the force-push of +//! its members' branches. So a push that only refreshes commits +//! needs no dissolve at all. +//! - The 422 is **not atomic**: a body carrying `base` *and* `title` +//! applies the title and rejects the base. Firing one blind and +//! treating the error as "nothing happened" is not an option. +//! - Orphan teardown ([`crate::pr_upsert::delete_orphan_branch`]) +//! deletes a dropped change's head branch, and GitHub closes not only +//! that PR but every PR still based on the deleted branch. Push is +//! safe today only because step 9 retargets the survivors *before* +//! step 11 deletes the branch — a retarget the lock above would +//! block. A PR closed this way cannot be reopened (its base branch is +//! gone) or retargeted (it is closed): it is permanently lost. And +//! the dropped PR, closed but unmerged, stays a member of the stack, +//! which then no longer describes anything real. +//! - `POST /stacks` on PRs that are already in an open stack fails with +//! 422 *"are already part of a stack"* — a fresh POST does not +//! replace an existing registration, so *re-forming* requires an +//! unstack first. +//! +//! # The three shapes of a push +//! +//! Hence [`crate::commands::push`] dissolves the stack for exactly the +//! pushes that need it, and nothing else: +//! +//! | push | stacks requests | +//! |---|---| +//! | refresh commits (amend, reword, force-push) | **none** | +//! | append a change on top | one [`append`] | +//! | retarget a PR, or tear down an orphan | [`unstack`] up front, [`register`] at the end | +//! +//! The fence, when it is needed, is the whole mutation stage rather +//! than one call site: the retarget can come from +//! [`crate::pr_upsert::neutralize_stale_bases`] *or* from the upsert +//! itself, and the orphan teardown at the end depends on the retarget +//! having landed. In between, the flow is byte-for-byte the flow that +//! runs with the flag off, which is what makes the failure mode benign +//! — an interrupted push leaves the stack merely unregistered, i.e. +//! exactly today's behaviour. +//! +//! # Failure policy — deliberately asymmetric +//! +//! - [`register`] and [`append`] **never fail the push.** A repo +//! without the feature, an old GHES, a chain with a hole in it, or a +//! stack that has shrunk below the 2-PR floor all just leave the PRs +//! unregistered. That is the documented graceful degradation: the +//! user gets today's stack. +//! - [`unstack`] failing **is** a hard error *before* the mutations. It +//! is the one case where carrying on produces the unrecoverable state +//! above, so the push stops before it can touch a single PR. (After +//! the mutations, when it is only used to rebuild a stale +//! registration, a failure is harmless and the caller swallows it.) + +use mergify_core::{CliError, HttpClient}; +use serde::Serialize; +use serde_json::Value; + +/// Fewest PRs GitHub will accept in a stack. A `POST /stacks` with one +/// pull request is rejected with 422 *"2 items required; only 1 was +/// supplied"*, so a single-change stack stays a plain PR — and a +/// 2-member stack that loses a member dissolves rather than re-forming. +const MIN_STACK_SIZE: usize = 2; + +#[derive(Serialize)] +struct CreateStack<'a> { + /// Bottom-to-top. GitHub requires JSON integers here; strings are + /// rejected with a schema 422. + pull_requests: &'a [u64], +} + +/// The stack number these PR payloads say they currently belong to. +/// +/// GitHub puts a `stack` object on the PR payload on the **default** +/// API version, and [`crate::remote_changes`] already fetches every +/// PR in full — so the current registration costs no extra request. +/// The `number` (not the `id`) is the path key for [`unstack`]. +/// +/// Members of one stack all report the same number; the first one +/// found wins. +pub fn registered_number<'a>(pulls: impl IntoIterator) -> Option { + pulls + .into_iter() + .find_map(|pull| pull.pointer("/stack/number").and_then(Value::as_u64)) +} + +/// Whether `pull` is already a member of stack `number`, read from the +/// same `stack` object [`registered_number`] uses. +pub fn is_member_of(pull: &Value, number: u64) -> bool { + pull.pointer("/stack/number").and_then(Value::as_u64) == Some(number) +} + +/// Dissolve stack `number` so the PRs can be mutated again. +/// +/// All-or-nothing by design — the endpoint takes no body and there is +/// no way to drop a single member. A 404 means the stack is already +/// gone (a stale number, or a concurrent unstack) and counts as +/// success: the postcondition "these PRs are not in a stack" holds +/// either way. +/// +/// # Errors +/// +/// Any other failure is returned. Unlike [`register`], a failure here +/// must stop a push that has not started mutating yet: the caller is +/// about to issue the `PATCH`es that a live registration turns into an +/// unrecoverable teardown (see the module docs). The same call is also +/// used *after* the mutations to rebuild a registration [`append`] +/// could not extend — there the postcondition is only cosmetic, and the +/// caller ignores the error. +pub async fn unstack( + client: &HttpClient, + user: &str, + repo: &str, + number: u64, +) -> Result<(), CliError> { + let path = format!("/repos/{user}/{repo}/stacks/{number}/unstack"); + match client.post_empty_if_exists(&path).await { + Ok(()) => { + tracing::debug!(stack = number, "dissolved GitHub stack"); + Ok(()) + } + Err(e) => Err(CliError::wrap( + format!( + "could not dissolve GitHub stack #{number} before updating the pull requests \ + (GitHub refuses to change a pull request's base branch while it is stacked). \ + Retry, or unstack it by hand and push again", + ), + e, + )), + } +} + +/// Register `pulls` (bottom-to-top) as one native stack, best effort. +/// +/// Returns the new stack number, or `None` when the stack was not +/// registered — which is a routine, non-failing outcome: +/// +/// - fewer than [`MIN_STACK_SIZE`] open PRs (a one-change stack is a +/// plain PR, and a shrunken 2-PR stack dissolves); +/// - the repository or GitHub deployment has no Stacks API (old GHES, +/// feature not enabled) — 404; +/// - the chain has a hole in it, e.g. under +/// `--only-update-existing-pulls` — 422. +/// +/// None of these are worth failing a push that has already created, +/// updated and linked every PR correctly, so the error is logged at +/// debug and swallowed. The caller reports the outcome as a progress +/// row rather than an error. +pub async fn register(client: &HttpClient, user: &str, repo: &str, pulls: &[u64]) -> Option { + if pulls.len() < MIN_STACK_SIZE { + tracing::debug!( + count = pulls.len(), + "not registering a GitHub stack: below the 2 pull request minimum" + ); + return None; + } + let path = format!("/repos/{user}/{repo}/stacks"); + let body = CreateStack { + pull_requests: pulls, + }; + match client.post::<_, Value>(&path, &body).await { + Ok(stack) => { + let number = stack.get("number").and_then(Value::as_u64); + tracing::debug!(?number, ?pulls, "registered GitHub stack"); + number + } + Err(e) => { + tracing::debug!( + error = %e, + "GitHub stack registration unavailable; leaving the pull requests unstacked" + ); + None + } + } +} + +/// Append `pulls` (bottom-to-top) on top of registered stack `number`, +/// best effort. +/// +/// The incremental counterpart to [`register`]: `POST /stacks/{n}/add` +/// keeps the stack — its number, its webhooks, its existing members' +/// registration — and only extends it, so pushing a new change on top +/// of a stack costs one request instead of a dissolve plus a full +/// re-registration. +/// +/// GitHub requires the first appended PR's `base` ref to be the current +/// top member's `head` ref; a list that does not chain on is rejected +/// with 422 *"Pull requests must form a stack, where each PR's base ref +/// is the previous PR's head ref"*. Everything else the API cannot do +/// — inserting in the middle, reordering, dropping a member — has no +/// endpoint at all and goes through [`unstack`] + [`register`]. +/// +/// Returns `false` when the append did not happen, on the same terms as +/// [`register`]: never an error, always a caller-visible outcome. The +/// caller's remedy is to rebuild the stack from scratch. +pub async fn append( + client: &HttpClient, + user: &str, + repo: &str, + number: u64, + pulls: &[u64], +) -> bool { + if pulls.is_empty() { + return true; + } + let path = format!("/repos/{user}/{repo}/stacks/{number}/add"); + let body = CreateStack { + pull_requests: pulls, + }; + match client.post::<_, Value>(&path, &body).await { + Ok(_) => { + tracing::debug!(stack = number, ?pulls, "appended to GitHub stack"); + true + } + Err(e) => { + tracing::debug!( + error = %e, + stack = number, + "could not append to the GitHub stack; rebuilding it instead" + ); + false + } + } +} + +/// One open pull request of the stack we are about to describe, in +/// stack order, paired with whether GitHub's registration already +/// holds it. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct Member { + pub number: u64, + pub registered: bool, +} + +/// The members GitHub is missing, when they are exactly a suffix of +/// `members` — the only difference [`append`] can express. +/// +/// `Some([])` means the registration already describes the stack and +/// the push should issue no request at all. `None` means the difference +/// is something else — a new change landed *under* an existing one, or +/// a registered member is no longer in the stack — and the caller has +/// to dissolve and re-register. +/// +/// Merged members need no special case: they are not in `members` at +/// all (GitHub keeps them in the stack and infers the merged prefix +/// itself), so a stack whose bottom just merged still reads as "already +/// correct". +#[must_use] +pub fn appendable_tail(members: &[Member]) -> Option> { + let split = members.iter().take_while(|m| m.registered).count(); + if members[split..].iter().any(|m| m.registered) { + // A registered member sits above an unregistered one: the new + // change went into the middle, not on top. + return None; + } + Some(members[split..].iter().map(|m| m.number).collect()) +} + +/// PR number of `pull` when it is an open, unmerged pull request — +/// i.e. one that belongs in a stack registration. +/// +/// Merged members are excluded deliberately: a fresh `POST /stacks` +/// describes the stack that is still in flight, and GitHub derives the +/// merged prefix itself. +pub fn open_pull_number(pull: Option<&Value>) -> Option { + let pull = pull?; + if pull.get("merged_at").is_some_and(|v| !v.is_null()) { + return None; + } + if pull.get("state").and_then(Value::as_str) == Some("closed") { + return None; + } + pull.get("number").and_then(Value::as_u64) +} + +#[cfg(test)] +mod tests { + use super::*; + use mergify_core::{ApiFlavor, HttpClient}; + use serde_json::json; + use url::Url; + use wiremock::matchers::{body_json, method, path as wm_path}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + fn client(server: &MockServer) -> HttpClient { + HttpClient::new( + Url::parse(&server.uri()).unwrap(), + "token", + ApiFlavor::GitHub, + ) + .unwrap() + } + + #[test] + fn registered_number_reads_the_stack_object_off_a_pull_payload() { + // Membership rides along on the payload `remote_changes` + // already fetches — pinning the pointer keeps us from + // reintroducing a `GET /stacks` round-trip. + let pulls = [ + json!({"number": 1, "stack": null}), + json!({"number": 2, "stack": {"id": 162_170, "number": 7, "position": 1}}), + ]; + // The path key is `number` (7), not `id` (162170). + assert_eq!(registered_number(pulls.iter()), Some(7)); + } + + #[test] + fn registered_number_is_none_when_nothing_is_stacked() { + let pulls = [json!({"number": 1, "stack": null}), json!({"number": 2})]; + assert_eq!(registered_number(pulls.iter()), None); + } + + #[test] + fn open_pull_number_selects_only_live_members() { + assert_eq!( + open_pull_number(Some( + &json!({"number": 5, "state": "open", "merged_at": null}) + )), + Some(5) + ); + // Merged members are GitHub's to infer, not ours to re-send. + assert_eq!( + open_pull_number(Some( + &json!({"number": 5, "state": "closed", "merged_at": "2026-01-01T00:00:00Z"}) + )), + None + ); + assert_eq!( + open_pull_number(Some( + &json!({"number": 5, "state": "closed", "merged_at": null}) + )), + None + ); + assert_eq!(open_pull_number(None), None); + } + + #[tokio::test] + async fn register_posts_members_bottom_to_top_as_integers() { + // GitHub rejects stringified numbers with a schema 422, and + // order is the stack order — both are load-bearing, so assert + // the exact body. + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(wm_path("/repos/o/r/stacks")) + .and(body_json(json!({"pull_requests": [9, 10, 11]}))) + .respond_with(ResponseTemplate::new(201).set_body_json(json!({"number": 12}))) + .expect(1) + .mount(&server) + .await; + + let got = register(&client(&server), "o", "r", &[9, 10, 11]).await; + assert_eq!(got, Some(12)); + } + + #[tokio::test] + async fn register_below_the_floor_issues_no_request() { + // A one-change stack is a plain PR. Wiremock with no mounted + // mock 404s any call, so `Some(_)` here would mean we made one. + let server = MockServer::start().await; + assert_eq!(register(&client(&server), "o", "r", &[9]).await, None); + assert_eq!(register(&client(&server), "o", "r", &[]).await, None); + assert!(server.received_requests().await.unwrap().is_empty()); + } + + #[tokio::test] + async fn register_degrades_silently_when_the_api_is_unavailable() { + // 404 is an old GHES or a repo without the feature; 422 is a + // chain with a hole in it. Neither may fail a push whose PRs + // are already correct. + for status in [404, 422, 403] { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(wm_path("/repos/o/r/stacks")) + .respond_with(ResponseTemplate::new(status).set_body_string("nope")) + .mount(&server) + .await; + assert_eq!( + register(&client(&server), "o", "r", &[9, 10]).await, + None, + "status {status} must degrade, not fail" + ); + } + } + + #[tokio::test] + async fn register_tolerates_a_response_without_a_number() { + // A proxy or a future API version returning a shape we don't + // recognise still means "registered"; we just have nothing to + // display. Must not panic or error. + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(wm_path("/repos/o/r/stacks")) + .respond_with(ResponseTemplate::new(201).set_body_json(json!({}))) + .mount(&server) + .await; + assert_eq!(register(&client(&server), "o", "r", &[9, 10]).await, None); + } + + #[test] + fn is_member_of_matches_only_the_stack_we_read() { + let pull = json!({"number": 1, "stack": {"number": 7, "position": 1}}); + assert!(is_member_of(&pull, 7)); + assert!(!is_member_of(&pull, 8)); + assert!(!is_member_of(&json!({"number": 1, "stack": null}), 7)); + } + + #[test] + fn appendable_tail_is_empty_when_the_registration_is_already_right() { + // The routine push: same members, same order. Nothing to send — + // this is the case that used to cost an unstack + a re-register + // on every single push. + let members = [ + Member { + number: 101, + registered: true, + }, + Member { + number: 102, + registered: true, + }, + ]; + assert_eq!(appendable_tail(&members), Some(vec![])); + } + + #[test] + fn appendable_tail_returns_the_new_changes_on_top() { + let members = [ + Member { + number: 101, + registered: true, + }, + Member { + number: 102, + registered: true, + }, + Member { + number: 103, + registered: false, + }, + Member { + number: 104, + registered: false, + }, + ]; + assert_eq!(appendable_tail(&members), Some(vec![103, 104])); + } + + #[test] + fn appendable_tail_refuses_a_change_inserted_under_a_member() { + // A new change in the middle moves the bases of everything + // above it — there is no endpoint for that, so the caller has + // to dissolve and re-register. + let members = [ + Member { + number: 101, + registered: true, + }, + Member { + number: 103, + registered: false, + }, + Member { + number: 102, + registered: true, + }, + ]; + assert_eq!(appendable_tail(&members), None); + } + + #[test] + fn appendable_tail_of_an_all_new_stack_is_everything() { + let members = [ + Member { + number: 101, + registered: false, + }, + Member { + number: 102, + registered: false, + }, + ]; + assert_eq!(appendable_tail(&members), Some(vec![101, 102])); + assert_eq!(appendable_tail(&[]), Some(vec![])); + } + + #[tokio::test] + async fn append_posts_the_new_members_to_the_add_endpoint() { + // Same body shape as `register` — integers, bottom-to-top — but + // onto the existing stack, so its number and its members' + // registration survive. + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(wm_path("/repos/o/r/stacks/7/add")) + .and(body_json(json!({"pull_requests": [11]}))) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({"number": 7}))) + .expect(1) + .mount(&server) + .await; + + assert!(append(&client(&server), "o", "r", 7, &[11]).await); + } + + #[tokio::test] + async fn append_of_nothing_issues_no_request() { + // The most common push of all: members unchanged. Wiremock + // would 404 any call, and `true` here means we made none. + let server = MockServer::start().await; + assert!(append(&client(&server), "o", "r", 7, &[]).await); + assert!(server.received_requests().await.unwrap().is_empty()); + } + + #[tokio::test] + async fn append_reports_failure_instead_of_erroring() { + // 422 (the appended PR doesn't chain onto the current top), 404 + // (someone dissolved the stack meanwhile). Both mean "rebuild + // it", never "fail the push". + for status in [404, 422, 409] { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(wm_path("/repos/o/r/stacks/7/add")) + .respond_with(ResponseTemplate::new(status).set_body_string("nope")) + .mount(&server) + .await; + assert!( + !append(&client(&server), "o", "r", 7, &[11]).await, + "status {status} must report failure, not error" + ); + } + } + + #[tokio::test] + async fn unstack_posts_to_the_stack_number_with_no_body() { + // The endpoint takes no request body and returns an empty + // 204 — decoding it as JSON would fail. + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(wm_path("/repos/o/r/stacks/7/unstack")) + .respond_with(ResponseTemplate::new(204)) + .expect(1) + .mount(&server) + .await; + + unstack(&client(&server), "o", "r", 7).await.unwrap(); + } + + #[tokio::test] + async fn unstack_treats_404_as_already_dissolved() { + // A stale stack number (someone unstacked by hand between our + // fetch and now) satisfies the postcondition, so it must not + // block the push. + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(wm_path("/repos/o/r/stacks/7/unstack")) + .respond_with(ResponseTemplate::new(404).set_body_string("Not Found")) + .mount(&server) + .await; + + unstack(&client(&server), "o", "r", 7).await.unwrap(); + } + + #[tokio::test] + async fn unstack_failure_is_fatal_and_explains_itself() { + // The one asymmetry with `register`: carrying on past a failed + // unstack is what permanently closes a surviving PR, so this + // must stop the push before it mutates anything. + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(wm_path("/repos/o/r/stacks/7/unstack")) + .respond_with(ResponseTemplate::new(403).set_body_string("forbidden")) + .mount(&server) + .await; + + let err = unstack(&client(&server), "o", "r", 7).await.unwrap_err(); + let msg = err.to_string(); + assert!( + msg.contains("could not dissolve GitHub stack #7"), + "got: {msg}" + ); + assert!(msg.contains("unstack it by hand"), "got: {msg}"); + } +} diff --git a/crates/mergify-stack/src/pr_upsert.rs b/crates/mergify-stack/src/pr_upsert.rs index 5e3e85cf..bbce795a 100644 --- a/crates/mergify-stack/src/pr_upsert.rs +++ b/crates/mergify-stack/src/pr_upsert.rs @@ -2,7 +2,8 @@ //! //! - [`create_or_update_pr`] — the `Create` action `POST`s a //! fresh PR; `Update` `PATCH`es the existing one with refreshed -//! `head`/`base`/`title`/`body`. Body always goes through +//! `head`/`title`/`body`, plus `base` only when the PR is really +//! being retargeted (see [`base_if_changed`]). Body always goes through //! [`crate::push_helpers::format_pull_description`] so the //! `Change-Id:` trailer is stripped and the rendered //! `Depends-On:` header points at the current predecessor PR. @@ -63,7 +64,8 @@ pub struct PrUpsertInput<'a> { #[derive(Serialize)] struct UpdateBodyBoth<'a> { head: &'a str, - base: &'a str, + #[serde(skip_serializing_if = "Option::is_none")] + base: Option<&'a str>, title: &'a str, body: String, } @@ -71,7 +73,8 @@ struct UpdateBodyBoth<'a> { #[derive(Serialize)] struct UpdateBodyKeepTitle<'a> { head: &'a str, - base: &'a str, + #[serde(skip_serializing_if = "Option::is_none")] + base: Option<&'a str>, body: String, } @@ -84,6 +87,34 @@ struct CreateBody<'a> { base: &'a str, } +/// The `base` value to PATCH, or `None` when the PR already targets +/// `planned` and the key must be left out of the body entirely. +/// +/// Omitting it is not just economy. GitHub treats the *presence* of +/// `base` as a retarget attempt: while a PR belongs to a native stack, +/// `PATCH /repos/{owner}/{repo}/pulls/{n}` carrying `base` fails with +/// 422 *"Cannot change the base branch because the pull request is +/// part of a stack"* even when the value is the one the PR already has +/// (measured against the live API, 2026-08-05). Sending it only when +/// it actually moves is what lets a routine push run over a registered +/// stack untouched — see [`crate::native_stack`]. +/// +/// A payload without `base.ref` (a mock, a proxy, a future API +/// version) falls back to sending the key, which is what this code did +/// unconditionally before. +/// +/// `head` needs no such care: it is not an updatable field, and GitHub +/// ignores it silently — a PATCH carrying a `head` that does not even +/// exist is a 200 that changes nothing, stacked or not (measured the +/// same day). We keep sending it for parity with the Python +/// implementation. +fn base_if_changed<'a>(pull: &Value, planned: &'a str) -> Option<&'a str> { + match pull.pointer("/base/ref").and_then(Value::as_str) { + Some(current) if current == planned => None, + _ => Some(planned), + } +} + /// Upsert the PR for `input.action` and return the PR payload. /// /// `Update` returns the existing pull verbatim (Python does the @@ -113,6 +144,8 @@ pub async fn create_or_update_pr( .ok_or_else(|| CliError::Generic("update pull payload missing `number`".into()))?; let path = format!("/repos/{user}/{repo}/pulls/{number}"); + let base = base_if_changed(pull, input.base_branch); + // Two PATCH body shapes for the same endpoint: when // `keep_pull_request_title_and_body` is true we want // GitHub to leave `title` alone, so we just don't @@ -123,14 +156,14 @@ pub async fn create_or_update_pr( let existing_body = pull.get("body").and_then(Value::as_str).unwrap_or(""); let body = UpdateBodyKeepTitle { head: input.dest_branch, - base: input.base_branch, + base, body: format_pull_description(existing_body, input.depends_on_number), }; let _: Value = client.patch(&path, &body).await?; } else { let body = UpdateBodyBoth { head: input.dest_branch, - base: input.base_branch, + base, title: input.title, body: format_pull_description(input.message, input.depends_on_number), }; @@ -308,6 +341,8 @@ mod tests { let existing = json!({ "number": 42, "body": "old body\n\nDepends-On: #999", + // Base is moving (stack/x → main), so the key is sent. + "base": {"ref": "stack/x"}, }); Mock::given(method("PATCH")) .and(wm_path("/repos/o/r/pulls/42")) @@ -343,6 +378,117 @@ mod tests { assert_eq!(body["body"], "feat: x\n\nfresh body"); } + #[tokio::test] + async fn update_omits_base_when_the_pull_already_targets_it() { + // The routine push: nothing was reordered, so the PR's base is + // the one the planner picked. Sending it anyway is what makes + // GitHub 422 the PATCH while the PR is in a native stack, even + // though nothing is moving. + let server = MockServer::start().await; + let existing = json!({ + "number": 42, + "body": "old body", + "base": {"ref": "stack/tester/feature/a--Iaaaaaa"}, + }); + Mock::given(method("PATCH")) + .and(wm_path("/repos/o/r/pulls/42")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({}))) + .mount(&server) + .await; + + let input = PrUpsertInput { + action: Action::Update, + title: "feat: x", + message: "feat: x", + dest_branch: "jd/feature/Ibbbbbb", + base_branch: "stack/tester/feature/a--Iaaaaaa", + pull: Some(&existing), + depends_on_number: None, + create_as_draft: false, + keep_pull_request_title_and_body: false, + }; + create_or_update_pr(&client(&server), "o", "r", input) + .await + .unwrap(); + + let body = request_body(&server.received_requests().await.unwrap()[0]); + assert!( + body.get("base").is_none(), + "an unchanged base must not be sent at all, got: {body}", + ); + // The rest of the update still happens. + assert_eq!(body["title"], "feat: x"); + assert_eq!(body["head"], "jd/feature/Ibbbbbb"); + } + + #[tokio::test] + async fn update_sends_base_when_the_pull_is_actually_retargeted() { + // The reorder/drop path: the planner moved this PR, so the key + // has to be there — and this is the case that legitimately + // needs the native stack dissolved first. + let server = MockServer::start().await; + let existing = json!({ + "number": 42, + "body": "old body", + "base": {"ref": "stack/tester/feature/a--Iaaaaaa"}, + }); + Mock::given(method("PATCH")) + .and(wm_path("/repos/o/r/pulls/42")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({}))) + .mount(&server) + .await; + + let input = PrUpsertInput { + action: Action::Update, + title: "feat: x", + message: "feat: x", + dest_branch: "jd/feature/Ibbbbbb", + base_branch: "main", + pull: Some(&existing), + depends_on_number: None, + create_as_draft: false, + keep_pull_request_title_and_body: false, + }; + create_or_update_pr(&client(&server), "o", "r", input) + .await + .unwrap(); + + let body = request_body(&server.received_requests().await.unwrap()[0]); + assert_eq!(body["base"], "main"); + } + + #[tokio::test] + async fn update_sends_base_when_the_payload_does_not_say_what_it_is() { + // Unknown current base → behave as this code always did and + // send it. Better a redundant key than a PR left on a stale + // base because a payload was missing a field. + let server = MockServer::start().await; + let existing = json!({"number": 42, "body": "old body"}); + Mock::given(method("PATCH")) + .and(wm_path("/repos/o/r/pulls/42")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({}))) + .mount(&server) + .await; + + let input = PrUpsertInput { + action: Action::Update, + title: "feat: x", + message: "feat: x", + dest_branch: "b", + base_branch: "main", + pull: Some(&existing), + depends_on_number: None, + create_as_draft: false, + keep_pull_request_title_and_body: true, + }; + create_or_update_pr(&client(&server), "o", "r", input) + .await + .unwrap(); + + let body = request_body(&server.received_requests().await.unwrap()[0]); + assert_eq!(body["base"], "main"); + } + #[tokio::test] async fn update_with_keep_title_omits_title_and_rewrites_body_from_existing() { // The existing PR body's `Depends-On: #999` gets rewritten diff --git a/crates/mergify-stack/src/stack_context.rs b/crates/mergify-stack/src/stack_context.rs index cc851ec8..defb453f 100644 --- a/crates/mergify-stack/src/stack_context.rs +++ b/crates/mergify-stack/src/stack_context.rs @@ -257,6 +257,21 @@ pub fn resolve_default_revision_history(repo_dir: Option<&Path>) -> bool { .map_or(true, |v| v != "false") } +/// `mergify-cli.stack-github-native`, defaulting to `false`. +/// +/// Opt-in like `stack-create-as-draft`: only the literal string +/// `"true"` enables it, so a stray value can't silently start +/// registering stacks with GitHub — which changes how those pull +/// requests can be merged (see [`crate::native_stack`]). +#[must_use] +pub fn resolve_default_github_native(repo_dir: Option<&Path>) -> bool { + run_git_capture( + repo_dir, + &["config", "--get", "mergify-cli.stack-github-native"], + ) + .is_ok_and(|v| v == "true") +} + #[cfg(test)] mod tests { use super::*; diff --git a/skills/mergify-stack/SKILL.md b/skills/mergify-stack/SKILL.md index 1b262665..b29bcbc3 100644 --- a/skills/mergify-stack/SKILL.md +++ b/skills/mergify-stack/SKILL.md @@ -58,6 +58,7 @@ A branch is a stack. Keep stacks short and focused: ```bash mergify stack new NAME # Create a new stack/branch for new work mergify stack push # Push and create/update PRs +mergify stack push --github-native # ...and register it as a GitHub-native stack (opt-in) mergify stack checkout NAME # Checkout an existing stack from GitHub (e.g. someone else's) mergify stack sync # Fetch trunk, remove merged commits, rebase mergify stack list # Show commit <-> PR mapping for current stack @@ -88,6 +89,36 @@ Use `mergify stack sync` to bring your stack up to date. It fetches the latest t Use `mergify stack list` to see which commits have been pushed, which PRs they map to, and whether the stack is up to date with the remote. It also shows CI status, review status, and merge conflicts for each PR. Use `--verbose` for detailed check names and reviewer names. Use `--json` when you need to parse the output programmatically — it includes full CI check details and review data. +## GitHub-native stacks (experimental, opt-in) + +`mergify stack push --github-native` additionally registers the stack with +GitHub's own Stacks API, so GitHub renders it as a stack. Off by default; turn +it on per repo with `git config mergify-cli.stack-github-native true`. + +It is *additive* — Change-Ids, branch layout, stack comments and revision +history are unchanged — and it degrades quietly: where the API isn't available +(older GitHub Enterprise, a repo without the feature) the push reports +`not registered on GitHub` and succeeds exactly as it would have. + +Two things to know before turning it on: + +- **A stack needs at least 2 pull requests.** GitHub rejects a 1-PR stack, so a + single-change stack stays a plain PR — and a 2-PR stack that loses a member + is dissolved rather than re-registered. +- **Registering changes how the PRs merge.** While a stack is registered, + GitHub refuses the classic merge endpoint + (`PUT /repos/{owner}/{repo}/pulls/{pull_number}/merge` → 403) for + its members. That is GitHub's contract, not ours; it is the reason this is + opt-in. + +Pushing stays cheap. Refreshing commits (amend, reword, force-push) leaves the +registration untouched, and adding a change on top extends the same stack. +Only a push that moves a pull request's base — a reorder, a drop, a change +inserted in the middle — dissolves the registration first and rebuilds it at +the end, because GitHub rejects any base-branch change while a PR is stacked. +An interrupted push therefore leaves the stack merely unregistered — never +half-registered — and the next push repairs it. + ## Amend Notes `mergify stack note` records *why* a commit was amended. The note travels with the stack: