Fix error where update notice checked prerelease versions incorrectly and improperly references cached test output#541
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes the update-notification logic when running prerelease builds and adds a safety check against malformed/test-artifact cache content.
Changes:
- Compare cached “latest” vs an “effective” current version for prerelease builds (core version only).
- Detect and purge “unreasonable” cached versions likely produced by tests.
- Update and extend unit tests for the new version parsing/sanity logic.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/winapp-CLI/WinApp.Cli/Services/UpdateNotificationService.cs | Adds prerelease/core-version helpers and purges unreasonable cached versions before notifying. |
| src/winapp-CLI/WinApp.Cli.Tests/UpdateNotificationServiceTests.cs | Updates fixtures away from extreme versions and adds unit tests for new helper methods + cache purge behavior. |
| src/winapp-CLI/WinApp.Cli.Tests/UpdateNotificationGatingTests.cs | Updates gating seed version to avoid being purged by new “unreasonable version” logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Build Metrics ReportBinary Sizes
Test Results✅ 1010 passed, 1 skipped out of 1011 tests in 427.4s (+17 tests, -38.0s vs. baseline) Test Coverage❌ 16.7% line coverage, 34.8% branch coverage · ✅ no change vs. baseline CLI Startup Time42ms median (x64, Updated 2026-05-21 15:23:32 UTC · commit |
|
@copilot apply changes based on the comments in this thread |
Agent-Logs-Url: https://github.com/microsoft/winappCli/sessions/bc7a5cfe-999d-4d8e-917a-87e70759b0f0 Co-authored-by: zateutsch <88554871+zateutsch@users.noreply.github.com>
Implemented the requested fixes in 6f9b769: prerelease/stable comparison now uses full SemVer ordering (so stable of same core is treated as newer than prerelease), unreasonable cache-version detection is now relative to current major (with v/V prefix handling), brittle hardcoded newer-version tests were made dynamic, and new end-to-end 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:
|
nmetulev
left a comment
nmetulev
left a comment
There was a problem hiding this comment.
Copilot PR review — multi-dimensional
Summary: Critical 0 · High 1 · Medium 5 · Low 3
| Dimension | Status |
|---|---|
| security | ✓ clean |
| correctness | ⚠ 1 high |
| cli-ux | ⚠ 1 low |
| alternative-solution | ⚠ 2 medium |
| test-coverage | ⚠ 4 findings |
| docs-and-samples | ✓ clean |
| packaging | ✓ clean |
| multi-model (GPT-5.4 cross-check) | 1 confirmed, 1 downgraded |
Findings at a glance
| ID | File:lines | Domain | Summary |
|---|---|---|---|
| H1 | Services/UpdateNotificationService.cs:55-62 |
correctness | Cache wipe preserves LastCheck → user gets no update notice for ~24h after a poisoned-cache encounter (regresses the fix) |
| M1 | Tests/UpdateNotificationGatingTests.cs:47-60 |
test-coverage | Setup nulls CI/GITHUB_ACTIONS/TF_BUILD without saving them; leaks across in-process tests |
| M2 | Tests/UpdateNotificationServiceTests.cs:478-499 |
test-coverage | IsUnreasonableVersion has no test for unparseable (corrupt-cache) input |
| M3 | Tests/UpdateNotificationServiceTests.cs:586-602 |
test-coverage | Wipe test doesn't assert LastCheck / LastShownDate outcome |
| M4 | Services/UpdateNotificationService.cs:202-263 |
alternative-solution | GetCoreVersion/TryParseMajorVersion duplicate splitter logic already in TryParseSemVer |
| M5 | Services/UpdateNotificationService.cs:55-67 |
alternative-solution | CheckAndNotify parses each version string 2-3× across consecutive helpers |
| L1 | Services/UpdateNotificationService.cs:186-196 |
test-coverage / alternative-solution | IsPreReleaseVersion is added with 6 unit tests but has zero production callers |
| L2 | Services/UpdateNotificationService.cs:55-62 |
cli-ux | Silent cache mutation has no LogDebug breadcrumb |
| L3 | Tests/UpdateNotificationServiceTests.cs |
test-coverage | No test pinning CurrentVersionProvider throw → swallow behavior |
Details
H1 — src/winapp-CLI/WinApp.Cli/Services/UpdateNotificationService.cs:55-62 · correctness · multi-model: confirmed
When IsUnreasonableVersion fires on a poisoned cache (e.g. leftover 999.0.0 from a prior test run), CheckAndNotify wipes LatestVersion and LastShownDate but preserves LastCheck. The very next gate at lines 77-80 (!cache.LastCheck.HasValue || (UtcNow - LastCheck).TotalHours >= 24) then short-circuits to false, so the background refresh is skipped.
Net effect: the false notification goes away (good), but the user receives no real update notification for up to 24 hours — silently regressing the fix this PR exists to deliver.
// Line 60 — LastCheck intentionally omitted:
cache = cache with { LatestVersion = "", LastShownDate = "" };The new test CheckAndNotify_UnreasonableCachedVersion_DiscardsAndNoNotification only asserts no notification on the first call and can't catch the missed refresh because SkipBackgroundRefreshForTesting = true.
Recommendation: Also reset LastCheck:
cache = cache with { LastCheck = null, LatestVersion = "", LastShownDate = "" };Then extend the test to assert LastCheck is null afterward, and ideally add a second test with SkipBackgroundRefreshForTesting = false + a stubbed GetLatestVersionAsync that verifies the refresh actually runs.
M1 — src/winapp-CLI/WinApp.Cli.Tests/UpdateNotificationGatingTests.cs:47-60 · test-coverage
Setup explicitly nulls CI / GITHUB_ACTIONS / TF_BUILD to defeat CIEnvironmentDetectorForTelemetry, but Cleanup only restores WINAPP_CLI_CACHE_DIRECTORY / WINAPP_CLI_CALLER / WINAPP_CLI_UPDATE_CHECK. Contrast UpdateNotificationServiceTests.cs:22-28,67-70 which snapshots and restores all 13 CiVarNames. On CI runners (CI=true), this test class permanently clears those vars for the rest of the test process, silently changing the behavior of subsequent CI-detection tests in the same run.
Recommendation: Mirror the _savedCiVars pattern (or extract it into BaseCommandTests / a shared fixture) so both classes snapshot and restore the same set.
M2 — src/winapp-CLI/WinApp.Cli.Tests/UpdateNotificationServiceTests.cs:478-499 · test-coverage
IsUnreasonableVersion returns false when either side fails to parse. The new tests cover normal / high / test-artifact / boundary / above-threshold / v-prefix, but never exercise the realistic "corrupt cache file" branch ("garbage", "", "1", "1.x"). That's exactly the failure mode the guard is supposed to backstop.
Recommendation: Add IsUnreasonableVersion_UnparseableCached_ReturnsFalse and IsUnreasonableVersion_UnparseableCurrent_ReturnsFalse, plus a CheckAndNotify_GarbageCachedVersion_DoesNotNotifyOrThrow integration test.
M3 — src/winapp-CLI/WinApp.Cli.Tests/UpdateNotificationServiceTests.cs:586-602 · test-coverage
The new wipe test asserts cache.LatestVersion == "" but does not assert LastShownDate is cleared, and (per H1) does not pin LastCheck behavior. Whichever way H1 is resolved, this test should pin the intended LastCheck outcome so a future refactor can't silently regress it.
Recommendation: Also assert cache.LastShownDate == "" and the intended LastCheck value (null after H1 fix, or preserved if the team prefers to throttle refreshes).
M4 — src/winapp-CLI/WinApp.Cli/Services/UpdateNotificationService.cs:202-263 · alternative-solution
GetCoreVersion (202-212) and TryParseMajorVersion (229-240) reimplement the +/- stripping already inside the nested TryParseSemVer local in IsNewerVersion (244-263). Three copies of the same SemVer-decomposition rules in one file; future fixes have to be remembered in all three places.
Recommendation: Promote TryParseSemVer from a nested local to a private static method and implement GetCoreVersion / TryParseMajorVersion in terms of it:
return TryParseSemVer(v, out var core, out _) ? core.Major : 0;M5 — src/winapp-CLI/WinApp.Cli/Services/UpdateNotificationService.cs:55-67 · alternative-solution
CheckAndNotify calls IsUnreasonableVersion(latest, current) then IsNewerVersion(latest, current) back-to-back, parsing each string 2-3 times. Once TryParseSemVer is promoted (M4), parse each string once and pass Version objects to a tiny IsUnreasonable(Version, Version) predicate — or inline it, since it has exactly one caller.
L1 — src/winapp-CLI/WinApp.Cli/Services/UpdateNotificationService.cs:186-196 · test-coverage / alternative-solution · multi-model: downgraded from high → low
internal static bool IsPreReleaseVersion(string) is added with six dedicated unit tests but has zero production callers. The PR title says "prerelease versions checked incorrectly", but the actual prerelease behavior is delivered entirely by the unchanged IsNewerVersion/ComparePreRelease path. A future reader will reasonably assume this helper IS the prerelease fix — it isn't.
Recommendation: Delete the method and its six tests. If a future caller actually needs the distinction (e.g. to suppress notifications when on a prerelease against an equal-core stable), add it back together with the production code path that uses it.
L2 — src/winapp-CLI/WinApp.Cli/Services/UpdateNotificationService.cs:55-62 · cli-ux
The cache-wipe branch silently mutates user state on disk with no log entry. A user troubleshooting under --verbose has no breadcrumb. The service already has ILogger<UpdateNotificationService> injected, so the cost is one line:
logger.LogDebug("Discarded unreasonable cached update version {Version}", cache.LatestVersion);L3 — src/winapp-CLI/WinApp.Cli.Tests/UpdateNotificationServiceTests.cs · test-coverage
The new CurrentVersionProvider test seam is invoked inside CheckAndNotify's silent outer try/catch. No test pins "provider throws → no crash, no cache mutation, no notification".
Recommendation: Add CheckAndNotify_CurrentVersionProviderThrows_DoesNotCrashAndDoesNotNotify.
Coverage notes
- security: Verified no new
Process.Start/ HTTP / cert / manifest / regex / deserialization /NoWarnsurface. Cache path and atomic-write pattern unchanged. New helpers operate on local strings only. - docs-and-samples: No
Commands/*.cschanged →cli-schema.jsonregen not needed.docs/usage.md"Update Checks" section documents the opt-out env var only; no described behavior contradicted by this PR. - packaging: Lands uniformly in all distribution channels (native / npm / NuGet / VSC) via the shared CLI binary. No version bump needed (Nerdbank.GitVersioning is height-based).
- multi-model: GPT-5.4 cross-checked H1 (confirmed) and the original H2 (downgraded to L1). Independently scanned the static
_refreshScheduledinterlock — no additional critical/high findings.
Blocker: H1 — without also resetting LastCheck, the fix suppresses the false notification but then suppresses real ones too for ~24h.
fixes #538
Changes: