TSG ghost CSV: safety and correctness fixes from a four-family multi-model review

[John Neemes (1008covingtonlane)]

Fixes from a four-family multi-model review

Follow-up to #344. The persona lenses were re-run across four frontier model families (Opus, GPT, Gemini, Grok) and each found real defects the others missed. This PR fixes the safety-class and factually-wrong ones.

Safety

An unset pattern matched every path. Steps 2A to 2C depend on $GhostPathPattern from an earlier block, and PowerShell -match against an unset variable returns true for everything. Pasting a step into a fresh session therefore reported every VM on the node as referencing a ghost root, and could feed all of them into Move-VMStorage. Confirmed by running it, not by reasoning about it:

"C:\ClusterStorage\UserStorage_1\vm.vhdx" -match $null   ->  True

The definition now fails loudly if the pattern is empty, and each paste-ready block re-defines it if missing.

A ghost root that is itself a mount point was invisible. Step 1C tested only the children for the ReparsePoint attribute, so a numbered root that is itself a volume mount point reported IsReparsePoint = False and classified as safe to delete. That is the worst available miss: the whole root is live storage. The root is now tested first.

Verify the fix was weaker than the detection it confirms. It matched only attached disk paths, so a differencing or checkpoint parent left on a ghost root passed verification, which is the exact data-loss case Step 2A warns about. It now walks the full parent chain, and also checks reparse points and platform content that Step 3 requires for Path A but verification omitted.

Unverified nodes read as clean. Verification enumerates only nodes in State 'Up' while Step 3 requires clearance on every node, and ghost roots frequently appear precisely while a node is drained for a solution update. It now warns explicitly that skipped nodes are unverified and the condition is not resolved.

Correctness

CreationTime guidance was wrong. A ghost root is produced by renaming the CSV root, and an NTFS rename preserves CreationTime, so it reflects when the original C:\ClusterStorage was created, not when it was ghosted. Now stated plainly, with the reader pointed at the cluster log instead.

Also in this PR:

• probe $env:SystemDrive rather than a hardcoded C:, so a node whose system drive is not C: is not reported clean because the wrong volume was inspected
• match string arrays in cluster parameters; a bare -is [string] test silently skipped multi-valued parameters
• drop Select-String -List, which stopped at the first match per file and missed later references
• write Get-ClusterLog to an explicit path, since an elevated shell starts in System32
• warn on logs skipped for size instead of silently passing a clean result
• report an incomplete enumeration as UNVERIFIED rather than empty

Terms

Added the terms three of four families independently flagged as undefined: fan-out, canonical, parent chain / differencing disk, VHD-Set and .vhdpmem, solution update, and the MOC acronym expansion.

Validation

All 25 PowerShell blocks parse-checked with the PowerShell AST parser (Parser::ParseFile), 0 failures. Structure and safety lint: A, all checks pass, all relative and in-page anchor links resolve.

Known remaining, not in this PR

Deliberately left for a separate change so this one stays reviewable:

• Get-SmbOpenFile sees only remote SMB opens, so a local AV, backup, or filter-driver handle (the same lock class that creates the ghost) is never checked
• Get-ChildItem -Recurse on Windows PowerShell 5.1 follows volume mount points, so a scan intended to detect a reparse point can descend through it
• Get-VHD cannot open shared VHD Sets, so those are permanently flagged UNREADABLE with no documented escape
• the [System.IO.Directory]::Delete reparse-safety rationale does not hold as written
• Get-VM -Name accepts wildcards, so a partial name can select multiple VMs
• Step 3's "exactly one category" does not fit a node with an empty .000 and a referenced .001
• the preflight table prints but never halts

[Copilot]

Pull request overview

Hardens the ghost CSV troubleshooting guide following multi-model safety review.

Changes:

• Improves ghost-root and reference detection.
• Strengthens verification and incomplete-scan reporting.
• Clarifies terminology and timestamp guidance.

Suppressed comments (1)

TSG/Storage/Troubleshoot-Storage-GhostCsvMountPoints.md:1316

• An unreadable VHD makes this verification stop walking the chain without recording any finding. A ghost-root parent behind that unreadable link can therefore still yield References = 0, unlike Step 2A and the audit function, which correctly treat incomplete chains as blockers. Record the unreadable path in $refs (or a separate unverified collection) before breaking.

                $vhd = Get-VHD -Path $path -ErrorAction SilentlyContinue
                if (-not $vhd) { break }   # missing or unreadable: chain ends here

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[John Neemes (1008covingtonlane)]

Fixed in 5ae3384c, and you found a real inconsistency: the warning text made a claim the code did not honour.

Rather than guard only 2B and 2C, I audited every powershell block programmatically for "uses $GhostPathPattern but neither defines it nor receives it as param($Pattern)". That found two more unguarded consumers in Path B beyond the two you named. All are now guarded, and the audit reports zero remaining:

unguarded consuming blocks: 0

So the warning is now true rather than aspirational. Fixing only the two named blocks would have left the same fail-open in the more dangerous Path B.

[John Neemes (1008covingtonlane)]

Fixed in 5ae3384c. Correct, and this was the worse of the two orderings: a root whose scan FAILED emitted the <ENUMERATION INCOMPLETE> row and then fell through to also emit <empty>, which the results table calls the lowest-risk outcome. An incomplete scan could therefore read as unverified and safe at the same time, and the safe reading is the one an operator acts on.

It is now an elseif, with the reason recorded inline so it does not get "simplified" back into two separate if statements later:

if ($enumErrors.Count) { ...'<ENUMERATION INCOMPLETE>'... }
# elseif, NOT a separate if: a root whose enumeration FAILED must not also emit the
# '<empty>' row, because the results table calls empty the lowest-risk outcome and an
# incomplete scan would then read as both unverified and safe at the same time.
elseif (-not $children) { ...'<empty>'... }

[John Neemes (1008covingtonlane)]

Fixed in 5ae3384c. This was the sharpest of the three: cluster state Up does not mean reachable, and $missingNodes only compared cluster STATE. A node that is Up but fails Invoke-Command (WinRM stopped, credentials, firewall) returned no row at all, and a missing row was silently indistinguishable from a clean one, which is exactly what the new comment claimed to prevent.

Verification now captures the results and the remoting errors, then reconciles the nodes that actually ANSWERED against full cluster membership, and warns by name for any node that returned nothing:

$nodeResults = Invoke-Command ... -ErrorAction SilentlyContinue -ErrorVariable +remotingErrors
$answered    = @($results | Select-Object -ExpandProperty Node -ErrorAction SilentlyContinue)
$noAnswer    = @($allNodes | Where-Object { short-name not in $answered })
if ($noAnswer.Count) { Write-Warning "... returned NO RESULT and are UNVERIFIED ..." }

The short-name normalisation is deliberate: Get-ClusterNode returns FQDNs while $env:COMPUTERNAME does not, so comparing them raw would have flagged every node as unanswered.

Verified behaviourally rather than by inspection. Simulating three nodes where one answers, one is Down, and one is Up but silent, both non-answering nodes are reported and the Up-but-silent node is caught.