Repository navigation
fix(release): close two fail-open gaps in the exposure cascade - #672
Merged
Kateřina Churanová (kate-shine) merged 3 commits intoAug 17, 2026
Conversation
…iner `allowed_external_types` must be an array, but `[package.metadata]` is arbitrary TOML that cargo passes through unvalidated, so any shape at all reaches the planner. Normalising it with `@($allowed.Value)` promoted a scalar into a well-formed one-entry allowlist: given `allowed_external_types = "std::*"`, the crate read as declaring a policy that names no target, so `Test-PackageExposesTarget` reported it as provably not exposing anything. That is a fail-open of the class this analysis exists to prevent -- a breaking dependency ships inside a semver-compatible version -- and it was the only malformed shape with that direction, since a non-string entry inside a real array is already caught per entry. Accept the value only when it is genuinely an array. Anything else leaves `$allowedTypes` as `$null`, which the existing absent-metadata branch already fails closed on, so no new branch is needed. Strings are excluded explicitly because a string is itself IEnumerable and would otherwise read as an array of single-character entries. An empty array stays a declared policy: it is the positive claim "my public API names nothing foreign", so it must not collapse to `$null` and be re-read as absent. Covered end-to-end through real `cargo metadata` rather than synthetic records, so the test also pins that cargo accepts the malformed manifest. Mutation-checked: restoring the wrapping fails exactly the two malformed-container tests and leaves the array and empty-array cases passing, confirming the guard does not over-reject. Refs microsoft#665 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 that holds both paths was judged only on the direct one. That is a fail-open, because the two predicates accept different allowlist roots by design. The direct test builds roots from the edge's DepAliases and deliberately rejects the target's own crate root, since a `package = "..."` rename shadows it. The indirect test builds roots from the target's own record. So a crate that imports the target under a rename, and also reaches it through a conduit that re-exports its types, loses the root its indirect path legitimately earned: it reports "not exposed" and stays on the patch floor while its public API is the target's types. Both edges are now evaluated independently and the crate is raised if either one claims exposure. 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 -- so the distinction is load-bearing, not stylistic. Shape notes: - Get-TransitiveDependentClosureFromBaseline is new and returns both the published-folder set and the full cargo-name closure. Get-TransitivePublishedDependentsFromBaseline now delegates to it and keeps its contract, since it has its own tests and a second caller. - The fixpoint's memo caches the closure object rather than the folder set, so the indirect test reuses the same BFS. - Test-PackageExposesTargetOnAnyEdge is split out so neither nested pipeline shadows $_. - [AllowEmptyCollection()] is required on the closure parameter: a mandatory parameter otherwise rejects an empty HashSet. Mutation evidence: - Reverting to 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 -- confirming the control actually guards the condition it names. - 87/87 in unit\release-packages, including a third test holding the positive-evidence requirement on the indirect path. The releasing.ps1 change is comment-only: contracts on Test-PackageAllowlistNamesTarget, its -TargetCrateRoot parameter, and Get-AcceptedExposureRoots described the old mutually-exclusive behaviour, as did the indirect-edge section of docs/releasing.md. Refs microsoft#665 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Kateřina Churanová (kate-shine)
August 14, 2026 07:58
View session
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes two fail-open gaps in the release-planner “exposure cascade” so dependents are correctly raised when their public API still surfaces a breaking dependency’s types—especially in mixed direct+indirect topologies and when allowed_external_types metadata is malformed.
Changes:
- Evaluate direct-edge exposure and indirect-path exposure independently (instead of
if/else) to correctly handle crates that have both paths. - Fail closed when
allowed_external_typesis not a genuine array (e.g., scalar or table), preserving[]as an explicit “no foreign types” policy. - Add/extend Pester unit tests and update docs/comments to reflect the corrected contracts.
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 |
|---|---|
| scripts/lib/release-flow.ps1 | Refactors dependent-closure computation and updates exposure selection to test direct and indirect relationships independently. |
| scripts/lib/releasing.ps1 | Tightens parsing of allowed_external_types so only real arrays become declared policy; malformed shapes route to fail-closed behavior. |
| scripts/tests/Pester/_common/New-SyntheticWorkspace.ps1 | Enables synthetic-workspace tests to emit malformed allowed_external_types shapes verbatim. |
| scripts/tests/Pester/unit/release-packages/ResolveReleaseSet.Tests.ps1 | Adds regression tests covering mixed direct+indirect path exposure semantics (including renamed direct edges). |
| scripts/tests/Pester/unit/releasing/GitFs.Tests.ps1 | Adds container-shape tests ensuring scalar/table fail closed and empty array is preserved as an explicit policy. |
| docs/releasing.md | Updates documentation to describe the independent evaluation of direct vs indirect paths and the rationale. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Kateřina Churanová (kate-shine)
requested a review
from Sander Saares (sandersaares)
August 14, 2026 08:00
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #672 +/- ##
=======================================
Coverage 100.0% 100.0%
=======================================
Files 503 503
Lines 57407 57406 -1
=======================================
- Hits 57407 57406 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Copilot started reviewing on behalf of
Kateřina Churanová (kate-shine)
August 14, 2026 09:32
View session
Sander Saares (sandersaares)
left a comment
Sander Saares (sandersaares)
left a comment
Member
There was a problem hiding this comment.
[Copilot speaking]
Published 1 finding. No finding follows up on an existing discussion thread.
See diagnostics
| Diagnostic | Value |
|---|---|
| Cache | Miss |
Sander Saares (sandersaares)
approved these changes
Aug 14, 2026
Get-WorkspacePackages recorded every non-dev dependency in Deps under its package name alone, discarding the resolved path. Nothing downstream could then tell a workspace member from a registry crate or an out-of-workspace path dependency that happens to share its name. Every consumer of Deps asks a workspace-reachability question -- which crates can carry a released package's types into their own public API -- so a text-name match lets an unrelated external crate stand in for a workspace conduit and fabricate a path that does not exist. Consider a member `relay` that depends on the breaking target, and a candidate depending on a *different*, non-member `relay`. The candidate lands in the target's dependent closure, and the indirect-path predicate reports a path it does not have. If the candidate allowlists the target's own crate root, the renamed direct edge correctly rejects that root but the fabricated indirect path accepts it, and the planner assigns and propagates a breaking release nobody needs. Dependency identity now comes from the resolved path: a dependency joins the graph only when its `path` resolves to a member manifest directory. `path` is absent for a registry dependency and points outside crates/ for a non-member path dependency. Notes on shape: - The fix is at the loader rather than at the three cascade call sites, because all eight consumers of Deps already filter by workspace membership -- two of them with a literal `# external package` skip -- so they were all asking for this and matching on the wrong key. - DepAliases is filtered by the same guard. Leaving it unfiltered would keep the same conflation one level down, letting an external crate's `package = "..."` rename supply an allowlist root for a same-named member. - A member depended on through the registry rather than by path is now excluded, which is correct: bumping the local copy does not affect a consumer resolving to the published one. No such dependency exists in the workspace today -- all 58 members are depended on by path. The name-only match predates the exposure cascade; it dates to microsoft#436 and also affected the reverse closure and the direct-edge gate. Fixing the loader closes all of them at once. Mutation evidence: removing the path guard fails exactly the two tests that assert identity -- the non-member does not become an edge, and the candidate does not enter the target's dependent closure -- while the two controls still pass, so genuine member edges are not over-rejected. Suites: unit\release-packages 87/87, integration\DependencyRename 16/16, integration\ExposureCascade-RealWorkspace 14/14, integration\Releasing-Integration 44/44. New-SyntheticWorkspace grows an ExternalPackages spec key and an External dep flag, emitting a crate under external/ that the workspace excludes, since a non-member path dependency is the only shape that tells name matching apart from identity matching. Refs microsoft#665 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Kateřina Churanová (kate-shine)
August 14, 2026 13:13
View session
martintmk
approved these changes
Aug 17, 2026
Kateřina Churanová (kate-shine)
merged commit Aug 17, 2026
c69cc2c
into
microsoft:main
51 checks passed
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.
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-PublishedDependentsExposingTargetpicked its exposure predicate with anif/elseon whether a direct dependency edge existed, making the direct andindirect 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:
DepAliases(rename-aware)package = "..."rename shadows itSo 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_typescontainer@($allowed.Value)promoted a malformed scalar into a valid-looking one-entryallowlist that matches nothing, yielding "not exposed".
[package.metadata]isarbitrary 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:
IEnumerable(over its chars), so it must be excluded explicitly;@()must survive as a positive claim of "no external types" — collapsing it to$nullwould 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:
if/elsefails exactly the new mixed-path test;test, whose only path to the target is the renamed direct edge;
@($allowed.Value)fails exactly the scalar and table tests, whilethe array and empty-array tests keep passing, proving no over-rejection.
Suites:
unit\release-packages87/87,unit\releasing67/67,integration\ExposureCascade-RealWorkspace14/14 (re-run after the rebase,since it reads live manifests and
mainhas moved).Also included: contract corrections on
Test-PackageAllowlistNamesTarget,Get-AcceptedExposureRootsand the indirect-edge section ofdocs/releasing.md,all of which described the old mutually-exclusive behaviour. The
releasing.ps1part of the first commit is comment-only.