Add Start-FinOpsMultitool cmdlet — interactive GUI for tenant-wide FinOps scanning - #2155
Add Start-FinOpsMultitool cmdlet — interactive GUI for tenant-wide FinOps scanning#2155z-larsen wants to merge 65 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" |
|
@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
MSBrett
left a comment
MSBrett
left a comment
There was a problem hiding this comment.
Review: FinOps Multitool (TUI / CLI / MCP)
Thanks for this — it's an ambitious, genuinely useful contribution and the breadth of capabilities is impressive. I ran hands-on UAT on the three core surfaces and did a code + security pass with a couple of reviewers. Good news up front: the TUI, CLI, and MCP server all run. Module imports cleanly (352 commands), the TUI parses and launches with 0 errors, and the MCP server starts and responds to tools/list / tools/call. Since TUI/CLI/MCP are the features that matter for this release, the WPF/GUI piece can be dropped without blocking — but the current entry point still points at it, which is the first must-fix below.
I'm marking this as comment rather than request-changes since it's a draft, but there are a few hard blockers I'd want resolved before merge. Grouped by severity.
🔴 Blockers
1. Public entry point is dead on arrival (Public/Start-FinOpsMultitool.ps1).
Line 45 builds a path to Private/FinOpsMultitool/Start-FinOpsMultitool.ps1 and invokes it (line 54), but that file doesn't exist — there's no gui/ and no MainWindow.xaml. The Test-Path guard at line 47 means the public cmdlet emits "FinOps Multitool files not found..." and returns. Only the TUI (Private/.../Invoke-FinOpsMultitool.ps1) actually exists. Since WPF is cut, please repoint the public cmdlet at the TUI and update its help (lines 9, 19 still describe a "WPF-based GUI"). Right now the only public command is non-functional.
2. The "read-only" contract is not true — there are 4 live Azure write tools.
The PR description and SKILL.md state all tools are read-only / Reader-scope, but these perform real ARM mutations via Invoke-AzRestMethodWithRetry:
Remove-OrphanedResource.ps1:236— DELETEStop-IdleVm.ps1:130— deallocate POSTEnable-HybridBenefit.ps1:151— PATCHSet-CostAllocationRule.ps1:238— PUT
All four are advertised unfiltered over tools/list. Also worth noting: the server exposes 40 tools, not the 21/22 claimed in the docs and tests. The read-only framing needs to be corrected everywhere (PR description, SKILL.md lines 5/13/15/133), or these tools need to be gated/opt-in.
3. Set-CostAllocationRule bypasses the write-safety gate entirely. (most material issue)
Every other write tool routes through Resolve-WriteDecision (e.g. Remove-OrphanedResource.ps1:188), which enforces ReadOnly/Enforced mode, the confirmation token, protected-tag/sub/RG guardrails, impact caps, and the audit log. Set-CostAllocationRule has no ConfirmationToken param and never calls Resolve-WriteDecision — it rolls its own -Apply dry-run (line 217) then PUTs directly (line 238). Consequences I verified:
- Setting
FINOPS_WRITE_MODE=ReadOnlydoes not stop this tool (ReadOnly is only enforced insideResolve-WriteDecision). An operator who believes they're in read-only mode can still have chargeback rules written. - These writes are never audited — no
applyentry hits the audit log, so the "every apply is logged" guarantee is false for cost-allocation writes.
Please route this tool through the same gate as the other three.
4. String JSON-RPC id terminates the server (Start-McpServer.ps1).
All handlers and Send-Result / Send-Error type the id as [int] (lines 1530–1626), and the dispatch switch (line 1684) sits outside the inner try/catch — the outer try (1664) has a finally but no catch. A spec-legal string id ("abc") throws an argument-transformation error that abandons the while($true) loop and exits the process. Many MCP clients use string ids, so this will hard-crash the server in practice. Suggest typing ids as [object]/[string] and echoing them back verbatim.
🟠 High
5. Default write mode is Interactive, which one-shots writes without a token.
Confirm-WriteAction.ps1:51-52 defaults the mode to Interactive; the Apply path only requires a confirmation token when Mode -eq 'Enforced' (line 259). .vscode/mcp.json sets no FINOPS_WRITE_MODE, so the default applies — an agent can perform an irreversible delete with a single apply=true call. To be fair: dry-run is the genuine default (apply must be explicitly set) and the protected-tag list + orphan-type allowlist still apply, so this is partly by-design "low friction." But I'd strongly recommend defaulting to ReadOnly (or requiring a token in Interactive too), especially for an agent-driven surface.
6. SKILL.md misrepresents the safety contract to the agent.
Lines 5/13/15/133 tell the LLM all tools are read-only and "never modify resources." Because this is the agent-facing contract an LLM uses to decide an action is safe, the misstatement is higher-risk than a normal doc bug — please fix alongside #2.
🟡 Medium
7. run_full_scan invokes remediation tools.
Invoke-FullScan (Start-McpServer.ps1:1373) selects every tool except _full_scan with no category filter, so remediation tools (and set_cost_allocation_rule) get called in the loop. To be precise on impact: missing mandatory params raise ParameterBindingException which is caught at line 1500 and recorded in $errors — so the scan does not crash or mutate anything (Apply is never passed). The real effect is noisy per-tool errors / junk in results. Please filter the scan to read-only/diagnostic tools by category.
8. nuget.exe is downloaded from a mutable URL and executed unverified (Read-FinOpsHubData.ps1:72-77,159).
Pulls from .../latest/nuget.exe, executes it, then Add-Type-loads the fetched DLLs with no hash/signature check. Mitigating context: it's an official Microsoft TLS endpoint and the package version is pinned (Parquet.Net -Version 4.24.0), and nuget.exe is Authenticode-signed — but the signature isn't verified here. Pin/verify the nuget.exe hash or validate the Authenticode signature before execution.
9. Bundled tests fail as written.
Test-McpServer.ps1:139 asserts 21 tools (actual: 40); Start-FinOpsMultitool.Tests.ps1:23,28 assert the nonexistent Start-FinOpsMultitool.ps1 / gui/MainWindow.xaml. These need updating once the entry point and tool count are settled. The write modules also have 0 SupportsShouldProcess, which the repo lint may flag.
Minor
- Audit log defaults to
%TEMP%(Confirm-WriteAction.ps1:63) — an "append-only" trail in a user-clearable temp dir weakens the audit story. Consider a more durable default location.
Net: the core surfaces work and the design of the write-safety gate is sound — the problem is that one write tool skips it, the default mode is permissive, and the docs/entry-point don't match reality. Fix #1–#4 and correct the read-only claims and I think this is in good shape. Happy to re-review once those land, and thanks again for the contribution.
microsoft-github-policy-service
Bot
added
Needs: Attention 👋
microsoft-github-policy-service
Bot
added
Needs: Review 👀
|
I did another pass specifically against a large local FinOps hub dataset and want to call out a scaling concern with the current hub-data path. The current "FinOps Hub" fast path appears to be PowerShell over storage exports: it detects a hub storage account, downloads For reference, the local hub dataset we just validated was ~41.9 GB of exported files, 734 Parquet files, ~322M source rows, and 644M rows across raw + transformed Kusto tables. Loading that shape as For larger hubs, the tool should prefer querying the ADX/Fabric/Kusto Hub database directly and push aggregation/filtering into the engine ( This also matters for the new local-hub/on-own-hardware scenario: the local Kusto endpoint can query the full dataset successfully, but this PR currently has no direct Kusto/Hub query path to use it. |
|
One clarification to the recommendation above: if the goal is offline/local analysis and you want to keep the data on your own machine, the better scalable path is the ftklocal approach — load the exports into the local Kusto emulator and query the local Hub database there. So I would frame the options as:
The important point is the same in both deployed and local cases: avoid loading tens of GB / hundreds of millions of rows into PowerShell objects. Push the aggregation into Kusto and bring back summarized results. |
microsoft-github-policy-service
Bot
removed
the
Needs: Documentation 📝
flanakin
left a comment
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.
flanakin
left a comment
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.
5 participants
🛠️ Description
Adds the Azure FinOps Multitool to the FinOps toolkit. Discussed with @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.