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. | diff --git a/crates/mergify-ci/src/junit_process/command.rs b/crates/mergify-ci/src/junit_process/command.rs index f020d1f0..13c614ae 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, 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. // ---------------------------------------------------------------