Bump nerdbank deps#8096
Conversation
There was a problem hiding this comment.
Pull request overview
Pins Nerdbank.MessagePack (a transitive dependency of StreamJsonRpc) to a newer version intended to address a security vulnerability, and makes the dependency explicit in the affected integration test projects.
Changes:
- Add a centralized package version entry for
Nerdbank.MessagePackinDirectory.Packages.props. - Add explicit
PackageReferenceentries forNerdbank.MessagePackin two integration test.csprojfiles so the pinned version is actually used.
Show a summary per file
| File | Description |
|---|---|
| test/IntegrationTests/MSTest.Acceptance.IntegrationTests/MSTest.Acceptance.IntegrationTests.csproj | Adds explicit Nerdbank.MessagePack reference alongside StreamJsonRpc. |
| test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests.csproj | Adds explicit Nerdbank.MessagePack reference alongside StreamJsonRpc. |
| Directory.Packages.props | Introduces centralized version for Nerdbank.MessagePack with a security-focused comment. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 1
Evangelink
left a comment
There was a problem hiding this comment.
Summary
Workflow: Expert Code Reviewer
Date: 2026-05-11
Repository: microsoft/testfx
Key Findings
No issues found. This PR is a targeted security fix:
Directory.Packages.props— Adds an explicitNerdbank.MessagePackv1.1.62 version pin. Without this, Central Package Management (CPM) would resolve the version transitively fromStreamJsonRpc, potentially picking up a vulnerable version.- Two integration test
.csprojfiles — Add explicitPackageReferenceentries so the pinned version propagates into those projects. This is the standard CPM pattern: aPackageVersioninDirectory.Packages.propsonly governs versions; the package must also appear in aPackageReferencein each project that needs the override to take effect.
Correctness ✅
The CPM override pattern used here is correct. The PackageVersion pin in Directory.Packages.props sets the ceiling version, and the two PackageReference additions in the test projects ensure the resolved graph picks 1.1.62 rather than whatever older version StreamJsonRpc would otherwise pull in.
Recommendations
None — the approach is idiomatic and minimal.
Generated by Expert Code Reviewer 🧠
🧠 Reviewed by Expert Code Reviewer 🧠
Evangelink
left a comment
There was a problem hiding this comment.
Summary
Workflow: PR Nitpick Reviewer 🔍
Date: 2026-05-11
Repository: microsoft/testfx
Key Findings
✅ No nitpicks found — this is a clean, minimal security-fix PR.
- The explanatory comments clearly communicate why
Nerdbank.MessagePackis being explicitly listed (transitive dep ofStreamJsonRpc, security vulnerability fix). - Package reference ordering in both
.csprojfiles is alphabetically correct (MSBuild.StructuredLogger→Nerdbank.MessagePack→StreamJsonRpc). - The comment in
Directory.Packages.propsintentionally groups the entry nearStreamJsonRpcfor context, which is a reasonable trade-off over strict alphabetical order given the clear accompanying note. - Comment phrasing is consistent and informative across all three files.
Recommendations
No changes needed. The approach of placing the pinned transitive dependency immediately after its parent in Directory.Packages.props (with an explanatory comment) is a good practice for making dependency intent explicit.
🔍 Meticulously inspected by PR Nitpick Reviewer
🔍 Meticulously inspected by PR Nitpick Reviewer 🔍
Co-authored-by: GitHub Copilot <copilot@github.com>
No description provided.