From 60cb56f63d5f7b01409e9fcf1e7302c3b014fcc6 Mon Sep 17 00:00:00 2001 From: iunanua Date: Fri, 21 Aug 2026 15:33:06 +0200 Subject: [PATCH 1/3] docs: add a practical guide to release proposals Document the crates.io release-proposal flow end to end: the dispatch workflow, how the semver bump level is derived, what CI validates on the proposal PR, and the manual GitLab publish job. Calls out the semver-level.sh / major-bumps-level.sh limitations (they only under-estimate, so minor and patch bumps are the ones to review), that the hotfix path is implemented but untested, and a troubleshooting table. Co-Authored-By: Claude Opus 5 (1M context) --- .../release-proposal-pr-review-bumps/SKILL.md | 180 ++++++++++++++ docs/release-proposals.md | 234 ++++++++++++++++++ 2 files changed, 414 insertions(+) create mode 100644 .claude/skills/release-proposal-pr-review-bumps/SKILL.md create mode 100644 docs/release-proposals.md diff --git a/.claude/skills/release-proposal-pr-review-bumps/SKILL.md b/.claude/skills/release-proposal-pr-review-bumps/SKILL.md new file mode 100644 index 0000000000..fefd347dfc --- /dev/null +++ b/.claude/skills/release-proposal-pr-review-bumps/SKILL.md @@ -0,0 +1,180 @@ +--- +name: release-proposal-pr-review-bumps +description: Review a libdatadog (or similar Rust workspace) release-proposal PR that bumps multiple crate versions, verifying each crate's semver bump (major/minor/patch) is correct based on its ACTUAL per-crate public-API delta — not just the conventional-commit `!` markers. Use when asked to "review a release PR", "check version bumps", "verify the semver bumps", or review a "chore(release): proposal..." PR. +--- + +# Reviewing release-proposal version bumps + +A release-proposal PR (usually authored by the release bot, e.g. `chore(release): proposal for ...`) bumps several crates at once. Each crate's bump is derived from the commits since its last release. The reviewer's job has **two layers**: + +1. **Per-crate:** confirm each crate's bump matches the **real public-API change in that specific crate** (the bulk of this skill). +2. **Workspace-level:** confirm the bumps are *consistent across the dependency graph* — specifically that every major bump has cascaded through its reverse-dependency closure (see "The major-version cascade" below). The release bot routinely gets this wrong: it bumps/releases only crates that have their own commits, while silently rewriting path-dependency requirements everywhere else. That produces under-bumped dependents that force a new major of a shared crate onto consumers without a version bump — the single most damaging defect in these PRs. **Always run this check; it is easy to miss because the under-bumped crate's own diff looks innocent.** + +## The core principle + +A commit marked breaking (`!` in its conventional-commit title, e.g. `feat(data-pipeline)!: ...`) is often a **multi-crate sweep**. The breaking change usually lands in only ONE crate; the same commit may touch other crates with purely additive or internal edits. So: + +- **Never** infer a crate's bump from the `!` marker alone. +- For each crate, look at **only that crate's slice** of each commit and classify the highest-severity change to *its own* public API. + +Bump rules (per crate, based on the highest-severity change): +- **major** — a breaking public-API change: removed/renamed/signature-changed `pub` item; changed `pub` struct field type; changed/removed enum variant; removed trait method; dropped public trait impl (e.g. a `derive` removed in default builds). +- **minor** — only additive: new `pub` items, nothing removed/changed. (Promoting a `pub(crate)`/private item to `pub`, or renaming a non-`pub` item, counts as additive — it was never externally visible.) +- **patch** — internal only: private code, `#[cfg(test)]`/`mod tests`, benches, `[dev-dependencies]`, comments, bug fixes with no public-API change. + +## Watch for these subtle cases + +1. **Sub-major bump carrying a `!` commit** (minor/patch crate that includes a breaking-marked commit) — the highest-priority thing to verify. Confirm the breaking part is NOT in this crate. +2. **Transitive breakage** — a crate that re-exports, or uses in a `pub` signature, a type from a dependency that changed. If the changed type leaks into the crate's public API, the crate breaks too. If it's only used internally / behind a trait with stable signatures, it does not. + - Check: does the crate `pub use` the changed type? Does any `pub fn`/struct field expose it directly (vs. being generic over a trait whose method signatures are unchanged)? +3. **Feature-gated breaks** — a breaking change behind a non-default Cargo feature is weaker justification for a major under strict default-feature semver. Note it, but a default-surface break elsewhere still independently justifies major. +4. **Forced-major dependency bumps are often breaking** (do NOT reflexively treat as patch). When crate A goes **major**, every dependent's `Cargo.toml` requirement on A is rewritten to A's new major (`^1` → `^2`), forcing A's new major onto the dependent's consumers. Whether that obliges the dependent to *also* go major depends on whether A is **safe to duplicate** — run the two-part test in "The major-version cascade". Short version: if A is a public dependency of the dependent (exposed type, or a foreign-trait-impl on a public type) **or** A is unsafe to duplicate (singleton/global state, single-artifact link) and consumers use `^` ranges, the dependent must go **major** too. (The older guidance "dep bump = patch" is wrong for these.) Minor/patch dependency bumps of A (same major) never cascade — `^1.2` already unifies with `1.3.0`. +5. **Initial releases** (e.g. `1.0.0`, CHANGELOG newly added, previously `publish = false`/unpublished) — nothing to semver-diff against; just confirm the version is sane and the crate was genuinely unpublished. +6. **Test/bench-only commits** — patch is the safe, conservative choice even when arguably no bump was needed. + +## The major-version cascade (workspace-level check) + +Two semver-incompatible majors of the same crate can be resolved into a single dependency tree by Cargo. Whether that is harmless or a "boom" depends entirely on whether the dependency is **safe to duplicate**. Whether the dependent needs a forced-major bump (the cascade) hinges on the same question. So before forcing majors up the graph, run the test below — don't blanket-cascade. + +### The "safe to duplicate" test — BOTH checks must pass + +A crate is safe to duplicate (two majors can coexist harmlessly) **only if both** of these hold. Failing *either* one makes duplication harmful and forces the cascade. Checking only the first is the classic mistake. + +**(a) No process-global / singleton state.** Grep the crate's `src` for anything that must be unique per process: + ```bash + grep -rnE 'static |lazy_static|once_cell|OnceLock|OnceCell|Lazy|thread_local|#\[no_mangle\]|#\[export_name|#\[ctor|atexit|pthread_atfork|signal\(' /src --include=*.rs + ``` + Hits on real globals, FFI exported symbols, ctors, or fork/signal/atexit handlers = duplication is unsafe (two copies fight over the same process resource or clash at link time, especially inside the single FFI/C `builder` artifact). Comments and instance-scoped registration (e.g. `AtomicWaker::register`, "worker registered *on a SharedRuntime instance*") do NOT count — confirm the constructor is an instance method (`Foo::new()`), not a global accessor (`fn global() -> &'static Foo`). + +**(b) No shared types/traits crossing a crate boundary.** Even a globally-stateless crate is unsafe to duplicate if its types or **traits** are part of the *integration contract* between two other crates — because v1's type/trait is a different type from v2's, so a value/impl from a v1-built crate won't satisfy a v2 bound. Check the dependents, not just the dependency: + - Does dependent X **expose the dep's type in its public API** (re-export, `pub fn` arg/return, `pub` field)? e.g. `pub fn set_shared_runtime(_: Arc)`. + - Does dependent X **implement a trait from the dep on one of X's public types**? A foreign-trait impl on a public type *is* public API. e.g. `impl libdd_shared_runtime::Worker for TelemetryWorker` — even though `SharedRuntime` never appears in telemetry's signatures, consumers rely on `TelemetryWorker: Worker`. + - Does some *other* crate Y then **consume that across the boundary**? e.g. data-pipeline calls `shared_runtime.spawn_worker(telemetry_worker)` (production, `trace_exporter/builder.rs`), which requires `TelemetryWorker: ::Worker`. If telemetry is on `shared-runtime ^1` and data-pipeline on `^2`, the trait impl targets the wrong major → **hard compile error**, not just redundant copies. + ```bash + # type in dependent's public API: + grep -rnE 'pub use .*|pub fn .*|pub .*: &?(mut )?|-> .*' /src --include=*.rs | grep -v cfg.test + # foreign-trait impls of the dep's traits on the dependent's types: + grep -rnE 'impl .* for ' /src --include=*.rs | grep -v cfg.test + # cross-crate consumption of that contract (e.g. spawn/register taking the impl): + grep -rnE '\(' /src --include=*.rs | grep -v cfg.test + ``` + +Worked example (`libdd-shared-runtime`): passes (a) — instance-based `SharedRuntime::new()`, zero globals — but **fails (b)**: `TelemetryWorker` implements its `Worker` trait and data-pipeline spawns that worker on a `SharedRuntime` in production. So telemetry, data-pipeline, and any `^`-range consumer (e.g. dd-trace-rs) must all agree on one shared-runtime major → the cascade is a genuine correctness requirement here, not conservative over-bumping. + +### Classifying the dependent's bump once duplication is unsafe + +- If the dep is a **public dependency** of the dependent (fails (b): exposed type *or* foreign-trait-impl-on-public-type) → the dependent's own public contract changed with the dep's major → **major is correct, not weird**. This is the real public-dependency case. +- If the dep is **private** to the dependent (passes (b): used only internally, never crossing a boundary) but **fails (a)** (singleton/global, or single-artifact link) → the dependent's API is technically unchanged, so strict SemVer would allow patch — but a patch/minor is auto-picked by `^`-range consumers during `cargo update`, silently dragging in the incompatible major. So bump **major anyway** to force a deliberate, loud upgrade. (Major doesn't *prevent* the diamond; it makes it opt-in instead of a silent surprise.) +- If the dep passes **both** (a) and (b) → genuinely safe to duplicate → **no cascade**; a patch re-release (for manifest coherence) or nothing is fine. + +So a major bump of a non-duplicable shared crate **cascades as a major bump through its reverse-dependency closure of publishable crates**, in topological order. Each crate in the closure that gains a new-major requirement (directly or transitively, e.g. `crashtracker → telemetry → shared-runtime`) must itself go major, which in turn forces *its* dependents major, and so on. + +Rules of thumb: +- **Cascade triggers ONLY on major dependency bumps.** Minor/patch dep bumps (same major) never cascade. +- **`publish = false` crates don't need a version bump** (no registry artifact / no `^`-range consumers), but their path-dep requirements must still be internally consistent. +- A crate **already in the release list can still be under-bumped** by this rule — e.g. a crate correctly classified `patch` on its *own* API but which directly depends on a major-bumped shared crate must be upgraded to `major`. Check in-list crates against the cascade too, not just the omitted ones. +- Confirm how downstream actually pins libdatadog. If every consumer pins the *whole workspace at one exact version*, the duplicate-major hazard can't arise and the cascade is moot — but for any crate consumed independently with `^` ranges, it is real. + +## Workflow + +1. **Fetch the PR** with `gh pr view --json title,body,headRefName,baseRefName,files,commits`. The body lists each crate, its next version, the bump type, and the attributed commits. (Base ref may be another `release/...` branch in a stacked release — review only the crates in the body.) +2. **Resolve commits locally.** For each PR number in the body: `git log --oneline --all --grep="(#)" -1`. Confirm all are present. +3. **Map each commit's crate footprint** so you know which commits are multi-crate sweeps: + ``` + git show --stat --format= | grep -oE '^ [a-zA-Z0-9_./-]+' | sed 's,/.*,,' | sort | uniq -c | sort -rn + ``` +4. **Fan out one subagent per crate** (run them concurrently — multiple Agent calls in one message). Give each subagent: the crate name, proposed next version + bump, the attributed commit hashes (flagging which are `!`-marked multi-crate sweeps), and the method below. Prioritize the sub-major-with-`!` cases. +5. **Each subagent's method:** + - Inspect ONLY the crate's slice: `git show -- /` (or `/src/` to skip tests). + - Verify public reachability: is the changed item reachable from the crate root (`pub mod` chain in `lib.rs`, `pub use` re-exports)? `#[cfg(test)]`/`mod tests`/`benches/` and private items don't count. + - Classify highest severity (major/minor/patch) with **evidence**: file path, item name, before/after signature. + - Check transitive breakage via re-exports and `pub` signatures (point 2 above) and `Cargo.toml` dep changes. + - Return a verdict: is the proposed bump correct, too low, or too high — with cited evidence. +6. **Run the major-version cascade check** (workspace-level — do this whenever ANY crate in the proposal gets a *major* bump). For each major-bumped crate, compute its reverse-dependency closure among publishable workspace crates and confirm every crate in it is also bumped **major**. A helper to build the closure and surface under-bumped crates against the proposal head/base refs: + ```bash + python3 - "$HEAD_REF" "$BASE_REF" <<'PY' + import subprocess, re, sys, os + head, base = sys.argv[1], sys.argv[2] + MAJOR_BUMPED = {"libdd-shared-runtime","libdd-trace-utils","libdd-data-pipeline"} # set to the crates getting a MAJOR bump in this proposal + def manifest(ref,d): + try: return subprocess.check_output(["git","show",f"{ref}:{d}/Cargo.toml"],stderr=subprocess.DEVNULL).decode() + except Exception: return "" + def parse(ref,d): + t=manifest(ref,d) + if not t: return None + ver=re.search(r'(?m)^version\s*=\s*"([^"]+)"',t) + publish = not re.search(r'(?m)^publish\s*=\s*false',t) + cut=len(t) + for mk in ("[dev-dependencies]","[build-dependencies]"): # normal deps only + i=t.find(mk); cut=min(cut,i) if i!=-1 else cut + deps=set(re.findall(r'(?m)^(libdd-[a-z0-9-]+)\s*=',t[:cut])) + return {"ver":ver.group(1) if ver else "?","publish":publish,"deps":deps} + dirs=[d for d in os.listdir(".") if os.path.isfile(os.path.join(d,"Cargo.toml"))] + info={d:parse(head,d) for d in dirs}; info={k:v for k,v in info.items() if v} + # transitive reverse-dependency closure + targets=set(MAJOR_BUMPED); changed=True + while changed: + changed=False + for d,m in info.items(): + if d not in targets and (m["deps"] & targets): + targets.add(d); changed=True + print(f"{'crate':28} {'pub':4} {'base_ver':10} {'head_ver':10} bumped? deps-on-major") + for d in sorted(targets - MAJOR_BUMPED): + m=info[d]; b=parse(base,d) + bumped = "MAJOR" if (b and b['ver'].split('.')[0]!=m['ver'].split('.')[0]) else "** NOT-MAJOR **" + direct=sorted(m["deps"] & MAJOR_BUMPED) + print(f"{d:28} {'PUB' if m['publish'] else '-':4} {(b['ver'] if b else '?'):10} {m['ver']:10} {bumped:16} {direct}") + PY + ``` + Any `PUB` crate flagged `** NOT-MAJOR **` is a defect: it either needs adding to the release as a major bump, or (if already in the list) its bump needs raising to major. Remember the intra-closure requirement edges must also move to the new majors (e.g. `crashtracker → telemetry ^N`). +7. **Synthesize** a verdict table (crate | proposed | correct? | why) plus the cascade findings and any non-blocking notes (changelog accuracy, feature-gated breaks). + +## What the automated level cannot see + +`scripts/semver-level.sh` (and the `pr-title-semver-check` job built on it) runs +`cargo-semver-checks` plus a `cargo-public-api` diff. Treat its answer as a **floor, not a +verdict**: a `patch` result is only trustworthy for changes that touch none of the +categories below. Each is pinned by a test in `scripts/tests/semver-level/` +(`detection_matrix.bats`, grep `KNOWN MISS`), verified against cargo-semver-checks 0.47.0 +and cargo-public-api 0.52.0 — so if one starts being detected, that suite is what tells you. + +Manually check these whenever the automated level is `patch` or `minor`: + +1. **`#[repr(C)]` field reordering.** `repr_c_plain_struct_fields_reordered` is a + **warning-level** lint: cargo-semver-checks prints `Summary no semver update required` + and exits 0, so the script never sees it. This is the one that matters most here — it is + a silent ABI break for every FFI consumer compiled against the old header, and it reports + as `patch`. The rest of the repr family (`repr_c_removed`, `repr_align_changed`, + `repr_packed_added`, `enum_repr_int_changed`) fails properly. **Check any diff that + touches field order in a `#[repr(C)]` type.** +2. **Public dependency major bumps behind unchanged signatures.** `pub fn f(u: hyper::Uri)` + renders identically whether `hyper` is 0.14 or 1.0; only the resolved dependency version + moved. Overlaps with subtle case 2 above, and is the mechanism behind the cascade check. +3. **Non-host targets.** The script passes no `--target`, so only the host triple is + analysed. Windows- and macOS-only `#[cfg]` API is never compared — relevant for + crashtracker and common. +4. **Feature-gated API.** `cargo semver-checks` runs `--all-features` but `cargo public-api` + runs default features only, and the public-api pass is the *only* thing that catches + parameter and return type changes. So a signature change behind a non-default feature is + caught by neither. (Compounds subtle case 3.) +5. **Crate renames.** A renamed crate is absent from the baseline, so it is classified as a + *new* crate and reported `minor`. Renaming a published crate breaks every consumer. +6. **Declarative macro bodies.** Only removal of a `#[macro_export] macro_rules!` is linted; + narrowing or dropping an arm is invisible to both tools. +7. **Trait-impl and inference breakage.** Adding `impl Trait for T`, or an inherent method + that shadows a trait method downstream, reads as a plain addition. +8. **Behaviour.** New panics, changed error semantics, altered serde/wire representation — + no signature tool can see these. +9. **The generated C API.** Both tools read rustdoc JSON, so nothing validates `builder`'s + generated headers or pkg-config output. Per `AGENTS.md` the C FFI offers no ABI + compatibility guarantee, so this is by design — but do not mistake a green semver check + for FFI safety. + +Two properties of the tooling that also affect how you reproduce a level locally: the script +needs a **clean working tree** (`cargo public-api diff` does a real `git checkout`), and it +needs `RUSTUP_TOOLCHAIN` overridden because `rust-toolchain.toml` pins an MSRV older than +cargo-semver-checks requires. See `scripts/tests/semver-level/README.md`. + +## Output + +A concise table of per-crate verdicts with file:symbol evidence, then the **cascade verdict** (every major bump propagated through its publishable reverse-dependency closure? list any under-bumped/omitted crates and the major version each needs), then a short list of non-blocking observations. State plainly whether every bump is correct, or which need changing and to what. diff --git a/docs/release-proposals.md b/docs/release-proposals.md new file mode 100644 index 0000000000..0a4139d5c0 --- /dev/null +++ b/docs/release-proposals.md @@ -0,0 +1,234 @@ +# Release proposals (publishing crates to crates.io) + +Practical guide to the flow that publishes `libdd-*` (and other workspace) crates to +crates.io. + +> **Not this flow:** the FFI **artifact** release (`vX.Y.Z` tarballs + headers) is a +> different pipeline — `scripts/create-release.sh` and libddprof-build's +> `draft_github_release.sh`. It uses `release/vX.Y.Z` branches (one path segment), which +> do not collide with the `release//` branches used here. + +## At a glance + +``` + ┌─ GitHub Actions ────────────────────────────────────────────────┐ + │ 1. workflow_dispatch: "Release - Open a release proposal PR" │ + │ release-proposal-dispatch.yml │ + │ ├─ validate crates, membership, no ongoing proposal │ + │ ├─ create ephemeral branch release// │ + │ ├─ create proposal branch release-proposal// │ + │ ├─ per crate: semver-level.sh → cargo release version │ + │ ├─ force major on direct libdd-* major bumps │ + │ ├─ git-cliff CHANGELOGs │ + │ └─ draft PR: release-proposal/... → release/... │ + │ 2. release-proposal-test.yml (on that push / PR) │ + │ cargo package + compile dd-trace-rs against the packages │ + └─────────────────────────────────────────────────────────────────┘ + │ squash-merge the proposal PR + ▼ (push to release/**) + ┌─ GitLab (libddprof-build, via the ddbuild mirror) ──────────────┐ + │ 3. publish_cargo_crates (MANUAL job, DRY_RUN=true by default) │ + │ ├─ crates-to-package.sh → publication-order.sh → tags │ + │ ├─ publish-crates.sh: test, cargo publish, crates.io owners │ + │ ├─ create annotated GitHub tags -v │ + │ └─ create_pr_to_merge_release_branch.sh: release/... → main │ + └─────────────────────────────────────────────────────────────────┘ + │ merge that PR + ▼ + main has the bumps + CHANGELOGs +``` + +## 1. Open the proposal + +Actions → **Release - Open a release proposal PR** → Run workflow. + +| Input | Notes | +|---|---| +| `crates` | Comma-separated. Each crate is released together with its workspace `libdd-*` dependencies (`scripts/publication-order.sh`). Only publishable crates (`publish != false`) are accepted. | +| `main_start_ref` | Empty = tip of `origin/main`. A SHA/branch/tag is allowed **only if reachable from `origin/main`** or from the matching `origin/hotfix//N.x.x`. `refs/pull/*` is rejected. | +| `bypass_standard_checks` | Testing only: skips the ongoing-proposal guard and the team-membership check, uses `release-testing/` + `release-proposal-testing/` prefixes, pushes plainly (no verified commits), and stops skipping crates whose tag is not the latest. | + +Guards that will stop you: + +- **Ongoing proposal** — any existing `origin/release-proposal/*` or `origin/release/*/*` + branch aborts the run. One release at a time. +- **Membership** — the actor must be in `Datadog/apm-common-components-core`. +- **Untrusted `cargo-release` config** — the tree must not mention + `pre-release-hook` / `pre-release-replacements` anywhere in `Cargo.toml` / `release.toml`. +- Release scripts are always taken from the **workflow revision** (`github.sha`), not from + `main_start_ref`. + +What the job produces: two branches, one bump commit + one CHANGELOG commit per crate +(pushed via `DataDog/commit-headless` so they are verified), and a **draft** PR titled +`chore(release): proposal for `, based on the ephemeral `release/...` branch. +The `release-dispatch-data` artifact (1 day retention) holds the intermediate JSON +(`commits-by-crate.json`, `api-changes*.json`) — start debugging there. + +### Which crates actually get released + +`scripts/commits-since-release.sh` lists commits since `-v` **that touch +the crate's directory**, dropping merge commits, `chore(release)` subjects, and anything +authored by `dd-octo-sts[bot]`. + +- Commits found → the crate is bumped. +- No commits and a tag exists → deferred; released **only** if a direct `libdd-*` + dependency goes major in this proposal. +- No tag at all → initial release, forced to `major`, and the run **fails unless the + manifest version is exactly `0.1.0`**. +- The crate's resolved tag is not the latest SemVer tag for that crate → skipped + (that release is already on `main`), unless it is a hotfix or `bypass_standard_checks`. + +## 2. How the bump level is decided + +`scripts/semver-level.sh refs/tags/` computes `major | minor | patch` +from two tools and takes the **higher** of the two: + +1. `cargo semver-checks -p --all-features --baseline-rev ` + → `major` on "requires new major", `minor` on "requires new minor", `minor` if the + crate is absent from the baseline (new crate). +2. `cargo public-api --package diff ..HEAD` (skipped if 1. already said + major, or the crate is new) + → removed items = `major`; changed items = `major` **if** a difference survives + normalization (diff markers, `#[...]` attributes and `const`/`async`/`unsafe` are + stripped); added items = `minor`. + +No signal at all ⇒ `patch`. The level is then fed to `cargo release version -p -x `. + +Then `scripts/major-bumps-level.sh` re-reads each crate's **direct** `libdd-*` +dependency requirements at `prev_tag` vs. the proposal tree and forces `major` where a +requirement's major digit increased — this is how "protobuf 3→4" propagates to its +dependents, and how a no-commit crate can still end up in the release. + +### Limitations you must review by hand + +`semver-level.sh` looks only at the Rust public API surface. It does **not** know about: + +- **Conventional-commit intent.** `feat!:` / `BREAKING CHANGE:` markers are ignored + entirely. A breaking change that does not alter a signature lands as `patch`. +- **Behavioural breakage.** Same signature, different semantics (defaults, error + behaviour, panics, wire format, protobuf/proto file changes) ⇒ `patch`. +- **Feature-gated API.** Everything runs with `--all-features`, so API that only exists + under a non-default feature combination (e.g. `libdd-http-client`'s mutually exclusive + `reqwest-backend` / `hyper-backend`) is analysed in exactly one configuration. +- **`cargo-semver-checks` false negatives** — notably parameter type changes on + non-generic functions (`function_parameter_type_changed` is unimplemented). The + `cargo-public-api` pass exists to cover that; it needs **`cargo-public-api >= 0.52.0`** + (older versions include parameter names, so a harmless *rename* is promoted to major). + +### ⚠️ Review every `minor` and `patch` bump by hand + +Every limitation above fails in the same direction: it **under**-estimates the level. So +the bumps that need scrutiny are the low ones. + +- **`patch` / `minor` — dangerous.** A missed breaking change published as a patch or + minor silently breaks consumers on `cargo update`. Read the commits listed for that + crate in the PR body and ask whether any of them changes behaviour, an FFI layout, a + wire format, or an API under a feature the analysis did not exercise. If so, raise the + level on the proposal branch before merging. +- **`major` — safe to accept.** An over-estimated major only costs a version number; + consumers must opt in, so nothing breaks. Never argue a `major` down to save a digit. +- **Don't use the `!` marker as your verdict.** It is per-PR, while a PR usually touches + several crates: a `feat!:` in the list says *something* in that PR breaks, not that it + breaks for every crate the PR modified, and not which one. Treat a `!` under a crate as + a prompt to read that crate's slice of the diff — and remember the converse, that a + commit with no `!` can still be breaking for one of the crates it touches. + +⇒ Sanity-check each bump in the PR body against its listed commits, spending the effort on +the `patch` and `minor` rows. The `/release-proposal-pr-review-bumps` skill does exactly +this review. + +## 3. Review the proposal PR + +`release-proposal-test.yml` runs on every push to `release-proposal/**` and on PRs based +on `release/**`: + +1. `scripts/crates-to-package.sh` (base = PR base / merge-base with `main`) lists + publishable crates whose **own** version changed, then `cargo +1.92.0 package` them. + Cargo ≥ 1.92 is required because sibling versions are not on crates.io yet. +2. Resolves the newest patch of the 3 most recent `datadog-opentelemetry-v*` release + lines in `DataDog/dd-trace-rs`, and builds each of them with + `--config patch.crates-io..path=...` pointing at the unpacked `.crate` files. + The log also prints duplicated `libdd-*` versions in the tree — check it. + +Note the PR is a **draft**: mark it ready before merging. Its `skip-*` labels disable the +metadata/changelog/PR-title checks that do not apply to release commits. + +## 4. Publish (GitLab) + +Squash-merge the proposal PR into the ephemeral `release//` branch. +That push is mirrored to `gitlab.ddbuild.io/DataDog/libdatadog`, whose `.gitlab-ci.yml` +triggers libddprof-build with `LIBDATADOG_IS_RELEASE_BRANCH=true` (branch matches +`^release/` or `^hotfix/`). + +`publish_cargo_crates` is created only when **both** hold +(`.rules_run_on_module_release`): + +- `LIBDATADOG_IS_RELEASE_BRANCH == "true"`, and +- `LIBDATADOG_COMMIT_TITLE =~ /chore\(release\): proposal/` — i.e. the squash commit must + keep the PR title. **Do not** merge with a merge commit. + +It is a **manual** job with `DRY_RUN: "true"`. Run it as-is first (it validates versions, +runs the tests and `cargo publish --dry-run`), then re-run it with `DRY_RUN=false` to +publish for real. In order, per crate (`publish-crates.sh`, in publication order): + +1. tag version must equal the manifest version; +2. skip if that version is already on crates.io; +3. `cargo nextest --no-default-features` (warn only) and `--all-features` (**blocking**), + excluding `tracing_integration_tests::`; +4. `cargo publish --all-features`, then add the `github:datadog:libdatadog-owners` owner. + +Only **after every crate in the batch succeeds** are the annotated GitHub tags +`-v` created on the release-branch commit. Finally +`create_pr_to_merge_release_branch.sh` opens a draft PR `release/... → main` (skipped for +hotfixes) — merge it so the bumps and CHANGELOGs land on `main`, and delete the ephemeral +branch (a leftover `release/*/*` blocks the next proposal). + +## Hotfixes + +> ⚠️ **Untested path.** Every stage below is implemented — the dispatch workflow, the +> GitLab publish rule and the cleanup steps all special-case hotfixes — but the flow has +> never been exercised end to end on a real hotfix. + +Pass `main_start_ref = hotfix//.x.x` (the branch must exist on origin) and +**exactly one crate**. Differences: + +- the hotfix branch *is* the ephemeral branch — nothing new is created and it is never + deleted by the cleanup steps; +- crates whose tag is not the latest are **not** skipped; +- no merge-back PR to `main` is opened. + +Since the proposal PR targets `hotfix/**` and not `release/**`, only the `push`-triggered +half of `release-proposal-test.yml` runs for it. + +## Cancelling / retrying a proposal + +The dispatch job cleans up both branches on failure. If a run half-succeeded or the +proposal is wrong: close the PR and delete **both** `release-proposal/<...>` and +`release/<...>` on origin, then dispatch again. The ongoing-proposal guard will keep +failing until both are gone. + +If publication failed **after** some crates were published, those versions are on +crates.io but no GitHub tags exist. Do not re-publish them — start a new proposal for the +remaining crates from the same commit the release branch was cut from (the failure output +prints that merge-base). + +## Troubleshooting + +| Symptom | Cause / fix | +|---|---| +| `Error: A release proposal is ongoing` | A leftover `release-proposal/*` or `release/*/*` branch on origin. Delete it (or wait for the merge-back PR). | +| `Error: unknown or unpublishable crate(s)` | Typo, or the crate has `publish = false`. The error prints the valid list. | +| `Error: resolved commit ... is not reachable from origin/main` | `main_start_ref` points outside `main` / the hotfix branch. Use a commit on a trusted branch. | +| `Error: is not a 0.1.0 release` | No `-v*` tag exists, so the run treats it as an initial release. Set the manifest version to `0.1.0` or create the missing tag. | +| `No changes to push. Cancelling the workflow.` | Nothing to release: no commits touched the selected crates' directories since their tags (remember: path-filtered, and `chore(release)` / bot commits are dropped). | +| `Semver level:` followed by a `jq` parse error, or `cargo release version -x` with an empty level | `semver-level.sh` output is parsed as JSON; anything it printed on stderr (it is captured with `2>&1`) breaks the parse. Read the raw step log for the real error, usually a `cargo semver-checks` / `cargo public-api` build failure. | +| `Unexpected exit code from cargo-semver-checks` / `Unexpected error from cargo-public-api` | The crate does not build at the baseline tag or at HEAD with `--all-features`. Reproduce locally: `./scripts/semver-level.sh -v refs/tags/-v`. | +| Bump level looks too low in the PR body | Expected for behavioural/ABI/`0.x`-dependency breakage — see the limitations above. A too-low `patch`/`minor` is the failure that matters; edit the version and CHANGELOG on the proposal branch before merging, or close and re-dispatch. A too-high `major` is harmless — leave it. | +| `cargo package` fails in `release-proposal-test.yml` | Usually a sibling crate version not yet on crates.io; the job pins `cargo +1.92.0` for this. Also check for `Cargo.lock` drift and missing files in `include`. | +| dd-trace-rs compile job fails | A real breaking change reaching a consumer — that is the point of the job. Check the "Duplicated dependencies" section for two `libdd-*` majors in one tree. | +| `publish_cargo_crates` is absent from the GitLab pipeline | The merge commit title does not start with `chore(release): proposal` (merge commit instead of squash), or the branch is not `release/**` / `hotfix/**`, or the pipeline was not triggered from the mirror (`CI_PIPELINE_SOURCE != "pipeline"`). | +| `Skipping cargo package: no crates had a version bump` in GitLab | `crates-to-package.sh` compares `LIBDATADOG_COMMIT_BEFORE_SHA..LIBDATADOG_COMMIT_SHA`; a push that carries no version change (e.g. a follow-up commit on the release branch) produces nothing. Re-run the job on the push that contains the bumps. | +| `Version mismatch! tag vs Cargo.toml` | A manual edit desynced the manifest from the computed tag. Fix the manifest on the release branch. | +| `Version X of is already published` | Skipped, not an error — normal on a re-run. | +| Publication succeeded but no tags | Some crate in the batch failed; tags are created only after the whole batch succeeds. See "Cancelling / retrying" above. | +| Everything published but `main` lacks the bumps | Merge the `release/... → main` draft PR (it is not created for hotfixes — port those manually). | From 4e1257efa6084c37f746bf2d65f8eb2ea271e36f Mon Sep 17 00:00:00 2001 From: iunanua Date: Wed, 2 Sep 2026 14:59:42 +0200 Subject: [PATCH 2/3] remove doc --- docs/release-proposals.md | 234 -------------------------------------- 1 file changed, 234 deletions(-) delete mode 100644 docs/release-proposals.md diff --git a/docs/release-proposals.md b/docs/release-proposals.md deleted file mode 100644 index 0a4139d5c0..0000000000 --- a/docs/release-proposals.md +++ /dev/null @@ -1,234 +0,0 @@ -# Release proposals (publishing crates to crates.io) - -Practical guide to the flow that publishes `libdd-*` (and other workspace) crates to -crates.io. - -> **Not this flow:** the FFI **artifact** release (`vX.Y.Z` tarballs + headers) is a -> different pipeline — `scripts/create-release.sh` and libddprof-build's -> `draft_github_release.sh`. It uses `release/vX.Y.Z` branches (one path segment), which -> do not collide with the `release//` branches used here. - -## At a glance - -``` - ┌─ GitHub Actions ────────────────────────────────────────────────┐ - │ 1. workflow_dispatch: "Release - Open a release proposal PR" │ - │ release-proposal-dispatch.yml │ - │ ├─ validate crates, membership, no ongoing proposal │ - │ ├─ create ephemeral branch release// │ - │ ├─ create proposal branch release-proposal// │ - │ ├─ per crate: semver-level.sh → cargo release version │ - │ ├─ force major on direct libdd-* major bumps │ - │ ├─ git-cliff CHANGELOGs │ - │ └─ draft PR: release-proposal/... → release/... │ - │ 2. release-proposal-test.yml (on that push / PR) │ - │ cargo package + compile dd-trace-rs against the packages │ - └─────────────────────────────────────────────────────────────────┘ - │ squash-merge the proposal PR - ▼ (push to release/**) - ┌─ GitLab (libddprof-build, via the ddbuild mirror) ──────────────┐ - │ 3. publish_cargo_crates (MANUAL job, DRY_RUN=true by default) │ - │ ├─ crates-to-package.sh → publication-order.sh → tags │ - │ ├─ publish-crates.sh: test, cargo publish, crates.io owners │ - │ ├─ create annotated GitHub tags -v │ - │ └─ create_pr_to_merge_release_branch.sh: release/... → main │ - └─────────────────────────────────────────────────────────────────┘ - │ merge that PR - ▼ - main has the bumps + CHANGELOGs -``` - -## 1. Open the proposal - -Actions → **Release - Open a release proposal PR** → Run workflow. - -| Input | Notes | -|---|---| -| `crates` | Comma-separated. Each crate is released together with its workspace `libdd-*` dependencies (`scripts/publication-order.sh`). Only publishable crates (`publish != false`) are accepted. | -| `main_start_ref` | Empty = tip of `origin/main`. A SHA/branch/tag is allowed **only if reachable from `origin/main`** or from the matching `origin/hotfix//N.x.x`. `refs/pull/*` is rejected. | -| `bypass_standard_checks` | Testing only: skips the ongoing-proposal guard and the team-membership check, uses `release-testing/` + `release-proposal-testing/` prefixes, pushes plainly (no verified commits), and stops skipping crates whose tag is not the latest. | - -Guards that will stop you: - -- **Ongoing proposal** — any existing `origin/release-proposal/*` or `origin/release/*/*` - branch aborts the run. One release at a time. -- **Membership** — the actor must be in `Datadog/apm-common-components-core`. -- **Untrusted `cargo-release` config** — the tree must not mention - `pre-release-hook` / `pre-release-replacements` anywhere in `Cargo.toml` / `release.toml`. -- Release scripts are always taken from the **workflow revision** (`github.sha`), not from - `main_start_ref`. - -What the job produces: two branches, one bump commit + one CHANGELOG commit per crate -(pushed via `DataDog/commit-headless` so they are verified), and a **draft** PR titled -`chore(release): proposal for `, based on the ephemeral `release/...` branch. -The `release-dispatch-data` artifact (1 day retention) holds the intermediate JSON -(`commits-by-crate.json`, `api-changes*.json`) — start debugging there. - -### Which crates actually get released - -`scripts/commits-since-release.sh` lists commits since `-v` **that touch -the crate's directory**, dropping merge commits, `chore(release)` subjects, and anything -authored by `dd-octo-sts[bot]`. - -- Commits found → the crate is bumped. -- No commits and a tag exists → deferred; released **only** if a direct `libdd-*` - dependency goes major in this proposal. -- No tag at all → initial release, forced to `major`, and the run **fails unless the - manifest version is exactly `0.1.0`**. -- The crate's resolved tag is not the latest SemVer tag for that crate → skipped - (that release is already on `main`), unless it is a hotfix or `bypass_standard_checks`. - -## 2. How the bump level is decided - -`scripts/semver-level.sh refs/tags/` computes `major | minor | patch` -from two tools and takes the **higher** of the two: - -1. `cargo semver-checks -p --all-features --baseline-rev ` - → `major` on "requires new major", `minor` on "requires new minor", `minor` if the - crate is absent from the baseline (new crate). -2. `cargo public-api --package diff ..HEAD` (skipped if 1. already said - major, or the crate is new) - → removed items = `major`; changed items = `major` **if** a difference survives - normalization (diff markers, `#[...]` attributes and `const`/`async`/`unsafe` are - stripped); added items = `minor`. - -No signal at all ⇒ `patch`. The level is then fed to `cargo release version -p -x `. - -Then `scripts/major-bumps-level.sh` re-reads each crate's **direct** `libdd-*` -dependency requirements at `prev_tag` vs. the proposal tree and forces `major` where a -requirement's major digit increased — this is how "protobuf 3→4" propagates to its -dependents, and how a no-commit crate can still end up in the release. - -### Limitations you must review by hand - -`semver-level.sh` looks only at the Rust public API surface. It does **not** know about: - -- **Conventional-commit intent.** `feat!:` / `BREAKING CHANGE:` markers are ignored - entirely. A breaking change that does not alter a signature lands as `patch`. -- **Behavioural breakage.** Same signature, different semantics (defaults, error - behaviour, panics, wire format, protobuf/proto file changes) ⇒ `patch`. -- **Feature-gated API.** Everything runs with `--all-features`, so API that only exists - under a non-default feature combination (e.g. `libdd-http-client`'s mutually exclusive - `reqwest-backend` / `hyper-backend`) is analysed in exactly one configuration. -- **`cargo-semver-checks` false negatives** — notably parameter type changes on - non-generic functions (`function_parameter_type_changed` is unimplemented). The - `cargo-public-api` pass exists to cover that; it needs **`cargo-public-api >= 0.52.0`** - (older versions include parameter names, so a harmless *rename* is promoted to major). - -### ⚠️ Review every `minor` and `patch` bump by hand - -Every limitation above fails in the same direction: it **under**-estimates the level. So -the bumps that need scrutiny are the low ones. - -- **`patch` / `minor` — dangerous.** A missed breaking change published as a patch or - minor silently breaks consumers on `cargo update`. Read the commits listed for that - crate in the PR body and ask whether any of them changes behaviour, an FFI layout, a - wire format, or an API under a feature the analysis did not exercise. If so, raise the - level on the proposal branch before merging. -- **`major` — safe to accept.** An over-estimated major only costs a version number; - consumers must opt in, so nothing breaks. Never argue a `major` down to save a digit. -- **Don't use the `!` marker as your verdict.** It is per-PR, while a PR usually touches - several crates: a `feat!:` in the list says *something* in that PR breaks, not that it - breaks for every crate the PR modified, and not which one. Treat a `!` under a crate as - a prompt to read that crate's slice of the diff — and remember the converse, that a - commit with no `!` can still be breaking for one of the crates it touches. - -⇒ Sanity-check each bump in the PR body against its listed commits, spending the effort on -the `patch` and `minor` rows. The `/release-proposal-pr-review-bumps` skill does exactly -this review. - -## 3. Review the proposal PR - -`release-proposal-test.yml` runs on every push to `release-proposal/**` and on PRs based -on `release/**`: - -1. `scripts/crates-to-package.sh` (base = PR base / merge-base with `main`) lists - publishable crates whose **own** version changed, then `cargo +1.92.0 package` them. - Cargo ≥ 1.92 is required because sibling versions are not on crates.io yet. -2. Resolves the newest patch of the 3 most recent `datadog-opentelemetry-v*` release - lines in `DataDog/dd-trace-rs`, and builds each of them with - `--config patch.crates-io..path=...` pointing at the unpacked `.crate` files. - The log also prints duplicated `libdd-*` versions in the tree — check it. - -Note the PR is a **draft**: mark it ready before merging. Its `skip-*` labels disable the -metadata/changelog/PR-title checks that do not apply to release commits. - -## 4. Publish (GitLab) - -Squash-merge the proposal PR into the ephemeral `release//` branch. -That push is mirrored to `gitlab.ddbuild.io/DataDog/libdatadog`, whose `.gitlab-ci.yml` -triggers libddprof-build with `LIBDATADOG_IS_RELEASE_BRANCH=true` (branch matches -`^release/` or `^hotfix/`). - -`publish_cargo_crates` is created only when **both** hold -(`.rules_run_on_module_release`): - -- `LIBDATADOG_IS_RELEASE_BRANCH == "true"`, and -- `LIBDATADOG_COMMIT_TITLE =~ /chore\(release\): proposal/` — i.e. the squash commit must - keep the PR title. **Do not** merge with a merge commit. - -It is a **manual** job with `DRY_RUN: "true"`. Run it as-is first (it validates versions, -runs the tests and `cargo publish --dry-run`), then re-run it with `DRY_RUN=false` to -publish for real. In order, per crate (`publish-crates.sh`, in publication order): - -1. tag version must equal the manifest version; -2. skip if that version is already on crates.io; -3. `cargo nextest --no-default-features` (warn only) and `--all-features` (**blocking**), - excluding `tracing_integration_tests::`; -4. `cargo publish --all-features`, then add the `github:datadog:libdatadog-owners` owner. - -Only **after every crate in the batch succeeds** are the annotated GitHub tags -`-v` created on the release-branch commit. Finally -`create_pr_to_merge_release_branch.sh` opens a draft PR `release/... → main` (skipped for -hotfixes) — merge it so the bumps and CHANGELOGs land on `main`, and delete the ephemeral -branch (a leftover `release/*/*` blocks the next proposal). - -## Hotfixes - -> ⚠️ **Untested path.** Every stage below is implemented — the dispatch workflow, the -> GitLab publish rule and the cleanup steps all special-case hotfixes — but the flow has -> never been exercised end to end on a real hotfix. - -Pass `main_start_ref = hotfix//.x.x` (the branch must exist on origin) and -**exactly one crate**. Differences: - -- the hotfix branch *is* the ephemeral branch — nothing new is created and it is never - deleted by the cleanup steps; -- crates whose tag is not the latest are **not** skipped; -- no merge-back PR to `main` is opened. - -Since the proposal PR targets `hotfix/**` and not `release/**`, only the `push`-triggered -half of `release-proposal-test.yml` runs for it. - -## Cancelling / retrying a proposal - -The dispatch job cleans up both branches on failure. If a run half-succeeded or the -proposal is wrong: close the PR and delete **both** `release-proposal/<...>` and -`release/<...>` on origin, then dispatch again. The ongoing-proposal guard will keep -failing until both are gone. - -If publication failed **after** some crates were published, those versions are on -crates.io but no GitHub tags exist. Do not re-publish them — start a new proposal for the -remaining crates from the same commit the release branch was cut from (the failure output -prints that merge-base). - -## Troubleshooting - -| Symptom | Cause / fix | -|---|---| -| `Error: A release proposal is ongoing` | A leftover `release-proposal/*` or `release/*/*` branch on origin. Delete it (or wait for the merge-back PR). | -| `Error: unknown or unpublishable crate(s)` | Typo, or the crate has `publish = false`. The error prints the valid list. | -| `Error: resolved commit ... is not reachable from origin/main` | `main_start_ref` points outside `main` / the hotfix branch. Use a commit on a trusted branch. | -| `Error: is not a 0.1.0 release` | No `-v*` tag exists, so the run treats it as an initial release. Set the manifest version to `0.1.0` or create the missing tag. | -| `No changes to push. Cancelling the workflow.` | Nothing to release: no commits touched the selected crates' directories since their tags (remember: path-filtered, and `chore(release)` / bot commits are dropped). | -| `Semver level:` followed by a `jq` parse error, or `cargo release version -x` with an empty level | `semver-level.sh` output is parsed as JSON; anything it printed on stderr (it is captured with `2>&1`) breaks the parse. Read the raw step log for the real error, usually a `cargo semver-checks` / `cargo public-api` build failure. | -| `Unexpected exit code from cargo-semver-checks` / `Unexpected error from cargo-public-api` | The crate does not build at the baseline tag or at HEAD with `--all-features`. Reproduce locally: `./scripts/semver-level.sh -v refs/tags/-v`. | -| Bump level looks too low in the PR body | Expected for behavioural/ABI/`0.x`-dependency breakage — see the limitations above. A too-low `patch`/`minor` is the failure that matters; edit the version and CHANGELOG on the proposal branch before merging, or close and re-dispatch. A too-high `major` is harmless — leave it. | -| `cargo package` fails in `release-proposal-test.yml` | Usually a sibling crate version not yet on crates.io; the job pins `cargo +1.92.0` for this. Also check for `Cargo.lock` drift and missing files in `include`. | -| dd-trace-rs compile job fails | A real breaking change reaching a consumer — that is the point of the job. Check the "Duplicated dependencies" section for two `libdd-*` majors in one tree. | -| `publish_cargo_crates` is absent from the GitLab pipeline | The merge commit title does not start with `chore(release): proposal` (merge commit instead of squash), or the branch is not `release/**` / `hotfix/**`, or the pipeline was not triggered from the mirror (`CI_PIPELINE_SOURCE != "pipeline"`). | -| `Skipping cargo package: no crates had a version bump` in GitLab | `crates-to-package.sh` compares `LIBDATADOG_COMMIT_BEFORE_SHA..LIBDATADOG_COMMIT_SHA`; a push that carries no version change (e.g. a follow-up commit on the release branch) produces nothing. Re-run the job on the push that contains the bumps. | -| `Version mismatch! tag vs Cargo.toml` | A manual edit desynced the manifest from the computed tag. Fix the manifest on the release branch. | -| `Version X of is already published` | Skipped, not an error — normal on a re-run. | -| Publication succeeded but no tags | Some crate in the batch failed; tags are created only after the whole batch succeeds. See "Cancelling / retrying" above. | -| Everything published but `main` lacks the bumps | Merge the `release/... → main` draft PR (it is not created for hotfixes — port those manually). | From bbcd408f94d0d3269370d68739a8b60fbb7f38ff Mon Sep 17 00:00:00 2001 From: iunanua Date: Wed, 2 Sep 2026 15:40:18 +0200 Subject: [PATCH 3/3] docs(release): fix semver review skill's minor rule and tool citations The bump rules classified any additive diff as minor ("new pub items, nothing removed/changed"), which contradicted the skill's own note that added trait impls can break inference. Adding an enum variant, a required trait method, or a field to a constructible struct breaks downstream matches, impls, and literals, so a reviewer following the rule could approve a breaking change as a minor release. - List the four additive-but-breaking forms under major, each with the exemption that must be verified in code (#[non_exhaustive] at the base ref, a default body, a pre-existing private field). - Require subagents to walk that list before returning minor. - Replace the scripts/tests/semver-level/ citation: no such directory, detection_matrix.bats, or README exists in the tree. Point at semver-level.sh, the workflow tool pins (0.48.0, not the claimed 0.47.0), and cargo semver-checks --list instead. - Note that --list's type column is the required update, not the lint level, so a major-typed lint can still be warn-level and exit 0. --- .../release-proposal-pr-review-bumps/SKILL.md | 68 +++++++++++++++---- 1 file changed, 55 insertions(+), 13 deletions(-) diff --git a/.claude/skills/release-proposal-pr-review-bumps/SKILL.md b/.claude/skills/release-proposal-pr-review-bumps/SKILL.md index fefd347dfc..078db5f146 100644 --- a/.claude/skills/release-proposal-pr-review-bumps/SKILL.md +++ b/.claude/skills/release-proposal-pr-review-bumps/SKILL.md @@ -18,10 +18,21 @@ A commit marked breaking (`!` in its conventional-commit title, e.g. `feat(data- - For each crate, look at **only that crate's slice** of each commit and classify the highest-severity change to *its own* public API. Bump rules (per crate, based on the highest-severity change): -- **major** — a breaking public-API change: removed/renamed/signature-changed `pub` item; changed `pub` struct field type; changed/removed enum variant; removed trait method; dropped public trait impl (e.g. a `derive` removed in default builds). -- **minor** — only additive: new `pub` items, nothing removed/changed. (Promoting a `pub(crate)`/private item to `pub`, or renaming a non-`pub` item, counts as additive — it was never externally visible.) +- **major** — a breaking public-API change: removed/renamed/signature-changed `pub` item; changed `pub` struct field type; changed/removed enum variant; removed trait method; dropped public trait impl (e.g. a `derive` removed in default builds); **plus any of the additive-but-breaking forms below**. +- **minor** — additive *and* checked non-breaking: new `pub` items, nothing removed or changed, **and none of the additions is one of the additive-but-breaking forms below**. (Promoting a `pub(crate)`/private item to `pub`, or renaming a non-`pub` item, counts as additive — it was never externally visible.) - **patch** — internal only: private code, `#[cfg(test)]`/`mod tests`, benches, `[dev-dependencies]`, comments, bug fixes with no public-API change. +### Additive-but-breaking: "new `pub` item" is NOT automatically minor + +A new item can break every downstream compile without removing or changing anything. Never classify a crate minor just because the diff only adds. For each new `pub` item, check which of these it is — the exemption must be **verified in the code**, not assumed: + +- **New variant on a public enum** → breaks downstream exhaustive `match`. Minor only if the enum is `#[non_exhaustive]` *and already was before this commit* (adding `#[non_exhaustive]` is itself breaking). Check the enum's attributes at the base ref, and that it is reachable from the crate root. +- **New required trait method on a public trait** (no default body) → breaks every downstream `impl` of that trait. Minor only if the method has a default body, or the trait is genuinely not downstream-implementable (sealed via a private supertrait / private-type argument). A default body added to a trait everyone already implements is still an inference/ambiguity risk — see the last bullet. +- **New `pub` field on a constructible public struct** → breaks downstream struct literals and exhaustive struct patterns. Minor only if the struct is `#[non_exhaustive]` (before this commit) or already had a private field, i.e. it could never be built or destructured by literal outside the crate. +- **New `impl Trait for T`, new blanket impl, or a new inherent method shadowing a trait method** → breaks downstream type inference and method resolution (the same hazard as "What the automated level cannot see" #7). Strict SemVer calls these minor-with-possible-breakage, so minor is defensible — but only as an *explicit* judgment call: state the impl/method, and whether anything plausibly resolves the shadowed name or relies on inference there. Do not approve it silently as "just an addition". + +`cargo-semver-checks` has a lint for each of the first three — `enum_variant_added` ("an exhaustive enum has a new variant"), `trait_method_added` ("a non-sealed public trait added a new method without a default implementation"), `constructible_struct_adds_field` — all typed `major` in `cargo semver-checks --list`, so an automated level of `minor` over one of these deserves a second look at the invocation (check the exit code, and see "What the automated level cannot see" for why a `major`-typed lint can still pass). The fourth form has no lint at all, so an automated `minor` there proves nothing. + ## Watch for these subtle cases 1. **Sub-major bump carrying a `!` commit** (minor/patch crate that includes a breaking-marked commit) — the highest-priority thing to verify. Confirm the breaking part is NOT in this crate. @@ -31,6 +42,7 @@ Bump rules (per crate, based on the highest-severity change): 4. **Forced-major dependency bumps are often breaking** (do NOT reflexively treat as patch). When crate A goes **major**, every dependent's `Cargo.toml` requirement on A is rewritten to A's new major (`^1` → `^2`), forcing A's new major onto the dependent's consumers. Whether that obliges the dependent to *also* go major depends on whether A is **safe to duplicate** — run the two-part test in "The major-version cascade". Short version: if A is a public dependency of the dependent (exposed type, or a foreign-trait-impl on a public type) **or** A is unsafe to duplicate (singleton/global state, single-artifact link) and consumers use `^` ranges, the dependent must go **major** too. (The older guidance "dep bump = patch" is wrong for these.) Minor/patch dependency bumps of A (same major) never cascade — `^1.2` already unifies with `1.3.0`. 5. **Initial releases** (e.g. `1.0.0`, CHANGELOG newly added, previously `publish = false`/unpublished) — nothing to semver-diff against; just confirm the version is sane and the crate was genuinely unpublished. 6. **Test/bench-only commits** — patch is the safe, conservative choice even when arguably no bump was needed. +7. **Additive-only diff proposed as minor** — the easiest bump to wave through and a common under-bump. An enum variant, a required trait method, or a struct field can be the whole diff and still be major; run the additive-but-breaking list above rather than eyeballing "nothing removed". ## The major-version cascade (workspace-level check) @@ -88,6 +100,7 @@ Rules of thumb: - Inspect ONLY the crate's slice: `git show -- /` (or `/src/` to skip tests). - Verify public reachability: is the changed item reachable from the crate root (`pub mod` chain in `lib.rs`, `pub use` re-exports)? `#[cfg(test)]`/`mod tests`/`benches/` and private items don't count. - Classify highest severity (major/minor/patch) with **evidence**: file path, item name, before/after signature. + - **Before returning `minor`, walk the additive-but-breaking list.** For every added `pub` item, say which form it is and why it is exempt: new enum variant → quote the enum's `#[non_exhaustive]` at the *base* ref; new trait method → quote its default body or the sealing mechanism; new struct field → quote the pre-existing private field or `#[non_exhaustive]`; new trait impl / shadowing inherent method → state the inference risk explicitly. "Nothing was removed" is not evidence for minor. - Check transitive breakage via re-exports and `pub` signatures (point 2 above) and `Cargo.toml` dep changes. - Return a verdict: is the proposed bump correct, too low, or too high — with cited evidence. 6. **Run the major-version cascade check** (workspace-level — do this whenever ANY crate in the proposal gets a *major* bump). For each major-bumped crate, compute its reverse-dependency closure among publishable workspace crates and confirm every crate in it is also bumped **major**. A helper to build the closure and surface under-bumped crates against the proposal head/base refs: @@ -134,19 +147,45 @@ Rules of thumb: `scripts/semver-level.sh` (and the `pr-title-semver-check` job built on it) runs `cargo-semver-checks` plus a `cargo-public-api` diff. Treat its answer as a **floor, not a verdict**: a `patch` result is only trustworthy for changes that touch none of the -categories below. Each is pinned by a test in `scripts/tests/semver-level/` -(`detection_matrix.bats`, grep `KNOWN MISS`), verified against cargo-semver-checks 0.47.0 -and cargo-public-api 0.52.0 — so if one starts being detected, that suite is what tells you. +categories below. + +The list below is a checklist, not a guarantee — a gap can close when the tooling is upgraded. +Before citing one as a reason to override the automated level, confirm it still holds against +the PR's base ref: + +- **The script's own logic and comments** — `scripts/semver-level.sh` (the two passes at + `# 1) cargo-semver-checks` and `# 2) cargo-public-api diff`, combined by `max_level`; the + header comment above `normalize_api_line` documents which signature deltas it deliberately + drops as non-semver-significant). +- **The tool versions actually installed**, which decide whether a miss below is still a + miss. Read them off the workflows rather than trusting any version quoted here: + ```bash + grep -rn 'cargo-semver-checks@\|cargo-public-api@' .github/workflows/ + ``` + At the time of writing both `pr-title-semver-check.yml` and `release-proposal-dispatch.yml` + pin `cargo-semver-checks@0.48.0` and `cargo-public-api@0.52.0`. If the pin has moved, a + category below may now be caught — re-check the specific lint before citing it as a gap. +- **Whether a lint exists at all**, for the `cargo-semver-checks` cases: `cargo semver-checks + --list` (`--explain ` for detail). Read its `type` column carefully — it is the + semver update the lint *reports* (`major`/`minor`), **not** whether a violation fails the + run. A lint can list as `major` and still be warn-level, printing the violation while + exiting 0; neither `--list` nor `--explain` shows it, and 0.47.0 has no `--deny`/`--warn` + override flag to force the issue (re-check `--help` for the pinned version). + So the only local way to settle a "does the script see it?" question is a + minimal two-crate repro plus `echo $?` on the actual invocation the script uses + (`cargo semver-checks -p --color=never --all-features --baseline-rev `). Manually check these whenever the automated level is `patch` or `minor`: -1. **`#[repr(C)]` field reordering.** `repr_c_plain_struct_fields_reordered` is a - **warning-level** lint: cargo-semver-checks prints `Summary no semver update required` - and exits 0, so the script never sees it. This is the one that matters most here — it is - a silent ABI break for every FFI consumer compiled against the old header, and it reports - as `patch`. The rest of the repr family (`repr_c_removed`, `repr_align_changed`, - `repr_packed_added`, `enum_repr_int_changed`) fails properly. **Check any diff that - touches field order in a `#[repr(C)]` type.** +1. **`#[repr(C)]` field reordering.** `repr_c_plain_struct_fields_reordered` is + **warning-level**: cargo-semver-checks prints the violation but still reports + `Summary no semver update required` and exits 0, so the script never sees it and the level + comes out `patch`. Note `--list` shows this lint as type `major` — that is the update it + *would* require, and does not contradict the warn level; confirm by exit code, not by + `--list`. This is the one that matters most here: a silent ABI break for every FFI + consumer compiled against the old header. The rest of the repr family (`repr_c_removed`, + `repr_align_changed`, `repr_packed_added`, `enum_repr_int_changed`) fails properly. + **Check any diff that touches field order in a `#[repr(C)]` type.** 2. **Public dependency major bumps behind unchanged signatures.** `pub fn f(u: hyper::Uri)` renders identically whether `hyper` is 0.14 or 1.0; only the resolved dependency version moved. Overlaps with subtle case 2 above, and is the mechanism behind the cascade check. @@ -173,7 +212,10 @@ Manually check these whenever the automated level is `patch` or `minor`: Two properties of the tooling that also affect how you reproduce a level locally: the script needs a **clean working tree** (`cargo public-api diff` does a real `git checkout`), and it needs `RUSTUP_TOOLCHAIN` overridden because `rust-toolchain.toml` pins an MSRV older than -cargo-semver-checks requires. See `scripts/tests/semver-level/README.md`. +cargo-semver-checks requires — see the `RUSTUP_TOOLCHAIN: ${{ env.RUST_VERSION }}` env on the +`Run semver checks on changed crates` step in `.github/workflows/pr-title-semver-check.yml`, +and the job-level `RUSTUP_TOOLCHAIN` env in `release-proposal-dispatch.yml`, for the +toolchain CI actually uses. ## Output