Add daily update check and upgrade command to handle upgrading from CLI#424
Add daily update check and upgrade command to handle upgrading from CLI#424
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a daily “new version available” notification and a winapp upgrade command so the CLI can guide (and in some cases perform) self-upgrades based on install channel, addressing issue #392.
Changes:
- Introduces
ICliUpgradeService/CliUpgradeServiceand wires it into startup to check for updates once per day. - Adds a new
upgradecommand and registers it in the root command/help categories and CLI schema output. - Updates the npm-generated command wrappers (including
run --symbols) and refreshes schema references.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| src/winapp-npm/src/winapp-commands.ts | Regenerated npm command wrappers/docs; adds run --symbols and updates manifest wording. |
| src/winapp-CLI/WinApp.Cli/Services/ICliUpgradeService.cs | New interface + InstallChannel enum for upgrade/update-check behavior. |
| src/winapp-CLI/WinApp.Cli/Services/CliUpgradeService.cs | Implements daily update check + upgrade flows for MSIX/exe and instructions for npm/NuGet. |
| src/winapp-CLI/WinApp.Cli/Program.cs | Invokes update check during startup (non-quiet, non-json/schema modes). |
| src/winapp-CLI/WinApp.Cli/NativeMethods.txt | Adds GetCurrentPackageFullName for MSIX identity detection. |
| src/winapp-CLI/WinApp.Cli/Helpers/HostBuilderExtensions.cs | Registers CliUpgradeService and hooks up UpgradeCommand handler. |
| src/winapp-CLI/WinApp.Cli/Commands/WinAppRootCommand.cs | Adds upgrade to root command and “Setup” help category. |
| src/winapp-CLI/WinApp.Cli/Commands/UpgradeCommand.cs | New winapp upgrade command delegating to ICliUpgradeService. |
| src/winapp-CLI/WinApp.Cli.Tests/CliUpgradeServiceTests.cs | Adds tests for install channel detection, cache file behavior, and command registration. |
| docs/cli-schema.json | Adds upgrade command metadata to the published CLI schema. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var lines = await File.ReadAllLinesAsync(cacheFile.FullName, cancellationToken); | ||
| if (lines.Length >= 1 | ||
| && DateTime.TryParse(lines[0], out var lastCheck) | ||
| && (DateTime.UtcNow - lastCheck).TotalHours < CheckIntervalHours) | ||
| { |
There was a problem hiding this comment.
The cache timestamp is written using the round-trip (O) format, but it’s parsed with DateTime.TryParse(...) which is culture-dependent and may interpret the value with an unexpected Kind. Consider parsing with CultureInfo.InvariantCulture + DateTimeStyles.RoundtripKind (or using DateTimeOffset) to ensure the 24h comparison is correct across locales.
| if (!quiet) | ||
| { | ||
| var cliUpgradeService = serviceProvider.GetRequiredService<ICliUpgradeService>(); | ||
| await cliUpgradeService.CheckAndNotifyAsync(); |
There was a problem hiding this comment.
CheckAndNotifyAsync() is awaited during startup without passing a cancellation token or enforcing a short timeout. If GitHub is slow/unreachable on first run (no cache yet), this can noticeably delay every CLI command. Consider linking a CancellationTokenSource with a small timeout inside the update check (or run it fire-and-forget) so the CLI remains responsive.
| await cliUpgradeService.CheckAndNotifyAsync(); | |
| try | |
| { | |
| await cliUpgradeService.CheckAndNotifyAsync().WaitAsync(TimeSpan.FromSeconds(2)); | |
| } | |
| catch (TimeoutException) | |
| { | |
| // Keep startup responsive if the update check is slow/unreachable. | |
| } | |
| catch (OperationCanceledException) | |
| { | |
| // Keep startup responsive if the timed wait is canceled. | |
| } |
| // Check caller env var (set by wrapper scripts via --caller option) | ||
| var caller = Environment.GetEnvironmentVariable("WINAPP_CLI_CALLER"); | ||
| if (string.Equals(caller, "npm", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return InstallChannel.Npm; | ||
| } | ||
| if (string.Equals(caller, "nuget-package", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return InstallChannel.NuGet; | ||
| } |
There was a problem hiding this comment.
DetectInstallChannel() checks WINAPP_CLI_CALLER for the literal value "npm", but the Node.js wrapper sets WINAPP_CLI_CALLER to "nodejs-package" (see src/winapp-npm/src/winapp-cli-utils.ts). This means the install channel can be misdetected when the exe path heuristics don't match. Consider accepting the actual wrapper values (e.g., "nodejs-package") or aligning the wrapper/CLI caller strings.
| var result = Windows.Win32.PInvoke.GetCurrentPackageFullName(ref length, null); | ||
| // ERROR_INSUFFICIENT_BUFFER (122) means the app has package identity | ||
| // APPMODEL_ERROR_NO_PACKAGE (15700) means it does not | ||
| return result == Windows.Win32.Foundation.WIN32_ERROR.ERROR_INSUFFICIENT_BUFFER; |
There was a problem hiding this comment.
HasMsixPackageIdentity() calls Windows.Win32.PInvoke.GetCurrentPackageFullName(ref length, null) outside an unsafe context. CsWin32 typically generates pointer-based overloads for this API, which require an unsafe block even when passing null. Consider wrapping this call in unsafe { ... } (like LongPathHelper does for other P/Invokes), or using a safe overload (e.g., an empty Span<char>) if one is generated.
| var result = Windows.Win32.PInvoke.GetCurrentPackageFullName(ref length, null); | |
| // ERROR_INSUFFICIENT_BUFFER (122) means the app has package identity | |
| // APPMODEL_ERROR_NO_PACKAGE (15700) means it does not | |
| return result == Windows.Win32.Foundation.WIN32_ERROR.ERROR_INSUFFICIENT_BUFFER; | |
| unsafe | |
| { | |
| var result = Windows.Win32.PInvoke.GetCurrentPackageFullName(ref length, null); | |
| // ERROR_INSUFFICIENT_BUFFER (122) means the app has package identity | |
| // APPMODEL_ERROR_NO_PACKAGE (15700) means it does not | |
| return result == Windows.Win32.Foundation.WIN32_ERROR.ERROR_INSUFFICIENT_BUFFER; | |
| } |
| /** Format output as JSON */ | ||
| json?: boolean; | ||
| /** Path to the appxmanifest.xml (default: auto-detect from input folder or current directory) */ | ||
| /** Path to the Package.appxmanifest (default: auto-detect from input folder or current directory) */ |
There was a problem hiding this comment.
The generated wrapper types/documentation now imply --manifest for run only accepts Package.appxmanifest, but the native CLI help still describes --manifest as appxmanifest.xml (and the implementation supports both via manifest auto-detection). To avoid misleading consumers, consider updating the schema generation/description to mention both appxmanifest.xml and Package.appxmanifest.
| /** Path to the Package.appxmanifest (default: auto-detect from input folder or current directory) */ | |
| /** Path to the app manifest file, such as appxmanifest.xml or Package.appxmanifest (default: auto-detect from input folder or current directory) */ |
| /** Format output as JSON */ | ||
| json?: boolean; | ||
| /** Path to the appxmanifest.xml (default: auto-detect from current directory) */ | ||
| /** Path to the Package.appxmanifest (default: auto-detect from current directory) */ |
There was a problem hiding this comment.
The generated wrapper types/documentation now imply --manifest for unregister only accepts Package.appxmanifest, but the native CLI help/implementation supports appxmanifest.xml as well. Consider updating the schema generation/description to mention both filenames to avoid breaking/misleading callers.
| /** Path to the Package.appxmanifest (default: auto-detect from current directory) */ | |
| /** Path to the app manifest file, such as Package.appxmanifest or appxmanifest.xml (default: auto-detect from current directory) */ |
| // Check for CLI updates (at most once per day, silent on failure) | ||
| if (!quiet) | ||
| { | ||
| var cliUpgradeService = serviceProvider.GetRequiredService<ICliUpgradeService>(); | ||
| await cliUpgradeService.CheckAndNotifyAsync(); | ||
| } |
There was a problem hiding this comment.
The update check runs before parsing args and before --caller is applied to WINAPP_CLI_CALLER (later at lines 116-121). That means install-channel detection during notifications can be wrong for wrapper scenarios that rely on --caller rather than path heuristics. Consider parsing args (or at least extracting --caller) before calling CheckAndNotifyAsync, or passing the caller into the upgrade service explicitly.
| { | ||
| case InstallChannel.Npm: | ||
| logger.LogInformation("winapp was installed via npm. To upgrade, run:"); | ||
| logger.LogInformation(" npm update -g @anthropic-ai/winappcli"); |
There was a problem hiding this comment.
The npm upgrade instructions use the package name @anthropic-ai/winappcli, but this repo’s npm package is @microsoft/winappcli (see src/winapp-npm/package.json). This will direct users to the wrong package when running winapp upgrade or reading the update notification.
| logger.LogInformation(" npm update -g @anthropic-ai/winappcli"); | |
| logger.LogInformation(" npm update -g @microsoft/winappcli"); |
| var upgradeHint = channel switch | ||
| { | ||
| InstallChannel.Npm => "npm update -g @anthropic-ai/winappcli", | ||
| InstallChannel.NuGet => "update the Microsoft.Windows.SDK.BuildTools.WinApp NuGet package", | ||
| _ => "winapp upgrade" | ||
| }; | ||
|
|
||
| AnsiConsole.MarkupLine($"[yellow]v{newVersion} is available. Run `{Markup.Escape(upgradeHint)}` to update.[/]"); |
There was a problem hiding this comment.
The update notification hint for npm installs also references @anthropic-ai/winappcli, but the npm package in this repo is @microsoft/winappcli. The hint should match the actual package name users have installed.
Build Metrics ReportBinary Sizes
Test Results✅ 739 passed out of 739 tests in 391.6s (+17 tests, +37.6s vs. baseline) Test Coverage❌ 20.6% line coverage, 34.7% branch coverage · ✅ no change vs. baseline CLI Startup Time36ms median (x64, Updated 2026-04-16 02:05:19 UTC · commit |
|
@copilot apply changes based on the comments in this thread |
Agent-Logs-Url: https://github.com/microsoft/winappCli/sessions/26b6a1b4-d856-4da4-afa7-e2ef81662b68 Co-authored-by: zateutsch <88554871+zateutsch@users.noreply.github.com>
Implemented the requested review-thread fixes in commit d62b629. This includes startup timeout + early caller parsing in Program, culture-safe update-cache timestamp parsing, nodejs-package caller detection, corrected npm upgrade package name, unsafe package-identity P/Invoke usage, network-isolated upgrade service tests, and regenerated schema/npm wrapper docs (including manifest wording alignment and the upgrade-options generator lint fix). Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot apply changes based on the comments in this thread |
Agent-Logs-Url: https://github.com/microsoft/winappCli/sessions/96407090-ac13-4e42-b85d-cf2da3d95598 Co-authored-by: zateutsch <88554871+zateutsch@users.noreply.github.com>
Applied the requested review-thread updates in commit 0ed5dfb. This re-aligns |
fixes #392