Open
Conversation
🤖 AI PR Validation ReportPR Review ResultsThank you for your submission! Here's detailed feedback on your PR title and body compliance:
|
| Section | Status | Recommendation |
|---|---|---|
| Title | Make it explicit and remove trailing space. Example: Add MCP connection support for Consumption Logic Apps (built-in and managed) |
|
| Commit Type | ✅ | No action needed |
| Risk Level | ❌ | Update PR label to match body and consider setting to Medium (recommended). Provide justification if keeping Low. |
| What & Why | ✅ | Add file list / short mapping to diff for faster review. |
| Impact of Change | Expand "System" section to reflect storage/auth/telemetry impacts; mention any security considerations. | |
| Test Plan | ✅ | Link to CI run or confirm tests pass; consider adding manual/E2E tests for UI and storage behaviors. |
| Contributors | Add contributor list or note none. | |
| Screenshots/Videos | Provide screenshots or manual test steps for UI change. |
Final notes:
- Advised risk level: Medium (higher than the PR's selected Low). This is because the PR adds new connection behaviors (built-in vs managed), a new connector manifest, and alters UI behavior for connection creation — all of which can have broader impacts than a small/localized change.
- Please update the PR with the following actionable items before re-requesting review:
- Update the PR title to be more descriptive and remove trailing whitespace.
- Update the Risk Level selection or provide a detailed justification to keep it at Low; add the appropriate repository label (
risk:mediumif you accept the advised level, orrisk:lowif justified). - Expand the Impact of Change section to include system-level implications and any security considerations for storing connection data in workflow.json.
- Confirm that the newly added unit tests are passing in CI and link to the CI run, or include instructions for reproducing the successful test run.
- Add manual testing notes or E2E tests to validate the user flow (creating built-in MCP connection stores expected values in workflow.json, managed connection flows via API Hub, and UI hides/shows name input as intended).
- Add contributors (if any) and provide screenshots or manual test steps for the UI change.
Please update the PR with the requested changes and labels, then re-submit. Thanks for the thorough implementation and for adding unit tests — this is a solid start; with the requested clarifications and label corrections we can proceed to a full review.
Last updated: Fri, 06 Mar 2026 06:39:14 GMT
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds MCP (Model Context Protocol) connection support for Consumption Logic Apps by implementing built-in and managed MCP connection capabilities. The implementation reuses most existing MCP code for managed connectors, with Consumption-specific additions for built-in connections that store connection objects locally in workflow.json instead of API Hub.
Changes:
- Added Consumption-specific MCP connector manifest (
mcpclientconnector.ts) with builtin capability - Created
createBuiltInMcpConnection()method inConsumptionConnectionServicefor local connection storage - Added comprehensive unit tests for MCP connection creation and parameter extraction
- Modified connection panel to hide name input for Consumption MCP connections
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| servers | Subproject commit reference update |
| libs/logic-apps-shared/src/designer-client-services/lib/consumption/manifests/mcpclientconnector.ts | Defines the MCP Client connector manifest with authentication types and parameter sets |
| libs/logic-apps-shared/src/designer-client-services/lib/consumption/connection.ts | Implements built-in and managed MCP connection creation logic with parameter extraction |
| libs/logic-apps-shared/src/designer-client-services/lib/consumption/tests/connection.spec.ts | Adds comprehensive unit tests for MCP connection functionality |
| libs/designer-v2/src/lib/ui/panel/connectionsPanel/createConnection/createConnection.tsx | Hides name input for Consumption MCP connections in the UI |
| }, | ||
| }, | ||
| uiDefinition: { | ||
| displayName: 'Basic', |
There was a problem hiding this comment.
The authentication type name 'Basic' should match the parameter set name defined on line 41. Currently the parameter set uses 'Basic' as the name, but this should be 'BasicAuth' for consistency with the extractAuthParameters method which checks for 'BasicAuth' in connection.ts line 239.
|
|
||
| if (connectionParametersSet?.values) { | ||
| const values = connectionParametersSet.values; | ||
| const authKeys = ['username', 'password', 'key', 'keyHeaderName', 'clientId', 'secret', 'tenant', 'authority', 'audience', 'pfx']; |
There was a problem hiding this comment.
The hardcoded list of authentication keys should be defined as a constant at the class or module level to improve maintainability and allow reuse. Consider extracting this to a private static readonly property or a module-level constant.
| }); | ||
|
|
||
| try { | ||
| const connectionName = connectionInfo.displayName || connectionId.split('/').at(-1) || `mcp-${Date.now()}`; |
There was a problem hiding this comment.
The fallback connection name generation using Date.now() could produce non-unique names if multiple connections are created in rapid succession. Consider using a more robust unique identifier generation approach, such as a UUID or a combination of timestamp with a random component.
Suggested change
| const connectionName = connectionInfo.displayName || connectionId.split('/').at(-1) || `mcp-${Date.now()}`; | |
| const fallbackSuffix = `${Date.now()}-${Math.random().toString(16).slice(2)}`; | |
| const connectionName = connectionInfo.displayName || connectionId.split('/').at(-1) || `mcp-${fallbackSuffix}`; |
| expect(result.name).toBe('test-mcp-connection'); | ||
| expect(result.id).toContain('connectionProviders/mcpclient/connections/'); | ||
| expect((result.properties as any).parameterValues.mcpServerUrl).toBe('https://mcp-server.example.com'); | ||
| expect((result.properties as any).parameterValues.authenticationType).toBe('ApiKey'); |
There was a problem hiding this comment.
The test expects authenticationType to be 'ApiKey' but the connection info specifies 'ApiKey' on line 78. However, the connector manifest defines 'Key' as the authentication type name (line 275). This inconsistency should be resolved to match the actual authentication type names defined in the manifest.
This was referenced Feb 24, 2026
This was referenced Mar 6, 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.
Commit Type
Risk Level
What & Why
This PR implements MCP (Model Context Protocol) connection support for Consumption Logic Apps. Most of the MCP implementation code was already common for managed connectors, shared between Standard and Consumption. For built-in MCP connections in Consumption, I removed the connection panel settings dependency and created a dedicated
createBuiltInMcpConnection()method inConsumptionConnectionServicethat generates local connection objects stored inworkflow.jsoninstead of API Hub. I also created a Consumption-specific MCP connectormcpclientconnector.tswith the builtin capability to differentiate it from managed connectors.Impact of Change
ConsumptionConnectionServicefor handling built-in MCP connections.Test Plan
Contributors
Screenshots/Videos