[vm-repair] Fetch scripts from the library's main branch, fix and validate --preview (2.4.2)

[Edwin Bernal Microsoft (EdwinBernal1)]

🤖 PR Validation — ️✔️ All clear

Breaking Changes
️✔️ None

Summary

vm-repair 2.4.2. This PR started as documentation for the NVMe platform-migration readiness checks, and grew to include the script-resolution and --preview fixes that testing those checks against a forked library uncovered. The behaviour changes are listed first because they are the part that affects users.

Behaviour changes

Scripts are resolved from the library's main branch instead of master. Azure/repair-script-library renamed its default branch. master only still resolves through a GitHub rename redirect, which GitHub is free to withdraw; if it were withdrawn, az vm repair run --run-id and az vm repair list-scripts would stop finding any script in the default library. Both the map.json lookup and the Linux run driver's bundle download were pinned to master; both now use main. The set of available scripts is unchanged, and invocations that pass --preview were never affected because they replace the library location before it is used.

--preview now works on Linux source VMs. The fork and branch taken from the preview URL were sent to the Linux VM but never consumed by the driver, so the run id was resolved from the fork while the scripts themselves were still downloaded from Azure. Worse, the two unused values were passed on to the repair script as its first two arguments, shifting every parameter supplied with --parameters. Previewing a forked library now behaves on Linux the way it already did on Windows.

Malformed --preview URLs are rejected instead of silently redirected. The URL used to be parsed positionally (parts[index('map.json') - 4]), so https://github.com/SomeUser/repair-script-library/blob/feature/nvme/map.json resolved the owner to repair-script-library and downloaded scripts from an unrelated GitHub organisation, reporting nothing. A URL is now matched against an explicit pattern: it must name a repair-script-library repository and the branch must be a single path segment. az vm repair run and az vm repair list-scripts both apply the check, and both apply it before the library location is replaced, so a rejected URL is never fetched from.

Documentation

• az vm repair run -h gains examples for checking whether a Windows or Linux guest is ready to boot from an NVMe controller. Both run the corresponding read-only readiness detector on the linked repair VM.
• README.md lists which platform-migration scenarios the extension can currently help with, so that an unsupported combination is not mistaken for a supported one, and states plainly that neither detector modifies the attached source OS disk.
• The --preview help text now states the single-path-segment branch requirement.

Changes

• azext_vm_repair/custom.py — PREVIEW_URL_PATTERN / _parse_preview_url / _build_repo_params replace the positional URL parser; run and list_scripts validate before calling _set_repair_map_url.
• azext_vm_repair/repair_utils.py — REPAIR_MAP_URL targets main.
• azext_vm_repair/scripts/linux-run-driver.sh — consumes repo_fork/repo_branch and defaults to main.
• azext_vm_repair/_params.py, azext_vm_repair/_help.py, README.md — help and docs.
• azext_vm_repair/tests/latest/test_repair_utils.py — regression tests.
• setup.py, HISTORY.rst — version and changelog.

Version & changelog

• setup.py VERSION: 2.4.1 -> 2.4.2
• HISTORY.rst entry added: yes

Testing

azext_vm_repair/tests/latest/test_repair_utils.py, 14 tests, all passing:

• RepairMapUrlTest — asserts the map URL and both run drivers agree on one branch, so a run id can no longer resolve from one branch and execute content from another.
• BuildRepoParamsTest — Linux always receives repo_fork/repo_branch positionally, Windows omits them without --preview, and URLs missing map.json, naming another repository, or carrying a slashed branch are rejected.
• PreviewUrlIsValidatedBeforeUseTest — run and list-scripts both raise before _set_repair_map_url is called, which is the regression guard for the review comment on custom.py.

No live Azure tests were run.

Cross-repo impact (repair-script-library)

None. No run id, script path, or map.json entry changes. The branch rename this PR follows already happened in that repository.

Security review

The positional --preview parser was the one input-handling defect here: a user-supplied URL could redirect script downloads to an arbitrary GitHub owner without any signal. It is now validated against an explicit pattern before use. No shell interpolation of untrusted input is introduced, and no secrets are logged.

Backward compatibility

No command, parameter, or default is removed or renamed. --preview URLs that were previously accepted and resolved to what the caller intended continue to work; the URLs that now fail are the ones that were resolving somewhere other than where they pointed.

[azure-client-tools-bot-prd]

Hi Edwin Bernal Microsoft (@EdwinBernal1),
Please write the description of changes which can be perceived by customers into HISTORY.rst.
If you want to release a new extension version, please update the version in pyproject.toml (or setup.py, if the extension has not migrated yet) as well.

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

🟡 Changes recommended

The Windows example uses an unavailable default map URL, and README wording needs clarification.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Documentation-only update adding NVMe readiness guidance and releasing vm-repair 2.4.2.

Changes:

• Adds Windows and Linux readiness detector examples to CLI help.
• Documents platform-migration scenarios and controller guidance.
• Updates version and changelog metadata.

File summaries

File	Summary
src/vm-repair/setup.py	Updates version from 2.4.1 to 2.4.2.
src/vm-repair/README.md	Documents NVMe migration scenarios; wording clarifications requested.
src/vm-repair/HISTORY.rst	Adds the 2.4.2 release entry.
src/vm-repair/azext_vm_repair/_help.py	Adds readiness examples; the default map URL issue requires correction.

Review details

Suppressed comments (2)

src/vm-repair/README.md:51

• The implementation does not prefer SCSI for every source VM: it only queries the repair SKU's controller capabilities and pins SCSI when the source controller is NVMe; otherwise it leaves the platform default. This unconditional wording can lead users migrating from a SCSI source to assume the repair VM was forced to SCSI when it was not. Scope the statement to NVMe source VMs.

`az vm repair create` selects a repair VM disk controller that can attach the copied OS disk, and
prefers SCSI whenever the repair VM size supports it, so that existing repair scripts can see the disk.

src/vm-repair/README.md:31

• Attaching the disk to a repair VM only makes offline inspection possible; it does not repair an NVMe boot-driver problem. This is especially misleading because the table below says boot-driver recovery is not available yet. Please describe the disk as being inspected (and repaired only when a supported repair exists).

(`0x7B`) on Windows, or a dracut emergency shell on Linux. The fault is inside the guest OS disk, so it
is repaired by attaching that disk to a repair VM.

• Files reviewed: 4/4 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.

[Yong Zhang (yonzhan)]

vm-repair

[Edwin Bernal Microsoft (EdwinBernal1)]

Review feedback addressed in 7322aaa. Summary of every point raised:

1. Default map URL still targets master (inline comment)

Fixed — but the diagnosis needed correcting. master does still resolve (HTTP 200, byte-identical to main, and it contains win-detect-nvme-readiness), because GitHub's branch-rename redirect is still serving it. The documented examples were not broken. The real problem is the dependency on that redirect, which affects every run-id lookup rather than just the NVMe ones, so REPAIR_MAP_URL now targets main and a regression test pins it. Details in the thread.

2. README overclaims that attaching the disk repairs the fault

Agreed — this was a genuine contradiction with the table immediately below it, which says boot-driver recovery is not available yet. Reworded to describe the disk as inspected, and repaired only where a repair exists.

3. README overclaims the SCSI repair VM preference

Agreed, and confirmed against _select_repair_disk_controller_type in repair_utils.py: SCSI is pinned only when the source VM controller is NVMe; every other source leaves the platform default for the repair VM size. The README now scopes the statement to NVMe source VMs and notes that --disk-controller-type overrides either path.

4. @azure-client-tools-bot-prd — changelog and version

Both were already in the original push and remain in place: HISTORY.rst has a customer-facing 2.4.2 entry, and setup.py is bumped 2.4.1 → 2.4.2. vm-repair has not migrated to pyproject.toml, so setup.py is the correct file. The HISTORY.rst entry has been extended to cover the map URL change, since that is user-visible.

Testing

test_repair_utils, test_disk_controller_type, test_telemetry_dimensions — 28 tests, all pass. All help entries parse. No repair-script-library/master references remain anywhere under src/vm-repair.

Scope note

The map URL fix makes this no longer strictly documentation-only. It is a one-line, content-identical change (master and main return the same 6347 bytes today), it ships with a regression test, and it is described in HISTORY.rst. Happy to split it into a separate PR if the reviewers would prefer to keep this one docs-only.

[Copilot]

🔵 Needs a closer look

The Linux driver still downloads from master while map resolution uses main; the documented clarifications also remain outstanding.

Review details

Suppressed comments (3)

src/vm-repair/HISTORY.rst:8

• This release note overstates the impact of losing the master redirect: commands that pass --preview replace the default map URL before fetching it, so they would continue to use their fork. Limit the statement to invocations using the default map (or explicitly exclude --preview) so the changelog accurately describes the compatibility impact.

Scripts are now looked up from the ``main`` branch of the repair script library instead of ``master``. The library renamed its default branch, and the old name only kept working through a redirect that can be withdrawn at any time; if that happened, every ``az vm repair run --run-id`` and ``az vm repair list-scripts`` invocation would fail to find any script. The set of available scripts is unchanged.

src/vm-repair/README.md:43

• These detectors are read-only with respect to the attached source OS disk, but they still write logs and evidence files on the repair VM (the Windows script uses the Public desktop and the Linux script uses /tmp). The unqualified “They make no changes” is broader than the actual behavior; clarify that only the attached source disk is not modified.

`--run-on-repair`. They make no changes.

src/vm-repair/azext_vm_repair/repair_utils.py:23

• This switches map lookups to the canonical main branch, but Linux script execution still downloads https://github.com/Azure/repair-script-library/tarball/master/ from azext_vm_repair/scripts/linux-run-driver.sh:13. As a result, the new Linux run-id can resolve from main and then fail (or remain dependent on the rename redirect) when the driver downloads the script bundle. Update the Linux driver to use main as well, and keep its branch selection aligned with the map/preview path.

REPAIR_MAP_URL = 'https://raw.githubusercontent.com/Azure/repair-script-library/main/map.json'

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Edwin Bernal Microsoft (EdwinBernal1)]

Round 2 addressed in ceb3480. All three suppressed comments were correct — the Linux driver one is the best catch in the review, and it is a bug my previous commit would have made worse.

1. Linux driver still on master — fixed, and it was a real inconsistency

Confirmed:

Component	Branch before	Branch now
REPAIR_MAP_URL (run-id resolution)	main (from 7322aaa)	main
win-run-driver.ps1 — $repo_branch default	main (already)	main
linux-run-driver.sh:13 — tarball/master/	master	main

So on Linux a run id resolved from one branch and then executed from a bundle downloaded from another. Harmless while the redirect keeps them identical, wrong the moment they diverge. Fixed.

I also extended the regression test rather than only pinning the constant — test_run_drivers_agree_with_the_map_url parses both driver scripts and asserts every declared library branch matches the map URL branch, so this specific drift cannot recur silently. Verified the regex catches tarball/master/ before concluding it guards anything.

2. HISTORY.rst overstated the --preview impact — fixed

Correct: _set_repair_map_url(preview) replaces REPAIR_MAP_URL before the fetch, so --preview users were never exposed to the redirect for map resolution. "Every invocation would fail" was wrong. The entry is now scoped to the default library and states explicitly that --preview is unaffected.

3. README — "They make no changes" was too broad — fixed

Verified in the scripts themselves:

• win-detect-nvme-readiness.ps1:44-45 → $env:PUBLIC\Desktop\...log and nvme-evidence-<timestamp>\, plus reg.exe export of a temporarily loaded hive
• linux-detect-nvme-readiness.sh:38 → /tmp/nvme-evidence-<timestamp>

Reworded to say the attached source OS disk is not modified, and that both write a log and evidence bundle on the repair VM.

---

One related bug found while fixing this — flagging, not fixing here

--preview does not work on Linux at all, independently of the branch issue. custom.py appends repo_fork= / repo_branch= to run_command_params for both platforms, but only win-run-driver.ps1 declares them as parameters. linux-run-driver.sh takes the script path as $1 and forwards $*, so those tokens are never honoured and are passed through to the repair script as stray positional arguments. A Linux --preview run therefore resolves the path from the fork's map.json and then downloads the bundle from Azure/main.

Fixing that means reworking the bash driver's argument handling, which is a functional change with its own test needs and does not belong in this PR. I am happy to open a separate issue or PR for it — let me know which the maintainers prefer.

Testing

43 unit tests (up from 41; 3 pre-existing modules still skip locally for a missing pytest, unrelated). All help entries parse. No repair-script-library/...master references remain anywhere under src/vm-repair.

[Edwin Bernal Microsoft (EdwinBernal1)]

Folding the Linux --preview fix into this PR rather than splitting it — pushed as 8850574.

The bug

custom.py appended repo_fork= / repo_branch= for both platforms, but only win-run-driver.ps1 declared them as Param(...). Linux run-command passes parameter values positionally, and linux-run-driver.sh read the script path from $1 and then forwarded $*. Two consequences:

1. The driver ignored the fork and branch entirely and always downloaded from Azure, so a Linux --preview run resolved the script path from the fork's map.json and then fetched the bundle from upstream.
2. Those two values were handed to the repair script as its own first two arguments, shifting every value supplied with --parameters.

The fix

• linux-run-driver.sh now consumes the fork and branch positionally and builds command_string from the remaining arguments, matching what the Windows driver already did with named parameters.
• The fork and branch are always sent for Linux, so the positions of the parameters that follow them are stable whether or not --preview is used.
• Extracted _build_repo_params() so the assembly is directly testable rather than inline in run().
• REPAIR_MAP_URL is now derived from REPAIR_LIBRARY_FORK / REPAIR_LIBRARY_BRANCH — the same constants the drivers default to. The map URL and the driver branch can no longer drift apart, which is exactly the defect the previous round found.

While extracting the helper I also fixed a latent bug: the old guard read parts.index('map.json') == -1, but str.index raises rather than returning -1, so a preview URL that was long enough but contained no map.json surfaced a bare ValueError instead of the intended message. Now covered by a test.

Verification

The driver logic change is the risky part, so I exercised it rather than reasoning about it — a harness reproducing the argument handling, run under bash across five shapes:

Scenario	Downloaded from	Command built
default run id, no params	Azure / main	./src/linux/linux-detect-nvme-readiness.sh
default run id, with params	Azure / main	./src/linux/linux-alar2.sh fstab initiator=SELFHELP
--preview fork/branch	SomeUser / my-branch	./src/linux/mine.sh p1 p2
custom script (no-op)	Azure / main	custom-script path taken
fewer than 3 args (defensive)	Azure / main	./src/linux/legacy.sh

The last row caught a defect in my own first attempt: with shift 3 guarded only by if [ $# -ge 3 ], a short argument list left $1 in $* and duplicated the script path. Corrected to else shift $#.

bash -n syntax check passes. 47 unit tests (up from 43), with the same 3 pre-existing modules skipping locally for a missing pytest.

Note for reviewers

This PR now contains a behaviour fix on a GA Linux path, not just documentation. The blast radius is the run-command parameter list and the bundle URL for Linux runs; Windows behaviour is untouched. No test recordings reference repo_fork / repo_branch, so none needed regenerating. Worth a careful look at linux-run-driver.sh specifically.

[Edwin Bernal Microsoft (EdwinBernal1)]

Pushed caba26c, and refreshed the PR title and description — the old body claimed "documentation-only, no behaviour change", which stopped being true two rounds ago.

One more fix, and it is the most serious thing found so far

While adding a test for the single-segment branch constraint documented in dec9a5f, I checked what actually happens when the constraint is violated. It is worse than a footgun:

--preview https://github.com/SomeUser/repair-script-library/blob/feature/nvme/map.json
  ->  repo_fork  = "repair-script-library"
      repo_branch = "nvme"
  ->  driver downloads https://github.com/repair-script-library/repair-script-library/tarball/nvme/

The URL is read positionally, so a slash in the branch name shifts the owner onto the repository name. The operator asked for their own fork, got a bundle from an unrelated GitHub organization — one that is not controlled by Azure and could be registered by anyone — and no error was reported. Those scripts then execute on the repair VM, which is attached to the customer's OS disk.

--preview is a script-author affordance, so this needs someone to be talked into pasting a URL rather than being remotely triggerable. But "paste this preview URL" is exactly the kind of instruction that circulates in support threads, and silent redirection to an attacker-registerable org is not a footgun I want to leave documented-but-unguarded.

The URL is now matched against the shape the driver actually requires — https://github.com/<fork>/repair-script-library/(blob|tree)/<single-segment-branch>/map.json — and anything else is rejected. tree URLs are accepted alongside blob since both are valid GitHub paths users copy from the address bar.

Also upgraded the failure from a bare ValueError traceback to InvalidArgumentValueError, which is the correct Azure CLI user-fault type.

Compatibility: the rejected inputs never worked. A slashed branch silently fetched from the wrong organization, and a non-repair-script-library repository was ignored entirely because the driver hard-codes the repository name. Failing closed is strictly an improvement over both.

49 unit tests now, same 3 pre-existing pytest import skips.

Still outstanding, deliberately not touched

_set_repair_map_url in repair_utils.py has a stray print(REPAIR_MAP_URL) that writes to stdout outside the CLI's output formatting, so it corrupts -o json when --preview is used. Unrelated to this PR's purpose and trivially separable — happy to fix here if reviewers want it, otherwise I will raise it separately.

CI

Checks are still pending with no runs recorded; the pipelines bot notes an authorized user must comment /azp run.

[Edwin Bernal Microsoft (EdwinBernal1)]

/azp run

[azure-pipelines]

Commenter does not have sufficient privileges for PR 10326 in repo Azure/azure-cli-extensions

[Copilot]

🟡 Changes recommended

Resolve the two moderate preview URL validation issues in custom.py.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/vm-repair/azext_vm_repair/custom.py:89

• The new strict parser is only reached through run()'s _build_repo_params() call. list_scripts() still sends --preview directly to _set_repair_map_url() and fetches the map, so URLs with another repository or a slashed branch are still accepted by az vm repair list-scripts --preview, contrary to the updated help text and validation contract. Validate the preview URL through the same parser before fetching the map in that command too.

def _build_repo_params(preview, is_linux):

• Files reviewed: 9/9 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Two moderate preview-URL handling issues remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 2
• Review effort level: Lite

[Ethan Yang (necusjz)]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[Edwin Bernal Microsoft (EdwinBernal1)]

Ran the pipeline's CheckExternalUrls job locally — it would have failed. Fixed in 680dcc4.

What it flagged

scripts/ci/validate_external_source_urls.py rejects newly added raw.githubusercontent.com URLs. Two lines in repair_utils.py tripped it:

ERROR: Found forbidden external GitHub URL(s) in this change:
  repair_utils.py: REPAIR_MAP_URL = 'https://raw.githubusercontent.com/{fork}/repair-script-library/{branch}/map.json'
  repair_utils.py: raw_url = raw_url.replace("https://github.com/", "https://raw.githubusercontent.com/", 1)

Worth noting for reviewers: neither URL is new to the extension. REPAIR_MAP_URL on main already points at raw.githubusercontent.com. The job works on added diff lines, so it fires simply because this PR touched them.

Why Option 1 does not apply

The tool's preferred fix is mirroring the file to azcliprod.blob.core.windows.net. That cannot work here:

• Azure/repair-script-library is the authoritative source. az vm repair run exists to fetch and execute scripts from it, and the library releases independently of this extension — mirroring would freeze the catalog against the repo it is meant to track.
• --preview deliberately resolves an arbitrary user fork, by design, so there is no file to mirror in the general case.

This is precisely the situation the tool describes as the fallback: "Only if the GitHub URL is required by design (e.g. the upstream repo IS the authoritative source)."

Applied

Documented inline suppression with a reason on each line:

# external-url-exempt: Azure/repair-script-library is the upstream source this command fetches scripts from
# external-url-exempt: --preview names a user fork, which cannot be mirrored internally

Re-ran the exact pipeline command afterwards:

python scripts/ci/validate_external_source_urls.py --src=HEAD --tgt=origin/main
No forbidden external GitHub URL found in added lines.

Status

azdev-linter, azdev-style, version-cal, version-output, block-merge, license/cla all pass. 52 unit tests, same 3 pre-existing pytest import skips. tree and blob preview URLs both still resolve to the correct raw map.

The remaining pending state is the Azure Pipelines build, which has no recorded runs — it needs /azp run from an authorized user.