Add the offline repair foundation helpers (1 of 3)

[Marcus Ferreira (mvaferreira)]

What this adds

The two foundation helpers that every offline repair scenario in this series depends on.

Helper	Used by	Purpose
OfflineRepairCommon.ps1	20	Shared primitives: buffered logging, drive-safe paths, offline binary trust checks, a read-only offreg hive reader, and offline-root binding
Get-OfflineWindowsDisk.ps1	20	Locates and prepares the offline Windows installation on a broken OS disk attached to a rescue VM

No map.json entries are added. Helpers are not run-ids, so nothing new is invocable yet.

This is #143, narrowed

This pull request is the original #143 reduced to its foundation. The review on it asked for a
smaller reviewable unit, and the helpers split cleanly along their real dependency lines into three
independent pull requests:

	Branch	Contents
A (this one)	rsl-pr1	OfflineRepairCommon.ps1, Get-OfflineWindowsDisk.ps1
B — #146	rsl-helpers-registry	Use-OfflineRegistryHive.ps1, Use-OfflineProtectedResource.ps1, Use-OfflinePrivilegedRegistry.ps1
C — #147	rsl-helpers-repair	Use-OfflineFileRemoval.ps1, Get-OfflineBcdStore.ps1, Use-NestedRepairVm.ps1

They can be merged in any order. B and C dot-source this one, but because no map.json entry
points at any helper, a helper that dot-sources a not-yet-merged helper is not reachable by
az vm repair run and so cannot break a shipped run-id. The twenty scenario pull requests that
follow do require all three, and they come afterwards.

The problem the review found, and what actually fixes it

The review's most serious findings all had one cause: nothing bound these helpers to the offline
disk. They run as SYSTEM on a rescue VM whose own healthy Windows is at C:, and every helper
trusted an ambient drive letter. A precondition that degraded quietly — a disk lookup that failed and
only warned, for instance — could redirect a privileged take-ownership-and-delete at the rescue VM
itself.

The fix is in this pull request, because this is where the drive letter is produced:

• Get-OfflineWindowsDisk.ps1 now throws instead of warning when it cannot establish which disk
  belongs to the rescue VM, and excludes that disk by IsBoot/IsSystem on both candidate
  filters, so exclusion no longer depends on a single lookup succeeding.
• Once a volume is chosen it calls Set-OfflineRepairRoot, binding the offline root for the run.
• Assert-OfflineTarget in OfflineRepairCommon.ps1 is the gate every destructive helper calls
  before enabling a privilege. It throws — it never warns and never returns $false — and it
  throws when nothing is bound, so a helper used without discovery fails closed rather than
  defaulting to the rescue VM.

This is a deliberate deviation from the literal review comment, which asked for the offline root as a
required parameter on each function. Taken literally that changes roughly forty signatures across
sixty-plus call sites. Binding once and asserting everywhere gives the same guarantee — no privileged
write outside the bound root — without that churn, and an optional -OfflineRoot parameter on the
public entry points still lets a caller be explicit. The reply on #143 argues this in full.

Other findings fixed here: NVMe disks are now visible (BusType selection, adopted from #144), the
Azure temporary disk is excluded, $DriveLetter is validated before it reaches a diskpart script,
actual disk state is checked before anything is logged as ready, temporary drive letters are
tracked and released from a caller's finally, and candidate ordering is deterministic.

September 10 review follow-up

• Disk state, not the exit code: Set-OfflineDisksOnline re-reads Get-Disk after diskpart
  and counts a disk only when it is online and writable. An already-ready disk receives no writes.
  Failed preparation also excludes the disk from installation discovery. On the live fixture,
  diskpart returned 0 while a read-only-backed VHD stayed read-only; the helper correctly
  refused to count it.
• No registry mounts for discovery: the SOFTWARE/SYSTEM probe now uses the shared read-only
  offreg.dll reader in OfflineRepairCommon.ps1. Recovery stays in memory, so there is no
  reg.exe load/unload, HKLM probe key or weaker duplicate unload implementation. Add the offline registry and protected-resource helpers (2 of 3) #146 uses this
  same reader for Test-OfflineHiveFile. Read failures remain visible in ProbeStatus; a failed
  close throws. The existing writable HKLM-based repair APIs remain unchanged.
• Language-independent resource-disk exclusion: either the Temporary Storage label or
  DATALOSS_WARNING_README.txt at a volume root excludes the disk. Existing drive letters,
  partition access paths and volume GUID paths are supported without mounting a disk.
• Required caller contract: the helper README includes a runnable try/catch/finally
  skeleton that logs errors, releases assigned letters, flushes buffered messages and returns
  $STATUS_ERROR or $STATUS_SUCCESS last. It distinguishes installation discovery from the
  partition-only v3 helper using the reviewer's suggested wording.

Relationship to Get-Disk-Partitions-v3

This branch includes upstream #144 and its #145 follow-up. It adds files alongside
Get-Disk-Partitions-v3.ps1 without reverting its fixes or README entry.

The two overlap but are not duplicates. Get-Disk-Partitions-v3 returns every partition of every
attached disk; Get-OfflineWindowsDisk identifies the Windows installation to repair, scores it
when there is more than one, locates its boot partition and BCD store, and binds it as the offline
root. Both select disks by BusType; installation discovery additionally requires verified
write readiness and binds the offline target. The README explains which helper to use.

Conventions followed

• Every repair is evidence-driven: nothing is written unless a specific fault is detected, so a
  healthy image produces no changes.
• No security protection is disabled as a workaround. Several changes here do the opposite.
• Logging is ordered so the verdict is last, because az vm run-command keeps only the final 4096
  characters of the output stream.

Testing

• PSScriptAnalyzer: 0 findings on both files, at every severity.
• 120/120 existing defect cases pass.
• 53/53 new follow-up cases pass on PowerShell 7 and Windows PowerShell 5.1, including the
  latter on the lab VM. They cover misleading diskpart exits, exclusions, zero writes on healthy
  disks, resource markers, metadata degradation, checked cleanup, native scalar reads and the
  exact README skeleton on success and discovery/dependency failures.
• 57/57 live assertions on Windows Server 2022: a disposable VHD with real VSS-copied hives
  exercised offline/read-only fault → preparation → discovery → hidden-partition letter cleanup.
  Discovery ran before loading Add the offline registry and protected-resource helpers (2 of 3) #146. Ten hive types opened, including four dirty hives with their
  logs; SHA-256 hashes, timestamps and file inventories stayed unchanged, and no probe/validation
  HKLM mounts appeared. A read-only attachment reproduced diskpart's masked failure. The VHD and
  VSS snapshot were removed, and the original disks' states were preserved.
• The earlier az vm repair create attached-OS-disk validation is recorded in the
  September 8 reply.
  Today's disposable-VHD helper run is not presented as --preview validation of all twenty
  scenarios. Those run-ids ship and receive end-to-end validation separately.

[Edwin Bernal Microsoft (EdwinBernal1)]

Overview

Marcus I suggest to split this in at least 3 pr in that way we reduce the iteration of the review

Metric	Value
Author	mvaferreira
State	Open, not draft
Mergeable state	blocked
Files changed	7 (all new)
Lines	+4893 / -0
Commits	1
Existing reviews	0
Existing comments	0
Checks	license/cla ✅ success (only check configured)
map.json changed	No — correct, helpers are not run-ids
Linked issue	None

Files

File	Lines	Role
Use-OfflineProtectedResource.ps1	1773	Ownership/ACL take-and-restore for TrustedInstaller-owned resources
Use-OfflineFileRemoval.ps1	672	Guarded file deletion with backup, verification, rollback
Use-NestedRepairVm.ps1	656	Drives the nested Hyper-V guest
Get-OfflineWindowsDisk.ps1	655	Locates/prepares the offline Windows install
Get-OfflineBcdStore.ps1	411	Offline BCD read/modify
Use-OfflineRegistryHive.ps1	407	Offline hive load/use/unload
OfflineRepairCommon.ps1	319	Buffered logging, drive-safe paths, trust checks

Static validation performed locally

Check	Result
PowerShell parser (all 7 files)	PASS — 0 errors
map.json diff vs main	PASS — unchanged
Hardcoded HTTP/HTTPS URLs	PASS — none
Write-Host usage	PASS — none
PSScriptAnalyzer	NOT RUN — module unavailable locally
Automated tests	NOT AVAILABLE — repo has none

---

🔴 Critical

Rescue VM can be targeted instead of the offline disk

File	Line	Issue	Fix
Get-OfflineWindowsDisk.ps1	497–509	If resolving the rescue VM's own system disk throws, the catch only warns and the exclusion array becomes empty. The rescue VM's live OS disk is then cleared read-only and its C:\Windows becomes a selectable candidate for downstream offline repairs.	throw instead of warn; add -and -not ($_.IsBoot -or $_.IsSystem) to both filters as defence in depth
Get-OfflineBcdStore.ps1	51–53	$BootDrive is unvalidated. Empty or '\' produces \Boot\BCD, which resolves to the rescue VM's own store. Invoke-BcdEdit adds no existence or drive check.	[ValidatePattern('^[A-Za-z]:\\?$')], plus store-existence and rescue-drive rejection in Invoke-BcdEdit
Use-OfflineProtectedResource.ps1	182–196, 743–752, 805, 1073, 1167	No function constrains its target to the offline image. Any HKLM:\… path or C:\Windows\System32 path is accepted, so a wrong drive letter redirects a privileged take-own-and-delete at the rescue VM's own OS.	Require the offline root / mounted-hive prefix as a parameter and hard-reject anything outside it before enabling privileges
Use-OfflineFileRemoval.ps1	156, 262–266	No offline-volume assertion, no $env:SystemRoot exclusion, and -Force enumerates reparse points unfiltered.	Require -OfflineRoot, assert the full path is under it, skip ReparsePoint items
OfflineRepairCommon.ps1	128–131	Join-OfflinePath -Root '\' passes the guard and returns a root-relative path resolving on the rescue VM's current drive.	Validate the trimmed root against `^([A-Za-z]:

Command injection / unsanitized input

File	Line	Issue	Fix
Get-OfflineBcdStore.ps1	392–395	Invoke-BcdEdit builds an interpolated string and runs it through cmd.exe /c. Caller-supplied $Command containing &, `	, >, ^executes as SYSTEM; a"in$StorePath` breaks the quoting.
Get-OfflineWindowsDisk.ps1	328–340	Add-PartitionDriveLetter is a public entry point with an unvalidated [string]$DriveLetter. TrimEnd(':','\') strips only trailing chars, so an embedded newline injects additional diskpart commands (e.g. select disk 0 / clean).	[ValidatePattern('^[A-Za-z]:?\\?$')] and re-assert ^[A-Z]$ after trimming

Deletion safety weaker than the PR claims

File	Line	Issue	Fix
Use-OfflineFileRemoval.ps1	94–99, 510	The "two-layer allow-list" is extension-only on both layers — the same test twice. A caller clearing .log1 .log2 .blf .regtrans-ms in System32\config (the documented use case) makes SYSTEM.LOG1, SOFTWARE.LOG2 and TxR .blf eligible. All six post-checks still pass, because those files are classified MatchedFile, not OtherFile.	Add a base-name veto: reject when GetFileNameWithoutExtension matches any HiveName, ordinal-ignore-case
Use-OfflineFileRemoval.ps1	400	Rollback enumerates the backup with -ErrorAction SilentlyContinue. An unreadable/empty backup yields zero items and returns Restored=0; Failed=0 — total rollback failure shaped exactly like success.	-ErrorAction Stop; fail when recovered count ≠ expected count
Use-OfflineFileRemoval.ps1	404	Rollback uses plain Copy-Item, not Copy-OfflineProtectedFile, so restore is refused precisely for files that needed ownership escalation to delete.	Route restores through Copy-OfflineProtectedFile
Use-OfflineFileRemoval.ps1	646, 664	$rollback.Failed is never inspected. Callers cannot distinguish "rolled back cleanly" from "folder is now permanently half-empty".	Add RollbackSucceeded/RollbackDetail; return a distinct fatal outcome
Use-OfflineFileRemoval.ps1	582–583	New-Item -Force neither requires nor clears an empty backup folder, so a reused Label re-injects stale files during rollback.	Unique per-run subfolder, or hard-fail on a non-empty folder

Registry hive handling does not deliver its headline guarantee

File	Line	Issue	Fix
Use-OfflineRegistryHive.ps1	184–193 vs 195	The mount loop runs outside the try. A second hive failing to mount strands the first with no finally to reach it, and leaves the depth counter poisoned.	Move the loop inside try, or wrap it with its own dismount-and-rethrow
Use-OfflineRegistryHive.ps1	196–210	& $ScriptBlock streams results to the caller, so a block returning a live RegistryKey (the documented usage pattern) keeps handles rooted and the unload deterministically fails.	Capture output locally, convert/dispose registry objects before finally
Use-OfflineRegistryHive.ps1	204–207	The depth entry is removed before the dismount and unconditionally; $null = Dismount-OfflineHive discards the $false failure. In-memory state claims unmounted while the file stays locked.	Remove only after a $true dismount; throw or flag on failure
Use-OfflineRegistryHive.ps1	341–371	Test-OfflineHiveFile copies the hive and its logs to %TEMP% with default ACLs, and SAM/SECURITY are in scope. The unload is Out-Null'd with no exit-code check, and cleanup is -ErrorAction SilentlyContinue — credential material can be left on the rescue VM.	Per-run directory with SYSTEM/Administrators-only ACL, exit-code-checked unload with retry, error on surviving scratch files

Ownership/ACL restore can be lost

File	Line	Issue	Fix
Use-OfflineProtectedResource.ps1	235–328	Grant-OfflineRegistryKeyAccess mutates owner and DACL in a loop with no finally; $captured is only returned on normal return. A throw mid-subtree loses every capture, permanently leaving keys owned by the rescue VM's SYSTEM with an explicit FullControl ACE.	Accept a caller-owned [ref]/list appended to per key
Use-OfflineProtectedResource.ps1	520–523	Get-ItemProperty -ErrorAction SilentlyContinue turns access-denied into $null, and the function returns Removed = $true; Reason = 'The value was not set.' A CBS marker is reported cleared when it was never read.	-ErrorAction Stop; on access-denied fall through to the ownership path
OfflineRepairCommon.ps1	211–222	IsMicrosoft/IsSigned are set $true from the unsigned Win32 version resource and for UnknownError/NotSupportedFileFormat. A planted or corrupt binary is reported as trusted Microsoft.	Keep IsMicrosoft = $false on both fallbacks; expose a separate Confidence/Status field

Nested Hyper-V disk hand-off

File	Line	Issue	Fix
Use-NestedRepairVm.ps1	418	Connect-NestedRepairVmDisk is passed the caller's full disk list, not $offlined. A disk skipped at 384 because it is the rescue VM's own boot/system disk is still handed to Add-VMHardDiskDrive.	-DiskNumber $offlined
Use-NestedRepairVm.ps1	374–431	No try/finally. Every early return after Set-Disk -IsOffline $true leaves the problem disk offline on the host with the guest not running, so the caller cannot read or restore it.	Wrap in try/finally; restore only the disks this call offlined
Use-NestedRepairVm.ps1	366–372	Already-running short-circuits to Started = $true before any disk attach — reporting success for exactly the diskless-guest defect this file exists to fix.	Verify Get-VMHardDiskDrive contains the requested disk before returning

---

🟡 Important

Portability — affects every file

All seven helpers bootstrap with a CWD-relative dot-source:

. .\src\windows\common\helpers\OfflineRepairCommon.ps1

az vm repair run copies the tree to the VM and does not guarantee the working directory is the repo root. The dot-source fails non-terminating, execution continues, and the first Add-OfflineRepairLog call throws far from the cause. If the CWD is writable by a lower-privileged principal this is also a code-load hijack surface.

Fix: . (Join-Path $PSScriptRoot 'OfflineRepairCommon.ps1') wrapped in try/catch { throw } so a missing dependency fails fast.

Affected lines: Get-OfflineBcdStore.ps1:34, Get-OfflineWindowsDisk.ps1:38, Use-OfflineRegistryHive.ps1:33, and the equivalent lines in the remaining helpers.

Failure indistinguishable from success

File	Line	Issue
Get-OfflineWindowsDisk.ps1	293–298	diskpart output discarded, $LASTEXITCODE unchecked, yet "Brought attached virtual disk(s) online" is logged unconditionally
Get-OfflineBcdStore.ps1	197, 232, 338	bcdedit.exe exit code never inspected; an empty inventory is indistinguishable from a failed enum
Get-OfflineBcdStore.ps1	367	Copy-Item without -ErrorAction Stop or verification, yet "Backed up…" is logged and a nonexistent path returned
Use-OfflineRegistryHive.ps1	119–122	reg query non-zero is treated as "not loaded → success"; access-denied reports a clean unload
Use-OfflineRegistryHive.ps1	134–149	Retry loop is well-bounded, but there is no post-unload verification
Use-OfflineProtectedResource.ps1	359–368, 884–893	Restore is counted, never verified — the same file byte-compares a registry value at 1748–1770 but not the descriptor
Use-OfflineProtectedResource.ps1	362–363	A key that cannot be reopened is skipped silently; Restored = 3 out of 40 reads as success
Use-NestedRepairVm.ps1	308–310	Failed Set-VMFirmware only warns; a Gen2 guest then PXE-boots into a 600 s timeout
Use-NestedRepairVm.ps1	612–618	Catch sets Reason but does not return; later branches overwrite it

Other important findings

File	Line	Issue	Fix
Get-OfflineWindowsDisk.ps1	282, 509	FriendlyName -like '*Virtual Disk*' matches SCSI only. Azure NVMe disks report Microsoft NVMe Direct Disk and fall through to the throw at 514.	Match on BusType -in @('SCSI','NVMe','SAS','RAID','File Backed Virtual')
Get-OfflineWindowsDisk.ps1	282, 509	The Azure temporary/resource disk is excluded by no signal; it is brought online and made writable	Exclude by FileSystemLabel -eq 'Temporary Storage'
Get-OfflineWindowsDisk.ps1	340–361	A single fixed 500 ms sleep after diskpart; on a slower volume stack the letter is leaked and the partition silently dropped	Poll ~10 s; release the letter on failure
Get-OfflineWindowsDisk.ps1	—	Temporary drive letters are never released; each run leaks up to two	Return assigned letters and expose a cleanup function for the caller's finally
Get-OfflineWindowsDisk.ps1	439	$Error.Clear() in finally wipes the caller's session-wide error history	Delete the line
Use-OfflineRegistryHive.ps1	75, 116	Fixed mount key BROKEN<HIVE> is reused without verifying its backing file — a leftover or concurrent mount silently retargets every read/write	Per-run unique key; verify backing file if reuse is kept
Use-OfflineProtectedResource.ps1	846–893	DACL round-tripped through SDDL parsed on a different machine; machine-relative aliases (LA, DA, DU, DC) and P/AI control flags are not asserted	Use GetSecurityDescriptorBinaryForm() / SetSecurityDescriptorBinaryForm()
Use-OfflineProtectedResource.ps1	534–537	The Grant catch returns without Restore-OfflineRegistrySecurity, unlike the equivalent paths at 649 and 731	Mirror line 651
Use-OfflineProtectedResource.ps1	640, 665	& $Action runs bare in the pipeline, so caller scriptblock output contaminates the returned object	[void](& $Action)
Use-OfflineFileRemoval.ps1	486–492	Post-check 4 is half tautological — $unexpectedlyGone is always empty by construction	Compute independently of $Removed
Use-OfflineFileRemoval.ps1	510–522	Post-check 6 is vacuous when no hive loaded beforehand — the normal state of a non-booting disk — and reports PASS	Report INCONCLUSIVE; require explicit opt-in
Use-OfflineFileRemoval.ps1	331–336, 407–409	Backup never captures the source security descriptor, so rollback restores bytes but not owner/DACL	Record and replay SDDL
Use-NestedRepairVm.ps1	425–431	No insufficient-memory handling, which the repo checklist calls out explicitly for nested Hyper-V	Compare startup memory to free physical memory, retry with reduced/dynamic memory
Use-NestedRepairVm.ps1	514	Wait loop exits early only on Off; PausedCritical (out of memory/disk) burns the full 600 s	Exit on Off, Paused, Saved, PausedCritical
Use-NestedRepairVm.ps1	93	vmms service state never checked, so role+module present with the service stopped reports Supported = $true	Require vmms running
OfflineRepairCommon.ps1	70–76	The buffer is cleared before entries are written; if init.ps1 was not sourced, Log-Warning throws and every buffered message is lost	Flush first, clear in finally, fall back to Write-Output
OfflineRepairCommon.ps1	38–98	$script: binds to the running script's scope at call time, not the defining file, so buffered entries can land in a child scope the parent never flushes	Use $global: or wrap the file in New-Module

---

🔵 Suggestions

• Get-OfflineBcdStore.ps1:139–240 — all parsing keys off English bcdedit field names and section titles, silently yielding an empty inventory on a localized image. Consider the locale-neutral Root\WMI BCD provider.
• Use-OfflineProtectedResource.ps1 — 1773 lines containing two distinct subsystems; lines 1207–1773 (#region Privileged registry access) belong in their own file, and the three near-identical parent-take → child-take → act → restore bodies should be factored onto one Invoke-OfflineProtectedFileOperation -Action {…}.
• Use-OfflineProtectedResource.ps1:325–376 — blocking GC::Collect() + WaitForPendingFinalizers() on every grant and restore; expose a single Clear-OfflineRegistryHandle instead.
• Use-OfflineFileRemoval.ps1:95 — exclusion of extensionless files is what keeps bare SYSTEM/SOFTWARE safe today. This is load-bearing, undocumented and untested — add a comment and a test.
• Get-OfflineWindowsDisk.ps1:581–587 — DiskNumber is missing from the sort keys, so two broken disks with equal score select nondeterministically and ambiguity only warns.
• Use-NestedRepairVm.ps1:360–622 — Get-VM -Name treats the value as a wildcard; resolve by -Id.
• Use-NestedRepairVm.ps1:26–35 — the header omits Connect-NestedRepairVmDisk and Stop-NestedRepairVmGraceful from the exposed-function list.
• OfflineRepairCommon.ps1:206 — -match 'O=Microsoft Corporation' is unanchored; O=Not Microsoft Corporation Inc matches.

---

Operational Risk Assessment

Factor	Rating	Notes
Scope	High	Seven shared helpers intended as the foundation for 20 follow-on repair scenarios
Destructive operations	High	File deletion in System32\config, registry hive load/unload, BCD modification, ownership/ACL changes, disk online/offline
Rollback possible	Partial	Rollback exists and is well designed, but can fail silently (400), be refused by the ACL that forced escalation (404), or restore stale files (582)
Testing documented	Partial	Real-VM testing via az vm repair run --preview is described, but no per-helper evidence, matrix, or output is included
Gen1/Gen2 compatibility	Partial	BCD paths handle both; Gen2 nested-guest firmware failure is only warned (308)
Blast radius on the rescue VM	High	Multiple paths can target the rescue VM's own OS when a precondition silently fails

Overall Risk: 🔴 High

---

Verdict

Request changes.

The engineering quality here is genuinely above the repo norm — the buffered-logging design correctly solves the real problem that Logger.ps1 writes on the same stream a function returns on, the single-enumeration deletion plan with a hash-verified pre-deletion backup is the right architecture, Use-OfflineProtectedResource captures descriptors before changing them and never shells out to takeown.exe/icacls.exe, and the nested-VM timeout is honestly reported as a non-boot rather than converted into success. The documentation headers are the best in the repository.

The problem is that the failure paths do not uphold the guarantees the headers advertise, and these scripts run as SYSTEM against a disk that is already unbootable:

1. Nothing binds these helpers to the offline disk. Five separate paths can act on the rescue VM's own OS when a precondition silently degrades.
2. Invoke-BcdEdit shells an interpolated string through cmd.exe, and Add-PartitionDriveLetter interpolates an unvalidated string into a diskpart script.
3. The "two-layer" allow-list in Use-OfflineFileRemoval is one test performed twice, so hive recovery logs are removable in the documented use case while all six post-checks report PASS — and the rollback that is supposed to catch this can return "restored 0, failed 0".
4. Use-OfflineRegistryHive can strand a mounted hive (mount loop outside the try) and discards unload failures, which is the exact condition it was written to prevent.

Recommended before merge

Priority	Item
Must fix	Offline-root binding on every helper that writes
Must fix	Remove cmd.exe from Invoke-BcdEdit; parameterize arguments
Must fix	Validate $DriveLetter in Add-PartitionDriveLetter
Must fix	Base-name veto for hive files in Use-OfflineFileRemoval
Must fix	Move the hive mount loop inside try; surface unload failures
Must fix	try/finally around the nested-VM disk hand-off; pass $offlined
Must fix	Capture list must survive a partial Grant-OfflineRegistryKeyAccess failure
Must fix	%TEMP% hive copy needs an explicit ACL and verified cleanup
Should fix	$PSScriptRoot-based dot-sourcing in all seven files
Should fix	Exit-code checking for diskpart, bcdedit, reg
Should fix	NVMe bus-type matching and Temporary Storage exclusion in Get-OfflineWindowsDisk
Should fix	Binary-form ACL round-trip instead of SDDL
Ask	Per-helper test evidence, and PSScriptAnalyzer results
Ask	Split Use-OfflineProtectedResource.ps1 at the #region Privileged registry access boundary

Process note

The batching rationale in the PR description is sound — landing helpers first keeps the 20 follow-on scenario PRs independently reviewable. However, 4893 lines of security-sensitive, SYSTEM-privileged code in a single review is a lot to hold. Consider splitting into two or three PRs along dependency lines (OfflineRepairCommon + Get-OfflineWindowsDisk first, then the destructive helpers), which preserves the independence argument while making each review tractable.

Overlap with in-flight work

Get-OfflineWindowsDisk.ps1 overlaps functionally with the pending Get-Disk-Partitions-v3.ps1 work on branch nvme-migration (NVMe-aware disk discovery). Both solve attached-disk discovery; this PR's version does not handle NVMe (line 282/509 matches the SCSI model string only), while v3 selects by BusType. Worth raising on the PR so the two do not diverge into competing helpers.

[Marcus Ferreira (mvaferreira)]

Reply to the #143 review — point-by-point matrix

Point-by-point reply. Every finding in the review is answered below.

---

Thank you for this review. It was specific, it was correct, and it was clearly a lot of work — every
critical finding I checked was real, and I checked all of them against the branch source rather than
taking them on trust. Nothing in it was a false positive.

You identified the thing that ties the whole set together better than I had: nothing bound these
helpers to the offline disk. They run as SYSTEM on a rescue VM whose own healthy Windows is at C:,
and I had been treating "the caller passed us the right drive letter" as an invariant when it is
actually the single assumption most likely to fail. Everything else follows from that.

What changed structurally

The PR is now three PRs, which addresses your point about surface area. 7 files and +4893 lines
against a repo whose merged PRs touch 1-2 files was not a reasonable thing to ask one reviewer for.

PR	Helpers	~Lines
A — this PR (#143) — foundation	OfflineRepairCommon, Get-OfflineWindowsDisk	1763
B — #146 — registry & protected resources	Use-OfflineRegistryHive, Use-OfflineProtectedResource, Use-OfflinePrivilegedRegistry (new)	~2250
C — #147 — destructive, boot & nested	Use-OfflineFileRemoval, Get-OfflineBcdStore, Use-NestedRepairVm	~1800

They are independent and can merge in any order, because no map.json entry points at a helper — a
helper that dot-sources a not-yet-merged helper is unreachable by az vm repair run, so it cannot
break a shipped run-id. This PR becomes A rather than being closed, so this thread survives.

I also split Use-OfflineProtectedResource.ps1 at its #region Privileged registry access
boundary, as you suggested. 7 self-contained functions, all named *OfflinePrivileged*, now live in
Use-OfflinePrivilegedRegistry.ps1; only two scenarios call into that region.

If you would prefer a different split, say so before you start reading — restructuring the
branches is cheap, re-reviewing is not.

One deliberate deviation, flagged up front

You asked for the offline root to be a required parameter, hard-rejecting anything outside it. I
have implemented the guarantee but not that signature, and I want to be explicit about why rather
than let you find it.

Requiring it as a parameter changes ~40 function signatures across 60+ call sites, because the
scenario scripts call Grant-OfflinePathAccess, Invoke-OfflineProtectedKeyRemoval and
Restore-OfflineRegistrySecurity directly, not only through the helpers. That is a large blast
radius for a guarantee that can be enforced at the point of use.

Instead the root is bound once and asserted everywhere:

• Set-OfflineRepairRoot binds the volume when Get-OfflineWindowsDisk selects it.
• Register-OfflineHiveKey records each hive as Use-OfflineRegistryHive mounts it.
• Assert-OfflineTarget is the single gate, called by every function that writes, deletes, takes
  ownership or changes a security descriptor — before any privilege is enabled.

It throws. It never warns and never returns $false. Critically, it also throws when nothing
is bound, so the failure mode you identified — a degraded precondition leaving the guard inert — is
the one case it is guaranteed to catch. It additionally refuses anything under the rescue VM's own
$env:SystemRoot, and Set-OfflineRepairRoot refuses to bind $env:SystemDrive at all.

Public entry points also take an optional -OfflineRoot, so a caller that wants to be explicit
gets exactly the form you asked for.

If you would still rather have the mandatory parameter, I will do it — it is mechanical, just wide.

---

OfflineRepairCommon.ps1

Your finding	Status	What I did
38-98 — $script: state binds to the sourcing scope	Fixed	Moved to a shared global via Get-OfflineRepairState. You offered $global: or New-Module; I took $global: because this codebase dot-sources helpers from inside functions, and each az vm repair run is a fresh process so there is nothing to leak into. Clear-OfflineRepairRoot exists for tests
70-76 — buffer cleared before entries are written	Fixed	Flushes first, clears in finally, and falls back to Write-Output when init.ps1 was not sourced. Previously a missing Log-Info discarded the whole buffer unwritten
128-131 — Join-OfflinePath -Root '\' passes the guard	Fixed, then fixed again	See below — my first fix for this caused the worst regression in the whole change
206 — unanchored O=Microsoft Corporation	Fixed	Anchored on the RDN boundary: (?:^|,)\s*O=Microsoft Corporation\s*(?:,|$). The subject that motivated it — CN=O=Microsoft Corporation, O=Evil Corp — no longer matches
210-223 — IsMicrosoft = $true from the unsigned version resource and on UnknownError	Fixed, and split three ways	See below — the literal fix would have caused a regression
CWD-relative dot-source	Fixed	$PSScriptRoot, wrapped in try/catch { throw }
new	—	Set-OfflineRepairRoot, Get-OfflineRepairRoot, Clear-OfflineRepairRoot, Register-OfflineHiveKey, Unregister-OfflineHiveKey, Get-OfflineHiveKey, Assert-OfflineTarget

Join-OfflinePath — the fix for your finding was worse than the finding

This is the one I would most like you to check, because I got it wrong and only caught it on a real
rescue VM.

Your finding was correct: -Root '\' passed the emptiness check and produced a root-relative path.
My first fix required the root to match the volume-root pattern
^([A-Za-z]:|\\\\[^\\]+\\[^\\]+)$. That does reject '\' — and it also rejects 'E:\Windows'.

Join-OfflinePath is called 77 times, and 33 of those pass a nested root: everything built under
$windowsPath, $ConfigPath, $windowsRoot, $ScratchDir and $regBack. All of them silently
returned $null. Test-OfflinePath $null is $false, so every one of those paths reported file not
present — a repair would look at a broken system, find nothing to fix, and report success. That is
strictly worse than the bug I was fixing, and it is exactly the silent-degradation class your review
was written about.

Two things are worth saying about how it was found, because neither was code review:

• The unit tests did not catch it. They asserted the guard rejected '\', which it did. Nothing
  asserted the guard still accepted an ordinary path. A test that only proves the door is shut does
  not prove you can still walk through it.
• The rescue-VM run passed 13 of 14 and still exposed it. The offline hive probe reported
  SYSTEM hive present = False, SOFTWARE hive present = False for a volume the very next assertion
  proved held E:\Windows. Two outputs that cannot both be true. I only had that string to look at
  because of the ProbeStatus field added in the previous round, which itself came from noticing an
  empty product name in an otherwise green 11/14 run.

The corrected guard uses a separate pattern,
^([A-Za-z]:|\\\\[^\\]+\\[^\\]+)(\\[^\\]+)*$ — volume-qualified at any depth. A repair root, which
genuinely must be a volume root, still uses the original pattern. It is also worth recording that
ConvertTo-OfflineComparablePath strips leading separators, so '\' collapses to empty and is
refused before any pattern is consulted; the pattern's real job is to catch the residue, such as
'\Windows' arriving as 'Windows'.

There are now 11 cases on this function — 9 behavioural, covering both directions — and the three
nested-root ones fail against the tree that carried the regression.

Having shipped one over-tight guard, I audited the rest rather than assuming it was isolated. Every
ValidatePattern left in the helpers constrains a drive letter, which genuinely is a volume root.
The one remaining allowlist is the ValidateSet of hive names, so I resolved all 55 -Hive
arguments across the tree by AST — including the array and variable forms, such as
$servicingHive, which is @('SOFTWARE','COMPONENTS') — and every one falls inside the set. No
second instance.

Test-OfflineFileSignature — fixed, but not literally

Setting IsMicrosoft = $false on both fallbacks is correct, and I did it. But applied on its own it
breaks three callers, so I want to explain what else had to move.

Offline, most inbox binaries are catalog signed, and the catalogs that would verify them are on the
offline image — not registered on the rescue VM. So Get-AuthenticodeSignature cannot prove a
perfectly healthy winlogon.exe. Three scenarios were using IsMicrosoft to decide whether it was
safe to act, and a strict flag would have made them refuse to repair healthy VMs — and made
win-fix-bcd rebuild the boot store on every one of them.

So the signal is now three fields:

Field	Meaning
IsMicrosoft	Cryptographically proven: valid Authenticode and an anchored Microsoft RDN
IsLikelyMicrosoft	Proven, or claimed by the version resource — and never true when Authenticode actively said the file is bad
Confidence	High when Authenticode answered definitively, Low when only the version resource was available, None when nothing could be established

IsSigned is also now true only for a valid Authenticode signature; the version-resource path used
to claim it. 'CatalogSigned' is preserved as a status string because a caller matches on it.

Measured on real binaries:

File	Status	IsMicrosoft	IsLikelyMicrosoft	Confidence
bootmgfw.efi (healthy)	Valid	✔	✔	High
kernel32.dll (healthy)	Valid	✔	✔	High
kernel32.dll with one byte flipped	HashMismatch	✘	✘	High
valid signature, non-Microsoft signer	Valid	✘	✘	High
unsigned junk, no version resource	NotVerifiable	✘	✘	None

Note the third row: a tampered Microsoft binary still carries CompanyName = Microsoft Corporation,
and IsLikelyMicrosoft correctly refuses it, because a definitive Authenticode answer overrides the
version-resource claim. That is what makes the advisory field safe to use.

The callers were updated accordingly: the paths that decide whether to act use
IsLikelyMicrosoft, and the path that decides whether a boot manager is damaged requires
Confidence -eq 'High' -and -not $signature.IsMicrosoft.

---

Get-OfflineBcdStore.ps1

Your finding	Status	What I did
392-395 — cmd.exe /c with an interpolated command string	Fixed	Invoke-BcdEdit now takes [string[]]$Arguments and calls & bcdedit.exe /store $StorePath @Arguments. No shell
51-53 — $BootDrive unvalidated	Fixed	[ValidatePattern('^[A-Za-z]:\\?$')], plus a new Assert-OfflineBcdStorePath that rejects an unrooted path, rejects the rescue VM's own system drive outright, and defers to Assert-OfflineTarget once a root is bound
197, 232, 338 — bcdedit exit code never inspected	Fixed	New Invoke-BcdEnum returns Success/ExitCode/Text; Get-BcdInventory gained Exists, EnumSucceeded and EnumExitCode, so "the store could not be opened" is no longer indistinguishable from "the store is empty" — which mattered, because an empty inventory is what makes win-fix-bcd decide to rebuild
367 — backup copy unverified	Fixed	-ErrorAction Stop, then the copy is confirmed to exist and to match the source length, before anything is logged as backed up

You were right that this was the most serious one. The identifiers reaching Invoke-BcdEdit are
parsed out of the broken VM's own store, so they are attacker-controlled. Measured both ways with a
payload of {default} & echo PWNED-INJECTION-RAN:

OLD (cmd.exe /c, interpolated):  the injected command executed
NEW (direct invocation, array):  ARG=</set>
                                 ARG=<{default} & echo PWNED-INJECTION-RAN>
                                 ARG=<recoveryenabled>
                                 ARG=<No>
                                 -> 0 executions, payload delivered as one literal argument

win-fix-bcd.ps1 was updated for the new signature (3 call sites).

---

Use-OfflineRegistryHive.ps1

Your finding	Status	Detail
Mount loop sits outside the try (184-195)	Fixed	Mounting moved inside the try; each acquired hive tracked and unwound in reverse by the finally, so a second-hive failure no longer strands the first
Scriptblock output can keep a RegistryKey alive (196-210)	Fixed	Output is walked once and only live registry objects are replaced with disconnected snapshots exposing the same value members; everything else is returned by reference
Depth entry removed before dismount; $null = discards the failure (203-207)	Fixed	Entry removed only after a confirmed unload; failures collected and Invoke-WithHive throws naming the hives still loaded and the reg unload command to clear them
Non-zero reg query treated as "not loaded → success" (119-122)	Fixed	Key state is now Present / Absent / Unknown; only the genuine not-found message means Absent, so access-denied is never read as a clean unload
No verification the hive is gone after unload (134-149)	Fixed	Unload retries and then re-queries; returns $true only on a confirmed absence
%TEMP% hive copy is unprotected credential material (341-371)	Fixed	Per-run directory created empty and locked to SYSTEM + Administrators with inheritance disabled before any bytes are written; surviving files reported as an error
Fixed BROKEN<HIVE> mount key (75, 116)	Fixed differently	See below

Mount key — kept, with the risk closed a different way

You asked for a per-run unique key. I measured the blast radius first: 35 references across 13
scenario scripts hardcode HKLM:\BROKEN<HIVE> inside their scriptblocks (for example
'HKLM:\BROKENSYSTEM\Setup'). A random key would have been 35 edits and a large regression surface
for no gain inside a single SYSTEM-owned run.

The real risk you identified — inheriting a stale or foreign mount — is instead closed fail-closed:
an existing key is reused only after Test-OfflineFileInUse proves it is backed by this disk's hive
file, and otherwise the helper throws and names the reg unload command. The unique-key idiom is
used where nothing depends on the name: the scratch validation copy mounts as RSLVALIDATE<guid8>.

Evidence the output-capture change is safe. All 53 Invoke-WithHive call sites across 15
scenario files were enumerated by AST and classified — not sampled. 40 consume the return value:
37 return a detached [PSCustomObject], 2 return Get-ItemProperty output, and 1 returns an array of
strings. Zero return a live RegistryKey, so no scenario script needed an edit. Verified against
the real shapes with a read-only registry read: a Get-ItemProperty bag keeps its values with the
provider link severed; arrays and PSCustomObjects are not rebuilt at all.

---

Use-OfflineFileRemoval.ps1

Your finding	Status	Detail
"Two-layer" guard is extension-only on both layers (94-99, 510)	Fixed	Base-name veto added: a file is refused when GetFileNameWithoutExtension matches any HiveName, ordinal-ignore-case, before the extension logic runs
Nothing binds removal to the offline disk (156, 262-266)	Fixed	Optional -OfflineRoot; Assert-OfflineTarget on the set root and on every resolved file path; reparse points are no longer followed out of the image
Rollback enumerates with -ErrorAction SilentlyContinue (400)	Fixed	-ErrorAction Stop; success now requires Failed -eq 0 -and Restored -eq Expected; a missing or unreadable backup folder is an explicit failure
Rollback uses plain Copy-Item (404)	Fixed	Restores route through Copy-OfflineProtectedFile
Failed rollback invisible to the caller (646, 664)	Fixed	RollbackAttempted / RollbackSucceeded / RollbackDetail added; a failed rollback returns a distinct fatal outcome
Backup folder reused across runs (582-583)	Fixed	Per-run folder Label_PID_timestamp, hard-fail if non-empty
Post-check 4 is tautological (486-492)	Fixed	Re-tests the filesystem directly instead of deriving the verdict from the variable it audits
Post-check 6 reports PASS when no hive was loaded (510-522)	Fixed	Reports INCONCLUSIVE as a distinct status, logged at Warning and carried into the verdict
Rollback restores bytes but not security (331-336, 407-409)	Fixed	Owner and DACL recorded in binary form and replayed; SDDL was rejected because machine-relative aliases would re-resolve on the rescue VM
Extensionless exclusion is undocumented but load-bearing (95)	Fixed	Documented in .DESCRIPTION and covered by tests — it is what keeps a bare SYSTEM safe when a caller passes no HiveName

Evidence the veto does not turn the scenarios into no-ops. This was the real risk: both callers
pass MatchExtension = @('.blf','.regtrans-ms'), and real Windows TxR files carry a GUID and a
.TM/.TxR infix, so their base name never equals a hive name. Proven with a 12-case table:

REFUSE  SYSTEM / SYSTEM.LOG1 / SYSTEM.blf / SYSTEM.regtrans-ms / SYSTEM.SAV
REFUSE  system.BLF (casing)  SYSTEM. (trailing dot)  BCD-Template.blf (hyphen)
remove  SYSTEM{guid}.TM.blf          <- REAL TxR file, must stay removable
remove  SOFTWARE{guid}.TMContainer.regtrans-ms
remove  {guid}.TxR.blf               <- REAL config\TxR file
remove  unrelated.blf
12 of 12 passed

---

Use-NestedRepairVm.ps1

Your finding	Status	Detail
Connect-NestedRepairVmDisk passed $DiskNumber, not $offlined (418)	Fixed	Passes the disks actually offlined
No try/finally; four early returns leave disks offline (374-431)	Fixed	Wrapped in try/finally. Restoration is scoped to the disks this call offlined: disks already offline at entry are not ours to hand back, and a guest that started owns its disks
Already-running short-circuits to Started = $true (366-372)	Fixed	The attach is verified before success is reported
Failed Set-VMFirmware only warns (308-310)	Fixed	Now a failure and a return; a Gen2 guest would otherwise PXE-boot into the 600 s timeout
Catch does not return; later branches overwrite Reason (612-618)	Fixed	Returns from the catch
Insufficient memory unhandled (425-431)	Fixed	Startup memory compared to free physical memory, retried reduced with dynamic memory
Wait loop misses terminal states (514)	Fixed	Exits on Off, Paused, Saved and PausedCritical
vmms not required for Supported = $true (93)	Fixed	Service must be running
VM resolved by -Name, which is a wildcard (360, 622)	Fixed	Resolved by -Id
Header omits two functions (26-35)	Fixed	Both added, .VERSION bumped

Two defects were found beyond your list while fixing these, and are fixed too: the rescue VM's own
IsBoot/IsSystem disk could be taken offline, and an empty offline set would start a diskless
guest that reported Started = $true and then waited out its full heartbeat timeout — the same
"success without the work" shape as your 366-372 finding. Both ends now refuse.

---

Get-OfflineWindowsDisk.ps1

This is the file the whole contribution turned on. Every other helper trusted a drive letter that
this one produced, so its failure modes were the ones that reached the rescue VM's own C:.

Your finding	Status
Catch only warned, so $systemDiskNumber stayed -1, the -ne filter never excluded anything, and the rescue VM's own disk became a repair candidate	Fixed. It now throws. This is the single most important change in the PR, and it deliberately converts some previously "successful" runs into loud failures
FriendlyName -like '*Virtual Disk*' is SCSI-only, so NVMe disks were invisible	Fixed, using the BusType selection from your #144. Credited below
Azure temporary/resource disk not excluded	Fixed — excluded by FileSystemLabel, plus IsBoot/IsSystem predicates on both candidate filters, so exclusion no longer depends on one lookup succeeding
Unvalidated $DriveLetter interpolated into a diskpart script	Fixed. ValidatePattern at the binding boundary, re-asserted after trimming. There is a test that feeds it a newline-injection payload
diskpart exit code ignored; success logged unconditionally	Fixed — exit code checked before anything is logged as done
Fixed 500 ms sleep after assigning a letter	Fixed — polls, and releases the letter if it never appears
Letters leaked on every run	Fixed — assignments are tracked and a cleanup function releases them from a caller's finally
$Error.Clear()	Removed
Candidate ordering not deterministic	Fixed — DiskNumber added to the sort keys

It also now calls Set-OfflineRepairRoot for both the Windows root and the boot root as soon as a
volume is chosen, which is what arms Assert-OfflineTarget for every other helper. That is the
mechanism described in the deviation section.

---

Use-OfflineProtectedResource.ps1 and Use-OfflinePrivilegedRegistry.ps1

You asked for this file to be split. It is: the #region Privileged registry access block, seven
functions all named *OfflinePrivileged*, is now Use-OfflinePrivilegedRegistry.ps1. Only two
scenarios reach into that region, so each gained a single dot-source line. Nothing was lost or
duplicated in the move — the function count went from 31 to 26 + 7, and the two extra are new
functions described below, not copies.

Your finding	Status	Detail
Privileges enabled with no check on the target (182-196, 743-752, 805, 1073, 1167)	Fixed	Assert-OfflineTarget before any privilege is enabled
Grant-OfflineRegistryKeyAccess loses every capture if one key throws mid-subtree (235-328)	Fixed	Captures append to a caller-owned list as each key is taken, so a later failure cannot discard the earlier ones
Access-denied reported as Removed = $true (520-523)	Fixed	-ErrorAction Stop; access-denied now falls through to the ownership path instead of claiming success
Restores counted, not verified (359-368, 884-893)	Fixed	Test-OfflineDescriptorMatch compares what was written back with what was captured
A key that cannot be reopened is silently skipped (362-363)	Fixed	That is now a failure
DACL round-tripped as SDDL (846-893)	Fixed for registry, deliberately not for files — see below
Grant catch does not restore, unlike 651 (534-537)	Fixed	Mirrors 651
Caller scriptblock output contaminates the result (640, 665)	Fixed	[void](& $Action)
File is too large (1210-1773)	Fixed	Split as above

SDDL — fixed for registry keys, deliberately kept for file paths

You are right that SDDL is lossy: it re-resolves machine-relative aliases (LA, DA, DU, DC)
against whichever machine parses it, and the P/AI control flags do not survive. For registry keys
nothing outside the helper ever sees the descriptor, so those now capture and replay in binary
form and the problem is gone.

For file paths the SDDL string is an external contract, not an implementation detail. Scenario
scripts already consume it: win-fix-rdp-certificate.ps1 parses it in about ten places
(RawSecurityDescriptor::new($Sddl) at 256, Get-OfflineSddlOwner at 604-605,
Restore-OfflinePathSecurity -Sddl at 342 and 355), and win-fix-catalog-store.ps1 passes it
through at 826→856. Changing that return type would have been a silent breaking change across
already-validated repairs. So the SDDL string stays, and an optional -BinaryDescriptor was added
alongside it for callers that want the lossless form. I would rather migrate those call sites in the
scenario PRs, where each one can be re-validated, than change the contract underneath them here.

One finding beyond your list

Restore-OfflinePathSecurity enabled SeTakeOwnershipPrivilege on a caller-supplied path with
nothing checking where that path pointed — the same shape as the findings above, and the last
ungated one. It is gated now. The gate sits after the empty-descriptor and missing-path guards on
purpose: two of its four call sites restore from a catch or a finally
(win-fix-catalog-store.ps1:856, win-fix-rdp-certificate.ps1:617), and a gate that threw there
would replace the operator's real error with a complaint about paths. All four sites restore a path
that a gated Grant-OfflinePathAccess had already accepted, so in practice the gate cannot fire —
but the ordering means it could not mask anything even if it did.

The three remaining functions that enable the ownership privilege have no scenario callers and are
reached only through gated entry points. Backup-OfflineFile is deliberately left ungated: it
writes to a scratch folder that is intentionally outside the offline root, where the gate would be
actively wrong. There is a test asserting this invariant structurally, so a future function that
takes ownership without gating will fail the suite rather than be caught by review.

---

PSScriptAnalyzer

You recorded this as "NOT RUN — module unavailable". It is available (1.24.0), so it was run and the
findings on the helpers are resolved. All eight helpers are new files in this contribution, so none of
these findings is inherited — they are ours.

Helper	Lines	Findings, all severities
OfflineRepairCommon.ps1	715	0
Get-OfflineBcdStore.ps1	579	0
Get-OfflineWindowsDisk.ps1	992	0
Use-OfflineRegistryHive.ps1	771	0
Use-OfflineProtectedResource.ps1	1584	0
Use-OfflinePrivilegedRegistry.ps1 (new, from the split)	670	0
Use-OfflineFileRemoval.ps1	1013	0
Use-NestedRepairVm.ps1	902	0

That is zero at every severity, not merely at Warning and Error — Information included. The
three other files in common\helpers\ (Get-Disk-Partitions.ps1, Get-Disk-Partitions-v2.ps1,
Logger.ps1) still report findings; those are pre-existing and not part of this contribution, so
they were left alone.

Three functions were renamed for PSUseSingularNouns (10 references updated). One of those renames
is worth calling out because the obvious name was wrong: Get-OfflineControlSetNames became
Get-OfflineReferencedControlSetName, not Get-OfflineControlSetName, because that name is
already taken by a different function returning the active control set. Collapsing them would have
left the later definition silently winning, and win-fix-boot-partition.ps1 — which wants every
referenced set — would have quietly received the active one instead.

Two findings are suppressed with a justification rather than changed, because the rule's remedy would
be actively wrong:

• PSAvoidGlobalVars on Get-OfflineRepairState. Explained in the deviation section above: a
  $script: variable binds to the scope that dot-sourced the file, so the gate would fail open.
• PSUseShouldProcess on Set-OfflineRepairRoot. It changes in-process state only. A -WhatIf
  that skipped the bind would leave no root registered, so every later Assert-OfflineTarget would
  throw for a reason unrelated to what the operator asked about.

---

Test evidence

A defect-level test suite was written, one case class per bug you reported, each asserting the
defect is gone rather than merely exercising the function. Current state: 108 of 108 pass.

Defect class	Cases
offline-root gate	10
signature trust	5
bcdedit injection	4
hive base-name veto	11
nested-VM disk hand-off	4
disk selection	4
qualified path join	11
hive probe visibility	6
drive-letter tracking	7
drive-letter cleanup	7
-WhatIf safety	15
ownership gate	2
dependency loading	22

The later classes cover defects that your review did not raise but that fixing it surfaced, and
they are worth naming because they are the same kind of bug you were objecting to:

• hive probe visibility. Found by the lab run, not by reading the code — and it is the finding I
  would most want you to look at, because it is your review's central complaint reproducing itself
  one layer down. Get-OfflineWindowsInstallCandidate mounts the offline SOFTWARE hive to read
  ProductName and the build number, which are worth 10 and up to 30 points of the score that
  decides which installation gets repaired when a disk carries more than one. It discarded the
  result of reg.exe load with $null =, tested only $LASTEXITCODE, and had no else branch — so
  a hive it could not read was indistinguishable from one it read fine. The run passed every
  assertion while silently scoring the candidate on partial data. It now records a ProbeStatus on
  every path, warns, and returns it to the caller. It deliberately still does not throw: an
  unreadable SOFTWARE hive is a fault this library exists to repair, so refusing to proceed would
  be the wrong failure mode. The unload path had the same flaw and matters more — a probe hive that
  will not unload holds the offline hive file locked for the rest of the run.
• drive-letter tracking. The list of letters the run assigned was returned with return $list.
  PowerShell unrolls a returned collection, so an empty list came back as $null and a one-element
  list came back as a fixed-size Object[]. Registering the first temporary drive letter during
  discovery would therefore have thrown. Fixed with a unary comma; the tests exercise the real
  round-trip rather than matching on source text.
• drive-letter cleanup. Clear-OfflineDriveLetter exists to be called from a caller's
  finally, but one failing release aborted the loop — leaking every remaining letter at exactly
  the moment it mattered, after a repair had already failed. It now releases each letter
  independently, untracks only what it actually released, names what is still stuck, and never
  throws. The test makes the middle of three releases fail, so it proves cleanup continues past
  a failure rather than merely surviving one at the end.
• -WhatIf safety. Five functions take host disks offline, release drive letters or stop a
  guest. None declared SupportsShouldProcess, so -WhatIf performed them for real. All five now
  guard their first mutation and return their normal result shape with Started/Stopped left
  $false, so a preview cannot be mistaken for a completed operation. ConfirmImpact is
  deliberately left at the default on every one of them — these run non-interactively as SYSTEM
  under az vm repair run, and High would block forever on a prompt nobody can answer. There is
  an explicit test asserting that, so it cannot regress.
• ownership gate. A structural invariant rather than a fixed list: any helper function that
  enables the take-ownership privilege and is reachable from a scenario script must call
  Assert-OfflineTarget. Reachability is resolved from the AST, because the helper name also
  appears inside a comment in win-fix-pending-servicing.ps1 and a text search treats that as a
  call site.
• dependency loading. Covered below.

Two things about the suite are worth stating plainly, because they are what makes it evidence rather
than decoration.

It was scored against the code you reviewed. Running the same suite against dd148b3 — the tree
as it stood when you wrote the review — gives 25 passed, 83 failed, against 108 passed, 0
failed now:

Defect class	Reviewed code	Now
offline-root gate	7 / 10	10 / 10
signature trust	0 / 5	5 / 5
bcdedit injection	2 / 4	4 / 4
hive base-name veto	0 / 11	11 / 11
nested-VM disk hand-off	0 / 4	4 / 4
disk selection	0 / 4	4 / 4
qualified path join	5 / 11	11 / 11
hive probe visibility	0 / 6	6 / 6
drive-letter tracking	0 / 7	7 / 7
drive-letter cleanup	2 / 7	7 / 7
-WhatIf safety	5 / 15	15 / 15
ownership gate	0 / 2	2 / 2
dependency loading	4 / 22	22 / 22

I am not claiming all 108 discriminate. The 25 that pass against the old code fall into three groups,
and it is worth being exact about which:

• Vacuous under baseline. Where the function did not exist at all, an assertion of the form
  "this must throw" or "this must not report success" is satisfied for the wrong reason — a missing
  command throws too. The seven offline-root gate passes and the two drive-letter cleanup passes
  are this. In the fixed tree they are meaningful; in the baseline they only show the function was
  absent, which the other cases in the same class record properly as failures.
• Guarding a future regression rather than a past defect. The five -WhatIf passes assert that
  ConfirmImpact is not High. The old code had no SupportsShouldProcess at all, so that was
  trivially true. Their job is to stop someone "hardening" these functions later into a prompt that
  a SYSTEM run can never answer, so they are supposed to pass in both trees.
• Testing a regression the review predates. Five of the eleven qualified path join cases pass
  against your tree, and they should: three of them assert that an ordinary nested root still joins,
  which was true before I broke it. That class is not evidence about the code you reviewed — it is
  evidence about the code I wrote afterwards. Its real mutation check is the tree that carried the
  regression, where exactly those three fail.

Assertions that could only ever be satisfied by matching source text were mutation-checked. The
ownership-gate invariant is the one that matters: deleting the Assert-OfflineTarget call from an
in-memory copy of Restore-OfflinePathSecurity flips the case to FAIL. A test that cannot fail is
not being counted as a pass.

Dependency loading

Your finding was that every helper dot-sourced with a current-directory-relative path. Fixing it
surfaced something worse: three helpers had no dependency loading at all and silently relied on
the caller having sourced the core first. That mattered most in the file that takes ownership —
discovering Assert-OfflineTarget is absent part-way through a privileged operation is far worse
than refusing to load.

All eight now resolve siblings against $PSScriptRoot, skip anything already defined so a scenario
that sources them in order does not load twice, and re-check the sentinels afterwards so a
dependency that loaded but did not define what was needed is still an error at load time. Verified
by loading each helper in its own fresh process with the working directory set to C:\, which has
no src\windows tree: 8 of 8 load.

The scenario scripts keep their existing repository-root-relative form. They belong to separate pull
requests, and rewriting twenty of them inside a helpers-only change would mix concerns.

The disk-selection fix, measured on a real machine

The change I am least comfortable asking you to take on trust is Get-OfflineWindowsDisk throwing
where it used to warn, because it turns runs that previously "succeeded" into loud failures. So I
scored the old and new selection predicates against real Get-Disk output on an Azure-hosted Windows
machine, read-only — no disk was brought online, no letter assigned, no diskpart run:

Disks visible:
  #0  BusType=SAS   IsBoot=True  IsSystem=True  TempStorage=False  2048GB

  [PASS] System disk resolves (throw does not fire)      SystemDrive C: is on disk 0
  [PASS] Own system disk excluded from candidates        candidates = []; system disk = 0
  [PASS] No boot/system disk is a candidate              boot/system disks = [0]
  [PASS] Pre-fix filter would not have excluded the OS disk
                                                         with sentinel -1 the -ne filter excludes
                                                         nothing; old model match admitted [0]
  [PASS] Probe changed no disk state (read-only)         0 differences

The fourth line is the finding you raised, reproduced rather than asserted: that machine's disk 0 is
its own boot and system disk, and the pre-fix predicate — FriendlyName -like '*Virtual Disk*'
with $systemDiskNumber still at its -1 sentinel — admitted it as a repair candidate. The
first line is the answer to the obvious objection to the new throw: the precondition it guards
resolves normally, so it does not fire on a healthy machine.

That machine reports BusType = SAS, incidentally, which is a second argument for selecting on bus
type rather than a model string.

The gate, proven on a real rescue VM

The read-only probe above cannot answer the question your review actually asks, which is whether the
gate holds when the helpers are running as SYSTEM on a rescue VM with a broken disk attached. So the
foundation branch was run end to end on one: az vm repair create against a lab VM, then the checks
executed through az vm run-command as SYSTEM, exactly the way az vm repair run invokes a script.

[PASS] Helpers load from an unrelated working directory :: cwd=C:\Windows
[PASS] Get-OfflineWindowsDisk succeeded (no fail-closed throw) :: Windows Server 2022 Datacenter build 20348 on disk 2 Gen2
[PASS] Selected volume is NOT the rescue VM system drive :: selected=E: rescue=C:
[PASS] Selected disk is NOT the rescue VM system disk :: selected disk=2
[PASS] Offline root was bound by disk selection :: root=Y:,E:
[PASS] Gate ACCEPTS a path on the attached disk
[PASS] Gate REJECTS a path on the rescue VM :: tried C:\Windows\System32\config\SYSTEM
[PASS] Assigned drive letters are tracked for cleanup :: tracked=[Z:,Y:]
[PASS] Selected volume really holds a Windows installation :: E:\Windows
[PASS] Offline hive probe reports its own status :: ProbeStatus=OK
[PASS] Offline hive probe read product name and build :: ProductName='Windows Server 2022 Datacenter' Build='20348' Guest='win2022b'
[PASS] Probe left no hive mounted :: leaked=[]
[PASS] Clear-OfflineDriveLetter released every letter :: before=[Z:,Y:] stillTracked=[]
[PASS] Rescue VM system drive still present :: C:

LAB RESULT: 14/14 passed

The seventh line is the one worth having: running as SYSTEM, with every privilege available to it,
Assert-OfflineTarget refused C:\Windows\System32\config\SYSTEM — the rescue VM's own live
registry, and the exact target the five findings you grouped together would have reached.

It took three runs, and the two failures were worth more than the pass. The first was 11/11
green while quietly reporting an empty product name and build; that led to the hive-probe swallow.
The second was 13/14 and reported both hives absent on a volume that demonstrably had E:\Windows;
that contradiction is what exposed the Join-OfflinePath regression described earlier. Neither was
visible from reading the code, and neither would have been caught by a suite that only tests what it
already suspects.

---

What I have not validated

I would rather list this than let you assume a clean bill of health.

• The scenario scripts are not in these PRs. Three call sites change behaviour and are validated
  only by unit test and call-site audit so far: Invoke-BcdEdit's -Command → -Arguments
  (win-fix-bcd.ps1, 3 sites), the new dot-source of Use-OfflinePrivilegedRegistry.ps1
  (win-fix-firewall-service.ps1, win-fix-user-rights.ps1), and Invoke-WithHive's output
  capture (17 scenarios; all 53 call sites audited by AST, none returns a live RegistryKey). Those
  scripts come in their own PRs and I will validate each end-to-end there.
• The binary-form ACL round-trip has not been re-run against a broken image. Switching registry
  descriptors from SDDL to GetSecurityDescriptorBinaryForm is a behaviour change on repairs that
  were previously validated — win-fix-catalog-store and win-fix-secure-boot are the two that
  exercise it. The unit tests cover the round-trip; a real break/repair/verify pass does not exist
  yet.
• Restore-OfflineRegistrySecurity's deleted-versus-stuck-key branch is untested live. It needs
  a key that cannot be reopened after a privileged removal, and I will not enable those privileges on
  a workstation to manufacture one.
• The nested Hyper-V path is unit-tested, not lab-tested. Use-NestedRepairVm is in Add the offline removal, BCD and nested-VM helpers (3 of 3) #147 and
  needs a rescue VM with nested virtualisation to exercise for real.

If you would rather any of these be evidenced before you read the corresponding PR, tell me which and
I will do that one first.

---

On #144

Your NVMe finding was correct and I have adopted your BusType selection, matching
Get-Disk-Partitions-v3. All three branches here are cut from main after #144 merged, so
they add files alongside Get-Disk-Partitions-v3.ps1 and leave your README entry for it intact.

Your live-validation notes on #144 also caught two defects that were present in my code in a
different form — the drive-letter timing and the helper returning its own log lines as data — so that
PR was useful to me twice over.

Get-OfflineWindowsDisk and Get-Disk-Partitions-v3 overlap but are not duplicates: yours returns
every partition of every attached disk, mine identifies the Windows installation to repair and
binds it as the offline root. I have said so in the README rather than leaving a reader to guess
which to reach for. If you would rather I layer mine on top of Get-Disk-Partitions-v3 instead of
running its own Get-Disk pass, that is a small change and I am happy to make it — the selection
predicate is already the same one.

[Marcus Ferreira (mvaferreira)]

Follow-up: the four gaps I listed as unvalidated are now closed — and two things I said were wrong

My previous reply ended with a "What I have not validated" section. Leaving those open would
have meant asking you to review claims I had not tested, so they are now closed. Doing that turned
up two incorrect statements of my own and four more defects. Both corrections are below,
before the good news, because they are the parts that affect how you read the rest.

---

Correction 1 — "can be merged in any order" was false when I wrote it

All three descriptions said the pull requests are independent and mergeable in any order. I asserted
that from the file lists: the three sets of .ps1 files are disjoint, so nothing can collide.

That reasoning was wrong, and I only found out because I measured it instead of re-reading it. All
three branches also edited the same ten-line table in src/windows/common/helpers/README.md.

Simulated with git merge-tree --write-tree across all six permutations against upstream/main:

Merge order	Result
A → B → C	second merge conflicts
A → C → B	second merge conflicts
B → A → C	second merge conflicts
B → C → A	second merge conflicts
C → A → B	second merge conflicts
C → B → A	second merge conflicts

0 of 6 clean. Whichever went in first was fine; the second always stopped. In every case the
conflict was README.md alone — the helper .ps1 files never conflicted, which is why the
file-list reasoning looked sound and was not.

Fixed by giving PR A sole ownership of the README. It now carries the complete twelve-row table
covering all eight helpers, and B and C carry no README change at all. Re-measured: 6 of 6
orders clean. files=3 on all three PRs now, where B and C previously showed 4.

I also ran the same test against the previously published branches to confirm it can actually fail:
it reported 0 of 6 on those. A green result from a test that cannot go red is not evidence.

If you would rather each PR carried only its own rows and you resolved the trivial README conflict
yourself, say so and I will switch it back — it is a one-command rebuild.

Correction 2 — I overstated what SDDL loses

I justified the binary-form ACL round-trip by saying an SDDL string can drop the protected (P)
and auto-inherited (AI) control flags and re-resolve machine-relative aliases. That claim
appeared in five places in the source comments.

Measuring it, the P/AI half did not reproduce. I am not able to stand behind it, so it is
removed.

What did reproduce is the alias half, and it is a stronger argument for the same design:

Alias	Captured	Parsed back on a workgroup host
LA	offline image's local administrator	silently became the parsing host's own administrator
DA	Domain Admins	failed to parse — no domain to resolve against
DU	Domain Users	failed to parse
DC	Domain Computers	failed to parse

LA is the dangerous one: it does not error, it just comes back pointing at the wrong machine. A
rescue VM is a workgroup machine by definition, so an SDDL replay there is either wrong or refuses.
Binary capture and replay is unchanged; only the justification is corrected, so it now says what was
measured rather than more than was measured.

One further note on measuring this, in case it saves you time later: SDDL string equality is not a
valid way to assert an ACL round-trip. A real filesystem write/read cycle gained the AI flag
(D: → D:AI) while preserving all three ACEs exactly — correct OS behaviour, since un-protecting
a DACL re-enables inheritance. Restore-OfflinePathSecurity compares owner, DACL and protection
state, not strings, which is why it is not affected.

---

The four gaps, closed

Gap I declared open	Now
Binary vs SDDL descriptor round-trip; the never-hit deleted-vs-stuck-key branch	24 / 24 checks pass; alias loss measured as above
Use-NestedRepairVm was unit-tested but never lab-tested	47 / 47 checks over all 11 fixed findings, via fakes and AST
Nothing proved the 20 scenarios resolve once all three helper PRs merge	4173 function calls resolved, 0 unresolved, on the merged tree
PSScriptAnalyzer had never been run on the 20 scenario scripts	run: 52 findings, 0 of them Error; 51 after the fix below

On the merged-tree check, two caveats so the number is not read as more than it is. 25 calls are
dynamic (& $someVariable) and cannot be resolved statically, so this proves every static call
target exists, not every call. And it initially failed on a duplicate definition — which turned out
to be pre-existing on upstream/main, where Get-Disk-Partitions.ps1 and
Get-Disk-Partitions-v2.ps1 both define Get-Disk-Partitions. That is yours, not ours, and I have
not touched it; the check now distinguishes pre-existing duplicates from any we introduce, so it
fails only on defects we cause.

Four more defects, found while closing those gaps

1. Save-OfflinePathSecurity would fail outright on PowerShell 7.
[System.IO.File]::SetAccessControl() exists on .NET Framework but was removed in .NET 7+,
where it becomes [System.IO.FileSystemAclExtensions]. Now probed by reflection with a fallback.
Production runs Windows PowerShell 5.1, so I verified on both engines rather than assuming: on
5.1 the probe finds the method and the fallback is unreachable; on 7 the fallback carries it. This
was latent — no shipped run-id hits it today — but it would have failed silently the day anything
invoked these helpers under pwsh.

2 and 3. Two post-checks in Use-OfflineFileRemoval.ps1 reported PASS on a value they could not
read. Checks 2 and 3 verify a removal actually took effect. When the underlying read failed, they
returned PASS rather than distinguishing "verified" from "could not verify" — the same
silently-degraded-precondition class your review is about, and awkward given the file already had an
INCONCLUSIVE status that check 6 used correctly. Both now return INCONCLUSIVE.

Fixing that exposed a knock-on: the success message hard-coded the explanation for check 6 ("no hive
was loaded beforehand"), so any other inconclusive check would have printed a confidently wrong
reason. The caveat is now built from whichever checks were actually unproven.

4. An empty catch { } in win-fix-ntfs-attribute-list.ps1 swallowed a failed
Set-Disk -IsOffline $false. After it, the script scanned nothing and still returned SUCCESS.
It now warns, naming the disk and the consequence. Found by the PSScriptAnalyzer pass on the
scenario scripts — the run you recorded as "NOT RUN — module unavailable" was worth doing.

Two other empty catches in that file I deliberately left: both are cleanup in a finally, where
swallowing is the correct idiom.

Evidence as it now stands

Check	Result
Defect test suite	120 / 120
Same suite against the reviewed tree dd148b3	28 / 120 — 9 of the 12 newest cases fail there
PSScriptAnalyzer, 8 helpers, every severity	0 findings
PSScriptAnalyzer, 20 scenario scripts	51 findings, 0 Error (52 before the empty-catch fix above)
Merged-tree call resolution	0 unresolved of 4173
Merge-order simulation	6 / 6 clean
Nested-VM path coverage	47 / 47
ACL round-trip	24 / 24
20 scenario branches rebuilt from upstream/main	ALL CHECKS PASSED
Rescue-VM validation, as SYSTEM on a real Azure repair VM	14 / 14

Every test is mutation-checked against the reviewed tree, so a passing number means the defect is
gone rather than that the test is agreeable.

All three PRs are MERGEABLE with zero deletions, so none of them can revert your merged #144.