Re-land the NuGet-client migration on stable NuGet 7.9.0 by azchohfi · Pull Request #761 · microsoft/winappCli

Alexandre Zollinger Chohfi (azchohfi) · 2026-08-17T20:25:27Z

Description

This reverts #654, which reverted #629 (and its stacked coverage PR #647).

#629 migrated NugetService off hand-rolled NuGet v3 HTTP calls and onto the official NuGet client libraries, so init / restore / update honor the user's nuget.config hierarchy: custom and private package sources, credentials, packageSourceMapping, and globalPackagesFolder. Nothing was wrong with that migration. It was reverted purely because it pinned NuGet.* to the prerelease 7.9.0-rc.36120 from the public dnceng dotnet-tools feed, and the ADO security gate rejects consuming a prerelease from a secondary public feed. That was always documented as temporary: the NuGet.UseSystemTextJsonDeserialization switch needed to keep the Native AOT publish clean had not shipped in a nuget.org stable yet.

NuGet 7.9.0 is now stable on nuget.org, which is exactly the condition #654 named for re-landing. So the pins point at 7.9.0 stable, and nuget.config plus .pipelines/release-nuget.config go back to main's minimal form: no dotnet-tools source, no packageSourceMapping, no upstream note.

Beyond a plain re-revert

About 100 commits landed on main between the revert and now, so this is not a clean re-application. The notable resolutions:

NugetService.cs takes the migrated implementation wholesale. The only two commits that touched it after the revert (Test coverage: NuGet/update/install files reverted by #654 (issue #630) #671, [s360-breeze-toolkit: SFI-ES-4.2.4] Use internal npm feed in Azure Pipelines #715) added seams and env-var endpoint overrides that exist solely to serve the hand-rolled implementation, so nothing real is dropped.
PackageInstallationServiceTests.cs / UpdateCommandTests.cs were add/add conflicts. Main's versions came from Test coverage: NuGet/update/install files reverted by #654 (issue #630) #671 and cover the "already present, resolve deps via feed" branch and the string-inequality update gate, both of which the migration deletes. The migrated versions win; one still-relevant test from main (UpdateCommand_ExposesNameShortDescriptionAndSetupSdksOption) was ported forward.
Deleted NugetServiceOfflineTests.cs (748 lines). Every method targeted the removed HttpGetAsync / GetUserProfileDirectory seams. The restored NuGet suite is a superset for everything still reachable.
Interface drift ported into the restored tests: IsPackageInstalled on FakeSignToolNugetService, the widened IBuildToolsService.RunBuildToolAsync signature, and the runtime-install step moving from IWorkspaceSetupService to IWindowsAppRuntimeService.

Feed configuration, and why the pipeline env vars go away

#715 added WINAPP_NUGET_FLAT_CONTAINER / _REGISTRATION / _AUTH_PREFIX so the network-isolated ADO legs could reach pde-oss_Internal. Those variables only ever configured the hand-rolled endpoints, which no longer exist. ci.yml and release.yml already copy release-nuget.config to both %APPDATA%\NuGet\NuGet.Config and the repo root and then run NuGetAuthenticate@1, and the NuGet client picks all of that up on its own, including the credential provider. So the env blocks are removed from the Build CLI and Bundle MSIX Packages steps.

The one place that still needed an explicit feed is the live-feed integration tests. Those helpers used to hand-roll registration and flat-container HTTP against a hardcoded api.nuget.org; they now go through PackageMetadataResource / FindPackageByIdResource against the same source the test config declares, and that source is overridable with a new WINAPP_TEST_NUGET_SOURCE. AGENTS.md is updated with the nuget.config-based equivalent for corp machines, replacing the old env-var recipe.

Follow-on fixes from review and from testing the branch

These came out of the review passes and from running the branch against a real app, and are in the later commits rather than the re-land itself:

A fully cached dependency graph now restores without querying sources (Restoring a fully cached dependency graph still requires querying NuGet sources #762). Reading the dependency list from a cached package's local .nuspec was not enough, because each declared range was still resolved to a concrete version against the feeds only. So an already-extracted graph failed offline, or under a packageSourceMapping that no longer maps a transitive package anywhere. Resolution now falls back to the lowest fully installed cache entry (completion marker required) before failing. Deliberately confined to the failure paths, so a reachable source always decides the version and online restores select exactly what they did before.
winapp restore works after winapp init on a .NET project. For .NET projects init records versions as PackageReferences rather than a winapp.yaml, and restore then exited 1 telling the user to run dotnet restore by hand. It runs it instead.
Feed failures explain themselves. On a project whose nuget.config pins a blocked or unreachable feed, init reported a bare Failed to get version for Microsoft.WindowsAppSDK; the NuGet layer's message naming the source and the reason was logged at debug level only. It is now surfaced and included in the returned error.
An unsatisfiable diamond involving a float and an exact prerelease pin is no longer accepted. RangesHaveCommonVersion decides a single-point intersection exactly, since prerelease eligibility is not expressible as a numeric interval (1.* and [1.5.0-preview] intersect at a non-empty point yet share no version). Kept narrow on purpose: widening the existing witness heuristic instead would wrongly fail cases like 1.* with (1.2.0, 1.8.0).
Code-quality findings: deterministic disposal of the test listeners' port probes, cleanup catches narrowed to the exceptions those blocks exist to swallow, explicit .Where(...) filters, and a guard on the one Path.Combine whose segment comes from an externally supplied package id.

Usage Example

The behavior this restores: winapp restore (and init / update) picks up a nuget.config like this and restores the SDK packages from a private mirror.

<?xml version="1.0" encoding="utf-8"?>
<configuration>
  <packageSources>
    <clear />
    <add key="contoso" value="https://pkgs.dev.azure.com/contoso/_packaging/winsdk-mirror/nuget/v3/index.json" />
  </packageSources>
</configuration>

winapp restore

Related Issue

Closes #653.
Closes #762.
Re-lands #629 and #647, reverting #654.

Type of Change

✨ New feature
🐛 Bug fix
♻️ Refactoring
🔧 Config/build

Checklist

New tests added for new functionality (if applicable)
Tested locally on Windows
docs/usage.md updated (if CLI commands changed)
Shipped skills updated in plugins/winapp/skills/ (if CLI commands/workflows changed)

Additional Notes

Validation

dotnet build winapp.sln -c Debug -p:TreatWarningsAsErrors=true: 0 warnings, 0 errors.
Full suite green on two consecutive runs: 4643 total, 4638 passed, 5 skipped, 0 failed.
Release Native AOT publish clean for both win-x64 and win-arm64. Confirmed NuGet.UseSystemTextJsonDeserialization still reaches ILC as a trim feature on 7.9.0 stable (--feature: and --runtimeknob: both present in the generated .ilc.rsp), and no Newtonsoft.Json assets land in the published output.
End to end against the published AOT binary: winapp init in a directory whose nuget.config clears nuget.org, declares a single custom mirror, and sets globalPackagesFolder restored all 18 SDK packages from that mirror into the project-local folder. This exercises the actual feature, not just the build.
The Restoring a fully cached dependency graph still requires querying NuGet sources #762 tests were verified non-vacuous: the offline and mapping-excluded cases fail without the fix and pass with it.

For reviewers

The highest-risk part is the conflict resolution, not the migration itself, since the migration already went through review in #629. The resolutions above are the places worth a careful look. I also ran an independent review pass over the conflict resolution; it confirmed NugetService.cs and friends are byte-identical to their pre-revert state apart from the documented UpdateCommand port-forward, and it caught the dropped UpdateCommandTests metadata test that is now restored.

NugetServiceCachedGraphTests is marked [DoNotParallelize]: it warms a real global-packages folder, then reconfigures and re-reads it, and that two-phase sequence proved intermittently sensitive to the 32-way parallel test run.

Two deliberate omissions: running scripts/build-cli.ps1 also regenerates docs/npm-usage.md (line endings only) and bumps a stale schema-version string in src/winapp-npm/src/winapp-commands.ts from 0.5.1 to 0.6.1. Both are pre-existing drift on main and unrelated to this change, so they are left out to keep the diff focused.

Screenshots / Demo

N/A

AI Description

This section is auto-generated by AI when the PR is opened or updated. To opt out, delete this entire section including the marker comments.

Reverts #654, which reverted #629 (+ its stacked coverage PR #647). #654 was only needed because #629 pinned NuGet.* to the prerelease 7.9.0-rc.36120 from the public dnceng dotnet-tools feed, which the ADO security gate rejects. NuGet 7.9.0 is now a stable release on nuget.org, so the migration can land without a secondary feed. Closes #653. NugetService is back on the official NuGet client libraries, so init/restore/ update honor the user's nuget.config hierarchy: custom and private package sources, credentials, packageSourceMapping and globalPackagesFolder. Changes on top of a plain re-revert: - NuGet.* pinned to stable 7.9.0. nuget.config drops the dotnet-tools source, the packageSourceMapping pins and the disabledPackageSources clear, returning to the minimal pre-#629 file; .pipelines/release-nuget.config drops the note about the dotnet-tools upstream. - Conflict resolution against ~100 commits of main: NugetService.cs takes the migrated implementation wholesale (the post-revert edits there were test seams for the hand-rolled version), and PackageInstallationServiceTests / UpdateCommandTests take the migrated versions, since #671's replacements covered the "already present, resolve deps via feed" branch and the string-inequality update gate that the migration removes. - Deleted NugetServiceOfflineTests.cs: it exercised the hand-rolled HttpGetAsync/GetUserProfileDirectory seams, which no longer exist. - Ported forward interface drift into the restored tests: IsPackageInstalled on FakeSignToolNugetService, the widened IBuildToolsService.RunBuildToolAsync signature, and the runtime-install step moving from IWorkspaceSetupService to IWindowsAppRuntimeService. - NugetServiceTests' live-feed helpers no longer hand-roll registration and flat-container HTTP against a hardcoded api.nuget.org; they go through PackageMetadataResource / FindPackageByIdResource against the same source the test config declares. That source is overridable with WINAPP_TEST_NUGET_SOURCE so the network-isolated ADO pipeline and corp machines can point at a mirror. - Replaced the WINAPP_NUGET_FLAT_CONTAINER / _REGISTRATION / _AUTH_PREFIX environment overrides added by #715 in .pipelines/templates/build.yaml. They only ever configured the hand-rolled endpoints; the pipeline already writes release-nuget.config to both the repo and user scope and runs NuGetAuthenticate, which the NuGet client picks up on its own. AGENTS.md documents the nuget.config-based equivalent for corp machines. Validation: - dotnet build winapp.sln -c Debug: 0 warnings, 0 errors. - Full suite: 4634 tests, 4624 passed, 5 skipped. The 5 failures are the pre-existing EndToEndTests node subcommands that require src/winapp-npm/dist/cli.js to be built first. - Release Native AOT publish clean for win-x64 and win-arm64; NuGet.UseSystemTextJsonDeserialization still reaches ILC as a trim feature on 7.9.0 stable, and no Newtonsoft.Json assets land in the published output. - End-to-end against the AOT binary: winapp init in a directory whose nuget.config clears nuget.org, declares a single custom mirror and sets globalPackagesFolder restored all 18 SDK packages from that mirror into the project-local folder. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Re-lands the official NuGet client migration on stable NuGet 7.9.0, enabling standard nuget.config behavior while preserving Native AOT compatibility.

Changes:

Adds NuGet-backed source, credential, download, cache, and dependency resolution.
Updates package installation and update workflows.
Expands tests, documentation, and pipeline configuration.

Reviewed changes

Copilot reviewed 35 out of 35 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`.pipelines/templates/build.yaml`	Configures the integration-test feed.
`AGENTS.md`	Updates corporate NuGet setup guidance.
`docs/dotnet-run-support.md`	Marks NuGet AOT issues resolved.
`docs/usage.md`	Documents private feeds and limitations.
`plugins/winapp/skills/winapp-setup/SKILL.md`	Updates setup workflow guidance.
`src/winapp-CLI/Directory.Packages.props`	Pins stable NuGet 7.9.0 packages.
`src/winapp-CLI/WinApp.Cli.Tests/AzureSignToolServiceTests.cs`	Updates the NuGet test fake.
`src/winapp-CLI/WinApp.Cli.Tests/BuildToolsServiceTests.cs`	Tests normalized version pins.
`src/winapp-CLI/WinApp.Cli.Tests/FakeNugetService.cs`	Expands fake cache and cancellation behavior.
`src/winapp-CLI/WinApp.Cli.Tests/NugetFeedTestHelpers.cs`	Adds local-feed test utilities.
`src/winapp-CLI/WinApp.Cli.Tests/NugetPackageDownloaderCoverageTests.cs`	Covers download diagnostics and cleanup.
`src/winapp-CLI/WinApp.Cli.Tests/NugetServiceCoverageTests.cs`	Covers remaining NuGet branches.
`src/winapp-CLI/WinApp.Cli.Tests/NugetServiceDependencyTests.cs`	Tests dependency resolution and cache scoping.
`src/winapp-CLI/WinApp.Cli.Tests/NugetServiceDownloadTests.cs`	Tests downloads, authentication, and feeds.
`src/winapp-CLI/WinApp.Cli.Tests/NugetServiceFeedTests.cs`	Tests source configuration and versions.
`src/winapp-CLI/WinApp.Cli.Tests/NugetServiceInstallGraphTests.cs`	Tests graph integrity and partial caches.
`src/winapp-CLI/WinApp.Cli.Tests/NugetServiceOfflineTests.cs`	Removes obsolete HTTP-seam tests.
`src/winapp-CLI/WinApp.Cli.Tests/NugetServiceTests.cs`	Migrates live-feed integration tests.
`src/winapp-CLI/WinApp.Cli.Tests/NugetServiceVersionRangeTests.cs`	Tests range-intersection helpers.
`src/winapp-CLI/WinApp.Cli.Tests/PackageInstallationServiceCacheMarkerTests.cs`	Tests partial-cache recovery.
`src/winapp-CLI/WinApp.Cli.Tests/PackageInstallationServiceTests.cs`	Updates installation orchestration tests.
`src/winapp-CLI/WinApp.Cli.Tests/UpdateCommandTests.cs`	Tests update decisions and failures.
`src/winapp-CLI/WinApp.Cli.Tests/WinApp.Cli.Tests.csproj`	Enables NuGet’s STJ path in tests.
`src/winapp-CLI/WinApp.Cli.Tests/WorkspaceSetupServiceConfigRootTests.cs`	Tests project-rooted NuGet settings.
`src/winapp-CLI/WinApp.Cli/Commands/UpdateCommand.cs`	Improves version comparisons and failure handling.
`src/winapp-CLI/WinApp.Cli/Helpers/HostBuilderExtensions.cs`	Registers new NuGet services.
`src/winapp-CLI/WinApp.Cli/Services/BuildToolsService.cs`	Normalizes pinned cache versions.
`src/winapp-CLI/WinApp.Cli/Services/INugetService.cs`	Documents and extends the NuGet contract.
`src/winapp-CLI/WinApp.Cli/Services/NugetPackageDownloader.cs`	Implements package download and extraction.
`src/winapp-CLI/WinApp.Cli/Services/NugetService.Dependencies.cs`	Implements version and dependency resolution.
`src/winapp-CLI/WinApp.Cli/Services/NugetService.cs`	Implements NuGet-backed installation and caching.
`src/winapp-CLI/WinApp.Cli/Services/NugetSourceProvider.cs`	Resolves sources, mappings, and credentials.
`src/winapp-CLI/WinApp.Cli/Services/PackageInstallationService.cs`	Delegates graph installation to NuGetService.
`src/winapp-CLI/WinApp.Cli/Services/WorkspaceSetupService.cs`	Roots NuGet settings at the config directory.
`src/winapp-CLI/WinApp.Cli/WinApp.Cli.csproj`	Adds NuGet libraries and AOT settings.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…p catches Resolves the github-code-quality threads on the re-landed NuGet code. - Dispose the temporary TcpListener port probes in the two in-process feed fakes (using var), so the probe socket is released deterministically even if HttpListener.Start throws mid-retry. - Narrow best-effort cleanup catches to the exceptions the operation actually reports: IOException/UnauthorizedAccessException for File.Delete and Directory.Delete, and HttpListenerException/ObjectDisposedException (plus OperationCanceledException on the accept loop) for the listener teardown paths. Behavior is unchanged for the failures these blocks exist to swallow, but an unexpected defect now surfaces instead of being silently discarded. Also fixed the same patterns the analyzer sampled but did not individually flag, in the same methods, so a re-scan does not reopen equivalent threads. Left as-is deliberately: the two catch (Exception ex) blocks in NugetService.Dependencies.cs. Those do not suppress anything -- each records the failure and the caller rethrows it (fail-closed for latest-version resolution, reported via CandidateVersionsResult.Error for range resolution), and cancellation is already rethrown ahead of them. Narrowing to HttpRequestException/FatalProtocolException would drop the 'which source failed and why' context and turn a recoverable single-source failure into a hard failure across the remaining sources. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

…ters, path hardening - RangesHaveCommonVersion: decide a single-point intersection exactly by testing that version against every range with the float-aware predicate. Prerelease eligibility is not an interval property, so a stable float and an exact in-band prerelease pin (1.* vs [1.5.0-preview]) intersect at a non-empty numeric point yet share no version, and were wrongly accepted as the documented keep-first-selected case instead of failing. Exact pins are the common shape here since the SDK packages pin their sub-packages that way. Deliberately narrow: a single point is fully decidable, so this cannot produce a false conflict, unlike widening the existing witness heuristic (which would wrongly fail e.g. 1.* with (1.2.0, 1.8.0)). - ReadDependenciesFromNuspec: force the nuspec segment to a bare file name before Path.Combine. The id comes from another package's manifest, so a rooted value would otherwise make Combine discard the package directory. Callers already validate ids via GetNuGetPackageDir; this keeps the invariant local and checkable. - Filter dependency groups and float witnesses with explicit .Where(...). - Drop a stale comment claiming experimental mode is unfiltered, which contradicted the branch immediately above it. Extended NugetServiceVersionRangeTests with five single-point rows covering the stable-float/prerelease-pin case and its stable and prefix-float counterparts. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

github-actions · 2026-08-17T21:09:56Z

Build Metrics Report

Binary Sizes

Artifact	Baseline	Current	Delta
CLI (ARM64)	38.63 MB	43.10 MB	📈 +4.47 MB (+11.57%)
CLI (x64)	38.74 MB	43.09 MB	📈 +4.35 MB (+11.22%)
MSIX (ARM64)	16.02 MB	17.85 MB	📈 +1.83 MB (+11.44%)
MSIX (x64)	17.02 MB	18.94 MB	📈 +1.92 MB (+11.26%)
NPM Package	33.43 MB	37.25 MB	📈 +3.82 MB (+11.43%)
NuGet Package	33.47 MB	37.30 MB	📈 +3.83 MB (+11.45%)

Test Results

✅ 4662 passed, 5 skipped out of 4667 tests in 858.2s (+66 tests, +166.0s vs. baseline)

Test Coverage

✅ 89.2% line coverage, 82.5% branch coverage · ✅ +0.1% vs. baseline

CLI Startup Time

54ms median (x64, winapp --version) · ✅ no change vs. baseline

Try This Build

Installs the MSIX for your architecture, replacing any previously installed build. Needs the GitHub CLI — the command offers to install it and sign you in if it is missing.

& ([scriptblock]::Create((irm https://raw.githubusercontent.com/microsoft/winappCli/main/scripts/winapp-pr.ps1))) 761

Switching between builds often?

Put the tool on your PATH once:

& ([scriptblock]::Create((irm https://raw.githubusercontent.com/microsoft/winappCli/main/scripts/winapp-pr.ps1))) -AddToPath

Then this build is just:

winapp-pr 761

Run winapp-pr with no arguments to pick from a list of open PRs.

Updated 2026-08-18 21:41:00 UTC · commit 7f22bca · workflow run

For .NET projects, init records the SDK package versions as PackageReferences in the .csproj instead of a winapp.yaml. Restore then found no yaml and exited 1 with a message telling the user to run 'dotnet restore' by hand, so the natural 'winapp init' then 'winapp restore' sequence failed on every .NET project. Run dotnet restore instead, surfacing its exit code. Also apply the reviewer's EnsurePackageAsync finding: a cached root no longer returns early. InstallPackageAsync already short-circuits a completed root via the completion marker, so this does not re-download it, but it now walks the graph from the root's local .nuspec, repairing a transitive package that was deleted or never finished installing instead of reporting a complete install. Both packages EnsurePackageAsync is used for (SDK.BuildTools.WinApp and Trusted.Signing.Client) declare no dependencies, so this is inert today; it removes a latent gap and matches the multi-package path. Verified end to end: dotnet new classlib, winapp init --use-defaults, then winapp restore now succeeds (exit 0) where it previously failed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

… output Reported while testing this PR: on a project whose nuget.config pins a feed the user's network blocks, 'winapp init' failed with a bare 'Failed to get version for Microsoft.WindowsAppSDK' and no indication that a nuget.config had selected an unreachable source. The NuGet layer already produces an actionable message naming the source and the failure, but WorkspaceSetupService logged it with AddDebugMessage and returned a generic string, so it was invisible without --verbose. Report it at status level and include it in the returned error, for both the version lookup and the 'dotnet add package' step. Now: 'Failed to get version for Microsoft.WindowsAppSDK: Could not reliably determine the versions of Microsoft.WindowsAppSDK: source 'nuget.org' could not be queried: Unable to load the service index for source https://api.nuget.org/v3/index.json.' Also assert the nuspec file name is not rooted before Path.Combine, per review. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 36 out of 36 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/winapp-CLI/WinApp.Cli/Services/PackageInstallationService.cs:1

IsPackageInstalled(packageName, version) is invoked before any version normalization in this method. If a caller passes a shorthand pin (e.g. "1.0"), NuGet’s on-disk folder will typically be normalized (e.g. "1.0.0"), so the cache-hit check can incorrectly miss and report/install unnecessarily, and the returned version can remain non-canonical. Fix by normalizing version before the cache check, before calling InstallPackageAsync, and before returning it (so downstream path builders don’t concatenate a non-existent folder name).
src/winapp-CLI/WinApp.Cli/Services/WorkspaceSetupService.cs:1
dotNetService.FindCsproj(options.BaseDirectory) is executed twice in the same branch (once for the Count > 0 check and once to get the list). Capture it once (e.g., call it once and use Any()/Count) to avoid duplicate directory scans and to guarantee the selection list matches the predicate that entered the branch.
src/winapp-CLI/WinApp.Cli/Services/WorkspaceSetupService.cs:1
dotNetService.FindCsproj(options.BaseDirectory) is executed twice in the same branch (once for the Count > 0 check and once to get the list). Capture it once (e.g., call it once and use Any()/Count) to avoid duplicate directory scans and to guarantee the selection list matches the predicate that entered the branch.
src/winapp-CLI/WinApp.Cli.Tests/AzureSignToolServiceTests.cs:329
The test fake now throws NotImplementedException for IsPackageInstalled. Since this method is part of INugetService and is increasingly used as an “already installed” predicate, throwing here can create fragile tests if code paths change. Prefer returning a deterministic default (typically false) or implement minimal behavior consistent with the rest of this fake.

    public DirectoryInfo GetNuGetPackageDir(string packageName, string version)
        => throw new NotImplementedException();

    public bool IsPackageInstalled(string packageName, string version)
        => throw new NotImplementedException();

…762) Reading a cached package's dependency list from its local .nuspec was not enough: each declared range still had to be resolved to a concrete version, and that only consulted the configured sources. So a graph already fully extracted in the global packages folder failed to restore whenever the feeds could not answer -- offline, or under a packageSourceMapping that no longer maps a transitive package to any source. ResolveDependencyVersionAsync now falls back to the lowest FULLY installed cache entry satisfying the range before it throws. Only entries carrying NuGet's .nupkg.metadata completion marker count, matching the predicate the rest of the service gates 'already installed' on, and selection re-checks the float-aware predicate exactly like the online path. Deliberately confined to the failure paths. Folding cached versions into the candidate set would let a cached version win NuGet's lowest-applicable rule and change which version an online restore selects; as a fallback it cannot, which the third test pins down. Tests: restore with zero configured sources (feed deleted), restore when the mapping excludes a transitive package, and no change to the selected version while sources can answer. Verified non-vacuous -- the first two fail without the fix. The class is serialized with DoNotParallelize: it warms a real global-packages folder then reconfigures and re-reads it, which proved intermittently sensitive to the 32-way parallel run. Also corrected the PackageInstallationService comment that claimed the marker hit resolved dependencies entirely from the local .nuspec, and documented the fallback in docs/usage.md and the winapp-setup skill. Full suite green twice: 4643 total, 4638 passed, 5 skipped, 0 failed. Closes #762 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Both from code-quality review of the #762 fallback. FindSatisfyingCachedVersion built a path straight from packageId, which arrives from a dependency entry in another package's manifest. Validate it as a real NuGet id and prove it is a plain single segment (not rooted, no separators) before combining it with the cache root, so a hostile or malformed id cannot make Path.Combine discard the root and probe elsewhere. An unusable id returns null -- a cache probe should not throw, and the caller then falls through to its regular source-based diagnostics. Also express the completed-entry selection as an explicit .Where(...) filter rather than a foreach with an inner if. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Two more code-quality findings. FindSatisfyingCachedVersion now locates the package folder by enumerating for it instead of composing a path. The suggested DirectoryInfo.CreateSubdirectory was not usable: it CREATES the directory, so every lookup that missed would litter the user's global packages folder with empty folders as a side effect of a read-only probe. Enumerating is side-effect free, and the id is validated directly above, so it carries no wildcard characters and matches at most one folder. In BuildToolsServiceTests the flagged calls build fixed paths from compile-time constants, so nothing could ever be dropped, but Path.Join expresses that directly: unlike Path.Combine it always concatenates and cannot discard earlier segments. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Same rule as 68c0ea7, now on the assertion fragments and the packages-dir setup in the shorthand-pin test. All segments are compile-time constants or a trusted temp root, so nothing could be dropped, but Path.Join concatenates without Path.Combine's rooted-segment reset and states that directly. Converts the whole method so a later scan does not re-flag the one that was left. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

The code-quality scanner flags every Path.Combine in the diff a few at a time, so fix the pattern rather than the three latest instances. Every call site here joins a trusted base directory to a relative segment: a literal, a GUID, a package id, an architecture, a tool name, or a random temp file name. Path.Join is equivalent for those inputs and strictly safer for the variable ones, because it always concatenates and cannot let a rooted segment discard the base -- which is precisely the hazard the rule describes. Verified the product call sites individually rather than trusting the sweep: FindPackagePath's subPath is an internal literal, the arch values come from RuntimeInformation, and the id-derived paths keep their existing validation. The defaultWinapp comparison in IsTestOverride still produces the identical string, so the test-override check is unchanged. Comments that explained the old Path.Combine drop semantics were reworded to describe the invariant rather than the API. Full suite green: 4643 total, 4638 passed, 5 skipped, 0 failed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 37 out of 37 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/winapp-CLI/WinApp.Cli/Services/WorkspaceSetupService.cs:83

This newly supported .NET restore path is still contradicted by the public interface: RestoreCommand says winapp.yaml is required, docs/usage.md:227 and docs/guides/dotnet.md:106 tell .NET users to run dotnet restore instead, and the shipped setup skill only documents YAML restores. Update those surfaces to advertise winapp restore for initialized .NET projects, then regenerate the CLI schema so users can discover this behavior.

Copilot

Pull request overview

Copilot reviewed 37 out of 37 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/winapp-CLI/WinApp.Cli.Tests/WorkspaceSetupServiceConfigModeTests.cs:109

This assertion only verifies the new success code, so the test would still pass if the .NET-project branch returned success without invoking dotnet restore. Assert the fake's recorded inherited call as well to cover the behavior named by the test and prevent the old no-op path from satisfying it.

        Assert.AreEqual(0, result);

From the Copilot review of the restore change. Making 'winapp restore' work on a .NET project was only half the job: every place that describes the command still told users it was unsupported there. - RestoreCommand's description and short description no longer say winapp.yaml is required; they describe reading versions from winapp.yaml or, for a .NET project, from the .csproj via dotnet restore. - docs/usage.md and docs/guides/dotnet.md previously told .NET users to run dotnet restore themselves; they now say winapp restore does it for them. - The shipped winapp-setup skill said restore is for cloned repos that have a winapp.yaml; it now covers both project shapes. - Regenerated docs/cli-schema.json so the new description is discoverable. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 40 out of 40 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

src/winapp-CLI/WinApp.Cli/Services/NugetService.Dependencies.cs:297

The cache fallback also runs when eligible sources were queried successfully but none offers a version satisfying the range. A completed version left by a different feed can therefore turn a previously failing online restore into success, contrary to the PR's stated invariant that a reachable source decides the version and online selection remains unchanged. Restrict this fallback to the intended no-eligible-source/all-source-query-failed cases; the candidate result needs to track successful queries so mixed success/failure is distinguishable too.
src/winapp-CLI/WinApp.Cli.Tests/WorkspaceSetupServiceConfigModeTests.cs:109
This assertion does not prove the new delegation: the previous missing-yaml no-op also returned 0, so the test passes even if RunDotnetInheritedAsync is never called. Assert that the fake received exactly one invocation with the expected project path.

        Assert.AreEqual(0, result);

…restore prompting Three findings from the Copilot review. 1. The #762 cache fallback ran on every failure path, including the one where every eligible source WAS queried successfully and simply offers no satisfying version. That is an authoritative answer, so consulting the cache there let a version left behind by a different feed turn a failing online restore into a success -- exactly the resolution change this PR claimed not to make. It is now gated on candidates.Error is not null || EligibleSourceCount == 0, so only a source that could not answer enables it. 2. The .NET restore branch called init's interactive project picker, which opens a SelectionPrompt when a directory holds several projects. Restore runs non-interactively (CI, or straight after a clone) and has no project-selection option, so a multi-project repo would block on redirected input. It now restores every detected project without prompting, which is also what such a repo needs: restore only reinstalls what is already declared. 3. Restore_DotNetProject_WithoutYaml only asserted a zero exit code, which the pre-existing no-op branch also returns, so it could pass without any delegation happening. It now asserts exactly one dotnet restore naming the project, plus new coverage for the multi-project and failure cases. A direct negative test for (1) was written and then removed: under the full 32-way parallel run the local folder feed query intermittently fails, which legitimately enables the fallback, so the assertion could not be stabilized without masking the condition under test. The gate was verified by inverting it (the test failed as expected) before removal, and the class comment records why. The remaining test still pins that a reachable source's version wins over a lower cached one. Full suite: 4645 total, 4639 passed, 6 skipped, 0 failed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 40 out of 40 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/winapp-CLI/WinApp.Cli/Services/WorkspaceSetupService.cs:102

When --config-dir differs from the project directory, this delegated restore ignores the NuGet hierarchy selected above: dotnet restore is launched for/from projectToRestore.Directory and receives no config file from options.ConfigDir. As a result, the .NET path can restore from different feeds than the native path and contradicts the new documented restore --config-dir behavior. The delegated command must be given the selected NuGet configuration (while preserving its intended hierarchy), or this option must be rejected for this path.

From the Copilot review. The native restore path roots its NuGet hierarchy at options.ConfigDir via SetConfigRoot, but a delegated 'dotnet restore' resolves nuget.config relative to the project it is run for. So 'winapp restore --config-dir <dir>' on a .NET project silently ignored the selected configuration and could restore from different feeds than the native path given the same option. The selected config is now passed with --configfile, but only when the caller actually chose a different directory AND that directory supplies a nuget.config. In the default case (config dir == project dir) nothing is passed, so dotnet's normal nuget.config discovery -- including the user and machine levels -- stays intact; --configfile would otherwise replace that whole hierarchy with a single file and change behavior for everyone who never used the option. Tests cover both directions: the separate-config-dir case asserts the flag and path are passed, and the default case asserts they are not. Full suite: 4647 total, 4642 passed, 5 skipped, 0 failed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 40 out of 40 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/winapp-CLI/WinApp.Cli/Services/WorkspaceSetupService.cs:116

Passing --configfile here makes dotnet restore use only this one file, so the normal user/machine nuget.config hierarchy is discarded. With a source in the selected config but its credentials (or other required settings) in the user's config—the hierarchy the native path honors—this .NET restore fails authentication. Preserve the merged hierarchy when rooting discovery at ConfigDir (for example, by supplying a merged temporary config rather than the raw project-level file).
src/winapp-CLI/WinApp.Cli/Services/NugetService.Dependencies.cs:613
Local-folder NuGet resources can throw IOException or UnauthorizedAccessException while reading package metadata, but this catches only FatalProtocolException. That makes an unreadable first local source abort dependency resolution even when a later eligible source can provide the package. Include local source-access failures in the per-source failover path.
src/winapp-CLI/WinApp.Cli/Services/NugetPackageDownloader.cs:99
This catches only HTTP/protocol failures, but NuGet's local-folder resource can throw IOException or UnauthorizedAccessException while opening a package. If an earlier local source is unavailable/locked and a later source has the package, the exception escapes here instead of failing over as intended. Treat those source-access failures like FatalProtocolException while still preserving cancellation.

…failover Three findings from the Copilot review. 1. My previous commit forwarded the selected config directory as --configfile, which is worse than the problem it fixed: that switch REPLACES the whole nuget.config hierarchy with one file. Verified empirically -- restoring with --configfile pointing at a config listing one source dropped every user-level source, so a feed authenticated through credentials in the user config would start failing. Reverted to dotnet's standard project-relative discovery, which merges the project's ancestors with the user and machine levels and is what the user gets running dotnet restore by hand. When the selected config directory is not part of that hierarchy, winapp now says so instead of silently restoring from feeds the user did not select. A warning rather than an error on purpose: ConfigDir defaults to the CURRENT directory, so failing would break an ordinary 'winapp restore <dir>' run from an unrelated cwd. 2/3. The per-source failover in FetchDirectDependenciesAsync and DownloadPackageAsync caught only FatalProtocolException. A local-folder source reads packages straight off disk, so an unreadable or locked feed folder surfaces as IOException/UnauthorizedAccessException and aborted the whole operation instead of failing over to a source that could serve the package. Both now treat those as source failures, with cancellation still rethrown first. Full suite: 4647 total, 4641 passed, 6 skipped, 0 failed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 40 out of 40 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/winapp-CLI/WinApp.Cli/Services/WorkspaceSetupService.cs:83

This file is now 1,287 lines, well beyond the repository's ~1,000-line hard limit, and this new .NET-restore workflow adds another distinct responsibility. Extract the project discovery/restore loop (including hierarchy warning logic) into a dedicated service or helper so this change does not further expand the monolithic workspace setup service.
src/winapp-CLI/WinApp.Cli/Services/NugetSourceProvider.cs:99
The cache fingerprint omits source options that change repository behavior. Two config roots with the same name/URL but different protocolVersion (or TLS-validation policy) currently share DependencyCache; a graph resolved under one root can therefore be returned under a root where that source would use another protocol or fail certificate validation. Include these effective source properties in the fingerprint.

Two findings from the Copilot review. 1. The .NET restore branch pushed WorkspaceSetupService from 1,060 lines (its size on main) to 1,286, past the ~1,000-line hard limit in AGENTS.md, and added a distinct responsibility to an already-large service. Moved to DotNetProjectRestoreService behind IDotNetProjectRestoreService, following the interface + DI service pattern the guidance prescribes. The file is back to 1,215; the remainder is pre-existing content this PR did not author, so bringing it fully under the limit is a separate refactor. 2. NugetSourceProvider's config fingerprint keyed sources on name and URL only, so two roots declaring the same feed at a different protocolVersion (v2 vs v3), or differing on allowInsecureConnections, shared one DependencyCache entry -- and a graph resolved through one protocol could be served to a root that talks to the feed through another. Both properties are now part of the key. Full suite: 4647 total, 4641 passed, 6 skipped, 0 failed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 42 out of 42 changed files in this pull request and generated no new comments.

Copilot AI balanced review requested due to automatic review settings August 17, 2026 20:25

Copilot started reviewing on behalf of Alexandre Zollinger Chohfi (azchohfi) August 17, 2026 20:26 View session