Remove dead coverlet.collector dependency and the no-longer-needed System.Formats.Asn1 pin - #1281
Merged
Alexandre Zollinger Chohfi (azchohfi) merged 2 commits intoSep 24, 2026
Conversation
Follow-up cleanup to #1279, which removed Microsoft.NET.Test.Sdk from Directory.Packages.props and all 13 test projects. A PR review on that change flagged two leftovers it did not address. coverlet.collector is a VSTest data collector and is dead in this repo for two independent reasons: 1. Its only wiring hook is `<Target Name="SetXPlatDataCollectorPath" BeforeTargets="VSTest">` in coverlet.collector.targets. The `VSTest` MSBuild target was supplied by Microsoft.NET.Test.Sdk, which #1279 removed, so that hook now points at a target that no longer exists. (Inert, not an error - MSBuild tolerates BeforeTargets on a missing target.) 2. Nothing ever drove it. Coverage in this repo runs on the dotnet-coverage global tool: tools/coverage/run-coverage.ps1, .github/workflows/coverage.yml, TESTING.md and the coverage-uplift skill all invoke `dotnet-coverage collect|instrument|merge`. A repo-wide sweep for `coverlet` / `--collect` / `XPlat Code Coverage` found zero active consumers, with `dotnet-coverage` matching in 12 files as the positive control. src/vs-reactor/TESTING.md even records that `--collect:"XPlat Code Coverage"` fails with "Unable to find a datacollector with friendly name 'XPlat Code Coverage'". Removed the PackageVersion and the three PackageReference blocks (Reactor.Tests, Reactor.IntegrationTests, Reactor.DocPipeline.Tests). Under central package management a surviving PackageReference without a PackageVersion fails restore with NU1010, so the sweep mattered. Also corrected the now-stale comment on the System.Formats.Asn1 pin in Reactor.Compile.Analyzer.Tests, which attributed the vulnerable transitive 5.0.0 to "the Microsoft.CodeAnalysis.* / Test.Sdk graph". `dotnet nuget why` shows the only path today is Microsoft.CodeAnalysis.CSharp.Analyzer.Testing, and removing the pin resolves 9.0.6 - not 5.0.0 - with no NuGet audit warning. The pin is kept as defence in depth against a future transitive regression; only the comment changed, so it now describes verifiable reality. xunit.runner.visualstudio is deliberately retained: it runs xUnit v1/v2/v3, its build props emit `<ProjectCapability Include="TestContainer" />` for Visual Studio Test Explorer discovery, and its net8.0 dependency group is empty, so it never depended on Test.Sdk on modern TFMs. Verified: `dotnet restore Reactor.slnx` exit 0 (no NU1010); `dotnet build Reactor.slnx --no-restore -c Release` 0 errors; Reactor.Tests 14144/0 failed, Reactor.DocPipeline.Tests 472/0, Reactor.Compile.Analyzer.Tests 11/0. Reactor.IntegrationTests' 6 packaging failures are the pre-existing NU1301 network gate its own source documents ("On a network-restricted machine both fail identically with NU1301 during restore"). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Alexandre Zollinger Chohfi (azchohfi)
requested a review
from Chris Anderson (codemonkeychris)
as a code owner
September 24, 2026 20:22
Contributor
📦 Build metricsArtifact sizes for Packages (compressed .nupkg)
Assemblies in Microsoft.UI.Reactor
Assemblies in Microsoft.UI.Reactor.Advanced
Assemblies in Microsoft.UI.Reactor.Devtools
No size change beyond the noise floor. ✅ ✅ smaller / |
Contributor
🧪 Merged coverageCoverage for
No coverage change beyond the noise floor. ✅ ✅ higher / |
…earing Follow-up within this PR. The previous commit kept the pin and only corrected its comment; this removes it outright, because the condition it guarded no longer holds. GHSA-447r-wph3-92pm (CVE-2024-38095, High) lists two affected ranges for System.Formats.Asn1, read from the GitHub advisories API: >= 5.0.0-preview.7.20364.11, < 6.0.1 (patched 6.0.1) >= 7.0.0-preview.1.22076.8, < 8.0.1 (patched 8.0.1) Without the pin the package resolves to 9.0.6 (project.assets.json, net8.0 target), which is outside both ranges. It is now purely transitive - System.Formats.Asn1 no longer appears in projectFileDependencyGroups - reached only via Microsoft.CodeAnalysis.CSharp.Analyzer.Testing. The audit oracle was validated rather than assumed. The repo nuget.config pins sources to nuget.org, which is TLS-blocked here, so NuGet emits NU1900 ("unable to get package vulnerability data") and NU1901-NU1904 can never fire - a zero result would have been vacuous. Restoring instead against a feed that advertises VulnerabilityInfo/6.7.0 makes audit live: injecting System.Formats.Asn1 6.0.0 produced NU1903: Package 'System.Formats.Asn1' 6.0.0 has a known high severity vulnerability, GHSA-447r-wph3-92pm confirming the oracle fires. With the pin removed and the same live audit, `dotnet restore Reactor.slnx` exits 0 with zero NU1901/1902/1903/1904. Also dropped the now-orphaned <PackageVersion> from Directory.Packages.props. A repo-wide sweep found this project was its only consumer, and CentralPackageTransitivePinningEnabled is not set anywhere (defaults false), so a PackageVersion alone pins no transitive resolution - removing it changes nothing. Verified: solution restore exit 0, zero audit warnings; `dotnet build Reactor.slnx --no-restore -c Release` 0 errors; Reactor.Compile.Analyzer.Tests 11/11 passed. Replicating the CI "Vulnerable packages" gate end to end (restore + `dotnet list Reactor.slnx package --vulnerable --include-transitive` + its slnx-derived exclusion parse) gives listExit=0, 0 findings, no Asn1 rows. Note that gate excludes the 104 projects under samples/ and tests/, so it would not have caught a regression here on its own; the NU1903 control and the resolved 9.0.6 are the load-bearing evidence. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Alexandre Zollinger Chohfi (azchohfi)
changed the title
Alexandre Zollinger Chohfi (azchohfi)
merged commit Sep 24, 2026
71bc108
into
main
54 of 55 checks passed
Alexandre Zollinger Chohfi (azchohfi)
deleted the
azchohfi-coverlet-cleanup-followup
branch
September 24, 2026 21:08
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
1 participant
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.
Follow-up cleanup to #1279 ("Remove unnecessary
Microsoft.NET.Test.Sdkdependencies"), which removedMicrosoft.NET.Test.SdkfromDirectory.Packages.propsand all 13 test projects. A review on that PR flagged two leftovers it did not address. This PR removes both — net 20 deletions, 0 insertions across 5 files.1. Removed the dead
coverlet.collectordependencycoverlet.collectoris a VSTest data collector and is dead in this repo for two independently verified reasons:coverlet.collector.targetshangs everything off<Target Name="SetXPlatDataCollectorPath" BeforeTargets="VSTest">. TheVSTestMSBuild target was supplied byMicrosoft.NET.Test.Sdk, which Remove unnecessary Microsoft.NET.Test.Sdk dependencies #1279 removed. (This is inert rather than an error — MSBuild toleratesBeforeTargetson a missing target — which is exactly why it went unnoticed.)dotnet-coverageglobal tool:tools/coverage/run-coverage.ps1(dotnet-coverage collect/instrument/merge),.github/workflows/coverage.yml,TESTING.md, and.github/skills/coverage-uplift/SKILL.md. A repo-wide sweep forcoverlet,--collectandXPlat Code Coveragefound zero active consumers, withdotnet-coveragematching in 12 files as the positive control.src/vs-reactor/TESTING.mdeven records that--collect:"XPlat Code Coverage"fails with "Unable to find a datacollector with friendly name 'XPlat Code Coverage'".Removed the
PackageVersionplus thePackageReferenceblocks fromtests/Reactor.Tests,tests/Reactor.IntegrationTestsandtests/Reactor.DocPipeline.Tests. The only surviving mention istools/reviewer/reports/fix-list.md, a frozen 2026-04-11 review artifact describing a project shape that no longer exists — left untouched.Under central package management a surviving
PackageReferencewith noPackageVersionfails restore with NU1010, so the sweep mattered; the clean solution restore below is the proof it was complete.2. Removed the
System.Formats.Asn1pintests/Reactor.Compile.Analyzer.TestspinnedSystem.Formats.Asn1, with a comment attributing the vulnerable transitive 5.0.0 (GHSA-447r-wph3-92pm, High) to "theMicrosoft.CodeAnalysis.*/ Test.Sdk graph" — the Test.Sdk half is wrong after #1279. Rather than just re-word it, the pin is removed, because the condition it guarded no longer holds.Advisory range. GHSA-447r-wph3-92pm (CVE-2024-38095, High), read from the GitHub advisories API, lists two affected ranges for
System.Formats.Asn1:>= 5.0.0-preview.7.20364.11, < 6.0.1>= 7.0.0-preview.1.22076.8, < 8.0.1Resolved version without the pin:
9.0.6(project.assets.json,net8.0target) — outside both ranges. It is now purely transitive:System.Formats.Asn1no longer appears inprojectFileDependencyGroups, anddotnet nuget whyshows the only path isMicrosoft.CodeAnalysis.CSharp.Analyzer.Testing→Microsoft.CodeAnalysis.Analyzer.Testing(directly, and viaNuGet.Packaging→System.Security.Cryptography.Pkcs→Microsoft.Bcl.Cryptography).Microsoft.CodeAnalysis.CSharpand.Workspacescontribute no path at all.The audit oracle was validated, not assumed. This matters: the repo
nuget.config<clear />s sources down tonuget.org, which is TLS-blocked on the machine this was verified on, so NuGet emitsNU1900("unable to get package vulnerability data") and NU1901–NU1904 can never fire — a zero result would have been vacuous. Restoring instead against a feed advertisingVulnerabilityInfo/6.7.0makes audit live. InjectingSystem.Formats.Asn16.0.0 (inside the first affected range) produced:…confirming the oracle fires for exactly this advisory. With the pin removed and the same live audit,
dotnet restore Reactor.slnxexits 0 with zero NU1901/1902/1903/1904.The
PackageVersionwas decided separately. A repo-wide sweep (case-insensitiveAsn1) found this project was its sole consumer, andCentralPackageTransitivePinningEnabledis not set anywhere in the repo (defaultsfalse), so aPackageVersionalone pins no transitive resolution — removing it changes nothing. Dropped as orphaned.Deliberately NOT done
xunit.runner.visualstudiois retained on purpose — please don't re-litigate it. It looks like a VSTest leftover but is load-bearing: its nuspec (v4.0.0) states it runs "xUnit.net v1, v2, and v3 tests"; itsbuild/net8.0/xunit.runner.visualstudio.propsemits<ProjectCapability Include="TestContainer" />and copies the adapter DLL, which is what gives Visual Studio Test Explorer discovery; and its net8.0 dependency group is empty (only net472 depends onMicrosoft.TestPlatform.ObjectModel), so it never needed Test.Sdk on modern TFMs. Removing it would degrade the IDE experience for contributors.tests/Reactor.Compile.Analyzer.Testsandtests/external_proof/Reactor.External.TestControl.Testshave no CI test gate. That gap is real but pre-existing and intentionally left for a separate PR.OutputType, or anything else Remove unnecessary Microsoft.NET.Test.Sdk dependencies #1279 changed was touched.CHANGELOG: skipped per
CHANGELOG.md's own contributor-convention block and recent precedent — build-infrastructure-only change with no runtime effect.Verification
dotnet restore Reactor.slnxdotnet build Reactor.slnx --no-restore -c ReleaseSystem.Formats.Asn1after pin removalAsn16.0.0 injected)dotnet test tests/Reactor.Compile.Analyzer.Testsdotnet test tests/Reactor.Testsdotnet test tests/Reactor.DocPipeline.Testsdotnet test tests/Reactor.IntegrationTestsCI
Vulnerable packagesgate, replicated end to end (restore +dotnet list Reactor.slnx package --vulnerable --include-transitive+ the job's ownReactor.slnx-derived exclusion parse):listExit=0, 0 High/Critical findings in shipped projects, noAsn1rows at all → the job would pass. Worth flagging honestly: that gate excludes the 104 projects undersamples/andtests/from failing the build (issue #607), andReactor.Compile.Analyzer.Testsis in that excluded set — so the gate would not have caught a regression here on its own. The NU1903 control and the resolved9.0.6are the load-bearing evidence, not the gate.The 6
Reactor.IntegrationTestsfailures are all inPackaging.SourceMapPackageConsumerTests/Packaging.CreateTemplateTestsand are the documented network gate, not a regression.SourceMapPackageConsumerTests.cssays so in its own doc comment: "this needs network access to restore the Windows App SDK, so it only runs where NuGet.org is reachable (CI's 'Integration Tests' job). On a network-restricted machine both fail identically with NU1301 during restore." Every failure is verbatimNU1301againstapi.nuget.org, in temp consumer projects that never referenced coverlet. CI's Integration Tests job has the network access to run them for real.Test counts were checked to be non-zero: MTP prints "Zero tests ran" with exit 5 when it rejects a forwarded argument, and the module labels read
net10.0|x64/net8.0|x64(handshake-derived), confirming the hosts actually started.