From befb4f430837dd6157ecc9146cda2d982049056b Mon Sep 17 00:00:00 2001 From: Mehdi ABAAKOUK Date: Wed, 9 Sep 2026 21:56:03 +0200 Subject: [PATCH 1/3] refactor(cli): assert the clap env hook is absent without mutating the environment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two tests that pinned "clap must not read `MERGIFY_*` itself" each exported one variable empty and parsed one argv. They were the last two `temp_env` calls in `mergify-cli`, and the only ones whose reader was a dependency rather than our own code. They are two tests now, at the two altitudes the rule lives at. A walk over the built `clap::Command` tree asserts that no argument anywhere declares `env = "…"`, which catches the hook at declaration and covers every argument rather than `--config` and `--test-exit-code`. A parse of each of those two argvs asserts the observable property the deleted tests asserted, because the walk only sees that one spelling: `default_value_t = std::env::var(…).unwrap_or_default()` reproduces monorepo#33423 exactly and declares no hook. Neither test needs an environment. `MERGIFY_BASE_URL` joins the rest in treating exported-but-empty as unset. `install.sh` already guards the same lever with `[ -n … ]`, so the two halves of one feature disagreed: an empty value built the URL `/latest-release.json`, which reqwest rejects as relative, instead of falling back to the default host. The rest is plumbing: `MERGIFY_CLI_TESTING_UTF8_MODE`, `NO_COLOR` / `FORCE_COLOR` / `CLICOLOR_FORCE` and `self_update`'s `MERGIFY_BASE_URL` read through `mergify_core::env`. That is the `env` in scope in both files now; `args()` and `current_exe()` are spelled `std::env::`, since neither is the environment. Co-Authored-By: Claude Opus 5 Change-Id: I512c50f33a89380f3d9f4a8f7a37725a875d6804 --- Cargo.lock | 1 - crates/mergify-cli/Cargo.toml | 1 - crates/mergify-cli/src/main.rs | 128 +++++++++++++------------- crates/mergify-cli/src/self_update.rs | 37 +++++++- 4 files changed, 102 insertions(+), 65 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index fda3f33a..1516c449 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1811,7 +1811,6 @@ dependencies = [ "serde_json", "serde_yaml_ng", "sha2 0.11.0", - "temp-env", "tempfile", "tokio", "tracing", diff --git a/crates/mergify-cli/Cargo.toml b/crates/mergify-cli/Cargo.toml index f10d5762..3bbdf97d 100644 --- a/crates/mergify-cli/Cargo.toml +++ b/crates/mergify-cli/Cargo.toml @@ -57,7 +57,6 @@ url = { workspace = true } insta = { workspace = true } regex = { workspace = true } serde_yaml_ng = { workspace = true } -temp-env = { workspace = true } tempfile = { workspace = true } tokio = { workspace = true, features = ["rt-multi-thread"] } wiremock = { workspace = true } diff --git a/crates/mergify-cli/src/main.rs b/crates/mergify-cli/src/main.rs index 42c6daa4..3fbe63f0 100644 --- a/crates/mergify-cli/src/main.rs +++ b/crates/mergify-cli/src/main.rs @@ -8,7 +8,7 @@ //! ``?" suggestion off clap's built-in Levenshtein //! distance. -use std::env; +use mergify_core::env; use std::io::IsTerminal; use std::path::PathBuf; use std::process::ExitCode; @@ -53,7 +53,7 @@ mod self_update; const VERSION: &str = env!("MERGIFY_CLI_VERSION"); fn main() -> ExitCode { - let argv: Vec = env::args().skip(1).collect(); + let argv: Vec = std::env::args().skip(1).collect(); // Test hook used by `test_binary_build.py` to verify the // wheel-installed binary produces UTF-8 output (especially on @@ -2812,12 +2812,12 @@ enum ColorArg { /// `true` when `name` is exported to something other than the empty /// string, which is what means by "present". /// -/// The rule itself lives in `mergify_core::env::var_non_empty`, which -/// is where it is documented and tested; spelling it out a second time -/// here is how the color variables would drift away from every other -/// variable this CLI reads. +/// The rule itself lives in `mergify_core::env`, which is where it is +/// documented and tested; spelling it out a second time here is how +/// the color variables would drift away from every other variable +/// this CLI reads. fn non_empty(name: &str) -> bool { - mergify_core::env::var_non_empty(name).is_some() + env::var_os_non_empty(name).is_some() } /// Whether the log subscriber may emit ANSI on stderr. @@ -3984,8 +3984,9 @@ struct ScopesCliArgs { // `mergify_ci::scopes_detect::resolve_config_path` instead, // where empty correctly falls through to auto-detect. The // matching regression tests are - // `ci_scopes_parses_when_mergify_config_path_env_var_is_empty` - // (clap parse) and + // `no_argument_takes_its_value_from_the_environment` (asks the + // built `Command` whether *any* argument carries an `env` + // attribute, which covers this one) and // `resolve_config_path_treats_empty_env_var_as_unset` // (lower-level resolver). #[arg(long)] @@ -4897,72 +4898,75 @@ mod tests { // or test the terminal the suite happens to run under. } + /// No argument anywhere in the tree may take its value from the + /// environment through clap's `env = "…"` attribute. + /// + /// Twice now that attribute broke a caller who exports the + /// variable empty. `gha-mergify-ci` sets `MERGIFY_CONFIG_PATH=""` + /// when the user pinned no path, and clap read the empty string + /// as a present-but-empty `--config`, aborting with "a value is + /// required for '--config'" (monorepo#33423). Same shape for + /// `MERGIFY_TEST_EXIT_CODE=""` and `--test-exit-code`: "cannot + /// parse integer from empty string". Env lookup belongs in the + /// resolver, where `mergify_core::env::var_non_empty` treats + /// empty as unset. + /// + /// This replaces two tests that each exported one variable empty + /// and parsed one argv. Asking the built `Command` covers every + /// argument rather than those two, and needs no process + /// environment to mutate. #[test] - fn ci_scopes_parses_when_mergify_config_path_env_var_is_empty() { - // Regression for monorepo#33423 / gha-mergify-ci: - // the action sets `MERGIFY_CONFIG_PATH=""` (empty) when - // the caller didn't pin a config path, expecting - // auto-detect. The previous `ScopesCliArgs::config` - // declaration used `env = "MERGIFY_CONFIG_PATH"` on - // clap's side, which interpreted the empty env value as - // a present-but-empty `--config` flag and exited parsing - // with `a value is required for '--config'`. The clap - // env hook has been dropped — env lookup lives inside - // `scopes_detect::resolve_config_path` where empty is - // correctly treated as unset. Pin that here so the hook - // can't sneak back in. - let parsed = temp_env::with_var("MERGIFY_CONFIG_PATH", Some(""), || { - CliRoot::try_parse_from([ - "mergify".to_string(), - "ci".to_string(), - "scopes".to_string(), - "--write".to_string(), - "scopes.json".to_string(), - ]) - .expect("argv parses with empty MERGIFY_CONFIG_PATH") - }); + fn no_argument_takes_its_value_from_the_environment() { + fn walk(cmd: &clap::Command, path: &str, found: &mut Vec) { + for arg in cmd.get_arguments() { + if let Some(var) = arg.get_env() { + found.push(format!( + "{path} {} <- {}", + arg.get_id(), + var.to_string_lossy() + )); + } + } + for sub in cmd.get_subcommands() { + walk(sub, &format!("{path} {}", sub.get_name()), found); + } + } + + let mut found = Vec::new(); + walk(&CliRoot::command(), "mergify", &mut found); + assert!(found.is_empty(), "clap env hooks found: {found:#?}"); + } + + /// The observable half of the rule above, for the two arguments + /// it was reported on. + /// + /// The walk asks clap whether an `env = "…"` hook is declared, + /// which is the spelling that caused both regressions but not the + /// only one: `default_value_t = std::env::var(…).unwrap_or_default()` + /// or a `value_parser` that reads the environment reproduce it + /// exactly and declare no hook. This asserts what the user sees + /// instead. It needs no environment of its own — with the + /// variable unset, any of those spellings still surfaces a + /// present-but-empty value where `None` is required. + #[test] + fn an_omitted_flag_stays_omitted() { + let parsed = CliRoot::try_parse_from(["mergify", "ci", "scopes", "--write", "scopes.json"]) + .expect("argv parses"); let Dispatch::Native(NativeCommand::CiScopes(opts)) = dispatch_from_parsed(parsed) else { panic!("ci scopes must dispatch natively"); }; - // `--config` was never supplied; the empty env var must - // not surface as a value (which would change the - // downstream resolver's branch). assert!(opts.config.is_none(), "got: {:?}", opts.config); - } - #[test] - fn ci_junit_process_parses_when_mergify_test_exit_code_env_var_is_empty() { - // Second instance of the same class of regression as - // `ci_scopes_parses_when_…`: `gha-mergify-ci` exports - // `MERGIFY_TEST_EXIT_CODE=""` when the previous step - // didn't produce a runner exit code. Previously the clap - // `env = "MERGIFY_TEST_EXIT_CODE"` attribute on - // `--test-exit-code` tried to parse `""` as `i32` and - // exited parsing with `invalid value '' for - // '--test-exit-code': cannot parse integer from empty - // string`. The clap env hook has been dropped — env - // lookup lives in `junit_process::command::resolve_test_exit_code` - // where empty is correctly treated as `None`. Pin that - // here so the hook can't sneak back in. - let parsed = temp_env::with_var("MERGIFY_TEST_EXIT_CODE", Some(""), || { - CliRoot::try_parse_from([ - "mergify".to_string(), - "ci".to_string(), - "junit-process".to_string(), - "report.xml".to_string(), - ]) - .expect("argv parses with empty MERGIFY_TEST_EXIT_CODE") - }); + let parsed = CliRoot::try_parse_from(["mergify", "ci", "junit-process", "report.xml"]) + .expect("argv parses"); let Dispatch::Native(NativeCommand::CiJunitProcess(opts)) = dispatch_from_parsed(parsed) else { panic!("ci junit-process must dispatch natively"); }; - // `--test-exit-code` was never supplied; the empty env - // var must not surface as a value. assert!( opts.test_exit_code.is_none(), "got: {:?}", - opts.test_exit_code, + opts.test_exit_code ); } diff --git a/crates/mergify-cli/src/self_update.rs b/crates/mergify-cli/src/self_update.rs index 7c0b0000..f3e96d03 100644 --- a/crates/mergify-cli/src/self_update.rs +++ b/crates/mergify-cli/src/self_update.rs @@ -35,6 +35,7 @@ use std::path::Path; use std::time::Duration; use mergify_core::CliError; +use mergify_core::env; use serde::Deserialize; use sha2::Digest; use sha2::Sha256; @@ -58,7 +59,7 @@ const BASE_URL_ENV: &str = "MERGIFY_BASE_URL"; /// read from the response, never reconstructed, so we only need to /// know where the metadata lives. fn latest_release_url() -> String { - if let Ok(base) = std::env::var(BASE_URL_ENV) { + if let Some(base) = env::var_non_empty(BASE_URL_ENV) { format!("{base}/latest-release.json") } else { format!("{DEFAULT_API_BASE}/repos/{REPO}/releases/latest") @@ -443,6 +444,40 @@ mod tests { } } + #[test] + fn latest_release_url_falls_back_when_the_base_url_is_empty() { + // `gha-mergify-ci` exports unset variables as `""`, so an + // empty `MERGIFY_BASE_URL` must mean "no fixture" and not + // "fetch from `/latest-release.json`". This is the + // `var_non_empty` half of the empty-string rule; the + // overlay is what lets the case be written at all, since + // nothing may mutate the process environment. + let url = env::testing::with_var(BASE_URL_ENV, Some(""), latest_release_url); + assert_eq!( + url, + format!("{DEFAULT_API_BASE}/repos/{REPO}/releases/latest") + ); + } + + #[test] + fn latest_release_url_uses_a_non_empty_base_url() { + let url = env::testing::with_var( + BASE_URL_ENV, + Some("https://example.test"), + latest_release_url, + ); + assert_eq!(url, "https://example.test/latest-release.json"); + } + + #[test] + fn latest_release_url_defaults_when_the_base_url_is_unset() { + let url = env::testing::with_no_vars(latest_release_url); + assert_eq!( + url, + format!("{DEFAULT_API_BASE}/repos/{REPO}/releases/latest") + ); + } + #[test] fn select_asset_matches_versioned_name() { let assets = [ From 8ed6d62e69ab54a35a06b8b086578a60a0221320 Mon Sep 17 00:00:00 2001 From: Mehdi ABAAKOUK Date: Fri, 18 Sep 2026 08:55:15 +0200 Subject: [PATCH 2/3] refactor(auth): read the environment through the funnel `mergify-auth` landed while this stack was in flight and its production side already reads through `mergify_core::env`, since `var_non_empty` predates the funnel. Only its tests were left: `machine`'s `COMPUTERNAME` / `HOSTNAME` chain, `browser`'s `SSH_CONNECTION` / `DISPLAY` / `WAYLAND_DISPLAY` probes, and `with_mergify_token`. They install an overlay now, like everywhere else, so the crate stops mutating the process environment and drops its `temp-env` dependency. `with_mergify_token` keeps its shape: the overlay is on this thread and the `current_thread` runtime it builds drives the future on that same thread, so the closure form still works and no call site changes. Co-Authored-By: Claude Opus 5 Change-Id: I7d4c755cee8cc90db511ea671e5c23215300c03c --- Cargo.lock | 10 ---------- crates/mergify-auth/Cargo.toml | 1 - crates/mergify-auth/src/browser.rs | 8 ++++---- crates/mergify-auth/src/lib.rs | 15 ++++++++------- crates/mergify-auth/src/machine.rs | 6 +++--- 5 files changed, 15 insertions(+), 25 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1516c449..dc65e64c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1753,7 +1753,6 @@ dependencies = [ "mergify-tui", "serde", "serde_json", - "temp-env", "tempfile", "tokio", "tracing", @@ -3099,15 +3098,6 @@ dependencies = [ "syn 2.0.117", ] -[[package]] -name = "temp-env" -version = "0.3.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "96374855068f47402c3121c6eed88d29cb1de8f3ab27090e273e420bdabcf050" -dependencies = [ - "parking_lot", -] - [[package]] name = "tempfile" version = "3.27.0" diff --git a/crates/mergify-auth/Cargo.toml b/crates/mergify-auth/Cargo.toml index 60d09e4b..7bba6753 100644 --- a/crates/mergify-auth/Cargo.toml +++ b/crates/mergify-auth/Cargo.toml @@ -25,7 +25,6 @@ url = { workspace = true } mergify-core = { path = "../mergify-core", features = ["test-support"] } mergify-test-support = { path = "../mergify-test-support" } serde_json = { workspace = true } -temp-env = { workspace = true } tempfile = { workspace = true } tokio = { workspace = true, features = ["rt-multi-thread"] } wiremock = { workspace = true } diff --git a/crates/mergify-auth/src/browser.rs b/crates/mergify-auth/src/browser.rs index 827328d1..c5776570 100644 --- a/crates/mergify-auth/src/browser.rs +++ b/crates/mergify-auth/src/browser.rs @@ -231,7 +231,7 @@ mod tests { #[cfg(target_os = "macos")] #[test] fn macos_opens_the_url_with_open() { - let command = temp_env::with_vars( + let command = mergify_core::env::testing::with_vars( [("SSH_CONNECTION", None::<&str>), ("SSH_TTY", None::<&str>)], || command_for("https://dashboard.mergify.com/device"), ) @@ -246,7 +246,7 @@ mod tests { #[cfg(all(unix, not(target_os = "macos")))] #[test] fn a_graphical_session_gets_xdg_open() { - let command = temp_env::with_vars( + let command = mergify_core::env::testing::with_vars( [("DISPLAY", Some(":0")), ("WAYLAND_DISPLAY", None::<&str>)], || command_for("https://dashboard.mergify.com/device"), ) @@ -263,7 +263,7 @@ mod tests { #[cfg(target_os = "macos")] #[test] fn an_ssh_session_to_a_mac_opens_nothing() { - let opened = temp_env::with_vars( + let opened = mergify_core::env::testing::with_vars( [ ("SSH_CONNECTION", Some("10.0.0.1 52000 10.0.0.2 22")), ("SSH_TTY", None), @@ -281,7 +281,7 @@ mod tests { #[cfg(all(unix, not(target_os = "macos")))] #[test] fn a_headless_session_opens_nothing() { - let opened = temp_env::with_vars( + let opened = mergify_core::env::testing::with_vars( [("DISPLAY", None::<&str>), ("WAYLAND_DISPLAY", None::<&str>)], || command_for("https://dashboard.mergify.com/device").is_ok(), ); diff --git a/crates/mergify-auth/src/lib.rs b/crates/mergify-auth/src/lib.rs index dc4e4eb7..863f7186 100644 --- a/crates/mergify-auth/src/lib.rs +++ b/crates/mergify-auth/src/lib.rs @@ -41,17 +41,18 @@ mod testing { /// Run `body` to completion with `MERGIFY_TOKEN` forced to /// `value`. /// - /// `temp_env` cannot wrap an `.await`, so the future is driven - /// inside the closure instead. Without this the wiring that - /// reads the variable is untestable, and untestable wiring is - /// wiring a future edit can delete with the suite still green: - /// asserting on the renderer alone proves only that the renderer - /// can print a note, never that anything asks it to. + /// The overlay is installed on this thread and the future is + /// driven on it, by a `current_thread` runtime built here. + /// Without this the wiring that reads the variable is + /// untestable, and untestable wiring is wiring a future edit can + /// delete with the suite still green: asserting on the renderer + /// alone proves only that the renderer can print a note, never + /// that anything asks it to. pub fn with_mergify_token(value: Option<&str>, body: F) -> F::Output { let runtime = tokio::runtime::Builder::new_current_thread() .enable_all() .build() .unwrap(); - temp_env::with_var("MERGIFY_TOKEN", value, || runtime.block_on(body)) + mergify_core::env::testing::with_var("MERGIFY_TOKEN", value, || runtime.block_on(body)) } } diff --git a/crates/mergify-auth/src/machine.rs b/crates/mergify-auth/src/machine.rs index ec730038..1672b077 100644 --- a/crates/mergify-auth/src/machine.rs +++ b/crates/mergify-auth/src/machine.rs @@ -111,7 +111,7 @@ mod tests { // the variable instead. #[test] fn the_variables_are_the_fallback() { - let from_windows = temp_env::with_vars( + let from_windows = mergify_core::env::testing::with_vars( [ ("COMPUTERNAME", Some("WIN-BOX")), ("HOSTNAME", Some("ignored")), @@ -120,13 +120,13 @@ mod tests { ); assert_eq!(from_windows.as_deref(), Some("WIN-BOX")); - let from_shell = temp_env::with_vars( + let from_shell = mergify_core::env::testing::with_vars( [("COMPUTERNAME", None), ("HOSTNAME", Some("build-42"))], from_env, ); assert_eq!(from_shell.as_deref(), Some("build-42")); - let from_nothing = temp_env::with_vars( + let from_nothing = mergify_core::env::testing::with_vars( [("COMPUTERNAME", None::<&str>), ("HOSTNAME", None)], from_env, ); From 2e66ef54c97bc7e0274307a54f7513b917cb7217 Mon Sep 17 00:00:00 2001 From: Julien Danjou Date: Mon, 21 Sep 2026 12:28:36 +0200 Subject: [PATCH 3/3] feat(stack): hand the stack's member list to GitHub's native UI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `mergify stack push` posted a sticky "This pull request is part of a Mergify stack" comment on every member, carrying a table of the whole stack. GitHub's native Stacks UI now renders that list on the pull request page itself, so the comment is a second copy of what the reader is already looking at — one they have to reconcile against the first whenever the two disagree. So a push whose stack GitHub registered posts no table, and deletes the one an earlier push left on each open member. **Keyed off the registration, not off the flag.** This is the same argument as the `Depends-On:` header one step up, and it gets the same treatment for the same reason: `native_stack::register` is explicitly allowed to do nothing — an older GitHub Enterprise, a repo without the feature, a chain GitHub will not accept, or `--no-github-native`. Drop the comment unconditionally and those pushes end up with no member list anywhere, which is a worse place than where they started. A push that did not register keeps posting and updating the table exactly as before. The settling moves to the end of the push (new step 13), next to the header restore it mirrors, because the registration's outcome is only known there. **Comments already on live stacks are deleted, not left or rewritten.** Leaving them is the tempting option and the wrong one: nothing refreshes a table we no longer write, so each would sit frozen at the membership of the last pre-native push, under a live list that keeps moving — stale next to accurate is worse than absent next to accurate. A one-time rewrite to a tombstone would leave permanent noise on every member of every stack ever pushed. Deleting costs nothing recoverable: the comment was ours, bot-authored, and carried no reply thread. The scan that finds it is the GET the upsert already did on every push, so this is not a new request — it is the same one, ending in a DELETE instead of a PATCH. Merged members are skipped, as the upsert always skipped them. Only the fallback upsert is gated on the stack having more than one pull request, where the count saves a genuinely one-change stack a GET that can only come back empty. The removal is not: a stack whose members merge down to a single open pull request keeps its registration, so that last member is still carrying the table it was given while the stack had several — exactly the frozen list this is taking down. The gate costs the removal nothing anyway, since GitHub rejects a stack below two members and an unregistered stack never reaches the removal. The delete is best-effort: it changes nothing about how the stack merges, so a push that has already done everything else reports it and retries on the next push rather than failing. That is the opposite policy from the header restore, which is fatal precisely because an unregistered, unchained stack merges out of order. **The revision-history comment is untouched.** GitHub renders nothing like it, which is what the sticky-comment surface exists for now. Also corrects a stale claim in `stack_comment`'s docstring: it said `mergify stack checkout` rebuilds a stack from the `` JSON marker. It does not, and nothing in this repo reads that marker — `checkout` discovers a stack by chaining each pull request's `head.ref` to the next one's `base.ref`, which has the advantage of working on stacks this CLI never touched. The marker's readers are out of tree, so its wire shape stays pinned. MRGFY-9496 Co-Authored-By: Claude Opus 5 (1M context) Change-Id: I4d272321c1cfcb3b836a1fe1ac2ef0ce463ec550 --- .../tests/stack_push_github_native.rs | 220 +++++++++++++++++- crates/mergify-stack/src/commands/push.rs | 185 +++++++++++---- crates/mergify-stack/src/comment_upsert.rs | 179 +++++++++++++- crates/mergify-stack/src/lib.rs | 4 +- crates/mergify-stack/src/stack_comment.rs | 33 ++- skills/mergify-stack/SKILL.md | 15 +- 6 files changed, 573 insertions(+), 63 deletions(-) diff --git a/crates/mergify-cli/tests/stack_push_github_native.rs b/crates/mergify-cli/tests/stack_push_github_native.rs index fa70df85..94323467 100644 --- a/crates/mergify-cli/tests/stack_push_github_native.rs +++ b/crates/mergify-cli/tests/stack_push_github_native.rs @@ -17,7 +17,10 @@ //! 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. +//! test on the module in isolation can catch it; +//! - the two surfaces keyed off the registration's outcome — the +//! `Depends-On:` header and the sticky stack comment — must follow +//! the outcome and not the flag, in both directions. use std::path::{Path, PathBuf}; use std::process::Command; @@ -447,6 +450,221 @@ async fn depends_on_restored_when_native_registration_does_not_happen() { ); } +/// The `url` GitHub puts on an issue comment, as an absolute URL — +/// which is what the CLI has to turn back into a path before it can +/// call the API with it. +fn comment_url(server: &MockServer, id: u64) -> String { + format!("{}/repos/myorg/myrepo/issues/comments/{id}", server.uri()) +} + +/// Every `METHOD /path` the server saw against the issue-comments +/// endpoints, which is where both stack-comment surfaces live. +async fn comment_requests(server: &MockServer) -> Vec { + request_log(server) + .await + .into_iter() + .filter(|r| r.contains("/issues/") && r.contains("comments")) + .collect() +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn no_stack_comment_is_posted_when_native_registration_succeeds() { + // GitHub's own Stacks UI lists the members on the pull request + // page, so the sticky table is a second copy of what the reader is + // already looking at — the same argument as the `Depends-On:` + // header one surface up. + 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}))) + .mount(&server) + .await; + + assert_success(&run_push(&local, &server.uri(), &[])); + + let comments = comment_requests(&server).await; + assert!( + comments.iter().all(|r| r.starts_with("GET ")), + "a registered stack must have nothing written to its comments, got: {comments:#?}", + ); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_stack_comment_from_an_older_push_is_deleted_once_github_holds_the_stack() { + // The migration half. Every stack pushed before this behaviour + // shipped carries the table, and nothing refreshes it any more, so + // it would sit frozen at that push's membership under a live list + // that keeps moving. Take it down on the next push instead. + let (work, _) = build_stack_repo(2); + let local = work.path().join("local"); + let server = mock_github_creating(&[101, 102]).await; + // Outranks `mock_github_creating`'s empty listing: wiremock breaks + // a tie between two matching mocks by mount order, and 1 beats the + // default 5 regardless of it. + 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!([ + {"url": comment_url(&server, 900), "body": "looks good to me"}, + { + "url": comment_url(&server, 901), + "body": "This pull request is part of a [Mergify stack](https://docs.mergify.com/stacks/):\n| # | Pull Request | Link | |\n", + }, + ]))) + .with_priority(1) + .mount(&server) + .await; + // Only ours is deletable: a DELETE aimed at comment 900 would 404 + // here and fail the push. + Mock::given(method("DELETE")) + .and(wm_path("/repos/myorg/myrepo/issues/comments/901")) + .respond_with(ResponseTemplate::new(204)) + .expect(2) + .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": 12}))) + .mount(&server) + .await; + + let output = run_push(&local, &server.uri(), &[]); + assert_success(&output); + + let comments = comment_requests(&server).await; + assert!( + comments.iter().all(|r| !r.starts_with("POST ")), + "the comment is taken down, never rewritten, got: {comments:#?}", + ); + let out = String::from_utf8_lossy(&output.stdout) + String::from_utf8_lossy(&output.stderr); + assert!( + out.contains("removed 2 stack comments"), + "the cleanup should be stated, got: {out}", + ); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_registered_stack_down_to_one_live_pull_request_still_loses_its_comment() { + // The stale-comment case the member count would hide. A stack + // whose members merge down to one open pull request keeps its + // registration — `appendable_tail` finds nothing to add, so GitHub + // goes on holding stack #7 — and that last pull request is still + // carrying the table it was given while the stack had several + // members. Counting pull requests before deciding to look would + // skip exactly this one, leaving a frozen list under GitHub's live + // one for ever. + // + // The count still gates the fallback upsert, and that costs + // nothing here: GitHub rejects a stack below two members, so a + // genuinely one-change stack is never registered and never reaches + // 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!({ + "number": 101, + "state": "open", + "merged_at": null, + "draft": false, + "title": "existing", + "body": "existing", + "head": {"ref": head_ref(&change_ids, 0), "sha": remote_head}, + "base": {"ref": "main"}, + "html_url": "https://github.com/myorg/myrepo/pull/101", + // 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; + Mock::given(method("GET")) + .and(wm_path("/repos/myorg/myrepo/pulls/101/reviews")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([]))) + .mount(&server) + .await; + Mock::given(method("PATCH")) + .and(wm_path("/repos/myorg/myrepo/pulls/101")) + .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!([{ + "url": comment_url(&server, 901), + "body": "This pull request is part of a [Mergify stack](https://docs.mergify.com/stacks/):\n| # | Pull Request | Link | |\n", + }]))) + .mount(&server) + .await; + Mock::given(method("DELETE")) + .and(wm_path("/repos/myorg/myrepo/issues/comments/901")) + .respond_with(ResponseTemplate::new(204)) + .expect(1) + .mount(&server) + .await; + + let output = run_push(&local, &server.uri(), &[]); + assert_success(&output); + + let out = String::from_utf8_lossy(&output.stdout) + String::from_utf8_lossy(&output.stderr); + assert!( + out.contains("GitHub stack #7 unchanged"), + "the registration must survive the shrink, or this is not the \ + case under test, got: {out}", + ); + assert!( + out.contains("removed 1 stack comment"), + "the last member's stale table is taken down, got: {out}", + ); + // A POST here would be the opposite failure: writing the table + // back onto a pull request GitHub is already listing. + let comments = comment_requests(&server).await; + assert!( + comments.iter().all(|r| !r.starts_with("POST ")), + "nothing is written back, got: {comments:#?}", + ); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn the_stack_comment_stays_when_native_registration_does_not_happen() { + // The other direction, and the reason this is keyed off the + // outcome rather than the flag: a push that degraded has GitHub + // rendering nothing, so dropping the comment would leave the stack + // with no member list anywhere. + 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; + + assert_success(&run_push(&local, &server.uri(), &[])); + + let comments = comment_requests(&server).await; + assert_eq!( + comments + .iter() + .filter(|r| r.starts_with("POST ") && r.ends_with("/comments")) + .count(), + 2, + "both pull requests keep the table, got: {comments:#?}", + ); + assert!( + comments.iter().all(|r| !r.starts_with("DELETE ")), + "nothing is taken down when nothing replaced it, got: {comments:#?}", + ); +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn by_default_the_stack_is_registered_after_every_pull_request_is_upserted() { let (work, _) = build_stack_repo(2); diff --git a/crates/mergify-stack/src/commands/push.rs b/crates/mergify-stack/src/commands/push.rs index d800d43c..1a017ab3 100644 --- a/crates/mergify-stack/src/commands/push.rs +++ b/crates/mergify-stack/src/commands/push.rs @@ -18,14 +18,16 @@ //! 9. Upsert each PR sequentially via [`crate::pr_upsert`] so //! each `Depends-On: #` header sees the predecessor's //! freshly-created PR number. -//! 10. Upsert stack comments, and render + upsert each prepared -//! revision-history comment, per PR via [`crate::comment_upsert`]. +//! 10. Render + upsert each prepared revision-history comment, per +//! PR via [`crate::comment_upsert`]. //! 11. Tear down orphan branches. //! 12. Unless native registration is disabled (`--no-github-native`, //! or git config `mergify-cli.stack-github-native=false`), bring //! GitHub's native stack in line with what was just pushed, via -//! [`crate::native_stack`], and restore any `Depends-On:` header -//! step 9 left out if it did not take. +//! [`crate::native_stack`]. +//! 13. Settle what step 12 left open: restore any `Depends-On:` header +//! step 9 left out if the registration did not take, and either +//! take the sticky stack comment down or go on writing it. //! //! 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 @@ -44,6 +46,21 @@ //! allowed to quietly not happen — so the headers are held and written //! back when it doesn't, rather than being dropped on the strength of //! that setting. +//! +//! Step 13 settles the sticky stack comment on the same bet, for the +//! same reason: a registered stack is listed by GitHub on the pull +//! request page, so our table of the same members is the redundant +//! copy. What differs is which way the work goes. The header is +//! *written back* when the bet fails, because an unregistered, +//! unchained stack merges out of order. The comment is *taken down* +//! when the bet lands, because nothing refreshes it once we stop +//! writing it, and a table frozen at the shape of the last pre-native +//! push is worse under a live list than no table at all. Its failure +//! policy is the opposite one too — a comment that would not delete +//! changes nothing about how the stack merges, so it is reported and +//! retried on the next push rather than failing a push that has +//! already done everything else. +//! //! The bet is settled inside the push, so the only way to end up with //! neither the registration nor the headers is a push that *fails* //! between steps 9 and 12 — and re-running it settles the stack either @@ -814,45 +831,6 @@ pub async fn run(opts: &Options<'_>) -> Result { entry.change.pull = Some(pull); } - // Stack comments (only when stack has > 1 PR — the upserter - // also guards on this but we pre-filter to avoid a useless - // GET when total_pulls == 1). - let entries: Vec = planned - .locals - .iter() - .filter_map(stack_entry_from_planned) - .collect(); - let total_pulls = entries.len(); - if total_pulls > 1 { - let cidx = prog.add("queued"); - prog.run(cidx, "updating stack comments", async { - for p in &planned.locals { - let Some(pull) = p.change.pull.as_ref() else { - continue; - }; - if pull.get("merged_at").is_some_and(|v| !v.is_null()) { - continue; - } - let Some(number) = pull.get("number").and_then(Value::as_u64) else { - continue; - }; - comment_upsert::update_stack_comment_for_pull( - opts.client, - opts.user, - opts.repo, - number, - &entries, - &dest_branch, - total_pulls, - ) - .await?; - } - Ok::<(), CliError>(()) - }) - .await?; - prog.resolve(cidx, Mark::Done, Some("stack comments updated")); - } - // Revision-history comments — rendered from the histories // prepared (and written as git notes) before the push. if !revision_histories.is_empty() { @@ -1024,6 +1002,127 @@ pub async fn run(opts: &Options<'_>) -> Result { ); } + // The stack comment, settled on the same bet as the `Depends-On:` + // headers above and for the same reason: a registered stack has + // GitHub listing its own members on the pull request page, so our + // sticky table is the redundant copy and this push does not write + // it. What it does instead is take down the ones earlier pushes + // left, because nothing refreshes them any more — a table frozen + // at the stack's shape as of the last pre-native push, sitting + // under the live list, is worse than no table. A push that did not + // register has nothing else describing the stack's shape, so it + // keeps the comment exactly as before. + // + // Only the fallback upsert is gated on the stack having more than + // one pull request. That gate is about cost — a one-change stack + // never got a comment, so looking for one is a GET per push that + // can only come back empty — and it is sound there because + // `update_stack_comment_for_pull` is the thing that would have + // written the comment in the first place. + // + // Applying it to the removal would take the PR's own guarantee + // away in the one case that needs it. A registered stack whose + // members merge down to a single open pull request keeps its + // registration (`appendable_tail` reports nothing to add, so the + // stack stands), so it arrives here registered with one live + // member — carrying the comment it was given while it still had + // several, now frozen under GitHub's live list. That is exactly + // the stale table this step exists to take down. It costs nothing + // in the steady state: GitHub rejects a stack below + // `native_stack::MIN_STACK_SIZE`, so a genuinely one-change stack + // is never registered and never reaches this branch at all. + // + // The case still not covered is a stack that shrank *and* lost its + // registration (GitHub dissolves a 2-member stack that drops to + // one). It leaves the same stale comment, and finding it would + // mean a GET on every single-change push for ever — the cost the + // gate above exists to avoid — to clean an artifact only stacks + // pushed before native registration shipped can have. Left as it + // was on `main`, where this gap already existed. + let entries: Vec = planned + .locals + .iter() + .filter_map(stack_entry_from_planned) + .collect(); + let total_pulls = entries.len(); + if stack_is_registered || total_pulls > 1 { + // Merged pull requests are skipped, as the upsert always + // skipped them: a landed pull request is a closed record of a + // review, not part of the shape this push is describing, and + // that holds for taking a comment down as much as for + // rewriting one. + let live: Vec = planned + .locals + .iter() + .filter_map(|p| { + let pull = p.change.pull.as_ref()?; + if pull.get("merged_at").is_some_and(|v| !v.is_null()) { + return None; + } + pull.get("number").and_then(Value::as_u64) + }) + .collect(); + + if stack_is_registered { + let mut removed = 0usize; + let mut failure: Option = None; + for number in &live { + match comment_upsert::remove_stack_comment_for_pull( + opts.client, + opts.user, + opts.repo, + *number, + ) + .await + { + Ok(true) => removed += 1, + Ok(false) => {} + // Unlike the `Depends-On:` restore above, this + // changes nothing about how the stack merges — it + // clears a surface GitHub now draws itself. Worth + // one more attempt on the next push; never worth + // failing a push that has already done everything. + Err(e) => failure = failure.or(Some(e)), + } + } + // Only ever reported when there was something to report: + // in the steady state every stack comment is long gone and + // a row saying so on every push is a tombstone. + if removed > 0 { + let plural = if removed == 1 { "" } else { "s" }; + prog.add_resolved( + Mark::Noop, + format!("removed {removed} stack comment{plural}"), + ); + } + if let Some(e) = failure { + deferred_notes.push(format!( + "Could not remove the stack comment GitHub's stack UI replaces; \ + the next push tries again. ({e})" + )); + } + } else { + let cidx = prog.add("queued"); + prog.run(cidx, "updating stack comments", async { + for number in &live { + comment_upsert::update_stack_comment_for_pull( + opts.client, + opts.user, + opts.repo, + *number, + &entries, + &dest_branch, + total_pulls, + ) + .await?; + } + Ok::<(), CliError>(()) + }) + .await?; + prog.resolve(cidx, Mark::Done, Some("stack comments updated")); + } + } + // 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 { diff --git a/crates/mergify-stack/src/comment_upsert.rs b/crates/mergify-stack/src/comment_upsert.rs index 46a26b8c..3fb23b9f 100644 --- a/crates/mergify-stack/src/comment_upsert.rs +++ b/crates/mergify-stack/src/comment_upsert.rs @@ -1,22 +1,30 @@ -//! Per-PR upserters for the two sticky comments `stack push` +//! Per-PR writers for the two sticky comments `stack push` //! maintains: //! //! - [`update_stack_comment_for_pull`] — the "this PR is part //! of a stack" table (see [`crate::stack_comment`]). Skipped //! when the stack has only one PR — a single-row table would -//! be noise. +//! be noise. Only reached by a push whose stack GitHub did not +//! register: a registered stack has GitHub's own UI listing +//! its members, and gets [`remove_stack_comment_for_pull`] +//! instead. +//! - [`remove_stack_comment_for_pull`] — the same comment's +//! teardown, for the stacks GitHub now describes itself. //! - [`upsert_revision_history_comment`] — the "Revision //! history" table (see [`crate::revision_history`]). The //! revision-history body is pre-rendered by the push //! orchestrator from the git-notes history (see //! [`crate::revision_note`]); this module only diffs the //! rendered body against the existing comment and writes it. +//! GitHub renders nothing like it, so nothing here replaces +//! it. //! -//! Both walk the issue comments once, match on the header +//! All three walk the issue comments once and match on the header //! (`StackComment::is_stack_comment` / `RevisionHistoryComment:: -//! is_revision_comment`), then choose between PATCH (existing -//! found, body changed), no-op (existing found, body unchanged), -//! and POST (no existing). +//! is_revision_comment`). The upserters then choose between PATCH +//! (existing found, body changed), no-op (existing found, body +//! unchanged), and POST (no existing); the remover issues a DELETE +//! for what it finds. //! //! Ported from //! `mergify_cli/stack/push.py::{_update_comment_for_pull, @@ -64,6 +72,10 @@ fn comment_path(absolute_url: &str) -> Result { /// Upsert the stack-comment for one PR. /// +/// Callers must have established that GitHub is not describing +/// this stack itself — see [`remove_stack_comment_for_pull`] for +/// why, and [`crate::commands::push`] for where that is decided. +/// /// `total_pulls` is the count of live PRs in the stack (i.e. /// the number of `entries`). When it's 1, skip *creation* — /// a single-row "stack" table is noise on a non-stacked PR. @@ -109,6 +121,41 @@ pub async fn update_stack_comment_for_pull( Ok(()) } +/// Take the stack comment down from one PR, if it is still there. +/// +/// GitHub's native Stacks UI lists a registered stack's members on +/// the pull request page itself, so our sticky table is a second +/// copy of what the reader is already looking at and +/// [`crate::commands::push`] stops writing it. Leaving the ones +/// earlier pushes posted is not an option: nothing refreshes them +/// any more, so each would go on showing the stack's shape as of +/// the last pre-native push — a frozen table disagreeing with the +/// live list beside it, which is worse than no table at all. +/// +/// Returns whether a comment was found and deleted. +pub async fn remove_stack_comment_for_pull( + client: &HttpClient, + user: &str, + repo: &str, + pull_number: u64, +) -> Result { + let path = format!("/repos/{user}/{repo}/issues/{pull_number}/comments"); + let comments: Vec = client.get(&path).await?; + + for comment in &comments { + if stack_comment::is_stack_comment(&comment.body) { + // 404 between the GET and the DELETE is somebody else + // having removed it — the end state we wanted. + client + .delete_if_exists(&comment_path(&comment.url)?) + .await?; + return Ok(true); + } + } + + Ok(false) +} + /// Upsert the revision-history comment with a pre-rendered body. /// /// The body is rendered by the caller from the git-notes history @@ -279,6 +326,126 @@ mod tests { .unwrap(); } + #[tokio::test] + async fn remove_stack_comment_deletes_ours_and_leaves_the_rest() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(wm_path("/repos/o/r/issues/1/comments")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!([ + { + "url": format!("{}/repos/o/r/issues/comments/99", server.uri()), + "body": "LGTM", + }, + { + "url": format!("{}/repos/o/r/issues/comments/100", server.uri()), + "body": "This pull request is part of a [Mergify stack](https://docs.mergify.com/stacks/):\nTABLE", + }, + { + "url": format!("{}/repos/o/r/issues/comments/101", server.uri()), + "body": "### Revision history\n\nKEEP", + }, + ]))) + .expect(1) + .mount(&server) + .await; + // Only comment 100 has a DELETE stubbed: a DELETE aimed at + // the human comment or at the revision history would 404 + // against the mock and fail the call. + Mock::given(method("DELETE")) + .and(wm_path("/repos/o/r/issues/comments/100")) + .respond_with(ResponseTemplate::new(204)) + .expect(1) + .mount(&server) + .await; + + let removed = remove_stack_comment_for_pull(&client(&server), "o", "r", 1) + .await + .unwrap(); + assert!(removed); + } + + #[tokio::test] + async fn remove_stack_comment_recognises_the_legacy_header() { + // Comments posted before the docs link went into the header + // are still ours to take down — the whole point of the + // removal is the backlog of pre-native pushes. + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(wm_path("/repos/o/r/issues/1/comments")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!([ + { + "url": format!("{}/repos/o/r/issues/comments/100", server.uri()), + "body": "This pull request is part of a stack:\nTABLE", + }, + ]))) + .mount(&server) + .await; + Mock::given(method("DELETE")) + .and(wm_path("/repos/o/r/issues/comments/100")) + .respond_with(ResponseTemplate::new(204)) + .expect(1) + .mount(&server) + .await; + + assert!( + remove_stack_comment_for_pull(&client(&server), "o", "r", 1) + .await + .unwrap() + ); + } + + #[tokio::test] + async fn remove_stack_comment_is_a_noop_when_there_is_none() { + // The steady state once this has shipped: nothing to delete, + // so no write request at all. No DELETE mock — one would 404. + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(wm_path("/repos/o/r/issues/1/comments")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!([ + { + "url": format!("{}/repos/o/r/issues/comments/101", server.uri()), + "body": "### Revision history\n\nKEEP", + }, + ]))) + .expect(1) + .mount(&server) + .await; + + let removed = remove_stack_comment_for_pull(&client(&server), "o", "r", 1) + .await + .unwrap(); + assert!(!removed); + } + + #[tokio::test] + async fn remove_stack_comment_treats_a_vanished_comment_as_removed() { + // Someone deleted it between our GET and our DELETE. That is + // the end state we were after, not a failure. + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(wm_path("/repos/o/r/issues/1/comments")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!([ + { + "url": format!("{}/repos/o/r/issues/comments/100", server.uri()), + "body": "This pull request is part of a [Mergify stack](https://docs.mergify.com/stacks/):\nTABLE", + }, + ]))) + .mount(&server) + .await; + Mock::given(method("DELETE")) + .and(wm_path("/repos/o/r/issues/comments/100")) + .respond_with(ResponseTemplate::new(404)) + .expect(1) + .mount(&server) + .await; + + assert!( + remove_stack_comment_for_pull(&client(&server), "o", "r", 1) + .await + .unwrap() + ); + } + #[tokio::test] async fn revision_comment_posts_when_missing() { let server = MockServer::start().await; diff --git a/crates/mergify-stack/src/lib.rs b/crates/mergify-stack/src/lib.rs index a00f8b8a..6e3eac0c 100644 --- a/crates/mergify-stack/src/lib.rs +++ b/crates/mergify-stack/src/lib.rs @@ -24,7 +24,9 @@ //! - [`stack_comment`] — the "this PR is part of a stack" //! sticky comment renderer + header recogniser. Pure //! markdown/JSON formatting ported from -//! `mergify_cli/stack/push.py::StackComment`. +//! `mergify_cli/stack/push.py::StackComment`. Only reached +//! for a stack GitHub did not register natively — GitHub's +//! own Stacks UI is the member list everywhere else. //! - [`replay`] — full port of `mergify_cli/stack/replay.py`: //! `git merge-tree` + `git diff-tree` to materialise the //! amendment, then `POST /git/trees` + `POST /git/commits` diff --git a/crates/mergify-stack/src/stack_comment.rs b/crates/mergify-stack/src/stack_comment.rs index ee840f05..071b27c4 100644 --- a/crates/mergify-stack/src/stack_comment.rs +++ b/crates/mergify-stack/src/stack_comment.rs @@ -1,5 +1,13 @@ -//! The "this PR is part of a stack" sticky comment Mergify posts -//! on every PR in a stack. +//! The "this PR is part of a stack" sticky comment, posted on +//! every PR of a stack GitHub is **not** describing itself. +//! +//! Since GitHub's native Stacks UI renders the member list on the +//! pull request page, a registered stack gets no comment from us — +//! [`crate::commands::push`] posts this only when the registration +//! did not happen (`--no-github-native`, a GitHub that does not +//! have the feature, a chain GitHub would not accept), and takes +//! down any comment it finds when it did. So this module renders +//! the fallback surface, not the default one. //! //! Body has three parts: //! @@ -9,13 +17,22 @@ //! rendered carrying a 👈 emoji. //! 3. A single-line HTML comment with the marker prefix //! `` carrying the same data -//! as JSON. The marker is what lets `mergify stack checkout` -//! rebuild the stack from any PR without re-walking GitHub. +//! as JSON. +//! +//! Nothing in this CLI reads that marker (checked 2026-09-21). +//! An earlier version of this docstring claimed `mergify stack +//! checkout` rebuilt a stack from it; it does not, and there is no +//! sign it ever did — [`crate::commands::checkout`] discovers a +//! stack by chaining each PR's `head.ref` to the next one's +//! `base.ref`, which has the advantage of working on stacks this +//! CLI never touched. What the marker is for is readers outside +//! this repo, which is why its wire shape is still pinned below. //! //! Ported from `mergify_cli/stack/push.py::StackComment`. Wire //! shape — header strings, JSON payload, compact one-line marker //! — is contract: existing comments on every Mergify-managed PR -//! need to parse, and `stack checkout` reads the marker. +//! need to parse, and the header is how the upserter and the +//! remover recognise one of ours. use std::fmt::Write; @@ -113,8 +130,8 @@ struct MarkerPayload<'a> { fn json_marker(entries: &[StackEntry], current_number: u64, stack_id: &str) -> String { // `is_current` is a JSON boolean — Python emits the result of // `int(...) == current_number` directly, which `json.dumps` - // serialises as `true`/`false`. Stack-comment readers (incl. - // `stack checkout`) expect a bool; an integer would break + // serialises as `true`/`false`. Out-of-tree readers (the + // browser extension) expect a bool; an integer would break // historic-comment parsing. let pulls = entries .iter() @@ -221,7 +238,7 @@ mod tests { assert_eq!(parsed["pulls"][1]["number"], 2); assert_eq!(parsed["pulls"][1]["is_current"], false); // change_id / head_sha / base_branch / dest_branch carry - // through verbatim — `stack checkout` reads them. + // through verbatim — out-of-tree readers index on them. assert_eq!( parsed["pulls"][0]["change_id"], "Iaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", diff --git a/skills/mergify-stack/SKILL.md b/skills/mergify-stack/SKILL.md index 39964c67..4c3d1d9d 100644 --- a/skills/mergify-stack/SKILL.md +++ b/skills/mergify-stack/SKILL.md @@ -99,10 +99,10 @@ default, so GitHub renders it as a stack. Opt out per invocation with `--no-github-native`, or per repo with `git config mergify-cli.stack-github-native false`. -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. +Change-Ids, branch layout 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. Three things to know: @@ -116,6 +116,13 @@ Three things to know: written back in the same push and Mergify keeps ordering the stack. Your commit messages are never touched either way — the header only ever existed in the rendered PR description. +- **The stack comment goes away too.** GitHub lists a registered stack's + members on the pull request page, so the CLI stops posting the sticky + "This pull request is part of a Mergify stack" table, and deletes the one an + older push left on each open member. Keyed off the registration the same way + the header is: a push that degrades keeps posting and updating the table, + because nothing else would be showing you the stack. The **revision history** + comment is untouched either way — GitHub renders nothing like it. - **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