Fix various win-toggle-safe-mode errors and confirm gen2 compatible - #106
Fix various win-toggle-safe-mode errors and confirm gen2 compatible#106Josh S (Vorsku) wants to merge 2 commits into
Conversation
|
@microsoft-github-policy-service agree |
|
Adam Sandor (@Sandido) hoping you can review please? The current script doesn't work anyway due to the missing comments |
Edwin Bernal Microsoft (EdwinBernal1)
left a comment
Edwin Bernal Microsoft (EdwinBernal1)
left a comment
There was a problem hiding this comment.
Summary
Fixes four issues in win-toggle-safe-mode.ps1:
- Broken comment syntax (lines 48-49) — Missing
#on version history lines causes PowerShell parse failure, making the entire script unusable. [switch]$DCincompatible with AZ CLI —az vm repair run --parameterspasses all values as strings;[switch]never activates. Changed to[ValidateSet("Yes","No")][string].- Gen2 EFI partition not detected — EFI System Partition has no drive letter. New code falls back to
AccessPaths(volume GUID) for partition scanning. - Error handling adjustments —
Start-VMchanged toSilentlyContinue; catch block simplified.
What's Good
| Area | Detail |
|---|---|
| Comment syntax fix (lines 48-49) | Critical fix. The current main has v0.4: and v0.3: without # — PowerShell treats these as statements and the script fails to parse. This alone justifies the PR |
$DC switch → string |
Correct. az vm repair run --parameters DC=$true passes "$true" as a literal string, not a boolean. [switch] never activates. ValidateSet("Yes","No") is the right pattern |
| Volume GUID access paths | Gen2 VMs have an EFI System Partition without a drive letter. The new $driveCandidates logic correctly falls back to AccessPaths (e.g., \\?\Volume{...}\), enabling BCD discovery at \efi\microsoft\boot\bcd |
$osDrive tracking |
Fixes a real bug — the old code used $drive (loop variable) for registry operations after the loop, meaning it would reference the last iterated partition, not the one containing winload.exe |
$root normalization |
Clean handling of both "C:" and \\?\Volume{...}\ path formats for BCD/OS path construction |
Issues Found
🔴 Bug: $osDrive used with drive-letter syntax for registry paths
After the loop, the code uses $osDrive with drive-letter syntax:
# Line 167 (PR)
$regPath = $osDrive + ':\Windows\System32\config\'
# Line 176 (PR)
reg load "HKLM\BROKENSYSTEM" "$($osDrive):\Windows\System32\config\SYSTEM"If $osDrive is a volume GUID path (e.g., \\?\Volume{abc}\), appending :\Windows\... produces an invalid path like \\?\Volume{abc}\:\Windows\....
In practice this is low-risk because winload.exe lives on the OS partition which almost always has a drive letter (the letterless partition is typically just EFI). But for correctness, the fix should store a normalized root alongside $osDrive:
if ($isOsPath) {
$osDrive = $drive
$osRoot = $root # <-- store the normalized root too
}Then use $osRoot for all path construction after the loop:
$regPath = "${osRoot}\Windows\System32\config\"
reg load "HKLM\BROKENSYSTEM" "${osRoot}\Windows\System32\config\SYSTEM"🟡 Concern: Start-VM with -ErrorAction SilentlyContinue
start-vm $guestHyperVVirtualMachine -ErrorAction SilentlyContinue
#Sometimes the repair VM doesn't have enough memory to power it onThis silently swallows the failure, then returns $STATUS_SUCCESS. The user sees "success" but the nested VM is still stopped. A better approach:
try {
Start-VM $guestHyperVVirtualMachine -ErrorAction Stop
}
catch {
Log-Output "WARNING: Could not start nested VM (may need more memory). BCD changes were applied successfully." |
Tee-Object -FilePath $logFile -Append
}This still doesn't block the script but gives the user visibility.
🟡 Concern: Catch block no longer restarts the VM
The old catch block attempted to restart the nested VM after an error. The PR removes this:
# REMOVED:
# Start Hyper-V VM again
# Log-Output "#06 - Starting VM"
# start-vm $guestHyperVVirtualMachine -ErrorAction StopIf the script fails mid-operation, the nested VM is left stopped with its disk offline. The old behavior at least tried to restore the VM to a running state. Consider keeping this (with SilentlyContinue or a try/catch) so the user's nested environment isn't left broken.
🟢 Minor: Missing newline at end of file
The diff shows No newline at end of file on the last line. Should add a trailing newline.
🟢 Minor: Catch block log step number
Log-Output "#99 - Bringing disk offline to restart Hyper-V VM"Changing #05 to #99 is fine for disambiguation but #99 is arbitrary. Consider a naming convention like #ERR-01 to clearly indicate it's an error-path step.
|
Edwin Bernal Microsoft (@EdwinBernal1) could you review the changes please? |
VMRepair Script Test Report: win-toggle-safe-mode.ps1SummaryOverall Score: 30/100 (Grade: F) — BLOCK
Fault Injection Results
Testing PerformedStrategy: Native
Opportunities for Improvement70 points recoverable (current 30 → potential 100)
How to Reach 100/100
Validation EvidenceBreaker Output (Confirmed Corrupted): Repair Script Output (Failed): Root Cause DetailThe disk filter at line 89: $disk = get-disk -ErrorAction Stop | where { $_.FriendlyName -eq 'Msft Virtual Disk' }matches all Azure virtual disks including the repair VM's OS disk (Disk 0). When Suggested fix: $disk = Get-Disk -ErrorAction Stop | Where-Object {
$_.FriendlyName -eq 'Msft Virtual Disk' -and $_.IsBoot -eq $false
}Review Checklist
Test Artifacts
Generated by VMRepairMint Script Testing Agent | Test ID: |
3 participants
Hi,
Thanks for this project!
I've encountered various issues with the
win-toggle-safe-modescript which this PR fixes:I've tested this updated version successfully against a 2016 domain controller running in Azure as a Gen2:
Thanks,
Josh