fix(release): classify releases via cargo-semver-checks vs previous version-bump commit#554
Conversation
093eabf to
673b000
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #554 +/- ##
=======================================
Coverage 100.0% 100.0%
=======================================
Files 356 356
Lines 27388 27388
=======================================
Hits 27388 27388 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
93d3b39 to
e34c1e6
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the release-planning logic to classify required version bumps using cargo semver-checks against each crate’s last published crates.io version, replacing the previous [package.metadata.cargo_check_external_types] heuristic. It also refactors failure handling in the release entrypoint to throw (for testability) while preserving exit 1 behavior in the thin CLI wrapper, and updates CI to run semver checks only for crates whose [package] version is bumped in the PR.
Changes:
- Replace dependent “exposes target” heuristics with per-crate
cargo semver-checks-based classification (with caching and explicit “no fallback” parsing/throwing behavior). - Add self-flooring so user-requested change types are raised (never lowered) to meet the crate’s own semver-checks-required minimum.
- Rework the CI semver job to baseline against crates.io and scope runs to the “publishing set” determined by version diffs.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/lib/releasing.ps1 | Adds cargo semver-checks invocation + output parsing and change-type ranking helpers; removes external-types exposure heuristic. |
| scripts/lib/release-flow.ps1 | Updates cascade resolution to use per-dependent semver-check verdicts, adds self-flooring, and refactors error handling to throw for testability. |
| scripts/release-packages.ps1 | Wraps the library entrypoint to preserve CLI exit 1 behavior while allowing the library to throw. |
| scripts/ci/compute-publishing-set.ps1 | New helper script to compute which crates are “publishing” (version-bumped) and emit --package ... args for CI. |
| .github/workflows/main.yml | Updates semver CI job to baseline against crates.io and run only for the publishing set; keeps it informational via sticky PR comment. |
| docs/releasing.md | Updates release documentation to reflect semver-check-based classification and the “hard dependency / no fallback” behavior. |
| scripts/tests/Pester/unit/releasing/PureFunctions.Tests.ps1 | Adds unit tests for parsing cargo semver-checks output and for change-type strengthening behavior. |
| scripts/tests/Pester/unit/release-packages/ResolveReleaseSet.Tests.ps1 | Updates resolver tests to use injected semver classifier instead of external-types exposure heuristics. |
| scripts/tests/Pester/scenarios/Scenarios.Tests.ps1 | Mocks preflight tool presence and replaces real semver checks with deterministic scenario verdict maps. |
| scripts/tests/Pester/_common/Invoke-Scenario.ps1 | Adds scenario plumbing for Run.SemverVerdicts (folder → verdict) used by the classifier mock. |
| scripts/tests/Pester/scenarios/S11-stable-version-minor-distinct.scenario.psd1 | Adds simulated semver verdicts to make scenario deterministic under the new classifier. |
| scripts/tests/Pester/scenarios/S13-pin-with-cascade-satisfied.scenario.psd1 | Adds simulated semver verdicts for pin/cascade interactions. |
| scripts/tests/Pester/scenarios/S14-pin-with-cascade-conflict.scenario.psd1 | Adds simulated semver verdicts for pin rejection scenario. |
| scripts/tests/Pester/scenarios/S15-auto-upgrade-of-user-source.scenario.psd1 | Adds simulated semver verdicts for auto-upgrade behavior. |
| scripts/tests/Pester/scenarios/S16-stable-cascade-elevation.scenario.psd1 | Updates scenario narrative and adds verdicts for semver-based dependent classification. |
| scripts/tests/Pester/scenarios/S23-all-force-release-unchanged.scenario.psd1 | Adds simulated verdicts to explain cascade behavior under the new semver classifier. |
| scripts/tests/Pester/scenarios/S24-pin-with-cascade-conflict-force.scenario.psd1 | Adds simulated verdicts for forced-pin behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
✅ Semver: version bumps look sufficient
|
61d2729 to
5128fe0
Compare
sandersaares
left a comment
There was a problem hiding this comment.
Reactivated a comment thread from my previous review - my concern remains unaddressed.
…history) Replace the crates.io registry baseline with a git-history baseline for semver-checks breakage analysis, per Sander's review on PR #554. Instead of discovering the last published version via `cargo info` and diffing against `--baseline-version`, the crate's previous version-bump commit (the most recent commit that changed its `[package] version`) is located in git history and passed to cargo-semver-checks as `--baseline-rev <sha>`, which rebuilds the baseline rustdoc from source. Why: versioning is a source-level concern and one workflow must serve both OSS and enterprise/offline consumers. The registry lookup cannot reach crates.io in private environments and its published content lags the source, so a declared-but-unpublished version (e.g. an aborted 4.0.0) was wrongly judged against a stale published 3.3.3. The git baseline is whatever the repo last declared, regardless of publish status, and needs no network access. Changes: - New Get-PreviousVersionBumpCommit helper (scripts/lib/releasing.ps1), unit-tested, returning the bump commit SHA + declared version. It matches only the [package] version (skips dependency-version and publish-only edits) and honours BaseRef so a PR's own bump is excluded. - CI report (scripts/ci/semver-report.ps1) now uses --baseline-rev; drops the -Registry param. Baseline column shows "<version> (<short-sha>)". Graceful warn/fail contract preserved (unknown baseline is warn). - Release planner unified onto the same git baseline: Invoke-CrateSemverCheck uses the previous version-bump commit; -Registry / SEMVER_REGISTRY plumbing removed from release-packages.ps1, release-flow.ps1 and main.yml. The registry helpers (Get-PublishedCrateVersion, ConvertFrom-CargoInfoOutput, Test-CargoInfoCrateMissing, Get-CargoInfoReplacementRegistry) and their tests are removed. - docs/releasing.md: describe the previous-version-bump-commit baseline; the former "Baseline limitation" (4.0.0 vs 3.3.3) now behaves correctly. Validated: full Pester suite passes (316 unit + 24 scenarios + 53 integration) and a real `cargo semver-checks --baseline-rev` smoke run produces verdicts with no registry/`cargo info` calls. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…baseline The release cascade decided each crate's change type from a [package.metadata.cargo_check_external_types] allowlist heuristic to guess whether a dependent exposed a changed dependency. That allowlist drifts from the real public API, so an exposed dependency's breaking change could be mis-cascaded as a patch (AB#7577159). Replace the heuristic with real API diffing: run cargo-semver-checks against each crate's last published crates.io version (the tool's default baseline). Every release in the plan -- user-source roots and cascade-pulled dependents -- is classified from its own working-tree API diff. Dependents are floored at patch (they must re-release to pick up the new dependency) then raised to their own semver verdict. No fallback: cargo-semver-checks is a required tool and a crate that cannot be analysed is a hard error; a crate not yet on crates.io imposes no change-type floor. Also rework the CI semver job: baseline against crates.io (drop --baseline-rev), run only when the PR bumps a crate's [package] version (the publishing set), and keep it non-failing -- it posts/clears an informational sticky PR comment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the inline bash version-diff loop in the semver job with scripts/ci/compute-publishing-set.ps1, which reuses the release library's Get-PackagesWithVersionChanges (the same per-crate version-diff logic the release scripts use, already unit-tested) and maps changed crate folders to their cargo package names. More readable and testable than the awk/bash. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Invoke-ReleasePackagesMain called Exit 1 on validation / pre-flight / execution errors. Exit tears down the entire runspace, so when the Pester scenario suite ran an error case (e.g. a pin below the required version) it killed the whole run -- every subsequent scenario was reported as failed. This was a latent bug masked until the semver-checks change made more error scenarios actually reach the throw path. Surface those failures as terminating errors (throw) instead. The thin CLI shell (release-packages.ps1) now wraps the call and converts a throw into �xit 1, preserving the command-line / CI exit-code contract, while test harnesses catch the exception in-process. Full Pester suite now passes 385/385 in a single process (previously the scenario file cascaded to failures). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The scenario suite mocks the semver classifier (Get-CrateRequiredChangeType) so the real cargo-semver-checks binary is never invoked, but Invoke-ReleasePackagesMain's pre-flight asserts the binary is on PATH. The script-tests CI job does not install cargo-semver-checks (it would be a needless multi-minute compile for tests that mock it), so every scenario threw at pre-flight. Mock Test-CommandExists in the scenario BeforeEach so the presence check passes without the binary, matching how the classifier itself is mocked. Verified by running the scenario suite with cargo-semver-checks removed from PATH (cargo still present): 24/24 pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Update-EntryForRequiredChangeType: encapsulate change-type ranking via
Get-StrongerChangeType instead of reaching across files for
\. Behaviourally identical (both files dot-source into
one scope, so the rank table already resolved — verified by the full suite),
but robust if the libraries are ever loaded in isolation.
- ConvertFrom-SemverChecksOutput: only map crate-absent signals ("not found in
registry" / "no released versions") to 'none'. Drop the generic
"failed to retrieve crate data from registry" match, which also fires on
transient network outages and would silently skip classification, violating
the no-fallback contract. Added a test asserting a transient failure throws.
- Fix stale references to the removed New-SemverChangeTypeClassifier in the
Resolve-ReleaseSet param doc and the test helper comment (production uses
\ / Get-CrateRequiredChangeType).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the single `⚠️ ` code-dump comment with a per-crate report rendered by scripts/ci/semver-report.ps1 (supersedes compute-publishing-set.ps1). For every crate the PR publishes it runs cargo semver-checks against the crates.io baseline and shows a table: Crate | crates.io | This PR | Minimum required | Status with a 🛑 header + collapsible cargo-semver-checks detail when any crate is under-bumped, a ✅ header when all bumps are sufficient, and a link to the Actions run. The minimum-required version is computed from the crate's parsed verdict via the release library's Get-NextVersion. The job posts a NEW comment on every push (via `gh pr comment`) instead of updating one sticky comment, so each run's result is visible in the timeline. Still non-failing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address round-2 PR review feedback:
- Discover the semver-checks baseline with 'cargo info <crate>
--registry <reg>' (run outside the workspace) and pin it with
--baseline-version, instead of relying on cargo-semver-checks'
hard-coded crates.io default. Add an optional -Registry parameter
(default 'crates-io') to release-packages.ps1, the classifier, and the
CI report so forks publishing to a private registry can point the
baseline at it. Split parsing into the pure ConvertFrom-CargoInfoOutput
for unit testing.
- Document the last-published-version baseline limitation (unpublished
intermediate versions) and its manual-pin workaround, with a covering
test.
- Report wording: drop the noisy 'Semver' title prefix, replace
colloquial 'bump' with 'additional version increment', and make the
table column registry-agnostic ('Published').
- docs/releasing.md: explain the concrete defect, add the result->change
-type mapping table, link cargo-semver-checks docs, drop the redundant
hard-dependency paragraph, and fix 'semver-imposed' wording.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The first live run classified every publishing crate as sufficient because
'cargo info --registry crates-io' is rejected in a vanilla environment (cargo
reserves the name 'crates-io' and refuses it as an explicit --registry value).
The error was swallowed, so every crate fell through to 'unpublished' -> no
baseline -> vacuously sufficient (the silent-fallback trap).
Fix Get-PublishedCrateVersion:
- Use cargo's default source (NO --registry flag) for crates.io; pass
--registry only for a genuine private registry name.
- Detect source-replacement dev boxes ('crates-io is replaced with remote
registry X') and retry against the named replacement.
- THROW on an indeterminate lookup instead of returning \, so a
transient/config failure cannot be mistaken for 'unpublished' and silently
skip the semver floor. \ is returned ONLY when cargo info specifically
reports the crate is absent from the registry (a brand-new crate).
The CI report now renders a stop-sign '⚠️ baseline unknown' row (not a false
green) when a lookup fails. New pure helpers Test-CargoInfoCrateMissing and
Get-CargoInfoReplacementRegistry are unit-tested; 399/399 Pester green.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
cargo forces colour even when piped in CI, wrapping the version label in SGR escapes (ESC[1mESC[92mversion:ESC[0m 0.6.0), so the anchored ^version: match missed and every baseline lookup threw. Pass --color never to cargo info and strip ANSI escapes in ConvertFrom-CargoInfoOutput as a belt-and-suspenders. The previous run correctly surfaced this as a stop-sign '⚠️ baseline unknown' row rather than a false green, confirming the throw-on-indeterminate safety net. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A PR that bumps no crate versions (e.g. this feature PR itself) still got a spurious '0 crate(s) - version increments look sufficient' comment on every push. Cause: Get-PackagesWithVersionChanges returns a HashSet via Write-Output -NoEnumerate, and '@(...)' wraps that as a 1-element array containing the (empty) set, so the 'publishing set empty' guard (\.Count -eq 0) never fired and the step emitted publishing=true. Cast the return to [string[]] (which reliably enumerates the set) before sorting, so an empty publishing set collapses to a zero-length array, the early return fires, publishing=false is emitted, and the Post Semver Comment step (gated on publishing == 'true') is skipped. Added two GitFs regression tests exercising the exact call-site expression for the empty and populated cases. 402/402 Pester green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The report headline and step 'status' output were computed from all non-sufficient rows, which lumps a crate whose baseline could not be determined (ChangeType 'unknown') in with genuinely under-incremented crates. That produced a '🛑 Additional version increments required' headline and status=fail even when nothing was actually under-incremented and the check was merely incomplete — contradicting the script's own contract that 'fail' means at least one crate is under-incremented. Separate the two: - fail (🛑) only when a real under-increment is detected; - warn (⚠️ 'Semver baseline could not be determined') when the sole problem is an unresolved baseline; - pass (✅) otherwise. The per-crate table and collapsible detail already flag unknown-baseline rows distinctly; this aligns the headline, the footer note, and the step 'status' output with that distinction. Docstring updated. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…history) Replace the crates.io registry baseline with a git-history baseline for semver-checks breakage analysis, per Sander's review on PR #554. Instead of discovering the last published version via `cargo info` and diffing against `--baseline-version`, the crate's previous version-bump commit (the most recent commit that changed its `[package] version`) is located in git history and passed to cargo-semver-checks as `--baseline-rev <sha>`, which rebuilds the baseline rustdoc from source. Why: versioning is a source-level concern and one workflow must serve both OSS and enterprise/offline consumers. The registry lookup cannot reach crates.io in private environments and its published content lags the source, so a declared-but-unpublished version (e.g. an aborted 4.0.0) was wrongly judged against a stale published 3.3.3. The git baseline is whatever the repo last declared, regardless of publish status, and needs no network access. Changes: - New Get-PreviousVersionBumpCommit helper (scripts/lib/releasing.ps1), unit-tested, returning the bump commit SHA + declared version. It matches only the [package] version (skips dependency-version and publish-only edits) and honours BaseRef so a PR's own bump is excluded. - CI report (scripts/ci/semver-report.ps1) now uses --baseline-rev; drops the -Registry param. Baseline column shows "<version> (<short-sha>)". Graceful warn/fail contract preserved (unknown baseline is warn). - Release planner unified onto the same git baseline: Invoke-CrateSemverCheck uses the previous version-bump commit; -Registry / SEMVER_REGISTRY plumbing removed from release-packages.ps1, release-flow.ps1 and main.yml. The registry helpers (Get-PublishedCrateVersion, ConvertFrom-CargoInfoOutput, Test-CargoInfoCrateMissing, Get-CargoInfoReplacementRegistry) and their tests are removed. - docs/releasing.md: describe the previous-version-bump-commit baseline; the former "Baseline limitation" (4.0.0 vs 3.3.3) now behaves correctly. Validated: full Pester suite passes (316 unit + 24 scenarios + 53 integration) and a real `cargo semver-checks --baseline-rev` smoke run produces verdicts with no registry/`cargo info` calls. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add a "Baseline commit" column to the semver report table showing the exact commit each crate was compared against (the --baseline-rev SHA). The SHA is rendered as a short hash linking to the commit when the repo can be derived from the Actions run URL, and as "—" for new crates and failed baseline lookups (no commit). The Baseline column now carries the version only, so the commit is no longer buried in parentheses. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…eport Address review feedback on PR #554: - semver-report.ps1: a crate's sufficiency was derived from the cargo-semver-checks verdict alone ('patch'/'none' => sufficient), which ignored the on-disk bump. A correctly-bumped crate (e.g. baseline 1.0.0 -> on-disk 1.1.0 with a 'non-breaking' verdict) was wrongly flagged as under-incremented. Now compute the minimum acceptable version (baseline incremented by the required change type) and mark the crate sufficient when its on-disk version is >= that minimum, via Compare-SemanticVersions. 'none' (no baseline) stays unconstrained. Verified across 1.x and 0.x semver rules. - main.yml: the Semver Report step comment claimed the step emits status = pass|fail, but the script documents and emits pass|warn|fail (warn = a baseline could not be determined). Comment corrected to match the actual output contract. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… parser Address review feedback on PR #554: with the git-history baseline (--baseline-rev), cargo-semver-checks always builds the baseline from source, so it never emits registry "not found / no released versions" messages. Those branches in ConvertFrom-SemverChecksOutput were dead code and the accompanying comment described a registry baseline that no longer applies. Remove the branches, and document that the 'none' (no baseline) case is decided by the caller before invoking the parser when there is no previous version-bump commit. Update the unit tests accordingly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address review feedback on PR #554: the Semver Report step's git fetch refspec escaped the ':' separator with a backtick and used unbraced $env:BASE_REF, which is fragile and easy to misread. Switch to the explicit ${env:BASE_REF} form (and brace the other env references in the step) so the refspec is unambiguous with no backtick escaping. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
18820f5 to
e65a632
Compare
Address review feedback on PR #554: Get-PreviousVersionBumpCommit ran `git log` with -AllowFailure, so a genuine failure (an unresolvable BaseRef that was never fetched or is a typo, repo corruption) returned $null and was silently treated as "no previous version-bump commit" => 'none' (no change-type floor). That could misclassify a release and make the CI semver report incorrectly pass when the baseline could not actually be determined. Now guard with Test-GitRef (throws a clear error when the ref cannot be resolved) and run `git log` without -AllowFailure so any other git error also propagates. A valid ref that simply matches no version-bump commit still exits 0 with empty output and correctly yields $null (brand-new crate). The CI report already catches the throw and records an unknown (⚠️ warn) row; the release planner treats it as a hard error. Adds a unit test for the unresolvable-ref case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The release cascade decided each crate's change type from a [package.metadata.cargo_check_external_types] allowlist heuristic to guess whether a dependent exposed a changed dependency. That allowlist drifts from the real public API, so an exposed dependency's breaking change could be mis-cascaded as a patch (AB#7577159).
Replace the heuristic with real API diffing: run cargo-semver-checks against each crate's previous version-bump commit in git history — the most recent commit that changed the crate's [package] version, supplied as --baseline-rev so the baseline rustdoc is rebuilt from that commit's source. Every release in the plan — user-source roots and cascade-pulled dependents — is classified from its own working-tree API diff. Dependents are floored at patch then raised to their own semver verdict. No fallback: cargo-semver-checks is required and an unanalysable crate is a hard error; a crate with no prior version-bump commit imposes no floor.
The baseline is source-level, not registry-based: whatever the repo last declared, regardless of publish status — no crates.io access, identical for OSS and enterprise/offline. A version committed but never published (an aborted 4.0.0) is the baseline, not a stale published 3.3.3.
Also rework the CI semver job: baseline each publishing crate against its previous version-bump commit (--baseline-rev, no registry), run only on the publishing set, non-failing — posts a fresh per-crate table (baseline + resolved commit vs this-PR vs minimum required). Outputs: publishing=true|false, status=pass|warn|fail.
Example of a CI check: #565 (comment)