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 title specific and remove trailing space. Example provided above. | |
| Commit Type | ✅ | None required. |
| Risk Level | ❌ | Change PR checkbox to Medium and add label risk:medium, or justify Low with details. |
| What & Why | ✅ | Add brief file list and exact behavioral summary. |
| Impact of Change | Expand System section to mention storage/architecture impact. | |
| Test Plan | ✅ | Unit tests present; consider integration/E2E for createManagedMcpConnection or justify absence. |
| Contributors | Add contributor credits or state none. | |
| Screenshots/Videos | ✅ | None needed. |
Final notes
- Overall: The PR content is promising and you added unit tests which is excellent. However, because this change adds new connection creation flows, a manifest, and UI logic changes, I recommend labeling this as Medium risk rather than Low and adding the repo label
risk:medium.
Action items for you to update the PR before re-submission:
- Update the PR title to a clearer form (example:
Add MCP (mcpclient) support for Consumption Logic Apps (createBuiltInMcpConnection + manifest)). - Update the Risk Level checkboxes to select Medium and add the repository label
risk:medium. If you want to keep Low, add a short justification in the PR body explaining why the behavioral impact is minimal and why regression risk is low. - In "What & Why", add a one-line summary of changed files (e.g., ConsumptionConnectionService, createConnection UI, new manifest file path, new unit tests) and clarify what is stored in workflow.json vs API Hub.
- Expand the "System" slice under Impact of Change to call out storage/architecture implications and any migration or compatibility considerations.
- Add contributor credits or explicitly say none.
- Consider adding E2E/integration tests for the managed MCP flow (createManagedMcpConnection) or add a note explaining why E2E is not included and how this will be validated.
Please update the PR with the above items and add the risk:medium label (or justify Low) — once updated I can re-run the check. Thank you for the thorough work and tests added so far!
Last updated: Wed, 04 Feb 2026 12:03:49 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 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
1 participant
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