🤖 [AI][Claude Code] Deferred from PR review of #2155 (FinOps multitool) — three non-urgent polish items, none blocking that PR's merge.
1. Diagnostic message misattributes non-RBAC storage errors
src/powershell/Private/FinOpsMultitool/modules/helpers/Resolve-CostDataSource.ps1, Test-HubStorageAccess (~lines 305-319). When Get-AzStorageAccount throws for any reason other than missing access, the result is unconditionally classified Blocker = 'NoRbac' with remediation "Grant Reader on the hub storage account." A transient network/timeout failure gets the same (wrong) remediation hint as an actual permissions problem. The raw exception text is still surfaced in Detail, so this is a UX/diagnostic-accuracy issue, not silent failure.
2. No static guard enforces the "read-only tool" invariant
src/powershell/Tests/Unit/MultitoolSafety.Tests.ps1. The suite asserts scanner modules only call commands that resolve (no undefined commands), and has strong local file-system safety coverage (path traversal, symlink rejection, ACLs), but nothing asserts that no scanner module or helper calls a mutating Az cmdlet (Set-Az*, New-Az*, Remove-Az*, Invoke-AzResourceAction, etc.). Today's code is confirmed read-only by manual grep across all 30 scanner modules and helpers, but that invariant is enforced by review discipline, not by a test — a future PR could add a mutating call to a scanner module and nothing here would catch it. Suggest a static-AST guard similar in spirit to the existing "no undefined commands" check.
3. Invoke-FinOpsMultitool (the primary entry point) has no comment-based help
src/powershell/Private/FinOpsMultitool/Invoke-FinOpsMultitool.ps1, lines 10-27. The banner is a plain #-line comment block, not <# #>, so Get-Help Invoke-FinOpsMultitool -Full returns nothing useful for -SubscriptionId, -OutputPath, -Scans, -DataSource, -NonInteractive, etc. It's a Private function, so lower priority than a public cmdlet, but it's the tool's actual interactive entry point per the README quick-start.
Source: PR #2155
🤖 [AI][Claude Code] Follow-up from a verification re-review of PR #2155's 2026-09-24 fix commits — adding 5 more non-blocking items here (plus confirming item #2 above is still open).
4. Get-PolicyInventory.ps1 still doesn't surface definition-read failures
src/powershell/Private/FinOpsMultitool/modules/Get-PolicyInventory.ps1:103. The original review asked for something more visible than Write-Verbose here, since it affects a coverage count. The fix only added the Write-Verbose message — still invisible by default, and still no CoverageIncomplete/error field tied to a failed policy-definition read (unlike AssignmentErrors/ComplianceErrors, which exist for other failure paths in the same function). A real transient API failure here still reads identically to "this policy genuinely has no explicit effect."
5. MgCostScope.ps1 dropped its candidate cap without a replacement
src/powershell/Private/FinOpsMultitool/modules/helpers/MgCostScope.ps1:53-58. Fixing the pagination gap (good) also removed the old if ($candidates.Count -ge 12) { break } cap entirely. The probe loop a few lines below still issues one sequential Cost Management API call per candidate MG, so a large tenant with hundreds of management groups can now trigger hundreds of sequential probe calls (each with its own retry budget) before falling back to the tenant root. Not a correctness bug — it still finds the right scope or falls back — but a latent performance/throttling regression. Consider a reasonable cap (25-50) or prioritizing candidates.
6. Item #2 (no read-only static guard) — confirmed still open
Re-checked as of the 2026-09-24 fix commits: still no test anywhere asserts scanner modules avoid mutating Az cmdlets (Set-Az*/New-Az*/Remove-Az*). Several new test Contexts were added to MultitoolSafety.Tests.ps1 in this fix round, but they're all domain-logic coverage additions, not read-only enforcement.
7. Get-BillingStructure.ps1 — dead-code branches from the pagination fix
src/powershell/Private/FinOpsMultitool/modules/Get-BillingStructure.ps1:37-38 (and the equivalent spot in steps 2-4). Get-FinOpsListResult is called before checking $resp.StatusCode -eq 200, but that helper itself throws on any non-200 status — so the else { Write-Warning "...returned HTTP..." } branches further down are now unreachable; a non-200 is always caught by the outer catch instead (with a different but still-correct message). Step 5 (cost allocation rules) already does this the right way — checks status first, only calls the helper inside the 200 branch. Worth matching that pattern in steps 1-4 for consistency.
8. Get-ContractInfo.ps1 — trailing space in a Note string
src/powershell/Private/FinOpsMultitool/modules/Get-ContractInfo.ps1:175. 'Agreement inferred from subscription metadata, not confirmed billing-account details. ' + ($probeErrors -join ' ') unconditionally appends the join even when $probeErrors is empty, leaving a trailing space in the "inferred" case. Cosmetic only.
9. Get-AhbVmSavingsRatio.ps1 — last scanner module with zero domain-logic test coverage
The original review found 16 of 30 scanner modules had no test of their own domain logic; the 2026-09-24 fix round closed that gap to 29 of 30. The one holdout is Get-AhbVmSavingsRatio.ps1 (Get-AhbVmRates/Get-AhbVmSavingsRatio) — every existing AHB test mocks this module away entirely, so its retail-price parsing, Windows/Linux meter matching, the 0.6 fallback path, and its per-SKU+region cache are never exercised against a realistic API response. If that parsing regex breaks, every AHB savings estimate silently falls back to the generic 0.6 ratio with nothing catching it.
Source: verification re-review of PR #2155's fix commits, 2026-09-30.
🤖 [AI][Claude Code] Deferred from PR review of #2155 (FinOps multitool) — three non-urgent polish items, none blocking that PR's merge.
1. Diagnostic message misattributes non-RBAC storage errors
src/powershell/Private/FinOpsMultitool/modules/helpers/Resolve-CostDataSource.ps1,Test-HubStorageAccess(~lines 305-319). WhenGet-AzStorageAccountthrows for any reason other than missing access, the result is unconditionally classifiedBlocker = 'NoRbac'with remediation "Grant Reader on the hub storage account." A transient network/timeout failure gets the same (wrong) remediation hint as an actual permissions problem. The raw exception text is still surfaced inDetail, so this is a UX/diagnostic-accuracy issue, not silent failure.2. No static guard enforces the "read-only tool" invariant
src/powershell/Tests/Unit/MultitoolSafety.Tests.ps1. The suite asserts scanner modules only call commands that resolve (no undefined commands), and has strong local file-system safety coverage (path traversal, symlink rejection, ACLs), but nothing asserts that no scanner module or helper calls a mutating Az cmdlet (Set-Az*,New-Az*,Remove-Az*,Invoke-AzResourceAction, etc.). Today's code is confirmed read-only by manual grep across all 30 scanner modules and helpers, but that invariant is enforced by review discipline, not by a test — a future PR could add a mutating call to a scanner module and nothing here would catch it. Suggest a static-AST guard similar in spirit to the existing "no undefined commands" check.3.
Invoke-FinOpsMultitool(the primary entry point) has no comment-based helpsrc/powershell/Private/FinOpsMultitool/Invoke-FinOpsMultitool.ps1, lines 10-27. The banner is a plain#-line comment block, not<# #>, soGet-Help Invoke-FinOpsMultitool -Fullreturns nothing useful for-SubscriptionId,-OutputPath,-Scans,-DataSource,-NonInteractive, etc. It's aPrivatefunction, so lower priority than a public cmdlet, but it's the tool's actual interactive entry point per the README quick-start.Source: PR #2155
🤖 [AI][Claude Code] Follow-up from a verification re-review of PR #2155's 2026-09-24 fix commits — adding 5 more non-blocking items here (plus confirming item #2 above is still open).
4. Get-PolicyInventory.ps1 still doesn't surface definition-read failures
src/powershell/Private/FinOpsMultitool/modules/Get-PolicyInventory.ps1:103. The original review asked for something more visible thanWrite-Verbosehere, since it affects a coverage count. The fix only added theWrite-Verbosemessage — still invisible by default, and still noCoverageIncomplete/error field tied to a failed policy-definition read (unlikeAssignmentErrors/ComplianceErrors, which exist for other failure paths in the same function). A real transient API failure here still reads identically to "this policy genuinely has no explicit effect."5. MgCostScope.ps1 dropped its candidate cap without a replacement
src/powershell/Private/FinOpsMultitool/modules/helpers/MgCostScope.ps1:53-58. Fixing the pagination gap (good) also removed the oldif ($candidates.Count -ge 12) { break }cap entirely. The probe loop a few lines below still issues one sequential Cost Management API call per candidate MG, so a large tenant with hundreds of management groups can now trigger hundreds of sequential probe calls (each with its own retry budget) before falling back to the tenant root. Not a correctness bug — it still finds the right scope or falls back — but a latent performance/throttling regression. Consider a reasonable cap (25-50) or prioritizing candidates.6. Item #2 (no read-only static guard) — confirmed still open
Re-checked as of the 2026-09-24 fix commits: still no test anywhere asserts scanner modules avoid mutating Az cmdlets (
Set-Az*/New-Az*/Remove-Az*). Several new test Contexts were added toMultitoolSafety.Tests.ps1in this fix round, but they're all domain-logic coverage additions, not read-only enforcement.7. Get-BillingStructure.ps1 — dead-code branches from the pagination fix
src/powershell/Private/FinOpsMultitool/modules/Get-BillingStructure.ps1:37-38(and the equivalent spot in steps 2-4).Get-FinOpsListResultis called before checking$resp.StatusCode -eq 200, but that helper itself throws on any non-200 status — so theelse { Write-Warning "...returned HTTP..." }branches further down are now unreachable; a non-200 is always caught by the outercatchinstead (with a different but still-correct message). Step 5 (cost allocation rules) already does this the right way — checks status first, only calls the helper inside the 200 branch. Worth matching that pattern in steps 1-4 for consistency.8. Get-ContractInfo.ps1 — trailing space in a Note string
src/powershell/Private/FinOpsMultitool/modules/Get-ContractInfo.ps1:175.'Agreement inferred from subscription metadata, not confirmed billing-account details. ' + ($probeErrors -join ' ')unconditionally appends the join even when$probeErrorsis empty, leaving a trailing space in the "inferred" case. Cosmetic only.9. Get-AhbVmSavingsRatio.ps1 — last scanner module with zero domain-logic test coverage
The original review found 16 of 30 scanner modules had no test of their own domain logic; the 2026-09-24 fix round closed that gap to 29 of 30. The one holdout is
Get-AhbVmSavingsRatio.ps1(Get-AhbVmRates/Get-AhbVmSavingsRatio) — every existing AHB test mocks this module away entirely, so its retail-price parsing, Windows/Linux meter matching, the 0.6 fallback path, and its per-SKU+region cache are never exercised against a realistic API response. If that parsing regex breaks, every AHB savings estimate silently falls back to the generic 0.6 ratio with nothing catching it.Source: verification re-review of PR #2155's fix commits, 2026-09-30.