Add Start-FinOpsMultitool cmdlet — interactive GUI for tenant-wide FinOps scanning - #2155
Add Start-FinOpsMultitool cmdlet — interactive GUI for tenant-wide FinOps scanning#2155Zac larsen (z-larsen) wants to merge 79 commits into
Conversation
… GUI Adds the Azure FinOps Multitool as a new PowerShell cmdlet in the FinOps toolkit. The Multitool is a WPF-based GUI that scans an Azure tenant for cost optimization, governance, and FinOps insights including cost trends, orphaned resources, idle VMs, tag hygiene, reservation/savings plan utilization, AHB opportunities, budgets, anomaly alerts, and policy compliance. - Public/Start-FinOpsMultitool.ps1: thin launcher cmdlet with comment-based help - Private/FinOpsMultitool/: full implementation (24 scanner modules, WPF GUI, Power BI template) - Tests/Unit/Start-FinOpsMultitool.Tests.ps1: Pester unit tests Windows-only (requires WPF support).
microsoft-github-policy-service
Bot
added
Needs: Documentation 📝
microsoft-github-policy-service
Bot
added
the
Tool: PowerShell
|
@microsoft-github-policy-service agree company="Microsoft" |
|
Zac larsen (@z-larsen) This is exciting! I don't know much about the tool, but would love to learn more. Can you join us at the contributor sync next Wednesday to share? |
|
Thanks, Michael! Would love to join. |
…info - Add contract-aware cost access warning banner (EA/MCA/CSP) on Overview tab - Add contract-specific billing tab messages when billing access unavailable - Add MG hierarchy unavailable info node in tree view with role guidance - Fix tag cost queries: use TagKey grouping type (not Tag/Dimension) - Add batched TagKey+TagValue query attempt with per-tag fallback - Clear skipSubs between batched and per-tag strategies - Add throttle pacing (2s every 2 queries) to avoid 429s - Add EA/MCA cost access detection in Get-CostData - Add runspace pool for API call parallelization
microsoft-github-policy-service
Bot
removed
the
Needs: Documentation 📝
Michael Flanakin (flanakin)
left a comment
Michael Flanakin (flanakin)
left a comment
There was a problem hiding this comment.
🤖 [AI][Claude Code] PR Review
Summary: This is an ambitious, well-designed contribution — the write-safety gate (Resolve-WriteDecision/Confirm-WriteAction.ps1), the Kusto-first hub data path, and the MCP server's JSON-RPC handling are all solid, and every blocker from the prior maintainer review (dead entry point, permissive default write mode, Set-CostAllocationRule bypassing the gate, string-id crash, run_full_scan invoking write tools) is fixed on this branch. This pass focuses on what's still open. A second, separate review will follow on naming/packaging (module integration, "Multitool" naming) per the PR author's request — this pass is the technical/mechanical review only.
🚫 Blockers (8)
Deploy-ResourceTag.ps1/Deploy-PolicyAssignment.ps1ship live ARM writes with zero write-safety gate, reachable afterImport-Module.Get-BudgetStatus.ps1silently loses all budget data for tenants with 50+ subscriptions.Get-PolicyRecommendations.ps1has a malformed policy definition GUID that can never match.Read-FinOpsHubData.ps1's Parquet-reader cache uses a predictable path that skips all verification once installed.Read-FinOpsHubData.ps1never hash/signature-verifies the downloaded Parquet.Net payload DLLs before loading them.Get-SharedCostAllocation.ps1builds a KQL clause via unescaped string interpolation.Get-VmCostBreakdown.ps1builds a KQL clause via unescaped string interpolation.
⚠️ Should fix (23)
Grouped by theme: test coverage gaps (4), scanner module bugs/fragility (6), documentation accuracy (7), style/naming consistency (3), TUI robustness (3).
💡 Suggestions (14)
Grouped by theme: minor security hygiene (3), additional test coverage gaps (3), code quality/duplication (4), doc/skill completeness (4).
One systemic note not tied to a single line: none of the 33 scanner modules use standard PowerShell comment-based help (.SYNOPSIS/.PARAMETER) — they use a custom banner-comment convention instead. Low priority since these are private, non-exported functions, but worth a conscious call rather than a silent drift from CLAUDE.md's "public functions must have comment-based help" convention (most of these are effectively public within the module's own surface).
| # Preserves existing tags -- only adds or updates the target tag. | ||
| ########################################################################### | ||
|
|
||
| function Deploy-ResourceTag { |
There was a problem hiding this comment.
🤖 [AI][Claude Code] 🚫 Blocker
Deploy-ResourceTag issues a real ARM PATCH (line ~55) with no dry-run, no -Apply switch, no ConfirmationToken, and no call to Resolve-WriteDecision/Confirm-WriteAction.ps1 — unlike every other mutating tool in this PR (Remove-OrphanedResource, Stop-IdleVm, Enable-HybridBenefit, Set-CostAllocationRule), which all route through that single write-safety gate.
FinOpsMultitool.psm1 has no manifest/Export-ModuleMember, so Deploy-ResourceTag and Remove-ResourceTag become directly callable in any session that does Import-Module FinOpsMultitool.psm1 — which is exactly what Start-FinOpsMultitool/Invoke-FinOpsMultitool and Start-McpServer.ps1 both do. Neither the TUI menu nor the MCP tool list currently calls these functions, so there's no reachable path today — but they ship fully wired and load automatically, contradicting the PR's core safety claim ("every mutating tool routes through the gate") and creating a live foot-gun for anyone who later wires a menu item or MCP tool to them without remembering to add the gate.
Either route this through Resolve-WriteDecision like the other four write tools, or remove it from this PR until it's wired up with the same safety story.
| # Uses ARM REST API PUT to create policy assignments. | ||
| ########################################################################### | ||
|
|
||
| function Deploy-PolicyAssignment { |
There was a problem hiding this comment.
🤖 [AI][Claude Code] 🚫 Blocker
Same issue as Deploy-ResourceTag.ps1: Deploy-PolicyAssignment (PUT, line ~99) and Remove-PolicyAssignment (DELETE, line ~144) perform real ARM mutations with no Resolve-WriteDecision call, no -Apply gate, and no confirmation token — bypassing the write-safety architecture entirely. They're dot-sourced unconditionally by FinOpsMultitool.psm1:82 and become callable in any session that imports the module (both TUI and MCP server do). Not currently reachable via the TUI menu or an MCP tool definition, but shipping live/ungated ARM-mutating code in a PR whose entire safety narrative is "every write goes through the gate" is a real inconsistency — please gate these the same way, or drop them from this PR.
| $budgetPath = "/subscriptions/$($sub.Id)/providers/Microsoft.Consumption/budgets?api-version=2023-05-01" | ||
| $resp = Invoke-AzRestMethodWithRetry -Path $budgetPath -Method GET | ||
| if ($resp.StatusCode -eq 200) { | ||
| $budgets = ($resp.Content | ConvertFrom-Json).value |
There was a problem hiding this comment.
🤖 [AI][Claude Code] 🚫 Blocker
$budgets starts as a [System.Collections.Generic.List[PSCustomObject]] (line 29), but for tenants with more than 50 subscriptions this line reassigns it to a plain array from the sampling response: $budgets = ($resp.Content | ConvertFrom-Json).value. The main collection loop later calls [void]$budgets.Add(...) (line ~148), which throws on a plain array/$null — and that exception is silently swallowed by the surrounding try/catch, counted as subsWithoutBudget.
Net effect: for any tenant with 50+ subscriptions, budget data comes back silently empty even when budgets exist. Use a distinct variable name for the sampling-response value (e.g. $sampleBudgetsResp) so it doesn't clobber the accumulator.
| ) | ||
| } | ||
| [PSCustomObject]@{ | ||
| PolicyDefId = '/providers/Microsoft.Authorization/policyDefinitions/ea3f2387-9b95-492a-a190-fcbef5-37f7' |
There was a problem hiding this comment.
🤖 [AI][Claude Code] 🚫 Blocker
The policy definition ID for "Inherit a tag from the resource group if missing" has an extra hyphen: ea3f2387-9b95-492a-a190-fcbef5-37f7. Splitting on - gives segments [8,4,4,4,6,4] instead of the valid GUID shape [8,4,4,4,12] — it's 35 characters, not 36. This recommendation can never match a real policy assignment's definition ID, so it will always be reported "missing" even when the built-in policy is actually assigned, and the ID can't be used to deploy it. Please verify the correct GUID against Get-AzPolicyDefinition -Builtin | Where-Object DisplayName -eq 'Inherit a tag from the resource group if missing' and fix the typo.
| if ($loaded) { return $true } | ||
|
|
||
| $parquetDir = Join-Path ([System.IO.Path]::GetTempPath()) 'FinOpsMultitool-Parquet' | ||
| $markerFile = Join-Path $parquetDir '.installed' |
There was a problem hiding this comment.
🤖 [AI][Claude Code] 🚫 Blocker
The Parquet-reader install cache lives at a fixed, predictable path ($env:TEMP/FinOpsMultitool-Parquet). Once the .installed marker file exists there, subsequent runs skip nuget.exe entirely and jump straight to Import-ParquetAssemblies — no Authenticode check, no hash check, nothing (lines 52-61). On a shared/multi-tenant temp directory (e.g. /tmp on Linux/macOS, or any host an attacker has prior local access to), someone who can write to that predictable path before the victim's first run can plant malicious DLLs plus a .installed marker and have them Add-Type-loaded into the victim's PowerShell process with zero verification on every subsequent run. Consider a per-user, randomized, or ACL'd cache location, and/or re-verifying on every load rather than trusting a marker file alone.
| $rows = $null | ||
| $cols = $null | ||
|
|
||
| switch ($mod.Fn) { |
There was a problem hiding this comment.
🤖 [AI][Claude Code] 💡 Suggestion
This ~450-line switch ($mod.Fn) block (result-table formatting) and a similarly large one further down (contextual guidance text) each independently enumerate all ~26 scan modules, duplicating per-module knowledge in two places. Combined with the picker/runner/export logic all living as nested functions inside one 2441-line top-level function, this is a maintainability concern worth addressing in a follow-up — e.g. colocating format/guidance metadata with each Get-*.ps1 module, or splitting the renderer and guidance engine into their own files.
| $results = Invoke-SelectedScans -Modules $finalModules -Subscriptions $subs -TenantId $tenantId -DataSource $dataSource | ||
|
|
||
| # Step 5: Summary + export | ||
| $global:FinOpsResults = Show-ResultsSummary -Results $results -Modules $finalModules -ExportPath $OutputPath -Subscriptions $subs |
There was a problem hiding this comment.
🤖 [AI][Claude Code] 💡 Suggestion
$global:FinOpsResults pollutes global scope. Likely intentional for interactive drill-down after the TUI exits (documented a few lines up), but worth a one-line comment noting the trade-off, or a more collision-resistant name (e.g. $global:FinOpsMultitoolResults).
| $exportDir = $ExportPath | ||
| } | ||
| else { | ||
| $defaultPath = Join-Path (Get-Location) 'FinOpsResults' |
There was a problem hiding this comment.
🤖 [AI][Claude Code] 💡 Suggestion
-OutputPath (and the interactively-typed export path here) flows directly into Test-Path/New-Item/Join-Path with no normalization or traversal check. Low risk — this is a local interactive tool acting on a path the user themselves typed for their own machine, no untrusted/remote input reaches it — but a Resolve-Path-based sanity check would be a defensive-programming nicety. Separately, the doc at start-finopsmultitool.md line 40 says -OutputPath "defaults to the tool's working folder," but the actual behavior is an interactive [E] Export [Enter] Skip prompt when omitted — if the user doesn't press E, nothing is exported at all; if they do, the suggested default is <current directory>/FinOpsResults, not simply "the working folder."
|
|
||
| Pick the narrowest tool that answers the question. Use `run_full_scan` only for a broad assessment. | ||
|
|
||
| | Intent | Tool | Category | |
There was a problem hiding this comment.
🤖 [AI][Claude Code] 💡 Suggestion
This intent-routing table lists only 21 of the 36 read-only tools — it omits scan_budget_history, scan_unit_economics, scan_vm_cost_breakdown, scan_allocate_shared_cost, scan_billing_account, scan_usage_allocation, scan_ai_workloads, scan_legacy_resources, scan_carbon, scan_macc_commitment, get_azure_context, generate_powerbi_template, connect_powerbi_to_hub, and explore_finops_kpis. Since this table is exactly how an AI agent decides which tool to call for a given intent, the missing rows mean the agent may not discover those 14 tools exist for their matching questions. Worth filling out for completeness.
| 4. **Alert thresholds** — set multiple (e.g. 50/80/100% actual, plus a forecasted-to-exceed alert). Forecasted alerts warn *before* the overrun. | ||
| 5. **Actions** — wire alerts to an action group (email/Teams/webhook). For automated response, trigger off the budget alert, not a manual check. | ||
|
|
||
| ## Variance analysis (budget vs actual) |
There was a problem hiding this comment.
🤖 [AI][Claude Code] 💡 Suggestion
This variance-analysis section doesn't reference scan_budget_history ("monthly budget vs actual history") despite it being the tool purpose-built for exactly this analysis — only scan_budget_status/scan_cost_trend are cited. Worth adding a pointer to it.
Michael Flanakin (flanakin)
left a comment
Michael Flanakin (flanakin)
left a comment
There was a problem hiding this comment.
🤖 [AI][Claude Code] PR Review
Summary: This is an ambitious, well-designed contribution — the write-safety gate (Resolve-WriteDecision/Confirm-WriteAction.ps1), the Kusto-first hub data path, and the MCP server's JSON-RPC handling are all solid, and every blocker from the prior maintainer review (dead entry point, permissive default write mode, Set-CostAllocationRule bypassing the gate, string-id crash, run_full_scan invoking write tools) is fixed on this branch. This pass focuses on what's still open. A second, separate review will follow on naming/packaging (module integration, "Multitool" naming) per the PR author's request — this pass is the technical/mechanical review only.
🚫 Blockers (8)
Deploy-ResourceTag.ps1/Deploy-PolicyAssignment.ps1ship live ARM writes with zero write-safety gate, reachable afterImport-Module.Get-BudgetStatus.ps1silently loses all budget data for tenants with 50+ subscriptions.Get-PolicyRecommendations.ps1has a malformed policy definition GUID that can never match.Read-FinOpsHubData.ps1's Parquet-reader cache uses a predictable path that skips all verification once installed.Read-FinOpsHubData.ps1never hash/signature-verifies the downloaded Parquet.Net payload DLLs before loading them.Get-SharedCostAllocation.ps1builds a KQL clause via unescaped string interpolation.Get-VmCostBreakdown.ps1builds a KQL clause via unescaped string interpolation.
⚠️ Should fix (23)
Grouped by theme: test coverage gaps (4), scanner module bugs/fragility (6), documentation accuracy (7), style/naming consistency (3), TUI robustness (3).
💡 Suggestions (14)
Grouped by theme: minor security hygiene (3), additional test coverage gaps (3), code quality/duplication (4), doc/skill completeness (4).
One systemic note not tied to a single line: none of the 33 scanner modules use standard PowerShell comment-based help (.SYNOPSIS/.PARAMETER) — they use a custom banner-comment convention instead. Low priority since these are private, non-exported functions, but worth a conscious call rather than a silent drift from CLAUDE.md's "public functions must have comment-based help" convention (most of these are effectively public within the module's own surface).
| # Preserves existing tags -- only adds or updates the target tag. | ||
| ########################################################################### | ||
|
|
||
| function Deploy-ResourceTag { |
There was a problem hiding this comment.
🤖 [AI][Claude Code] 🚫 Blocker
Deploy-ResourceTag issues a real ARM PATCH (line ~55) with no dry-run, no -Apply switch, no ConfirmationToken, and no call to Resolve-WriteDecision/Confirm-WriteAction.ps1 — unlike every other mutating tool in this PR (Remove-OrphanedResource, Stop-IdleVm, Enable-HybridBenefit, Set-CostAllocationRule), which all route through that single write-safety gate.
FinOpsMultitool.psm1 has no manifest/Export-ModuleMember, so Deploy-ResourceTag and Remove-ResourceTag become directly callable in any session that does Import-Module FinOpsMultitool.psm1 — which is exactly what Start-FinOpsMultitool/Invoke-FinOpsMultitool and Start-McpServer.ps1 both do. Neither the TUI menu nor the MCP tool list currently calls these functions, so there's no reachable path today — but they ship fully wired and load automatically, contradicting the PR's core safety claim ("every mutating tool routes through the gate") and creating a live foot-gun for anyone who later wires a menu item or MCP tool to them without remembering to add the gate.
Either route this through Resolve-WriteDecision like the other four write tools, or remove it from this PR until it's wired up with the same safety story.
| # Uses ARM REST API PUT to create policy assignments. | ||
| ########################################################################### | ||
|
|
||
| function Deploy-PolicyAssignment { |
There was a problem hiding this comment.
🤖 [AI][Claude Code] 🚫 Blocker
Same issue as Deploy-ResourceTag.ps1: Deploy-PolicyAssignment (PUT, line ~99) and Remove-PolicyAssignment (DELETE, line ~144) perform real ARM mutations with no Resolve-WriteDecision call, no -Apply gate, and no confirmation token — bypassing the write-safety architecture entirely. They're dot-sourced unconditionally by FinOpsMultitool.psm1:82 and become callable in any session that imports the module (both TUI and MCP server do). Not currently reachable via the TUI menu or an MCP tool definition, but shipping live/ungated ARM-mutating code in a PR whose entire safety narrative is "every write goes through the gate" is a real inconsistency — please gate these the same way, or drop them from this PR.
| $budgetPath = "/subscriptions/$($sub.Id)/providers/Microsoft.Consumption/budgets?api-version=2023-05-01" | ||
| $resp = Invoke-AzRestMethodWithRetry -Path $budgetPath -Method GET | ||
| if ($resp.StatusCode -eq 200) { | ||
| $budgets = ($resp.Content | ConvertFrom-Json).value |
There was a problem hiding this comment.
🤖 [AI][Claude Code] 🚫 Blocker
$budgets starts as a [System.Collections.Generic.List[PSCustomObject]] (line 29), but for tenants with more than 50 subscriptions this line reassigns it to a plain array from the sampling response: $budgets = ($resp.Content | ConvertFrom-Json).value. The main collection loop later calls [void]$budgets.Add(...) (line ~148), which throws on a plain array/$null — and that exception is silently swallowed by the surrounding try/catch, counted as subsWithoutBudget.
Net effect: for any tenant with 50+ subscriptions, budget data comes back silently empty even when budgets exist. Use a distinct variable name for the sampling-response value (e.g. $sampleBudgetsResp) so it doesn't clobber the accumulator.
| ) | ||
| } | ||
| [PSCustomObject]@{ | ||
| PolicyDefId = '/providers/Microsoft.Authorization/policyDefinitions/ea3f2387-9b95-492a-a190-fcbef5-37f7' |
There was a problem hiding this comment.
🤖 [AI][Claude Code] 🚫 Blocker
The policy definition ID for "Inherit a tag from the resource group if missing" has an extra hyphen: ea3f2387-9b95-492a-a190-fcbef5-37f7. Splitting on - gives segments [8,4,4,4,6,4] instead of the valid GUID shape [8,4,4,4,12] — it's 35 characters, not 36. This recommendation can never match a real policy assignment's definition ID, so it will always be reported "missing" even when the built-in policy is actually assigned, and the ID can't be used to deploy it. Please verify the correct GUID against Get-AzPolicyDefinition -Builtin | Where-Object DisplayName -eq 'Inherit a tag from the resource group if missing' and fix the typo.
| if ($loaded) { return $true } | ||
|
|
||
| $parquetDir = Join-Path ([System.IO.Path]::GetTempPath()) 'FinOpsMultitool-Parquet' | ||
| $markerFile = Join-Path $parquetDir '.installed' |
There was a problem hiding this comment.
🤖 [AI][Claude Code] 🚫 Blocker
The Parquet-reader install cache lives at a fixed, predictable path ($env:TEMP/FinOpsMultitool-Parquet). Once the .installed marker file exists there, subsequent runs skip nuget.exe entirely and jump straight to Import-ParquetAssemblies — no Authenticode check, no hash check, nothing (lines 52-61). On a shared/multi-tenant temp directory (e.g. /tmp on Linux/macOS, or any host an attacker has prior local access to), someone who can write to that predictable path before the victim's first run can plant malicious DLLs plus a .installed marker and have them Add-Type-loaded into the victim's PowerShell process with zero verification on every subsequent run. Consider a per-user, randomized, or ACL'd cache location, and/or re-verifying on every load rather than trusting a marker file alone.
| $rows = $null | ||
| $cols = $null | ||
|
|
||
| switch ($mod.Fn) { |
There was a problem hiding this comment.
🤖 [AI][Claude Code] 💡 Suggestion
This ~450-line switch ($mod.Fn) block (result-table formatting) and a similarly large one further down (contextual guidance text) each independently enumerate all ~26 scan modules, duplicating per-module knowledge in two places. Combined with the picker/runner/export logic all living as nested functions inside one 2441-line top-level function, this is a maintainability concern worth addressing in a follow-up — e.g. colocating format/guidance metadata with each Get-*.ps1 module, or splitting the renderer and guidance engine into their own files.
| $results = Invoke-SelectedScans -Modules $finalModules -Subscriptions $subs -TenantId $tenantId -DataSource $dataSource | ||
|
|
||
| # Step 5: Summary + export | ||
| $global:FinOpsResults = Show-ResultsSummary -Results $results -Modules $finalModules -ExportPath $OutputPath -Subscriptions $subs |
There was a problem hiding this comment.
🤖 [AI][Claude Code] 💡 Suggestion
$global:FinOpsResults pollutes global scope. Likely intentional for interactive drill-down after the TUI exits (documented a few lines up), but worth a one-line comment noting the trade-off, or a more collision-resistant name (e.g. $global:FinOpsMultitoolResults).
| $exportDir = $ExportPath | ||
| } | ||
| else { | ||
| $defaultPath = Join-Path (Get-Location) 'FinOpsResults' |
There was a problem hiding this comment.
🤖 [AI][Claude Code] 💡 Suggestion
-OutputPath (and the interactively-typed export path here) flows directly into Test-Path/New-Item/Join-Path with no normalization or traversal check. Low risk — this is a local interactive tool acting on a path the user themselves typed for their own machine, no untrusted/remote input reaches it — but a Resolve-Path-based sanity check would be a defensive-programming nicety. Separately, the doc at start-finopsmultitool.md line 40 says -OutputPath "defaults to the tool's working folder," but the actual behavior is an interactive [E] Export [Enter] Skip prompt when omitted — if the user doesn't press E, nothing is exported at all; if they do, the suggested default is <current directory>/FinOpsResults, not simply "the working folder."
|
|
||
| Pick the narrowest tool that answers the question. Use `run_full_scan` only for a broad assessment. | ||
|
|
||
| | Intent | Tool | Category | |
There was a problem hiding this comment.
🤖 [AI][Claude Code] 💡 Suggestion
This intent-routing table lists only 21 of the 36 read-only tools — it omits scan_budget_history, scan_unit_economics, scan_vm_cost_breakdown, scan_allocate_shared_cost, scan_billing_account, scan_usage_allocation, scan_ai_workloads, scan_legacy_resources, scan_carbon, scan_macc_commitment, get_azure_context, generate_powerbi_template, connect_powerbi_to_hub, and explore_finops_kpis. Since this table is exactly how an AI agent decides which tool to call for a given intent, the missing rows mean the agent may not discover those 14 tools exist for their matching questions. Worth filling out for completeness.
| 4. **Alert thresholds** — set multiple (e.g. 50/80/100% actual, plus a forecasted-to-exceed alert). Forecasted alerts warn *before* the overrun. | ||
| 5. **Actions** — wire alerts to an action group (email/Teams/webhook). For automated response, trigger off the budget alert, not a manual check. | ||
|
|
||
| ## Variance analysis (budget vs actual) |
There was a problem hiding this comment.
🤖 [AI][Claude Code] 💡 Suggestion
This variance-analysis section doesn't reference scan_budget_history ("monthly budget vs actual history") despite it being the tool purpose-built for exactly this analysis — only scan_budget_status/scan_cost_trend are cited. Worth adding a pointer to it.
Michael Flanakin (flanakin)
left a comment
Michael Flanakin (flanakin)
left a comment
There was a problem hiding this comment.
🤖 [AI][Claude Code] PR Review — packaging & ecosystem alignment
Summary: Separate pass from the earlier technical review, focused on how this fits the existing FinOps toolkit PowerShell module rather than code correctness. Two items below; more (versioning sync, shared .vscode/mcp.json scope) are being discussed with the author directly rather than posted here, to avoid overwhelming a first release with polish items that can land in a fast-follow.
⚠️ Should fix (2)
- The ~40 scan/write functions aren't real module cmdlets — they're private and only reachable through the TUI menu or a separately-launched MCP server process.
- A nested
LICENSEfile with a personal copyright holder shouldn't exist inside a Microsoft OSS repo.
| @@ -0,0 +1,94 @@ | |||
| # Copyright (c) Microsoft Corporation. | |||
There was a problem hiding this comment.
🤖 [AI][Claude Code]
All 18 existing public cmdlets in this module (Get-FinOpsCostExport, New-FinOpsCostExport, etc.) are eagerly loaded and exported by src/powershell/FinOpsToolkit.psm1 via its flat Private/*.ps1 / Public/*.ps1 glob. This file bypasses that entirely: Public/Start-FinOpsMultitool.ps1 manually dot-sources Invoke-FinOpsMultitool.ps1 at call time, which lazy-loads this whole subsystem — none of the ~40 scan/write functions here are ever exported or visible to Get-Command -Module FinOpsToolkit. They're only reachable through the TUI menu or by separately launching Start-McpServer.ps1 as its own process.
For a toolkit whose whole existing story is "install one module, get named Verb-FinOps*Noun cmdlets you can script directly," that's a real inconsistency, not just style. It also means these functions don't follow the -FinOps noun-prefix convention used everywhere else (Get-OrphanedResources vs. the established Get-FinOpsCostExport pattern) — worth fixing together, since promoting these to public cmdlets later would need the rename anyway to avoid generic-name collisions with other modules (e.g. Get-BillingAccount, Get-BudgetStatus are exactly the kind of generic nouns the FinOps prefix exists to avoid).
Suggest at least the read-only scan functions become real Get-FinOps* public cmdlets that the TUI and MCP server both call as thin wrappers, matching how the rest of the module works — rather than a self-contained subsystem bolted on behind one entry point.
| @@ -0,0 +1,21 @@ | |||
| MIT License | |||
There was a problem hiding this comment.
🤖 [AI][Claude Code]
This file asserts "Copyright (c) 2026 Zac Larsen" — a different copyright holder than the repo's top-level LICENSE ("Copyright (c) Microsoft Corporation"), which already covers every other file in this module. A nested per-subsystem LICENSE with a personal copyright holder inside a Microsoft OSS repo is confusing at best and a real licensing-clarity problem at worst, once this code is merged and distributed as part of the FinOpsToolkit package.
This looks like a carry-over from the original standalone repo (linked in the PR description) rather than an intentional choice — 45 of the 53 .ps1 files under this folder are also missing the standard # Copyright (c) Microsoft Corporation. / # Licensed under the MIT License. header that every other file in the module (including 8 files within this same PR) carries, and 12 files still have a personal # Author: Zac Larsen banner instead. Recommend deleting this LICENSE file and bringing the header convention in line with the rest of the module — the top-level repo LICENSE already covers contributions under the MIT terms (and the CLA was signed on this PR).
Michael Flanakin (flanakin)
left a comment
Michael Flanakin (flanakin)
left a comment
There was a problem hiding this comment.
🤖 [AI][Claude Code] PR Review — packaging & ecosystem alignment (2/2)
One more from the same packaging/alignment pass.
| # ===================================================================== | ||
| $MCP_VERSION = '2024-11-05' | ||
| $SERVER_NAME = 'finops-multitool' | ||
| $SERVER_VERSION = '1.3.0' |
There was a problem hiding this comment.
🤖 [AI][Claude Code]
$SERVER_VERSION = '1.3.0' is a hardcoded, independent version number, disconnected from the toolkit's own release cycle (currently heading to 15.0.0 per the changelog entry in this PR). Every other major component (src/optimization-engine, src/templates/finops-workbooks, src/templates/finops-alerts/modules, src/templates/finops-hub/modules/fx) has a ftkver.txt that src/scripts/Update-Version.ps1 finds via a recursive Get-ChildItem and overwrites on every release — it's opt-in by file existence, so a missing ftkver.txt here just means this never gets touched. Without one, $SERVER_VERSION will silently drift from the actual toolkit version forever (an MCP client will keep seeing "1.3.0" no matter what FinOpsToolkit release it's actually running against). Add a ftkver.txt under src/powershell/Private/FinOpsMultitool/ and have Start-McpServer.ps1 read it instead of hardcoding the string, so it stays in sync automatically.
There was a problem hiding this comment.
Requesting changes. A post-submission rubber-duck pass independently rechecked all 11 findings against live head 5f5deaa8; none were withdrawn. Final severity is 0 Critical, 7 High, 4 Medium. The snapshot finding is High rather than Critical because the unsafe private-function path is not currently exposed through the TUI, and the subscription finding is financial-scope correctness rather than an RBAC boundary violation. Merge blockers remain: missing snapshot orphan validation, cross-subscription scope contamination, broken macOS Kusto authentication, silent Hub-to-API fallback with incorrect provenance, unusable nested CSV exports, invalid SecureString bearer-token handling, inaccurate realized-savings calculations, and a Hub-provider suite that fails Pester 6 discovery. Please address the inline findings, align the documentation and PR description with the current head, resolve the base conflicts, and rerun validation.
| 'Microsoft.Compute/disks' = @{ api = '2023-04-02'; label = 'Managed disk'; inUseProps = @('managedBy', 'diskState'); kind = 'attachment' } | ||
| 'Microsoft.Network/publicIPAddresses' = @{ api = '2023-09-01'; label = 'Public IP address'; inUseProps = @('ipConfiguration', 'natGateway'); kind = 'attachment' } | ||
| 'Microsoft.Network/networkInterfaces' = @{ api = '2023-09-01'; label = 'Network interface'; inUseProps = @('virtualMachine', 'privateEndpoint'); kind = 'attachment' } | ||
| 'Microsoft.Compute/snapshots' = @{ api = '2023-04-02'; label = 'Disk snapshot'; inUseProps = @(); kind = 'backup' } |
There was a problem hiding this comment.
High: This snapshot entry has no in-use properties, so the execution-time orphan loop evaluates nothing and always passes. The latest commit only blocks an unparseable GET response; it does not establish that a valid recovery point is orphaned. This path is not reachable through the current TUI and the MCP server was removed, so it is a latent defect requiring a direct private-function call rather than a current UI-triggerable delete. Please remove snapshots from this generic path or require fresh source, age, retention/protection, and scan-evidence validation before any remediation surface exposes it.
| if ($kustoProvider) { | ||
| Write-Host "" | ||
| Write-Host " Querying FinOps Hub Kusto database ($($kustoProvider.Mode))..." -ForegroundColor Green | ||
| $cs = Get-FOHubCostSummary -Provider $kustoProvider |
There was a problem hiding this comment.
High (functional correctness): The selected IDs are computed above, but none of the three Hub preload calls receives -SubscriptionIds $subIdsForDisco. In addition, -RestrictToSelected is declared by the cost summary, trend, and resource-cost scanners but is never passed at any call site. A one-subscription UAT consequently returned 25 Hub subscriptions and cross-subscription resources. This stays within the caller’s RBAC, but it contaminates reports labeled as single-subscription. Pass the selected scope through every Hub, storage, and management-group cost path.
| $tok = (Get-AzAccessToken -ResourceUrl $ResourceUrl).Token | ||
| if ($tok -is [securestring]) { | ||
| $bstr = [System.Runtime.InteropServices.Marshal]::SecureStringToBSTR($tok) | ||
| try { [System.Runtime.InteropServices.Marshal]::PtrToStringAuto($bstr) } |
There was a problem hiding this comment.
High: SecureStringToBSTR returns UTF-16 data, but PtrToStringAuto is platform-dependent. On macOS this converted a live 2,612-character JWT into a one-character string, causing every remote Kusto query to return 401. Decode this pointer with Marshal.PtrToStringBSTR.
| if (-not ($rc -is [System.Collections.IDictionary] -and $rc.Contains('Error') -and $rc.Error)) { $hubResourceCosts = $rc } | ||
| $ct = Get-FOHubCostByTag -Provider $kustoProvider | ||
| if (-not ($ct -is [System.Collections.IDictionary] -and $ct.Contains('Error') -and $ct.Error)) { $hubCostByTag = $ct } | ||
| Write-Host " Hub data summarized in-engine (no rows loaded). Forecast is not included; choose API source for live forecast." -ForegroundColor DarkGray |
There was a problem hiding this comment.
High: This success message is unconditional even when all three provider calls returned errors and their results were discarded. The scan then falls through to Cost Management APIs while the UI still labels the source as FinOps Hub. Preserve and surface the provider error, require an explicit fallback decision, and update result provenance whenever the provider changes.
| if ($data -and @($data).Count -gt 0) { | ||
| $safeName = $mod.Fn -replace '[^a-zA-Z0-9\-]', '' | ||
| $csvPath = Join-Path $exportDir "$safeName.csv" | ||
| $data | Export-Csv -Path $csvPath -NoTypeInformation |
There was a problem hiding this comment.
High: Several scan contracts return wrappers containing nested hashtables or arrays. Exporting the wrapper directly produces cells such as System.Collections.Hashtable and System.Object[]; the cost-summary export even turns subscription IDs into columns. Define a tabular projection for each scan contract before CSV export.
| $sub = if ($subNameById.ContainsKey($sid)) { $subNameById[$sid] } else { $sid } | ||
| } | ||
| if ($pm -match 'Reservation') { | ||
| $ri += $cost * 0.4 |
There was a problem hiding this comment.
High: Multiplying amortized committed cost by a fixed discount percentage does not calculate realized savings. For example, a paid cost of $100 at a 40% discount implies about $66.67 savings, not $40, and real discounts vary by SKU, term, region, and agreement. Use benefit-utilization/savings data or compare against the matching PAYG benchmark; otherwise label this explicitly as a heuristic rather than realized savings.
| } | ||
|
|
||
| if ($PreselectedId) { | ||
| $sub = Get-AzSubscription -SubscriptionId $PreselectedId -ErrorAction SilentlyContinue |
There was a problem hiding this comment.
Medium: A supplied -SubscriptionId is resolved only after the tenant picker, and this lookup omits -TenantId. In UAT it attempted tokens across unrelated tenants and emitted repeated conditional-access/MFA warnings. Resolve the subscription and tenant first, set that context, and bypass both pickers when the caller already supplied the scope.
|
|
||
| & "$PSScriptRoot/../Initialize-Tests.ps1" | ||
|
|
||
| BeforeAll { |
There was a problem hiding this comment.
Medium: Initialize-Tests.ps1, invoked above, already declares a root-level BeforeAll. Pester 6 rejects this second root-level block during discovery, so none of the Hub-provider tests runs. Combine the setup into one supported BeforeAll or move the module import into the existing initialization block.
| Write-Host "" | ||
| Write-Host " ↑↓ Navigate │ Enter = Select tenant │ Q = Stay in current" -ForegroundColor DarkGray | ||
|
|
||
| $tKey = $Host.UI.RawUI.ReadKey('NoEcho,IncludeKeyDown') |
There was a problem hiding this comment.
Medium: The public cmdlet has no non-RawUI path. Cursor repainting plus RawUI.ReadKey makes focus and selection state unreliable for screen readers and can hang or throw in remoting, redirected, and CI hosts. Add a numbered line-oriented accessible mode, explicit noninteractive parameters, and an early RawUI capability check.
| </div> | ||
| <div class="ftk-tile"> | ||
| <div>🛡️ Write-safety policy</div> | ||
| <div>Optional remediation tools are dry-run by default and gated by a configurable write-safety policy. The server is read-only out of the box.</div> |
There was a problem hiding this comment.
Medium: The current head no longer contains the MCP server, yet this page still describes a server and its write-safety mode. Other public help also advertises Excel/JSON/Power BI exports, TUI remediation, and Windows PowerShell 5.1 behavior that the reviewed head does not provide. Please reconcile the public docs, Learn content, module README, agent skill, and PR description with the code that will actually ship.
microsoft-github-policy-service
Bot
added
Needs: Attention 👋
Finding 6 (High): Get-AzAccessToken now returns Token as a SecureString, so interpolating it produced "Bearer System.Security.SecureString" and every metric query returned 401. The empty catch turned that into zero findings rather than an error. Get-IdleVMs, Get-StorageTierAdvice, and Get-AIWorkloadMetrics now use Get-PlainAccessToken, and the AI scanner surfaces a token failure instead of swallowing it. Finding 3 (High): Get-PlainAccessToken used PtrToStringAuto, which picks the platform default encoding and truncated the JWT to one character on macOS. A BSTR is always UTF-16, so decode with PtrToStringBSTR. Finding 1 (High): Microsoft.Compute/snapshots declared inUseProps = @(), so the orphan verification loop iterated zero times and passed vacuously. Whether a snapshot is safe to delete depends on retention and backup policy, which this tool does not evaluate, and the delete is irreversible. Removed snapshots from the deletable allow-list. Finding 2 (High): the selected subscription scope was computed but never applied. Passed -SubscriptionIds to the three Hub Kusto queries, added -RestrictToSelected in the generic scan dispatcher for the scanners that declare it, and added -SubscriptionIds plus a shared row-subscription resolver to the Hub storage reader, which had no scope mechanism at all. Finding 4 (High): provider errors were discarded and the UI printed "Hub data summarized in-engine" unconditionally, then fell back to the Cost Management API while still labelling results as Hub. Errors are now surfaced per query and a total failure states that results are not from the Hub. Finding 9 (Medium): Initialize-Tests.ps1 already declares a root-level BeforeAll, and Pester 6 rejects a second one during discovery, so the Hub provider tests would not run in CI (CI installs Pester unpinned). Moved the module import into the Describe block. Also replaced MCP tool names and apply=true syntax in user-facing messages with the PowerShell equivalents, since the MCP server was removed.
microsoft-github-policy-service
Bot
added
Needs: Review 👀
Savings was computed as amortized committed cost multiplied by a flat discount percentage, which measures a share of what was paid rather than the gap up to pay-as-you-go. $100 paid at a 40% discount implies $66.67 saved, not $40. Savings is now paid * d / (1 - d). The assumed rates are named constants with the derivation documented alongside them. Real discounts vary by SKU, term, region, and agreement, so the RI and savings plan figures remain an estimate. The result now carries IsEstimate and EstimateBasis, the module header no longer claims measured savings, and the terminal UI prints the basis under the breakdown.
…s (review finding 8)
A supplied -SubscriptionId was resolved only after the tenant picker had
already run, and the lookup omitted -TenantId. Get-AzSubscription without a
tenant probes every accessible tenant, so a scoped run emitted repeated
conditional-access/MFA warnings for tenants that refuse.
Resolution now happens immediately after the connection check: the current
context tenant is tried first, falling back to an explicit per-tenant lookup
only if that misses. On success the context is set to that subscription and
tenant and both pickers are bypassed, which is what an explicitly scoped
invocation should do.
Also picks up a formatter pass that split "} catch {" across lines in
Get-SavingsRealized.ps1 to match the surrounding style.
…review finding 5) Exports piped the raw scan wrapper straight to Export-Csv, so collection and hashtable properties landed as "System.Object[]" and "System.Collections.Hashtable" cells, and the cost summary - a hashtable keyed by subscription - turned every subscription id into a column. Adds ConvertTo-FinOpsExportRows, which projects a result to flat rows: explicit projections for the cost summary and cost-by-tag contracts, a key/value shape for other dictionaries, and for wrapper objects the single collection property (falling back to a preferred name when several exist) so scans that follow the existing pattern export correctly without a case of their own. Summary-only contracts export their scalar properties as one row. Every projection then passes through ConvertTo-FinOpsExportCell, which guarantees scalar cells regardless of contract - dictionaries and arrays are rendered as delimited text rather than type names. Verified against a wrapper-plus-array result, a subscription-keyed hashtable, a nested tag map, and a summary-only object: no object-type cells in any.
5 participants
🛠️ Description
Adds the Azure FinOps Multitool to the FinOps toolkit. Discussed with Brett Wilson (@MSBrett), who suggested contributing the tool into the official toolkit.
The Multitool scans an Azure tenant for cost optimization, governance, and FinOps insights — cost trends, orphaned resources, idle VMs, tag hygiene, reservation/savings-plan utilization, AHB opportunities, budgets, anomaly alerts, and policy compliance. Analysis scans are read-only (Reader / Cost Management Reader) and ground their findings in the customer's live resource state. Four write/remediation tools (delete orphaned resource, deallocate idle VM, enable AHB, set cost allocation rule) are dry-run by default, gated by a configurable write-safety policy, and disabled unless
FINOPS_WRITE_MODEis set — the server defaults toReadOnly.This PR delivers two interfaces over one shared scanner engine, so the same scan logic is reused everywhere:
Invoke-FinOpsMultitoolStart-McpServer.ps1Shared scanner modules (
modules/)30 modular scanners (one per category): orphaned resources, idle VMs, storage tier advice, AHB, tag inventory/recommendations, policy inventory/recommendations, cost data/trend/by-tag, resource costs, reservation advice, commitment utilization, savings realized, budget status, anomaly alerts, Advisor optimization advice, billing structure, contract info, tenant hierarchy, and more.
FinOps Hub data paths
When a FinOps Hub is present, cost scans prefer the hub's Kusto database — an Azure Data Explorer / Fabric cluster (auto-discovered via Resource Graph) or a local ftklocal emulator (
FINOPS_HUB_KUSTO_URI) — and push aggregation into the engine, returning only summarized results. This scales to large hubs (tens of GB / hundreds of millions of rows) without loading raw cost rows into PowerShell. The storage-export reader remains as a small-dataset convenience fallback, used only when no Kusto cluster is reachable.MCP server + agent skills
Start-McpServer.ps1exposes the scanners as 40 MCP tools — 36 read-only (30scan_*, plusrun_full_scan,detect_cost_data_source,get_azure_context, and other helpers) and 4 gated write/remediation tools — over the 2024-11-05 MCP protocol via stdio..vscode/mcp.jsonregisters the server for VS Code, andTest-McpServer.ps1provides protocol-level unit tests.A companion agent-skill ecosystem (
src/templates/agent-skills/) teaches AI agents to use the server proactively and to route findings into the wider FinOps practice. Thefinops-multitoolskill acts as the hub, handing off to 11 FinOps-adjacent skills:power-bi-finops,cost-allocation,azure-policy-governance,unit-economics,finops-reporting,azure-workbooks-finops,forecasting-budgeting,anomaly-investigation,focus-data-quality,sustainability-carbon, andrate-optimization-portfolio.📦 Files added / changed
Public/Start-FinOpsMultitool.ps1Invoke-FinOpsMultitool.ps1+FinOpsMultitool.psm1Start-McpServer.ps1Test-McpServer.ps1modules/helpers/Get-FOHubProvider.ps1+Invoke-FOHubKustoQuery.ps1helpers/Confirm-WriteAction.ps1agent-skills/finops-multitool/agent-skills/cost-data-source/agent-skills/{power-bi-finops, cost-allocation, …}/.vscode/mcp.jsonTests/Unit/Start-FinOpsMultitool.Tests.ps1+FOHubProvider.Tests.ps1docs-mslearn/.../powershell/multitool/+docs/multitool.md📸 Screenshots
Screenshots are in the public repo README.
📋 Checklist
🧪 How did you test this change?
🐳 Deploy to test?
N/A — standalone PowerShell tooling (TUI / MCP server), not a template deployment.
🏷️ Do any of the following that apply?
📄 Did you update
docs/changelog.md?📖 Did you update documentation?
docs-mslearn/.../powershell/multitool/, a Jekyll landing page, overview/TOC/changelog entries, and the module README +finops-multitool/cost-data-sourceskills.