Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unconditional candidate compatibility checks regress updates when the installed package already fails the conservative framework checker.
Review effort: Balanced
Findings: 1
What changed in this PR
Optimizes NuGet candidate selection by deferring package downloads and compatibility checks until candidates are evaluated.
Changes:
- Adds raw candidate discovery.
- Combines existence and compatibility validation into one package read.
- Adds regression and download-count coverage.
| File | Description |
|---|---|
Analyze/VersionFinder.cs |
Separates discovery from compatibility filtering. |
Analyze/CompatabilityChecker.cs |
Adds combined existence and compatibility validation. |
Analyze/AnalyzeWorker.cs |
Lazily validates ordered candidates. |
Analyze/VersionFinderTests.cs |
Tests compatibility-free discovery. |
Analyze/CompatibilityCheckerTests.cs |
Tests packages without project frameworks. |
Analyze/AnalyzeWorkerTests.cs |
Tests ordering, shared properties, and downloads. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
157a14e to
f710ad3
Compare
jmprieur
left a comment
There was a problem hiding this comment.
LGTM
One optional improvement: I would add a comment or test explaining that Dependabot may use a package already downloaded in the job’s NuGet cache, but only after confirming that the same version is available from an allowed package source. This behavior is intentional, but it may not be obvious to future maintainers (took me a bit of time)
f710ad3 to
099f6a8
Compare
|
@jmprieur Addressed the optional maintainability suggestion in 099f6a8. I added a regression that makes the two-part contract explicit: candidate discovery first confirms version 1.1.0 is advertised by the configured source, then compatibility selection reuses the matching archive already in the job-scoped |
There was a problem hiding this comment.
Note
Copilot was unable to run its full agentic suite in this review.
Copilot review overview
Review effort: Lite
Findings: 2
Open (4)
This method accepts acancellationTokenbut usesCancellationToken.Nonefor potentially… · New This method accepts acancellationTokenbut usesCancellationToken.Nonefor potentially… · New The file pathCompatabilityChecker.csuses the misspelling 'Compatability' (should be… · New Disposing the two readers without atry/finallymeans that if oneDispose()throws, the other… · New
099f6a8 to
093ea63
Compare
There was a problem hiding this comment.
Note
Copilot was unable to run its full agentic suite in this review.
Copilot review overview
Review effort: Lite
Findings: None
Resolved since last review (4)
This method accepts acancellationTokenbut usesCancellationToken.Nonefor potentially… This method accepts acancellationTokenbut usesCancellationToken.Nonefor potentially… Disposing the two readers without atry/finallymeans that if oneDispose()throws, the other… The file pathCompatabilityChecker.csuses the misspelling 'Compatability' (should be…
Discover candidate versions without opening every package, then combine existence and framework compatibility checks while evaluating candidates in preference order. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
093ea63 to
8eeeab8
Compare
jmprieur
left a comment
There was a problem hiding this comment.
Findings: 2 (0 critical, 2 high, 0 medium, 0 low).
Authored against head 8eeeab88.
| packageIds, | ||
| version, | ||
| nugetContext, | ||
| cancellationToken); |
There was a problem hiding this comment.
[high] Confirm allowed-source availability for cached sibling packages
Could we retain source-availability checks for every package sharing the version property before accepting a candidate? Discovery confirms the version only for the primary package, while both new validation branches can read a sibling directly from NUGET_PACKAGES.
If A and B share a property, the configured feed serves A 2.0.0 but not B 2.0.0, and a compatible B 2.0.0 archive is cached, this loop accepts 2.0.0. The removed DoVersionsExistAsync rejected it by checking the allowed sources for each identity. Please keep cache reuse for archive inspection, but independently confirm each sibling's source availability.
| cancellationToken) | ||
| : await DoAllPackagesExistAsync( | ||
| packageIds, | ||
| version, |
There was a problem hiding this comment.
[high] Preserve primary-package compatibility in the fallback path
Could the existence-only fallback still check compatibility for the primary package? Previously GetVersionsAsync(projectFrameworks, ...) filtered primary-package candidates before this loop, even when the installed version failed the checker. Raw discovery removes that filter.
For a net8.0 project with an incompatible installed package, a feed containing compatible 2.0.0 and net9.0-only 3.0.0 previously selected 2.0.0; this branch now selects 3.0.0. An incompatible current sibling can also disable filtering for the primary package. Keeping the primary's compatibility check lazy, while relaxing only sibling compatibility in this fallback, would preserve the previous selection behavior.



What are you trying to accomplish?
Reduce NuGet analysis work by separating version discovery from package compatibility validation. Analysis now discovers filtered candidate versions without opening every package, then validates package existence and target-framework compatibility lazily in preferred-version order until it finds a usable candidate.
This avoids downloading or opening package archives that cannot be selected and removes the duplicate existence and compatibility archive access previously performed for attempted candidates.
Part of #16372.
Anything you want to highlight for special attention from reviewers?
GetVersionsByNameAsyncand the existingGetVersionsAsyncoverloads remain compatibility-aware because dependency conflict resolution relies on that behavior. OnlyAnalyzeWorkeruses the new raw candidate-discovery path.Candidate ordering is unchanged:
Existence and compatibility are now evaluated through one package read per attempted package identity. Shared version properties still require every associated package ID to exist and be compatible.
How will you know you've accomplished your goal?
Added regression coverage for:
VersionFindercallers.Validation:
dotnet test nuget\helpers\lib\NuGetUpdater\NuGetUpdater.Core.Test\NuGetUpdater.Core.Test.csproj --no-restore --filter "FullyQualifiedName~NuGetUpdater.Core.Test.Analyze.AnalyzeWorkerTests|FullyQualifiedName~NuGetUpdater.Core.Test.Analyze.CompatibilityCheckerTests|FullyQualifiedName~NuGetUpdater.Core.Test.Analyze.VersionFinderTests" --nologo --verbosity minimalPassed: 66, Failed: 0, Skipped: 0.
Checklist