Fix runtime errors and automation blockers from PR #15 review feedback #17
Conversation
…se tests, improve automation support Co-authored-by: AprilDeFeu <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR addresses critical runtime errors and automation blockers identified in PR #15 code review. The changes fix removed function references, eliminate interactive prompts blocking automation, restore removed edge case tests, and improve CI reliability by removing test result caching.
Key changes:
- Replaced 5 calls to removed
Confirm-Actionfunction with$PSCmdlet.ShouldProcess() - Added
-DestructiveModeparameter to replace interactiveRead-Hostprompts - Restored 5 edge case tests and Dependencies context that were previously removed
- Removed CI test result caching optimization to prevent bypassing checks with stale results
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| PowerShell/system-administration/maintenance/system-maintenance.ps1 | Added -DestructiveMode parameter, replaced interactive prompts with ShouldProcess()/ShouldContinue(), fixed $script:ScriptPSCmdlet initialization |
| tests/unit/PowerShell/system-maintenance.Tests.ps1 | Restored Edge Cases and Dependencies contexts, simplified assertion patterns |
| .pre-commit-config.yaml | Added specific file pattern to scope pre-commit hook |
| .github/workflows/ci.yml | Removed test result caching logic |
| .githooks/pre-commit.ps1 | Added explicit failure when test file is missing |
Comments suppressed due to low confidence (1)
PowerShell/system-administration/maintenance/system-maintenance.ps1:250
- Inside scriptblocks passed to
Invoke-Step,$PSCmdletis not automatically available. This should use$script:ScriptPSCmdletinstead to properly respect-WhatIfand-Confirmparameters.
if ($PSCmdlet.ShouldProcess("Delete old files in $p", 'Delete files')) {
| 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 $PSCmdlet.ShouldContinue("Continue with disk cleanup after reviewing disk errors?", "Disk errors were found on $sysDrive")) { |
There was a problem hiding this comment.
Inside scriptblocks passed to Invoke-Step, $PSCmdlet is not automatically available. This should use $script:ScriptPSCmdlet instead (which is initialized at line 68) to access the correct cmdlet context with -WhatIf and -Confirm support.
| # 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 ($PSCmdlet.ShouldContinue("Schedule a disk repair on next reboot?", "CHKDSK repair")) { |
There was a problem hiding this comment.
Inside scriptblocks passed to Invoke-Step, $PSCmdlet is not automatically available. This should use $script:ScriptPSCmdlet instead to access the correct cmdlet context.
| $wuCache = "$env:WINDIR\SoftwareDistribution\Download" | ||
| if (Test-Path $wuCache) { | ||
| if (Confirm-Action -Target 'Windows Update download cache' -Action 'Clear cache') { | ||
| if ($PSCmdlet.ShouldProcess('Windows Update download cache', 'Clear cache')) { |
There was a problem hiding this comment.
Inside scriptblocks passed to Invoke-Step, $PSCmdlet is not automatically available. This should use $script:ScriptPSCmdlet instead to properly respect -WhatIf and -Confirm parameters.
| $doPath = "$env:ProgramData\Microsoft\Windows\DeliveryOptimization\Cache" | ||
| if (Test-Path $doPath) { | ||
| if (Confirm-Action -Target 'Delivery Optimization cache' -Action 'Clear cache') { | ||
| if ($PSCmdlet.ShouldProcess('Delivery Optimization cache', 'Clear cache')) { |
There was a problem hiding this comment.
Inside scriptblocks passed to Invoke-Step, $PSCmdlet is not automatically available. This should use $script:ScriptPSCmdlet instead to properly respect -WhatIf and -Confirm parameters.
|
|
||
| if ($isSSD) { | ||
| if (Confirm-Action -Target "${letter}: (SSD)" -Action 'ReTrim volume') { | ||
| if ($PSCmdlet.ShouldProcess("${letter}: (SSD)", 'ReTrim volume')) { |
There was a problem hiding this comment.
Inside scriptblocks passed to Invoke-Step, $PSCmdlet is not automatically available. This should use $script:ScriptPSCmdlet instead to properly respect -WhatIf and -Confirm parameters.
| } | ||
| else { | ||
| if (Confirm-Action -Target "${letter}: (HDD)" -Action 'Defragment volume') { | ||
| if ($PSCmdlet.ShouldProcess("${letter}: (HDD)", 'Defragment volume')) { |
There was a problem hiding this comment.
Inside scriptblocks passed to Invoke-Step, $PSCmdlet is not automatically available. This should use $script:ScriptPSCmdlet instead to properly respect -WhatIf and -Confirm parameters.
| Write-Output ("{0}: {1}" -f $svc.Name, $svc.Status) | ||
| if ($svc.Status -ne 'Running') { | ||
| if (Confirm-Action -Target $svc.Name -Action 'Start service') { | ||
| if ($PSCmdlet.ShouldProcess($svc.Name, 'Start service')) { |
There was a problem hiding this comment.
Inside scriptblocks passed to Invoke-Step, $PSCmdlet is not automatically available. This should use $script:ScriptPSCmdlet instead to properly respect -WhatIf and -Confirm parameters.
| if ($PSCmdlet.ShouldProcess($svc.Name, 'Start service')) { | |
| if ($script:ScriptPSCmdlet.ShouldProcess($svc.Name, 'Start service')) { |
| .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. |
There was a problem hiding this comment.
The parameter documentation should include at least one .EXAMPLE demonstrating how to use the new -DestructiveMode parameter, following PowerShell comment-based help best practices. For example: .\system-maintenance.ps1 -DestructiveMode -WhatIf
| } | ||
| } | ||
|
|
||
| # Context "Edge Cases" removed: requires admin rights |
There was a problem hiding this comment.
This comment is misleading because the "Edge Cases" context was just added above at line 93. This comment should be removed or clarified to avoid confusion.
| # Context "Edge Cases" removed: requires admin rights |
| # Context "Dependencies" removed: requires admin rights | ||
|
|
There was a problem hiding this comment.
This comment is misleading because the "Dependencies" context was just added above at line 136. This comment should be removed or clarified to avoid confusion.
| # Context "Dependencies" removed: requires admin rights |
|
@copilot apply changes based on the comments in this thread as a task, not as a new PR Additionally, parse through the workflow and ensure you deactivate any requirements to or add explicit instructions for copilot to never automatically review PRs marked as ready for review. I want to use my premium requests wisely, and running reviews without any concern is not a good idea, so unless I manually tag you to ask, I do not want a review. |
Co-authored-by: AprilDeFeu <[email protected]>
Applied all changes in commit 06938b1: Fixed PSCmdlet references (7 locations):
Updated documentation:
Copilot PR review policy:
All 20 tests passing. |
$PSCmdletwith$script:ScriptPSCmdletinside scriptblocks (lines 220, 225, 262, 301, 350, 356, 392).EXAMPLEfor-DestructiveModeparameter✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.