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, ); 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