Repository navigation
fix(release): cascade exposed dependency breaks - #646
Kateřina Churanová (kate-shine) merged 21 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the release planner’s cascade logic by combining cargo-semver-checks results with exposed external-type metadata so that breaking dependency bumps propagate to dependents whose public APIs expose those dependency types (including through chains) and missing/wildcard metadata is handled conservatively.
Changes:
- Extend workspace package metadata to include
allowed_external_typesand add a conservative exposure check helper. - Add a fixpoint-based “exposed dependency” cascade pass to strengthen dependents to
breakingwhen they expose an incompatibly bumped dependency. - Add unit + scenario regression coverage for the
bytesbuf→bytesbuf_iofailure mode and related edge cases.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| scripts/tests/Pester/unit/release-packages/ResolveReleaseSet.Tests.ps1 | Updates baseline stubs to carry external-type metadata and adds focused unit tests for the new exposure-driven cascade behavior. |
| scripts/tests/Pester/scenarios/S27-proc-macro-recursive-review.scenario.psd1 | Adds AllowedExternalTypes to keep scenario expectations stable under the new conservative exposure rules. |
| scripts/tests/Pester/scenarios/S15-auto-upgrade-of-user-source.scenario.psd1 | Shifts the scenario to rely on exposure metadata rather than an explicit semver verdict override. |
| scripts/release-packages.ps1 | Updates release-planner documentation to describe the new exposed-dependency cascade behavior and conservative handling. |
| scripts/lib/releasing.ps1 | Plumbs AllowedExternalTypes from cargo metadata and introduces Test-PackageExposesTarget for exposure checks. |
| scripts/lib/release-flow.ps1 | Implements the fixpoint cascade pass that upgrades dependents to breaking when they expose breaking dependency version transitions. |
| docs/releasing.md | Updates the release documentation to explain why exposed dependency version transitions require an additional cascade beyond rustdoc comparison. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #646 +/- ##
=======================================
Coverage 100.0% 100.0%
=======================================
Files 503 503
Lines 57407 57407
=======================================
Hits 57407 57407 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
GHCP: Fixed in d42c6cb, though the diagnosis needed correcting — the conclusion was right for the wrong reason. The comment predicts The actual defect is quieter and worse. A malformed entry collapses to So the recommended behaviour ("treat malformed entries as possible exposure") is exactly right and is what landed: any entry that is not a usable non-empty string now returns Added 8 unit tests for Full suite: 431 passed, 0 failed. Separately, on the red checks: all six are GitHub Actions infrastructure, not this change. Five ( |
6abc1d1 to
6e4ed2e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
scripts/lib/release-flow.ps1:374
Depscontains both normal and build dependencies (scripts/lib/releasing.ps1:591-595), so this predicate treats a build-only edge as a possible public-API exposure. With missing or wildcard metadata, a breaking build-dependency bump therefore forces the dependent tobreaking, even though build-dependency types cannot appear in that crate's Rust API. Preserve dependency kind in the workspace model and apply this exposure cascade only to normal dependency edges.
return @($WorkspaceBaseline | Where-Object {
$_.Published -and
-not $_.IsProcMacroOnly -and
$_.Deps -contains $targetCargoName -and
$Resolved.Contains($_.Folder) -and
(Test-PackageExposesTarget -Dependent $_ -TargetPackageName $TargetPackage.Name)
scripts/lib/release-flow.ps1:646
- The new
-Forcebehavior that bases propagation on the pinned version rather than the upgraded severity tag is not covered by the added tests. Add an exposed A → B → C chain where B is explicitly pinned to a non-breaking version with-Force; assert B retains the pin and breaking tag while C remains at its mechanical patch floor. This protects the control path described here from accidentally reverting to tag-based propagation.
# cargo-semver-checks compares one crate's rustdoc API and cannot identify
# that an unchanged signature now names a type from an incompatible version
# of an external crate. Propagate that condition over direct dependency edges
# until no dependent is strengthened. Use the version that will actually be
# written, not EffectiveChangeType, so -Force pins do not create a fictitious
# breaking transition farther up the graph.
24bb1f7 to
895ed21
Compare
|
GHCP: Responses to the two suppressed comments on this review.
This pins
The real fix is per-edge Full suite: 441/441. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
scripts/lib/releasing.ps1:614
- The critical rename path is not exercised from Cargo metadata: the unit tests construct
DepAliasesdirectly (already normalized), and the live-workspace test only covers an unrenamed edge. A regression in readingdependency.renameor convertingbytes-buftobytes_bufwould therefore restore the fail-open behavior while all added tests still pass. Add a synthetic workspace withalias = { package = "dependency", ... }, load it throughGet-WorkspacePackages, and assert that the alias-rooted allowlist drives the cascade.
$renameProp = $dep.PSObject.Properties['rename']
if ($renameProp -and -not [string]::IsNullOrWhiteSpace($renameProp.Value)) {
$alias = ([string]$renameProp.Value).Replace('-', '_')
# A package may be depended on more than once under different
# aliases (per-target or per-feature), so collect them all.
$depAliases[$depCargoName] = @(@($depAliases[$depCargoName]) + $alias |
Where-Object { $_ } | Sort-Object -Unique)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (3)
scripts/lib/release-flow.ps1:500
- The planner pass is no longer direct-edge-only: lines 681-685 and
Get-PublishedDependentsExposingTargetexplicitly include indirect defining-crate edges. Keeping the function contract phrased this way makes future maintenance likely to drop the indirect behavior that the new regression tests protect.
# floored at patch. A second direct-edge pass raises a dependent to breaking
# when it exposes a dependency whose planned version transition is
# incompatible. The pass repeats to a fixpoint so exposure chains cascade.
scripts/release-packages.ps1:68
- This help text now understates the implemented cascade:
Get-PublishedDependentsExposingTargetalso handles reachable indirect dependents whose allowlist names the type's defining crate. Describing only direct edges leaves users unaware of releases such astypespec_client_core -> ... -> fetch_azure; document both direct fail-closed edges and positively evidenced indirect edges.
Exposing a dependency whose planned version transition is breaking floors
the dependent at `breaking`, recursively through direct dependency edges.
docs/releasing.md:238
- This description is stale after adding defining-crate exposure edges: the implementation does not consult only direct edges. It also selects reachable indirect dependents when their allowlist explicitly names the defining crate, with different missing/malformed-metadata semantics. Please document that distinction so the release policy matches the planner.
each direct dependency edge, the planner therefore also consults
`[package.metadata.cargo_check_external_types].allowed_external_types`.
If the dependency's planned version transition is breaking and the
dependent allows that dependency's types in its public API, the dependent
is floored at `breaking`. The planner repeats this check to a fixpoint so
the result propagates through chains such as `bytesbuf` → `bytesbuf_io` →
another facade.
|
GHCP: All three fixed in 7cc3fc0 — correct on every count. 7865720 changed the behaviour and left three descriptions behind, which is the same mistake wukchung caught earlier in this PR, so this time I swept the rest of the scripts and docs rather than only fixing what was reported. The remaining "direct dependency/dependent" references cover the manual review chain and changelog reason attribution, which genuinely are direct-only and are unaffected. Taking the
Comments and docs only, no behaviour change; unit suite 380/380. |
The exposure cascade loop nested five levels deep and mixed four concerns: deciding whether a source plans a breaking release, selecting exposed dependents, applying the requirement, and recording the reason. Extract Test-EntryPlansBreakingRelease, Get-ExposedDependentPackages, Update-EntryForExposedDependency and Set-CascadeReason so the loop reads as its algorithm. Set-CascadeReason also removes a verbatim duplicate of the reason-dedup loop that existed in both the BFS pull-in and the exposure fixpoint. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…iases A dependency declared with `package = "..."` is nameable in Rust source -- and therefore in an allowed_external_types entry -- only under its alias. Test-PackageExposesTarget compared allowlist roots against the real package name only, so an aliased entry matched nothing and the crate was reported as not exposing the target: a fail-open that lets a breaking dependency bump ship as a compatible release. This is the same failure class the exposure cascade exists to prevent. Get-WorkspacePackages now records a DepAliases map alongside Deps, and the exposure test accepts the real name or any alias. DepAliases is additive, so the 84 existing Deps references are untouched and package records that predate the field still work. Also adds the missing coverage for -Force pin propagation through the cascade: Test-EntryPlansBreakingRelease deliberately derives from EffectiveTargetVersion rather than EffectiveChangeType so a pin honored below the required version does not invent a break farther up the graph. A mutation check confirms the new test is the only one that catches that regression. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…real manifests Every existing test of the exposed-dependency cascade builds its own synthetic package records, so the planner is only ever proven correct about graphs the tests themselves invent. The production instance the cascade exists for was never asserted. bytesbuf_io is "asynchronous I/O abstractions expressed via bytesbuf types": its public API returns BytesBuf and BytesView directly and its manifest declares `bytesbuf::*` in allowed_external_types. A breaking bytesbuf release is therefore a breaking bytesbuf_io release. Before the cascade, bytesbuf_io took a mechanical patch floor and shipped a silent break to consumers. Adds two layers: - Unit: pins bytesbuf_io's allowlist shape against Test-PackageExposesTarget, including the negative cases -- trait-variant is a dependency but is deliberately not allowlisted, and 'bytesbuf' being a strict prefix of 'bytesbuf_io' must not cross-match either way. - End-to-end: resolves a release set over the live workspace baseline and asserts bytesbuf@breaking raises bytesbuf_io to breaking with a recorded cascade reason, plus a negative control that bytesbuf@patch does not. The e2e test reads real manifests on purpose. Deleting `bytesbuf::*` from bytesbuf_io's allowlist is exactly how the fail-open would be reintroduced, and that must fail the suite rather than pass silently against a snapshot. Mutation-checked: reverting Test-PackageExposesTarget to the original fail-open fails 4 of the 9 end-to-end assertions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… analysis The comment above Invoke-CrateSemverCheck stated that working-tree analysis is "what lets an exposed-dependency breaking change cascade correctly, without the old allowed_external_types heuristic". That claim is the reason this bug existed. It was introduced by f78524e (microsoft#554), which removed the exposure heuristic on those grounds. Working-tree analysis is necessary but not sufficient. When a dependency's version bump is incompatible without its type shapes changing, the dependent's rustdoc is identical on both sides and semver-checks correctly reports no required bump -- but the release is still breaking, because type identity in Rust is per-version. No rustdoc diff can surface that. Replaces the comment with the actual division of responsibility: semver-checks supplies each crate's own floor, the exposure cascade supplies the floor its dependencies impose on it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two follow-ups from review, both about claims and coverage rather than behaviour: no production logic changes here. Correct the two remaining copies of the claim that semver-checks subsumes exposure analysis. f57656a fixed the comment above Invoke-CrateSemverCheck but missed its siblings: - docs/releasing.md said the working-tree API diff reflects "a dependency whose public types a dependent re-exports". It does not. A dependency version bump changes type identity without changing any rustdoc, which is the entire reason the exposed-dependency cascade exists -- and that cascade is described ten lines further down the same file, so the document contradicted itself. - release-flow.ps1 said the classifier decides "every change type in the plan (user-source and cascade) ... no allowed_external_types heuristic". Both halves are now false: it decides each crate's own floor, and the cascade can raise an entry above it. Cover the rename path end-to-end. The existing unit tests construct DepAliases by hand, already normalized, so they pin the matching logic but not the extraction that feeds it. A regression in reading `dependency.rename`, or in the hyphen-to-underscore conversion, would have restored the original fail-open with every test still green. New integration tests build a real manifest using the production form (`aliased-dep = { package = "dependency", ... }`, as multitude declares allocator-api2-02), load it through cargo metadata, and assert both the extraction and the resulting cascade. Verified by mutation: misreading the `rename` property fails 3 of the 6 tests, and the 3 that stay green are the negative controls, which is the correct split. New-SyntheticWorkspace gains a `Rename` field on dep specs. Renamed deps are emitted inline rather than via workspace inheritance because [workspace.dependencies] is keyed by real package name and cannot express an alias without rewriting that table. Full integration and scenario suites: 100/100. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
cargo-check-external-types attributes a re-exported type to the crate
that DEFINES it, not the crate you route it through. fetch_azure
documents this in its own manifest:
# azure_core re-exports its HttpClient trait from this crate;
# cargo-check-external-types reports re-exports by their defining crate.
"typespec_client_core::*",
So for a chain a -> b -> c where c reaches a::T through b, c allowlists
`a` while depending only on `b`. The cascade required a direct
dependency edge, so neither pass could see it: processing target `a`
skipped c (not a direct dependent), and processing target `b` found c
but looked for allowlist root `b`, which c does not name. c stayed on
its patch floor and a breaking bump of `a` shipped as compatible.
Three such edges exist in the workspace today:
fetch -> recoverable (via fetch_hyper/http_extensions/
seatbelt/seatbelt_http)
seatbelt_http -> recoverable (via http_extensions/seatbelt)
templated_uri -> data_privacy_core (via data_privacy)
None of them misfires right now, but only by coincidence: each crate
also allowlists an intermediate on the path, so the direct-edge chain
happens to reach it anyway. That is masking, not safety. Deleting one
now-unused allowlist entry from templated_uri would silently stop it
cascading from data_privacy_core. The restriction is inherited from the
pre-microsoft#554 design rather than introduced here.
Dependent selection now also accepts an indirect edge: a published
transitive dependent whose allowlist explicitly names the target.
The indirect edge deliberately does NOT reuse Test-PackageExposesTarget.
That predicate fails closed on absent or malformed metadata, which is
right for a direct dependency, where an unknown must not ship a break as
compatible. Applied to transitive dependents it would match every crate
in the graph that declares no allowlist and force it breaking, including
crates whose intermediate positively claims to expose nothing. So the
indirect edge uses Test-PackageAllowlistNamesTarget, which requires
positive evidence. Nothing is lost: a crate with no allowlist that truly
does expose the target still fails closed on its own direct edge to
whichever intermediate carries the type, and the fixpoint walks that up.
A wildcard root stays a match in both, since it can expand to the target
and is a deliberate declaration rather than missing information.
Reachability is memoised per resolve. The baseline is fixed for the
duration, so the BFS answer is stable, and the fixpoint would otherwise
recompute it for every breaking source on every pass.
Tests: 12 unit tests for the new predicate (including every case where
it diverges from the fail-closed one), 10 cascade tests covering the
re-export topology, rename aliases, unpublished conduits, proc-macro
exclusion and four over-cascade guards, and 4 tests pinning the three
real edges above against live manifests.
Verified by mutation, both directions:
* disabling the indirect edge (pre-fix behaviour) fails 5 tests
* using the fail-closed predicate on indirect edges -- the obvious
wrong fix -- fails the over-cascade guard
Full suite: 484/484.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…is documented 7865720 added indirect defining-crate edges to the exposure cascade but left three descriptions asserting the pass is direct-edge-only: - the Resolve-ReleaseSet contract header in release-flow.ps1 - the release-packages.ps1 help text - the exposed-dependency cascade section of docs/releasing.md Each now covers both edge kinds. The docs section also spells out the part most likely to look like an inconsistency to a future reader: the two edges treat missing evidence differently on purpose. A direct edge fails closed on absent, malformed or wildcard metadata; an indirect edge demands an explicit allowlist entry, because failing closed there would match every transitive dependency of every crate that declares no allowlist. Nothing is missed either way -- the direct edge to whichever intermediate carries the type still fails closed, and the fixpoint propagates that upward. Uses fetch_azure as the worked example, since it documents the mechanism in its own manifest: it allowlists `typespec_client_core::*` for a trait azure_core re-exports, while depending only on azure_core. Verified against the manifest rather than quoted from memory. Swept the rest of both scripts and docs for the same stale claim. The remaining "direct dependency/dependent" references describe the manual review chain and changelog reason attribution, which really are direct-only and are unaffected by this PR. Comments and docs only; no behaviour change. Unit suite: 380/380. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A dependency is nameable in Rust source -- and so in an
allowed_external_types entry -- by its crate root, which is not always its
package name. `Get-WorkspacePackages` recorded only the `package = "..."`
alias, missing the other construct that diverts it: `[lib] name = "..."` in
the dependency's own manifest, which renames the crate root for every
consumer.
A crate that allowlisted the lib name therefore matched nothing, and the
exposure test reported "not exposed" -- a fail-open that ships a breaking
dependency bump as a compatible release. Map each workspace package to its
crate root and record it alongside the rename, with the rename winning when
both are present: `foo = { package = "bar" }` makes the crate nameable as
`foo` whatever bar calls its lib target.
No workspace crate has such a mismatch today (53 packages, 0 divergent lib
names), so this is latent rather than live -- the same shape as the
build-dependency gap, and fixed for the same reason: the fail-open only
becomes visible once it has already shipped a silent break.
Also stop `New-BaselinePackage` from defaulting `AllowedExternalTypes` to
`$null`. `$null` is the fail-closed branch (absent metadata => assume
exposure), so defaulting to it made every package in every baseline expose
every dependency for free, satisfying cascade assertions on the fallback
rather than on the behaviour under test. Defaulting to the inert `@()`
means a test that needs exposure has to ask for it; the one test that is
actually about absent metadata now passes `$null` explicitly.
Measured with the stub classifier neutered to return 'none' for everything:
4 tests failed before, 11 after -- 7 assertions were being satisfied by the
default alone.
Both production changes are mutation-verified: suppressing the lib-name
alias fails 3 of the new tests, and dropping the rename precedence fails the
test that pins it.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…e does Three comments claimed more than the code delivered. The exposed-dependency pin diagnostic said the release required a bump "because of the incompatible planned version of 'X' exposed in its public API". Read naturally, "its" attaches to X, making the sentence say the dependency exposes itself. The crate being pinned is the one whose public API names the types, so say that. `Set-CascadeReason` documented its collection as one-per-edge, keyed on a (target -> dependent) edge. It is not: reasons arrive both from the transitive membership walk, where the target can sit several crates away behind unpublished conduits, and from exposure, where re-exported types are attributed to their defining crate. Neither implies a direct dependency, and the previous commit's indirect exposure edges widened the gap further. Broaden the contract to what it actually is -- package-level release causes keyed by target name -- and note that changelog attribution needs the narrower direct-dependency relation and must take it from the dependency graph instead. A unit test asserted `Test-PackageExposesTarget` against bytesbuf_io's allowlist copied verbatim from its manifest, claiming the integration test "asserts the real manifest still matches it, so this stays honest". The integration test only checked that a bytesbuf-rooted entry existed, leaving `ohno` and `futures_core` -- both asserted by the unit tests -- pinned by nothing. Rather than weaken the comment, make it true: move the literal to _common as the single source both files read, and assert set equality against the live manifest. Order-insensitive, so it pins the contents rather than the manifest's formatting. Verified by mutation: dropping futures_core from the shared literal fails the new assertion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The previous commit taught the exposure check about `[lib] name` by recording it in DepAliases. That fixed direct edges and could not fix anything else: DepAliases is keyed by edge, populated from a package's own declared dependencies. A crate that reaches a type re-exported from several hops away declares no edge to the crate that defines it, so it has no entry to look up -- and the indirect branch fell back to matching the package name, found nothing, and reported "not exposed". That is the fail-open the indirect branch exists to close. The name is a property of the target, not of any edge. cargo-check-external- types attributes a re-exported type to its defining crate and writes it under that crate's rustdoc name, which is its `[lib] name`. A `package = "..."` rename cannot apply here at all, since only a crate that declares the dependency can rename it. So the accepted root for an indirect match has to come from the target's own package record: Get-WorkspacePackages now carries CrateRoot, and the two exposure predicates accept it. Verified on a real synthetic workspace, `defining ([lib] name = "def_core") -> relay -> facade` with facade allowlisting def_core::Handle: before, facade stayed at patch 1.0.1 while naming def_core types -- a silent break shipped as compatible; after, it reaches breaking 2.0.0. Also rewrites the unit test that claimed to cover this. It injected a DepAliases entry onto a facade with no edge to the target, a state Get-WorkspacePackages can never produce, so it passed without exercising the mechanism. It now models the producible one: the target's crate root. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The previous commit threaded the target's CrateRoot into both exposure
predicates. That was right for the indirect one and wrong for the direct
one, where it quietly undid the rename precedence rule two commits back
had established.
On a declared edge DepAliases is not merely available, it is
authoritative: a `package = "..."` rename shadows the dependency's `[lib]
name` completely. A dependent importing the crate as `aliased_dep` cannot
write `dep_core::Handle` whatever the target's manifest says, so such an
entry must belong to some other crate. Adding the target's global root
back on that edge re-accepted a name the dependent provably cannot use,
turning an unrelated allowlist entry that happens to collide with it into
a false exposure.
Measured on a synthetic workspace -- dependent renames `dependency` to
`aliased-dep`, dependency sets `[lib] name = "dep_core"`, dependent
allowlists `dep_core::Handle`: accepted roots grew from
{aliased_dep, dependency} to {aliased_dep, dep_core, dependency} and the
dependent was dragged to breaking 2.0.0 instead of holding at patch
1.0.1. Not a fail-open -- a spurious bump -- but it defeats a rule with
its own test.
So the parameter is removed from Test-PackageExposesTarget outright
rather than merely left unpassed at the call site: the direct predicate
has no business accepting it, and deleting it makes that unstatable.
The existing precedence test missed this because it asserted what
DepAliases contains, never what the predicate does with it. The new test
pins the behaviour, end to end through the cascade -- which is where the
regression actually showed, since the unit-level call never passed the
argument.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Both are statements added by this branch that do not match its own code. docs/releasing.md described an indirect edge as matchable through a `package = "..."` alias. It cannot be: only a crate that declares a dependency can rename it, and an indirect dependent declares no edge to the target. The implementation uses the target's own crate root precisely because no alias is available there, so the prose contradicted the code it documents. The comment on Get-AcceptedExposureRoots called DepAliases "the complete and authoritative answer" for a declared edge while the function goes on to accept the real package name unconditionally. The narrow claim is true -- DepAliases wins over the target's *global* crate root, which is why the previous commit stopped passing it -- but as written it also implied the package name is filtered, which it is not. That over-acceptance is real: a rename shadows the package name exactly as it shadows the lib name, so `dependency::*` is unwritable on an edge reachable only as `aliased_dep`, and accepting it can force a spurious breaking bump. It is left in place deliberately, because removing it needs data the record does not carry -- whether an unrenamed edge to the target also exists, a package being dependable twice, once aliased and once not. Dropping the name without that would fail open on the ordinary unrenamed edge, which is the worse direction. The comment now says so rather than implying the case is already handled. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address the eight findings from Sander's review agent as one accuracy pass. Seven were contracts, examples, or explanations that no longer matched the implementation after the indirect-exposure and forced-pin changes. The eighth was an impossible helper state: the indirect predicate accepted DepAliases even though production can populate an alias only while processing a declared edge, which always takes the direct branch. Make the indirect predicate accept only roots that can exist without an edge: the target package name and its own CrateRoot. A focused negative test rejects an injected alias, while the divergent-CrateRoot test pins the real alternate-name path. Mutation-restoring the shared helper makes exactly that negative test fail. Correct the remaining contracts to state that: - forced-pin propagation follows the actual numeric version transition; EffectiveChangeType records the stronger unmet requirement only; - indirect wildcard roots are positive evidence; - DepAliases stores observed additional roots, not an exclusive root set; - CascadeReasons are package-level release causes, not graph edges; and - the relay fixture exercises direct exposure before facade exercises the indirect defining-crate path. Also fix the Cargo rename example and update PR flowchart step G to show both direct fail-closed and indirect positive-evidence paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address the three suppressed findings from the latest Copilot review. The direct predicate checked that an allowlist entry was a non-empty string but did not validate the root extracted from it. An entry such as `::Bytes` therefore fell through to "not exposed", violating the direct edge's fail-closed contract. Treat an empty or whitespace root as malformed evidence and return exposure. The indirect predicate also retained the package name alongside a known CrateRoot. A divergent `[lib] name` replaces the package name as the usable Rust root, so accepting both could let an unrelated `dependency::*` entry spuriously match a target whose only root is `dep_core`. Use CrateRoot exclusively when supplied and retain the package name only as compatibility for records that predate the field. Both fixes have focused regression tests and were mutation-verified independently: removing the empty-root guard fails only its test, and restoring the package-name union fails only the divergent-root negative. Also correct the duplicated word in the live-workspace test name. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Get-AcceptedExposureRoots kept an optional -TargetCrateRoot after its only caller stopped passing one. Test-PackageExposesTarget is now its sole caller and deliberately withholds the target's global crate root, so the parameter was unreachable surface that only re-offered the mistake it was removed to prevent. Drop it and fold the rationale into the function contract: on a declared edge DepAliases is the whole story, and the undeclared-edge case belongs to Test-PackageAllowlistNamesTarget, which builds roots from the target's own record. No behaviour change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A -Force pin honored below the cascade-required version wrote a numerically compatible version over a genuinely incompatible API, and Test-EntryPlansBreakingRelease judged the entry by that number alone -- so the cascade stopped at exactly the crate whose break was suppressed. The break is real. A crate pinned to 1.1.0 while exposing a dependency that went 2.0.0 is still compiled against 2.0.0, so its public API names different types than 1.0.0 did, and a consumer of \1.0\ upgrades into it silently under caret semantics. That is the SemVer break this cascade exists to prevent, reintroduced by the override meant only to relabel it. Widen the gate with PinHonoredAgainstCascade, which is set only on the -Force undershoot path. The version-derived signal is kept as primary so 0.x semantics stay correct, and the pin is still honored verbatim -- only the propagation decision changes. Proc-macro sources cannot reach this gate; the fixpoint already skips them. Correct the four contracts that documented the old behaviour, invert the test that encoded its rationale, and add a boundary test that a compatible release which suppressed nothing still does not propagate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PinHonoredAgainstCascade is set whenever -Force honors a pin below ANY stronger requirement, not only a breaking one. Treating the bare flag as proof of a break made a crate pinned from 1.0.0 to 1.0.1 over a merely additive non-breaking requirement drag every exposing dependent to a major release. Gate the override on the suppressed requirement actually being breaking, testing it with the same Test-IsBreakingChange already used for the planned transition rather than comparing against the literal 'breaking'. Routing both branches through one predicate keeps 0.x semantics consistent between them. Add the forced non-breaking regression case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tics The exposure fixpoint's own header still said to classify sources by the written version rather than EffectiveChangeType, so -Force pins would not create a fictitious break farther up the graph. That is now the opposite of what Test-EntryPlansBreakingRelease does, and it was the fifth copy of a claim the previous two commits corrected elsewhere. Describe what the predicate actually asks -- does this entry ship an incompatible API -- and name both signals that answer it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bb6f05f to
f7e228d
Compare
ddacce5
into
microsoft:main
Closes #665. Follow-up to #646, which the two findings below came out of. Both are fail-opens in the exposure cascade: cases where the planner reports "not exposed" and leaves a crate on the patch floor while its public API really does carry the bumped types. Neither is reachable in the workspace as it stands today, which is why #646 was merged ahead of them — see the issue for the reachability analysis. ## 1. Judge both exposure paths when a crate holds each `Get-PublishedDependentsExposingTarget` picked its exposure predicate with an `if`/`else` on whether a direct dependency edge existed, making the direct and indirect tests mutually exclusive. A crate holding both paths was judged only on the direct one. That is a fail-open, because **the two predicates accept different allowlist roots by design**: | | roots built from | treats the target's own crate root | |---|---|---| | direct | the edge's `DepAliases` (rename-aware) | **rejects** it — a `package = "..."` rename shadows it | | indirect | the target's own record | accepts it | So a crate that imports the target under a rename *and* also reaches it through a conduit re-exporting its types loses the root its indirect path legitimately earned. Both edges are now evaluated independently, and the crate is raised if either claims exposure. The load-bearing subtlety: the indirect test asks whether a path reaches the target **through some other package**, not whether the crate is merely reachable from it. Plain reachability would count the direct edge itself and readmit exactly the renamed root the direct predicate just rejected. ## 2. Fail closed on a malformed `allowed_external_types` container `@($allowed.Value)` promoted a malformed scalar into a valid-looking one-entry allowlist that matches nothing, yielding "not exposed". `[package.metadata]` is arbitrary TOML that cargo passes through unvalidated, so this is reachable by a typo. This was the **only** malformed shape that failed open — a non-string container already returned `True`, as did absent metadata. The value is now left `$null`, reusing the existing absent-metadata branch, which fails closed. Two shapes to be careful with: - a string is itself `IEnumerable` (over its chars), so it must be excluded explicitly; - `@()` must survive as a positive claim of "no external types" — collapsing it to `$null` would be a different fail-open. ## Verification Every change was mutation-verified in both directions — a guard test that does not exercise the condition it guards is worthless: - reverting the `if`/`else` fails **exactly** the new mixed-path test; - swapping path-independence for plain reachability fails **exactly** the control test, whose only path to the target is the renamed direct edge; - restoring `@($allowed.Value)` fails **exactly** the scalar and table tests, while the array and empty-array tests keep passing, proving no over-rejection. Suites: `unit\release-packages` **87/87**, `unit\releasing` **67/67**, `integration\ExposureCascade-RealWorkspace` **14/14** (re-run after the rebase, since it reads live manifests and `main` has moved). Also included: contract corrections on `Test-PackageAllowlistNamesTarget`, `Get-AcceptedExposureRoots` and the indirect-edge section of `docs/releasing.md`, all of which described the old mutually-exclusive behaviour. The `releasing.ps1` part of the first commit is comment-only. --------- Co-authored-by: Kateřina Churanová <katerina.churanova@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Release planning could ship a breaking API change inside a semver-compatible version.
cargo-semver-checksanalyses each crate in isolation. But a crate's public API is not only what it declares — it is also every foreign type it re-exports. Whenbytesbufbreaks,bytesbuf_iobreaks too, becausebytesbuf_io's API isbytesbuftypes:bytesbuf_ionever changed, so semver-checks saw nothing, and the planner gave it the mechanical patch floor:bytesbufbytesbuf_ioThat
0.7.1is the bug: a patch release that breaks every downstream build, delivered automatically bycargo update.This PR teaches the planner to combine semver-checks with the
cargo_check_external_typesallowlists each crate already declares, and to propagate incompatibility across public-API exposure edges to a fixpoint.Work item: https://o365exchange.visualstudio.com/O365%20Core/_workitems/edit/7705926
What the cascade actually does
Running
release-packages bytesbuf@breakingagainst today's workspace pulls in 9 crates, all breaking. Solid arrows are exposure edges; dashed arrows are dependencies that do not surface the type in their public API:graph LR bytesbuf["bytesbuf 0.7.0 → 0.8.0"] bytesbuf_io["bytesbuf_io 0.7.0 → 0.8.0"] cachet["cachet 0.10.0 → 0.11.0"] multitude["multitude 0.7.1 → 0.8.0"] http_extensions["http_extensions 0.8.0 → 0.9.0"] seatbelt_http["seatbelt_http 0.6.0 → 0.7.0"] fetch["fetch 0.14.0 → 0.15.0"] fetch_hyper["fetch_hyper 0.5.0 → 0.6.0"] fetch_azure["fetch_azure 0.4.0 → 0.5.0"] bytesbuf --> bytesbuf_io bytesbuf --> cachet bytesbuf --> multitude bytesbuf --> http_extensions bytesbuf --> fetch bytesbuf -.-> fetch_hyper bytesbuf -.-> fetch_azure http_extensions --> seatbelt_http http_extensions --> fetch_hyper http_extensions --> fetch seatbelt_http --> fetch fetch_hyper -.-> fetch fetch --> fetch_azureWhy this needs a fixpoint, not a single pass
Look at
fetch_hyper. It depends onbytesbufbut does not expose it — a single pass overbytesbuf's dependents would correctly leave it alone. It becomes breaking only on a later pass, throughhttp_extensions:graph LR A["bytesbuf — breaking"] -->|exposed| B["http_extensions — raised on pass 1"] B -->|exposed| C["fetch_hyper — raised on pass 2"] A -.->|"not exposed, no direct effect"| Cfetch_azurereaches breaking the same way, viafetch. So the cascade iterates until a pass strengthens nothing.Each pass only ever raises, through a single choke point (
Update-EntryForRequiredChangeType), which is what guarantees termination.Planner flow
flowchart TD A["Parse tokens, e.g. bytesbuf@breaking"] --> B["requestedFolders"] B --> C["Self-floor: cargo-semver-checks per requested crate"] C --> D["BFS: every transitive published dependent joins at a patch floor"] D --> E{"Exposure fixpoint"} E --> F["Select entries whose planned version is an incompatible transition"] F --> G["Find selected published dependents: declared edges that may expose them, or reachable indirect crates that positively allowlist the defining crate"] G --> H["Raise to breaking, record cascade reason"] H -->|"something changed"| E E -->|"nothing changed"| I["Manual-review queue for proc-macro crates"]Fail-closed policy
Exposure is decided from
[package.metadata.cargo_check_external_types]. The guiding rule: an unknown must never permit a break to ship as compatible.allowed_external_types$null)[](empty)bytesbuf::*vsbytesbufpackage = "..."renames — see below*,?,[)Note the asymmetry between absent and empty — that distinction is the one thing that makes the condition look inverted at a glance, so it is documented at the call site.
Two of these were fixed during review:
d42c6cb1) —-splitcoerces any operand to a string, so a non-string entry silently collapsed to''and matched nothing. A typo'd allowlist therefore read as "not exposed".7373a040) — a dependency declaredpackage = "..."is nameable in Rust only under its alias, so its allowlist entry carries the alias while the planner compared against the real package name. The edge was detected, but the exposure test missed, and the break shipped tagged compatible.Get-WorkspacePackagesnow records aDepAliasesmap and the exposure test accepts either name.-Forcesemantics-Forcewrites the number you asked for and warns you. It does not assert that the incompatibility is absent, so it does not stop the cascade.A pin honoured below a required breaking version writes a numerically compatible version over an API that is genuinely incompatible:
b@1.1.0is still compiled againsta@2.0.0and still exposesa::*, so its public API names different types thanb@1.0.0did — and a consumer ofb = "1.0"upgrades into it silently under caret semantics. Keying the cascade off the planned version alone would stop it at exactly the crate whose break was suppressed:graph LR a["a: 1.0.0 → 2.0.0, breaking"] -->|exposed| b["b: pinned 1.1.0 with --force, pin honoured, break suppressed not removed"] b -->|"cascade continues: the pin lowered the number, not the API"| c["c: raised to 2.0.0"]The pin itself is still honoured verbatim — only the propagation decision changed.
Propagation is widened solely by
PinHonoredAgainstCascade, and only when the suppressed requirement was itself breaking. That flag is set for any suppressed requirement, so a pin that merely undercuts an additivenon-breakingrequirement does not drag exposing dependents to a major release. Both branches route through the sameTest-IsBreakingChange, which keeps 0.x semantics consistent between them.Testing
497 Pester tests pass.
Regression coverage is deliberately split in two, because everything else in the suite builds synthetic package records and so only ever proves the planner correct about graphs the tests themselves invented:
trait-variantis abytesbuf_iodependency that is deliberately not allowlisted and must stay unexposed, andbytesbufbeing a strict prefix ofbytesbuf_iomust not cross-match either way.integration/ExposureCascade-RealWorkspace.Tests.ps1) — resolves a release set over the live workspace metadata and asserts the realbytesbuf → bytesbuf_iocascade, plus a negative control thatbytesbuf@patchdoes not force a break.The e2e test reads real manifests on purpose. Deleting
bytesbuf::*frombytesbuf_io's allowlist is exactly how this fail-open would be reintroduced, and that must break the suite rather than pass against a stale snapshot.Both regression layers were mutation-checked rather than assumed green:
Test-PackageExposesTargetreverted to the original fail-openTest-EntryPlansBreakingReleasekeyed off the change-type tag instead of the planned versionCommits
40b112bcd42c6cb1allowed_external_typesentries0a9ec0c5895ed2157373a04058ab2412bytesbuf/bytesbuf_iocascade against real manifestsKnown gap (not in scope)
Depsretains bothnormalandbuilddependency kinds, so a build-only edge could over-cascade. It is unreachable today — all four workspace build edges originate frompublish = falsecrates, which thePublishedfilter already excludes — and it fails in the safe direction (an unnecessary bump, never a silent break). The fix needs per-edgekindmetadata, tracked in AB#7729476.