Add preview Windows NVMe boot-driver recovery script - #153
Edwin Bernal Microsoft (EdwinBernal1) merged 3 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved rollback-safety and registry-fixture correctness issues remain, including one critical finding.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a preview-only Windows PowerShell workflow for offline stornvme driver inspection, repair, and rollback.
Changes:
- Implements guarded Report, Repair, and Rollback modes with backup and verification.
- Adds fixture-based safety and recovery tests.
- Keeps the permanent
map.jsonentry unpublished pending validation.
File summaries
| File | Review findings |
|---|---|
tests/test-win-enable-nvme-boot-driver.ps1 |
Moderate (2 votes): Test fixtures infer registry types from CLR values, exporting ImagePath as REG_SZ instead of the required REG_EXPAND_SZ. |
src/windows/win-enable-nvme-boot-driver.ps1 |
Critical (2 votes): Rollback scope validation accepts deletion-section headers, allowing deletion of the selected service key. Moderate (1 vote each): Backup value tracking includes child sections; absent-value removal is not idempotent; rollback cannot restore a missing stornvme key. |
Review details
Suppressed comments (5)
src/windows/win-enable-nvme-boot-driver.ps1:164
- The post-repair check repeats a value-only comparison, so it can print VERIFIED when a required value has the right text/number but the wrong registry kind, such as REG_SZ ImagePath instead of REG_EXPAND_SZ. Read and compare
GetValueKind()here as well so verification proves the exact desired state.
foreach ($name in $desiredValues.Keys) {
if ($verified.$name -ne $desiredValues[$name].Value) {
throw "Verification failed for $servicePath\$name. Expected '$($desiredValues[$name].Value)', read '$($verified.$name)'. Roll back with Mode=Rollback BackupFile='$backupPath'."
src/windows/win-enable-nvme-boot-driver.ps1:148
- The backup folder and filename are derived only from the drive letter and a second-resolution timestamp, so concurrent or repeated runs in the same second can overwrite the only rollback copy with another run's state. Use a per-run unique directory or filename and refuse reuse before exporting.
$evidenceRoot = Join-Path $env:PUBLIC "Desktop\nvme-repair-$(Get-Date -Format yyyyMMddHHmmss)"
New-Item -Path $evidenceRoot -ItemType Directory -Force -ErrorAction Stop | Out-Null
$backupPath = Join-Path $evidenceRoot "$($offline.WindowsDrive.TrimEnd(':'))-stornvme-before.reg"
src/windows/win-enable-nvme-boot-driver.ps1:120
$backedUpValueNamesis extracted from every section in the .reg file, including allowed child keys belowstornvme. If a child key happens to contain a name such asImagePathwhile the service root omitted that value, the root value is incorrectly treated as backed up, left in place after import, and the exact comparison then fails with an incomplete rollback. Track value names only in the exact$expectedKeysection before removing missing root values.
$backedUpValueNames = @([regex]::Matches($backupText, '(?m)^\s*"([^"]+)"=') |
ForEach-Object { $_.Groups[1].Value })
foreach ($name in $desiredValues.Keys) {
if ($backedUpValueNames -contains $name) { continue }
[void](Assert-OfflineTarget -Path $servicePath -Action "remove the offline stornvme $name value during rollback")
src/windows/win-enable-nvme-boot-driver.ps1:121
- When a desired value is absent from the backup, rollback always calls
Remove-ItemPropertyafter import. On a repeated rollback, or when that value is already absent in the target, the removal throws and a valid backup is reported as failed instead of being an exact no-op. Make this removal idempotent by skipping it when the value is not present after import, and add a repeated-rollback fixture.
foreach ($name in $desiredValues.Keys) {
if ($backedUpValueNames -contains $name) { continue }
[void](Assert-OfflineTarget -Path $servicePath -Action "remove the offline stornvme $name value during rollback")
Remove-ItemProperty -LiteralPath $servicePath -Name $name -Force -ErrorAction Stop
src/windows/win-enable-nvme-boot-driver.ps1:76
- Rollback cannot restore a missing
stornvmekey because this presence check, followed immediately byGet-ItemProperty, runs before the Rollback branch. A valid Repair backup can recreate that key underServices, so defer the current-state key/value read for Rollback and let the guarded import plus export verification handle an absent target.
if ((Get-OfflineHiveKeyState -HiveKey $servicePath) -ne 'Present') {
throw "The offline stornvme service key '$servicePath' is not present. No changes were made."
}
$current = Get-ItemProperty -LiteralPath $servicePath -ErrorAction Stop
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved critical rollback-safety findings and additional repair safeguards require human validation.
Review details
Suppressed comments (3)
src/windows/win-enable-nvme-boot-driver.ps1:52
Test-OfflinePathonly checks existence, not that the path is a file, so a directory or other non-file item namedstornvme.syssatisfies this fail-closed driver gate and Repair can proceed without a driver binary. Check for a leaf file here (or add an equivalent file-type check to the helper) before changing the hive.
if (-not (Test-OfflinePath $driverPath)) {
src/windows/win-enable-nvme-boot-driver.ps1:173
- If a property write fails here after an earlier property was changed, the outer catch reports only the provider exception and does not include
$backupPath. That leaves a partially mutated offline service with no rollback path in the command output; wrap the post-export mutation/verification in error handling that preserves and reports the backup path.
foreach ($change in $changes) {
[void](Assert-OfflineTarget -Path $servicePath -Action "set the offline stornvme $($change.Name) value")
$desired = $desiredValues[$change.Name]
New-ItemProperty -LiteralPath $servicePath -Name $change.Name -Value $desired.Value -PropertyType $desired.Kind -Force -ErrorAction Stop | Out-Null
}
src/windows/win-enable-nvme-boot-driver.ps1:74
Get-OfflineHiveKeyStatecan returnUnknownfor access-denied or indeterminate reads, but this condition allows that state through wheneverMode=Rollback; the script can then import into a target whose existing state was never safely resolved. RejectUnknownbefore the rollback exception, while continuing to allowAbsentso a missing service key can be recreated.
if ($Mode -ne 'Rollback' -and $serviceKeyState -ne 'Present') {
throw "The offline stornvme service key '$servicePath' is not present. No changes were made."
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
Both rollback checks that could reject a backup ran after reg.exe import, so a refused file had already changed the offline hive. The exact-root-section requirement now runs before the import, and a new preflight refuses when the offline subtree holds a subkey or value the backup does not restore - reg.exe import merges, so those would have survived the rollback and only surfaced as a verification failure once the hive was already modified. Adds regression coverage for all three shapes; each fails against the previous script.
6765727
into
Azure:main
Summary
Adds the Windows recovery script for inspecting, repairing, and rolling back the offline
stornvmeboot-driver configuration. The script defaults to read-only Report mode, refuses ambiguous or unsafe targets, backs up before Repair writes, verifies exact desired values after Repair, and verifies exact backup restoration after Rollback.The permanent
win-enable-nvme-boot-driverentry is intentionally withheld frommap.json. Until binary SYSTEM-hive and live broken-VM validation are complete, test this branch through the work order's--previewworkflow.Requested by / source
Edwin Bernal /
WORKORDER-repair-scripts.mdType
[ ] Bug fix (patch) [x] Feature (preview-only; public run-id withheld) [ ] Breaking (major, sign-off attached) [ ] Docs
Changes
src/windows/win-enable-nvme-boot-driver.ps1stornvmeservice.stornvmesubtree and verifies exact restoration.tests/test-win-enable-nvme-boot-driver.ps1map.jsonVersion & changelog
setup.pyVERSION: not applicable; this PR does not change the Azure CLI extension.HISTORY.rstentry added: no; this PR changes only the repair-script-library.Testing
tests/test-win-enable-nvme-boot-driver.ps1: pass.tests/test-get-disk-partitions-v3.ps1: pass.map.jsonparse, unique IDs, and path resolution: pass (28 entries).git diff --check: pass.Remaining Release Gates
map.jsonrun-id only after SCR-2, SCR-4, and SCR-5 evidence is reviewed.Cross-repo impact (repair-script-library)
map.json/ script changes: adds an unregistered script for preview validation; nomap.jsonor Azure CLI extension change.az vm repair run --previewworkflow.Security review
Assert-OfflineTarget.stornvmesubtree, including mixed-hive files.Invoke-Expression,cmd /c, string-built shell command, CriticalDeviceDatabase write, credential handling, or secret logging is introduced.Backward compatibility