From 74d789a2e8d5ac45b1bafd679ca2da2c2c3a6530 Mon Sep 17 00:00:00 2001 From: Mehdi ABAAKOUK Date: Mon, 31 Aug 2026 12:07:39 +0200 Subject: [PATCH 1/4] fix(ci): skip a test whose name is too large and say which one A test name arrives from JUnit unbounded and goes straight onto the wire: `build_traces` copies it into the span name and two attributes. INC-2436 was a Vitest title interpolating stringified React source at 31,207 characters, and nothing between the runner and the backend refused it. Skip a case whose name exceeds 65,536 bytes and report it through the path already built for cases too large to upload: the human report and, on GitHub Actions, a `::warning::` annotation. The CI outcome is untouched, so a skipped result never breaks a customer's build. A suite left with no cases emits no span rather than an empty suite. Skipped rather than truncated on purpose. The backend identifies a test by `uuid_generate_v5` of its name, so a truncated name is a different test: the result would split its history, flakiness and quarantine state in two. Not uploading it says so plainly, and names it so the owner can rename the test. Names are cut to 120 bytes for display in both surfaces, since the name is itself what made the case too large. Fixes MRGFY-8951 Related to MRGFY-8902 Change-Id: I224cd6b770d1228bc0ae884ec546b05a94102e3d --- .../mergify-ci/src/junit_process/command.rs | 90 ++++++++++-- crates/mergify-ci/src/junit_process/spans.rs | 131 ++++++++++++++++++ 2 files changed, 212 insertions(+), 9 deletions(-) diff --git a/crates/mergify-ci/src/junit_process/command.rs b/crates/mergify-ci/src/junit_process/command.rs index 724ba6ee..d32b19f1 100644 --- a/crates/mergify-ci/src/junit_process/command.rs +++ b/crates/mergify-ci/src/junit_process/command.rs @@ -171,12 +171,12 @@ async fn run_with_cap( .map(|c| c.name.clone()) .collect(), }; - let built = spans::build_traces(&parsed, &metadata); + let mut built = spans::build_traces(&parsed, &metadata); // Cap each gzipped upload at MAX_GZIPPED_UPLOAD_BYTES. A normal // report is one chunk (byte-identical to before); only an // oversized payload fans out into several uploads. - let (chunks, oversized_cases, mut upload_error) = + let (chunks, mut oversized_cases, mut upload_error) = match split::split_request(&built.request, upload_cap) { Ok(outcome) => (outcome.chunks, outcome.oversized_cases, None), // gzip is an in-memory write and effectively never fails, @@ -192,6 +192,16 @@ async fn run_with_cap( ), }; + // Cases the span builder refused on name length join the ones the split + // refused on payload size: from the user's side both are "this result was + // not uploaded", and one list is what they need to act on. + // + // Moved, not cloned. Every string in here is over MAX_TEST_NAME_BYTES -- + // that is why it was refused -- so copying them would allocate another + // 65 kB apiece on the one path guaranteed to be holding the biggest names + // in the run. `built` is not read for this field again. + oversized_cases.append(&mut built.oversized_case_names); + let client = upload::default_client(); // Nothing reached the backend when the split produced no chunks // (every case individually oversized) or gzip failed. Captured @@ -603,10 +613,16 @@ fn gha_oversized_annotation(names: &[String]) -> Option { return None; } Some(format!( - "::warning title=Mergify Test Insights::{n} test result(s) exceeded the upload size \ - limit and were skipped: {names}. The rest of the run was uploaded.", + "::warning title=Mergify Test Insights::{n} test result(s) were too large to upload \ + and were skipped: {names}. The rest of the run was uploaded.", n = names.len(), - names = gha_escape_data(&names.join(", ")), + names = gha_escape_data( + &names + .iter() + .map(|n| display_name(n)) + .collect::>() + .join(", "), + ), )) } @@ -640,14 +656,33 @@ fn write_upload_error_block(out: &mut String, error: &str, rejected: bool) { /// is no upload it can fit into. The CI verdict is unaffected (it's /// computed from the parsed cases, not the upload), so this is a /// best-effort data-loss notice, not a failure. +/// How much of a skipped test's name the report prints. A name can be the +/// reason it was skipped, so printing it whole would bury the report under +/// the same 65 kB that caused the problem. +const SKIPPED_NAME_DISPLAY_BYTES: usize = 120; + +/// `name` cut to [`SKIPPED_NAME_DISPLAY_BYTES`] on a character boundary, +/// marked when it was cut so nobody copies the prefix as the real name. +fn display_name(name: &str) -> String { + if name.len() <= SKIPPED_NAME_DISPLAY_BYTES { + return name.to_string(); + } + let mut end = SKIPPED_NAME_DISPLAY_BYTES; + while end > 0 && !name.is_char_boundary(end) { + end -= 1; + } + format!("{}… ({} bytes total)", &name[..end], name.len()) +} + fn write_oversized_cases(out: &mut String, names: &[String]) { - out.push_str("\n ⚠️ Some test results were too large to upload\n"); - out.push_str(" A single test's output exceeded the upload size limit and was skipped.\n"); + out.push_str("\n ⚠️ Some test results were skipped\n"); + out.push_str(" A test whose name or output is too large to upload is skipped; the rest\n"); + out.push_str(" of the run was uploaded. Rename the test to get its history back.\n"); out.push_str(" Quarantine status and CI outcome are unaffected.\n"); out.push('\n'); out.push_str(" ┌ Skipped\n"); for name in names { - out.push_str(&format!(" │ {name}\n")); + out.push_str(&format!(" │ {}\n", display_name(name))); } out.push_str(" └─\n"); } @@ -1025,7 +1060,44 @@ mod tests { .unwrap(); assert!(ann.starts_with("::warning"), "{ann}"); assert!(ann.contains("a.big, b.huge"), "{ann}"); - assert!(ann.contains("exceeded the upload size limit"), "{ann}"); + assert!(ann.contains("too large to upload"), "{ann}"); + } + + /// A name can itself be the reason a case was skipped, so neither the + /// report nor the annotation may print it whole -- 65 kB of generated + /// title would bury the very message telling you which test to rename. + #[test] + fn a_skipped_name_is_truncated_for_display() { + let long = "z".repeat(super::SKIPPED_NAME_DISPLAY_BYTES + 500); + + let shown = display_name(&long); + assert!(shown.len() < long.len(), "{shown}"); + // Marked as cut, and carrying the real size, so nobody copies the + // prefix believing it is the test's name. + assert!(shown.contains('…'), "{shown}"); + assert!( + shown.contains(&format!("{} bytes total", long.len())), + "{shown}" + ); + + let mut report = String::new(); + write_oversized_cases(&mut report, std::slice::from_ref(&long)); + assert!(!report.contains(&long), "the report printed the whole name"); + + let ann = temp_env::with_var("GITHUB_ACTIONS", Some("true"), || { + gha_oversized_annotation(std::slice::from_ref(&long)) + }) + .unwrap(); + assert!( + !ann.contains(&long), + "the annotation printed the whole name" + ); + } + + /// A short name is printed exactly, with no truncation marker. + #[test] + fn a_short_name_is_printed_verbatim() { + assert_eq!(display_name("tests.test_a"), "tests.test_a"); } // ── End-to-end orchestrator tests. Drive the full `run()` diff --git a/crates/mergify-ci/src/junit_process/spans.rs b/crates/mergify-ci/src/junit_process/spans.rs index d4542182..f8d03ee1 100644 --- a/crates/mergify-ci/src/junit_process/spans.rs +++ b/crates/mergify-ci/src/junit_process/spans.rs @@ -47,6 +47,22 @@ pub struct UploadMetadata { pub quarantined: BTreeSet, } +/// The widest test name this client will upload, in bytes. +/// +/// Not a storage limit. The engine keys every table an oversized name can +/// reach on `uuid_generate_v5` of the name rather than the name itself, so +/// the name is stored whole and no btree tuple limit is tripped (MRGFY-8902). +/// What this bounds is what a wire payload and a UI can usefully carry: a +/// name at this size is 65 kB of generated text — INC-2436 was a Vitest title +/// interpolating stringified React source — that no one reads and that costs +/// every downstream reader who has to render it. +/// +/// Skipped rather than truncated, and reported rather than dropped quietly. +/// Truncating would mint a second identity for the same test, which is the +/// one outcome worse than not having the result: the test would split into +/// two histories and its flakiness and quarantine state with it. +pub(crate) const MAX_TEST_NAME_BYTES: usize = 65_536; + /// Result of converting a [`ParseResult`] (one or more `JUnit` /// files) into a wire-ready OTLP request. #[derive(Debug, Clone)] @@ -55,6 +71,10 @@ pub struct BuiltTraces { /// Same value populates the `test.run.id` resource attribute. pub run_id: String, pub request: ExportTraceServiceRequest, + /// Names of cases left out because they exceed + /// [`MAX_TEST_NAME_BYTES`]. Reported to the user by the caller; + /// empty for every ordinary report. + pub oversized_case_names: Vec, } /// Convert a [`ParseResult`] (the union of every parsed `JUnit` @@ -87,6 +107,7 @@ fn build_traces_with( let common_attrs = common_attributes(metadata); let mut spans: Vec = Vec::new(); + let mut oversized_case_names: Vec = Vec::new(); // Suite spans are appended after we know each suite's earliest // case start (so the suite's start_time covers all its cases). @@ -101,7 +122,17 @@ fn build_traces_with( let suite_span_id = rng.bytes8(); let mut suite_start_time_unix_nanos = now_unix_nanos; + let mut suite_case_count = 0_usize; + for case in suite_cases { + // `len()` on a Rust `String` is already bytes, which is the unit + // the limit is stated in. + if case.name.len() > MAX_TEST_NAME_BYTES { + oversized_case_names.push(case.name.clone()); + continue; + } + suite_case_count += 1; + let case_span_id = rng.bytes8(); let start_time_unix_nanos = case_start_time(now_unix_nanos, case.duration); suite_start_time_unix_nanos = suite_start_time_unix_nanos.min(start_time_unix_nanos); @@ -164,6 +195,13 @@ fn build_traces_with( }); } + // A suite that kept nothing emits nothing: a suite span with no + // children is a test suite the backend would show as having run zero + // cases, which is a different lie from the one being fixed. + if suite_case_count == 0 { + continue; + } + session_start_time_unix_nanos = session_start_time_unix_nanos.min(suite_start_time_unix_nanos); @@ -234,6 +272,7 @@ fn build_traces_with( request: ExportTraceServiceRequest { resource_spans: vec![resource_spans], }, + oversized_case_names, } } @@ -470,6 +509,98 @@ mod tests { } } + /// A name past the cap costs its own result and nothing else. + #[test] + fn an_oversized_name_is_skipped_and_its_suite_mates_still_upload() { + let mut parsed = sample_parsed(); + // One byte over, so the assertion is about the boundary and not about + // some comfortably-huge number that a wrong `>=` would also pass. + let oversized = "x".repeat(MAX_TEST_NAME_BYTES + 1); + parsed.cases.push(TestCase { + name: oversized.clone(), + suite_name: "pytest".to_string(), + duration: Some(Duration::from_secs_f64(0.003)), + file: None, + line: None, + status: TestStatus::Passed, + failure: Failure::default(), + }); + + let mut bytes: Vec = Vec::with_capacity(16 + 4 * 8); + bytes.extend(std::iter::repeat_n(0xAA, 16)); + bytes.extend(std::iter::repeat_n(0x11, 8)); + bytes.extend(std::iter::repeat_n(0x22, 8)); + bytes.extend(std::iter::repeat_n(0x33, 8)); + bytes.extend(std::iter::repeat_n(0x44, 8)); + let mut rng = FixedRng::new(bytes); + + let now: u64 = 1_700_000_000_000_000_000; + let metadata = UploadMetadata::default(); + let built = with_ci_env(&[], || build_traces_with(&parsed, &metadata, now, &mut rng)); + + assert_eq!(built.oversized_case_names, vec![oversized.clone()]); + + let spans = &built.request.resource_spans[0].scope_spans[0].spans; + // Reported, not uploaded -- in no span name and no attribute value. + assert!(!spans.iter().any(|s| s.name == oversized)); + assert!(!spans.iter().any(|s| s.attributes.iter().any(|a| { + matches!( + &a.value.as_ref().and_then(|v| v.value.as_ref()), + Some(AnyValueOneof::StringValue(v)) if v == &oversized + ) + }))); + // The two well-named cases in the same suite are untouched. + assert!( + spans + .iter() + .any(|s| s.name == "tests.test_func.test_success") + ); + assert!( + spans + .iter() + .any(|s| s.name == "tests.test_func.test_failed") + ); + } + + /// A name exactly at the cap is uploaded: the limit is inclusive, and a + /// test sitting on it must not silently lose its history. + #[test] + fn a_name_exactly_at_the_cap_is_still_uploaded() { + let mut parsed = sample_parsed(); + let at_cap = "y".repeat(MAX_TEST_NAME_BYTES); + parsed.cases.push(TestCase { + name: at_cap.clone(), + suite_name: "pytest".to_string(), + duration: Some(Duration::from_secs_f64(0.003)), + file: None, + line: None, + status: TestStatus::Passed, + failure: Failure::default(), + }); + + let mut bytes: Vec = Vec::with_capacity(16 + 5 * 8); + bytes.extend(std::iter::repeat_n(0xAA, 16)); + bytes.extend(std::iter::repeat_n(0x11, 8)); + bytes.extend(std::iter::repeat_n(0x22, 8)); + bytes.extend(std::iter::repeat_n(0x33, 8)); + bytes.extend(std::iter::repeat_n(0x44, 8)); + bytes.extend(std::iter::repeat_n(0x55, 8)); + let mut rng = FixedRng::new(bytes); + + let built = with_ci_env(&[], || { + build_traces_with( + &parsed, + &UploadMetadata::default(), + 1_700_000_000_000_000_000, + &mut rng, + ) + }); + + assert!(built.oversized_case_names.is_empty()); + let spans = &built.request.resource_spans[0].scope_spans[0].spans; + assert!(spans.iter().any(|s| s.name == at_cap)); + } + #[test] fn builds_session_suite_and_case_spans_with_consistent_parent_chain() { // 16 bytes for trace_id; 4×8 bytes for session, suite, From 7c091c754c8701ba570db671fd1465906881c7ed Mon Sep 17 00:00:00 2001 From: Julien Danjou Date: Fri, 4 Sep 2026 16:38:47 +0200 Subject: [PATCH 2/4] docs(live-tests): drop the mirror claims for deleted Python files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The module header promised this file "deliberately mirrors the Python version 1:1 ... so the port can't drift the contract by accident", and named `func-tests/test_live_smoke.py` and `func-tests/conftest.py` as what it mirrors. Both were deleted when the port completed. Four helpers made the same promise individually (`Mirrors conftest.py::cli`, `Mirrors Python subprocess.run`, `Matches Python result.stdout + result.stderr`, and `live_token`'s "Mirrors Python `live_token` fixture"). AGENTS.md: "There is no Python: the port is complete ... If you find a doc, comment, or rule mentioning [it], it is stale — fix it." The harm is concrete rather than cosmetic: a maintainer auditing which secret each test needs goes looking for the fixture the header says pins that mapping, and there isn't one. The header now states the invariant that actually holds — tests are grouped by credential under a banner, and a test belongs under the banner matching its helper — which is the thing a reader needs and the thing the previous commit had to correct. It also spells `LIVE_TEST_MERGIFY_TOKEN_ADMIN` out rather than abbreviating it to `_ADMIN`, so grepping for either secret finds this file. Left alone: the in-body notes recording which wire contracts were preserved across the Python → Rust port. Those are provenance for why a contract is shaped the way it is, not claims about a file that no longer exists. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JBz3hxMUDCuWftT6qAHtnn Change-Id: Idd9e1e28bfc50b8b71d07c5938b4fa89af2745ff --- crates/mergify-cli/tests/live_smoke.rs | 36 ++++++++++++-------------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/crates/mergify-cli/tests/live_smoke.rs b/crates/mergify-cli/tests/live_smoke.rs index 08ca9cd6..d35019d1 100644 --- a/crates/mergify-cli/tests/live_smoke.rs +++ b/crates/mergify-cli/tests/live_smoke.rs @@ -1,16 +1,15 @@ -//! Live smoke tests against the real Mergify API. Port of -//! `func-tests/test_live_smoke.py` + `func-tests/conftest.py`. +//! Live smoke tests against the real Mergify API. //! //! Each test fires when the real API's URL, auth, or wire format //! diverges from what the CLI expects. API-hitting tests skip //! (early-return with a `SKIP:` line) unless their token -//! (`LIVE_TEST_MERGIFY_TOKEN_CI` or `_ADMIN`) is set in the env; -//! locally-evaluated tests run unconditionally. Driven by -//! `.github/workflows/func-tests-live.yaml` on every PR. +//! (`LIVE_TEST_MERGIFY_TOKEN_CI` or `LIVE_TEST_MERGIFY_TOKEN_ADMIN`) +//! is set in the env. Locally-evaluated tests run unconditionally. +//! Driven by `.github/workflows/func-tests-live.yaml` on every PR. //! -//! Implementation deliberately mirrors the Python version 1:1 — -//! same scrubbed env list, same assertion messages, same fixture -//! shape — so the port can't drift the contract by accident. +//! Tests are grouped by the credential they need, under a banner +//! per group — a test's banner is the index of which secret it +//! consumes, so keep a test under the one matching its helper. use std::collections::HashSet; use std::path::{Path, PathBuf}; @@ -23,13 +22,13 @@ const API_URL: &str = "https://api.mergify.com"; const REPOSITORY: &str = "mergify-clients-testing/mergify-cli-repo"; const PULL_REQUEST: &str = "1"; -/// CLI invocation timeout. Mirrors Python `subprocess.run(timeout=30)`. +/// CLI invocation timeout. const CLI_TIMEOUT: Duration = Duration::from_secs(30); /// Env vars the CLI auto-detects from the surrounding CI runner. /// Scrub them so a developer running tests inside GitHub Actions /// or Buildkite doesn't get different behavior than a clean -/// laptop run. Mirrors `conftest.py::_CI_ENV_VARS`. +/// laptop run. const CI_ENV_VARS: &[&str] = &[ "CI", "GITHUB_ACTIONS", @@ -63,7 +62,7 @@ struct CliResult { impl CliResult { /// Combined stream for grep-style assertions where the message /// could land on either stdout or stderr depending on the - /// command. Matches Python `result.stdout + result.stderr`. + /// command. fn combined(&self) -> String { format!("{}{}", self.stdout, self.stderr) } @@ -84,10 +83,9 @@ fn mergify_binary() -> &'static Path { /// Run `mergify ` with a scrubbed env and a fresh tmp cwd. /// -/// Mirrors Python `conftest.py::cli` exactly: closes stdin so an -/// accidental interactive prompt fails fast instead of blocking; -/// caps wall-clock at [`CLI_TIMEOUT`] so a pathological hang -/// doesn't drag the CI matrix down with it. +/// Closes stdin so an accidental interactive prompt fails fast +/// instead of blocking. Caps wall-clock at [`CLI_TIMEOUT`] so a +/// pathological hang doesn't drag the CI matrix down with it. /// /// **Concurrency.** Cargo's stock test harness runs every /// `#[test]` in this binary in a single process across a thread @@ -187,10 +185,10 @@ fn wait_timeout( } } -/// Look up `LIVE_TEST_MERGIFY_TOKEN_CI`. Mirrors Python -/// `live_token` fixture — empty / unset = skip the test (early -/// return with `SKIP:` printed to stderr so the cargo test log -/// shows what was skipped). +/// Look up `LIVE_TEST_MERGIFY_TOKEN_CI`, the key scoped to what a +/// CI job does. Empty / unset = skip the test (early return with +/// `SKIP:` printed to stderr so the cargo test log shows what was +/// skipped). fn live_token() -> Option { let token = std::env::var("LIVE_TEST_MERGIFY_TOKEN_CI") .unwrap_or_default() From d1cc0dca3b674d319303c955be93b409d4e43a55 Mon Sep 17 00:00:00 2001 From: Julien Danjou Date: Fri, 4 Sep 2026 12:04:06 +0200 Subject: [PATCH 3/4] docs(release): add a maintainer skill for cutting a release MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `RELEASING.md` documents the two-stage flow for a human driving the Actions UI. This adds the agent-facing counterpart: the `gh` commands for each stage, the pre-flight checks, and the guardrails. It lives in `.claude/skills/`, not the published `skills/` tree — that one ships as the `mergify` plugin for CLI users, where a runbook for releasing *this* repo would fire on anyone asking to release their own project. Content beyond what `RELEASING.md` already covers: - Up front: there is no version to bump in any file, so no "release prep" PR. The workflow stamps the tag at build time. - Pre-flight: main is green, no leftover draft, what ships since the last tag. - Stage 2 is irreversible and outward-facing (immutable release, PyPI push), so the skill stops after the draft and requires an explicit go-ahead. Stage 1 is marked safe and repeatable. - Post-publish verification of the Homebrew tap, which was undocumented: the `homebrew-tap-sync` workflow in `Mergifyio/mergify-ci-bot` opens a formula-bump PR against `Mergifyio/homebrew-tap` within ~20 min of publish, and it still needs a human to merge. The why — GitHub's immutable-releases policy and the reason stage 1 runs from `workflow_dispatch` — stays in `RELEASING.md`, which the skill points at, so the two can't drift on the rationale. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RVKnCd4MKakhVJJiJTfZcD Change-Id: I588ca3b86af6143c8440af6cab0b968f80e1a0f7 --- .claude/skills/releasing/SKILL.md | 117 ++++++++++++++++++++++++++++++ 1 file changed, 117 insertions(+) create mode 100644 .claude/skills/releasing/SKILL.md diff --git a/.claude/skills/releasing/SKILL.md b/.claude/skills/releasing/SKILL.md new file mode 100644 index 00000000..86cd2c62 --- /dev/null +++ b/.claude/skills/releasing/SKILL.md @@ -0,0 +1,117 @@ +--- +name: releasing +description: Cut and ship a new release of the `mergify` CLI — build the draft release with its platform binaries, publish it, and verify PyPI and the Homebrew tap picked it up. Use when asked to release, cut a release, ship a new version, publish a release, tag a version, or when a release run failed and needs recovery. Triggers on "release", "new version", "cut a release", "ship it", "publish the release", "bump the version", "release failed", "PyPI publish failed". +--- + +# Releasing `mergify-cli` + +Everything is driven by `.github/workflows/release.yml`. **There is no version +to bump in any file** — `pyproject.toml` and `Cargo.toml` keep their placeholder +versions and the workflow stamps the tag in at build time. Never open a "release +prep" PR. + +Versions are calver: `YYYY.M.D.N` (no zero padding, `N` starts at 1 each UTC day). + +`RELEASING.md` at the repo root is the human-facing runbook and explains *why* +the flow is shaped this way (GitHub's immutable-releases policy). Read it when +something goes wrong or the workflow itself needs changing. + +## Guardrails + +- **Never run `gh release create` or push a tag by hand.** A release created + outside the workflow has no binaries, and once published it is immutable — the + asset assertion then permanently blocks the PyPI publish for that version. +- **Never click / script "Draft a new release" in the Releases UI.** Same trap. +- **Stage 2 (Publish) is irreversible and outward-facing** — it locks the release + and pushes to PyPI. Always get the user's explicit go-ahead before publishing, + even if they already asked for "a release"; report the draft URL and stop. +- Stage 1 is safe and repeatable: a draft can be deleted and rebuilt. + +## Stage 1 — build the draft + +Pre-flight (report anything red, don't silently proceed): + +```shell +gh run list --workflow=ci.yaml --branch main --limit 1 # main is green +gh release list --limit 3 # no leftover draft +git log --oneline $(git describe --tags --abbrev=0)..origin/main # what ships +``` + +Trigger it: + +```shell +gh workflow run release.yml # auto-picks YYYY.M.D. +# or, only when a specific version is needed: +gh workflow run release.yml -f tag=2026.9.4.1 -f target_commitish= +``` + +Leave `tag` empty unless the user asked for a specific version; leave +`target_commitish` empty unless cherry-picking a release off an older line. + +Watch it (~5–10 min): + +```shell +gh run list --workflow=release.yml --limit 1 +gh run watch --exit-status +``` + +It builds the five-target wheel matrix, extracts the `mergify` binary from each +wheel, packages the archives + `SHA256SUMS`, dumps `cli-schema.json`, signs the +binaries with build provenance, and creates the **draft** release with notes +generated from the PRs merged since the previous tag. + +Then verify — the draft must carry exactly these seven assets: + +```shell +tag=$(gh release list --limit 1 --json tagName --jq '.[0].tagName') +gh release view "$tag" --json isDraft,url,assets --jq '{draft:.isDraft,url:.url,assets:[.assets[].name]}' +``` + +- `mergify--x86_64-unknown-linux-gnu.tar.gz` +- `mergify--aarch64-unknown-linux-gnu.tar.gz` +- `mergify--x86_64-apple-darwin.tar.gz` +- `mergify--aarch64-apple-darwin.tar.gz` +- `mergify--x86_64-pc-windows-msvc.zip` +- `SHA256SUMS` +- `cli-schema.json` + +Give the user the draft URL and the generated notes to review. **Stop here.** + +## Stage 2 — publish + +Only after the user explicitly says to publish: + +```shell +gh release edit "$tag" --draft=false --latest +``` + +That fires `release: published`, which re-asserts the seven assets, rebuilds the +wheels with the same version stamp, and pushes to PyPI via Trusted Publishing +(~5–10 min). Watch the `release` run the same way as in stage 1. + +Note edits must happen *before* this (`gh release edit "$tag" --notes-file +notes.md`) — drafts are mutable, published releases are not. + +## Verify after publishing + +```shell +gh run list --workflow=release.yml --limit 2 # publish job green +curl -s https://pypi.org/pypi/mergify-cli/json | jq -r .info.version +gh pr list --repo Mergifyio/homebrew-tap --author mergify-ci-bot +``` + +The Homebrew formula bump is automatic: the `homebrew-tap-sync` workflow in +`Mergifyio/mergify-ci-bot` opens a PR against `Mergifyio/homebrew-tap` within +~20 minutes of publish, updating `RELEASE` and the four per-arch checksums from +`SHA256SUMS`. It still needs a human to merge it. The docs site picks up +`cli-schema.json` off the latest release on its own — nothing to do there. + +## Recovery + +| Symptom | Fix | +|---|---| +| Stage 1 failed mid-run | No release exists yet, or a draft does. Delete the draft (`gh release delete --cleanup-tag`) and re-run stage 1. | +| Draft missing assets | Delete the draft and re-run stage 1 — never backfill by hand. | +| `assert-binaries-present` failed after publish | The release was created outside the workflow and is now immutable. Delete the release *and* its tag, then re-run stage 1 with the same tag. | +| PyPI publish failed (transient / outage) | Wheels are built; re-run just the failed job: `gh run rerun --failed`. | +| Wrong version already on PyPI | PyPI versions can't be reused or overwritten. Ship the next `N` — do not try to reuse the tag. | From 4c11a93a8a632a96fcd288d7fb14cda95f84ec59 Mon Sep 17 00:00:00 2001 From: Julien Danjou Date: Fri, 4 Sep 2026 17:37:33 +0200 Subject: [PATCH 4/4] test(live): cover the quarantine add/remove round-trip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MRGFY-9001 dropped `ci_application_key` from `POST` and `DELETE` on `/ci/{owner}/repositories/{repo}/quarantines` at the same time it dropped it from `search/tests`. Only `search/tests` had a live test, so only `search/tests` turned red; `mergify tests quarantines add` and `remove` broke for every `ci` key with no signal at all. The first two commits of this stack documented that. This closes it. The 20-odd wiremock tests in `tests_quarantine.rs` cannot cover this: a mock server never authenticates, so no scope change is visible to them. It has to be the live suite or nothing. Shape follows `freeze_create_update_delete_roundtrip`, the existing create-and-clean-up test in this file: a `Drop` guard removes the entry so a failed assertion mid-test still leaves the canary repository clean, and it warns rather than panics, because panicking in `Drop` during an unwind aborts the process and buries the assertion message that explains the failure. The guard is registered before the response is parsed — an unparseable body still means the row exists server-side. Two details specific to quarantines: - The quarantined name is `__mergify_cli_smoke_quarantine___` and matches nothing any real suite reports, so even a completely skipped cleanup cannot suppress a genuine failure on the canary repository. A leaked row is inert. - Removal goes by name rather than by the id `add` returned, so one run covers the list read that resolves the name *and* the delete. The guard tolerates exactly two outcomes: exit 0, when the body failed before its own remove, and `MergifyApiError` carrying `not_found`'s `'' is not quarantined`, when the body already removed the row. Both halves are load-bearing — that exit code is every Mergify API error including a 403 on a narrowed scope, and the message on its own would swallow any failure whose text happened to contain the phrase. It reads the code off `mergify_core::ExitCode` rather than a literal, so a renumbering cannot silently widen what cleanup calls success. The 8-char entropy the freeze test generated inline is now `unique_suffix()`, shared by both. Co-Authored-By: Claude Opus 5 (1M context) Change-Id: I20ed0098fe86acd633ad6864eed035f6e5f47dd1 --- crates/mergify-cli/tests/live_smoke.rs | 172 ++++++++++++++++++++++--- 1 file changed, 152 insertions(+), 20 deletions(-) diff --git a/crates/mergify-cli/tests/live_smoke.rs b/crates/mergify-cli/tests/live_smoke.rs index d35019d1..67f993f6 100644 --- a/crates/mergify-cli/tests/live_smoke.rs +++ b/crates/mergify-cli/tests/live_smoke.rs @@ -217,6 +217,24 @@ fn live_admin_token() -> Option { (!token.is_empty()).then_some(token) } +/// 8 random hex-ish chars, so concurrent or repeated runs never +/// fight over the same row on the shared canary repository. +/// `tempfile`'s name generation is already a dependency and draws +/// from `getrandom`, so it saves pulling in a uuid crate for this. +fn unique_suffix() -> String { + let dir = tempfile::tempdir().expect("tempdir for entropy"); + let name = dir + .path() + .file_name() + .and_then(|s| s.to_str()) + .unwrap_or("00000000") + .to_string(); + // tempdir names are like ".tmpXXXXXX" — take a tail slice so + // the literal prefix is not included. + let tail: String = name.chars().rev().take(8).collect(); + tail.chars().rev().collect::() +} + /// Helper for "early-return on missing token". Cargo's stock /// test harness doesn't have a "skip" outcome — early-returning /// counts as pass. Logging `SKIP:` to stderr makes the elided @@ -509,25 +527,8 @@ fn freeze_create_update_delete_roundtrip() { let token = skip_if_unset!(live_admin_token()); // Unique reason so concurrent or repeated runs don't fight - // over the same row. The Python suite uses - // `uuid.uuid4().hex[:8]`; reproduce that entropy with - // `tempfile`'s name-generation (32 hex chars from - // `getrandom`), truncated to 8. - let suffix = { - let dir = tempfile::tempdir().expect("tempdir for entropy"); - let name = dir - .path() - .file_name() - .and_then(|s| s.to_str()) - .unwrap_or("00000000") - .to_string(); - // tempdir names are like ".tmpXXXXXX" or "tmpXXXXXX" with - // ~6+ random chars after the prefix. Take a tail slice so - // we don't include the literal prefix. - let tail: String = name.chars().rev().take(8).collect(); - tail.chars().rev().collect::() - }; - let reason = format!("func-tests-live-smoke-{suffix}"); + // over the same row. + let reason = format!("func-tests-live-smoke-{}", unique_suffix()); let create = cli(&[ "freeze", @@ -608,7 +609,7 @@ fn freeze_create_update_delete_roundtrip() { } // --------------------------------------------------------------- -// CI-Insights test reads (admin token). +// CI-Insights tests family (admin token). // --------------------------------------------------------------- #[test] @@ -654,6 +655,137 @@ fn tests_show_no_match() { ); } +/// RAII cleanup for `tests_quarantine_add_remove_roundtrip` — +/// runs `quarantines remove` from `Drop` so a failed assertion +/// mid-test still leaves the canary repository clean. Same +/// warn-don't-panic posture as [`DeleteFreezeOnDrop`]: panicking +/// in `Drop` during an unwind aborts the process and buries the +/// real assertion message. +struct RemoveQuarantineOnDrop<'a> { + token: &'a str, + test_name: &'a str, +} +impl Drop for RemoveQuarantineOnDrop<'_> { + fn drop(&mut self) { + let remove = cli(&[ + "tests", + "quarantines", + "remove", + "--api-url", + API_URL, + "--token", + self.token, + "--repository", + REPOSITORY, + self.test_name, + ]); + // Two outcomes leave the repository clean: exit 0, when + // the body failed before its own remove, and the happy + // path's exit 6 — `MergifyApiError` — carrying + // `not_found`'s `'' is not quarantined`, because the + // body already removed the row. + // + // Both halves are load-bearing. Exit 6 alone is every + // Mergify API error, a 403 on a narrowed scope included; + // the message alone would swallow any failure whose text + // happens to contain that phrase. Anything else is a + // cleanup that did not happen, and the row is still there. + let already_gone = remove.returncode + == i32::from(mergify_core::ExitCode::MergifyApiError.as_u8()) + && remove.combined().contains("is not quarantined"); + if remove.returncode != 0 && !already_gone { + eprintln!("WARNING: quarantine cleanup failed{}", remove.context()); + } + } +} + +#[test] +fn tests_quarantine_add_remove_roundtrip() { + // `POST` + `DELETE` round-trip on + // `/v1/ci/{owner}/repositories/{repo}/quarantines`. + // + // MRGFY-9001 dropped `ci_application_key` from both of these + // alongside `search/tests`, and nothing here noticed, because + // the quarantine mutations had no live coverage at all — the + // wiremock tests in `tests_quarantine.rs` cannot see a scope + // change, since a mock server never authenticates. This is + // that canary: it fires on an auth, URL or wire-format change + // to either endpoint. + // + // Quarantines a name no real suite reports, so the entry can + // never suppress a genuine failure on the canary repository + // even if cleanup is skipped entirely. + let token = skip_if_unset!(live_admin_token()); + + let test_name = format!("__mergify_cli_smoke_quarantine_{}__", unique_suffix()); + let reason = "func-tests-live-smoke"; + + let add = cli(&[ + "tests", + "quarantines", + "add", + "--api-url", + API_URL, + "--token", + &token, + "--repository", + REPOSITORY, + "--reason", + reason, + "--json", + &test_name, + ]); + assert_eq!(add.returncode, 0, "quarantines add failed{}", add.context()); + + // Registered before the payload is parsed: an unparseable + // body still means the row was created server-side. + let _cleanup = RemoveQuarantineOnDrop { + token: &token, + test_name: &test_name, + }; + + let added: Value = serde_json::from_str(&add.stdout).unwrap_or_else(|e| { + panic!( + "quarantines add --json emitted non-JSON output\nerror: {e}\nstdout:\n{}", + add.stdout + ) + }); + assert_eq!( + added["test_name"], + serde_json::json!(test_name), + "add echoed a different test name\nstdout:\n{}", + add.stdout + ); + assert!( + added["id"].as_str().is_some_and(|id| !id.is_empty()), + "add emitted no quarantine id\nstdout:\n{}", + add.stdout + ); + + // Remove by name, which is the path that resolves the id via + // the list endpoint — so one run covers both the `ci`-allowed + // read and the admin-only delete. + let remove = cli(&[ + "tests", + "quarantines", + "remove", + "--api-url", + API_URL, + "--token", + &token, + "--repository", + REPOSITORY, + "--json", + &test_name, + ]); + assert_eq!( + remove.returncode, + 0, + "quarantines remove failed{}", + remove.context() + ); +} + // --------------------------------------------------------------- // CI commands — locally evaluated, no token needed. // ---------------------------------------------------------------