feat: add azure-advisor skill#2663
Conversation
Add the azure-advisor product-area router skill with a read-only review capability that uses available advisor_* MCP tools. Includes shared capability-routing and subscription-discovery references, trigger tests, and registers the skill in tests/skills.json.
There was a problem hiding this comment.
Pull request overview
Adds a new azure-advisor product-area router skill under plugin/skills/ with a single review capability, plus trigger tests and registration so it’s included in CI/integration scheduling.
Changes:
- Introduces the
azure-advisorskill (routerSKILL.md, shared references, andreview/capability workflow) with per-skillversion.jsonfor NBGV path filtering. - Adds trigger tests + snapshots for the new skill.
- Registers
azure-advisorintests/skills.jsonand adds it to the scheduled integration batch list.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/skills.json | Registers azure-advisor and includes it in the integration test schedule batch. |
| tests/azure-advisor/triggers.test.ts | Adds trigger/negative/edge-case tests for routing activation. |
| tests/azure-advisor/snapshots/triggers.test.ts.snap | Adds snapshots for extracted keywords and description-driven triggers. |
| plugin/skills/azure-advisor/version.json | Adds per-skill NBGV config via pathFilters. |
| plugin/skills/azure-advisor/SKILL.md | Adds the router skill frontmatter + capability routing table and constraints. |
| plugin/skills/azure-advisor/review/review.md | Adds the read-only “review” capability workflow, constraints, and error handling. |
| plugin/skills/azure-advisor/references/subscription-discovery.md | Adds shared subscription discovery guidance for all capabilities. |
| plugin/skills/azure-advisor/references/capability-routing.md | Adds shared guidance for resolving advisor_* MCP tools by capability. |
| plugin/skills/azure-advisor/README.md | Adds contributor-facing folder map and conventions for extending the skill. |
| - `*.bicepparam` / `*.parameters.json` → `subscriptionId` / `subscription` parameter | ||
| - `.env*` files → `AZURE_SUBSCRIPTION_ID` line | ||
| - `infra/**/main.parameters.json` |
|
|
||
| Reply in chat with this structure (no files written): | ||
|
|
||
| ``` |
| - ❌ **Never** write helper scripts, scratch files, or parsing utilities to disk (no `.tmp/*.js`, no `.tmp/*.json`, no temp files of any kind). Reason over MCP tool responses directly in-context. Prefer aggregating via the `advisor_recommendation_summary` tool over computing counts yourself. | ||
| - ❌ **Never** shell out to `node`, `python`, `pwsh`, `powershell`, `jq`, or any other interpreter to read, parse, group, or count MCP tool responses. The tool responses are already in your context — reason over them directly. If you need server-side aggregation, use the `advisor_recommendation_summary` tool. |
| @@ -0,0 +1,62 @@ | |||
| --- | |||
| name: azure-advisor | |||
| description: "Azure Advisor reviews and recommendation workflows using whichever advisor_* MCP tools are available. WHEN: \"run an advisor review\", \"check my Azure advisor recommendations\", \"summarize advisor findings\", \"what does Advisor say about my subscription\", \"give me an advisor health check\", \"audit my Azure resources with Advisor\". USE FOR: end-to-end Advisor sweeps that combine the recommendation catalog, active recommendations, summaries/aggregates, and IaaC fix suggestions into one chat summary. DO NOT USE FOR: applying changes to Azure resources directly (read-only review), cost analysis beyond Advisor's cost category (use azure-cost), or non-Advisor diagnostics (use azure-diagnostics)." | |||
| > 🛠️ **Contributing a new capability?** See [README.md](README.md) for the folder map | ||
| > and a step-by-step recipe. |
|
|
||
| | Capability | When to Use | Reference | | ||
| |-----------|-------------|-----------| | ||
| | **review** | Run a holistic, read-only Advisor sweep across a subscription — probe the catalog, pull active recommendations, aggregate by category/impact, spotlight high-impact items, and propose IaaC fix snippets. | [review](review/review.md) | |
|
|
||
| Drive an end-to-end Azure Advisor sweep using whichever `advisor_*` MCP tools the | ||
| connected Azure MCP server exposes. Designed to stay useful as new advisor tools | ||
| land — it routes by *capability* (catalog, recommendations, summary, IaaC fix), |
| | **Metadata / catalog** | "list recommendation types / categories / impact levels / supported values" | tenant context (no subscription needed) | | ||
| | **Active recommendations** | "list Advisor recommendations in a subscription" | subscription (resource group optional, filters optional) | | ||
| | **Aggregation / summary** | "summarize / group / aggregate Advisor recommendations" | subscription + group-by field | | ||
| | **IaaC remediation** | "apply Advisor recommendations to IaaC / ARM / Bicep / Terraform" | a resource type identifier | |
| | [references/capability-routing.md](references/capability-routing.md) | **Shared, capability-agnostic docs** reused by every capability. Don't duplicate these inside a capability. | | ||
| | [references/capability-routing.md](references/capability-routing.md) | How to resolve an `advisor_*` MCP tool by capability (catalog, recommendations, summary, IaaC fix). | | ||
| | [references/subscription-discovery.md](references/subscription-discovery.md) | How to resolve the target subscription from repo config / env without hardcoding. | | ||
| | [review/review.md](review/review.md) | **Capability:** holistic read-only Advisor sweep. | | ||
| | [review/review.md](review/review.md) | The review workflow (the only file the agent reads for this capability). | |
| `azure-advisor` is a **product area**, not a single workflow. It groups multiple | ||
| Advisor-related capabilities behind one router so the agent can pick the right one by | ||
| user intent. Each capability is a self-contained folder; shared concerns live in the | ||
| [references](references/capability-routing.md) folder. |
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Testing standards gap per tests/AGENTS.md:
- The "Should NOT Trigger" section has only 4 negative prompts (minimum is 5).
- None of the negative prompts test adjacent Azure services. The AGENTS.md guide explicitly requires prompts about "different Azure services not covered by this skill." Since the description's DO NOT USE FOR section mentions
azure-costandazure-diagnostics, boundary prompts like "analyze my Azure costs" or "troubleshoot my Azure subscription" are needed to verify the skill doesn't false-positive on adjacent domains. This matters because keyword extraction pulls "cost" and "diagnostics" into the trigger set from the DO NOT USE FOR text. - No
unit.test.tsfile is present. The testing checklist requires unit tests that verify the skill's content contains expected sections.
The eval CI check is also failing. Worth confirming whether it's related to this PR.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Two things need fixing before this can merge:
-
No
unit.test.ts. Every other skill in the repo has one (the template includes it).tests/AGENTS.mdStep 5 and the checklist require unit tests that verify skill metadata and content sections. -
The
shouldNotTriggerPromptsarray has 4 entries; minimum is 5. None test adjacent Azure services. Since the skill'sDO NOT USE FORcalls outazure-costandazure-diagnostics, add boundary prompts like "analyze my Azure subscription costs" or "troubleshoot my Azure VM" to verify the skill doesn't false-positive on neighboring domains.
The Copilot review also flagged "IaaC" typos (should be "IaC") and .env* scanning in subscription-discovery.md. Worth fixing in the same pass.
…s, IaaC->IaC - Match MCP tools whose name contains advisor_ (e.g. azure-mcp-advisor_*) instead of requiring an advisor_ prefix, so Copilot CLI server-prefixed tools resolve correctly - Reword description to stop cost/diagnostics keyword bleed and expand shouldNotTriggerPrompts with adjacent-Azure-service boundaries (4 -> 9) - Regenerate triggers snapshot for updated description - Correct IaaC -> IaC across SKILL.md, README, references, and review docs - Harden .env* handling to read only AZURE_SUBSCRIPTION_ID line
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Correction on my earlier unit.test.ts feedback: no other skill in this repo actually has one (checked azure-compute, azure-deploy, azure-validate, azure-prepare, microsoft-foundry). The AGENTS.md documents it but nobody follows it. Withdrawing that as a blocker.
One remaining nit: README.md folder map table has duplicate rows for references/capability-routing.md and review/review.md. Each path appears twice with different purpose text. Consider collapsing each pair into a single row.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
My prior changes-request items are all resolved: negative prompts now cover adjacent-service boundaries (9 total), IaaC typos are fixed, and the README folder-map duplicates are collapsed.
One consistency issue remains: review.md references advisor_recommendation_summary by exact name in its constraints, but the rest of the skill routes by capability, not by hard-coded tool name. See inline comment.
The eval CI failure is pre-existing (azure-compute) and isn't related to this PR.
…eview Auto-discover every subscription the repo references (per-environment param/azd configs) and classify each into prod/staging/test/dev/other, running the Advisor review per subscription with results grouped by environment. Tenant-wide subscription-list enumeration becomes a widen-on-request/fallback path. Also route summary via the Aggregation/summary capability instead of a hard-coded tool name, and strengthen the case-insensitive trigger test.
| |------|---------| | ||
| | [SKILL.md](SKILL.md) | **Router.** Frontmatter (makes the skill discoverable) + a capability table that routes intent to a capability file. Keep it thin. | | ||
| | [references/capability-routing.md](references/capability-routing.md) | **Shared, capability-agnostic docs** reused by every capability. Don't duplicate these inside a capability. | | ||
| | [references/capability-routing.md](references/capability-routing.md) | How to resolve an `advisor_*` MCP tool by capability (catalog, recommendations, summary, IaC fix). | |
There was a problem hiding this comment.
Duplicate rows, keep single row for capability and review
| @@ -0,0 +1,64 @@ | |||
| --- | |||
| name: azure-advisor | |||
| description: "Azure Advisor reviews and recommendations using available advisor_* MCP tools. WHEN: \"run an advisor review\", \"check my Azure advisor recommendations\", \"summarize advisor findings\", \"what does Advisor say about my subscription\", \"give me an advisor health check\", \"audit my Azure resources with Advisor\". USE FOR: read-only Advisor sweeps with catalog, recommendations, and IaC fixes. DO NOT USE FOR: changing resources, billing analysis (use azure-cost), or non-Advisor troubleshooting (use azure-diagnostics)." | |||
There was a problem hiding this comment.
Update start to -> "Azure Advisor reviews resources and provides recommendations...."
| - **greenfield** — Advisor-informed guidance for new/empty subscriptions | ||
| - **cost** — Advisor cost-category optimization (coordinates with `azure-cost`) | ||
| - **reliability** — Advisor reliability-category reviews | ||
| - **governance** — Advisor operational-excellence / governance reviews |
There was a problem hiding this comment.
We can also add security and performance reviews to the roadmap here. But consider it as optional
| const result2 = triggerMatcher.shouldTrigger( | ||
| "run an azure advisor review" | ||
| ); | ||
| expect(result1.triggered).toBe(result2.triggered); |
There was a problem hiding this comment.
Check one of the results to be true as well here.
| > **Shared across all `azure-advisor` capabilities.** Any capability that operates on a | ||
| > subscription should link here instead of re-defining discovery logic. | ||
|
|
||
| Subscription **must never be hardcoded**. Resolve in this order and stop at the first hit: |
There was a problem hiding this comment.
Better to not stop at the first sub, rather scan all subscriptions and then run the skill for all Subscriptions and provide results accordingly, as this could lead to us missing the important Production subscriptions for a user.
| ### Step 1 — Resolve subscription | ||
|
|
||
| Run [Subscription Discovery](../references/subscription-discovery.md). Save the resolved | ||
| value; mention its *source* in the final chat summary ("Subscription pulled from |
There was a problem hiding this comment.
values (in case of multiple subscriptions)
| ### Step 2 — Probe the catalog (metadata) | ||
|
|
||
| Invoke the **metadata / catalog** capability (see | ||
| [Capability Routing](../references/capability-routing.md)) with no filter to learn what |
There was a problem hiding this comment.
Instead of no filter, We should have a step to discover the relevant resource Types part of Infra Files within the repo and add those as filters to one by one invoke metadata tool for each resource type
OR
Get the results without any filters but only show results belonging to specific resource types part of repo files.
| ## Azure Advisor Review | ||
|
|
||
| **Subscription:** <id> (from <source>) | ||
| **Tenant catalog:** N categories, M impact levels, K resource types |
There was a problem hiding this comment.
Displaying the whole catalog of recommendations does not make sense. Only specific to customer resource types is better.
Also, remove Tenant Keyword from output.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Prior changes-request items are all resolved: capability routing now references capability-routing.md instead of hard-coded tool names.
One formatting defect in the new commit: the Error Handling table in review.md has two rows merged onto a single line (the "Catalog call returns empty" and "Recommendation list 401/403" rows are separated by || instead of a newline). This breaks table rendering.
- Add resource-discovery reference and mandatory Step 1b to collect every repo resource type/group/id; apply scope in Steps 3-4 (filter or post-filter). - Reword SKILL.md description opener and add resource-scope + security/performance roadmap rows. - Add resource-discovery row to README folder map. - Drop whole-tenant catalog line from the chat summary (scope to customer resources, remove Tenant keyword). - Split merged error-handling table rows onto separate lines. - Update trigger snapshots.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Incremental review of 106786a (resource-scoping commit). The table formatting defect from my last review is fixed, and the new resource-discovery.md is well-structured with good security awareness around .env file handling.
One minor nit on the new file (non-blocking).
Bicep/ARM provider types are literal extractions; the Terraform azurerm_<kind> -> Microsoft.* mapping must be derived and can be non-obvious. When uncertain, fall back to the resource group filter rather than risk a wrong type filter that hides recommendations.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Incremental review of 59e0776 (Terraform type-mapping fallback note).
The distinction between literal extraction (Bicep/ARM) and derived mapping (Terraform) is the right framing. The azurerm_linux_function_app example illustrates why guessing is dangerous, and the fallback-to-resource-group-filter guidance prevents silent data loss from wrong type filters.
No new issues. My prior changes-requested items (negative prompts, IaC typos, README duplicates, hard-coded tool names, table formatting) are all resolved across the earlier commits.
jongio
dismissed
their stale review
Dismissing: this review was incorrectly posted as changes-requested due to an automation bug. The findings remain valid as comments.
Summary
Adds the
azure-advisorproduct-area router skill, ported and adapted to repo conventions.What's included
plugin/skills/azure-advisor/with a read-only review capability that uses whicheveradvisor_*MCP tools are available (routes by capability, not hard-coded tool names).tests/azure-advisor/(16 passing).tests/skills.json(skills array +0 12 * * 2-6integration batch).version.jsonadded; frontmatter uses0.0.0-placeholder(NBGV stamps at build).Validation