Skip to content

(Fix): Persist Managed Identity and Audience in MCP Client Tool workflow definition#9037

Merged
Bhavd13 merged 3 commits intomainfrom
bhavya/mcpbugfix
Apr 9, 2026
Merged

(Fix): Persist Managed Identity and Audience in MCP Client Tool workflow definition#9037
Bhavd13 merged 3 commits intomainfrom
bhavya/mcpbugfix

Conversation

@Bhavd13
Copy link
Copy Markdown
Contributor

@Bhavd13 Bhavd13 commented Apr 9, 2026

Commit Type

  • feature - New functionality
  • fix - Bug fix
  • refactor - Code restructuring without behavior change
  • perf - Performance improvement
  • docs - Documentation update
  • test - Test-related changes
  • chore - Maintenance/tooling

Risk Level

  • Low - Minor changes, limited scope
  • Medium - Moderate changes, some user impact
  • High - Major changes, significant user/system impact

What & Why

When configuring a BYO MCP server (e.g., IcM MCP) with Managed Identity authentication, the identity and audience fields were silently dropped from the serialized workflow definition JSON. Manually adding them in Code View would get wiped on save.

The identityfield was missing at 4 layers: manifest definition, consumption/standard connection parameter extraction, and both serializers.

Impact of Change

Impact of Change

  • Users: Consumption MCP workflows using Managed Identity authentication will now correctly persist identity and audience in the workflow definition, enabling end-to-end MSI auth for BYO MCP servers.
  • Developers: No API changes. The ManagedServiceIdentity parameter set in both consumption and standard MCP connector manifests now includes an identity parameter.
  • System: No performance or architectural impact. Additive change to existing serialization paths.

Test Plan

  • Unit tests added/updated
  • E2E tests added/updated
  • Manual testing completed
  • Tested in:

Contributors

@Bhavd13

Screenshots/Videos

Code Output after fix -
image

Copilot AI review requested due to automatic review settings April 9, 2026 07:15
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 9, 2026

🤖 AI PR Validation Report

PR Review Results

Thank you for your submission! Here's detailed feedback on your PR title and body compliance:

⚠️ PR Title

  • Current: (Fix): Persist Managed Identity and Audience in MCP Client Tool workflow definition
  • Issue: The title is descriptive but uses parentheses and capitalized tag (Fix):. The repository commonly expects conventional-commit style prefixes (lowercase fix:) and no surrounding parentheses. Minor style issue only.
  • Recommendation: Use a conventional commit style and remove parentheses. Example: fix: persist managed identity and audience in MCP client tool workflow definition

Commit Type

  • Properly selected (fix).
  • Only one commit type selected, which is correct.

⚠️ Risk Level

  • Current PR body selected: Low.
  • Issue: The PR is missing a repository risk label (e.g., risk:low / risk:medium / risk:high) on the PR. Also, based on the code changes (serialization logic in multiple designer libs, connection parameter extraction for both consumption and standard, and manifests), I recommend a higher risk classification than the one stated in the PR body.
  • Recommendation: Add the repository label risk:medium to this PR. The change touches multiple serialization and manifest paths and persistence of authentication fields — while additive and covered by unit tests, the impact surface makes medium more appropriate.

What & Why

  • Current: When configuring BYO MCP with Managed Identity, identity and audience fields were dropped. The PR adds missing identity and audience handling across manifests, connection extraction, and serializers.
  • Issue: None — description is clear and concise.
  • Recommendation: (No change required.)

Impact of Change

  • Impact is filled out and maps to Users/Developers/System.
  • Recommendation: Expand a one-line note stating that this is additive and backward-compatible to reassure reviewers: e.g., "Additive change to serialization and manifests; existing workflows without identity/audience remain unaffected." (optional)
    • Users: Consumption MCP workflows using Managed Identity will correctly persist identity and audience in the workflow definition.
    • Developers: No API changes; ManagedServiceIdentity parameter set now includes identity in both consumption and standard manifests.
    • System: No performance or architecture impact; change is additive to serialization paths.

Test Plan

  • Test plan selection: Unit tests added/updated.
  • Assessment: Matches the code diff — unit tests were added in consumption connection and operation manifest tests to cover identity/audience extraction and manifest parameter definitions. Good.
  • Recommendation: If possible, add a short note in the Test Plan describing the new unit tests added (paths/files) so reviewers can quickly find them.

Contributors

  • Contributors listed (@Bhavd13). Good — credit given.

Screenshots/Videos

  • A screenshot was included for the code output. Good for visual confirmation.

Summary Table

Section Status Recommendation
Title ⚠️ Use conventional commit style: fix: ...
Commit Type No change needed
Risk Level ⚠️ Add risk:medium label; consider updating body
What & Why Clear and adequate
Impact of Change Optional small clarification suggested
Test Plan Add short note pointing to test file paths
Contributors No change needed
Screenshots/Videos No change needed

Summary / Next Steps

  • This PR passes the PR-title-and-body checks, but I recommend two small improvements before merging:
    1. Update the PR title to follow conventional-commit style. Example: fix: persist managed identity and audience in MCP client tool workflow definition.
    2. Add the repository risk label risk:medium to this PR (the PR body currently states Low). Reason: changes affect multiple serialization and manifest surfaces that persist authentication details; while tests were added and the changes are additive, the broader surface area warrants medium risk.
  • Optional: add a brief line in the Test Plan listing the new/updated unit test files (for quick reviewer navigation):
    • libs/logic-apps-shared/.../consumption/tests/connection.spec.ts
    • libs/logic-apps-shared/.../consumption/tests/operationmanifest.spec.ts
    • serializer changes in libs/designer(-v2)/.../bjsworkflow/serializer.ts

Please update the PR title and add the risk:medium label, then re-submit (or leave a comment once done). Thank you for the thorough description and for adding unit tests — that made review much easier!


Last updated: Thu, 09 Apr 2026 07:29:12 GMT

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes MCP Client Tool workflow serialization so Managed Identity authentication settings (identity, audience) are persisted end-to-end (manifest → connection parameter extraction → workflow serializers) instead of being dropped on save.

Changes:

  • Added identity parameter to the MCP connector ManagedServiceIdentity parameter set (standard + consumption manifests).
  • Included identity in MCP connection parameter extraction paths (standard + consumption).
  • Updated Designer and Designer-v2 built-in MCP operation serialization to emit Connection.Identity / Connection.Audience when present; added/updated unit tests around MSI parameters.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
libs/logic-apps-shared/src/designer-client-services/lib/standard/manifest/mcpclientconnector.ts Adds identity parameter to the MSI parameter set for standard MCP connector manifest.
libs/logic-apps-shared/src/designer-client-services/lib/standard/connection.ts Ensures identity is included when building standard MCP connection authentication payload.
libs/logic-apps-shared/src/designer-client-services/lib/consumption/manifests/mcpclientconnector.ts Adds identity parameter to the MSI parameter set for consumption MCP connector manifest.
libs/logic-apps-shared/src/designer-client-services/lib/consumption/connection.ts Ensures consumption auth-parameter extraction includes identity for MCP connections.
libs/logic-apps-shared/src/designer-client-services/lib/consumption/tests/operationmanifest.spec.ts Adds unit tests asserting MSI parameter set includes identity and preserves existing audience constraints.
libs/logic-apps-shared/src/designer-client-services/lib/consumption/tests/connection.spec.ts Adds unit tests verifying MSI identity/audience extraction and inclusion in built-in MCP connection parameterValues.
libs/designer/src/lib/core/actions/bjsworkflow/serializer.ts Emits Connection.Identity / Connection.Audience for built-in MCP operation serialization based on resolved connection values.
libs/designer-v2/src/lib/core/actions/bjsworkflow/serializer.ts Same as above for Designer-v2 serializer.

@Bhavd13 Bhavd13 added the risk:low Low risk change with minimal impact label Apr 9, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 9, 2026

📊 Coverage Check

The following changed files need attention:

⚠️ libs/designer-v2/src/lib/core/actions/bjsworkflow/serializer.ts - 36% covered (needs improvement)
⚠️ libs/designer/src/lib/core/actions/bjsworkflow/serializer.ts - 17% covered (needs improvement)
⚠️ libs/logic-apps-shared/src/designer-client-services/lib/consumption/connection.ts - 42% covered (needs improvement)
⚠️ libs/logic-apps-shared/src/designer-client-services/lib/standard/connection.ts - 25% covered (needs improvement)

Please add tests for the uncovered files before merging.

@Bhavd13 Bhavd13 merged commit 1709f81 into main Apr 9, 2026
17 checks passed
@Bhavd13 Bhavd13 deleted the bhavya/mcpbugfix branch April 9, 2026 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-validated risk:low Low risk change with minimal impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants