feat(oem-dell): add server lifecycle upgrade-vs-replace audit collector#86
feat(oem-dell): add server lifecycle upgrade-vs-replace audit collector#86AlrightLad wants to merge 3 commits into
Conversation
AlrightLad
added
type:enhancement
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, push a new commit or reopen this pull request to trigger a review.
DTC marks Device CALs individually per device; ceil(devices/5) pack math contradicted that practice and KB 3801. Report exact workstation count.
AlrightLad
force-pushed
the
feature/server-lifecycle-audit
branch
from
f97cec1 to
bc96f45
Compare
|
|
||
| $logDir = 'C:\DTC\Logs' | ||
| $logPath = Join-Path -Path $logDir -ChildPath ("server-lifecycle-audit-{0}.log" -f (Get-Date -Format 'yyyyMMdd-HHmmss')) | ||
| New-Item -ItemType Directory -Path $logDir -Force | Out-Null | ||
| Start-Transcript -Path $logPath -Append | Out-Null | ||
|
|
||
| try { | ||
| Write-Information ("Auditing {0} for client '{1}'" -f $env:COMPUTERNAME, $ClientName) -InformationAction Continue | ||
| $report = Get-DTCServerLifecycleAudit -ClientName $ClientName |
There was a problem hiding this comment.
🟡 The script does not implement the CLAUDE.md three-section RMM template: no $env:RMM detection (uses [Environment]::UserInteractive instead), no RMM variable declaration block at the top, and $LogPath is hardcoded to C:\DTC\Logs\ rather than branching between $env:RMMScriptPath\logs\ (RMM) and $env:WINDIR\logs\ (interactive). The author already flagged this as Standards Deviation #1 in the PR description and offered to conform in a follow-up commit — this comment records the reviewer decision. Recommend conforming before merge to keep the library consistent, but the script is functionally correct and read-only, so this does not block merge on its own.
Extended reasoning...
What the deviation is
CLAUDE.md requires every script to follow a three-section structure derived from script-template-powershell.ps1: (1) an RMM variable declaration comment block, (2) an input-handling section that branches on $env:RMM -eq "1" to pick RMM vs interactive mode and sets $LogPath accordingly, and (3) the script logic wrapped in Start-Transcript/Stop-Transcript. This script's main block (oem-dell/get-server-lifecycle-audit.ps1:258-266) instead does:
if (-not $ClientName) { $ClientName = $env:ClientName }
if (-not $ClientName -and [Environment]::UserInteractive) { $ClientName = Read-Host -Prompt 'Client name' }
if (-not $ClientName) { $ClientName = 'UNSPECIFIED' }
$logDir = 'C:\DTC\Logs'
$logPath = Join-Path -Path $logDir -ChildPath ("server-lifecycle-audit-{0}.log" -f (Get-Date -Format 'yyyyMMdd-HHmmss'))That is a valid pattern in isolation, but it diverges from the repo standard on three specific points: (a) it uses [Environment]::UserInteractive instead of $env:RMM -eq "1" as the mode discriminator, (b) it has no comment block enumerating expected RMM variables ($env:ClientName, and by convention $env:Description), and (c) it hardcodes the log directory instead of resolving to $env:RMMScriptPath\logs\ (RMM) or $env:WINDIR\logs\ (interactive/SYSTEM).
Why this is a nit, not a blocker
The PR description explicitly calls this out under 'Standards deviations to flag #1' and asks the reviewer for a decision — the author is not unaware, they are asking. The script is read-only (an audit collector; no writes to the target system), works correctly in both interactive and RMM contexts via the parameter → $env:ClientName → Read-Host → 'UNSPECIFIED' fallback chain, and the log-path deviation only affects where the transcript lands, not functional behavior. On NinjaOne this will run as SYSTEM and C:\DTC\Logs\ is writable, so nothing breaks.
Step-by-step: what the deviation actually costs
- A future maintainer copies this file as a starting template because it is in
oem-dell/and looks canonical — they now propagate the non-standard pattern. - Anyone grepping for
$env:RMMto enumerate RMM-mode scripts (e.g. for audit tooling) will miss this one. - The transcript log lands in
C:\DTC\Logs\on every host, which does not match the log-collection paths used by the rest of the library ($env:WINDIR\logs\or$env:RMMScriptPath\logs\), so centralized log-scraping will need a special case.
None of these are a functional break — they are consistency costs paid over time.
Recommended fix
Conform in a follow-up commit as the author offered. Concretely: add the ## $env:ClientName (and ## $env:Description) comment block at the top; replace the [Environment]::UserInteractive check with the standard if ($env:RMM -eq "1") { ... } else { ... } shape; and set $LogPath to $env:RMMScriptPath\logs\ (fallback $env:WINDIR\logs\) in RMM mode, $env:WINDIR\logs\ in interactive mode. The existing Get-DTCServerLifecycleAudit function itself does not need to change.
| .NOTES | ||
| Author: Zach Boogher | ||
| Repo: msp-script-library/ninjaone/audit/get-server-lifecycle-audit.ps1 (propose ninjaone/audit/ in PR) | ||
| Standards: KB 3299 (PowerShell Engineering Patterns), KB 3344 (Contributing a Script) | ||
| Read-only; inherently idempotent. PowerShell 5.1 compatible. |
There was a problem hiding this comment.
🟡 The .NOTES block cites msp-script-library/ninjaone/audit/get-server-lifecycle-audit.ps1 (propose ninjaone/audit/ in PR), but the file actually lands at oem-dell/get-server-lifecycle-audit.ps1 per the diff and PR description (Standards Deviation #2). Anyone running Get-Help on this script will see a repo path that doesn't exist. One-line doc fix — update the Repo: line to oem-dell/get-server-lifecycle-audit.ps1 and drop the stale (propose …) parenthetical.
Extended reasoning...
What the bug is. Line 24 of the .NOTES block reads:
Repo: msp-script-library/ninjaone/audit/get-server-lifecycle-audit.ps1 (propose ninjaone/audit/ in PR)
The parenthetical (propose ninjaone/audit/ in PR) is a giveaway that this line was authored when the placement was still under discussion. Per the diff header (diff --git a/oem-dell/get-server-lifecycle-audit.ps1) and the PR description's Standards Deviation #2 (New oem-dell/ directory proposed (best fit for a Dell/iDRAC/PowerEdge lifecycle tool)), the reviewer decision landed on oem-dell/. The .NOTES header was never updated to reflect that decision.
How it manifests. Get-Help .\get-server-lifecycle-audit.ps1 -Full renders the .NOTES block verbatim. Any technician who runs Get-Help — the standard PowerShell discoverability path — sees a ninjaone/audit/ repo path that does not exist in this repository. Grepping the codebase for ninjaone/audit returns no matches; the directory has never existed. Anyone searching by the documented path to locate the source will fail to find it.
Why existing code doesn't prevent it. .NOTES is free-form comment-based help; PowerShell does not validate that the Repo: string matches the actual file location. There's no lint rule or CI check that would catch this drift. The Pester tests in oem-dell/tests/get-server-lifecycle-audit.Tests.ps1 only assert the .SYNOPSIS marker is present (line 21: Should -Match '\.SYNOPSIS') — they don't parse or validate .NOTES.
Impact. Strictly documentation with no functional/runtime consequences. The script executes correctly and returns the audit report regardless of what the header says. The cost is downstream: a stale repo pointer misleads future maintainers and Get-Help consumers, and it undercuts the Standards: KB 3299 (PowerShell Engineering Patterns), KB 3344 (Contributing a Script) line immediately below it, which promises the file is contributed per standards.
Step-by-step proof.
- Diff header shows
diff --git a/oem-dell/get-server-lifecycle-audit.ps1— file placement inoem-dell/. - PR description Standards Deviation Local Account Token Filter Policy #2:
New oem-dell/ directory proposed (best fit for a Dell/iDRAC/PowerEdge lifecycle tool; msft-windows/ is the alternative). ninjaone/directory does not exist in the repo — grepping the codebase forninjaonereturns no results in the tree.- Line 24 of the new file:
Repo: msp-script-library/ninjaone/audit/get-server-lifecycle-audit.ps1 (propose ninjaone/audit/ in PR). - Contradiction: documented path (
ninjaone/audit/) ≠ actual path (oem-dell/), and the(propose …)parenthetical is itself an in-review draft artifact that shouldn't survive to a merged file.
How to fix. One-line edit to line 24:
Repo: msp-script-library/oem-dell/get-server-lifecycle-audit.ps1
Drop the parenthetical entirely — the placement is decided. No other changes needed.
| } catch { | ||
| Write-Error ("FAILED: {0}" -f $_) | ||
| Write-Error $_.ScriptStackTrace | ||
| exit 1 | ||
| } finally { | ||
| Stop-Transcript | Out-Null | ||
| } |
There was a problem hiding this comment.
🟡 The catch block at lines 269-272 has dead code: with $ErrorActionPreference = 'Stop' set at line 34, the first Write-Error ("FAILED: {0}" -f $_) at line 270 promotes to a terminating error and propagates out immediately, so Write-Error $_.ScriptStackTrace at line 271 and exit 1 at line 272 never execute. The finally block still runs and the uncaught error still yields a non-zero exit, so the exit contract is satisfied by accident — but the stack trace, the actual useful artifact for debugging a remote audit failure, is silently dropped from both stdout and the transcript. Fold both messages into a single Write-Error, or add -ErrorAction Continue to the first call.
Extended reasoning...
What the bug is. $ErrorActionPreference = 'Stop' is set at script scope on line 34. Under Stop, PowerShell promotes any Write-Error (a non-terminating error record via PSCmdlet.WriteError()) into a terminating error — this is documented PowerShell 3.0+ behavior and applies everywhere in the script, including inside catch blocks. So in the main-block catch at lines 269-275:\n\npowershell\n} catch {\n Write-Error ("FAILED: {0}" -f $_) # <-- terminates here\n Write-Error $_.ScriptStackTrace # dead code\n exit 1 # dead code\n} finally {\n Stop-Transcript | Out-Null\n}\n\n\nThe specific code path that triggers it. Any exception raised inside the try block (line 264: $report = Get-DTCServerLifecycleAudit ...) — e.g. a CIM query failure, Confirm-SecureBootUEFI throwing on an unusual host, or a registry read failing — routes to this catch. The very first statement in the catch is a Write-Error, which under Stop throws immediately.\n\nWhy the existing code doesn't prevent it. There is no inner try/catch around the Write-Error calls, and neither call passes -ErrorAction Continue to opt out of the preference. The line-34 preference applies uniformly.\n\nImpact. Low but real, and exactly opposite the reader's expectation. The FAILED message still lands in the transcript before the throw, and the finally block still runs Stop-Transcript, so the transcript closes cleanly and the process exits non-zero by way of the uncaught exception. What is lost is the ScriptStackTrace — the single most useful diagnostic when this collector fails on a remote NinjaOne target the tech cannot log into to reproduce. exit 1 being skipped is cosmetic (same non-zero exit either way).\n\nStep-by-step proof. Simulate the failure path with any exception in the try:\n1. Get-DTCServerLifecycleAudit throws. Control transfers to the catch.\n2. Line 270 evaluates Write-Error ("FAILED: {0}" -f $_). Write-Error calls PSCmdlet.WriteError() which emits the error record to $Error and stdout.\n3. Under $ErrorActionPreference = 'Stop', the runtime immediately promotes that non-terminating record to a terminating error and throws.\n4. The throw originates inside the catch, so the exception does not re-enter the same catch — it unwinds toward the enclosing scope (script scope).\n5. Statement at line 271 (Write-Error $_.ScriptStackTrace) is skipped. Statement at line 272 (exit 1) is skipped.\n6. The finally block runs on unwind — Stop-Transcript | Out-Null executes, transcript is closed.\n7. The unhandled terminating error propagates out of script scope. The PowerShell host writes the error to stderr and sets a non-zero exit code.\n\nReproducer:\npowershell\n$ErrorActionPreference = 'Stop'\ntry { throw 'boom' } catch {\n Write-Error 'first'\n Write-Error 'second' # never runs\n 'after' # never runs\n}\n\nOnly first is written; the RemoteException propagates.\n\nHow to fix. One line, three viable shapes:\n\n1. Fold both into one Write-Error (simplest, preserves single error record):\npowershell\nWrite-Error ("FAILED: {0}`n{1}" -f $_, $_.ScriptStackTrace)\n\n2. Opt the first call out of the preference:\npowershell\nWrite-Error ("FAILED: {0}" -f $_) -ErrorAction Continue\nWrite-Error $_.ScriptStackTrace -ErrorAction Continue\nexit 1\n\n3. Use Write-Host/Write-Warning for the trace (also written to the transcript, not subject to Stop):\npowershell\nWrite-Warning $_.ScriptStackTrace\n\n\nAny of these keeps the stack trace on stdout and in the transcript. Recommend option 1 — smallest diff, single error record for consumers.
| if ($cs.Model -match '[A-Za-z](\d)(\d)0(\b|$)') { | ||
| $gen = 10 + [int]$Matches[2] | ||
| $genText = "{0}G" -f $gen | ||
| if ($gen -le 13) { $idracGen = 'iDRAC8' } else { $idracGen = 'iDRAC9' } | ||
| } |
There was a problem hiding this comment.
🟡 The iDRAC generation inference at line 82 collapses 11G, 12G, and 13G all into iDRAC8 via if ($gen -le 13) { $idracGen = 'iDRAC8' }. Dell's actual mapping is 11G→iDRAC6, 12G→iDRAC7, 13G→iDRAC8, 14G+→iDRAC9, so unsuffixed 11G/12G models that match the regex (T410, R610, T320, R620, etc.) get labeled iDRAC8 (inferred from model) on hardware that actually ships iDRAC6/iDRAC7 — framed as authoritative, no verify hint. Distinct from the suffixed-model regex issue already flagged: that path fails safely to verify; this one succeeds with wrong data. Fix is a 3-line elseif ladder.
Extended reasoning...
What the bug is. Lines 79-83:
if ($cs.Model -match '[A-Za-z](\d)(\d)0(\b|$)') {
$gen = 10 + [int]$Matches[2]
$genText = "{0}G" -f $gen
if ($gen -le 13) { $idracGen = 'iDRAC8' } else { $idracGen = 'iDRAC9' }
}The final if ($gen -le 13) collapses three distinct Dell hardware generations into iDRAC8. Dell's documented iDRAC-to-generation mapping is well established:
- 11G PowerEdge (T110/T310/T410/R610/R710/R910, ~2009-2011) → iDRAC6
- 12G PowerEdge (T20/T320/T420/T620/R220-R920, ~2012-2014) → iDRAC7
- 13G PowerEdge (T130/T330/T430/T630/R230-R930, ~2014-2017) → iDRAC8
- 14G+ PowerEdge (T140/T340/T440/T640/R240-R940 and newer) → iDRAC9
Why the existing code doesn't prevent it. The regex [A-Za-z](\d)(\d)0(\b|$) at line 79 successfully matches unsuffixed 11G/12G models — their last char is '0' followed by end-of-string, so (\b|$) fires on $. The generation number is computed correctly (11 for T410, 12 for T320); only the iDRAC mapping is wrong.
Distinct from the already-flagged suffixed-model bug. The prior comment about R750xa/R760xs covers the case where the regex fails and both $genText and $idracGen fall back to 'verify' — that path is fail-safe. This bug is the opposite: the regex succeeds, $genText is correct ('11G', '12G'), but $idracGen is silently wrong. The report emits iDRAC gen : iDRAC8 (inferred from model) — authoritative framing, no verify cue for the reader to double-check.
Impact. Limited but real for the target use case. The .SYNOPSIS claims Model-agnostic and the audit is specifically designed for upgrade-vs-replace quoting — 11G/12G PowerEdges (2009-2014) are exactly the population most likely to be flagged for replacement, so this collector will frequently run on the affected models. Mitigating: the downstream iDRAC license line (racadm license view) pulls ground-truth from the hardware when racadm.exe is present, and the READINESS NOTE tells the technician to confirm licensing in the iDRAC web UI. Aggravating: a senior tech reading iDRAC8 on an actual iDRAC6 R710 could waste time hunting for an Enterprise license SKU that never existed on that generation, or map to the wrong KB section since iDRAC6/7 have materially different Virtual Console/Virtual Media licensing models than iDRAC8/9.
Step-by-step proof.
Take $cs.Model = 'PowerEdge T410' (11G, actual hardware ships iDRAC6):
- Regex
[A-Za-z](\d)(\d)0(\b|$)scans and matchesT-4-1-0at the end of the string;$in the alternation fires. $Matches[2] = '1', so$gen = 10 + 1 = 11.$genText = '11G'— correct.11 -le 13is$true→$idracGen = 'iDRAC8'— wrong (T410 has iDRAC6).- Report emits:
iDRAC gen : iDRAC8 (inferred from model)with noverifyhedge.
Same trace for PowerEdge R720 (12G, actual iDRAC7): $Matches[2]='2', $gen=12, 12 -le 13 → iDRAC8. Wrong. And for PowerEdge T330 (13G, actual iDRAC8): $Matches[2]='3', $gen=13, 13 -le 13 → iDRAC8. Correct — 13G is the only generation this branch labels right.
Fix. Three-line elseif ladder, doesn't touch the primary 13G/14G+ case:
if ($gen -le 11) { $idracGen = 'iDRAC6' }
elseif ($gen -eq 12) { $idracGen = 'iDRAC7' }
elseif ($gen -eq 13) { $idracGen = 'iDRAC8' }
else { $idracGen = 'iDRAC9' }| $lines.Add(("Gate 1 OS ceiling : model {0} ({1}) -> ceiling per KB 3801 matrix vs dental-stack OS support" -f $cs.Model, $genText)) | ||
| $lines.Add(("Gate 2 current OS : {0}" -f $os.Caption)) | ||
| $lines.Add(("Gate 3 hardware : warranty -> look up tag {0} at dell.com/support" -f $bios.SerialNumber)) | ||
| $lines.Add(("Gate 4 capacity : {0} GB RAM, {1} cores (max {2} GB per firmware)" -f $ramGB, $cores, $maxRamGB)) |
There was a problem hiding this comment.
🟡 The Gate 4 capacity summary line has no guard when $maxRamGB is null, producing Gate 4 capacity : 128 GB RAM, 16 cores (max GB per firmware) with a visible blank slot. The IDENTITY block at line 74 already guards its RAM-max line with if ($maxRamGB) — the summary should match by either defaulting to a marker (e.g. 'unknown') or gating the parenthetical clause. Rare on modern Dell firmware but reachable on virtualized/edge hosts where both MaxCapacityEx and MaxCapacity are 0/null.
Extended reasoning...
What the bug is. At oem-dell/get-server-lifecycle-audit.ps1:63-65, $maxRamGB is initialized to $null and only assigned when either $memArray.MaxCapacityEx or $memArray.MaxCapacity is truthy. The IDENTITY block at line 74 correctly guards its detail line with if ($maxRamGB) { $lines.Add((...)) }, so when firmware doesn't report the ceiling, that line is simply omitted. But the Gate 4 summary at line 246 has no such guard:
$lines.Add(("Gate 4 capacity : {0} GB RAM, {1} cores (max {2} GB per firmware)" -f $ramGB, $cores, $maxRamGB))PowerShell's -f operator renders $null as an empty string (no exception, no placeholder), so the line becomes literally:
Gate 4 capacity : 128 GB RAM, 16 cores (max GB per firmware)
with a visible double-space gap between max and GB.
Trigger conditions. Win32_PhysicalMemoryArray.MaxCapacityEx (UInt64) is the modern property; MaxCapacity (UInt32, KB) is the legacy fallback. On the T130/T230/T330/T340/T440 family the collector explicitly targets, most Dell firmwares populate at least one. But older BIOS revisions, some virtualized hosts, and cases where Get-CimInstance ... | Select-Object -First 1 returns $null all leave both properties null/zero, at which point the elseif chain leaves $maxRamGB at its $null initial value.
Why existing code doesn't prevent this. The inconsistency is the strongest evidence: the author knew about the null case (they added the if ($maxRamGB) guard at line 74) and simply forgot to apply the same guard at line 246.
Step-by-step proof.
Get-CimInstance -ClassName Win32_PhysicalMemoryArray | Select-Object -First 1returns an object where both.MaxCapacityExand.MaxCapacityare$nullor0.- Line 63:
$maxRamGB = $null. - Line 64:
if ($memArray.MaxCapacityEx)— evaluates falsy, branch skipped. - Line 65:
elseif ($memArray.MaxCapacity)— evaluates falsy, branch skipped. $maxRamGBremains$null.- Line 74:
if ($maxRamGB) { ... }— falsy, RAM max detail line is (correctly) suppressed. - Line 246:
"... (max {2} GB per firmware)" -f $ramGB, $cores, $maxRamGB—$nullinterpolates as empty string, producing(max GB per firmware)with the double-space gap.
Impact. Cosmetic. A senior tech reading the paste-ready report sees a blank slot between max and GB and understands it as "not reported", not as zero — and the RAM max detail line above is already suppressed in the same run, so context is consistent. No wrong data ships; no exception is thrown. But this is a visible defect in a report the PR description explicitly frames as "paste-ready", and it violates the pattern the author established at line 74.
How to fix. One-line change to match the line-74 pattern. Either default $maxRamGB to a marker before the summary is built:
if (-not $maxRamGB) { $maxRamGB = 'verify' } # or 'unknown'or gate the whole parenthetical clause so it drops out when unknown:
$capLine = "Gate 4 capacity : {0} GB RAM, {1} cores" -f $ramGB, $cores
if ($maxRamGB) { $capLine += (" (max {0} GB per firmware)" -f $maxRamGB) }
$lines.Add($capLine)The 'verify' marker is a good fit — the report already uses verify elsewhere (generation/iDRAC gen) as the "look this up out-of-band" convention.
| $userCount = (Get-ADUser -Filter 'Enabled -eq $true' | Measure-Object).Count | ||
| $wsCount = (Get-ADComputer -Filter "OperatingSystem -notlike '*Server*'" | Measure-Object).Count | ||
| $lines.Add(("AD enabled users : {0}" -f $userCount)) | ||
| $lines.Add(("AD workstations : {0}" -f $wsCount)) | ||
| $lines.Add(("Device CALs needed : {0} (1 per device, marked individually - do NOT round to 5-packs) [SFT-109 for 2022 / SFT-111 for 2025]" -f $wsCount)) |
There was a problem hiding this comment.
🟡 The workstation query on line 231 is missing the Enabled -eq $true filter that the sibling Get-ADUser on line 230 uses, so disabled/decommissioned workstation computer accounts (and prestaged accounts whose OperatingSystem attribute is still null) get counted. That inflated $wsCount feeds directly into the Device CALs needed line, which the format string explicitly warns must be counted individually and NOT rounded to 5-packs — so any inflation lands 1:1 on the quoted CAL total with no rounding cushion. Fix is a one-clause consistency change: -Filter "Enabled -eq \$true -and OperatingSystem -notlike '*Server*'" to match the neighboring user query.
Extended reasoning...
What the bug is. Lines 230-231 of oem-dell/get-server-lifecycle-audit.ps1:\n\npowershell\n$userCount = (Get-ADUser -Filter 'Enabled -eq $true' | Measure-Object).Count\n$wsCount = (Get-ADComputer -Filter "OperatingSystem -notlike '*Server*'" | Measure-Object).Count\n\n\nThe user query correctly filters out disabled accounts; the workstation query directly below does not. The mismatch is even visible in the labels the report emits on lines 232-233: AD enabled users vs. just AD workstations (no enabled qualifier).\n\nTwo inflation vectors. First, disabled workstation computer accounts still in AD get counted. This is the common case — MSP shops routinely disable rather than tombstone-delete decommissioned machines for auditability, so stale-workstation counts of 5-30% above the active fleet are typical. Second, computer accounts where OperatingSystem is $null match -notlike '*Server*' because in LDAP the predicate compiles to (!(operatingSystem=*Server*)), and RFC 4515 negation of an Undefined attribute match returns the entry — freshly-joined machines that haven't reported their OS, prestaged accounts, and orphaned computer objects all get counted.\n\nWhy existing code doesn't prevent it. There is no post-filter in the pipeline — $wsCount is the raw Get-ADComputer count fed straight into the Device CALs needed line at 234. The parallel user query already established the correct pattern one line above, so this is a pure consistency omission.\n\nImpact — the specific field where inflation matters most. The format string on line 234 reads: Device CALs needed : {0} (1 per device, marked individually - do NOT round to 5-packs) [SFT-109 for 2022 / SFT-111 for 2025]. The header on line 226 reinforces this: Device CALs billed per device; marked individually, do NOT round to 5-packs. The whole reason this collector exists is to produce the exact per-device count a senior tech pastes into an SFT-109/SFT-111 quote line, and the header explicitly instructs not to round — so unlike most CAL counts, there is no 5-pack cushion to absorb an over-count. A tech reading AD workstations : 27 / Device CALs needed : 27 has no independent signal that 4 of those 27 are decommissioned machines in the Disabled Computers OU.\n\nWhy this is still a nit, not a blocker. The report is explicitly designed for senior-tech review before it becomes a quote (line 242: senior tech makes the call), the audit is read-only, and no data is written back to the system. A senior tech eyeballing an unusually high count would sanity-check it. This is a data-quality inconsistency in the primary output field, not a functional break — but it directly counteracts the audit's stated purpose and the fix is trivially consistent with the immediately preceding line, so it's worth fixing before merge.\n\nStep-by-step proof.\n1. Domain has 30 workstation computer objects. 25 are Enabled with OS = Windows 11 Pro; 4 are Disabled with OS = Windows 10 Pro (retired but not deleted); 1 is prestaged Enabled with OS = $null.\n2. Get-ADUser -Filter 'Enabled -eq $true' → returns only enabled users (correct).\n3. Get-ADComputer -Filter "OperatingSystem -notlike '*Server*'" compiles to LDAP (!(operatingSystem=*Server*)) with no Enabled predicate. It matches all 25 enabled + 4 disabled + 1 null-OS = 30 objects.\n4. $wsCount = 30, formatted into: Device CALs needed : 30 (1 per device, marked individually - do NOT round to 5-packs) [SFT-109 for 2022 / SFT-111 for 2025].\n5. Senior tech pastes this into the quote workup. Client is over-quoted by 5 Device CALs (~$40 each list = ~$200) with no rounding cushion to absorb the error.\n6. With the fix (Enabled -eq $true -and OperatingSystem -notlike '*Server*'), the workstation count is 25 (correct).\n\nFix. One clause matching the sibling user query pattern:\n\npowershell\n$wsCount = (Get-ADComputer -Filter "Enabled -eq \$true -and OperatingSystem -notlike '*Server*'" | Measure-Object).Count\n
Fleet run failed on a downlevel host: 'Write-Information' is not recognized (introduced in PS 5.0). The audit's target population is old servers, so downlevel WMF 3/4 hosts are in scope by definition. - Write-Information banner -> Write-Output (PS 2.0+, analyzer-clean) - [List[string]]::new() -> New-Object (::new() is PS 5.0+) x2 - Add #Requires -Version 3.0 so PS 2.0 hosts fail immediately with an explicit version error instead of a mid-script cmdlet error - .NOTES compatibility line updated 5.1 -> 3.0+ Remaining floor is PS 3.0 (Get-CimInstance, [pscustomobject], -in). OS-gated cmdlets (Get-PhysicalDisk, Get-Tpm, Confirm-SecureBootUEFI) already degrade via try/catch.
There was a problem hiding this comment.
An organization admin can view or raise the cap at claude.ai/admin-settings/claude-code. The cap resets at the start of the next billing period.
Once the cap resets or is raised, push a new commit or reopen this pull request to trigger a review.
1 participant
What
Read-only server lifecycle audit collector. Gathers every input needed to quote and
review a PowerEdge OS upgrade vs. full replacement: identity/hardware, boot/firmware
(2022 eligibility gate), iDRAC remote-recovery readiness, OS, disk, roles, SQL edition,
dental/LOB stack, and CAL sizing counts. Model-agnostic (T130/T230/T330/T340/T440/etc.).
Makes NO changes. Pairs with KB 3801 (decision gates + quote paths).
Why
Standardizes the upgrade-vs-replace quote workup, which was done by hand per server.
Output is one paste-ready block a senior tech maps to the KB 3801 gates.
How tested
params, function defined, main-block dot-source guard). Tests are structural rather than
behavioral because the collector calls CDXML/CIM cmdlets (Get-PhysicalDisk, Get-Tpm,
Confirm-SecureBootUEFI) that Pester 5 cannot cleanly mock; live integration validated on
a server.
Changes existing behavior
No — net-new script in a new
oem-dell/directory.New dependencies
None for the script (built-in PowerShell 5.1 cmdlets; racadm/ActiveDirectory/ServerManager
used opportunistically with graceful fallback). Pester 5+ to run tests locally.
Deployment notes (post-merge)
64-bit, Language: PowerShell.
ClientName(Text) for the report header; absent => UNSPECIFIED.Standards deviations to flag
$LogPath to $env:WINDIR\logs\ / $env:RMMScriptPath\logs). Current version uses a
function-wrapper + C:\DTC\Logs. Flagging for reviewer decision — will conform in a
follow-up commit if required.
oem-dell/directory proposed (best fit for a Dell/iDRAC/PowerEdge lifecycle tool;msft-windows/is the alternative).Compliance & safety
Read-only, no system changes. No PHI/CUI/credentials in output or logs. Client name is a
runtime input, never hardcoded.
@claude can you review this? Does it follow our standards, and is the structure sound?