Add weekly orphaned installer patch monitor for NinjaRMM#43
Open
AlrightLad wants to merge 7 commits intoDTC-Inc:mainfrom
Open
Add weekly orphaned installer patch monitor for NinjaRMM#43AlrightLad wants to merge 7 commits intoDTC-Inc:mainfrom
AlrightLad wants to merge 7 commits intoDTC-Inc:mainfrom
Conversation
Adds Monitor-InstallerPatches.ps1 — a read-only NinjaRMM scheduled script that detects orphaned .msi/.msp files accumulating in C:\Windows\Installer. This folder is not cleaned by Disk Cleanup or DISM and can silently grow to consume entire drives on long-lived dental workstations (ref: HALO Ticket 1125653 — 128 GB orphaned patches, 96.1% disk utilization at Guardian Dentistry Partners). How orphan detection works: The script queries the Windows Installer registry database at HKLM:\SOFTWARE\Microsoft\Windows\CurrentVersion\Installer\UserData\S-1-5-18\Products and \Patches, collecting every LocalPackage value to build a set of referenced cached installers. Any .msi/.msp file in C:\Windows\Installer not in that referenced set is classified as orphaned. This avoids the Win32_Product WMI class entirely, which triggers a reconfigure/consistency check on every installed MSI and can take 20+ minutes while destabilizing applications. What the script reports: Total installer folder size, orphaned file count and size, referenced file count and size $PatchCache$ subfolder size WinSxS size as a secondary disk consumption indicator Status classification: Healthy (<20 GB), Warning (20–50 GB), Critical (>50 GB), or Error on scan failure Where results are written: Six NinjaRMM custom fields via Ninja-Property-Set (graceful no-op outside NinjaRMM runtime) Windows Application Event Log under source DTC-InstallerMonitor (Event ID 1000 = Healthy, 2000 = Warning/Critical, 3000 = Error) Console output for NinjaRMM script activity log Safety guarantees: Makes zero filesystem changes — no files created, moved, or deleted All thresholds defined as variables at script top for easy adjustment Self-contained with no external dependencies (PowerShell 5.1 only) Designed to run as SYSTEM on Windows 10, 11, Server 2019, and Server 2022
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new PowerShell Monitor (Monitor-InstallerPatches) that weekly, read-only scans C:\Windows\Installer, enumerates registry-referenced Products/Patches, classifies on-disk Changes
Sequence Diagram(s)sequenceDiagram
participant Script as Script (Monitor-InstallerPatches)
participant Reg as Registry (Products/Patches)
participant FS as FileSystem (C:\Windows\Installer)
participant Est as Estimators (PatchCache / WinSxS)
participant Ninja as NinjaRMM (optional)
participant Event as Windows Event Log
Script->>Reg: Query referenced Products and Patches
Reg-->>Script: Return referenced file list (and errors)
Script->>FS: Enumerate Installer directory files
FS-->>Script: Return on-disk file list
Script->>Script: Classify files -> Referenced vs Orphaned, compute counts & sizes
Script->>Est: Request PatchCache and WinSxS estimates
Est-->>Script: Return size estimates
Script->>Script: Compute health status using thresholds
Script->>Ninja: (optional) Write custom fields
Ninja-->>Script: Ack / error
Script->>Event: Emit structured event with results & errors
Event-->>Script: Ack / error
Script-->>Console: Output PSCustomObject summary and details
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
rmm-ninja/Monitor-InstallerPatches (3)
199-202: WinSxS recursive enumeration over-reports size and is I/O-heavy.Two concerns with
Get-ChildItem -Recurseon WinSxS:
- Correctness: WinSxS uses hardlinks extensively.
Measure-Object Length -Sumcounts each hardlink at full file size, significantly over-reporting actual disk usage (often 2–3×).- Performance: WinSxS routinely contains 80k–150k+ files. Recursively enumerating it generates substantial disk I/O, which matters even at 2 AM on busy servers.
Consider using
Dism.exe /Online /Cleanup-Image /AnalyzeComponentStoreorGet-WindowsPackagefor an accurate, OS-supported WinSxS size, or drop the metric if it's only a secondary indicator.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmm-ninja/Monitor-InstallerPatches` around lines 199 - 202, The current measurement using Get-ChildItem -Recurse and Measure-Object (variables $winsxsSize and $winsxsSizeGB) overcounts due to hardlinks and is I/O-heavy; replace this approach by invoking an OS-supported query (e.g., call DISM.exe /Online /Cleanup-Image /AnalyzeComponentStore or use Get-WindowsPackage/Get-WindowsOptionalFeature) to obtain the WinSxS/component store size, parse its output into $winsxsSizeGB, or remove the metric entirely if it’s only a secondary indicator; ensure you stop using recursive enumeration of C:\Windows\WinSxS and update any downstream references to $winsxsSize/$winsxsSizeGB accordingly.
246-266: Event ID and EntryType don't distinguish Warning from Critical, and Error is logged asInformation.
- Lines 248–249: Both
"Warning"and"Critical"map to event ID2000, reducing alerting granularity. Consider giving Critical its own ID (e.g.,2500or3000, shifting Error to9000).- Line 266:
-EntryType Informationis used for every status, including"Critical"and"Error". Monitoring tools that filter onEntryTypewill miss these. Map the entry type to match the status:Proposed fix
+$entryType = switch ($status) { + "Healthy" { "Information" } + "Warning" { "Warning" } + "Critical" { "Error" } + "Error" { "Error" } +} ... - Write-EventLog -LogName $logName -Source $source -EventId $eventId -EntryType Information -Message $message + Write-EventLog -LogName $logName -Source $source -EventId $eventId -EntryType $entryType -Message $message🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmm-ninja/Monitor-InstallerPatches` around lines 246 - 266, The event mapping currently collapses Warning and Critical to the same $eventId and always uses -EntryType Information; update the switch that sets $eventId (the switch on $status) to assign distinct IDs for "Warning" (e.g., 2000), "Critical" (e.g., 2500 or 3000) and "Error" (a high/unique ID) and add a new variable (e.g., $entryType) that maps $status to the proper EventLogEntryType values ("Information" for Healthy, "Warning" for Warning, "Error" for Critical and Error as appropriate), then pass -EntryType $entryType to Write-EventLog so the event ID and entry type both reflect the actual $status.
148-163:Measure-Object -Sumreturns$nullfor empty collections.When no files match (e.g., empty Installer directory or no orphaned/referenced files),
.Sumis$nullrather than0. PowerShell's arithmetic coercion to0makes downstream math work today, but this is fragile — e.g., serializing these values or comparing them with-eq 0would behave unexpectedly.A defensive one-liner pattern:
# coerce $null → 0 explicitly $patchCacheSize = ($items | Measure-Object Length -Sum).Sum -as [long] # or: $patchCacheSize = [long]($items | Measure-Object Length -Sum).SumEither form converts
$nullto0Lexplicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmm-ninja/Monitor-InstallerPatches` around lines 148 - 163, Measure-Object .Sum can be $null for empty collections causing fragile values; update all places that use (.Sum) in this file (e.g., the $patchCacheSize assignment and the PSCustomObject properties TotalSizeBytes, ReferencedSizeBytes, OrphanedSizeBytes which use ($allFiles | Measure-Object Length -Sum).Sum and similar expressions) to explicitly coerce $null → 0 by casting or using -as [long] (for example replace X.Sum with X.Sum -as [long] or [long](X.Sum)); apply the same coercion pattern consistently for any other Measure-Object .Sum usages in Monitor-InstallerPatches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rmm-ninja/Monitor-InstallerPatches`:
- Around line 258-261: Get-OrphanedInstallerFiles can throw leaving $results
null so member access like $results.ScanDuration.TotalSeconds and
$results.TotalFiles yields blank output; before calling
Get-OrphanedInstallerFiles initialize fallback variables (e.g. $scanDurationSec
= 0, $totalFiles = 0) then in the try block set $scanDurationSec and $totalFiles
from $results when successful, and in the catch ensure they keep sensible
defaults or set error-specific values; update the event log/console output code
that currently references $results.ScanDuration.TotalSeconds and
$results.TotalFiles to use $scanDurationSec and $totalFiles (referencing
Get-OrphanedInstallerFiles and $results in the change).
- Around line 238-244: The call to
[System.Diagnostics.EventLog]::SourceExists($source) can throw a terminating
SecurityException and abort the script; wrap the whole event-source registration
block (the SourceExists check plus the CreateEventSource call that uses $source
and $logName) in a try/catch so any exception is handled non-terminatingly, and
in the catch either log the exception to console or silently continue (as
previously intended) to ensure subsequent event log writes and the console
summary always run.
- Around line 205-211: The threshold comparisons in the status determination use
-gt (strictly greater) but should be inclusive per spec; update the conditional
in the block that checks $totalSizeGB against $CriticalThresholdGB and
$WarningThresholdGB to use -ge (greater-or-equal) so that values exactly equal
to 20.00 GB or 50.00 GB are classified correctly (keep the order: evaluate
Critical first, then Warning, then default to Healthy), referencing the
variables $totalSizeGB, $CriticalThresholdGB, and $WarningThresholdGB in the
existing conditional logic.
---
Nitpick comments:
In `@rmm-ninja/Monitor-InstallerPatches`:
- Around line 199-202: The current measurement using Get-ChildItem -Recurse and
Measure-Object (variables $winsxsSize and $winsxsSizeGB) overcounts due to
hardlinks and is I/O-heavy; replace this approach by invoking an OS-supported
query (e.g., call DISM.exe /Online /Cleanup-Image /AnalyzeComponentStore or use
Get-WindowsPackage/Get-WindowsOptionalFeature) to obtain the WinSxS/component
store size, parse its output into $winsxsSizeGB, or remove the metric entirely
if it’s only a secondary indicator; ensure you stop using recursive enumeration
of C:\Windows\WinSxS and update any downstream references to
$winsxsSize/$winsxsSizeGB accordingly.
- Around line 246-266: The event mapping currently collapses Warning and
Critical to the same $eventId and always uses -EntryType Information; update the
switch that sets $eventId (the switch on $status) to assign distinct IDs for
"Warning" (e.g., 2000), "Critical" (e.g., 2500 or 3000) and "Error" (a
high/unique ID) and add a new variable (e.g., $entryType) that maps $status to
the proper EventLogEntryType values ("Information" for Healthy, "Warning" for
Warning, "Error" for Critical and Error as appropriate), then pass -EntryType
$entryType to Write-EventLog so the event ID and entry type both reflect the
actual $status.
- Around line 148-163: Measure-Object .Sum can be $null for empty collections
causing fragile values; update all places that use (.Sum) in this file (e.g.,
the $patchCacheSize assignment and the PSCustomObject properties TotalSizeBytes,
ReferencedSizeBytes, OrphanedSizeBytes which use ($allFiles | Measure-Object
Length -Sum).Sum and similar expressions) to explicitly coerce $null → 0 by
casting or using -as [long] (for example replace X.Sum with X.Sum -as [long] or
[long](X.Sum)); apply the same coercion pattern consistently for any other
Measure-Object .Sum usages in Monitor-InstallerPatches.
Addresses all 6 findings from the CodeRabbit review on PR DTC-Inc#43, plus proactive fixes for patterns that would trigger findings on a second pass. Threshold comparison logic (bug fix): Changed -gt to -ge in the status determination block. The spec defines Warning as >= 20 GB and Critical as >= 50 GB, but the original code used strict greater-than. A folder sized exactly 20.00 GB or 50.00 GB would be misclassified one tier lower (e.g., a 50.00 GB folder reported as "Warning" instead of "Critical"). This is the kind of boundary condition that matters when alerting thresholds drive technician response. Unprotected EventLog SourceExists() call (crash fix): [System.Diagnostics.EventLog]::SourceExists() throws a terminating SecurityException if the caller lacks permission to enumerate registered event-log sources. This would abort the entire script before event logging and the console summary ever executed — meaning NinjaRMM custom fields would update but the script log would show a cryptic error with no scan results. Wrapped the entire SourceExists + CreateEventSource block in a single try/catch so the script continues gracefully if event source registration fails. Null $results on scan failure (error path fix): If Get-OrphanedInstallerFiles throws, $results remains unassigned. PowerShell silently returns $null for member access on $null, so the event log message and console output would contain garbled lines like "Scan Duration: seconds" and "Installer Folder: 0 GB ( files)". Extracted all $results members into safe fallback variables ($totalFiles, $scanDurationSec, etc.) initialized to zero before the try block, and assigned from $results only on success. The event log and console output now reference these safe variables exclusively. Event ID and EntryType granularity (alerting improvement): Both "Warning" and "Critical" previously mapped to Event ID 2000, and all statuses used -EntryType Information. This meant SIEM tools, Windows Event Viewer filtering, and NinjaRMM condition-based alerting on event severity couldn't distinguish between Warning and Critical, and couldn't filter by the standard Windows event severity levels at all. Assigned distinct Event IDs (1000 Healthy, 2000 Warning, 2500 Critical, 3000 Error) and mapped -EntryType to the actual severity (Information, Warning, Error, Error) so monitoring infrastructure can key off both. WinSxS hardlink overcount caveat (accuracy): WinSxS uses hardlinks extensively. Recursive enumeration with Measure-Object overcounts actual disk footprint by 2-3x because the same physical data is counted once per hardlink. Added a comment explaining this limitation and appended "(approximate — hardlink overcount)" to the WinSxS line in both the event log message and console output, so technicians don't take the number at face value. The measurement is still useful as a rough trending indicator. Null coercion on Measure-Object .Sum (defensive arithmetic): Measure-Object .Sum returns $null rather than 0 when piped an empty collection. On an empty C:\Windows\Installer folder or a machine with zero orphans, downstream arithmetic like [math]::Round($null / 1GB, 2) produces 0 by accident in PowerShell but serialization and equality checks behave unexpectedly. Applied -as [long] coercion to every .Sum call in the shared Get-OrphanedInstallerFiles function and the WinSxS measurement to make the zero-result behavior explicit and deterministic.
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
rmm-ninja/Monitor-InstallerPatches (3)
62-62:$errorsshadows the automatic$Errorvariable.PowerShell variable names are case-insensitive, so
$errors = @()creates a local variable that overrides the built-in$Errorcollection for the entire function scope. This is a debugging hazard — any$Errorinspection inside the function will return this local array instead of the actual error stream. Rename to something like$registryErrors(and update all references to it including the returnedErrorsproperty).♻️ Proposed rename
- $errors = @() + $registryErrors = @()…and propagate through all
$errors +=/Errors = $errorssites in the function body.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmm-ninja/Monitor-InstallerPatches` at line 62, The function defines a local variable named $errors which inadvertently shadows PowerShell's automatic $Error collection; rename the local variable (for example to $registryErrors) and update every occurrence (including all "$errors +=" mutations and the final "Errors = $errors" return assignment) to use the new name so the built-in $Error remains intact and the returned Errors property reflects the renamed local variable.
261-273:switchstatements have nodefaultbranch — an unexpected$statusvalue silently drops the event log write.If
$statusever holds a value not covered by the four arms (e.g. after a future refactor adds a new tier), both$eventIdand$entryTypewill be$null.Write-EventLog -EventId $nullthen throws aParameterBindingExceptionwhich is swallowed by the outercatch, so the event log entry is lost without any indication.♻️ Proposed fix — add default arms
$eventId = switch ($status) { "Healthy" { 1000 } "Warning" { 2000 } "Critical" { 2500 } "Error" { 3000 } + default { 3000 } } $entryType = switch ($status) { "Healthy" { "Information" } "Warning" { "Warning" } "Critical" { "Error" } "Error" { "Error" } + default { "Error" } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmm-ninja/Monitor-InstallerPatches` around lines 261 - 273, The switch expressions that set $eventId and $entryType from $status lack default arms, so an unexpected $status yields $null and causes Write-EventLog to fail silently; update both switch expressions that assign $eventId and $entryType (the switch on $status) to include a default/else arm that maps unknown statuses to safe fallback values (e.g., a generic event id and "Warning" or "Information") and optionally emit a diagnostic/log entry before calling Write-EventLog to surface the unexpected $status.
210-213: Recursive WinSxS enumeration can block the entire script for several minutes.
Get-ChildItem "C:\Windows\WinSxS" -Recurse -Forcemust enumerate 50,000–200,000+ filesystem objects on a typical Windows installation. On HDD-backed or heavily-loaded servers this routinely takes 2–10+ minutes, and because this call is inside the sametryblock as the main scan, a stall here delays all downstream writes (NinjaRMM fields, event log, console output). There is also no timeout boundary.Since the comment already notes this value is approximate due to hardlink overcounting, consider one of:
- Skip it entirely and rely on
DISM /AnalyzeComponentStorefor accurate WinSxS sizing if needed.- Cap with a Job timeout so the script always completes in a bounded time.
- Use
[System.IO.Directory]::EnumerateFiles()(lazy iterator) instead ofGet-ChildItem, which is significantly faster for large trees.Also ensure the NinjaRMM script timeout is set higher than the worst-case WinSxS scan time to avoid premature termination.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmm-ninja/Monitor-InstallerPatches` around lines 210 - 213, The current blocking Get-ChildItem recursion (Get-ChildItem "C:\Windows\WinSxS" -Recurse -Force) that populates $winsxsSize can stall the whole script; replace that call with a non-blocking/lazy enumeration or a bounded-job fallback: use [System.IO.Directory]::EnumerateFiles("C:\Windows\WinSxS", "*", "AllDirectories") to stream file lengths into $winsxsSize (or run the existing Get-ChildItem inside a Start-Job with a short timeout and fall back to DISM /AnalyzeComponentStore if the job times out), ensure you still set $winsxsSize to 0 when empty and recompute $winsxsSizeGB from $winsxsSize, and raise the script timeout in the NinjaRMM job to exceed the worst-case scan time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rmm-ninja/Monitor-InstallerPatches`:
- Around line 124-125: The current $allFiles variable is filtered to only '.msi'
and '.msp' which makes derived metrics (TotalFiles, TotalSizeBytes,
$totalSizeGB, installerFolderSizeGB) reflect a subset not the actual Installer
folder; either remove the Where-Object filter on $allFiles so
TotalFiles/TotalSizeBytes compute the true folder totals (and use those for
threshold checks), or if the intent is to measure only MSI/MSP files, rename the
variables/labels (e.g., MsiMspTotalFiles, MsiMspTotalSizeBytes,
installerMsiMspSizeGB) and update the console/event-log and NinjaRMM field
installerFolderSizeGB to the new name so operators aren’t misled—also ensure the
status threshold comparison uses the correct variable depending on which
approach you choose.
---
Nitpick comments:
In `@rmm-ninja/Monitor-InstallerPatches`:
- Line 62: The function defines a local variable named $errors which
inadvertently shadows PowerShell's automatic $Error collection; rename the local
variable (for example to $registryErrors) and update every occurrence (including
all "$errors +=" mutations and the final "Errors = $errors" return assignment)
to use the new name so the built-in $Error remains intact and the returned
Errors property reflects the renamed local variable.
- Around line 261-273: The switch expressions that set $eventId and $entryType
from $status lack default arms, so an unexpected $status yields $null and causes
Write-EventLog to fail silently; update both switch expressions that assign
$eventId and $entryType (the switch on $status) to include a default/else arm
that maps unknown statuses to safe fallback values (e.g., a generic event id and
"Warning" or "Information") and optionally emit a diagnostic/log entry before
calling Write-EventLog to surface the unexpected $status.
- Around line 210-213: The current blocking Get-ChildItem recursion
(Get-ChildItem "C:\Windows\WinSxS" -Recurse -Force) that populates $winsxsSize
can stall the whole script; replace that call with a non-blocking/lazy
enumeration or a bounded-job fallback: use
[System.IO.Directory]::EnumerateFiles("C:\Windows\WinSxS", "*",
"AllDirectories") to stream file lengths into $winsxsSize (or run the existing
Get-ChildItem inside a Start-Job with a short timeout and fall back to DISM
/AnalyzeComponentStore if the job times out), ensure you still set $winsxsSize
to 0 when empty and recompute $winsxsSizeGB from $winsxsSize, and raise the
script timeout in the NinjaRMM job to exceed the worst-case scan time.
Monitor script changes:
WinSxS enumeration replaced with .NET EnumerateFiles (performance fix):
Get-ChildItem "C:\Windows\WinSxS" -Recurse must enumerate 50,000-200,000+ filesystem objects. On HDD-backed or heavily-loaded servers this routinely takes 2-10+ minutes, and because it was inside the same try block as the main scan, a stall blocked all downstream writes (NinjaRMM fields, event log, console output). Replaced with [System.IO.Directory]::EnumerateFiles() which is a lazy iterator — significantly faster because it doesn't materialize the entire directory tree into memory. Inner try/catch per file handles access-denied on individual hardlinks. Outer try/catch ensures any WinSxS enumeration failure falls back to 0 instead of failing the entire script.
switch statements given default branches (silent failure fix):
Both the $eventId and $entryType switch statements had no default arm. If $status ever held an unexpected value (e.g., after a future refactor adds a new tier), both would evaluate to $null. Write-EventLog -EventId $null throws a ParameterBindingException swallowed by the outer catch, silently losing the event log entry. Added default { 3000 } and default { "Error" } so unknown statuses fail safe into the highest-severity category.
Threshold comparison now uses unfiltered folder total:
Status determination and NinjaRMM installerFolderSizeGB field now use InstallerFolderTotalBytes (all file types) instead of the MSI/MSP-only subset. This ensures the reported status matches what operators observe in Explorer.
Console/event log output clarified:
Output now distinguishes "Installer Folder: X GB" (total) from "MSI/MSP Files: Y GB (Z files)" with Referenced/Orphaned as sub-items. Removes ambiguity about what the reported numbers include.
Redundant [math]::Round calls removed from NinjaRMM section:
Variables were already rounded to 2 decimal places at assignment time. The additional rounding in the Ninja-Property-Set calls was a no-op.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
rmm-ninja/Monitor-InstallerPatches (2)
153-157:Get-ChildItem -Recursefor$PatchCache$inconsistent with the WinSxS lazy-enumeration optimization; line 156 null guard is redundant.Two independent nits in this block:
The commit that replaced
Get-ChildItem -Recursewith[System.IO.Directory]::EnumerateFiles()for WinSxS cited the same blocking concern that applies to$PatchCache$— which can also grow into tens of GBs. Consider applying the same pattern here for consistency.The
if (-not $patchCacheSize)guard on line 156 is redundant:$null -as [long]already evaluates to0at line 155, so$patchCacheSizeis never$nullafter that assignment.✨ Proposed refactor
if (Test-Path $patchCachePath) { - $patchCacheSize = (Get-ChildItem $patchCachePath -Recurse -Force -ErrorAction SilentlyContinue | - Measure-Object Length -Sum).Sum -as [long] - if (-not $patchCacheSize) { $patchCacheSize = [long]0 } + try { + foreach ($f in [System.IO.Directory]::EnumerateFiles($patchCachePath, "*", [System.IO.SearchOption]::AllDirectories)) { + try { $patchCacheSize += ([System.IO.FileInfo]::new($f)).Length } catch { } + } + } catch { + $patchCacheSize = [long]0 + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmm-ninja/Monitor-InstallerPatches` around lines 153 - 157, Replace the slow Get-ChildItem -Recurse usage when computing $patchCacheSize with a lazy enumerator: use [System.IO.Directory]::EnumerateFiles($patchCachePath, '*', [System.IO.SearchOption]::AllDirectories) piped to Measure-Object to avoid full recursion blocking (targeting the block that references $patchCachePath and $patchCacheSize), and remove the redundant if (-not $patchCacheSize) null-guard because ($null -as [long]) already yields 0 after the current assignment.
124-124: Prefer$env:SystemRootover hardcodedC:\Windows.Both
$installerPath(line 124) and the WinSxS path (line 223) hardcode the drive letter and folder name. Using$env:SystemRootis idiomatic for SYSTEM-run scripts and avoids silent failures on the rare non-default installation path.✨ Proposed refactor
- $installerPath = "C:\Windows\Installer" + $installerPath = Join-Path $env:SystemRoot "Installer"- foreach ($f in [System.IO.Directory]::EnumerateFiles("C:\Windows\WinSxS", "*", [System.IO.SearchOption]::AllDirectories)) { + foreach ($f in [System.IO.Directory]::EnumerateFiles((Join-Path $env:SystemRoot "WinSxS"), "*", [System.IO.SearchOption]::AllDirectories)) {Also applies to: 223-223
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmm-ninja/Monitor-InstallerPatches` at line 124, Replace hardcoded "C:\Windows" usages with $env:SystemRoot and build paths via Join-Path to support non-default system roots: update the $installerPath assignment (currently "C:\Windows\Installer") to derive from $env:SystemRoot and likewise change the WinSxS path (referenced in the diff as the WinSxS path) to use $env:SystemRoot; use Join-Path (or explicit string concat with $env:SystemRoot) so the script works when Windows is installed to a different folder.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rmm-ninja/Monitor-InstallerPatches`:
- Line 127: The computed InstallerFolderTotalBytes currently sums only top-level
files found in $allInstallerFiles (no -Recurse), so it omits the $PatchCache$
folder and can diverge from Explorer; update the code that builds
InstallerFolderTotalBytes to add the already-computed $patchCacheSize (i.e.,
include $patchCacheSize in the total before deriving $installerFolderSizeGB and
populating the result object InstallerFolderTotalBytes/installerFolderSizeGB) so
dashboard status reflects Explorer totals, or alternatively adjust the
comment/commit text to explicitly state the value excludes $PatchCache$ if
exclusion is intended.
---
Nitpick comments:
In `@rmm-ninja/Monitor-InstallerPatches`:
- Around line 153-157: Replace the slow Get-ChildItem -Recurse usage when
computing $patchCacheSize with a lazy enumerator: use
[System.IO.Directory]::EnumerateFiles($patchCachePath, '*',
[System.IO.SearchOption]::AllDirectories) piped to Measure-Object to avoid full
recursion blocking (targeting the block that references $patchCachePath and
$patchCacheSize), and remove the redundant if (-not $patchCacheSize) null-guard
because ($null -as [long]) already yields 0 after the current assignment.
- Line 124: Replace hardcoded "C:\Windows" usages with $env:SystemRoot and build
paths via Join-Path to support non-default system roots: update the
$installerPath assignment (currently "C:\Windows\Installer") to derive from
$env:SystemRoot and likewise change the WinSxS path (referenced in the diff as
the WinSxS path) to use $env:SystemRoot; use Join-Path (or explicit string
concat with $env:SystemRoot) so the script works when Windows is installed to a
different folder.
Updated InstallerFolderTotalBytes calculation to include $PatchCache$ size and improved path handling for Windows directories.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rmm-ninja/Monitor-InstallerPatches`:
- Around line 281-287: The switch assigning $eventId uses "Critical" => 2500
while the spec/doc states 2000 should cover Warning/Critical; update the switch
in the $eventId assignment so "Critical" maps to 2000 (merging it with
"Warning") OR update the spec/comment to reflect the new mapping ("Warning" =>
2000, "Critical" => 2500); change the case for "Critical" in the switch (and any
related comment near the $eventId assignment) to keep code and docs consistent.
- Around line 79-120: The code hardcodes S-1-5-18 for $productsPath and
$patchesPath, causing per-user MSI LocalPackage entries to be missed; change the
logic to enumerate all SID subkeys under
"HKLM:\SOFTWARE\Microsoft\Windows\CurrentVersion\Installer\UserData" (e.g.
Get-ChildItem on the UserData key) and for each SID build the Products and
Patches paths and collect LocalPackage values into $referencedFiles; keep the
top-level UserData read as a thrown error if it fails, but treat per-SID
failures inside the loop the same way as existing inner catches by appending
messages to $registryErrors (reference symbols: $productsPath/$patchesPath,
$referencedFiles, $registryErrors, and the inner try/catch blocks that read
InstallProperties and LocalPackage).
Comment on lines
+281
to
+287
| $eventId = switch ($status) { | ||
| "Healthy" { 1000 } | ||
| "Warning" { 2000 } | ||
| "Critical" { 2500 } | ||
| "Error" { 3000 } | ||
| default { 3000 } | ||
| } |
Contributor
There was a problem hiding this comment.
Critical event ID 2500 contradicts the documented spec.
The PR description specifies 2000 = Warning/Critical, but the code assigns 2500 to "Critical" and 2000 to "Warning". Any SIEM or monitoring alert filter configured against the documented mapping will miss Critical events (they won't match the expected ID 2000). Either align the code to the spec (merge Critical into 2000) or update the spec/doc comment to declare 2000 = Warning, 2500 = Critical so operators know to alert on both IDs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rmm-ninja/Monitor-InstallerPatches` around lines 281 - 287, The switch
assigning $eventId uses "Critical" => 2500 while the spec/doc states 2000 should
cover Warning/Critical; update the switch in the $eventId assignment so
"Critical" maps to 2000 (merging it with "Warning") OR update the spec/comment
to reflect the new mapping ("Warning" => 2000, "Critical" => 2500); change the
case for "Critical" in the switch (and any related comment near the $eventId
assignment) to keep code and docs consistent.
Updated error handling to use List for registry errors and scan errors, improving performance and readability.
Updated comments for clarity on event ID usage.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
rmm-ninja/Monitor-InstallerPatches (2)
94-96:Test-Path $localPackageis redundant and adds unnecessary I/O.
$allFilesis built fromGet-ChildItemand only ever contains files that physically exist. ALocalPackagepath that does not exist on disk cannot appear in$allFiles, so it will never be misclassified regardless of whether it's in$referencedFiles. TheTest-Pathcalls (one per registered product and one per registered patch) are dead checks that add filesystem syscalls on every invocation — potentially hundreds on a production server.♻️ Proposed simplification (×2 locations)
- if ($localPackage -and (Test-Path $localPackage)) { + if ($localPackage) { [void]$referencedFiles.Add($localPackage) }- if ($localPackage -and (Test-Path $localPackage)) { + if ($localPackage) { [void]$referencedFiles.Add($localPackage) }Also applies to: 114-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmm-ninja/Monitor-InstallerPatches` around lines 94 - 96, Remove the redundant Test-Path check around LocalPackage: when adding referenced installer/patch files, simply retrieve LocalPackage from Get-ItemProperty ($installProps) and if $localPackage is non-empty add it to $referencedFiles without calling Test-Path; the existence check is unnecessary because $allFiles was built via Get-ChildItem and will never include non-existent paths—apply the same removal in the second occurrence that processes LocalPackage for registered patches so both locations use only the $localPackage truthy check before adding to $referencedFiles.
231-236: WinSxS mid-enumeration failure silently discards partial count.
EnumerateFileswithAllDirectoriesis lazy; anUnauthorizedAccessExceptionon a subdirectory throws from the iterator'sMoveNext(), propagating to the outercatch, which resets$winsxsSizeto0. Rather than discarding the partial accumulation, keep it and optionally log the interruption:♻️ Proposed improvement
} catch { - $winsxsSize = [long]0 + # Enumeration interrupted — keep partial accumulation and surface the gap + Write-Verbose "WinSxS enumeration interrupted: $($_.Exception.Message). Size is partial." }Given the existing "rough indicator" disclaimer this is low-priority, but the change avoids
0 GBmasking a partial measurement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmm-ninja/Monitor-InstallerPatches` around lines 231 - 236, The current outer catch resets $winsxsSize to 0 when the lazy EnumerateFiles iterator throws (e.g., UnauthorizedAccessException), discarding any partial accumulation; change the error handling so partial size is preserved and the interruption is logged instead of zeroing out the result. Specifically, keep the foreach/EnumerateFiles accumulation (variable $winsxsSize) but remove or stop assigning [long]0 in the outer catch; instead log the exception (or write a warning) and preserve the current $winsxsSize value; alternatively replace the foreach with an explicit enumerator loop (GetEnumerator()/while MoveNext()) so you can catch per-move exceptions, accumulate sizes from ([System.IO.FileInfo]::new($f)).Length, and only handle per-file errors in the inner catch while letting the outer catch log and keep the partial total.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rmm-ninja/Monitor-InstallerPatches`:
- Around line 1-18: The file is missing the PowerShell extension; rename the
script from Monitor-InstallerPatches to Monitor-InstallerPatches.ps1 so
Windows/IDEs recognize it as PowerShell, update any references or scheduled-task
entries that call Monitor-InstallerPatches to use Monitor-InstallerPatches.ps1,
and ensure any README, deployment manifests, or CI/scheduled script
configuration are updated to the new filename.
---
Duplicate comments:
In `@rmm-ninja/Monitor-InstallerPatches`:
- Around line 286-298: The comment explains the intended Event ID mapping, but
the code must explicitly match that intent: ensure the switch on $status assigns
"Healthy" => 1000, "Warning" => 2000, "Critical" => 2500, "Error" => 3000 and
default => 3000; keep or expand the documentation block above the switch to
state this is intentional for SIEM rules and update any PR description/tests to
reflect the mapping so reviewers can verify the behavior of $eventId produced by
the switch.
---
Nitpick comments:
In `@rmm-ninja/Monitor-InstallerPatches`:
- Around line 94-96: Remove the redundant Test-Path check around LocalPackage:
when adding referenced installer/patch files, simply retrieve LocalPackage from
Get-ItemProperty ($installProps) and if $localPackage is non-empty add it to
$referencedFiles without calling Test-Path; the existence check is unnecessary
because $allFiles was built via Get-ChildItem and will never include
non-existent paths—apply the same removal in the second occurrence that
processes LocalPackage for registered patches so both locations use only the
$localPackage truthy check before adding to $referencedFiles.
- Around line 231-236: The current outer catch resets $winsxsSize to 0 when the
lazy EnumerateFiles iterator throws (e.g., UnauthorizedAccessException),
discarding any partial accumulation; change the error handling so partial size
is preserved and the interruption is logged instead of zeroing out the result.
Specifically, keep the foreach/EnumerateFiles accumulation (variable
$winsxsSize) but remove or stop assigning [long]0 in the outer catch; instead
log the exception (or write a warning) and preserve the current $winsxsSize
value; alternatively replace the foreach with an explicit enumerator loop
(GetEnumerator()/while MoveNext()) so you can catch per-move exceptions,
accumulate sizes from ([System.IO.FileInfo]::new($f)).Length, and only handle
per-file errors in the inner catch while letting the outer catch log and keep
the partial total.
Comment on lines
+1
to
+18
| <# | ||
| .SYNOPSIS | ||
| DTC Installer Patch Monitor — Weekly scan of C:\Windows\Installer for orphaned .msi/.msp files. | ||
| .DESCRIPTION | ||
| Scans C:\Windows\Installer to identify orphaned installer cache files by querying the | ||
| Windows Installer registry database. Reports results to NinjaRMM custom fields and | ||
| Windows Event Log. Makes ZERO filesystem changes — read-only analysis only. | ||
|
|
||
| Deployment: NinjaRMM scheduled script, run weekly (e.g., Sunday 2:00 AM). | ||
| Runs as: SYSTEM | ||
|
|
||
| Reference: HALO Ticket 1125653 — 128 GB orphaned patches, 96.1% disk utilization. | ||
| .NOTES | ||
| Author: DTC Engineering | ||
| Version: 1.0.0 | ||
| Requires: PowerShell 5.1, Windows 10/11/Server 2019/2022 | ||
| Dependencies: None (self-contained) | ||
| #> |
Contributor
There was a problem hiding this comment.
Missing .ps1 file extension.
The file is committed as Monitor-InstallerPatches without a .ps1 extension. On Windows, .ps1 is required for the OS to associate the file with PowerShell, and most IDEs/SCM tools use the extension for syntax highlighting and linting. Rename to Monitor-InstallerPatches.ps1.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rmm-ninja/Monitor-InstallerPatches` around lines 1 - 18, The file is missing
the PowerShell extension; rename the script from Monitor-InstallerPatches to
Monitor-InstallerPatches.ps1 so Windows/IDEs recognize it as PowerShell, update
any references or scheduled-task entries that call Monitor-InstallerPatches to
use Monitor-InstallerPatches.ps1, and ensure any README, deployment manifests,
or CI/scheduled script configuration are updated to the new filename.
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
1 participant
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.
Adds Monitor-InstallerPatches.ps1 — a read-only NinjaRMM scheduled script that detects orphaned .msi/.msp files accumulating in C:\Windows\Installer. This folder is not cleaned by Disk Cleanup or DISM and can silently grow to consume entire drives on long-lived dental workstations (ref: HALO Ticket 1125653 — 128 GB orphaned patches, 96.1% disk utilization at Guardian Dentistry Partners).
How orphan detection works:
The script queries the Windows Installer registry database at HKLM:\SOFTWARE\Microsoft\Windows\CurrentVersion\Installer\UserData\S-1-5-18\Products and \Patches, collecting every LocalPackage value to build a set of referenced cached installers. Any .msi/.msp file in C:\Windows\Installer not in that referenced set is classified as orphaned. This avoids the Win32_Product WMI class entirely, which triggers a reconfigure/consistency check on every installed MSI and can take 20+ minutes while destabilizing applications.
What the script reports:
Total installer folder size, orphaned file count and size, referenced file count and size$PatchCache$ subfolder size
WinSxS size as a secondary disk consumption indicator Status classification: Healthy (<20 GB), Warning (20–50 GB), Critical (>50 GB), or Error on scan failure Where results are written:
Six NinjaRMM custom fields via Ninja-Property-Set (graceful no-op outside NinjaRMM runtime) Windows Application Event Log under source DTC-InstallerMonitor (Event ID 1000 = Healthy, 2000 = Warning/Critical, 3000 = Error) Console output for NinjaRMM script activity log
Safety guarantees:
Makes zero filesystem changes — no files created, moved, or deleted All thresholds defined as variables at script top for easy adjustment Self-contained with no external dependencies (PowerShell 5.1 only) Designed to run as SYSTEM on Windows 10, 11, Server 2019, and Server 2022
Summary by CodeRabbit