Skip to content

Add develop-mcp commands to grant BYO MCP server permissions - #500

Merged
Bhaarath Raguru (ragurubhaarath) merged 20 commits into
microsoft:mainfrom
ranceglammers1:ranlam-microsoft-mcp-server-permission-commands
Sep 24, 2026
Merged

Bhaarath Raguru (ragurubhaarath) merged 20 commits into
microsoft:mainfrom
ranceglammers1:ranlam-microsoft-mcp-server-permission-commands

Conversation

@ranceglammers1

@ranceglammers1 ranceglammers1 commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Adds one a365 develop-mcp command for granting BYO MCP server permissions to agent identities.

a365 develop-mcp grant-agents-access --agent-blueprint-id <GUID> --mcp-server-name <NAME>

It reports which agent instances of the blueprint are missing Tools.ListInvoke.All for the MCP server, then prompts for which ones to grant -- all, a comma-separated list of the numbers shown, or Enter to skip. --yes grants to every listed instance without prompting; --dry-run reports and exits without granting; when stdin is redirected the command reports and exits without granting, since there is nobody to prompt.

The MCP server name maps to an Entra application by appending - BYO (ext_Learn1 resolves ext_Learn1 - BYO); that application's service principal is the grant resource. The grant is a tenant-wide AllPrincipals delegated oauth2PermissionGrants entry.

An earlier revision of this branch also exposed --agent-serviceprincipal-id to grant one identity directly. It was removed: the interactive selection already covers picking individual instances, and a second way to name a target only widened the surface.

Blueprint discovery

Users generally do not know the blueprint GUID. First-party blueprint names and IDs are rendered from AgentBlueprintCatalog into both the --agent-blueprint-id help text and the error shown when the value is malformed, so a failed invocation tells you what to pass:

ERROR: --agent-blueprint-id is required and must be a GUID.
ERROR: First-party blueprints:
ERROR:   Sales Development Agent  eae28989-4f01-479b-8072-22902e554780

Both surfaces read the same constant, so adding a blueprint updates them together. The option is deliberately not marked IsRequired, so the handler -- not the parser -- reports a missing value and can print this catalog. An earlier revision added a separate list-agent-blueprints command; it was removed in favour of this, since requiring a second command to learn a value the failing command could print is worse for the one case that matters.

Device code authentication

--device-code covers terminals where the Windows WAM broker cannot show a dialog (embedded, SSH, CI). Making that work required fixing three separate bugs:

  1. MicrosoftGraphTokenProvider shelled out to Connect-MgGraph when no client app was configured. Verified experimentally that Connect-MgGraph -UseDeviceCode cannot work as a child process with redirected stdio -- it produces no output, exits 0, and leaves no context. Device code now resolves to in-process MSAL.
  2. That MSAL path needed a client app preauthorized for Graph delegated scopes. PowershellClientId is not, and returned AADSTS65002; it now uses GraphPowershellClientId (14d82eec-...), the app Connect-MgGraph itself authenticates as.
  3. That path had no persistent MSAL account to acquire silently against, so it re-prompted. It now routes through MsalBrowserCredential with useDeviceCode: true, which shares the OS-protected MSAL cache. AuthenticationService.CreateDeviceCodeCredential is deliberately left untouched, so setup on Linux/macOS/WSL, the browser-unsupported fallback, and develop get-token --device-code are unchanged.

The CLI authenticates two different client apps -- its base identity for directory lookups and the Graph command-line app for scoped calls -- and device code cannot SSO across distinct client apps. MSAL partitions its account cache per client ID, so the cached account is now resolved under the client actually being authenticated rather than a hardcoded one; the second app reuses that account instead of starting a fresh sign-in. A device-code run prompts once. Collapsing the two apps entirely would mean changing the CLI's base client app, which affects every command and does not belong here.

Testing

2088 unit tests pass. ConsoleHelper gains an input-redirection test seam alongside the existing ReadLine one, so the interactive prompt path is covered under the test runner (which always redirects stdin) rather than only its redirected fallback.

Verified end to end against a live tenant: resolved ext_Learn1 - BYO, listed two agent instances of a real blueprint, granted the scope to the one missing it, and confirmed the grant was created. Re-verified with tenant auto-detection (no --tenant-id) and the normal WAM path.

Also verified from the packed .nupkg installed as a tool: --device-code --dry-run completed with a single sign-in prompt, resolved the server, enumerated both agent instances, reported 0 missing, and exited 0.

Review round

Changes made in response to review, beyond the two gaps closed below:

  • A failed oauth2PermissionGrants read is no longer indistinguishable from an empty one, so --yes cannot grant on the strength of a lookup that never succeeded.
  • An ambiguous MCP server name (more than one application sharing the - BYO display name) no longer silently picks the first match: every match is listed and the user chooses. With input redirected there is nobody to ask, so it is an error.
  • --mcp-server-name is validated against the shared input allowlist before it reaches the Graph filter.
  • An explicitly blank --tenant-id is an error rather than a silent fallback to the Azure CLI context.
  • The device-code path no longer falls back to Connect-MgGraph and passes GraphPowershellClientId for Graph token acquisition.

Second review round

  • The cached login hint was looked up under a hardcoded client ID. MSAL partitions its account cache per client, so a device-code account could never be found. The client ID is now threaded through, and the token request and hint lookup derive from one property so they cannot drift apart.
  • A Graph authorization failure while reading applications was reported as "no application named X was found", sending the user off to create an application that may already exist. A failed read and an absent application are now distinguished.
  • An interactive selection that could not be parsed logged an error but exited 0. It now exits 1, so a typo cannot pass silently in a script.
  • Added HTTP-level coverage for the multi-match, read-failure, and empty-result application lookups, for the login-hint client wiring, and for the invalid-selection exit code.

Third review round: scoping to this command

Reviewers asked that this PR not change infrastructure other commands depend on. Everything flagged has been pulled back so the new behaviour is confined to grant-agents-access:

  • AuthenticationService.CreateDeviceCodeCredential is fully reverted, including the logger block that prints the device code prompt. This command never needed it -- its device-code path runs through GraphApiService -> MicrosoftGraphTokenProvider.GetMgGraphAccessTokenAsync(..., useDeviceCode, ...), which already took useDeviceCode. setup on Linux/macOS/WSL, the browser-unsupported fallback, and develop get-token --device-code are unchanged.
  • The mutable UseDeviceCodeAuthentication flag is off the GraphApiService singleton. useDeviceCode is an optional per-call parameter defaulting to false; the flag lives on McpServerPermissionService, which passes it explicitly. No later Graph call in the process can be affected.
  • The stricter consentType grant lookup is opt-in via requireMatchingConsentType, defaulting to false, so setup and create-instance are byte-for-byte unchanged. A test pins the unchanged default alongside the opt-in one.
  • FindApplicationByDisplayNameAsync is restored to its original standalone $top=1 query rather than delegating to the new multi-match lookup, so cleanup, ConfigService and SetupHelpers are unaffected.

Two correctness items were also addressed:

  • The multi-match lookup pages through @odata.nextLink with no $top, so every application sharing the name is returned and offered to the user to choose from.
  • The application lookup now returns why it failed instead of re-acquiring a token to decide which message to show, removing a path where a cancelled sign-in could prompt a second time.

Notes for reviewers

  • No new client app identity is introduced for the default path; this command reuses the CLI's existing base identity.
  • It reads the tenant from --tenant-id or Azure CLI context, and does not consult a365.config.json. That matches publish/register/setup, but differs from develop add-permissions/get-token, which do read config. Happy to align if reviewers prefer.
  • Pre-existing gap, deliberately not fixed here: the AllPrincipals grant lookup in CreateOrUpdateOauth2PermissionGrantCoreAsync filters on clientId + resourceId only and PATCHes arr[0], so an existing Principal grant for the same pair is patched and the tenant-wide grant is never created. Reviewers asked that this land separately with tests on the setup path, so the stricter consentType eq 'AllPrincipals' lookup is opt-in via requireMatchingConsentType, defaulting to false. setup and create-instance keep their exact current behaviour; grant-agents-access is the only caller opting in. A follow-up PR will fix the shared path.
  • Pre-existing gap, now fixed after review: DevelopMcpCommandTests enforced that every develop-mcp subcommand accepts --dry-run, but built the command with a null permission service, so this subcommand was never registered and escaped the check. The test now supplies a permission service, and the command supports --dry-run.
  • Not verified by me: the default (non---device-code) Windows sign-in path on a clean machine -- my box hits an unrelated WAM crash -- and a setup all --authmode obo run against a scratch agent, which would exercise the shared grant change above.

rancelammers and others added 5 commits September 21, 2026 10:05
Adds two subcommands under a365 develop-mcp:
- list-agent-instances reports agent instances of a blueprint missing
  Tools.ListInvoke.All for a BYO MCP server and offers to grant it.
- grant-mcpserver-permissions creates the AllPrincipals delegated grant
  for a single agent identity.

The MCP server name resolves to the Entra application '{name} - BYO';
that application's service principal is the grant resource.

Co-authored-by: Copilot App <[email protected]>
A failed Graph sign-in made FindApplicationByDisplayNameAsync return null,
which was reported as "No Entra application named '<name> - BYO' was found"
and pointed the user at creating an app that may already exist.

Co-authored-by: Copilot App <[email protected]>
Adds 'develop-mcp list-agent-blueprints', which lists Microsoft's
first-party agent blueprint names alongside their IDs. Users had no way
to find the GUID that --agent-blueprint-id requires. The invalid-GUID
error and the option help both point at the new command, so the hint
appears at the moment the user is stuck.

Also makes --device-code work on the two MCP permission commands:

- Connect-MgGraph cannot render a device code prompt from a child
  process with redirected I/O, so it silently produced no context.
  Device code now runs in-process via MSAL as the Graph command-line
  app, which is preauthorized for Graph delegated scopes (the Azure
  PowerShell app is rejected with AADSTS65002).
- Device code credentials were rebuilt per token request with no
  AuthenticationRecord, so the persisted cache could be written but
  never silently read, prompting on every call. They now route through
  MsalBrowserCredential, which attempts silent acquisition first.
- The MCP server lookup no longer acquires a pre-flight token on the
  success path.

Co-authored-by: Copilot App <[email protected]>
Replaces the standalone list-agent-blueprints command with the two
surfaces users already reach: the --agent-blueprint-id help text and
the error shown when the option is missing or not a GUID. With a single
first-party blueprint, a dedicated command asked users to run something
else to learn a value the failing command could simply print.

Both surfaces render from AgentBlueprintCatalog, so adding a blueprint
updates help and error output together and neither can go stale.

The service principal ID option deliberately does not list blueprints:
it takes a tenant-specific object ID, so those values are never valid
there.

Co-authored-by: Copilot App <[email protected]>
The summary labels said "Scope" and "Instances", which read ambiguously
next to the MCP server line, and the grant prompt quoted the scope with
no indication of which server it applied to. Name both in full.

The prompt uses the server name the caller passed rather than the
resolved '<name> - BYO' display name, so it echoes what they typed.

Co-authored-by: Copilot App <[email protected]>
Copilot AI lite review requested due to automatic review settings September 22, 2026 14:32
@ranceglammers1
ranceglammers1 requested review from a team as code owners September 22, 2026 14:32
- list-agent-instances exited 0 when a blueprint had no agent instances,
  so a script could not distinguish "nothing to do" from "wrong ID".
- ResolveMsalClientAppId carried two stacked doc comments, leaving
  AcquireGraphTokenViaMsalAsync undocumented.
- A comment named the wrong fallback client app.
- AgentBlueprintCatalog.TryGetDisplayName had no production caller.

Co-authored-by: Copilot App <[email protected]>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved issues affect dry-run safety, command fidelity, tenant handling, authentication, and Graph error reporting.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 6 Medium severity

Open (6)
What changed in this PR

Adds develop-mcp commands for discovering agent instances and granting BYO MCP server permissions, including device-code authentication support.

Changes:

  • Adds permission-management commands, services, models, and Graph integration.
  • Adds blueprint discovery and authentication updates.
  • Adds tests and changelog documentation.
File Summary
src/​Tests/​Microsoft.Agents.A365.DevTools.Cli.Tests/​Services/​MsalBrowserCredentialTests.cs Tests device-code credential behavior.
src/​Tests/​Microsoft.Agents.A365.DevTools.Cli.Tests/​Services/​MicrosoftGraphTokenProviderTests.cs Tests Graph token acquisition.
src/​Tests/​Microsoft.Agents.A365.DevTools.Cli.Tests/​Services/​McpServerPermissionServiceTests.cs Tests permission service behavior.
src/​Tests/​Microsoft.Agents.A365.DevTools.Cli.Tests/​Constants/​AgentBlueprintCatalogTests.cs Tests blueprint catalog formatting and validation.
src/​Tests/​Microsoft.Agents.A365.DevTools.Cli.Tests/​Commands/​McpServerPermissionsSubcommandsTests.cs Tests permission command validation and execution.
src/​Microsoft.Agents.A365.DevTools.Cli/​Services/​MsalBrowserCredential.cs Implements device-code credential support.
src/​Microsoft.Agents.A365.DevTools.Cli/​Services/​McpServerPermissionService.cs Resolves MCP resources and manages grants.
src/​Microsoft.Agents.A365.DevTools.Cli/​Services/​Internal/​MicrosoftGraphTokenProvider.cs Handles Graph token acquisition and client selection.
src/​Microsoft.Agents.A365.DevTools.Cli/​Services/​GraphApiService.cs Integrates Graph API authentication.
src/​Microsoft.Agents.A365.DevTools.Cli/​Services/​AuthenticationService.cs Updates authentication flows and persistence.
src/​Microsoft.Agents.A365.DevTools.Cli/​Program.cs Registers the new service.
src/​Microsoft.Agents.A365.DevTools.Cli/​Models/​McpServerPermissionModels.cs Defines permission-related models.
src/​Microsoft.Agents.A365.DevTools.Cli/​Constants/​McpConstants.cs Defines BYO MCP naming constants.
src/​Microsoft.Agents.A365.DevTools.Cli/​Constants/​AuthenticationConstants.cs Adds authentication client constants.
src/​Microsoft.Agents.A365.DevTools.Cli/​Constants/​AgentBlueprintCatalog.cs Provides first-party blueprint metadata.
src/​Microsoft.Agents.A365.DevTools.Cli/​Commands/​McpServerPermissionsSubcommands.cs Implements listing, validation, prompting, and granting.
src/​Microsoft.Agents.A365.DevTools.Cli/​Commands/​DevelopMcpCommand.cs Registers permission subcommands.
CHANGELOG.md Documents the new commands and authentication behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs Outdated
Comment thread src/Microsoft.Agents.A365.DevTools.Cli/Services/McpServerPermissionService.cs Outdated
Copilot AI review requested due to automatic review settings September 22, 2026 14:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate findings affect grant targeting, safety, authentication, validation, and permission correctness.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity · 6 Medium severity

Open (7)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Invalid numeric selection incorrectly exits successfully

src/​Microsoft.Agents.A365.DevTools.Cli/​Commands/​McpServerPermissionsSubcommands.cs:298

An invalid numeric selection logs an error and returns an empty list, but the caller treats selected.Count == 0 as a normal decline and leaves the exit code at 0. Scripts cannot detect invalid input; propagate a failure result from ResolveSelection and set context.ExitCode = 1 for this branch.

Medium severity Grant predicate ignores consent type

src/​Microsoft.Agents.A365.DevTools.Cli/​Services/​McpServerPermissionService.cs:110

The predicate ignores consentType. A Principal grant for the same client and resource that contains Tools.ListInvoke.All is not the tenant-wide AllPrincipals grant promised by these commands, but it will be reported as already granted and skipped. Require consentType to equal AllPrincipals here.

This issue also appears on line 132 of the same file.

Comment thread src/Microsoft.Agents.A365.DevTools.Cli/Services/McpServerPermissionService.cs Outdated
The command's primary effect is granting, not listing -- the listing is
a confirmation step before the grant. Leads the description with the
grant for the same reason.

Co-authored-by: Copilot App <[email protected]>
Copilot AI review requested due to automatic review settings September 22, 2026 17:12
One command now covers both targets: --agent-blueprint-id reviews every
agent instance of a blueprint, --agent-serviceprincipal-id grants one
identity directly. Exactly one must be supplied.

Co-authored-by: Copilot App <[email protected]>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved moderate issues remain in command contracts, validation, generated commands, authentication retries, and permission error handling.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity · 6 Medium severity

Open (7)
Resolved since last review (1)
Previously missed (4)

In code that hasn't changed since last review

Medium severity Mutating subcommands lack required --dry-run support

src/​Microsoft.Agents.A365.DevTools.Cli/​Commands/​DevelopMcpCommand.cs:47

These newly registered mutating subcommands have no --dry-run, while the established develop-mcp contract requires every subcommand to expose it (DevelopMcpCommandTests.cs:185-195). The existing test constructs this command with a null permission service, so it skips these registrations and cannot catch the production mismatch; add the safety option/handling or update the contract and tests intentionally.

Medium severity Invalid interactive selection incorrectly reports success

src/​Microsoft.Agents.A365.DevTools.Cli/​Commands/​McpServerPermissionsSubcommands.cs:298

This branch logs an invalid interactive selection and returns an empty list, but the caller treats every empty selection as a normal decline and returns without setting ExitCode = 1. A typo such as 99 therefore reports success to scripts even though no requested grant was performed; propagate an invalid-selection result separately from an intentional empty response.

Medium severity Device-code flag is lost during tenant-mismatch retry

src/​Microsoft.Agents.A365.DevTools.Cli/​Services/​Internal/​MicrosoftGraphTokenProvider.cs:152

The new useDeviceCode flag is propagated for the initial MSAL acquisition, but the tenant-mismatch retry later calls AcquireGraphTokenViaMsalAsync without that argument. A device-code invocation can therefore retry through browser/WAM (and lose the Graph PowerShell client fallback) instead of recovering; thread the flag through the retry path as well.

Medium severity Grant status check ignores consent type

src/​Microsoft.Agents.A365.DevTools.Cli/​Services/​McpServerPermissionService.cs:110

The status check matches only resource and scope, not consentType. A Principal grant for the same agent/resource is therefore reported as satisfying the command's required tenant-wide AllPrincipals grant, so the command can skip creating the grant it promises to manage.

This issue also appears on line 132 of the same file.

Copilot AI review requested due to automatic review settings September 22, 2026 17:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved validation, tenant/authentication, safety, and error-handling issues remain.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 High severity · 7 Medium severity

Open (9)
Previously missed (3)

In code that hasn't changed since last review

Medium severity Invalid selections incorrectly return success

src/​Microsoft.Agents.A365.DevTools.Cli/​Commands/​McpServerPermissionsSubcommands.cs:279

An invalid interactive selection is logged here but returned as []; the caller treats any empty result as a normal skip at lines 181-184 and leaves the exit code at 0. Thus 1,999 reports an error while scripting sees success. Preserve an invalid-selection result separately (for example, return null) and set context.ExitCode = 1 for it.

Medium severity Device-code flow uses the wrong Graph client ID

src/​Microsoft.Agents.A365.DevTools.Cli/​Services/​GraphApiService.cs:179

The new device-code switch only changes useInteractiveBrowser; this ambient path still lets AuthenticationService default the client ID to PowershellClientId. The resource lookup uses this path when no configured client app exists, so --device-code can fail with AADSTS65002 before the token-provider substitution to GraphPowershellClientId is reached. Pass the Graph PowerShell client ID for this device-code path or route ambient calls through the token provider.

Medium severity Application lookup failures are reported as not found

src/​Microsoft.Agents.A365.DevTools.Cli/​Services/​McpServerPermissionService.cs:60

FindApplicationByDisplayNameAsync returns null for HTTP, network, and response errors as well as for a missing application. This follow-up token check can still succeed from cache, causing a transient Graph failure to be reported as "No Entra application" and misleading the caller. Use a response-bearing lookup that distinguishes not-found from failure.

Drops --agent-serviceprincipal-id. The command lists the blueprint's
agent instances missing the MCP server scope and the user picks which
to grant, so there is no second way to name a target.

Adds a test seam for input redirection so the prompt path can be
covered under the test runner, which always redirects stdin.

Co-authored-by: Copilot App <[email protected]>
Copilot AI review requested due to automatic review settings September 22, 2026 17:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved critical authentication and permission-grant issues, plus moderate CLI behavior gaps, block approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 5 High severity · 5 Medium severity

Open (10)
Resolved since last review (2)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Return non-zero exit code for invalid interactive input

src/​Microsoft.Agents.A365.DevTools.Cli/​Commands/​McpServerPermissionsSubcommands.cs:201

An out-of-range or non-numeric response is logged as an error but returns an empty selection; the handler treats an empty selection as a deliberate skip and exits with code 0. Invalid interactive input must produce a non-zero exit code so scripts can distinguish it from Enter or redirected-input no-ops. Return a validity indicator (or nullable selection) and set context.ExitCode = 1 in the handler when the selection is invalid.

Medium severity Preserve application lookup failures instead of reporting not found

src/​Microsoft.Agents.A365.DevTools.Cli/​Services/​McpServerPermissionService.cs:60

FindApplicationByDisplayNameAsync returns null for both 'not found' and HTTP, network, or response failures. A second token acquisition only proves credentials can be obtained, so a failed query with a cached token is reported as 'No Entra application ... was found', potentially sending users to create an app that already exists. Use a status-bearing lookup result or preserve the original failure reason.

Comment thread src/Microsoft.Agents.A365.DevTools.Cli/Services/AuthenticationService.cs Outdated
Comment thread src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs Outdated
Comment thread src/Microsoft.Agents.A365.DevTools.Cli/Services/McpServerPermissionService.cs Outdated
Copilot AI review requested due to automatic review settings September 22, 2026 17:47
- Add --dry-run so the command can report missing grants without writing.
- Let the handler reject a missing --agent-blueprint-id so the blueprint
  catalog is printed instead of a bare parser error.
- Treat an explicitly blank --tenant-id as an error rather than silently
  falling back to the Azure CLI context.
- Stop falling back to Connect-MgGraph when --device-code is requested;
  the PowerShell prompt cannot work with redirected stdio.
- Distinguish a failed oauth2PermissionGrants read from an empty one so the
  command no longer reports every instance as missing after a Graph failure.
- Fail when more than one application shares the BYO MCP server display name
  instead of silently picking the first match.
- Validate --mcp-server-name against the shared input allowlist.

Co-authored-by: Copilot App <[email protected]>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Failed authentication can prompt twice, malformed selections can silently succeed, and one regression test does not exercise its claimed branch.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2 Medium severity · 2 Low severity

Open (4)
Resolved since last review (3)

Comment thread src/Microsoft.Agents.A365.DevTools.Cli/Services/McpServerPermissionService.cs Outdated
Comment thread CHANGELOG.md Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes.

  1. CI hasn't run the build/test workflow on this PR, only license/cla. Please get the .NET workflow to run; the new tests aren't validated otherwise.
  2. Beyond the new command, this changes shared auth/Graph behavior: the device code credential used by every command (AuthenticationService) and the AllPrincipals grant lookup used by setup / create-instance. Both look like legitimate fixes, but I'd like them in a separate PR so they can be reviewed, tested and reverted on their own. If they stay here, the title/description should call them out and they need test coverage on the setup/create-instance path.

All inline comments below need addressing before merge.

Comment thread src/Microsoft.Agents.A365.DevTools.Cli/Services/McpServerPermissionService.cs Outdated
Comment thread src/Microsoft.Agents.A365.DevTools.Cli/Services/AuthenticationService.cs Outdated
Comment thread src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs Outdated
Comment thread src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs Outdated
Comment thread src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs Outdated
Comment thread CHANGELOG.md Outdated
The interactive prompt split on ',' with RemoveEmptyEntries, so input like
"," or "1," produced zero tokens and was treated as "skip everything" with
exit code 0. A typo therefore left agents without the permission while the
command reported success. Empty tokens are now rejected with exit code 1.

The sign-in-failure test mocked the lookup as an empty list, which takes the
"application not found" branch and never reaches the token probe, so it would
have passed with the auth-failure handling deleted. Both resolve-failure tests
now mock a failed read and assert on the logged diagnostic.

Corrects the CHANGELOG entry for the consentType fix: setup and
create-instance create the missing tenant-wide grant rather than reporting a
failure.

Co-authored-by: Copilot App <[email protected]>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Lookup failure handling and incomplete pagination can misreport permission state or trigger incorrect remediation.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Resolved since last review (3)
Previously missed (3)

In code that hasn't changed since last review

Medium severity Treat missing or non-array Graph value as lookup failure

src/​Microsoft.Agents.A365.DevTools.Cli/​Services/​GraphApiService.cs:949

TryFindApplicationAppIdsByDisplayNameAsync promises null when the lookup cannot produce a trustworthy result, but a successful response with no array-valued value is converted to an empty list. The caller then reports that the application does not exist, even though the response was malformed. Treat a missing or non-array value as a failed lookup instead.

This issue also appears on line 1301 of the same file.

Medium severity Preserve Graph lookup failure details instead of reporting no principal

src/​Microsoft.Agents.A365.DevTools.Cli/​Services/​McpServerPermissionService.cs:101

LookupServicePrincipalByAppIdAsync returns null both for a successful empty result and for authentication, HTTP, transport, or response-format failures (GraphApiService.cs:652-664). Consequently a 403 or outage here is incorrectly reported as “has no service principal.” Use the existing status-bearing LookupServicePrincipalByAppIdWithResponseAsync; log its failure reason when IsSuccess is false, and reserve this message for a successful empty result.

Medium severity Handle all Graph grant pages before checking permissions

src/​Microsoft.Agents.A365.DevTools.Cli/​Services/​McpServerPermissionService.cs:130

This status check relies on TryGetOauth2PermissionGrantsAsync, which reads only the first Graph collection page and ignores @odata.nextLink (GraphApiService.cs:1308-1326). An agent identity with enough grants can therefore have this server's AllPrincipals grant on a later page and be incorrectly reported as missing, especially in --dry-run. Query by client/resource/consent type or follow all pages before computing HasScope.

Address review feedback that the PR altered shared infrastructure used by
setup, create-instance, cleanup and develop get-token.

- Revert CreateDeviceCodeCredential to its original DeviceCodeCredential
  implementation. The new command never used it; it reaches device code
  through MicrosoftGraphTokenProvider, which already took useDeviceCode.
- Replace the mutable UseDeviceCodeAuthentication flag on the GraphApiService
  singleton with an optional per-call parameter. The flag now lives on
  McpServerPermissionService, which passes it explicitly.
- Make the oauth2PermissionGrants consentType lookup opt-in via
  requireMatchingConsentType. Existing callers keep the original behaviour;
  only grant-agents-access opts in. The shared setup-path fix lands separately.
- Restore FindApplicationByDisplayNameAsync to its original standalone
  \=1 query instead of delegating to the new multi-match lookup.
- Query one past ApplicationDisplayNameMatchLimit so an over-limit result is
  reported as ambiguous rather than silently truncated.
- Return why an application lookup failed instead of re-acquiring a token to
  decide which error to show, which could prompt the user a second time.

Co-authored-by: Copilot App <[email protected]>
Copilot AI review requested due to automatic review settings September 23, 2026 16:26
The tenant-wide grant fix is now opt-in and no longer alters setup or
create-instance, and the device-code fix applies only to the new command.

Co-authored-by: Copilot App <[email protected]>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Device-code routing remains incomplete, client-specific cache resolution is inconsistent, and the shared grant behavior contradicts the documented fix.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 4 Medium severity

Open (4)
Resolved since last review (1)

Comment thread src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs
Comment thread src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs Outdated
Comment thread src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs
Comment thread src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs
Copilot AI review requested due to automatic review settings September 23, 2026 16:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Device-code routing can still use normal interactive authentication, and failed grant lookups can produce false success.

Review effort: Balanced
Findings: 4 Medium severity

Open (4)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Abort strict grant mode when existing-grant lookup fails

src/​Microsoft.Agents.A365.DevTools.Cli/​Services/​GraphApiService.cs:1224

GraphGetAsync can return null on an authentication or HTTP failure. This then leaves existingId null and proceeds to POST; if the grant already exists without the desired scope, the duplicate response is treated as success at lines 1296-1302, so the command can print Granted while the scope is still missing. For the new strict mode, abort when the existing-grant read fails.

Medium severity Distinguish service principal lookup failures from absent results

src/​Microsoft.Agents.A365.DevTools.Cli/​Services/​McpServerPermissionService.cs:100

LookupServicePrincipalByAppIdAsync returns null both for a successful empty result and for authentication, HTTP, or response failures (GraphApiService.cs:659-672), but this path reports every null as “has no service principal.” A permission or transient failure therefore sends users toward fixing a nonexistent registration problem. Use the status-bearing lookup and distinguish lookup failure from an absent service principal.

Drop the \ cap on the MCP server application lookup and page through
@odata.nextLink instead, so no match is hidden from the user.

When more than one application shares the MCP server's display name, list
them all and prompt for which to use rather than failing outright. With
input redirected there is nobody to ask, so that remains an error: display
names are not unique and guessing could grant against the wrong server.

Co-authored-by: Copilot App <[email protected]>
Copilot AI review requested due to automatic review settings September 23, 2026 16:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Device-code propagation and permission-grant discovery still contain correctness gaps.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity · 4 Medium severity

Open (5)
Previously missed (3)

In code that hasn't changed since last review

Medium severity Follow all pages when looking up delegated grants

src/​Microsoft.Agents.A365.DevTools.Cli/​Services/​GraphApiService.cs:1377

This new status lookup reads only the first collection page and ignores @odata.nextLink. If an agent identity has enough delegated grants for the MCP server's grant to appear on a later page, the command reports it as missing and attempts a duplicate grant. Follow all pages before deciding HasScope, as the new application lookup already does.

Medium severity Distinguish missing service principals from lookup failures

src/​Microsoft.Agents.A365.DevTools.Cli/​Services/​McpServerPermissionService.cs:120

LookupServicePrincipalByAppIdAsync returns null both when no service principal exists and when authentication, transport, or Graph authorization fails. This branch therefore tells users the application definitively has no service principal even when the directory could not be read. Use a response-bearing lookup or otherwise distinguish lookup failure from a successful empty result before emitting this message.

Low severity Correct device-code callback message about browser navigation

src/​Microsoft.Agents.A365.DevTools.Cli/​Commands/​McpServerPermissionsSubcommands.cs:56

The device-code callback only prints the verification URL and code; it does not open a browser. Saying it "Opens" the site is misleading, especially for the remote-terminal scenario this option targets. Describe that the user must visit the URL instead.

Comment thread src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs
…e grant state

EnsureGraphHeadersAsync honored useDeviceCode only in the token-provider branch.
The legacy fallback dropped it, so grant-agents-access could launch WAM despite
--device-code whenever CustomClientAppId was unresolved. Existing callers all
pass false, so the forwarded flag is a no-op for them.

A failed grant lookup left existingId null and fell through to POST, where
"Permission entry already exists" is reported as success even though the scope
was never merged. Gated behind abortWhenLookupFails so only the new command
opts in and setup/create-instance keep their current behavior.

Co-authored-by: Copilot App <[email protected]>
Copilot AI review requested due to automatic review settings September 23, 2026 17:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Device-code fallback semantics and paginated grant lookup remain incorrect.

Review effort: Balanced
Findings: None

Resolved since last review (5)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Graph lookup ignores pagination and may miss existing grants

src/​Microsoft.Agents.A365.DevTools.Cli/​Services/​GraphApiService.cs:1392

This reads only the first Graph page and ignores @odata.nextLink. If an agent identity has enough delegated grants for the target MCP grant to appear on a later page, the command reports it as missing (including in --dry-run) and may attempt an unnecessary grant. Follow all pages, as the application and blueprint lookups do, and return null if any page fails.

Low severity Device-code message falsely claims to open the verification URL

src/​Microsoft.Agents.A365.DevTools.Cli/​Commands/​McpServerPermissionsSubcommands.cs:56

The device-code implementation prints the verification URI and code; it does not open a browser. Saying it “Opens” the URL is misleading, especially for the remote-terminal users this option targets. Describe the manual navigation accurately.

The legacy fallback built a fresh Azure.Identity DeviceCodeCredential per
call with no AuthenticationRecord, so every Graph call under --device-code
re-prompted for a new code, and it wrote to a different cache store than the
scoped calls. Route the device-code path through the token provider, which
reads the cache before prompting and shares one store with every other call.

Gated on useDeviceCode, which only the new grant-agents-access path sets, so
all pre-existing callers keep the AuthenticationService path unchanged.

Co-authored-by: Copilot App <[email protected]>
Copilot AI review requested due to automatic review settings September 23, 2026 19:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Paging, duplicate application IDs, and the device-code retry path can produce incorrect or failed command behavior.

Review effort: Balanced
Findings: None

Previously missed (2)

In code that hasn't changed since last review

Medium severity Deduplicate application IDs across paginated results

src/​Microsoft.Agents.A365.DevTools.Cli/​Services/​GraphApiService.cs:1007

The paging guard can still return the same application ID more than once: the new repeated-nextLink test reads the same app-1 from two pages and currently asserts a count of two. ResolveServerResourceAsync then treats those duplicate IDs as distinct applications, causing a bogus selection prompt or a failure when input is redirected. Deduplicate IDs case-insensitively while accumulating pages, and update the cycle test to expect one ID.

This issue also appears on line 1395 of the same file.

Low severity Distinguish unreadable service principals from missing ones

src/​Microsoft.Agents.A365.DevTools.Cli/​Services/​McpServerPermissionService.cs:120

LookupServicePrincipalByAppIdAsync returns null both when the service principal is absent and when the Graph read fails, so this definitive message misreports authorization, network, or authentication failures as a missing service principal. Prefer a response-bearing lookup; at minimum, make this diagnostic acknowledge that the principal may be unreadable.

@ragurubhaarath
Bhaarath Raguru (ragurubhaarath) merged commit 83460b6 into microsoft:main Sep 24, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants