Skip to content

Register the win-enable-nvme-boot-driver run id (SCR-3) - #154

Merged
Edwin Bernal Microsoft (EdwinBernal1) merged 2 commits into
Azure:mainfrom
EdwinBernal1:nvme-release-scr3
Sep 18, 2026
Merged

Edwin Bernal Microsoft (EdwinBernal1) merged 2 commits into
Azure:mainfrom
EdwinBernal1:nvme-release-scr3

Conversation

@EdwinBernal1

@EdwinBernal1 Edwin Bernal Microsoft (EdwinBernal1) commented Sep 15, 2026

Copy link
Copy Markdown
Member

Summary

Registers the permanent win-enable-nvme-boot-driver run id in map.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, so az vm repair run --run-id win-enable-nvme-boot-driver resolves 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:

Gate What was proven Evidence
SCR-2 — hive isolation All three registry mutation sites target $root\Services\stornvme, each gated by Assert-OfflineTarget. A sibling service (Services\disk) exported to a byte-identical hash before and after repair on both chassis (de02f1f9…), while stornvme changed by exactly one value: Start=dword:00000004Start=dword:00000000 reg export content-diff, both chassis
SCR-4 — resource-disk exclusion Repair ran correctly on a SCSI chassis (Standard_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 \\.\PhysicalDrive1 unchanged on both. Behaviour is bus-agnostic scr4-scsi-result.json, scr4-nvme-result.json
SCR-5 — break to boot Full ten-phase cycle on a WS2022 NVMe VM: breaker flipped stornvme Start 0→4, reboot produced a genuine INACCESSIBLE_BOOT_DEVICE with STATUS_OBJECT_NAME_NOT_FOUND on 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 preserved serial console logs, screenshots, scr5-summary.json

The 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 the win-enable-nvme-boot-driver entry. 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 every path resolves to a file that exists, forward slashes only (the path is used to build a URL), and that the src/<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-level SuppressMessageAttribute for PSReviewUnusedParameter. Mode and BackupFile are read inside the Invoke-WithHive script 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.

# Recommendation Outcome
1 Register the run id in map.json ✅ This PR
2 Silence the two PSReviewUnusedParameter false positives ✅ This PR — but not by the suggested method, see below
3 Add a log line so users can confirm Rollback ran Declined — the premise does not hold, see below
4 Two cross-repo signals for the vm-repair extension ➡️ Azure/azure-cli-extensions#10340, and one that needs no change

❌ 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]::SUCCESS is visible". It recommends adding Log-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:

'RolledBack' { Log-Output "VERIFIED: rollback backup was imported successfully from $($result.BackupFile)." }

So [STATUS]::SUCCESS is 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:

Assert-True ($rollback.Output -match 'rollback backup was imported') 'Rollback should report a verified import.'

Adding the suggested line would print a second, weaker confirmation immediately after the stronger one. No change made.

⚠️ Note on recommendation 2 — the suggested fix does not work

The report offers [Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSReviewUnusedParameter','')] "above each Param() block entry". That was tried first, in both the named-target form ('PSReviewUnusedParameter', 'Mode') and the empty-target form, applied per parameter inside Param(). PSScriptAnalyzer still reported both warnings in every variation. Only a single script-level attribute placed above Param( suppresses this rule, which is what this PR does. Recorded here so the next person does not repeat the experiment.

Testing

pwsh -NoProfile -File ./tests/test-map-catalog.ps1
  PASS: map.json catalogue — 29 entries, unique ids, existing paths, OS/extension agreement, NVMe run ids registered.

pwsh -NoProfile -File ./tests/test-win-enable-nvme-boot-driver.ps1
  PASS: win-enable-nvme-boot-driver report, repair, idempotency, rollback, refusal, cleanup, strict-selection, and write-gate fixtures.

Invoke-ScriptAnalyzer -Path ./src/windows/win-enable-nvme-boot-driver.ps1
  (no findings — was 2 warnings)

The new test was verified to fail for the right reason: deleting the win-enable-nvme-boot-driver entry makes it exit 1 with ASSERTION FAILED.

Behaviour change

--run-id win-enable-nvme-boot-driver becomes resolvable from the default library. The script is unchanged apart from the analyzer attribute — same modes, same defaults, Report still the default, and nothing writes without an explicit Mode=Repair.

This is reversible. Removing the map.json entry withdraws the run id without touching the script, and --preview continues to work either way.

Cross-repo follow-ups (recommendation 4)

  1. Raised as [vm-repair] Return command warnings in the output payload (2.4.3) azure-cli-extensions#10340. az vm repair create emitted "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 a warnings key. This validation run was unaffected — the script here uses bus-agnostic discovery via Get-OfflineWindowsDisk — but scripts that select disks by the SCSI model string would fail silently on an NVMe rescue chassis.
  2. No change needed. The report suggests a --keep-rescue-vm flag on az vm repair restore for 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.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@glimoli

Copy link
Copy Markdown
Contributor

✅ VMRepairMint Script Testing — APPROVE

Scope 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 stornvme\Start 0→4 breaker on WS2022 NVMe, serial-console INACCESSIBLE_BOOT_DEVICE, full break→repair→boot cycle). This PR is plumbing + hygiene, not behavior.

What actually changes here

# File Delta Purpose
1 map.json +5 Registers the win-enable-nvme-boot-driver run id (plus SCR-1/SCR-2 companions) in the default library catalog so az vm repair run --run-id … resolves. Without this, the script #153 merged isn't callable via the CLI's default catalog.
2 src/windows/win-enable-nvme-boot-driver.ps1 +3 [SuppressMessageAttribute('PSReviewUnusedParameter', '')] + 2 comment lines above Param(. Mode and BackupFile are consumed inside Invoke-WithHive via $PSBoundParameters, which PSA can't see through statically. Zero repair-logic bytes changed.
3 tests/test-map-catalog.ps1 (new) +56 New general catalog test: unique ids, existing paths, forward-slash separators, OS↔extension agreement, pins the three NVMe run ids. First regression guard for map.json.

Local verification (executed on PR head @ 3f660f9)

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) ⚠️ 2 warnings (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 in Param(), present and future. If a future edit adds a genuinely-unused parameter, PSA will stay silent. Consider:
    [Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSReviewUnusedParameter', 'Mode')]
    [Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSReviewUnusedParameter', 'BackupFile')]
    Param( ... )
    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.
  • N3 — Follow-up (not for this PR): consider a formal map.schema.json invoked via Test-Json -Schema from test-map-catalog.ps1 to 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).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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-driver entry to map.json.
  • Add tests/test-map-catalog.ps1 to validate map.json integrity (unique ids, paths exist, OS/extension agreement, NVMe ids pinned).
  • Suppress PSReviewUnusedParameter findings in src/windows/win-enable-nvme-boot-driver.ps1 for 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.

Comment thread src/windows/win-enable-nvme-boot-driver.ps1 Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Comment thread map.json
{
"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."

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if a user runs it on the original VM? Should we add guardrails around it

@haagha Haider Agha (haagha) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one comment

@EdwinBernal1
Edwin Bernal Microsoft (EdwinBernal1) merged commit 06712d8 into Azure:main Sep 18, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants