Merge pull request #17 from AprilDeFeu/copilot/sub-pr-15

AprilDeFeu · web-flow · commit 34de42d5c332 · 2025-11-15T15:56:47.000-05:00
Fix runtime errors and automation blockers from PR #15 review feedback
diff --git a/.githooks/pre-commit.ps1 b/.githooks/pre-commit.ps1
@@ -11,6 +11,9 @@ if (Test-Path $testPath) {
     # Set SCRIPTS_ROOT environment variable for tests
     $env:SCRIPTS_ROOT = (Get-Location).Path
 
+    # Ensure the results directory exists before running Pester
+    New-Item -ItemType Directory -Path (Split-Path $resultsPath -Parent) -Force -ErrorAction SilentlyContinue | Out-Null
+
     # Use Pester v5 configuration syntax
     $config = New-PesterConfiguration
     $config.Run.Path = $testPath
@@ -34,3 +37,8 @@ if (Test-Path $testPath) {
         exit 1
     }
 }
+else {
+    Write-Host "Test file not found: $testPath"
+    Write-Host "Cannot verify script quality. Aborting commit."
+    exit 1
+}
diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md
@@ -264,3 +264,10 @@ This repository prefers:
 - Create pull requests via GitHub web UI
 - Avoid using `gh` CLI in automation
 - Refer to `.github/PR_PREFERENCES.md` for detailed workflow guidance
+
+## Copilot PR Review Policy
+
+**IMPORTANT**: Do NOT automatically review pull requests when they are marked as "ready for review".
+- Only perform PR reviews when explicitly requested by tagging @copilot in a comment
+- Premium review requests should be used wisely and deliberately
+- Wait for manual request before analyzing or reviewing code changes
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -34,38 +34,12 @@ jobs:
       - name: Checkout Code
         uses: actions/checkout@v4
 
-      - name: Check for local test results
-        id: check_results
-        shell: pwsh
-        run: |
-          $resultsPath = 'tests/results/system-maintenance.xml'
-          if (Test-Path $resultsPath) {
-            $fileAge = (Get-Date) - (Get-Item $resultsPath).LastWriteTime
-            if ($fileAge.TotalDays -lt 2) {
-              echo "found=true" >> $env:GITHUB_OUTPUT
-            } else {
-              Write-Host "Test results are too old."
-              echo "found=false" >> $env:GITHUB_OUTPUT
-            }
-          } else {
-            Write-Host "No local test results found."
-            echo "found=false" >> $env:GITHUB_OUTPUT
-          }
-
-      - name: Use local test results if available
-        if: steps.check_results.outputs.found == 'true'
-        shell: pwsh
-        run: |
-          Write-Host "Using local test results. Skipping expensive PowerShell tests."
-
       - name: Install Pester
-        if: steps.check_results.outputs.found != 'true'
         shell: pwsh
         run: |
           Install-Module -Name Pester -Force -SkipPublisherCheck
 
       - name: Run Pester Tests
-        if: steps.check_results.outputs.found != 'true'
         shell: pwsh
         env:
           SCRIPTS_ROOT: ${{ github.workspace }}
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
@@ -40,4 +40,5 @@ repos:
         entry: pwsh .githooks/pre-commit.ps1
         language: system
         types: [powershell]
+        files: ^PowerShell/system-administration/maintenance/system-maintenance\.ps1$
         pass_filenames: false
diff --git a/PowerShell/system-administration/maintenance/system-maintenance.ps1 b/PowerShell/system-administration/maintenance/system-maintenance.ps1
@@ -22,9 +22,18 @@
     Maximum age (in days) files must be older than to be removed from temp
     locations. Default: 7. Set to 0 to remove everything (use with caution).
 
+.PARAMETER DestructiveMode
+    When specified, enables destructive operations (disk cleanup, network reset,
+    CHKDSK repair) without interactive prompts. Use with caution in automated
+    scenarios. Without this flag, destructive operations require confirmation.
+
 .EXAMPLE
     .\system-maintenance.ps1 -RunWindowsUpdate -MaxTempFileAgeDays 14
 
+.EXAMPLE
+    .\system-maintenance.ps1 -DestructiveMode -WhatIf
+    Preview destructive operations in non-interactive mode.
+
 .EXAMPLE
     .\system-maintenance.ps1 -WhatIf
     Preview all destructive operations without executing them.
@@ -38,7 +47,8 @@
 [CmdletBinding(SupportsShouldProcess = $true, ConfirmImpact = 'Medium')]
 param(
     [switch] $RunWindowsUpdate,
-    [ValidateRange(0, 3650)][int] $MaxTempFileAgeDays = 7
+    [ValidateRange(0, 3650)][int] $MaxTempFileAgeDays = 7,
+    [switch] $DestructiveMode
 )
 
 Set-StrictMode -Version Latest
@@ -58,15 +68,8 @@ function Get-LogFilePath {
 
 $script:LogFile = Get-LogFilePath
 
-
-# Store the script-level PSCmdlet for use in nested scriptblocks (set in Begin block)
-$script:ScriptPSCmdlet = $null
-
-Begin {
-    if ($null -eq $script:ScriptPSCmdlet -and $null -ne $PSCmdlet) {
-        $script:ScriptPSCmdlet = $PSCmdlet
-    }
-}
+# Store the script-level PSCmdlet for use in nested scriptblocks
+$script:ScriptPSCmdlet = $PSCmdlet
 
 # Helper to perform a confirmation check that works even when invoked inside
 # nested scriptblocks. Uses the script-scoped PSCmdlet reference.
@@ -96,7 +99,12 @@ function Invoke-Step {
     Write-Log "BEGIN: $Title"
     try {
         if ($Destructive.IsPresent -and $ConfirmTarget) {
-            if (-not ($PSCmdlet.ShouldProcess($ConfirmTarget, $Title))) {
+            # Use script-scoped PSCmdlet with null check
+            if ($null -eq $script:ScriptPSCmdlet) {
+                Write-Log -Message "SKIP: $Title (PSCmdlet not available)" -Level 'WARN'
+                return
+            }
+            if (-not ($script:ScriptPSCmdlet.ShouldProcess($ConfirmTarget, $Title))) {
                 Write-Log -Message "SKIP: $Title (not confirmed)" -Level 'WARN'
                 return
             }
@@ -136,41 +144,7 @@ function Invoke-Step {
 Write-Log "Starting system maintenance and health checks. Params: RunWindowsUpdate=$RunWindowsUpdate, MaxTempFileAgeDays=$MaxTempFileAgeDays"
 
 # --- Destructive Mode Selection ---
-$DestructiveMode = $false
-$DestructiveExplanation = @"
-==================== DESTRUCTIVE MODE WARNING ====================
-This script can perform several potentially destructive operations:
-
-1. Disk cleanup (Temp, Cache):
-   - Deletes files from system/user temp folders and Windows Update/Delivery Optimization caches.
-   - Danger: May remove files needed by some applications or pending updates.
-2. Network reset:
-   - Resets Winsock, IP stack, and flushes DNS.
-   - Danger: May disrupt network connectivity and require a reboot.
-3. CHKDSK repair:
-   - Schedules disk repair on next reboot if errors are found.
-   - Danger: Can cause data loss if disk is failing or interrupted.
-
-By default, these steps are run in safe (non-destructive) mode. To enable all destructive operations, choose 'Destructive' when prompted.
-==================================================================
-"@
-
-# Only prompt if running interactively and not -WhatIf
-if (-not $WhatIfPreference -and $Host.UI.RawUI -and $Host.Name -ne 'ServerRemoteHost') {
-    Write-Host $DestructiveExplanation -ForegroundColor Yellow
-    $choice = Read-Host "Run in [S]tandard (safe) or [D]estructive (dangerous) mode? [S/D] (default: S)"
-    if ($choice -match '^[Dd]') {
-        $DestructiveMode = $true
-        Write-Host "Destructive mode ENABLED. Proceeding with all operations." -ForegroundColor Red
-    }
-    else {
-        Write-Host "Standard (safe) mode selected. Destructive steps will be skipped or require extra confirmation." -ForegroundColor Green
-    }
-}
-else {
-    # Non-interactive or WhatIf: default to Standard
-    $DestructiveMode = $false
-}
+# Note: Now controlled via -DestructiveMode parameter (no longer prompts interactively)
 
 # ---------------------- Windows Update (optional) ----------------------
 if ($RunWindowsUpdate) {
@@ -246,10 +220,13 @@ Invoke-Step -Title 'CHKDSK read-only scan and user review' -ScriptBlock {
                 $affectedSectors | ForEach-Object { Write-Host $_ -ForegroundColor Yellow }
             }
             Write-Host "Please back up any important files before continuing." -ForegroundColor Yellow
-            $null = Read-Host "Press Enter to continue with disk cleanup or Ctrl+C to abort"
+            # Use ShouldContinue for non-interactive compatibility
+            if (-not $script:ScriptPSCmdlet.ShouldContinue("Continue with disk cleanup after reviewing disk errors?", "Disk errors were found on $sysDrive")) {
+                Write-Output "User chose not to continue with disk cleanup. Exiting maintenance."
+                return
+            }
             # After user review, offer to schedule repair
-            $scheduleRepair = Read-Host "Would you like to schedule a disk repair on next reboot? [y/N]"
-            if ($scheduleRepair -match '^[Yy]') {
+            if ($script:ScriptPSCmdlet.ShouldContinue("Schedule a disk repair on next reboot?", "CHKDSK repair")) {
                 $repairOutput = cmd /c "chkdsk $sysDrive /F /R" 2>&1 | Out-String
                 Write-Output $repairOutput
                 Write-Output 'Repair scheduled. A reboot will be required to complete the repair.'
@@ -266,8 +243,7 @@ Invoke-Step -Title 'CHKDSK read-only scan and user review' -ScriptBlock {
     }
 }
 
-if ($DestructiveMode) {
-    Invoke-Step -Title 'Disk cleanup (Temp, Cache)' -Destructive -ConfirmTarget 'Clean temporary and cache files' -ScriptBlock {
+Invoke-Step -Title 'Disk cleanup (Temp, Cache)' -Destructive -ConfirmTarget 'Clean temporary and cache files' -ScriptBlock {
         try {
             $paths = @($env:TEMP, "$env:WINDIR\Temp", "$env:LOCALAPPDATA\Temp") | Where-Object { Test-Path $_ }
             $threshold = (Get-Date).AddDays(-1 * [int]$MaxTempFileAgeDays)
@@ -287,7 +263,7 @@ if ($DestructiveMode) {
             # Windows Update download cache
             $wuCache = "$env:WINDIR\SoftwareDistribution\Download"
             if (Test-Path $wuCache) {
-                if (Confirm-Action -Target 'Windows Update download cache' -Action 'Clear cache') {
+                if ($script:ScriptPSCmdlet.ShouldProcess('Windows Update download cache', 'Clear cache')) {
                     # Stop services using proper PowerShell cmdlets
                     $wuService = Get-Service -Name wuauserv -ErrorAction SilentlyContinue
                     $bitsService = Get-Service -Name bits -ErrorAction SilentlyContinue
@@ -326,7 +302,7 @@ if ($DestructiveMode) {
             # Delivery Optimization
             $doPath = "$env:ProgramData\Microsoft\Windows\DeliveryOptimization\Cache"
             if (Test-Path $doPath) {
-                if (Confirm-Action -Target 'Delivery Optimization cache' -Action 'Clear cache') {
+                if ($script:ScriptPSCmdlet.ShouldProcess('Delivery Optimization cache', 'Clear cache')) {
                     Get-ChildItem $doPath -Recurse -Force -ErrorAction SilentlyContinue | Remove-Item -Recurse -Force -ErrorAction SilentlyContinue
                     Write-Output 'Cleared Delivery Optimization cache'
                 }
@@ -337,7 +313,6 @@ if ($DestructiveMode) {
             Write-Output "Disk cleanup error: $($_.Exception.Message)"
         }
     }
-}
 
 Invoke-Step -Title 'Drive optimization (trim/defrag)' -ScriptBlock {
     try {
@@ -376,13 +351,13 @@ Invoke-Step -Title 'Drive optimization (trim/defrag)' -ScriptBlock {
             }
 
             if ($isSSD) {
-                if (Confirm-Action -Target "${letter}: (SSD)" -Action 'ReTrim volume') {
+                if ($script:ScriptPSCmdlet.ShouldProcess("${letter}: (SSD)", 'ReTrim volume')) {
                     Optimize-Volume -DriveLetter $letter -ReTrim -Verbose:$false | Out-Null
                     Write-Output "Trimmed ${letter}: (SSD)"
                 }
             }
             else {
-                if (Confirm-Action -Target "${letter}: (HDD)" -Action 'Defragment volume') {
+                if ($script:ScriptPSCmdlet.ShouldProcess("${letter}: (HDD)", 'Defragment volume')) {
                     Optimize-Volume -DriveLetter $letter -Defrag -Verbose:$false | Out-Null
                     Write-Output "Defragmented ${letter}: (HDD)"
                 }
@@ -418,7 +393,7 @@ Invoke-Step -Title 'Service health checks (BITS, wuauserv, CryptSvc)' -ScriptBlo
             if ($null -ne $svc) {
                 Write-Output ("{0}: {1}" -f $svc.Name, $svc.Status)
                 if ($svc.Status -ne 'Running') {
-                    if (Confirm-Action -Target $svc.Name -Action 'Start service') {
+                    if ($script:ScriptPSCmdlet.ShouldProcess($svc.Name, 'Start service')) {
                         Start-Service $svc -ErrorAction SilentlyContinue
                         Write-Output "Started service: $($svc.Name)"
                     }
diff --git a/tests/unit/PowerShell/system-maintenance.Tests.ps1 b/tests/unit/PowerShell/system-maintenance.Tests.ps1
@@ -45,9 +45,8 @@ Describe "system-maintenance.ps1" {
 
         It "should have comment-based help" {
             $help = Get-Help $scriptPath -ErrorAction SilentlyContinue
-            $notNull = $false
-            if ($help -and $help.Name -eq 'system-maintenance.ps1') { $notNull = $true }
-            $notNull | Should -Be $true
+            $help | Should -Not -BeNullOrEmpty
+            $help.Name | Should -Be 'system-maintenance.ps1'
         }
 
         It "should support -WhatIf" {
@@ -91,7 +90,34 @@ Describe "system-maintenance.ps1" {
         }
     }
 
-    # Context "Edge Cases" removed: requires admin rights
+    Context "Edge Cases" {
+        It "should handle MaxTempFileAgeDays = 0 (delete all temp files)" {
+            $localPath = $scriptPath
+            { & $localPath -MaxTempFileAgeDays 0 -WhatIf } | Should -Not -Throw
+        }
+
+        It "should handle MaxTempFileAgeDays at upper boundary (3650 days)" {
+            $localPath = $scriptPath
+            { & $localPath -MaxTempFileAgeDays 3650 -WhatIf } | Should -Not -Throw
+        }
+
+        It "should handle MaxTempFileAgeDays = 1 (minimum practical value)" {
+            $localPath = $scriptPath
+            { & $localPath -MaxTempFileAgeDays 1 -WhatIf } | Should -Not -Throw
+        }
+
+        It "should handle RunWindowsUpdate switch with WhatIf" {
+            $localPath = $scriptPath
+            # WhatIf prevents actual Windows Update operations
+            { & $localPath -RunWindowsUpdate -WhatIf } | Should -Not -Throw
+        }
+
+        It "should handle DestructiveMode switch with WhatIf" {
+            $localPath = $scriptPath
+            # WhatIf prevents actual destructive operations
+            { & $localPath -DestructiveMode -WhatIf } | Should -Not -Throw
+        }
+    }
 
     Context "Permissions and Prerequisites" {
         It "should have #Requires -RunAsAdministrator directive" {
@@ -105,16 +131,23 @@ Describe "system-maintenance.ps1" {
         # handled by PowerShell itself.
     }
 
-    # Context "Dependencies" removed: requires admin rights
+    Context "Dependencies" {
+        It "should gracefully handle missing PSWindowsUpdate module when not requested" {
+            $localPath = $scriptPath
+            # When RunWindowsUpdate is not specified, the script should not attempt to use the module
+            { & $localPath -WhatIf } | Should -Not -Throw
+        }
+
+        # Note: Testing the RunWindowsUpdate path would require either:
+        # 1. Installing PSWindowsUpdate (which the script does automatically if missing)
+        # 2. Mocking the module import (complex in Pester 5 for external scripts)
+        # This demonstrates the dependency is optional and only loaded when needed
+    }
 
     Context "Parameter Validation" {
         It "should use default value when MaxTempFileAgeDays not specified" {
             $command = Get-Command -Name $scriptPath
-            $count = 0
-            foreach ($attr in $command.Parameters['MaxTempFileAgeDays'].Attributes) {
-                if ($attr -is [System.Management.Automation.ParameterAttribute]) { $count++ }
-            }
-            ($count -gt 0) | Should -Be $true
+            $command.Parameters['MaxTempFileAgeDays'].Attributes.Where({$_ -is [System.Management.Automation.ParameterAttribute]}).Count | Should -BeGreaterThan 0
         }
     }
 
