feat: MCP V1-to-V2 discovery migration#327
Conversation
…llback (will change to the endpoint once avl) - McpServerConfig: add Id and Publisher fields for V2 schema - McpConstants: add V2ScopeValue, IsV1Scope() helper, ManifestProperties.Id/Publisher, and hardcoded V2Catalog for dev unblocking - McpServerCatalogWriter: normalize V2 bare-array responses to wrapped format; add WriteHardcodedV2Catalog() fallback - DevelopCommand: fall back to hardcoded V2 catalog when live endpoint unavailable (Q1 pending) - Tests: 9 new McpServerCatalogWriterTests + 3 new McpServerConfigTests (V2 fields, deserialization, V1/V2 coexistence) Hardcoded catalog is temporary — remove once discoverToolServers?api-version=2 is confirmed live. V1 path unchanged; all 1280 existing tests pass.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Pull request overview
This PR advances the MCP discovery migration by supporting V2 catalog schema fields, normalizing V2 “bare array” responses into the existing wrapped catalog format, and introducing a temporary hardcoded V2 catalog fallback to unblock development while the live V2 endpoint is pending.
Changes:
- Added V2 schema support (
id,publisher) toMcpServerConfigplus related constants/helpers (V2ScopeValue,IsV1Scope, manifest property names). - Updated catalog writing to normalize V2 raw-array responses and added a hardcoded V2 catalog writer.
- Updated
DevelopCommandto fall back to the hardcoded catalog when the endpoint call fails; added/expanded unit tests for normalization and V2 fields.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Internal/McpServerCatalogWriterTests.cs | Adds tests for V2 normalization, hardcoded catalog writing, and new constants/helpers. |
| src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Models/McpServerConfigTests.cs | Adds tests asserting V2 fields default/null behavior and V1/V2 deserialization behavior. |
| src/Microsoft.Agents.A365.DevTools.Cli/Services/Internal/McpServerCatalogWriter.cs | Normalizes V2 raw-array catalogs into wrapped format; adds hardcoded V2 catalog writer. |
| src/Microsoft.Agents.A365.DevTools.Cli/Models/McpServerConfig.cs | Extends server config model with V2 id and publisher properties. |
| src/Microsoft.Agents.A365.DevTools.Cli/Constants/McpConstants.cs | Adds V2 scope constant, IsV1Scope helper, manifest property names, and hardcoded V2 catalog JSON. |
| src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopCommand.cs | Adds fallback behavior to write hardcoded V2 catalog when discover endpoint fails; adds second guard for add-mcp-servers. |
…V2 blueprint migration - ManifestHelper: add GetScopesByAudienceAsync() — groups manifest entries by audience (resourceAppId), supporting V1 (shared ATG AppId), V2 (per-server GUIDs), and mixed manifests. Fallback: missing or api:// audience resolves to ATG AppId. - PermissionsSubcommand: replace single-spec ATG write with per-audience ResourcePermissionSpec list so V2 per-server AppIds are written correctly alongside ATG scopes (additive by default). - PermissionsSubcommand: add --remove-legacy-scopes flag to setup permissions mcp with confirmation prompt — excludes ATG audience from blueprint write after V2 SDK is live. - Dry-run output now shows per-audience breakdown instead of flat scope list. - Tests: 8 new ManifestHelperGetScopesByAudienceTests covering V1/V2/mixed/edge cases. All existing callers unaffected — new removeLegacyAtgScopes param defaults to false. 1288 tests pass.
…ons mcp, and list-configured - add-mcp-servers: LogWarning when updated/added server resolves to legacy ATG audience (null, api://, or explicit ATG AppId), so operators know to re-run after V2 endpoint is live - setup permissions mcp: validate all manifest scopes are known (V1 pattern, V2 value, or McpServersMetadata.Read.All) before writing blueprint; returns false with LogError on unknown scopes - list-configured: derive and display VERSION column (V1/V2/Unknown) from each server's scope
…gured display
- CreateCompleteServerObject: add optional publisher param; writes 'publisher' key when present
- ConvertToServerObjects: read publisher from JsonElement and pass through
- UpsertMcpServersInManifest: read publisher from catalog on both update and add paths
- list-configured: read publisher from manifest entry and display 'Publisher: {value}' when set
publisher is part of the V2 catalog schema and was already defined in McpConstants.ManifestProperties
and McpServerConfig but was not flowing through to the manifest or output.
- Remove McpConstants.V2Catalog hardcoded JSON and WriteHardcodedV2Catalog()
- Remove both hardcoded fallback call sites in DevelopCommand
- Point ProductionDiscoverEndpointUrl to test V2 endpoint
(single constant update needed when prod V2 is ready)
- Remove V1 fallback logic — V2 mixed response covers all servers
(migrated + unmigrated) in one call, making V1 fallback redundant
- Delete 4 tests for the hardcoded catalog
No downstream schema changes — WriteCatalog already normalizes
V2 raw array [...] to {"mcpServers": [...]} format.
Existing agents and CLI commands unaffected.
…yAudienceAsync now treat the string "null" the same as a missing scope — it falls through to name-mapping fallback or is skipped entirely. Fix 2 — DevelopCommand.cs: Two places fixed: list-available display: Required Scope: null is now suppressed add-mcp-servers upsert (both update and add branches): "null" string is normalized to null before being written to ToolingManifest.json, so the file never contains a poisoned scope Fix 3 — GetTokenSubcommand.cs: The manifest-based path now calls AcquireAndDisplayManifestTokensAsync, which uses GetScopesByAudienceAsync and acquires one token per audience: V1 entries → token against ATG AppId with McpServers.*.All scopes V2 entries → token against per-server AppId with Tools.ListInvoke.All Mixed manifest → both tokens acquired and displayed Explicit --scopes / --resource paths are unchanged (single-token behavior preserved)
…mprovements - get-token writes BEARER_TOKEN_<SERVER_NAME> per V2 audience to .env / launchSettings.json; V1 shared token still written as BEARER_TOKEN for backward compatibility - setup permissions mcp --remove-legacy-scopes --dry-run now shows both "Would REMOVE" and "Would CONFIGURE" sections instead of only remaining - GetScopesByAudienceAsync treats "default" audience as a sentinel (same as missing or api:// legacy) — Dataverse custom servers no longer pass a literal "default" as an AAD resourceAppId - ToolingManifest duplicate check falls back to mcpServerName when mcpServerUniqueName is absent - ManifestHelper.GetServerNamesByAudienceAsync added to map audience GUIDs to server unique names for per-server token keying - AuthenticationConstants.GetPerServerBearerTokenEnvVar helper added
Adds -Mode (V1/V2/All), -ManifestPath, and -V2AppIds parameters to New-Agent365ToolsServicePrincipalProdPublic.ps1. Default mode (All) preserves existing V1 behaviour while also creating per-server V2 SPs when a ToolingManifest.json is supplied. Script is now fully idempotent — existing SPs are skipped on re-run rather than causing an early exit.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
scripts/cli/Auth/New-Agent365ToolsServicePrincipalProdPublic.ps1:145
- The script output includes non-ASCII symbols (e.g., "⚠", "✓", "✗"). Repo guidance is to avoid emojis/special characters in output for cross-platform terminal compatibility/log parsing. Replace these with plain text equivalents like "WARNING:", "Connected", "ERROR".
Write-Host "Connecting to Microsoft Graph..." -ForegroundColor Cyan
Write-Host "⚠ You need admin permissions for this operation." -ForegroundColor Yellow
Write-Host ""
try {
Connect-MgGraph -Scopes "AppRoleAssignment.ReadWrite.All" -NoWelcome
$context = Get-MgContext
Write-Host "✓ Connected to tenant: $($context.TenantId)" -ForegroundColor Green
Write-Host ""
}
catch {
Write-Host "✗ Failed to connect to Microsoft Graph" -ForegroundColor Red
Write-Host $_.Exception.Message -ForegroundColor Red
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
src/Microsoft.Agents.A365.DevTools.Cli/Constants/ConfigConstants.cs:1
GetDiscoverEndpointUrlnow always returnsProductionDiscoverEndpointUrlregardless ofenvironment, but still uses a switch expression. This adds complexity without behavior. Consider returning the constant directly (or reintroducing environment-specific endpoints if intended). Also, ensure indentation on the constant definition stays consistent with the surrounding file formatting to avoid style/lint churn.
// Copyright (c) Microsoft Corporation.
…ts.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…tion Agent-Logs-Url: https://github.com/microsoft/Agent365-devTools/sessions/7e73aa1b-ec4b-4af0-bce5-4400aa4c8155 Co-authored-by: biswapm <29349597+biswapm@users.noreply.github.com>
- Standardize log messages to use 'discoverMCPServers' (matches URL path) across all log sites in DevelopCommand - Add McpConstants.ResolveAudienceOrAtgFallback() to centralize audience normalization (null/whitespace, api://, "default" → ATG AppId); replace four inline copies in DevelopCommand and ManifestHelper - Update GetScopesByAudienceAsync doc comment to include "default" as an ATG fallback trigger - Avoid double manifest parse in dry-run --remove-legacy-scopes: compute remainingScopes in-memory from allScopes - Use mcpServerName (always present in endpoint response) instead of mcpServerUniqueName in GetServerNamesByAudienceAsync
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
scripts/cli/Auth/New-Agent365ToolsServicePrincipalProdPublic.ps1:145
- The script outputs non-ASCII symbols (e.g., "⚠", "✓", "✗"). Repository guidance for CLI-facing output is plain text without emojis/special characters to ensure cross-terminal compatibility and easy log parsing. Please replace these with ASCII equivalents like "WARNING:", "OK", "ERROR".
Write-Host "Connecting to Microsoft Graph..." -ForegroundColor Cyan
Write-Host "⚠ You need admin permissions for this operation." -ForegroundColor Yellow
Write-Host ""
try {
Connect-MgGraph -Scopes "AppRoleAssignment.ReadWrite.All" -NoWelcome
$context = Get-MgContext
Write-Host "✓ Connected to tenant: $($context.TenantId)" -ForegroundColor Green
Write-Host ""
}
catch {
Write-Host "✗ Failed to connect to Microsoft Graph" -ForegroundColor Red
Write-Host $_.Exception.Message -ForegroundColor Red
Replace ReadMcpScopesAsync + single ATG ResourcePermissionSpec in setup all and setup admin with ManifestHelper.GetScopesByAudienceAsync, building one ResourcePermissionSpec per audience — matching the behaviour already in setup permissions mcp. V1-only manifests are unchanged; mixed/V2 manifests now get the correct per-server AppId entries written to the blueprint. Consent URL helpers (PopulateAdminConsentUrls, BuildCombinedConsentUrl) retain V1/ATG compat by deriving ATG scopes from the audience dictionary; V2-only manifests produce empty ATG scopes (correct — no ATG entry needed). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rovider Replace raw Console.IsInputRedirected / Console.Write / Console.ReadLine in setup permissions mcp with confirmationProvider.ConfirmAsync, matching the pattern used by setup admin. The provider already handles EOF/Ctrl+C (throws OperationCanceledException) and accepts y/yes, so the explicit IsInputRedirected guard is no longer needed.
All 18 CreateCommand call sites in the test class were missing the new confirmationProvider parameter added in the previous commit. Add _mockConfirmationProvider (NSubstitute mock) to the test fixture and thread it through every call site.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
src/Microsoft.Agents.A365.DevTools.Cli/Helpers/ManifestHelper.cs:89
- V2 schema introduces an
idfield, andMcpConstants.ManifestProperties.Idexists, butCreateCompleteServerObject/ConvertToServerObjectsnever read or writeid. As a result, any manifest round-trip (read → write) oradd-mcp-serversupdate will dropidvalues. Ifidis meant to be persisted for V2 entries, add an optionalidparameter and include it in the dictionary when present, and plumb it through the callers that build server objects from catalog/manifest JSON.
public static object CreateCompleteServerObject(string serverName, string? uniqueName = null, string? url = null, string? scope = null, string? audience = null, string? publisher = null)
{
var serverObj = new Dictionary<string, object>
{
[McpConstants.ManifestProperties.McpServerName] = serverName,
[McpConstants.ManifestProperties.McpServerUniqueName] = uniqueName ?? serverName
};
scripts/cli/Auth/New-Agent365ToolsServicePrincipalProdPublic.ps1:145
- This script writes non-ASCII symbols (e.g., "⚠", "✓", "✗", and em-dash "—") to the console. These characters can render poorly or break copy/paste in some terminals and encodings (especially on Windows PowerShell with older code pages). Prefer plain ASCII equivalents (e.g., "WARNING:", "OK:", "ERROR:", and "-") for consistent cross-platform output.
Write-Host "Connecting to Microsoft Graph..." -ForegroundColor Cyan
Write-Host "⚠ You need admin permissions for this operation." -ForegroundColor Yellow
Write-Host ""
try {
Connect-MgGraph -Scopes "AppRoleAssignment.ReadWrite.All" -NoWelcome
$context = Get-MgContext
Write-Host "✓ Connected to tenant: $($context.TenantId)" -ForegroundColor Green
Write-Host ""
}
catch {
Write-Host "✗ Failed to connect to Microsoft Graph" -ForegroundColor Red
Write-Host $_.Exception.Message -ForegroundColor Red
Summary
idandpublisherfields for V2 schema (V1 entries get null — no breaking change)(resourceAppId), supporting V1 (shared ATG AppId), V2 (per-server GUIDs), and mixed
manifests. Fallback: missing or api:// audience resolves to ATG AppId.
list so V2 per-server AppIds are written correctly alongside ATG scopes (additive by default).
confirmation prompt — excludes ATG audience from blueprint write after V2 SDK is live.
Notes
V1 path unchanged. ATG scopes preserved by default.
Test plan