From 5183302a4723f46354d268708d73a80b2405d2ee Mon Sep 17 00:00:00 2001 From: Mehdi ABAAKOUK Date: Mon, 5 Oct 2026 14:33:41 +0200 Subject: [PATCH 1/3] fix(ci): pin non-UTF-8 scope paths to the name GitHub reports `mergify ci scopes` decodes `git diff` output lossily, so a Latin-1 `critical/caf\xe9.txt` reaches the scope globs as `critical/caf\u{FFFD}.txt`. The ticket read that as corruption and leaned toward hard-erroring like `mergify_stack`'s `run_git_capture`. It is not corruption. The engine scopes a pull request from GitHub's files API, and that API reports the same path with the same U+FFFD replacement (checked on a sandbox repository holding a Latin-1 and a Shift-JIS name). The lossy decode is therefore the name the engine matches. An error would fail CI on a pull request the engine scopes fine, and no pattern in the UTF-8 YAML config can spell the raw bytes. Keep the decode, say why in the doc comment that used to describe it as a known defect, and pin both probed names in a test so a later switch to strict decoding fails loudly. The test helpers now also drop GIT_INDEX_FILE and friends, since the new one writes the index directly. What still differs on such paths, and on any non-ASCII path, is glob semantics: globset's `?` and `[...]` match bytes, the engine's match characters, so `caf?.txt` misses `caf\u{FFFD}.txt` here and hits it there. That is MRGFY-10066; matching.rs no longer claims exact parity. Fixes MRGFY-8289 Co-Authored-By: Claude Opus 5.5 Change-Id: I37852b3d50345f42b6dfe8cca6e1b36c8858a3fb --- _typos.toml | 3 + .../src/scopes_detect/changed_files.rs | 130 +++++++++++++++--- .../mergify-ci/src/scopes_detect/matching.rs | 5 +- 3 files changed, 116 insertions(+), 22 deletions(-) diff --git a/_typos.toml b/_typos.toml index ee848258..1b1224be 100644 --- a/_typos.toml +++ b/_typos.toml @@ -13,3 +13,6 @@ nd = "nd" authentification = "authentification" # Hex-like test Change-Id fixture token ("Idead0000…ff"). idead = "idead" +# Latin-1 "café" fixture with the é as a raw byte ("caf\xe9.txt") or +# its replacement char ("caf\u{FFFD}.txt"). +caf = "caf" diff --git a/crates/mergify-ci/src/scopes_detect/changed_files.rs b/crates/mergify-ci/src/scopes_detect/changed_files.rs index 74890690..884824cd 100644 --- a/crates/mergify-ci/src/scopes_detect/changed_files.rs +++ b/crates/mergify-ci/src/scopes_detect/changed_files.rs @@ -1,13 +1,12 @@ //! `git diff` between two refs, with progressive deepening of a //! shallow clone if no merge base exists yet. //! -//! Mirrors `mergify_cli/ci/scopes/changed_files.py`. The history -//! deepening is necessary in CI: GitHub Actions checkouts default -//! to depth=1, and the merge base between `base` and `head` -//! probably lives further back. We fetch in batches of 100 -//! commits until either a merge base appears or the commit count -//! stops growing (meaning we've reached the root and there's -//! genuinely no common ancestor). +//! The history deepening is necessary in CI: GitHub Actions +//! checkouts default to depth=1, and the merge base between +//! `base` and `head` probably lives further back. We fetch in +//! batches of 100 commits until either a merge base appears or +//! the commit count stops growing (meaning we've reached the +//! root and there's genuinely no common ancestor). use std::path::Path; use std::process::Command; @@ -90,6 +89,10 @@ fn run_git(repo_dir: Option<&Path>, args: &[&str]) -> Result { // Untrimmed: `git_changed_files` reads NUL-delimited paths, // and a leading or trailing space is a legal filename // character. Callers that parse a scalar trim it themselves. + // + // Lossy on purpose: see `git_changed_files` for why a path + // that is not UTF-8 must come back with U+FFFD in it rather + // than as raw bytes or an error. Ok(String::from_utf8_lossy(&out.stdout).into_owned()) } @@ -171,10 +174,22 @@ pub fn ensure_history( /// /// A rename yields **both** of its paths, so a scope the file moved /// out of still counts as touched. Paths are not -/// `core.quotePath`-escaped, so the globs see the real name; bytes -/// that aren't valid UTF-8 are still replaced (see `run_git`), and +/// `core.quotePath`-escaped, so the globs see the real name, and /// the result is unescaped, so callers that print a path must /// escape it first. +/// +/// A path that is not valid UTF-8 (a Latin-1 or Shift-JIS name) +/// comes back with each invalid sequence replaced by U+FFFD: +/// `caf\xe9.txt` reads `caf\u{FFFD}.txt`. That is the name the +/// engine matches, not a corruption of it. The engine derives +/// scopes from GitHub's pull request files API, which reports that +/// exact replacement (checked on a sandbox repository holding +/// `caf\xe9.txt` and a Shift-JIS name, MRGFY-8289). An error would +/// fail CI on a pull request the engine scopes without complaint, +/// and no pattern in the UTF-8 YAML config can spell the raw bytes. +/// The name agrees with the engine's; how `?` and `[...]` match +/// its U+FFFD does not yet (globset matches bytes, the engine +/// characters, MRGFY-10066). pub fn git_changed_files( repo_dir: Option<&Path>, base: &str, @@ -190,7 +205,7 @@ pub fn git_changed_files( // two must agree or CLI-uploaded and engine-derived scopes // disagree for the same pull request. // - // `--diff-filter=ACMRTD` matches Python: Added, Modified, + // `--diff-filter=ACMRTD` keeps Added, Modified, // Type-changed, Deleted, plus the Renamed and Copied letters // that `--no-renames` has just made unreachable. They stay so // that removing `--no-renames` degrades to the old @@ -239,24 +254,39 @@ pub fn git_changed_files( #[cfg(test)] mod tests { use super::*; + use std::io::Write as _; + use std::process::Stdio; /// Run `git` in `dir` with the developer's global/system /// config out of the way, so a personal `diff.renames` or /// `core.quotePath` setting can't change what these tests see. /// - /// `GIT_DIR`/`GIT_WORK_TREE` are dropped too: they override - /// `-C`, and this helper is the one that runs `add`/`commit`. - /// Under `git bisect run cargo test` (or a hook, or `git - /// rebase -x`) those are set, and the fixture's `git add -A` - /// would otherwise stage into the developer's real repository. - fn git(dir: &Path, args: &[&str]) { - let ok = Command::new("git") - .arg("-C") + /// `GIT_DIR`, `GIT_WORK_TREE` and the other repository-locating + /// variables are dropped too: they override `-C`, and these + /// helpers run `add`/`commit`. Under `git bisect run cargo + /// test` (or a hook, or `git rebase -x`) they are set, and the + /// fixture's `git add -A` would otherwise stage into the + /// developer's real repository. + fn fixture_git(dir: &Path) -> Command { + let mut cmd = Command::new("git"); + cmd.arg("-C") .arg(dir) .env("GIT_CONFIG_GLOBAL", "/dev/null") - .env("GIT_CONFIG_NOSYSTEM", "1") - .env_remove("GIT_DIR") - .env_remove("GIT_WORK_TREE") + .env("GIT_CONFIG_NOSYSTEM", "1"); + for var in [ + "GIT_DIR", + "GIT_WORK_TREE", + "GIT_INDEX_FILE", + "GIT_OBJECT_DIRECTORY", + "GIT_COMMON_DIR", + ] { + cmd.env_remove(var); + } + cmd + } + + fn git(dir: &Path, args: &[&str]) { + let ok = fixture_git(dir) .args(args) .status() .expect("spawn git") @@ -264,6 +294,42 @@ mod tests { assert!(ok, "git {args:?} failed"); } + /// Stage `content` at a path given as raw bytes, which need + /// not be UTF-8. Goes through the index because macOS refuses + /// such a name on disk. + fn stage_raw_path(dir: &Path, path: &[u8], content: &str) { + let mut hash = fixture_git(dir) + .args(["hash-object", "-w", "--stdin"]) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .spawn() + .expect("spawn git hash-object"); + hash.stdin + .take() + .expect("stdin") + .write_all(content.as_bytes()) + .expect("write blob"); + let out = hash.wait_with_output().expect("wait git hash-object"); + assert!(out.status.success(), "git hash-object failed"); + let blob = String::from_utf8(out.stdout).expect("hex object id"); + + let mut entry = format!("100644 {} 0\t", blob.trim()).into_bytes(); + entry.extend_from_slice(path); + entry.push(b'\n'); + let mut index = fixture_git(dir) + .args(["update-index", "--index-info"]) + .stdin(Stdio::piped()) + .spawn() + .expect("spawn git update-index"); + index + .stdin + .take() + .expect("stdin") + .write_all(&entry) + .expect("write index entry"); + assert!(index.wait().expect("wait git update-index").success()); + } + fn write(dir: &Path, rel: &str, content: &str) { let path = dir.join(rel); std::fs::create_dir_all(path.parent().expect("path has a parent")).expect("mkdir"); @@ -367,6 +433,28 @@ mod tests { assert_eq!(changed(dir), ["critical/quote\"and\\slash.txt"]); } + #[test] + fn non_utf8_path_reads_as_github_reports_it() { + // MRGFY-8289: GitHub's pull request files API, which the + // engine scopes from, reports a Latin-1 `caf\xe9.txt` as + // `caf\u{FFFD}.txt`, and the Shift-JIS `テスト.txt` with one + // U+FFFD per lead byte. An error here would fail CI on a + // pull request the engine scopes fine. + let tmp = fixture(); + let dir = tmp.path(); + stage_raw_path(dir, b"critical/caf\xe9.txt", "latin-1\n"); + stage_raw_path(dir, b"critical/\x83e\x83X\x83g.txt", "shift-jis\n"); + git(dir, &["commit", "-q", "-m", "add non-UTF-8 names"]); + + assert_eq!( + changed(dir), + [ + "critical/caf\u{FFFD}.txt", + "critical/\u{FFFD}e\u{FFFD}X\u{FFFD}g.txt", + ], + ); + } + #[test] fn add_modify_and_delete_are_unaffected() { // Every ordinary status still reaches the caller. This diff --git a/crates/mergify-ci/src/scopes_detect/matching.rs b/crates/mergify-ci/src/scopes_detect/matching.rs index e7f210e7..92113607 100644 --- a/crates/mergify-ci/src/scopes_detect/matching.rs +++ b/crates/mergify-ci/src/scopes_detect/matching.rs @@ -18,7 +18,10 @@ //! compiles it inline, the engine expands it into one pattern per //! branch (MRGFY-8359). //! -//! Parity is exact for every path git can report as changed. The +//! Parity is exact for every ASCII path git can report as changed. +//! On a non-ASCII one, `?` and `[...]` differ: globset matches +//! bytes and the engine characters, so `caf?.txt` misses +//! `café.txt` here and hits it there (MRGFY-10066). The other //! residual differences all need a path git never emits: a leading //! `/`, a trailing `/`, an empty segment (`a//b`), or the empty //! string. An unterminated `[` is the one place globset is stricter From 2511f5c46977b29190d3b7c68a91ab7f933dab9e Mon Sep 17 00:00:00 2001 From: Mehdi ABAAKOUK Date: Mon, 5 Oct 2026 14:45:48 +0200 Subject: [PATCH 2/3] fix(ci): make `ci scopes --base origin/main` work on a shallow clone `mergify ci scopes --base origin/main --head HEAD` is the invocation the mergify-ci skill documents. Under `actions/checkout`'s default `fetch-depth: 1` it failed the step with a git error instead of emitting scopes, for two reasons: - The fetch asked the remote for `origin/main`, a name only a clone knows: "fatal: couldn't find remote ref origin/main". A leading `origin/` (or `remotes/origin/`, `refs/remotes/origin/`) is now stripped from the fetch source; the local ref name is unchanged. `feature/x` is left alone, only the remote we fetch from counts as a prefix. - With that fixed, `--deepen` only deepened `main`. HEAD is never fetched by name, so its depth-1 boundary kept hiding its parents and the loop ended with "cannot find a common ancestor". The `--deepen` rounds now also ask for every commit listed in the clone's `shallow` file, so each boundary moves, HEAD's included. The boundary commits rather than HEAD's SHA: they all came from the remote, while HEAD can be a commit made on top of the checkout, which the remote refuses and which would fail the whole fetch. They join only the `--deepen` rounds, never the first `--depth` one, which would cut a history the checkout already has. Pull request events with a head SHA were not affected: both SHAs are fetched. The other auto-detected references that use `HEAD` as head (the `HEAD^` fallback outside GitHub Actions, a push without `before`) get the boundary deepening too. Fixes MRGFY-8290 Co-Authored-By: Claude Opus 5.5 Change-Id: If4ba206ad2a6f84fe06a47ba62c1e9149b95c104 --- .../src/scopes_detect/changed_files.rs | 183 +++++++++++++++++- 1 file changed, 180 insertions(+), 3 deletions(-) diff --git a/crates/mergify-ci/src/scopes_detect/changed_files.rs b/crates/mergify-ci/src/scopes_detect/changed_files.rs index 74890690..00c80f4e 100644 --- a/crates/mergify-ci/src/scopes_detect/changed_files.rs +++ b/crates/mergify-ci/src/scopes_detect/changed_files.rs @@ -21,6 +21,9 @@ const FETCHED_REF_PREFIX: &str = "refs/mergify-cli/fetched/"; const COMMITS_BATCH_SIZE: u64 = 100; +/// The remote `ensure_history` deepens from. +const REMOTE: &str = "origin"; + fn is_sha(ref_: &str) -> bool { // Only full 40-char SHAs — abbreviated SHAs would false-match // branch names like "deadbeef" and cause `git fetch` to treat @@ -52,10 +55,25 @@ fn fetch_arg(ref_: &str) -> Option { // `git fetch origin ` only updates `FETCH_HEAD`; // use an explicit refspec so the branch becomes a real // local ref we can name later. - Some(format!("+{ref_}:{}", local_ref(ref_))) + Some(format!("+{}:{}", remote_name(ref_), local_ref(ref_))) } } +/// The name `REMOTE` knows `ref_` by. A remote-tracking name like +/// `origin/main` exists only in a clone, so asking the remote for it +/// fails with "couldn't find remote ref". That is what a depth-1 +/// CI checkout runs into, as it has no `refs/remotes/origin/main` +/// to resolve locally either. Only the `origin/` prefix is +/// stripped: `feature/x` is a branch, not a remote. +fn remote_name(ref_: &str) -> &str { + ref_.strip_prefix("refs/remotes/") + .or_else(|| ref_.strip_prefix("remotes/")) + .unwrap_or(ref_) + .strip_prefix(REMOTE) + .and_then(|rest| rest.strip_prefix('/')) + .unwrap_or(ref_) +} + /// Base `git` command, rooted at `repo_dir` via `-C` when one is /// given. Production always passes `None` (the process cwd); the /// parameter is what lets the tests below drive a fixture @@ -100,6 +118,31 @@ fn has_merge_base(repo_dir: Option<&Path>, base: &str, head: &str) -> bool { .is_ok_and(|o| o.status.success()) } +/// The commits a shallow clone's history stops at, empty for a full +/// clone. Read from the `shallow` file rather than asked of +/// `rev-parse --is-shallow-repository`, which git before 2.15 +/// echoes back instead of answering. +fn shallow_boundary(repo_dir: Option<&Path>) -> Result, CliError> { + let path = run_git(repo_dir, &["rev-parse", "--git-path", "shallow"])?; + let path = Path::new(path.trim()); + let path = repo_dir.map_or_else(|| path.to_path_buf(), |dir| dir.join(path)); + match std::fs::read_to_string(&path) { + Ok(content) => Ok(content.lines().map(str::to_string).collect()), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(Vec::new()), + Err(e) => Err(CliError::wrap( + format!("could not read {}", path.display()), + e, + )), + } +} + +fn is_ancestor(repo_dir: Option<&Path>, commit: &str, of: &str) -> bool { + git_cmd(repo_dir) + .args(["merge-base", "--is-ancestor", commit, of]) + .output() + .is_ok_and(|o| o.status.success()) +} + fn commits_count(repo_dir: Option<&Path>) -> Result { let out = run_git(repo_dir, &["rev-list", "--count", "--all"])?; let count = out.trim(); @@ -109,7 +152,7 @@ fn commits_count(repo_dir: Option<&Path>) -> Result { } fn fetch(repo_dir: Option<&Path>, depth_flag: &str, fetch_args: &[String]) -> Result<(), CliError> { - let mut args: Vec<&str> = vec!["fetch", "--no-tags", depth_flag, "origin"]; + let mut args: Vec<&str> = vec!["fetch", "--no-tags", depth_flag, REMOTE]; if !fetch_args.is_empty() { args.push("--"); for fa in fetch_args { @@ -146,7 +189,21 @@ pub fn ensure_history( let mut last_count = commits_count(repo_dir)?; while !has_merge_base(repo_dir, &local_base, &local_head) { depth = depth.saturating_mul(2); - fetch(repo_dir, &format!("--deepen={depth}"), &fetch_args)?; + // `--deepen` only moves the boundary of what the fetch asks + // for. A HEAD-relative ref asks for nothing, so a depth-1 + // checkout's HEAD would keep hiding its parents and no merge + // base would ever appear. Asking for every boundary commit + // deepens it too. Only the ones under `base` or `head`: the + // `shallow` file also holds the boundary of anything + // shallow-fetched from another remote, which `origin` may + // not have and would refuse the whole fetch over. Those left + // came from the remote, unlike a local commit on top of the + // checkout, which it would refuse too. + let mut deepen_args = fetch_args.clone(); + deepen_args.extend(shallow_boundary(repo_dir)?.into_iter().filter(|commit| { + is_ancestor(repo_dir, commit, &local_base) || is_ancestor(repo_dir, commit, &local_head) + })); + fetch(repo_dir, &format!("--deepen={depth}"), &deepen_args)?; let count = commits_count(repo_dir)?; if count == last_count { // No new commits this round — we've reached the root @@ -428,5 +485,125 @@ mod tests { fetch_arg("main"), Some(format!("+main:{FETCHED_REF_PREFIX}main")), ); + // A remote-tracking name is asked of the remote by its + // branch name, and still lands under its own local name. A + // slash alone is not a remote prefix. + for (ref_, remote) in [ + ("origin/main", "main"), + ("origin/feature/x", "feature/x"), + ("remotes/origin/main", "main"), + ("refs/remotes/origin/main", "main"), + ("feature/x", "feature/x"), + ("originals/x", "originals/x"), + ] { + assert_eq!( + fetch_arg(ref_), + Some(format!("+{remote}:{FETCHED_REF_PREFIX}{ref_}")), + "{ref_}", + ); + } + } + + /// An upstream where `feature` touched `docs/readme.md`, `main` + /// moved on after the fork (so the merge base is the tip of + /// neither), and `pull/merge` merges the two, the commit + /// `actions/checkout` checks out for a pull request. + fn upstream() -> tempfile::TempDir { + let tmp = fixture(); + let up = tmp.path(); + write(up, "docs/readme.md", "docs changed\n"); + git(up, &["add", "-A"]); + git(up, &["commit", "-q", "-m", "touch the docs"]); + git(up, &["checkout", "-q", "main"]); + write(up, "ignored/later.txt", "later\n"); + git(up, &["add", "-A"]); + git(up, &["commit", "-q", "-m", "main moves on"]); + git(up, &["checkout", "-q", "-b", "pull/merge"]); + git(up, &["merge", "-q", "--no-edit", "feature"]); + tmp + } + + /// `branch` of `up`, fetched at `depth` and checked out detached + /// the way `actions/checkout` does it: no `refs/remotes/origin/*`. + fn shallow_clone(up: &Path, branch: &str, depth: u32) -> tempfile::TempDir { + let tmp = tempfile::tempdir().expect("tempdir"); + let dir = tmp.path(); + // `file://`, because git ignores `--depth` for a plain + // local path. + let url = format!("file://{}", up.display()); + for args in [ + &["init", "-q"][..], + &["config", "user.email", "t@e.com"], + &["config", "user.name", "T"], + &["remote", "add", REMOTE, &url], + &[ + "fetch", + "-q", + "--no-tags", + &format!("--depth={depth}"), + REMOTE, + branch, + ], + &["checkout", "-q", "--detach", "FETCH_HEAD"], + ] { + git(dir, args); + } + tmp + } + + #[test] + fn remote_tracking_base_on_a_shallow_clone() { + // MRGFY-8290: on a depth-1 checkout, `--base origin/main` + // failed twice over: the remote has no ref by that name + // ("couldn't find remote ref origin/main"), and once it was + // asked for `main` instead, deepening `main` never lifted + // the shallow boundary sitting on HEAD, so the merge base + // stayed hidden behind it. + let up = upstream(); + let clone = shallow_clone(up.path(), "pull/merge", 1); + + let paths = git_changed_files(Some(clone.path()), "origin/main", "HEAD").expect("git diff"); + assert_eq!(paths, ["docs/readme.md"]); + } + + #[test] + fn local_commit_on_a_shallow_clone() { + // A commit made in CI on top of the checkout is unknown to + // the remote, which refuses the whole fetch if it is asked + // for. Deepening goes through the boundary commit below it. + let up = upstream(); + let clone = shallow_clone(up.path(), "feature", 1); + let dir = clone.path(); + write(dir, "ignored/generated.txt", "generated\n"); + git(dir, &["add", "-A"]); + git(dir, &["commit", "-q", "-m", "local only"]); + + let mut paths = git_changed_files(Some(dir), "origin/main", "HEAD").expect("git diff"); + paths.sort(); + assert_eq!(paths, ["docs/readme.md", "ignored/generated.txt"]); + } + + #[test] + fn unrelated_shallow_remote_on_a_shallow_clone() { + // The `shallow` file is per repository, not per remote. A + // boundary that came from another remote is unknown to + // `origin`, which would refuse the whole deepening fetch if + // it were asked for it. + let up = upstream(); + let clone = shallow_clone(up.path(), "pull/merge", 1); + let dir = clone.path(); + let other = fixture(); + write(other.path(), "ignored/other.txt", "other\n"); + git(other.path(), &["add", "-A"]); + git(other.path(), &["commit", "-q", "-m", "only on other"]); + let url = format!("file://{}", other.path().display()); + git(dir, &["remote", "add", "other", &url]); + git( + dir, + &["fetch", "-q", "--no-tags", "--depth=1", "other", "feature"], + ); + + let paths = git_changed_files(Some(dir), "origin/main", "HEAD").expect("git diff"); + assert_eq!(paths, ["docs/readme.md"]); } } From 36e518e45391507a56913804865e7a153f83aeea Mon Sep 17 00:00:00 2001 From: Mehdi ABAAKOUK Date: Mon, 5 Oct 2026 16:08:05 +0200 Subject: [PATCH 3/3] fix(ci): stop trusting a merge queue title from a fork `mergify ci git-refs` and `ci scopes` read a pull request body as the engine's merge queue metadata on the title prefix `merge queue: ` alone. The title is the author's to pick, so a fork's pull request could name its own head as `checking_base_sha`: the diff came out empty, every scope read false, and `source: merge_queue` force-enabled the merge-queue scope, with CI green. `pull_request_target` runs that path too. The engine always opens its drafts from a branch of the base repository, so merge queue metadata is now only read when the pull request's head and base live in the same repository (compared by id). That gates the git note as well as the body: a workflow that checks a fork out as `origin` fetches the fork's note, which the fork's author writes. A fork, a deleted fork's head, or a payload without repository ids falls through to the pull request base, with a warning when the title asked for the body. What this does not close: anyone who can read the repository can open a pull request from a branch someone else pushed there and write its title and body. The code under test is then still a writer's. The other signals the ticket listed do not hold up: the branch prefix is `queue_branch_prefix` and the author is `draft_bot_account`, both configurable. Dropping the body path for the git note only was rejected too: the engine writes the note best effort and swallows push failures, and a clone that cannot fetch notes would lose the merge-queue scope on every draft. Fixes MRGFY-8854 Co-Authored-By: Claude Opus 5.5 Change-Id: I19f70a5d13a2672ad13b1a4d66f54d132c3ad32e --- crates/mergify-ci/src/git_refs.rs | 93 +++++++++++++++++++------ crates/mergify-ci/src/github_event.rs | 51 ++++++++++++++ crates/mergify-ci/src/queue_metadata.rs | 75 +++++++++++++++----- 3 files changed, 178 insertions(+), 41 deletions(-) diff --git a/crates/mergify-ci/src/git_refs.rs b/crates/mergify-ci/src/git_refs.rs index 091e178c..82388468 100644 --- a/crates/mergify-ci/src/git_refs.rs +++ b/crates/mergify-ci/src/git_refs.rs @@ -83,8 +83,8 @@ pub struct References { pub source: ReferencesSource, /// The scopes the merge queue decided for this batch, read off /// the engine's git note: see [`batch_scopes`]. `None` everywhere - /// else, the pull request body included, which anyone opening a - /// pull request titled `merge queue: ` writes. + /// else, the pull request body included, which whoever opens the + /// pull request writes. pub batch_scopes: Option, } @@ -240,7 +240,10 @@ fn detect_from_pull_request_event( .and_then(|pr| pr.head.as_ref()) .map_or_else(|| "HEAD".to_string(), |r| r.sha.clone()); + // A fork can serve the note too, when the workflow checks it out + // as `origin`, so the note needs the same admission as the body. if let Some(pr) = &event.pull_request + && pr.is_from_base_repository() && let Some(head_ref) = &pr.head && let Some(branch) = head_ref.r#ref.as_deref() && let Some(note) = notes_reader(branch, &head_ref.sha) @@ -255,8 +258,9 @@ fn detect_from_pull_request_event( } if let Some(meta) = extract_from_event(event, output)? { - // Anyone who can open a pull request writes this body, so the - // value only becomes a base once it looks like one. Either way + // Anyone who can open a pull request from a branch of the + // repository writes this body, so the value only becomes a base + // once it looks like one. Either way // a payload we can't use is not metadata: fall through to the // PR base below, which is the *wrong* base for a genuine batch // build, so warn rather than let it be diagnosed silently. @@ -361,8 +365,9 @@ pub fn real_notes_reader(branch: &str, head_sha: &str) -> Option` in the body and git-refs // emitted it as `base`, where the workflow's next git call // read it as an option. A value that is not an object name is - // not metadata: fall through to the PR base and say so. + // not metadata: fall through to the PR base and say so. Forks + // are refused since MRGFY-8854, but a branch of the repository + // still writes the body. let dir = tempfile::tempdir().unwrap(); let path = write_event( &dir, @@ -806,8 +851,8 @@ mod tests { "pull_request": { "title": "merge queue: batch", "body": "```yaml\nchecking_base_sha: --output=/home/runner/.gitconfig\n```", - "head": {"sha": "mq-head", "ref": "mq/main/0"}, - "base": {"sha": "pr-base"}, + "head": {"sha": "mq-head", "ref": "mq/main/0", "repo": {"id": 1}}, + "base": {"sha": "pr-base", "repo": {"id": 1}}, }, }), ); @@ -842,8 +887,8 @@ mod tests { "pull_request": { "title": "merge queue: batch", "body": "```yaml\nchecking_base_sha: \"cafef00d\\n::error::pwned\"\n```", - "head": {"sha": "mq-head", "ref": "mq/main/0"}, - "base": {"sha": "pr-base"}, + "head": {"sha": "mq-head", "ref": "mq/main/0", "repo": {"id": 1}}, + "base": {"sha": "pr-base", "repo": {"id": 1}}, }, }), ); @@ -877,7 +922,8 @@ mod tests { "pull_request": { "title": "merge queue: batch", "body": format!("```yaml\nchecking_base_sha: {MQ_BASE}\n```"), - "head": {"sha": "mq-head", "ref": "mq/main/0"}, + "head": {"sha": "mq-head", "ref": "mq/main/0", "repo": {"id": 1}}, + "base": {"sha": "pr-base", "repo": {"id": 1}}, }, }), ); @@ -943,8 +989,8 @@ mod tests { #[test] fn mq_note_carries_batch_scopes_but_body_does_not() { - // The body is written by whoever opens a pull request titled - // `merge queue: `, so only the engine's note decides scopes. + // The body is written by whoever opens the pull request, so + // only the engine's note decides scopes. let dir = tempfile::tempdir().unwrap(); let path = write_event( &dir, @@ -952,7 +998,8 @@ mod tests { "pull_request": { "title": "merge queue: batch", "body": format!("```yaml\nchecking_base_sha: {MQ_BASE}\nscopes: [body]\n```"), - "head": {"sha": "mq-head", "ref": "mq/main/0"}, + "head": {"sha": "mq-head", "ref": "mq/main/0", "repo": {"id": 1}}, + "base": {"sha": "pr-base", "repo": {"id": 1}}, }, }), ); diff --git a/crates/mergify-ci/src/github_event.rs b/crates/mergify-ci/src/github_event.rs index 4495e9fc..bca24fb7 100644 --- a/crates/mergify-ci/src/github_event.rs +++ b/crates/mergify-ci/src/github_event.rs @@ -15,6 +15,17 @@ pub struct GitRef { pub sha: String, #[serde(default)] pub r#ref: Option, + /// `null` on a pull request's `head` once its fork is deleted. + #[serde(default)] + pub repo: Option, +} + +#[derive(Debug, Clone, Default, Deserialize)] +pub struct RefRepository { + /// Optional so that a `repo` without it costs only the trust in + /// [`PullRequest::is_from_base_repository`], not the whole event. + #[serde(default)] + pub id: Option, } #[derive(Debug, Clone, Default, Deserialize)] @@ -31,6 +42,31 @@ pub struct PullRequest { pub head: Option, } +impl PullRequest { + /// Whether the head branch lives in the base repository, which is + /// where the engine always opens its merge queue drafts. + /// + /// This is what lets the CLI trust merge queue metadata, never the + /// `merge queue: ` title, which the author picks (MRGFY-8854). It + /// rules out a fork, whose author writes the code, the body, and, + /// when the workflow checks the fork out as `origin`, the git note + /// too. It does not rule out a reader opening a pull request from a + /// branch someone else pushed: they write its title and body, but + /// not its code. The branch prefix and the author are no better: + /// `queue_branch_prefix` and `draft_bot_account` are configurable. + /// + /// `false` when either side has no repository id, including the + /// `null` head of a deleted fork. + #[must_use] + pub fn is_from_base_repository(&self) -> bool { + let repo_id = |r: &Option| r.as_ref()?.repo.as_ref()?.id; + matches!( + (repo_id(&self.head), repo_id(&self.base)), + (Some(head), Some(base)) if head == base + ) + } +} + #[derive(Debug, Clone, Default, Deserialize)] pub struct Repository { #[serde(default)] @@ -94,6 +130,21 @@ mod tests { assert_eq!(ev.pull_request.unwrap().number, Some(7)); } + #[test] + fn is_from_base_repository_needs_both_ids_and_equal() { + let pr = |raw: &str| serde_json::from_str::(raw).unwrap(); + assert!(pr(r#"{"head": {"sha": "h", "repo": {"id": 1}}, "base": {"sha": "b", "repo": {"id": 1}}}"#).is_from_base_repository()); + for raw in [ + r#"{"head": {"sha": "h", "repo": {"id": 2}}, "base": {"sha": "b", "repo": {"id": 1}}}"#, + r#"{"head": {"sha": "h", "repo": null}, "base": {"sha": "b", "repo": {"id": 1}}}"#, + r#"{"head": {"sha": "h", "repo": {"id": 1}}, "base": {"sha": "b"}}"#, + r#"{"head": {"sha": "h"}, "base": {"sha": "b"}}"#, + r#"{"head": {"sha": "h", "repo": {}}, "base": {"sha": "b", "repo": {}}}"#, + ] { + assert!(!pr(raw).is_from_base_repository(), "admitted {raw}"); + } + } + #[test] fn deserialize_push_event_shape() { let raw = r#"{"before": "a", "after": "b", "repository": {"default_branch": "main"}}"#; diff --git a/crates/mergify-ci/src/queue_metadata.rs b/crates/mergify-ci/src/queue_metadata.rs index dfa9c0b4..1816bc7d 100644 --- a/crates/mergify-ci/src/queue_metadata.rs +++ b/crates/mergify-ci/src/queue_metadata.rs @@ -21,6 +21,8 @@ use mergify_core::Output; use crate::github_event::GitHubEvent; +#[cfg(doc)] +use crate::github_event::PullRequest; /// Parse the first ```yaml``` fenced block out of `body` into a /// generic value, keeping every field. Returns `None` when the body @@ -51,9 +53,10 @@ pub fn parse_yaml_block(body: &str) -> Option { /// Extract MQ metadata from an event payload's pull-request body. /// -/// Emits a warning on `output` (stderr for human mode) when the PR is -/// an MQ draft but the body is missing or lacks the fenced block — -/// matches Python's stderr warnings. +/// The title only nominates the pull request: the body is read when +/// [`PullRequest::is_from_base_repository`] also holds. Emits a warning +/// on `output` (stderr for human mode) when it does not, or when the +/// body is missing or lacks the fenced block. pub fn extract_from_event( ev: &GitHubEvent, output: &mut dyn Output, @@ -67,6 +70,12 @@ pub fn extract_from_event( if !title.starts_with("merge queue: ") { return Ok(None); } + if !pr.is_from_base_repository() { + output.status( + "WARNING: pull request titled like a merge queue draft does not come from the base repository, skipping metadata extraction", + )?; + return Ok(None); + } let Some(body) = pr.body.as_deref() else { output.status("WARNING: MQ pull request without body, skipping metadata extraction")?; return Ok(None); @@ -154,16 +163,31 @@ mod tests { assert!(cap.stderr().is_empty()); } - #[test] - fn extract_warns_on_mq_pr_without_body() { - let ev = GitHubEvent { + /// A `merge queue: ` pull request whose head and base live in the + /// repositories with these ids (`None`: the payload's `repo` is + /// `null`, as on the head of a deleted fork). + fn mq_event(head_repo: Option, base_repo: Option, body: Option<&str>) -> GitHubEvent { + let git_ref = |id: Option| crate::github_event::GitRef { + repo: id.map(|id| crate::github_event::RefRepository { id: Some(id) }), + ..Default::default() + }; + GitHubEvent { pull_request: Some(crate::github_event::PullRequest { - title: Some("merge queue: deploy".into()), - body: None, + title: Some("merge queue: batch".into()), + body: body.map(Into::into), + head: Some(git_ref(head_repo)), + base: Some(git_ref(base_repo)), ..Default::default() }), ..Default::default() - }; + } + } + + const BODY: &str = "blah\n```yaml\nchecking_base_sha: deadbeef\n```"; + + #[test] + fn extract_warns_on_mq_pr_without_body() { + let ev = mq_event(Some(1), Some(1), None); let mut cap = Captured::human(); let result = extract_from_event(&ev, &mut cap.output).unwrap(); assert!(result.is_none()); @@ -173,17 +197,32 @@ mod tests { #[test] fn extract_returns_metadata_for_mq_pr() { - let body = "blah\n```yaml\nchecking_base_sha: deadbeef\n```"; - let ev = GitHubEvent { - pull_request: Some(crate::github_event::PullRequest { - title: Some("merge queue: batch".into()), - body: Some(body.into()), - ..Default::default() - }), - ..Default::default() - }; + let ev = mq_event(Some(1), Some(1), Some(BODY)); let mut cap = Captured::human(); let meta = extract_from_event(&ev, &mut cap.output).unwrap().unwrap(); assert_eq!(meta["checking_base_sha"], "deadbeef"); } + + #[test] + fn extract_ignores_mq_title_outside_the_base_repository() { + // MRGFY-8854: anyone can title a pull request `merge queue: `. + // From a fork, from a deleted fork, or with no repository to + // compare, the body is not the engine's. + for (head, base) in [ + (Some(2), Some(1)), + (None, Some(1)), + (Some(1), None), + (None, None), + ] { + let ev = mq_event(head, base, Some(BODY)); + let mut cap = Captured::human(); + let result = extract_from_event(&ev, &mut cap.output).unwrap(); + assert!(result.is_none(), "{head:?} -> {base:?} admitted"); + let stderr = cap.stderr(); + assert!( + stderr.contains("does not come from the base repository"), + "got: {stderr:?}" + ); + } + } }