test!: confirm the semver gate allows a declared break#10837
Closed
gustavovalverde wants to merge 7 commits into
Closed
test!: confirm the semver gate allows a declared break#10837gustavovalverde wants to merge 7 commits into
gustavovalverde wants to merge 7 commits into
Conversation
Fail a pull request that breaks a published crate's public Rust API unless the break is declared with a conventional-commit `!` in the PR title. cargo-semver-checks flags added-field and added-variant breaks, which require a major version bump even though they only add to the API. The `!` marker is the author's declaration and the signal release-plz uses to bump the major version. Add semver-checks-result to both Mergify queue condition sets so the gate blocks merges. Document the `!` convention in CONTRIBUTING and the tool division of labor in docs/proposals/release-changelog-pipeline.md.
The gate failed its own CI: the pinned nightly toolchain emitted a rustdoc JSON format that cargo-semver-checks cannot parse. Use stable, the supported and recommended toolchain, matching the sibling required-check workflows. Trigger on push and merge_group with the real check gated to pull_request, and add `edited` so a title change re-runs the gate, so the merge queue is not stalled. Compute the `!` marker in the changes job and skip the check for declared breaks, so a tooling failure is reported as itself rather than as a public API break.
…se baseline The workspace run failed two ways: it semver-checked the zebrad binary, whose build script needs test-only proto files absent from the baseline checkout, and it compared against the base SHA, which lags the merge commit and attributed unrelated main-branch changes to the PR. Run cargo-semver-checks through obi1kenobi/cargo-semver-checks-action, the way lint.yml runs cargo-deny. Exclude zebrad, check default features, check out the PR head, and use the merge-base with the base branch as the baseline so only the PR's own API delta is judged.
zebra-test has two feature-gated `expect_no_requests` methods with different return types; cargo-semver-checks resolved different ones for the workspace build and the isolated baseline build, producing a false "breaking change". Exclude the binary, test-helper, and tooling crates (zebrad, zebra-test, zebra-utils) so the gate checks only the nine library crates that downstream code depends on.
Contributor
Merge Protections🟢 Merge protection satisfied — ready to merge. Show 1 satisfied protection🟢 📃 Configuration Change RequirementsMergify configuration change
|
Contributor
There was a problem hiding this comment.
Pull request overview
This is an explicitly throwaway, "do not merge" PR whose sole purpose is to validate the cargo-semver-checks CI gate introduced in #10833. It deliberately introduces an additive-but-breaking public API change (constructible_struct_adds_field) by adding one pub field to RegtestParameters, so the gate should detect the undeclared break and fail. The PR also carries the gate infrastructure from #10833 (workflow, path filter, Mergify required check, and CONTRIBUTING guidance), since it is branched from that change.
I verified the technical correctness of the change set:
- All
RegtestParametersconstruction sites still compile: every other site (tests/vectors.rs,config/tests/vectors.rs,zebrad/tests/integration/database.rs) uses..Default::default(), so only the full struct literal inzebra-network/src/config.rsneeded the new field, andnew_regtestcorrectly ignores it via... - The
semver-checks.ymlworkflow mirrors the existinglint.ymlchanges+alls-greenaggregator pattern, and the baseline/gate logic (skip on declared!break, fail on undeclared break) is sound. - The
excludelist leaves exactly the nine published library crates (the workspace has 12 members; no crate ispublish = false), matching the comment and #10833's description. mergify.ymlcorrectly registerssemver-checks-resultas a required check in both thedefaultsandqueue_rulesblocks.
I found no objective code defects. The dead/unused gate_test_field on a consensus-critical zebra-chain struct is intentional and explicitly acknowledged in the description (the PR is to be closed once both gate behaviors are observed), so I did not raise it as a finding. That said, this PR modifies merge-gating CI (mergify.yml required checks) and a consensus-critical data structure, and is explicitly not intended to merge — both warrant human handling rather than automated approval.
Changes:
- Adds a temporary
pub gate_test_field: Option<bool>toRegtestParametersand updates the in-tree construction/destructuring sites to keep the workspace compiling. - Carries the
cargo-semver-checksgate from #10833: newsemver-checks.ymlworkflow,semverpath filter, andsemver-checks-resultas a Mergify required check. - Documents the conventional-commit
!breaking-change marker inCONTRIBUTING.md.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
zebra-chain/src/parameters/network/testnet.rs |
Adds the temporary gate_test_field to RegtestParameters (the intended breaking change) and ignores it via .. in new_regtest. |
zebra-network/src/config.rs |
Updates the only full struct-literal construction site to set gate_test_field: None so the workspace compiles. |
.github/workflows/semver-checks.yml |
Adds the gate workflow: changes paths-filter + PR-title ! detection, semver-checks against the merge-base baseline, and an alls-green result job. |
.github/path-filters.yml |
Adds a semver filter so the gate runs on Rust/manifest and workflow changes. |
.github/mergify.yml |
Registers semver-checks-result as the fourth required aggregator check in both queue condition sets. |
CONTRIBUTING.md |
Documents the ! breaking-change marker required by the gate. |
gustavovalverde
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Do not merge. Throwaway PR to validate the semver-checks gate from #10833.
What this does
Adds one
pubfield toRegtestParameters, a struct downstream code constructs with a struct literal. That is a breaking change (constructible_struct_adds_field) even though it only adds to the API, which is exactly the case a public-API surface diff treats as additive and misses. The two in-tree construction sites are updated so the workspace still compiles.What to expect
The PR title has no
!, so the gate should run and fail: cargo-semver-checks reports that the new field requires a major version bump, and the gate blocks because the break is not declared.To confirm the escape and the
editedre-trigger, change the title totest!: .... The gate should re-run and pass, because a declared break is allowed through (release-plz majors the version at release).Close this PR once both behaviors are observed.
AI Disclosure
AI tools were used: Claude created this validation PR.