fix: Windows Update install never applied updates (fake success)#609
Merged
Conversation
Install-WindowsUpdate received KB numbers prefixed with 'KB' (e.g. 'KB5034441') but its -KBArticleID parameter expects bare digits, so the cmdlet matched zero updates and exited silently. Updates without a KB (Defender Definitions, drivers) and updates with multiple KBs were also excluded by the selection filter. The status bar reported a fabricated 'Installed N update(s)' message based on the selection count rather than the cmdlet's actual result. Switch to a Title-based install pipeline that works for all updates including those without a KB. Capture and parse Install-WindowsUpdate output to report real counts and per-row outcomes. Unify the update list: 'List updates' now returns Standard + Feature upgrades + Hidden in one grouped table; the separate 'Feature upgrades' button is removed since it is no longer needed. - ListUpdatesAsync: single scan for Standard/Feature/Hidden, categorized (Security, Cumulative, Defender, Driver, Servicing, .NET, Feature upgrade, Hidden) - InstallUpdatesAsync: title array injected with single-quote escaping; pipeline matches by Title against the live feed; output captured via __RESULT__: marker and parsed into per-title results - ParseInstallResults: pure static helper, fully unit-tested - ApplyInstallResults: updates each row's Status (Installed/Failed/Not applied) so the DataGrid reflects reality - UpdateEntry.Status: now an ObservableProperty so per-row status updates during/after install - Status bar shows real counts: 'Installed X/Y. Failed: Z. Not applied: W.' - WindowsUpdateView.xaml: 'Feature upgrades' button removed - 8 new ParseInstallResults tests; ListFeatureUpdatesCommand removed from command-existence theory Closes the silent-success bug reported during a 20-update install where the UI claimed success but the next scan returned the same 20 updates.
📝 WalkthroughWalkthroughVersion 1.17.4 refactors the Windows Update feature to unify listing (consolidating feature upgrades and adding categorization), replace KB-based installation with title-based matching, parse JSON install results to report accurate outcomes, and enable observable status notifications per-update. ChangesWindows Update Installation Pipeline Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+466
to
+475
| if (results.TryGetValue(entry.Title, out var result) && !string.IsNullOrWhiteSpace(result)) | ||
| { | ||
| entry.Status = IsInstalledResult(result) ? "Installed" | ||
| : IsFailedResult(result) ? "Failed" | ||
| : result; | ||
| } | ||
| else | ||
| { | ||
| entry.Status = "Not applied"; | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@SysManager/SysManager/ViewModels/WindowsUpdateViewModel.cs`:
- Around line 367-373: The current script sets $matched by querying only the
standard feed first and only queries the 'Upgrades' feed if nothing matched,
causing mixed selections (normal update + feature upgrade) to miss the upgrade;
modify the logic so you query both feeds up front, combine their results, then
apply the filter by titles. Specifically, replace the sequential
Get-WindowsUpdate calls with two queries (the standard feed and the -UpdateType
Software -Category 'Upgrades' feed), merge their outputs (e.g., add or union the
collections) and then run Where-Object { $titles -contains $_.Title } to
populate $matched (instead of populating $matched from only the first query and
falling back to $up).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b9fb967f-22c9-4eb8-b139-7b6b4e347a88
📒 Files selected for processing (7)
CHANGELOG.mdREADME.mdSysManager/SysManager.Tests/WindowsUpdateViewModelTests.csSysManager/SysManager/Models/UpdateEntry.csSysManager/SysManager/SysManager.csprojSysManager/SysManager/ViewModels/WindowsUpdateViewModel.csSysManager/SysManager/Views/WindowsUpdateView.xaml
💤 Files with no reviewable changes (1)
- SysManager/SysManager/Views/WindowsUpdateView.xaml
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build & unit tests
- GitHub Check: Analyze (csharp)
🔇 Additional comments (6)
SysManager/SysManager/SysManager.csproj (1)
13-15: LGTM!CHANGELOG.md (1)
9-18: LGTM!README.md (1)
125-137: LGTM!SysManager/SysManager/Models/UpdateEntry.cs (1)
14-15: LGTM!SysManager/SysManager/ViewModels/WindowsUpdateViewModel.cs (1)
420-477: LGTM!SysManager/SysManager.Tests/WindowsUpdateViewModelTests.cs (1)
208-294: LGTM!
Comment on lines
+367
to
+373
| $matched = Get-WindowsUpdate -MicrosoftUpdate -ErrorAction SilentlyContinue | | ||
| Where-Object {{ $titles -contains $_.Title }} | ||
| if (-not $matched -or @($matched).Count -eq 0) {{ | ||
| $up = Get-WindowsUpdate -MicrosoftUpdate -UpdateType Software -Category 'Upgrades' -ErrorAction SilentlyContinue | | ||
| Where-Object {{ $titles -contains $_.Title }} | ||
| if ($up) {{ $matched = $up }} | ||
| }} |
There was a problem hiding this comment.
Query both feeds before filtering selected titles.
Line 367 only searches the standard feed, and Lines 369-373 only fall back to upgrades when nothing matched. If a user selects a normal update and a feature upgrade together, the standard match makes $matched non-empty, so the feature upgrade is skipped and later shows as Not applied.
Suggested fix
- $matched = Get-WindowsUpdate -MicrosoftUpdate -ErrorAction SilentlyContinue |
- Where-Object { $titles -contains $_.Title }
- if (-not $matched -or @($matched).Count -eq 0) {
- $up = Get-WindowsUpdate -MicrosoftUpdate -UpdateType Software -Category 'Upgrades' -ErrorAction SilentlyContinue |
- Where-Object { $titles -contains $_.Title }
- if ($up) { $matched = $up }
- }
+ $standard = @(Get-WindowsUpdate -MicrosoftUpdate -ErrorAction SilentlyContinue)
+ $upgrades = @(Get-WindowsUpdate -MicrosoftUpdate -UpdateType Software -Category 'Upgrades' -ErrorAction SilentlyContinue)
+ $matched = @($standard + $upgrades) |
+ Where-Object { $titles -contains $_.Title } |
+ Sort-Object Title -Unique🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SysManager/SysManager/ViewModels/WindowsUpdateViewModel.cs` around lines 367
- 373, The current script sets $matched by querying only the standard feed first
and only queries the 'Upgrades' feed if nothing matched, causing mixed
selections (normal update + feature upgrade) to miss the upgrade; modify the
logic so you query both feeds up front, combine their results, then apply the
filter by titles. Specifically, replace the sequential Get-WindowsUpdate calls
with two queries (the standard feed and the -UpdateType Software -Category
'Upgrades' feed), merge their outputs (e.g., add or union the collections) and
then run Where-Object { $titles -contains $_.Title } to populate $matched
(instead of populating $matched from only the first query and falling back to
$up).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Windows Update silently failed to install anything while reporting success. Reproduced live: select 20 updates, click Install — the UI shows "Installed 20 update(s)", live console is empty, and a re-scan immediately returns the same 20 updates. Reported by user during v1.17.3 testing.
Root cause
Three compounding bugs introduced in PR #473 (
feat: add individual update selection):KBArticleIDsare joined as"KB" + idssoUpdateEntry.KBholds e.g."KB5034441". At install time, the same string is passed toInstall-WindowsUpdate -KBArticleID 'KB5034441'. PSWindowsUpdate's-KBArticleIDexpects bare digits ('5034441'). With the prefix, the cmdlet matches zero updates and exits silently.u.KB.All(c => char.IsLetterOrDigit(c))rejects:"KB5034441,5034442"— comma is not a letter/digit)StatusMessage = $"Installed {selected.Count} update(s)"was hardcoded from the selection count, never from the cmdlet's actual result.Combined: when the user selects 20 updates, the filter quietly drops most of them, sends a malformed
KBArticleIDfor the rest, the cmdlet installs nothing, and the UI claims full success.Fix
Install-WindowsUpdateoutput is emitted as JSON via a__RESULT__:marker and parsed into per-titleInstalled/Failed/Downloadedoutcomes."Installed X/Y. Failed: Z. Not applied: W."derived from actual results, not selection count.UpdateEntry.Statusis now anObservableProperty; each row's Status column updates as the install progresses (Installing…→Installed/Failed/Not applied).Files changed
SysManager/SysManager/ViewModels/WindowsUpdateViewModel.cs— unified scan, Title-based install, result parserSysManager/SysManager/Models/UpdateEntry.cs— Status now ObservablePropertySysManager/SysManager/Views/WindowsUpdateView.xaml— removed "Feature upgrades" buttonSysManager/SysManager.Tests/WindowsUpdateViewModelTests.cs— 8 new ParseInstallResults testsCHANGELOG.md,README.md, csproj version → 1.17.4Test plan
dotnet build -c ReleaseSysManager.csproj — 0 errorsdotnet build -c ReleaseSysManager.Tests.csproj — 0 errorsParseInstallResults(empty, all installed, mixed, succeeded, error, invalid JSON, single object)Summary by CodeRabbit
Release Notes v1.17.4
New Features
Bug Fixes
Chores