Add --connectivity to register-external-mcp-server - #498
Open
Lala Sushant Srivastava (lasrivas) wants to merge 2 commits into
Open
Lala Sushant Srivastava (lasrivas) wants to merge 2 commits into
Lala Sushant Srivastava (lasrivas) wants to merge 2 commits into
Conversation
Lets the admin say whether the MCP server is internet-reachable or only reachable inside the environment's VNet, which decides whether the connector keeps VNet injection. Validated in the handler rather than via FromAmong so the error message matches the command's other options. The summary warns on 'public' that the bypass depends on environment enablement: Power Platform silently ignores the request for environments not enabled for it and still reports success, so we can report what was asked for but never what took effect. Inert until the platform side ships; an older platform ignores the unknown property. Co-authored-by: Copilot <[email protected]>
Lala Sushant Srivastava (lasrivas)
requested review from
a team
as code owners
September 21, 2026 20:07
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Copilot started reviewing on behalf of
Lala Sushant Srivastava (lasrivas)
September 21, 2026 20:08
View session
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Fix empty-value validation, add successful request-construction coverage, and document the option in the changelog.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (3)
What changed in this PR
Adds --connectivity public|private to external MCP server registration, supporting CLI/JSON input and forwarding the selected value to the platform.
Changes:
- Adds connectivity option, input-model fields, request payload support, and sample JSON.
- Implements precedence, normalization, validation, and public-connectivity warnings.
- Adds validation-focused tests.
| File | Summary |
|---|---|
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/RegisterCommandExecutorTests.cs |
Adds connectivity validation tests; successful request construction and precedence remain uncovered. |
src/Microsoft.Agents.A365.DevTools.Cli/Templates/register-external-mcp-server-sample.json |
Documents connectivity input and the private default. |
src/Microsoft.Agents.A365.DevTools.Cli/Models/RegisterExternalMcpServerInput.cs |
Adds JSON connectivity support. |
src/Microsoft.Agents.A365.DevTools.Cli/Models/AddMcpServerRequest.cs |
Adds connectivity to the platform request. |
src/Microsoft.Agents.A365.DevTools.Cli/Commands/RegisterCommandExecutor.cs |
Resolves, validates, summarizes, and forwards connectivity; empty values can bypass validation, and request construction needs coverage. |
src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs |
Registers the CLI option; a changelog entry is missing. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Rick Brighenti (rbrighenti)
requested changes
Sep 23, 2026
Rick Brighenti (rbrighenti)
left a comment
Rick Brighenti (rbrighenti)
left a comment
There was a problem hiding this comment.
Requesting changes, small ones. Also needs a CHANGELOG.md [Unreleased] entry for the new --connectivity option (one consumer-facing sentence with (#498)).
Reject a supplied-but-blank `--connectivity` rather than treating it as absent. The old `IsNullOrWhiteSpace` guard let `--connectivity " "` skip validation and register the server as private -- the opposite of what someone typing the option intends -- and let a blank CLI value quietly beat a valid input-file value through the `??=` merge. Cover the accepted path, not just the rejected one. `ResolveInputsAsync` and `ResolvedInput` become internal (the test assembly already has InternalsVisibleTo) so the normalised value can be asserted where it is resolved, including CLI-over-input-file precedence. Reaching `AddMcpServerAsync` itself is not testable: it sits behind concrete `GraphApiService` Entra app creation. Adds the missing CHANGELOG entry. Co-authored-by: Copilot <[email protected]>
github-actions
Bot
added
the
documentation
Copilot started reviewing on behalf of
Lala Sushant Srivastava (lasrivas)
September 24, 2026 01:04
View session
Author
|
@microsoft-github-policy-service agree [company="Microsoft"] |
Author
|
@microsoft-github-policy-service agree company="Microsoft" |
Rick Brighenti (rbrighenti)
approved these changes
Sep 24, 2026
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.
Adds
--connectivity public|privatetoa365 develop-mcp register-external-mcp-server. The admin says whether their MCP server is reachable on the internet or only inside the environment's VNet; the platform uses that to decide whether the connector keeps VNet injection. Platform side: bic/MCP-Platform#3774. Inert until that ships — an older platform ignores the unknown property. ## Summary ---connectivityoption, plusconnectivityin the JSON input file and the sample template. Explicit option beats input file beats theprivatedefault. - Validated in the handler rather than viaFromAmong, matching--auth-typein the same command, so the error message is ours. Trimmed and case-insensitive; anything other thanpublic/privateexits 1 without calling the service. - The registration summary warns onpublicthat the bypass applies only to environments enabled for it. ## What a reviewer should check Power Platform silently ignores the bypass for environments not enabled for it and still reports success, so the CLI can report what was asked for but never what took effect. Hence the warning rather than a claim.Review feedback addressed
--connectivityis now rejected. The guard wasIsNullOrWhiteSpace, so--connectivity " "skipped validation entirely and the server registered asprivate-- the opposite of what someone typing the option intends. It also let a blank CLI value quietly beat a valid input-file value through the??=merge. The guard is nowis not null, and the error quotes the offending value.ResolveInputsAsyncandResolvedInputareinternal(the test assembly already hadInternalsVisibleTo) so tests assert the normalised value where it is resolved, including CLI-over-input-file precedence. Asserting on the outbound platform request is not reachable:AddMcpServerAsyncsits behind concreteGraphApiServiceEntra app creation with no interface seam.register-external-mcp-serverhas no page underdocs/.Note the platform guard merged in bic/MCP-Platform#3774 still has the original
IsNullOrWhiteSpaceshape. That is not a behaviour gap (blank maps toprivateon both sides, and the CLI now rejects it before it gets there), but the two should converge in a follow-up.Full suite after the fixes: 2030 passed, 0 failed, 12 skipped.