From f2d53e1bbb6796659ea286ed296c9ef15b828626 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Mon, 12 Oct 2026 14:55:55 +0000 Subject: [PATCH 01/10] Validate resumed release PR ownership and exact version bump (#671) --- scripts/release.sh | 55 +++++++++++++++++++++++++++---- tests/fakes/gh.rs | 24 +++++++++++--- tests/release.rs | 82 +++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 148 insertions(+), 13 deletions(-) diff --git a/scripts/release.sh b/scripts/release.sh index d21c3d35e..c682c43d0 100755 --- a/scripts/release.sh +++ b/scripts/release.sh @@ -29,7 +29,9 @@ # of the bump PR, it says so and waits on or reports the release workflow run # instead; a v tag anywhere else on origin is refused. A bump PR # closed without merging is refused while release- is on origin; -# once that branch is deleted, the next run starts over. +# once that branch is deleted, the next run starts over. Resumed PRs must +# belong to origin, open heads must match its release branch, and their only +# changes must be the version bump this script makes. # # Prints a progress line on stderr for each step, with the release workflow # run's URL once it waits for that run, and the GitHub Release's URL last. @@ -105,16 +107,31 @@ main() { # $branch, or with no PR, head from $branch on origin if it was pushed. A PR # closed without merging whose branch is gone from origin counts as no PR. find_earlier_run() { - local pr - pr=$(gh pr list --head "$branch" --base main --state all --limit 1 \ - --json state,url,headRefOid,mergeCommit \ - --jq '.[0] // empty | "\(.state) \(.url) \(.headRefOid) \(.mergeCommit.oid // "")"') - read -r pr_state url head merge <<<"$pr" + local pr cross_repository origin_head + pr=$(gh pr list --repo "$(git remote get-url origin)" --head "$branch" --base main --state all --limit 1 \ + --json state,url,headRefOid,mergeCommit,isCrossRepository \ + --jq '.[0] // empty | "\(.state) \(.url) \(.headRefOid) \(.isCrossRepository) \(.mergeCommit.oid // "")"') + read -r pr_state url head cross_repository merge <<<"$pr" + if [ -n "$pr_state" ] && [ "$cross_repository" != false ]; then + refuse "$url has its head in another repository, not origin" + fi + if [ "$pr_state" = OPEN ]; then + origin_head=$(git ls-remote origin "refs/heads/$branch" | cut -f 1) + if [ "$head" != "$origin_head" ] || [ -z "$head" ]; then + refuse "$url head does not match refs/heads/$branch on origin" + fi + fi # A PR closed without merging whose branch was then deleted leaves # nothing to resume: start over. if [ "$pr_state" = CLOSED ] && [ -z "$(git ls-remote origin "refs/heads/$branch")" ]; then pr_state='' url='' head='' merge='' fi + if [ "$pr_state" = OPEN ] || [ "$pr_state" = MERGED ]; then + if ! git fetch --quiet origin "$head"; then + refuse "$url head cannot be fetched from origin" + fi + check_resumed_bump + fi if [ -z "$pr_state" ]; then head=$(git ls-remote origin "refs/heads/$branch" | cut -f 1) if [ -n "$head" ]; then @@ -123,6 +140,30 @@ find_earlier_run() { fi } +# check_resumed_bump +# Replays commit_bump's edit at the PR's merge base and compares the whole +# resulting tree with its head. For a merged PR, use main before that merge: +# current main already contains the bump and would make head its own base. +check_resumed_bump() ( + local base work repo expected + if [ "$pr_state" = MERGED ]; then + base=$(git merge-base "$head" "$merge^1") + else + base=$(git merge-base "$head" origin/main) + fi + work=$(mktemp -d) + repo=$PWD + trap 'git -C "$repo" worktree remove --force "$work" >/dev/null' EXIT + git worktree add --quiet --detach "$work" "$base" + cd "$work" + bump_version "$version" + git add -- Cargo.toml Cargo.lock + expected=$(git write-tree) + if [ "$expected" != "$(git rev-parse "$head^{tree}")" ]; then + refuse "$url is not just the version bump to $version in Cargo.toml and Cargo.lock" + fi +) + # finish_if_tagged # After an earlier run, if $tag is on origin at the merge of the bump PR, # waits for its release workflow run and exits as wait_for_release does, and @@ -202,7 +243,7 @@ wait_and_merge() { exit 1 fi - gh pr merge "$branch" --merge --match-head-commit "$head" >/dev/null + gh pr merge "$url" --merge --match-head-commit "$head" >/dev/null fetch_main merge=$(merge_commit_of "$head") progress "merged $url as ${merge:0:7}" diff --git a/tests/fakes/gh.rs b/tests/fakes/gh.rs index 1b83ed9de..b197448d4 100644 --- a/tests/fakes/gh.rs +++ b/tests/fakes/gh.rs @@ -265,9 +265,11 @@ fn check_repo(state: &Json, flags: &Flags) { /// Fail unless `repo`, if any, is the fake's. `{owner}/{repo}` is gh's /// placeholder for the current directory's repo, which is always the fake's. +/// The bare origin's path also names it, as release scripts select their origin. fn check_repo_is(state: &Json, repo: Option<&str>) { if let Some(repo) = repo && repo != "{owner}/{repo}" + && repo != origin_repo().to_string_lossy() && repo.to_lowercase() != state.at("repo").str().to_lowercase() { die(&format!("fake gh: unknown repo {repo}"), 1); @@ -286,13 +288,18 @@ fn newest_pr_from(state: &Json, head: &str) -> usize { .unwrap_or_else(|| no_pr_for(head)) } -/// Resolve a numeric identity or, for fake agents, a head branch. +/// Resolve a numeric identity, a PR URL or, for fake agents, a head branch. fn selected_pr(state: &Json, name: &str) -> usize { if !name.is_empty() && name.bytes().all(|b| b.is_ascii_digit()) { prs(state) .iter() .rposition(|pr| pr.at("number").as_i64() == name.parse().ok()) .unwrap_or_else(|| no_pr_for(name)) + } else if name.starts_with("https://") { + prs(state) + .iter() + .rposition(|pr| pr.at("url").str() == name) + .unwrap_or_else(|| no_pr_for(name)) } else { newest_pr_from(state, name) } @@ -443,6 +450,8 @@ fn pr_fields(pr: &Json, wanted: &[String]) -> Json { const PR_LINE_OF_NEWEST: &str = r#".[0] // empty | "\(.state) \(.url) \(.headRefOid) \(.mergeCommit.oid // "")""#; +const PR_RELEASE_LINE_OF_NEWEST: &str = r#".[0] // empty | "\(.state) \(.url) \(.headRefOid) \(.isCrossRepository) \(.mergeCommit.oid // "")""#; + const WORKFLOW_RUN_LINES: &str = r#".workflow_runs[] | "\(.status) \(.conclusion) \(.name)""#; const COMMIT_PR_LINES: &str = r#".[] | "\(.merge_commit_sha) \(.number)""#; @@ -455,7 +464,7 @@ const FAILED_JOB_NAMES: &str = r#"[.jobs[] | select(.conclusion != "success" and /// Newest first. `--search head:` matches head branches starting with /// , as GitHub's search does; `--head ` matches that branch -/// exactly. `--jq` is supported only as `PR_LINE_OF_NEWEST`. +/// exactly. `--jq` supports the newest PR line, with or without its repository flag. fn pr_list(state: &Json, flags: &Flags) { let wanted_state = flag(flags, "state").unwrap_or("open").to_uppercase(); let search = flag(flags, "search").unwrap_or(""); @@ -479,7 +488,7 @@ fn pr_list(state: &Json, flags: &Flags) { .collect(); match flag(flags, "jq") { None => println!("{}", Array(listed)), - Some(PR_LINE_OF_NEWEST) => { + Some(jq @ (PR_LINE_OF_NEWEST | PR_RELEASE_LINE_OF_NEWEST)) => { if let Some(pr) = listed.first() { let merge = match pr.at("mergeCommit") { commit if commit.truthy() => { @@ -488,10 +497,15 @@ fn pr_list(state: &Json, flags: &Flags) { _ => String::new(), }; let line = format!( - "{} {} {} {merge}", + "{} {} {}{}{merge}", pr.at("state").python(), pr.at("url").python(), - pr.at("headRefOid").python() + pr.at("headRefOid").python(), + if jq == PR_RELEASE_LINE_OF_NEWEST { + format!(" {} ", pr.at("isCrossRepository")) + } else { + " ".to_owned() + } ); println!("{}", line.trim_end()); } diff --git a/tests/release.rs b/tests/release.rs index 3b09e2866..01e6efc37 100644 --- a/tests/release.rs +++ b/tests/release.rs @@ -1391,7 +1391,7 @@ fn assert_refused(release: &Release, version: &str, reasons: &[&str]) { let output = release.run_script(version); - assert!(!output.status.success(), "{version} was not refused"); + assert_eq!(output.status.code(), Some(1), "{version} was not refused"); let err = stderr(&output); assert!(err.contains("refusing to release"), "{version}: {err}"); for reason in reasons { @@ -1560,6 +1560,84 @@ fn a_rerun_after_the_branch_was_pushed_opens_its_pr_then_merges_and_tags() { assert_eq!(release.origin_tag("v0.2.0"), Some(merge)); } +#[test] +#[cfg(target_os = "linux")] +fn a_resumed_bump_pr_from_another_repository_is_refused() { + for merged in [false, true] { + let release = Release::new(); + let head = release.push_bump("0.2.0"); + release.open_bump_pr("0.2.0"); + if merged { + release.merge_bump_pr("0.2.0", &head); + } + release.gh(&["fake", "pr", "release-0.2.0", "isCrossRepository", "true"]); + release.ci_reports(GREEN); + let url = release.pr("release-0.2.0")["url"] + .as_str() + .unwrap() + .to_owned(); + + assert_refused(&release, "0.2.0", &[&url, "another repository"]); + } +} + +#[test] +#[cfg(target_os = "linux")] +fn a_resumed_open_bump_pr_with_a_different_head_from_origin_is_refused() { + let release = Release::new(); + release.push_bump("0.2.0"); + release.open_bump_pr("0.2.0"); + // A stale PR observation must not authorize merging the current branch. + let stale = release.origin(&["rev-parse", "main"]); + release.gh(&[ + "fake", + "pr", + "release-0.2.0", + "headRefOid", + &json!(stale).to_string(), + ]); + release.ci_reports(GREEN); + let url = release.pr("release-0.2.0")["url"] + .as_str() + .unwrap() + .to_owned(); + + assert_refused(&release, "0.2.0", &[&url, "does not match", "origin"]); +} + +#[test] +#[cfg(target_os = "linux")] +fn a_resumed_bump_pr_with_changes_beyond_the_version_bump_is_refused() { + for merged in [false, true] { + for file in ["README.md", "Cargo.toml", "Cargo.lock"] { + let release = Release::new(); + release.push_bump("0.2.0"); + let contributor = release.contributor(); + git(&contributor, &["checkout", "-q", "release-0.2.0"]); + let path = contributor.join(file); + let text = fs::read_to_string(&path).unwrap(); + fs::write(&path, format!("{text}\n# Unrelated change\n")).unwrap(); + git( + &contributor, + &["commit", "-q", "-a", "-m", "Unrelated change"], + ); + git(&contributor, &["push", "-q", "origin", "release-0.2.0"]); + let head = git(&contributor, &["rev-parse", "HEAD"]); + release.open_bump_pr("0.2.0"); + if merged { + release.merge_bump_pr("0.2.0", &head); + } + release.ci_reports(GREEN); + let url = release.pr("release-0.2.0")["url"] + .as_str() + .unwrap() + .to_owned(); + + assert_refused(&release, "0.2.0", &[&url, "not just the version bump"]); + } + } +} + #[test] #[cfg(target_os = "linux")] fn a_rerun_after_the_pr_was_opened_merges_and_tags_without_a_second_pr_or_bump() { @@ -1567,6 +1645,8 @@ fn a_rerun_after_the_pr_was_opened_merges_and_tags_without_a_second_pr_or_bump() let head = release.push_bump("0.2.0"); release.open_bump_pr("0.2.0"); release.ci_reports(GREEN); + // Branch lookup is ambiguous; only the selected PR's URL may be merged. + release.gh(&["fake", "fails", "pr merge release-0.2.0"]); let output = release.run_script("0.2.0"); From 8e29194caed72405839d5aebec9c036798bc2c1a Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Mon, 12 Oct 2026 10:58:03 -0400 Subject: [PATCH 02/10] Validate resumed release PR ownership and bump-only changes (#671) --- scripts/release.sh | 63 +++++++++++-- tests/fakes/gh.rs | 25 ++++-- tests/release.rs | 215 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 289 insertions(+), 14 deletions(-) diff --git a/scripts/release.sh b/scripts/release.sh index d21c3d35e..caddd5de5 100755 --- a/scripts/release.sh +++ b/scripts/release.sh @@ -30,6 +30,9 @@ # instead; a v tag anywhere else on origin is refused. A bump PR # closed without merging is refused while release- is on origin; # once that branch is deleted, the next run starts over. +# A resumed PR must belong to origin, match its branch tip while open, and +# change only the version bump in Cargo.toml and Cargo.lock from its merge +# base. Any other matching PR is refused before merging or tagging. # # Prints a progress line on stderr for each step, with the release workflow # run's URL once it waits for that run, and the GitHub Release's URL last. @@ -104,25 +107,69 @@ main() { # CLOSED, or empty with no PR), url, head and merge from the newest PR from # $branch, or with no PR, head from $branch on origin if it was pushed. A PR # closed without merging whose branch is gone from origin counts as no PR. +# Validates a resumed PR's ownership, head and version-only tree first. find_earlier_run() { - local pr - pr=$(gh pr list --head "$branch" --base main --state all --limit 1 \ - --json state,url,headRefOid,mergeCommit \ - --jq '.[0] // empty | "\(.state) \(.url) \(.headRefOid) \(.mergeCommit.oid // "")"') - read -r pr_state url head merge <<<"$pr" + local pr cross_repository origin_head + pr=$(gh pr list --repo "$(git remote get-url origin)" --head "$branch" --base main --state all --limit 1 \ + --json state,url,headRefOid,mergeCommit,isCrossRepository \ + --jq '.[0] // empty | "\(.isCrossRepository) \(.state) \(.url) \(.headRefOid) \(.mergeCommit.oid // "")"') + read -r cross_repository pr_state url head merge <<<"$pr" + if [ -n "$pr_state" ] && [ "$cross_repository" != false ]; then + refuse "$url has its head in another repository, not origin" + fi + origin_head=$(git ls-remote origin "refs/heads/$branch" | cut -f 1) + if [ "$pr_state" = OPEN ] && { [ -z "$origin_head" ] || [ "$head" != "$origin_head" ]; }; then + refuse "$url head does not match refs/heads/$branch on origin" + fi # A PR closed without merging whose branch was then deleted leaves # nothing to resume: start over. - if [ "$pr_state" = CLOSED ] && [ -z "$(git ls-remote origin "refs/heads/$branch")" ]; then + if [ "$pr_state" = CLOSED ] && [ -z "$origin_head" ]; then pr_state='' url='' head='' merge='' fi if [ -z "$pr_state" ]; then - head=$(git ls-remote origin "refs/heads/$branch" | cut -f 1) + head=$origin_head if [ -n "$head" ]; then git fetch --quiet origin "refs/heads/$branch" fi + elif [ "$pr_state" = OPEN ] || [ "$pr_state" = MERGED ]; then + validate_bump_pr + fi +} + +# validate_bump_pr +# Refuses unless the selected PR's entire tree is the bump_version result +# from its merge base. A merged PR uses main just before its merge, since +# its head is already an ancestor of main now. +validate_bump_pr() { + local base target expected + if ! git fetch --quiet origin "$head"; then + refuse "$url head could not be fetched from origin" + fi + target=origin/main + if [ "$pr_state" = MERGED ]; then + target=$merge^1 + fi + base=$(git merge-base "$head" "$target") + expected=$(bump_tree "$base") + if [ "$expected" != "$(git rev-parse "$head^{tree}")" ]; then + refuse "$url changes more than the version bump to $version in Cargo.toml and Cargo.lock" fi } +# bump_tree +# The tree commit_bump would make, without committing or touching the caller's +# checkout. The subshell owns the temporary worktree and its cleanup trap. +bump_tree() ( + local repo=$PWD work + work=$(mktemp -d) + trap 'git -C "$repo" worktree remove --force "$work" 2>/dev/null || rm -rf "$work"' EXIT + git worktree add --quiet --detach "$work" "$1" + cd "$work" + bump_version "$version" + git add Cargo.toml Cargo.lock + git write-tree +) + # finish_if_tagged # After an earlier run, if $tag is on origin at the merge of the bump PR, # waits for its release workflow run and exits as wait_for_release does, and @@ -202,7 +249,7 @@ wait_and_merge() { exit 1 fi - gh pr merge "$branch" --merge --match-head-commit "$head" >/dev/null + gh pr merge "$url" --merge --match-head-commit "$head" >/dev/null fetch_main merge=$(merge_commit_of "$head") progress "merged $url as ${merge:0:7}" diff --git a/tests/fakes/gh.rs b/tests/fakes/gh.rs index 1b83ed9de..7ac71f5ee 100644 --- a/tests/fakes/gh.rs +++ b/tests/fakes/gh.rs @@ -101,7 +101,7 @@ //! //! `gh api --method PATCH repos//pulls/ -f body=` sets the //! PR's body. `gh pr view`, `gh pr ready` and `gh pr merge` name the PR by -//! its head branch or its number; numeric merge reads that PR's actual head. +//! its head branch, number or URL; an identity selects that PR's actual head. //! //! `gh issue close` closes the issue, recording its `--comment`; on an issue //! already closed it only warns, as gh does. @@ -265,9 +265,11 @@ fn check_repo(state: &Json, flags: &Flags) { /// Fail unless `repo`, if any, is the fake's. `{owner}/{repo}` is gh's /// placeholder for the current directory's repo, which is always the fake's. +/// The local origin path stands in for that repository's remote URL. fn check_repo_is(state: &Json, repo: Option<&str>) { if let Some(repo) = repo && repo != "{owner}/{repo}" + && Path::new(repo) != origin_repo() && repo.to_lowercase() != state.at("repo").str().to_lowercase() { die(&format!("fake gh: unknown repo {repo}"), 1); @@ -286,9 +288,14 @@ fn newest_pr_from(state: &Json, head: &str) -> usize { .unwrap_or_else(|| no_pr_for(head)) } -/// Resolve a numeric identity or, for fake agents, a head branch. +/// Resolve a number or URL, or, for fake agents, a head branch. fn selected_pr(state: &Json, name: &str) -> usize { - if !name.is_empty() && name.bytes().all(|b| b.is_ascii_digit()) { + if name.starts_with("https://") { + prs(state) + .iter() + .rposition(|pr| pr.at("url").str() == name) + .unwrap_or_else(|| no_pr_for(name)) + } else if !name.is_empty() && name.bytes().all(|b| b.is_ascii_digit()) { prs(state) .iter() .rposition(|pr| pr.at("number").as_i64() == name.parse().ok()) @@ -443,6 +450,8 @@ fn pr_fields(pr: &Json, wanted: &[String]) -> Json { const PR_LINE_OF_NEWEST: &str = r#".[0] // empty | "\(.state) \(.url) \(.headRefOid) \(.mergeCommit.oid // "")""#; +const RELEASE_PR_LINE_OF_NEWEST: &str = r#".[0] // empty | "\(.isCrossRepository) \(.state) \(.url) \(.headRefOid) \(.mergeCommit.oid // "")""#; + const WORKFLOW_RUN_LINES: &str = r#".workflow_runs[] | "\(.status) \(.conclusion) \(.name)""#; const COMMIT_PR_LINES: &str = r#".[] | "\(.merge_commit_sha) \(.number)""#; @@ -455,7 +464,7 @@ const FAILED_JOB_NAMES: &str = r#"[.jobs[] | select(.conclusion != "success" and /// Newest first. `--search head:` matches head branches starting with /// , as GitHub's search does; `--head ` matches that branch -/// exactly. `--jq` is supported only as `PR_LINE_OF_NEWEST`. +/// exactly. `--jq` supports the newest-PR line with or without its ownership. fn pr_list(state: &Json, flags: &Flags) { let wanted_state = flag(flags, "state").unwrap_or("open").to_uppercase(); let search = flag(flags, "search").unwrap_or(""); @@ -479,7 +488,7 @@ fn pr_list(state: &Json, flags: &Flags) { .collect(); match flag(flags, "jq") { None => println!("{}", Array(listed)), - Some(PR_LINE_OF_NEWEST) => { + Some(jq @ (PR_LINE_OF_NEWEST | RELEASE_PR_LINE_OF_NEWEST)) => { if let Some(pr) = listed.first() { let merge = match pr.at("mergeCommit") { commit if commit.truthy() => { @@ -493,7 +502,11 @@ fn pr_list(state: &Json, flags: &Flags) { pr.at("url").python(), pr.at("headRefOid").python() ); - println!("{}", line.trim_end()); + if jq == RELEASE_PR_LINE_OF_NEWEST { + println!("{} {}", pr.at("isCrossRepository"), line.trim_end()); + } else { + println!("{}", line.trim_end()); + } } } jq => unsupported_jq(jq), diff --git a/tests/release.rs b/tests/release.rs index 3b09e2866..d0f647fde 100644 --- a/tests/release.rs +++ b/tests/release.rs @@ -1584,6 +1584,14 @@ fn a_rerun_after_the_pr_was_opened_merges_and_tags_without_a_second_pr_or_bump() err.contains("release: waiting for CI on https://github.com/"), "{err}" ); + let calls: Vec> = + serde_json::from_str(&fs::read_to_string(release.root().join("gh-calls.json")).unwrap()) + .unwrap(); + let merges: Vec<_> = calls + .iter() + .filter(|call| call.starts_with(&["pr".into(), "merge".into()])) + .collect(); + assert_eq!(merges.last().unwrap()[2], pr["url"].as_str().unwrap()); } #[test] @@ -1772,6 +1780,213 @@ fn a_tag_with_no_bump_pr_is_refused() { assert_eq!(release.prs_from("release-0.2.0").len(), 0); } +#[test] +#[cfg(target_os = "linux")] +fn an_open_bump_pr_from_another_repository_is_refused() { + let release = Release::new(); + release.push_bump("0.2.0"); + release.open_bump_pr("0.2.0"); + release.gh(&["fake", "pr", "release-0.2.0", "isCrossRepository", "true"]); + release.ci_reports(GREEN); + let refs_before = release.origin_refs(); + let url = release.pr("release-0.2.0")["url"] + .as_str() + .unwrap() + .to_owned(); + + let output = release.run_script("0.2.0"); + + assert_eq!(output.status.code(), Some(1), "{}", stderr(&output)); + assert!(stderr(&output).contains(&url), "{}", stderr(&output)); + assert_eq!(release.pr("release-0.2.0")["state"], "OPEN"); + assert_eq!(release.origin_refs(), refs_before); +} + +#[test] +#[cfg(target_os = "linux")] +fn an_open_bump_pr_with_a_different_head_from_origin_is_refused() { + let release = Release::new(); + release.push_bump("0.2.0"); + release.open_bump_pr("0.2.0"); + // A stale PR head is still a real, fetchable commit, but not the branch tip. + let stale = release.origin(&["rev-parse", "main"]); + release.gh(&[ + "fake", + "pr", + "release-0.2.0", + "headRefOid", + &json!(stale).to_string(), + ]); + release.ci_reports(GREEN); + fs::remove_file(release.root().join("gh-calls.json")).unwrap(); + let refs_before = release.origin_refs(); + let url = release.pr("release-0.2.0")["url"] + .as_str() + .unwrap() + .to_owned(); + + let output = release.run_script("0.2.0"); + + assert_eq!(output.status.code(), Some(1), "{}", stderr(&output)); + assert!(stderr(&output).contains(&url), "{}", stderr(&output)); + assert!( + stderr(&output).contains("head does not match"), + "{}", + stderr(&output) + ); + assert_eq!(release.pr("release-0.2.0")["state"], "OPEN"); + assert_eq!(release.origin_refs(), refs_before); + let calls: Vec> = + serde_json::from_str(&fs::read_to_string(release.root().join("gh-calls.json")).unwrap()) + .unwrap(); + assert!( + !calls + .iter() + .any(|call| call.starts_with(&["pr".into(), "merge".into()])) + ); +} + +#[test] +#[cfg(target_os = "linux")] +fn a_resumed_bump_pr_with_extra_changes_is_refused() { + let release = Release::new(); + release.push_bump("0.2.0"); + let contributor = release.contributor(); + git(&contributor, &["checkout", "-q", "release-0.2.0"]); + fs::write(contributor.join("README.md"), "extra release change\n").unwrap(); + git(&contributor, &["commit", "-q", "-a", "-m", "Extra change"]); + git(&contributor, &["push", "-q", "origin", "release-0.2.0"]); + release.open_bump_pr("0.2.0"); + release.ci_reports(GREEN); + let refs_before = release.origin_refs(); + let url = release.pr("release-0.2.0")["url"] + .as_str() + .unwrap() + .to_owned(); + + let output = release.run_script("0.2.0"); + + assert_eq!(output.status.code(), Some(1), "{}", stderr(&output)); + assert!(stderr(&output).contains(&url), "{}", stderr(&output)); + assert!( + stderr(&output).contains("version bump"), + "{}", + stderr(&output) + ); + assert_eq!(release.pr("release-0.2.0")["state"], "OPEN"); + assert_eq!(release.origin_refs(), refs_before); +} + +#[test] +#[cfg(target_os = "linux")] +fn a_merged_bump_pr_from_another_repository_is_refused_even_if_tagged() { + for tagged in [false, true] { + let release = Release::new(); + let head = release.push_bump("0.2.0"); + release.open_bump_pr("0.2.0"); + let merge = release.merge_bump_pr("0.2.0", &head); + release.gh(&["fake", "pr", "release-0.2.0", "isCrossRepository", "true"]); + if tagged { + release.origin(&["tag", "v0.2.0", &merge]); + } + let refs_before = release.origin_refs(); + let url = release.pr("release-0.2.0")["url"] + .as_str() + .unwrap() + .to_owned(); + + let output = release.run_script("0.2.0"); + + assert_eq!(output.status.code(), Some(1), "{}", stderr(&output)); + assert!(stderr(&output).contains(&url), "{}", stderr(&output)); + assert!( + stderr(&output).contains("another repository"), + "{}", + stderr(&output) + ); + assert_eq!(release.origin_refs(), refs_before); + } +} + +#[test] +#[cfg(target_os = "linux")] +fn a_resumed_pr_must_contain_exactly_the_requested_version_bump() { + for change in [ + "manifest", + "lockfile", + "added file", + "file mode", + "wrong version", + "merged", + ] { + let release = Release::new(); + release.push_bump("0.2.0"); + let contributor = release.contributor(); + git(&contributor, &["checkout", "-q", "release-0.2.0"]); + match change { + "manifest" => { + let file = contributor.join("Cargo.toml"); + let text = fs::read_to_string(&file).unwrap(); + fs::write(file, format!("{text}\n# An unrelated manifest edit\n")).unwrap(); + } + "lockfile" => { + let file = contributor.join("Cargo.lock"); + let text = fs::read_to_string(&file).unwrap(); + fs::write(file, format!("{text}\n# An unrelated lockfile edit\n")).unwrap(); + } + "file mode" => { + fs::set_permissions( + contributor.join("Cargo.toml"), + fs::Permissions::from_mode(0o755), + ) + .unwrap(); + } + "wrong version" => { + for file in ["Cargo.toml", "Cargo.lock"] { + fs::write(contributor.join(file), with_version(file, "0.3.0")).unwrap(); + } + } + _ => fs::write(contributor.join("extra.txt"), "extra\n").unwrap(), + } + git(&contributor, &["add", "-A"]); + git(&contributor, &["commit", "-q", "-m", "Not just the bump"]); + git(&contributor, &["push", "-q", "origin", "release-0.2.0"]); + release.open_bump_pr("0.2.0"); + if change == "merged" { + let head = release.origin(&["rev-parse", "release-0.2.0"]); + release.merge_bump_pr("0.2.0", &head); + } + release.ci_reports(GREEN); + let refs_before = release.origin_refs(); + let pr_before = release.pr("release-0.2.0"); + + let output = release.run_script("0.2.0"); + + assert_eq!( + output.status.code(), + Some(1), + "{change}: {}", + stderr(&output) + ); + assert!( + stderr(&output).contains(pr_before["url"].as_str().unwrap()), + "{change}: {}", + stderr(&output) + ); + assert!( + stderr(&output).contains("version bump"), + "{change}: {}", + stderr(&output) + ); + assert_eq!( + release.pr("release-0.2.0")["state"], + pr_before["state"], + "{change}" + ); + assert_eq!(release.origin_refs(), refs_before, "{change}"); + } +} + #[test] #[cfg(target_os = "linux")] fn a_bump_pr_closed_without_merging_is_refused() { From 6138decc13b69aa5366c49917f186b378ca7e4ac Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Mon, 12 Oct 2026 11:02:49 -0400 Subject: [PATCH 03/10] Share release merge-call fixture after Standards review --- tests/release.rs | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/tests/release.rs b/tests/release.rs index d0f647fde..c89fae68f 100644 --- a/tests/release.rs +++ b/tests/release.rs @@ -277,6 +277,17 @@ impl Release { .clone() } + /// Calls that attempted to merge a PR, oldest first. + fn merge_calls(&self) -> Vec> { + let calls: Vec> = + serde_json::from_str(&fs::read_to_string(self.root().join("gh-calls.json")).unwrap()) + .unwrap(); + calls + .into_iter() + .filter(|call| call.starts_with(&["pr".into(), "merge".into()])) + .collect() + } + fn command(&self, program: &Path, dir: &Path) -> Command { let path = format!( "{}:{}", @@ -1584,13 +1595,7 @@ fn a_rerun_after_the_pr_was_opened_merges_and_tags_without_a_second_pr_or_bump() err.contains("release: waiting for CI on https://github.com/"), "{err}" ); - let calls: Vec> = - serde_json::from_str(&fs::read_to_string(release.root().join("gh-calls.json")).unwrap()) - .unwrap(); - let merges: Vec<_> = calls - .iter() - .filter(|call| call.starts_with(&["pr".into(), "merge".into()])) - .collect(); + let merges = release.merge_calls(); assert_eq!(merges.last().unwrap()[2], pr["url"].as_str().unwrap()); } @@ -1836,14 +1841,7 @@ fn an_open_bump_pr_with_a_different_head_from_origin_is_refused() { ); assert_eq!(release.pr("release-0.2.0")["state"], "OPEN"); assert_eq!(release.origin_refs(), refs_before); - let calls: Vec> = - serde_json::from_str(&fs::read_to_string(release.root().join("gh-calls.json")).unwrap()) - .unwrap(); - assert!( - !calls - .iter() - .any(|call| call.starts_with(&["pr".into(), "merge".into()])) - ); + assert!(release.merge_calls().is_empty()); } #[test] From 05dc9bffb59bc18807351f05603ffb750cda982e Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Mon, 12 Oct 2026 11:17:24 -0400 Subject: [PATCH 04/10] Consolidate stale-head refusal coverage after final review --- tests/release.rs | 37 +------------------------------------ 1 file changed, 1 insertion(+), 36 deletions(-) diff --git a/tests/release.rs b/tests/release.rs index 54dcff969..3e8a932bb 100644 --- a/tests/release.rs +++ b/tests/release.rs @@ -1592,30 +1592,6 @@ fn a_resumed_bump_pr_from_another_repository_is_refused() { } } -#[test] -#[cfg(target_os = "linux")] -fn a_resumed_open_bump_pr_with_a_different_head_from_origin_is_refused() { - let release = Release::new(); - release.push_bump("0.2.0"); - release.open_bump_pr("0.2.0"); - // A stale PR observation must not authorize merging the current branch. - let stale = release.origin(&["rev-parse", "main"]); - release.gh(&[ - "fake", - "pr", - "release-0.2.0", - "headRefOid", - &json!(stale).to_string(), - ]); - release.ci_reports(GREEN); - let url = release.pr("release-0.2.0")["url"] - .as_str() - .unwrap() - .to_owned(); - - assert_refused(&release, "0.2.0", &[&url, "does not match", "origin"]); -} - #[test] #[cfg(target_os = "linux")] fn a_resumed_bump_pr_with_changes_beyond_the_version_bump_is_refused() { @@ -1904,23 +1880,12 @@ fn an_open_bump_pr_with_a_different_head_from_origin_is_refused() { ]); release.ci_reports(GREEN); fs::remove_file(release.root().join("gh-calls.json")).unwrap(); - let refs_before = release.origin_refs(); let url = release.pr("release-0.2.0")["url"] .as_str() .unwrap() .to_owned(); - let output = release.run_script("0.2.0"); - - assert_eq!(output.status.code(), Some(1), "{}", stderr(&output)); - assert!(stderr(&output).contains(&url), "{}", stderr(&output)); - assert!( - stderr(&output).contains("head does not match"), - "{}", - stderr(&output) - ); - assert_eq!(release.pr("release-0.2.0")["state"], "OPEN"); - assert_eq!(release.origin_refs(), refs_before); + assert_refused(&release, "0.2.0", &[&url, "head does not match", "origin"]); assert!(release.merge_calls().is_empty()); } From 4e64fa1939f04db873b07103cc1157631f1ca305 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Mon, 12 Oct 2026 15:37:11 +0000 Subject: [PATCH 05/10] Consolidate incoming release refusal tests after review --- tests/release.rs | 208 ++++++++--------------------------------------- 1 file changed, 35 insertions(+), 173 deletions(-) diff --git a/tests/release.rs b/tests/release.rs index 3e8a932bb..bbd94e6fb 100644 --- a/tests/release.rs +++ b/tests/release.rs @@ -1574,12 +1574,15 @@ fn a_rerun_after_the_branch_was_pushed_opens_its_pr_then_merges_and_tags() { #[test] #[cfg(target_os = "linux")] fn a_resumed_bump_pr_from_another_repository_is_refused() { - for merged in [false, true] { + for (merged, tagged) in [(false, false), (true, false), (true, true)] { let release = Release::new(); let head = release.push_bump("0.2.0"); release.open_bump_pr("0.2.0"); if merged { - release.merge_bump_pr("0.2.0", &head); + let merge = release.merge_bump_pr("0.2.0", &head); + if tagged { + release.origin(&["tag", "v0.2.0", &merge]); + } } release.gh(&["fake", "pr", "release-0.2.0", "isCrossRepository", "true"]); release.ci_reports(GREEN); @@ -1596,18 +1599,40 @@ fn a_resumed_bump_pr_from_another_repository_is_refused() { #[cfg(target_os = "linux")] fn a_resumed_bump_pr_with_changes_beyond_the_version_bump_is_refused() { for merged in [false, true] { - for file in ["README.md", "Cargo.toml", "Cargo.lock"] { + for change in [ + "README.md", + "Cargo.toml", + "Cargo.lock", + "added file", + "file mode", + "wrong version", + ] { let release = Release::new(); release.push_bump("0.2.0"); let contributor = release.contributor(); git(&contributor, &["checkout", "-q", "release-0.2.0"]); - let path = contributor.join(file); - let text = fs::read_to_string(&path).unwrap(); - fs::write(&path, format!("{text}\n# Unrelated change\n")).unwrap(); - git( - &contributor, - &["commit", "-q", "-a", "-m", "Unrelated change"], - ); + match change { + "added file" => fs::write(contributor.join("extra.txt"), "extra\n").unwrap(), + "file mode" => { + fs::set_permissions( + contributor.join("Cargo.toml"), + fs::Permissions::from_mode(0o755), + ) + .unwrap(); + } + "wrong version" => { + for file in ["Cargo.toml", "Cargo.lock"] { + fs::write(contributor.join(file), with_version(file, "0.3.0")).unwrap(); + } + } + file => { + let path = contributor.join(file); + let text = fs::read_to_string(&path).unwrap(); + fs::write(&path, format!("{text}\n# Unrelated change\n")).unwrap(); + } + } + git(&contributor, &["add", "-A"]); + git(&contributor, &["commit", "-q", "-m", "Unrelated change"]); git(&contributor, &["push", "-q", "origin", "release-0.2.0"]); let head = git(&contributor, &["rev-parse", "HEAD"]); release.open_bump_pr("0.2.0"); @@ -1841,28 +1866,6 @@ fn a_tag_with_no_bump_pr_is_refused() { assert_eq!(release.prs_from("release-0.2.0").len(), 0); } -#[test] -#[cfg(target_os = "linux")] -fn an_open_bump_pr_from_another_repository_is_refused() { - let release = Release::new(); - release.push_bump("0.2.0"); - release.open_bump_pr("0.2.0"); - release.gh(&["fake", "pr", "release-0.2.0", "isCrossRepository", "true"]); - release.ci_reports(GREEN); - let refs_before = release.origin_refs(); - let url = release.pr("release-0.2.0")["url"] - .as_str() - .unwrap() - .to_owned(); - - let output = release.run_script("0.2.0"); - - assert_eq!(output.status.code(), Some(1), "{}", stderr(&output)); - assert!(stderr(&output).contains(&url), "{}", stderr(&output)); - assert_eq!(release.pr("release-0.2.0")["state"], "OPEN"); - assert_eq!(release.origin_refs(), refs_before); -} - #[test] #[cfg(target_os = "linux")] fn an_open_bump_pr_with_a_different_head_from_origin_is_refused() { @@ -1889,147 +1892,6 @@ fn an_open_bump_pr_with_a_different_head_from_origin_is_refused() { assert!(release.merge_calls().is_empty()); } -#[test] -#[cfg(target_os = "linux")] -fn a_resumed_bump_pr_with_extra_changes_is_refused() { - let release = Release::new(); - release.push_bump("0.2.0"); - let contributor = release.contributor(); - git(&contributor, &["checkout", "-q", "release-0.2.0"]); - fs::write(contributor.join("README.md"), "extra release change\n").unwrap(); - git(&contributor, &["commit", "-q", "-a", "-m", "Extra change"]); - git(&contributor, &["push", "-q", "origin", "release-0.2.0"]); - release.open_bump_pr("0.2.0"); - release.ci_reports(GREEN); - let refs_before = release.origin_refs(); - let url = release.pr("release-0.2.0")["url"] - .as_str() - .unwrap() - .to_owned(); - - let output = release.run_script("0.2.0"); - - assert_eq!(output.status.code(), Some(1), "{}", stderr(&output)); - assert!(stderr(&output).contains(&url), "{}", stderr(&output)); - assert!( - stderr(&output).contains("version bump"), - "{}", - stderr(&output) - ); - assert_eq!(release.pr("release-0.2.0")["state"], "OPEN"); - assert_eq!(release.origin_refs(), refs_before); -} - -#[test] -#[cfg(target_os = "linux")] -fn a_merged_bump_pr_from_another_repository_is_refused_even_if_tagged() { - for tagged in [false, true] { - let release = Release::new(); - let head = release.push_bump("0.2.0"); - release.open_bump_pr("0.2.0"); - let merge = release.merge_bump_pr("0.2.0", &head); - release.gh(&["fake", "pr", "release-0.2.0", "isCrossRepository", "true"]); - if tagged { - release.origin(&["tag", "v0.2.0", &merge]); - } - let refs_before = release.origin_refs(); - let url = release.pr("release-0.2.0")["url"] - .as_str() - .unwrap() - .to_owned(); - - let output = release.run_script("0.2.0"); - - assert_eq!(output.status.code(), Some(1), "{}", stderr(&output)); - assert!(stderr(&output).contains(&url), "{}", stderr(&output)); - assert!( - stderr(&output).contains("another repository"), - "{}", - stderr(&output) - ); - assert_eq!(release.origin_refs(), refs_before); - } -} - -#[test] -#[cfg(target_os = "linux")] -fn a_resumed_pr_must_contain_exactly_the_requested_version_bump() { - for change in [ - "manifest", - "lockfile", - "added file", - "file mode", - "wrong version", - "merged", - ] { - let release = Release::new(); - release.push_bump("0.2.0"); - let contributor = release.contributor(); - git(&contributor, &["checkout", "-q", "release-0.2.0"]); - match change { - "manifest" => { - let file = contributor.join("Cargo.toml"); - let text = fs::read_to_string(&file).unwrap(); - fs::write(file, format!("{text}\n# An unrelated manifest edit\n")).unwrap(); - } - "lockfile" => { - let file = contributor.join("Cargo.lock"); - let text = fs::read_to_string(&file).unwrap(); - fs::write(file, format!("{text}\n# An unrelated lockfile edit\n")).unwrap(); - } - "file mode" => { - fs::set_permissions( - contributor.join("Cargo.toml"), - fs::Permissions::from_mode(0o755), - ) - .unwrap(); - } - "wrong version" => { - for file in ["Cargo.toml", "Cargo.lock"] { - fs::write(contributor.join(file), with_version(file, "0.3.0")).unwrap(); - } - } - _ => fs::write(contributor.join("extra.txt"), "extra\n").unwrap(), - } - git(&contributor, &["add", "-A"]); - git(&contributor, &["commit", "-q", "-m", "Not just the bump"]); - git(&contributor, &["push", "-q", "origin", "release-0.2.0"]); - release.open_bump_pr("0.2.0"); - if change == "merged" { - let head = release.origin(&["rev-parse", "release-0.2.0"]); - release.merge_bump_pr("0.2.0", &head); - } - release.ci_reports(GREEN); - let refs_before = release.origin_refs(); - let pr_before = release.pr("release-0.2.0"); - - let output = release.run_script("0.2.0"); - - assert_eq!( - output.status.code(), - Some(1), - "{change}: {}", - stderr(&output) - ); - assert!( - stderr(&output).contains(pr_before["url"].as_str().unwrap()), - "{change}: {}", - stderr(&output) - ); - assert!( - stderr(&output).contains("version bump"), - "{change}: {}", - stderr(&output) - ); - assert_eq!( - release.pr("release-0.2.0")["state"], - pr_before["state"], - "{change}" - ); - assert_eq!(release.origin_refs(), refs_before, "{change}"); - } -} - #[test] #[cfg(target_os = "linux")] fn a_bump_pr_closed_without_merging_is_refused() { From 8dcebeac3e3bcb62794e6a346b852e7a40afe26c Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Mon, 12 Oct 2026 11:51:02 -0400 Subject: [PATCH 06/10] Record review of incoming issue-671 commits Reviewed 05dc9bffb59bc18807351f05603ffb750cda982e...4e64fa1939f04db873b07103cc1157631f1ca305 with independent Standards and Spec reviewers. Both axes found no findings and read every changed file. Reports saved in .thirdshift-review-QCD2vC; PR #679 records the outcome. No code fixes were needed. Validation: cargo test --test release (55 passed), cargo fmt --check, git diff --check, and cargo clippy --all-targets -- -D warnings. From 25dccb498b08ec0c3ec26f2864cb0611a77a681c Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Mon, 12 Oct 2026 15:59:27 +0000 Subject: [PATCH 07/10] Record empty fixed-point review for issue-671 The requested comparison 4e64fa1939f04db873b07103cc1157631f1ca305...8dcebeac3e3bcb62794e6a346b852e7a40afe26c has no changed files. The thirdshift-code-review skill requires stopping before reviewers are spawned for an empty diff. Recorded that result in .thirdshift-review-Fle5qX/standards.md and spec.md; no findings or code changes, and PR #679 body preserved. From 36fe5fe5804cb048621e0b6a0994a6a482888921 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Mon, 12 Oct 2026 12:10:11 -0400 Subject: [PATCH 08/10] thirdshift: failed run (gh api repos/JacobStephens2/thirdshift/commits/8dcebeac3e3bcb62794e6a346b852e7a40afe26c/check-runs?per_page=100 failed: unexpected end of JSON input) 2026-10-12T16:10:11Z, host thirdshift. Uncommitted work at the time of failure is included in this commit. From 00d135a4db21e845ff9b50374075b7741945141a Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 13 Oct 2026 14:26:19 -0400 Subject: [PATCH 09/10] thirdshift: failed run (interrupted) 2026-10-13T18:26:19Z, host Jacob-MacBook-Pro. Uncommitted work at the time of failure is included in this commit. --- scripts/release.sh | 26 +++--- tests/fakes/gh.rs | 18 ++-- tests/release.rs | 228 ++++++++++++++++++++++++++++++++++----------- 3 files changed, 195 insertions(+), 77 deletions(-) diff --git a/scripts/release.sh b/scripts/release.sh index caddd5de5..1d5180994 100755 --- a/scripts/release.sh +++ b/scripts/release.sh @@ -30,9 +30,10 @@ # instead; a v tag anywhere else on origin is refused. A bump PR # closed without merging is refused while release- is on origin; # once that branch is deleted, the next run starts over. -# A resumed PR must belong to origin, match its branch tip while open, and -# change only the version bump in Cargo.toml and Cargo.lock from its merge -# base. Any other matching PR is refused before merging or tagging. +# A resumed PR must belong to origin and match its branch tip while open, or +# it is refused, even once tagged. Unless already tagged, it must also change +# only the version bump in Cargo.toml and Cargo.lock from its merge base, or +# it is refused before merging or tagging. # # Prints a progress line on stderr for each step, with the release workflow # run's URL once it waits for that run, and the GitHub Release's URL last. @@ -86,10 +87,12 @@ main() { wait_and_merge ;; OPEN) + validate_bump_pr progress "resuming: $url is open" wait_and_merge ;; MERGED) + validate_bump_pr progress "resuming: $url was merged as ${merge:0:7}" ;; *) @@ -107,32 +110,31 @@ main() { # CLOSED, or empty with no PR), url, head and merge from the newest PR from # $branch, or with no PR, head from $branch on origin if it was pushed. A PR # closed without merging whose branch is gone from origin counts as no PR. -# Validates a resumed PR's ownership, head and version-only tree first. +# Refuses a PR from another repository, or an open PR whose head is not +# $branch on origin. find_earlier_run() { local pr cross_repository origin_head pr=$(gh pr list --repo "$(git remote get-url origin)" --head "$branch" --base main --state all --limit 1 \ --json state,url,headRefOid,mergeCommit,isCrossRepository \ --jq '.[0] // empty | "\(.isCrossRepository) \(.state) \(.url) \(.headRefOid) \(.mergeCommit.oid // "")"') read -r cross_repository pr_state url head merge <<<"$pr" + origin_head=$(git ls-remote origin "refs/heads/$branch" | cut -f 1) + # A PR closed without merging whose branch was then deleted leaves + # nothing to resume, whichever repository it came from: start over. + if [ "$pr_state" = CLOSED ] && [ -z "$origin_head" ]; then + pr_state='' url='' head='' merge='' + fi if [ -n "$pr_state" ] && [ "$cross_repository" != false ]; then refuse "$url has its head in another repository, not origin" fi - origin_head=$(git ls-remote origin "refs/heads/$branch" | cut -f 1) if [ "$pr_state" = OPEN ] && { [ -z "$origin_head" ] || [ "$head" != "$origin_head" ]; }; then refuse "$url head does not match refs/heads/$branch on origin" fi - # A PR closed without merging whose branch was then deleted leaves - # nothing to resume: start over. - if [ "$pr_state" = CLOSED ] && [ -z "$origin_head" ]; then - pr_state='' url='' head='' merge='' - fi if [ -z "$pr_state" ]; then head=$origin_head if [ -n "$head" ]; then git fetch --quiet origin "refs/heads/$branch" fi - elif [ "$pr_state" = OPEN ] || [ "$pr_state" = MERGED ]; then - validate_bump_pr fi } diff --git a/tests/fakes/gh.rs b/tests/fakes/gh.rs index 7ac71f5ee..4390dbfec 100644 --- a/tests/fakes/gh.rs +++ b/tests/fakes/gh.rs @@ -101,7 +101,7 @@ //! //! `gh api --method PATCH repos//pulls/ -f body=` sets the //! PR's body. `gh pr view`, `gh pr ready` and `gh pr merge` name the PR by -//! its head branch, number or URL; an identity selects that PR's actual head. +//! its head branch, number or URL; a number or URL selects that PR's actual head. //! //! `gh issue close` closes the issue, recording its `--comment`; on an issue //! already closed it only warns, as gh does. @@ -450,7 +450,7 @@ fn pr_fields(pr: &Json, wanted: &[String]) -> Json { const PR_LINE_OF_NEWEST: &str = r#".[0] // empty | "\(.state) \(.url) \(.headRefOid) \(.mergeCommit.oid // "")""#; -const RELEASE_PR_LINE_OF_NEWEST: &str = r#".[0] // empty | "\(.isCrossRepository) \(.state) \(.url) \(.headRefOid) \(.mergeCommit.oid // "")""#; +const PR_LINE_OF_NEWEST_WITH_OWNERSHIP: &str = r#".[0] // empty | "\(.isCrossRepository) \(.state) \(.url) \(.headRefOid) \(.mergeCommit.oid // "")""#; const WORKFLOW_RUN_LINES: &str = r#".workflow_runs[] | "\(.status) \(.conclusion) \(.name)""#; @@ -488,7 +488,7 @@ fn pr_list(state: &Json, flags: &Flags) { .collect(); match flag(flags, "jq") { None => println!("{}", Array(listed)), - Some(jq @ (PR_LINE_OF_NEWEST | RELEASE_PR_LINE_OF_NEWEST)) => { + Some(jq @ (PR_LINE_OF_NEWEST | PR_LINE_OF_NEWEST_WITH_OWNERSHIP)) => { if let Some(pr) = listed.first() { let merge = match pr.at("mergeCommit") { commit if commit.truthy() => { @@ -496,17 +496,17 @@ fn pr_list(state: &Json, flags: &Flags) { } _ => String::new(), }; + let ownership = match jq { + PR_LINE_OF_NEWEST_WITH_OWNERSHIP => format!("{} ", pr.at("isCrossRepository")), + _ => String::new(), + }; let line = format!( - "{} {} {} {merge}", + "{ownership}{} {} {} {merge}", pr.at("state").python(), pr.at("url").python(), pr.at("headRefOid").python() ); - if jq == RELEASE_PR_LINE_OF_NEWEST { - println!("{} {}", pr.at("isCrossRepository"), line.trim_end()); - } else { - println!("{}", line.trim_end()); - } + println!("{}", line.trim_end()); } } jq => unsupported_jq(jq), diff --git a/tests/release.rs b/tests/release.rs index bbd94e6fb..d236f9dcb 100644 --- a/tests/release.rs +++ b/tests/release.rs @@ -279,9 +279,12 @@ impl Release { /// Calls that attempted to merge a PR, oldest first. fn merge_calls(&self) -> Vec> { + let path = self.root().join("gh-calls.json"); + if !path.exists() { + return Vec::new(); + } let calls: Vec> = - serde_json::from_str(&fs::read_to_string(self.root().join("gh-calls.json")).unwrap()) - .unwrap(); + serde_json::from_str(&fs::read_to_string(path).unwrap()).unwrap(); calls .into_iter() .filter(|call| call.starts_with(&["pr".into(), "merge".into()])) @@ -336,6 +339,19 @@ impl Release { ) } + /// Sources the script in the maintainer's clone, with what `run_script` + /// runs it with, then runs the bash `code`, which sets what the earlier + /// steps would have and calls its functions. + fn with_functions(&self, code: &str) -> Output { + self.command(Path::new("bash"), &self.maintainer()) + .arg("-c") + .arg(format!("source \"$1\"\n{code}")) + .arg("bash") + .arg(manifest_dir().join("scripts/release.sh")) + .output() + .expect("could not run bash") + } + /// Runs the Release notes script for `tag` in the maintainer's clone, /// once it has fetched the tags, with `INSTALL_NOTES` as dist's install /// instructions. @@ -419,6 +435,42 @@ impl Release { head } + /// Pushes one more commit to `version`'s pushed bump branch, making + /// `change`: a line added to README.md, Cargo.toml or Cargo.lock, an + /// "added file", Cargo.toml's "file mode", or the "wrong version"; and + /// returns that commit. + fn push_change_to_bump(&self, version: &str, change: &str) -> String { + let contributor = self.contributor(); + let branch = format!("release-{version}"); + git(&contributor, &["checkout", "-q", &branch]); + match change { + "added file" => fs::write(contributor.join("extra.txt"), "extra\n").unwrap(), + "file mode" => { + fs::set_permissions( + contributor.join("Cargo.toml"), + fs::Permissions::from_mode(0o755), + ) + .unwrap(); + } + "wrong version" => { + for file in ["Cargo.toml", "Cargo.lock"] { + fs::write(contributor.join(file), with_version(file, "0.3.0")).unwrap(); + } + } + file => { + let path = contributor.join(file); + let text = fs::read_to_string(&path).unwrap(); + fs::write(&path, format!("{text}\n# Unrelated change\n")).unwrap(); + } + } + git(&contributor, &["add", "-A"]); + git(&contributor, &["commit", "-q", "-m", "Unrelated change"]); + git(&contributor, &["push", "-q", "origin", &branch]); + let head = git(&contributor, &["rev-parse", "HEAD"]); + git(&contributor, &["checkout", "-q", "--detach", "origin/main"]); + head + } + /// Opens the bump PR for `version`'s pushed branch. fn open_bump_pr(&self, version: &str) { self.gh(&[ @@ -538,6 +590,11 @@ fn release_url() -> String { format!("https://github.com/{REPO}/releases/tag/v0.2.0") } +/// Sets what `main` would for 0.2.0 and finds the earlier run, as `main` +/// does before anything else. +const FIND_EARLIER_RUN: &str = + "version=0.2.0 tag=v0.2.0 branch=release-0.2.0\nfetch_main\nfind_earlier_run"; + const GREEN: &str = r#"[{"name": "test", "conclusion": "success", "pending_polls": 2}]"#; const RED: &str = r#"[{"name": "test", "conclusion": "failure"}]"#; const AGENT_SUMMARY: &str = "The headline is the frobnicator (#2)."; @@ -1397,19 +1454,31 @@ fn when_the_bump_pr_cannot_be_read_the_release_body_is_the_generated_notes_with_ /// Runs the script with `version` and asserts it refused before pushing /// anything, with a message on stderr containing each of `reasons`. fn assert_refused(release: &Release, version: &str, reasons: &[&str]) { + assert_refused_by(release, version, reasons, || release.run_script(version)); +} + +/// Runs `run`, the script or some of its functions, and asserts it refused +/// the `case` before pushing anything, with a message on stderr containing +/// each of `reasons`. +fn assert_refused_by( + release: &Release, + case: &str, + reasons: &[&str], + run: impl FnOnce() -> Output, +) { let refs_before = release.origin_refs(); let prs_before = release.gh_state()["prs"].clone(); - let output = release.run_script(version); + let output = run(); - assert_eq!(output.status.code(), Some(1), "{version} was not refused"); + assert_eq!(output.status.code(), Some(1), "{case} was not refused"); let err = stderr(&output); - assert!(err.contains("refusing to release"), "{version}: {err}"); + assert!(err.contains("refusing to release"), "{case}: {err}"); for reason in reasons { - assert!(err.contains(reason), "{version}: no {reason:?} in {err}"); + assert!(err.contains(reason), "{case}: no {reason:?} in {err}"); } - assert_eq!(release.origin_refs(), refs_before, "{version}"); - assert_eq!(release.gh_state()["prs"], prs_before, "{version}"); + assert_eq!(release.origin_refs(), refs_before, "{case}"); + assert_eq!(release.gh_state()["prs"], prs_before, "{case}"); } #[test] @@ -1574,6 +1643,7 @@ fn a_rerun_after_the_branch_was_pushed_opens_its_pr_then_merges_and_tags() { #[test] #[cfg(target_os = "linux")] fn a_resumed_bump_pr_from_another_repository_is_refused() { + // The open PR end to end; finding the earlier run takes the rest. for (merged, tagged) in [(false, false), (true, false), (true, true)] { let release = Release::new(); let head = release.push_bump("0.2.0"); @@ -1590,14 +1660,25 @@ fn a_resumed_bump_pr_from_another_repository_is_refused() { .as_str() .unwrap() .to_owned(); + let reasons = [url.as_str(), "another repository"]; - assert_refused(&release, "0.2.0", &[&url, "another repository"]); + if merged { + assert_refused_by( + &release, + &format!("merged {merged}, tagged {tagged}"), + &reasons, + || release.with_functions(FIND_EARLIER_RUN), + ); + } else { + assert_refused(&release, "0.2.0", &reasons); + } } } #[test] #[cfg(target_os = "linux")] fn a_resumed_bump_pr_with_changes_beyond_the_version_bump_is_refused() { + // One open PR end to end; checking the bump PR takes the rest. for merged in [false, true] { for change in [ "README.md", @@ -1609,32 +1690,7 @@ fn a_resumed_bump_pr_with_changes_beyond_the_version_bump_is_refused() { ] { let release = Release::new(); release.push_bump("0.2.0"); - let contributor = release.contributor(); - git(&contributor, &["checkout", "-q", "release-0.2.0"]); - match change { - "added file" => fs::write(contributor.join("extra.txt"), "extra\n").unwrap(), - "file mode" => { - fs::set_permissions( - contributor.join("Cargo.toml"), - fs::Permissions::from_mode(0o755), - ) - .unwrap(); - } - "wrong version" => { - for file in ["Cargo.toml", "Cargo.lock"] { - fs::write(contributor.join(file), with_version(file, "0.3.0")).unwrap(); - } - } - file => { - let path = contributor.join(file); - let text = fs::read_to_string(&path).unwrap(); - fs::write(&path, format!("{text}\n# Unrelated change\n")).unwrap(); - } - } - git(&contributor, &["add", "-A"]); - git(&contributor, &["commit", "-q", "-m", "Unrelated change"]); - git(&contributor, &["push", "-q", "origin", "release-0.2.0"]); - let head = git(&contributor, &["rev-parse", "HEAD"]); + let head = release.push_change_to_bump("0.2.0", change); release.open_bump_pr("0.2.0"); if merged { release.merge_bump_pr("0.2.0", &head); @@ -1644,12 +1700,62 @@ fn a_resumed_bump_pr_with_changes_beyond_the_version_bump_is_refused() { .as_str() .unwrap() .to_owned(); - - assert_refused(&release, "0.2.0", &[&url, "version bump"]); + let reasons = [url.as_str(), "version bump"]; + + if !merged && change == "README.md" { + assert_refused(&release, "0.2.0", &reasons); + } else { + assert_refused_by( + &release, + &format!("merged {merged}, {change}"), + &reasons, + || release.with_functions(&format!("{FIND_EARLIER_RUN}\nvalidate_bump_pr")), + ); + } } } } +#[test] +#[cfg(target_os = "linux")] +fn a_merged_bump_pr_whose_head_is_not_on_origin_is_refused() { + let release = Release::new(); + let head = release.push_bump("0.2.0"); + release.open_bump_pr("0.2.0"); + release.merge_bump_pr("0.2.0", &head); + let missing = json!("0".repeat(40)).to_string(); + release.gh(&["fake", "pr", "release-0.2.0", "headRefOid", &missing]); + let url = release.pr("release-0.2.0")["url"] + .as_str() + .unwrap() + .to_owned(); + + assert_refused_by( + &release, + "missing head", + &[&url, "could not be fetched"], + || release.with_functions(&format!("{FIND_EARLIER_RUN}\nvalidate_bump_pr")), + ); +} + +#[test] +#[cfg(target_os = "linux")] +fn an_already_tagged_release_is_still_reported_when_its_pr_changed_more() { + // The tag is already out, so refusing would only hide its release run. + let release = Release::new(); + release.push_bump("0.2.0"); + let head = release.push_change_to_bump("0.2.0", "added file"); + release.open_bump_pr("0.2.0"); + let merge = release.merge_bump_pr("0.2.0", &head); + release.origin(&["tag", "v0.2.0", &merge]); + + let output = release.run_script("0.2.0"); + + assert!(output.status.success(), "{}", stderr(&output)); + let err = stderr(&output); + assert!(err.contains("v0.2.0 is already tagged"), "{err}"); +} + #[test] #[cfg(target_os = "linux")] fn a_rerun_after_the_pr_was_opened_merges_and_tags_without_a_second_pr_or_bump() { @@ -1882,14 +1988,14 @@ fn an_open_bump_pr_with_a_different_head_from_origin_is_refused() { &json!(stale).to_string(), ]); release.ci_reports(GREEN); - fs::remove_file(release.root().join("gh-calls.json")).unwrap(); let url = release.pr("release-0.2.0")["url"] .as_str() .unwrap() .to_owned(); + let merges_before = release.merge_calls(); assert_refused(&release, "0.2.0", &[&url, "head does not match", "origin"]); - assert!(release.merge_calls().is_empty()); + assert_eq!(release.merge_calls(), merges_before); } #[test] @@ -1913,25 +2019,35 @@ fn a_bump_pr_closed_without_merging_is_refused() { #[test] #[cfg(target_os = "linux")] fn a_bump_pr_closed_without_merging_and_its_branch_deleted_starts_over() { - let release = Release::new(); - let stale = release.push_bump("0.2.0"); - release.open_bump_pr("0.2.0"); - release.gh(&["fake", "pr", "release-0.2.0", "state", "\"CLOSED\""]); - // GitHub keeps a closed PR's head once its branch is deleted. - let head = format!("\"{stale}\""); - release.gh(&["fake", "pr", "release-0.2.0", "headRefOid", &head]); - release.origin(&["branch", "-D", "release-0.2.0"]); - release.ci_reports(GREEN); + // Even a closed PR from another repository leaves nothing to resume. + for cross_repository in ["false", "true"] { + let release = Release::new(); + let stale = release.push_bump("0.2.0"); + release.open_bump_pr("0.2.0"); + release.gh(&["fake", "pr", "release-0.2.0", "state", "\"CLOSED\""]); + release.gh(&[ + "fake", + "pr", + "release-0.2.0", + "isCrossRepository", + cross_repository, + ]); + // GitHub keeps a closed PR's head once its branch is deleted. + let head = format!("\"{stale}\""); + release.gh(&["fake", "pr", "release-0.2.0", "headRefOid", &head]); + release.origin(&["branch", "-D", "release-0.2.0"]); + release.ci_reports(GREEN); - let output = release.run_script("0.2.0"); + let output = release.run_script("0.2.0"); - assert!(output.status.success(), "{}", stderr(&output)); - let prs = release.prs_from("release-0.2.0"); - assert_eq!(prs.len(), 2); - assert_eq!(prs[0]["state"], "CLOSED"); - assert_eq!(prs[1]["state"], "MERGED"); - let merge = release.origin(&["rev-parse", "main"]); - assert_eq!(release.origin_tag("v0.2.0"), Some(merge)); + assert!(output.status.success(), "{}", stderr(&output)); + let prs = release.prs_from("release-0.2.0"); + assert_eq!(prs.len(), 2); + assert_eq!(prs[0]["state"], "CLOSED"); + assert_eq!(prs[1]["state"], "MERGED"); + let merge = release.origin(&["rev-parse", "main"]); + assert_eq!(release.origin_tag("v0.2.0"), Some(merge)); + } } #[test] From 8b7247f34559f7f6ab56d17802971bb22d21c1f4 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Fri, 16 Oct 2026 22:16:15 -0400 Subject: [PATCH 10/10] Validate all resumed release candidates before completion --- scripts/release.sh | 24 ++++++------ tests/release.rs | 91 +++++++++++++++++++++++++++++----------------- 2 files changed, 70 insertions(+), 45 deletions(-) diff --git a/scripts/release.sh b/scripts/release.sh index 1d5180994..1a7204c84 100755 --- a/scripts/release.sh +++ b/scripts/release.sh @@ -31,9 +31,9 @@ # closed without merging is refused while release- is on origin; # once that branch is deleted, the next run starts over. # A resumed PR must belong to origin and match its branch tip while open, or -# it is refused, even once tagged. Unless already tagged, it must also change -# only the version bump in Cargo.toml and Cargo.lock from its merge base, or -# it is refused before merging or tagging. +# it is refused, even once tagged. Its tree (or a pushed branch with no PR) +# must also be exactly the version bump in Cargo.toml and Cargo.lock from +# its merge base, or it is refused before merging or tagging. # # Prints a progress line on stderr for each step, with the release workflow # run's URL once it waits for that run, and the GitHub Release's URL last. @@ -71,6 +71,9 @@ main() { fetch_main find_earlier_run + case $pr_state in + OPEN | MERGED) validate_bump_pr ;; + esac finish_if_tagged case $pr_state in @@ -81,18 +84,17 @@ main() { git push --quiet origin "$head:refs/heads/$branch" else progress "resuming: $branch is on origin at ${head:0:7} with no PR" + validate_bump_pr write_summary "no PR was opened" fi open_pr wait_and_merge ;; OPEN) - validate_bump_pr progress "resuming: $url is open" wait_and_merge ;; MERGED) - validate_bump_pr progress "resuming: $url was merged as ${merge:0:7}" ;; *) @@ -139,13 +141,13 @@ find_earlier_run() { } # validate_bump_pr -# Refuses unless the selected PR's entire tree is the bump_version result -# from its merge base. A merged PR uses main just before its merge, since -# its head is already an ancestor of main now. +# Refuses unless the selected PR's (or pushed branch's) entire tree is the +# bump_version result from its merge base. A merged PR uses main just before +# its merge, since its head is already an ancestor of main now. validate_bump_pr() { - local base target expected + local base target expected candidate=${url:-$branch on origin} if ! git fetch --quiet origin "$head"; then - refuse "$url head could not be fetched from origin" + refuse "$candidate head could not be fetched from origin" fi target=origin/main if [ "$pr_state" = MERGED ]; then @@ -154,7 +156,7 @@ validate_bump_pr() { base=$(git merge-base "$head" "$target") expected=$(bump_tree "$base") if [ "$expected" != "$(git rev-parse "$head^{tree}")" ]; then - refuse "$url changes more than the version bump to $version in Cargo.toml and Cargo.lock" + refuse "$candidate changes more than the version bump to $version in Cargo.toml and Cargo.lock" fi } diff --git a/tests/release.rs b/tests/release.rs index d236f9dcb..3aee490c7 100644 --- a/tests/release.rs +++ b/tests/release.rs @@ -62,6 +62,14 @@ fn with_version(file: &str, version: &str) -> String { format!("{}{version}{}", &text[..start], &text[end..]) } +#[derive(Clone, Copy, Debug, PartialEq)] +enum BumpChange { + AppendComment(&'static str), + AddedFile, + FileMode, + WrongVersion, +} + /// A bare origin whose main is at version 0.1.0, tagged `v0.1.0`, with one PR /// merged before the tag and two after, and a green CI run on main's tip; a /// contributor's clone that makes them; the maintainer's clone the script @@ -436,28 +444,26 @@ impl Release { } /// Pushes one more commit to `version`'s pushed bump branch, making - /// `change`: a line added to README.md, Cargo.toml or Cargo.lock, an - /// "added file", Cargo.toml's "file mode", or the "wrong version"; and - /// returns that commit. - fn push_change_to_bump(&self, version: &str, change: &str) -> String { + /// `change`, and returns that commit. + fn push_change_to_bump(&self, version: &str, change: BumpChange) -> String { let contributor = self.contributor(); let branch = format!("release-{version}"); git(&contributor, &["checkout", "-q", &branch]); match change { - "added file" => fs::write(contributor.join("extra.txt"), "extra\n").unwrap(), - "file mode" => { + BumpChange::AddedFile => fs::write(contributor.join("extra.txt"), "extra\n").unwrap(), + BumpChange::FileMode => { fs::set_permissions( contributor.join("Cargo.toml"), fs::Permissions::from_mode(0o755), ) .unwrap(); } - "wrong version" => { + BumpChange::WrongVersion => { for file in ["Cargo.toml", "Cargo.lock"] { fs::write(contributor.join(file), with_version(file, "0.3.0")).unwrap(); } } - file => { + BumpChange::AppendComment(file) => { let path = contributor.join(file); let text = fs::read_to_string(&path).unwrap(); fs::write(&path, format!("{text}\n# Unrelated change\n")).unwrap(); @@ -1681,12 +1687,12 @@ fn a_resumed_bump_pr_with_changes_beyond_the_version_bump_is_refused() { // One open PR end to end; checking the bump PR takes the rest. for merged in [false, true] { for change in [ - "README.md", - "Cargo.toml", - "Cargo.lock", - "added file", - "file mode", - "wrong version", + BumpChange::AppendComment("README.md"), + BumpChange::AppendComment("Cargo.toml"), + BumpChange::AppendComment("Cargo.lock"), + BumpChange::AddedFile, + BumpChange::FileMode, + BumpChange::WrongVersion, ] { let release = Release::new(); release.push_bump("0.2.0"); @@ -1702,12 +1708,12 @@ fn a_resumed_bump_pr_with_changes_beyond_the_version_bump_is_refused() { .to_owned(); let reasons = [url.as_str(), "version bump"]; - if !merged && change == "README.md" { + if !merged && change == BumpChange::AppendComment("README.md") { assert_refused(&release, "0.2.0", &reasons); } else { assert_refused_by( &release, - &format!("merged {merged}, {change}"), + &format!("merged {merged}, {change:?}"), &reasons, || release.with_functions(&format!("{FIND_EARLIER_RUN}\nvalidate_bump_pr")), ); @@ -1738,24 +1744,6 @@ fn a_merged_bump_pr_whose_head_is_not_on_origin_is_refused() { ); } -#[test] -#[cfg(target_os = "linux")] -fn an_already_tagged_release_is_still_reported_when_its_pr_changed_more() { - // The tag is already out, so refusing would only hide its release run. - let release = Release::new(); - release.push_bump("0.2.0"); - let head = release.push_change_to_bump("0.2.0", "added file"); - release.open_bump_pr("0.2.0"); - let merge = release.merge_bump_pr("0.2.0", &head); - release.origin(&["tag", "v0.2.0", &merge]); - - let output = release.run_script("0.2.0"); - - assert!(output.status.success(), "{}", stderr(&output)); - let err = stderr(&output); - assert!(err.contains("v0.2.0 is already tagged"), "{err}"); -} - #[test] #[cfg(target_os = "linux")] fn a_rerun_after_the_pr_was_opened_merges_and_tags_without_a_second_pr_or_bump() { @@ -2069,3 +2057,38 @@ fn review_no_on_a_rerun_after_the_branch_was_pushed_stops_with_no_pr_opened() { assert!(!release.pr_opened("release-0.2.0")); assert_eq!(release.origin_refs(), refs_before); } + +#[test] +#[cfg(target_os = "linux")] +fn review_resumed_branch_without_pr_with_extra_changes_is_refused() { + let release = Release::new(); + release.push_bump("0.2.0"); + release.push_change_to_bump("0.2.0", BumpChange::AddedFile); + release.ci_reports(GREEN); + let refs_before = release.origin_refs(); + let merges_before = release.merge_calls(); + + let output = release.run_script("0.2.0"); + + assert_eq!(output.status.code(), Some(1), "{}", stderr(&output)); + assert!(stderr(&output).contains("refusing to release")); + assert_eq!(release.origin_refs(), refs_before); + assert_eq!(release.merge_calls(), merges_before); +} + +#[test] +#[cfg(target_os = "linux")] +fn review_tagged_resumed_pr_with_extra_changes_is_refused() { + let release = Release::new(); + release.push_bump("0.2.0"); + let head = release.push_change_to_bump("0.2.0", BumpChange::AddedFile); + release.open_bump_pr("0.2.0"); + let merge = release.merge_bump_pr("0.2.0", &head); + release.origin(&["tag", "v0.2.0", &merge]); + let url = release.pr("release-0.2.0")["url"] + .as_str() + .unwrap() + .to_owned(); + + assert_refused(&release, "0.2.0", &[&url, "version bump"]); +}