Add tool list export to temp file for drift detection#2576
Open
Add tool list export to temp file for drift detection#2576
Conversation
hallipr
requested review from
KarishmaGhiya,
conniey,
g2vinay,
jairmyree,
msalaman,
vukelich and
wbreza
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a repo-committed tools.json snapshot per server so metadata drift becomes reviewable, while extending tools list with --include-hidden and wiring that into contributor automation.
Changes:
- Added
--include-hiddensupport totools listoption binding and filtering. - Added
eng/scripts/Update-ToolsList.ps1and integrated tools metadata regeneration/verification intoAnalyze-Code.ps1. - Added/updated contributor guidance and committed
tools.jsonsnapshots for Fabric and Template servers.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
servers/Template.Mcp.Server/tools.json |
Adds committed tool metadata snapshot for the Template server. |
servers/Fabric.Mcp.Server/tools.json |
Adds committed tool metadata snapshot for the Fabric server. |
servers/Azure.Mcp.Server/docs/new-command.md |
Updates command-authoring guidance to use the new analysis workflow. |
eng/scripts/Update-ToolsList.ps1 |
Adds automation to generate/verify tools.json files from runtime metadata. |
eng/scripts/Analyze-Code.ps1 |
Adds -Fix mode and hooks in tools metadata regeneration/verification. |
core/Microsoft.Mcp.Core/src/Areas/Tools/Options/ToolsListOptions.cs |
Adds the bound option model for including hidden commands. |
core/Microsoft.Mcp.Core/src/Areas/Tools/Options/ToolsListOptionDefinitions.cs |
Defines the new --include-hidden CLI option. |
core/Microsoft.Mcp.Core/src/Areas/Tools/Commands/ToolsListCommand.cs |
Wires the new option into tool-list filtering logic. |
CONTRIBUTING.md |
Updates contributor instructions for the new analysis workflow. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Agent-Logs-Url: https://github.com/microsoft/mcp/sessions/96c767a3-4738-4d04-a2a8-a70b2344c3ec Co-authored-by: hallipr <1291634+hallipr@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
|
The changes adding support for The changes to generate the Additionally, needing to add a shipped runtime feature for CI concerns me. Anything could pass |
Contributor
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
hallipr
force-pushed
the
users/pahallis/tools-json
branch
from
f3af3d4 to
a76eb16
Compare
hallipr
changed the title
anannya03
reviewed
May 5, 2026
| @@ -1,15 +1,24 @@ | |||
| #!/bin/env pwsh | |||
Contributor
There was a problem hiding this comment.
Just to confirm, skipping the call to New-ToolList.ps1 in Analyze-Code.ps1 was intentional to avoid adding complexity in our Builds?
anannya03
reviewed
May 5, 2026
| Use -Verify to check that the committed tools.json files are up-to-date without | ||
| overwriting them (exits non-zero on drift). | ||
|
|
||
| .PARAMETER Verify |
Contributor
There was a problem hiding this comment.
The descrption says that we've VERIFY parameter support, but we don't implement it. I think it'd be safe to remove this parameter text,
jongio
reviewed
May 6, 2026
Contributor
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Two items that haven't been raised yet:
-
CONTRIBUTING.md and new-command.md both claim
Analyze-Code.ps1 -Fixhandles "tools.json drift" and "tools.json regeneration", but the actualAnalyze-Code.ps1changes in this PR don't callNew-ToolList.ps1. Either the integration is missing from this PR or the docs should drop the tools.json claim until it's wired in. -
New-ToolList.ps1line 72 usesWrite-Warning ... -ForegroundColor Red-Write-Warningdoesn't accept-ForegroundColor. This'll throw a ParameterBindingException at runtime when the null/missing-results branch is hit. Swap toWrite-Host(consistent with the other error paths in this script) or just drop the color parameter.
The --include-hidden implementation and test coverage look clean. Making searchCommandInCommandGroup static is a nice cleanup.
| ```pwsh | ||
| ./eng/scripts/Analyze-Code.ps1 -Fix | ||
| ``` | ||
|
|
Contributor
There was a problem hiding this comment.
This line claims Analyze-Code.ps1 -Fix runs "tools.json regeneration" but no call to New-ToolList.ps1 was added to the script in this PR. If the integration is planned for a follow-up, this doc update should wait for that commit so contributors don't hit a confusing gap between what the docs promise and what the script does.
| $parsed = $jsonString | ConvertFrom-Json | ||
|
|
||
| if ($null -eq $parsed -or $null -eq $parsed.results) { | ||
| Write-Warning " ❌ Invalid 'tools list' response for $($serverDir.Name)" -ForegroundColor Red |
Contributor
There was a problem hiding this comment.
Write-Warning doesn't accept -ForegroundColor. This'll throw a ParameterBindingException when the null/missing results branch executes. Use Write-Host here (consistent with the other error paths) or drop the color arg.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Refactoring command and options code has the potential to create unintentional drift in the tool metadata. Storing a definitive tool list for each server ensures that new PRs don't change runtime tool metadata without a corresponding change to a reviewable file in the repo.
This pull request introduces a new
--include-hiddenoption to thetools listcommand, updates the code analysis and formatting workflow, and improves automation for maintaining up-to-datetools.jsonmetadata files. The documentation is updated to reflect these changes and to guide contributors on the new workflow.Command-line enhancements:
--include-hiddenoption to thetools listcommand, allowing users to include hidden commands in the output. This involved updatingToolsListOptionDefinitions,ToolsListOptions, and the logic inToolsListCommandto filter commands based on the new option. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11]Automation and tooling improvements:
Analyze-Code.ps1script to support a-Fixflag, which now auto-fixes formatting, updates solution files, regeneratestools.json, and runs other checks. The script can also verify-only (as in CI) or apply fixes locally. [1] [2]Update-ToolsList.ps1script to automate regeneration and verification oftools.jsonmetadata files for each server, using thetools list --include-hiddencommand.Documentation updates:
CONTRIBUTING.mdandservers/Azure.Mcp.Server/docs/new-command.md) to instruct contributors to useAnalyze-Code.ps1 -Fixinstead of manual formatting and spelling commands, and to explain the new automation workflow. [1] [2] [3]GitHub issue number?
[Link to the GitHub issue this PR addresses]Pre-merge Checklist
servers/Azure.Mcp.Server/README.mdand/orservers/Fabric.Mcp.Server/README.mddocumentationREADME.mdchanges running the script./eng/scripts/Process-PackageReadMe.ps1. See Package READMEToolDescriptionEvaluatorand obtained a score of0.4or more and a top 3 ranking for all related test promptsconsolidated-tools.jsonbreaking-changelabelservers/Azure.Mcp.Server/docs/azmcp-commands.md./eng/scripts/Update-AzCommandsMetadata.ps1to update tool metadata inazmcp-commands.md(required for CI)servers/Azure.Mcp.Server/docs/e2eTestPrompts.mdcrypto mining, spam, data exfiltration, etc.)/azp run mcp - pullrequest - liveto run Live Test Pipeline