Add ToolName validation to AnalyzeCode.ps1 #1592
Conversation
conniey
requested review from
KarishmaGhiya,
g2vinay,
hallipr,
sandeep-sen and
weshaggard
There was a problem hiding this comment.
Pull request overview
Adds a new PowerShell validation step to enforce MCP tool naming character rules and wires it into the existing Analyze-Code.ps1 developer/CI analysis script.
Changes:
- Added
Test-ToolNameCharacters.ps1to scan tool source files for invalid name characters. - Updated
Analyze-Code.ps1to run the new character validation and print violations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| eng/scripts/Test-ToolNameCharacters.ps1 | New script that scans tools/ for Name definitions and validates allowed characters/dash placement. |
| eng/scripts/Analyze-Code.ps1 | Runs the new tool name character validation and fails analysis when violations are found. |
|
|
||
| $hasErrors = $true |
There was a problem hiding this comment.
This new validation will fail in the current repo and make Analyze-Code.ps1 always report errors: existing tool names include underscores (e.g., tools/Azure.Mcp.Tools.AzureBestPractices/src/AzureBestPracticesSetup.cs:13 has "get_azure_bestpractices" and AIAppBestPracticesCommand has "ai_app"), which Test-ToolNameCharacters.ps1 treats as invalid. Either rename/fix the existing tool names as part of this PR, or scope/exempt this check so it doesn’t break the baseline.
| $hasErrors = $true |
| class ToolArea { | ||
| [string]$ToolArea | ||
| [ToolNameViolation[]]$Violations | ||
|
|
||
| ToolArea([string]$ToolArea) { | ||
| $this.ToolArea = $ToolArea | ||
| $this.Violations = @() | ||
| } | ||
|
|
||
| [void]AddViolation([ToolNameViolation]$violation) { | ||
| $this.Violations += $violation | ||
| } | ||
|
|
||
| [bool]HasViolations() { | ||
| return $this.Violations.Count -gt 0 | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
ToolArea class is declared but never used (violations are accumulated as a plain array). Removing it (or actually using it to group violations) would reduce dead code and keep the script easier to maintain.
| class ToolArea { | |
| [string]$ToolArea | |
| [ToolNameViolation[]]$Violations | |
| ToolArea([string]$ToolArea) { | |
| $this.ToolArea = $ToolArea | |
| $this.Violations = @() | |
| } | |
| [void]AddViolation([ToolNameViolation]$violation) { | |
| $this.Violations += $violation | |
| } | |
| [bool]HasViolations() { | |
| return $this.Violations.Count -gt 0 | |
| } | |
| } | |
| # ToolArea class removed; violations are tracked directly as ToolNameViolation objects. |
| foreach ($file in $areaTools) { | ||
| Write-Debug "Processing: $($file.FullName)" | ||
| $content = Get-Content $file.FullName | ||
|
|
||
| if ($file.Name -like '*Command.cs') { | ||
| $matchingPattern = 'public override string Name => "(.*)";' | ||
| } elseif ($file.Name -like '*Setup.cs') { | ||
| $matchingPattern = 'public string Name => "(.*)";' | ||
| } else { | ||
| throw "Unexpected file name: $($file.Name)" | ||
| } | ||
|
|
||
| foreach ($line in $content) { | ||
| if ($line -match $matchingPattern) { | ||
| $toolName = $matches[1] |
There was a problem hiding this comment.
The PR description links to command design principles for command group naming, but this script only validates Name properties in *Command.cs/*Setup.cs. As a result, invalid CommandGroup names (e.g., tools/Azure.Mcp.Tools.ManagedLustre/src/ManagedLustreSetup.cs uses "blob_autoexport" / "blob_import") won’t be detected. Consider extending the scan to validate CommandGroup("...") names too, or clarify that this check intentionally excludes command group names.
| # Validate tool name format: | ||
| # - Each group contains alphanumeric chars or dashes | ||
| # - Each group cannot start or end with a dash | ||
| # Pattern breakdown: | ||
| # ^ - Start of string | ||
| # [a-zA-Z0-9] - First char must be alphanumeric | ||
| # ([a-zA-Z0-9-]*[a-zA-Z0-9])? - Optional: middle chars (alphanumeric or dash) ending with alphanumeric | ||
| # $ - End of string | ||
| $isValid = $($ToolName -match '^[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?$') |
There was a problem hiding this comment.
The validation regex allows uppercase letters, but the referenced command design principles specify lowercase command group/service/resource/operation names. To enforce the documented convention, restrict the allowed characters to lowercase (and digits/dashes) rather than [a-zA-Z0-9-].
1 participant
What does this PR do?
Adds tool name validation to AnalyzeCode.ps1 to ensure only tools with valid naming is added. See: Command Design Principles.
GitHub issue number?
Related: #1566
Pre-merge Checklist
servers/Azure.Mcp.Server/CHANGELOG.mdand/orservers/Fabric.Mcp.Server/CHANGELOG.mdfor product changes (features, bug fixes, UI/UX, updated dependencies)servers/Azure.Mcp.Server/README.mdand/orservers/Fabric.Mcp.Server/README.mddocumentationeng/scripts/Process-PackageReadMe.ps1. See Package README/servers/Azure.Mcp.Server/docs/azmcp-commands.mdand/or/docs/fabric-commands.md.\eng\scripts\Update-AzCommandsMetadata.ps1to update tool metadata in azmcp-commands.md (required for CI)ToolDescriptionEvaluatorand obtained a score of0.4or more and a top 3 ranking for all related test promptsconsolidated-tools.json/servers/Azure.Mcp.Server/docs/e2eTestPrompts.mdcrypto mining, spam, data exfiltration, etc.)/azp run mcp - pullrequest - liveto run Live Test Pipeline