Register the win-enable-nvme-boot-driver run id (SCR-3) - #154
Edwin Bernal Microsoft (EdwinBernal1) merged 2 commits into
Conversation
SCR-2, SCR-4 and SCR-5 all passed live validation on 2026-09-11: byte-level hive isolation proven by reg-export hashes on both SCSI and NVMe chassis, resource-disk exclusion confirmed bus-agnostic, and a full break-to-boot cycle that produced a real INACCESSIBLE_BOOT_DEVICE and recovered from it. The run id can now be resolved without --preview. Also adds a catalogue test, which the repository did not have. It asserts unique ids, existing paths, forward slashes, and agreement between the src/<os> directory and the file extension - a mismatch there would send a PowerShell script to a Linux guest. Removing the new entry makes it fail. And suppresses two PSReviewUnusedParameter false positives on Mode and BackupFile, which are read inside the Invoke-WithHive script block that PSScriptAnalyzer cannot see through. The script now analyses clean.
Edwin Bernal Microsoft (EdwinBernal1)
left a comment
There was a problem hiding this comment.
Reviewed the current head. The catalog entry matches the existing script and documents the required --run-on-repair usage. The added catalog test checks non-empty fields, unique IDs, existing paths, forward-slash paths, OS/extension agreement, and the expected NVMe registrations. I found no blocking issue.
Residual validation gap: GitHub currently reports only the passing CLA check, so the new PowerShell catalog test is not visible as an executed CI check in this PR.
✅ VMRepairMint Script Testing — APPROVEScope of review: static-analysis only. Grounded in the fact that the repair-script bytes are identical to what was end-to-end validated in #153 (SCR-2/4/5 with real What actually changes here
Local verification (executed on PR head @
|
| Check | Result |
|---|---|
pwsh -File ./tests/test-map-catalog.ps1 |
✅ PASS — 29 entries, all invariants hold |
pwsh -File ./tests/test-win-enable-nvme-boot-driver.ps1 |
✅ PASS — report, repair, idempotency, rollback, refusal, cleanup, strict-selection, write-gate |
Invoke-ScriptAnalyzer on PR head |
✅ 0 findings |
Invoke-ScriptAnalyzer on main (baseline) |
PSReviewUnusedParameter L26 Mode, L29 BackupFile) |
| PSA delta | 2 → 0 ✅ |
Byte-diff outside the attribute region on .ps1 |
✅ Empty — repair logic byte-identical to #153 |
All three test outcomes claimed in the PR description independently verified.
Why no VM-based fault-injection run for this PR
Per VMRepairMint governance, fault injection is mandatory for behavioral changes. Since the repair-script bytes are identical to #153 — where the full break-to-boot cycle was already validated end-to-end on WS2022 NVMe with real INACCESSIBLE_BOOT_DEVICE — re-injecting the same fault against the same bytes would replay #153, not test #154. The correct tests for this diff are:
- catalog test → PASS
- PSA delta → 2 → 0
- script self-test → PASS
All three ran.
Non-blocking suggestions (not merge-blockers)
- N1 — Tighten the suppression target.
SuppressMessageAttribute('PSReviewUnusedParameter', '')with an empty target suppresses the rule for every parameter inParam(), present and future. If a future edit adds a genuinely-unused parameter, PSA will stay silent. Consider:Understand the PR body notes per-parameter placement didn't work in every PSA variation — the script-scoped placement is the right portable choice; only the target argument is worth naming explicitly.[Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSReviewUnusedParameter', 'Mode')] [Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSReviewUnusedParameter', 'BackupFile')] Param( ... )
- N3 — Follow-up (not for this PR): consider a formal
map.schema.jsoninvoked viaTest-Json -Schemafromtest-map-catalog.ps1to additionally guard against wrong types and unknown fields.
Verdict
✅ APPROVE — minimal, reversible, well-scoped diff; makes the script #153 shipped actually callable through the default catalog; eliminates 2 PSA warnings; adds a regression test that will protect the catalog going forward.
Generated by VMRepairMint Script Testing subagent · static-analysis scope · no VMs were provisioned for this diff (behavior identical to already-validated #153).
There was a problem hiding this comment.
🟢 Approval recommended
The changes are low-risk and scoped, with only a minor conventions nit (suppression justification) noted for maintainability.
Pull request overview
Registers the permanent win-enable-nvme-boot-driver run-id in map.json so the default repair-script library can resolve it without --preview, and adds a basic catalogue test to prevent future map regressions.
Changes:
- Add the
win-enable-nvme-boot-driverentry tomap.json. - Add
tests/test-map-catalog.ps1to validatemap.jsonintegrity (unique ids, paths exist, OS/extension agreement, NVMe ids pinned). - Suppress
PSReviewUnusedParameterfindings insrc/windows/win-enable-nvme-boot-driver.ps1for parameters used inside a script block.
File summaries
| File | Description |
|---|---|
| tests/test-map-catalog.ps1 | New test to validate map.json entries and pin NVMe-related run ids. |
| src/windows/win-enable-nvme-boot-driver.ps1 | Adds a script-level PSScriptAnalyzer suppression for false-positive unused-parameter warnings. |
| map.json | Registers the permanent win-enable-nvme-boot-driver run id and description. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| { | ||
| "id": "win-enable-nvme-boot-driver", | ||
| "path": "src/windows/win-enable-nvme-boot-driver.ps1", | ||
| "description": "Enables the Windows inbox stornvme boot-start driver on the offline OS disk of a VM that no longer boots after switching to an NVMe disk controller. Mode=Report (default) is read-only; Mode=Repair backs up, writes and verifies; Mode=Rollback restores the backup emitted by Repair. NOTE: use option --run-on-repair." |
There was a problem hiding this comment.
what happens if a user runs it on the original VM? Should we add guardrails around it
Haider Agha (haagha)
left a comment
There was a problem hiding this comment.
Only one comment
06712d8
into
Azure:main
Summary
Registers the permanent
win-enable-nvme-boot-driverrun id inmap.json, closing SCR-3. The script itself merged in #153 with its run id deliberately withheld until three release gates were proven live. All three have now passed, soaz vm repair run --run-id win-enable-nvme-boot-driverresolves against the default library instead of requiring--preview.Two smaller changes ride along: a catalogue test the repository did not have, and a PSScriptAnalyzer suppression for two false positives.
Why the run id can be registered now
Validated 2026-09-11 against live Azure VMs, not fixtures:
$root\Services\stornvme, each gated byAssert-OfflineTarget. A sibling service (Services\disk) exported to a byte-identical hash before and after repair on both chassis (de02f1f9…), whilestornvmechanged by exactly one value:Start=dword:00000004→Start=dword:00000000reg exportcontent-diff, both chassisStandard_D2s_v3, SAS temp disk) and an NVMe chassis (Standard_D2ads_v6, NVMe temp disk). All four content hashes identical across chassis; first/last 8 MB of\\.\PhysicalDrive1unchanged on both. Behaviour is bus-agnosticscr4-scsi-result.json,scr4-nvme-result.jsonstornvme Start0→4, reboot produced a genuineINACCESSIBLE_BOOT_DEVICEwithSTATUS_OBJECT_NAME_NOT_FOUNDon the serial console,az vm repair create→ repair →az vm repair restore→ guest booted and the in-guest validator returned[BREAKER-VALIDATE]::HEALTHY. Rollback round-tripped back to 4, and a crafted out-of-scope backup was refused with the offline state preservedscr5-summary.jsonThe point of SCR-5 is that the script has now repaired a VM that was actually broken, verified by boot, not by its own return code.
Changes
map.json— adds thewin-enable-nvme-boot-driverentry. Five added lines; no reformatting of existing entries.tests/test-map-catalog.ps1(new) — the catalogue had no test at all. Asserts unique ids, that everypathresolves to a file that exists, forward slashes only (the path is used to build a URL), and that thesrc/<os>directory agrees with the file extension. That last check matters: a mismatch there hands a PowerShell script to a Linux guest. Also pins the three NVMe run ids so a future edit cannot quietly drop one.src/windows/win-enable-nvme-boot-driver.ps1— a script-levelSuppressMessageAttributeforPSReviewUnusedParameter.ModeandBackupFileare read inside theInvoke-WithHivescript block, which PSScriptAnalyzer cannot see through, so both were reported as unused on every run.Validation-report recommendations — applied, and not applied
The validation report closed with four recommendations. Three are addressed; one is declined on the evidence.
map.jsonPSReviewUnusedParameterfalse positivesvm-repairextension❌ Why recommendation 3 was not applied
The report states that the Rollback branch "returns
Outcome = 'RolledBack'(line 186), but no code path in the current script emits that string to the log/output", and that "currently only[STATUS]::SUCCESSis visible". It recommends addingLog-Info "Rollback completed"before the return.Both halves of that premise are incorrect. The script already emits a rollback-specific confirmation, at what is now L233:
So
[STATUS]::SUCCESSis not the only visible output — the operator also gets an explicit statement that the rollback was imported and verified, naming the backup file it came from. That is strictly more informative than "Rollback completed", which asserts less and names nothing.It is also already covered by a regression test in
tests/test-win-enable-nvme-boot-driver.ps1:Adding the suggested line would print a second, weaker confirmation immediately after the stronger one. No change made.
The report offers
[Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSReviewUnusedParameter','')]"above eachParam()block entry". That was tried first, in both the named-target form ('PSReviewUnusedParameter', 'Mode') and the empty-target form, applied per parameter insideParam(). PSScriptAnalyzer still reported both warnings in every variation. Only a single script-level attribute placed aboveParam(suppresses this rule, which is what this PR does. Recorded here so the next person does not repeat the experiment.Testing
The new test was verified to fail for the right reason: deleting the
win-enable-nvme-boot-driverentry makes it exit 1 withASSERTION FAILED.Behaviour change
--run-id win-enable-nvme-boot-driverbecomes resolvable from the default library. The script is unchanged apart from the analyzer attribute — same modes, same defaults,Reportstill the default, and nothing writes without an explicitMode=Repair.This is reversible. Removing the
map.jsonentry withdraws the run id without touching the script, and--previewcontinues to work either way.Cross-repo follow-ups (recommendation 4)
az vm repair createemitted "The repair VM size only supports NVMe…" to the log but not into the returned JSON, so an automated caller received a success payload with no way to know the repair VM it just created is one that several repair scripts cannot find a disk on. That PR returns warnings under awarningskey. This validation run was unaffected — the script here uses bus-agnostic discovery viaGet-OfflineWindowsDisk— but scripts that select disks by the SCSI model string would fail silently on an NVMe rescue chassis.--keep-rescue-vmflag onaz vm repair restorefor harnesses that need artefacts from the rescue VM. That capability already shipped in vm-repair 2.3.2 as--no-cleanup(alias--no), which keeps the repair VM and the disk copy. A second flag for the same behaviour would duplicate the surface; the gap is documentation, not capability.