Skip to content

fix(designer): Only populate deploymentModelProperties for MicrosoftFoundry#9012

Open
Elaina-Lee wants to merge 1 commit intomainfrom
elaina/fix-foundry-conn
Open

fix(designer): Only populate deploymentModelProperties for MicrosoftFoundry#9012
Elaina-Lee wants to merge 1 commit intomainfrom
elaina/fix-foundry-conn

Conversation

@Elaina-Lee
Copy link
Copy Markdown
Contributor

@Elaina-Lee Elaina-Lee commented Apr 5, 2026

Commit Type

  • fix - Bug fix

Risk Level

  • Low - Minor changes, limited scope

What & Why

When an agent node is created, deploymentModelProperties (name, format, version) were hardcoded with defaults (gpt-4.1, OpenAI, 2025-06-10) in the manifest. These values were baked in at node creation and never updated when the user selected a different model, because buildDependentParam always fell back to the schema defaults.

Additionally, deploymentModelProperties should only be populated for MicrosoftFoundry — for AzureOpenAI the backend derives model info from the deployment itself.

This PR:

  • Removes hardcoded defaults from the manifest
  • Changes visibility of deploymentModelProperties to MicrosoftFoundry only
  • Clears deploymentModelProperties when switching away from MicrosoftFoundry
  • Only populates deploymentModelProperties when a Foundry model is selected (using the model ID directly + AGENT_MODEL_CONFIG lookup)

Impact of Change

  • Users: Foundry model properties now correctly reflect the selected model instead of always showing gpt-4.1. AzureOpenAI agents no longer carry unnecessary deploymentModelProperties.
  • Developers: No API changes. The deploymentModelProperties fields in the manifest are now scoped to MicrosoftFoundry visibility only.
  • System: No performance or architectural changes.

Test Plan

  • Unit tests added/updated
  • E2E tests added/updated
  • Manual testing completed
  • Tested in: Standalone designer
    • Create a new agent with AzureOpenAI — verify deploymentModelProperties are NOT populated
    • Create a new agent with MicrosoftFoundry — verify properties remain empty until a model is selected
    • Select a Foundry model — verify name/format/version populate correctly
    • Switch from MicrosoftFoundry to AzureOpenAI — verify properties are cleared
    • Verify existing AzureOpenAI workflows load without regression

Contributors

@hyehwalee

Screenshots/Videos

Copilot AI review requested due to automatic review settings April 5, 2026 02:30
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 5, 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(designer): Only populate deploymentModelProperties for MicrosoftFoundry
  • Issue: None — title is clear, follows conventional commit style and describes the change scope.
  • Recommendation: None.

Commit Type

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

⚠️ Risk Level

  • The PR body and label declare: Low - Minor changes, limited scope and the PR is labeled risk:low.
  • Assessment: label and body match, so this is syntactically correct. However, the code touches manifest defaults and runtime UI behavior for agent model properties across AzureOpenAI vs MicrosoftFoundry. Those changes can affect persisted manifests and agent behavior in user workflows. Because of that potential for user-impacting behavior (clearing/omitting previously-present fields, changing visibility), I advise: risk:medium.
    • Recommendation: If you have strong evidence these changes are backward-compatible (migration handled server-side, or existing manifests unaffected), add a short note in the PR body under "Impact of Change" explaining why this is safe for existing workflows. If not, consider marking risk as medium and adding migration notes.

What & Why

  • Current: The section is present and explains the issue (hardcoded defaults, buildDependentParam fallback, and the correct scoping to MicrosoftFoundry) and lists the changes.
  • Issue: None substantive. Consider adding a one-line note about whether existing agent manifests in stored workflows will change or require migration.
  • Recommendation: Example sentence to add: This change will not modify existing persisted manifests at runtime because <reason> OR This change will cause existing persisted manifests to no longer show deploymentModelProperties and a migration may be necessary.

Impact of Change

  • The section is populated with Users/Developers/System breakdown.
  • Issue: It's good; reinforce whether existing persisted manifests/workflows will be changed or left intact.
  • Recommendation: Add a short line about whether this will require any server-side migration or whether existing AzureOpenAI manifests will still load identically.
    • Users: Foundry model properties now reflect selected model; AzureOpenAI agents no longer have unnecessary deploymentModelProperties.
    • Developers: No API changes; manifest visibility changed for the fields.
    • System: No perf/architectural change claimed; add confirmation if verified.

⚠️ Test Plan

  • The PR indicates only Manual testing completed and provides detailed manual steps — this is acceptable for a behavior/UI change.
  • Issue: No unit or E2E tests were added/updated in the diff. For changes that alter dependency-building and parameter population/clearing, it's recommended to add at least unit tests covering:
    • Clearing of deploymentModelProperties when model type changes away from MicrosoftFoundry.
    • Correct population of deploymentModelProperties when MicrosoftFoundry model is selected (using AGENT_MODEL_CONFIG lookup).
    • That AzureOpenAI paths do not populate these fields.
  • Recommendation: Add small unit tests for the parameter-handling logic in both libs/designer and libs/designer-v2. If adding tests is not feasible for this PR, add a short justification in the Test Plan explaining why manual testing is sufficient and whether follow-up tests will be added.

Contributors

  • Contributors are listed (@hyehwalee). Good — credit is given.

Screenshots/Videos

  • Marked N/A with reasoning (behavioral change). Acceptable.

Summary Table

Section Status Recommendation
Title None required.
Commit Type None required.
Risk Level ⚠️ Consider updating to risk:medium or add explicit migration/compatibility justification in the PR body.
What & Why Add a short note about effect on existing persisted manifests.
Impact of Change Clarify whether any migration or compatibility behavior is expected for existing workflows.
Test Plan ⚠️ Add unit tests for the dependency-building/clearing logic or justify why manual testing suffices; if manual testing was done, check the corresponding checkbox.
Contributors None required.
Screenshots/Videos None required.

Final Notes

  • Pass: This PR passes the PR title/body template checks.
  • Advised risk: I recommend risk:medium (higher than your risk:low) because these changes modify manifest defaults and UI-driven parameter population/clearing behavior which can affect persisted manifests and agent behavior. Please either update the PR label/body to reflect medium risk or add a short justification for why low risk is accurate (e.g., server-side migration/backwards compatibility is guaranteed).
  • Tests: Please add unit tests covering the new/changed logic (clearing and population of deploymentModelProperties), or add a clear justification in the Test Plan for why only manual testing is appropriate and when automated tests will be added.

Please update the PR title/body with the recommendations above (risk label or justification, small note about existing manifest compatibility, and test additions/justification). Thank you for the clear description and code comments — this made review straightforward!


Last updated: Sun, 05 Apr 2026 02:37:17 GMT

@Elaina-Lee Elaina-Lee added risk:low Low risk change with minimal impact and removed needs-pr-update labels Apr 5, 2026
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

This PR updates the agent-loop authoring experience to avoid populating deploymentModelProperties for AzureOpenAI (where the backend can derive model info from the deployment) and to remove stale hardcoded defaults from the agent loop manifest.

Changes:

  • Removed hardcoded defaults for deploymentModelProperties.{name,format,version} and restricted their visibility to agentModelType === MicrosoftFoundry in the agent loop manifest.
  • Added dependency updates to clear deploymentModelProperties when switching agentModelType away from MicrosoftFoundry.
  • Updated the deploymentId change handler to only populate deploymentModelProperties for MicrosoftFoundry in both Designer and Designer V2.

Reviewed changes

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

File Description
libs/logic-apps-shared/src/designer-client-services/lib/standard/manifest/agentloop.ts Removes stale defaults and limits visibility of deploymentModelProperties fields to MicrosoftFoundry.
libs/designer/src/lib/ui/panel/nodeDetailsPanel/tabs/parametersTab/index.tsx Clears deploymentModelProperties when leaving Foundry and conditionally populates them only for Foundry.
libs/designer-v2/src/lib/ui/panel/nodeDetailsPanel/tabs/parametersTab/index.tsx Same conditional populate + clearing behavior as Designer, for the v2 panel.

Comment on lines +1071 to +1072
const config = AGENT_MODEL_CONFIG[selectedModelId];
const modelName = selectedModelId;
Copy link

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

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

In the Foundry path, selectedModelId here is coming from the deploymentId combobox option value (currently value: deployment.name in getEditorAndOptions), so it’s a deployment name, not the model id (deployment.properties.model.name). Using it as the key for AGENT_MODEL_CONFIG will usually miss and also sets deploymentModelProperties.name to the deployment id instead of the model name. Consider restoring a deployment lookup (deploymentsForCognitiveServiceAccount.find(d => d.name === selectedModelId)) and then using deploymentInfo.properties.model.name for modelName / AGENT_MODEL_CONFIG lookup (or alternatively change the combobox option values for MicrosoftFoundry to emit the model id).

Suggested change
const config = AGENT_MODEL_CONFIG[selectedModelId];
const modelName = selectedModelId;
const deploymentInfo = deploymentsForCognitiveServiceAccount.find((deployment) => deployment.name === selectedModelId);
const modelName = deploymentInfo?.properties?.model?.name ?? selectedModelId;
const config = AGENT_MODEL_CONFIG[modelName];

Copilot uses AI. Check for mistakes.
Comment on lines +1085 to +1089
if (currentModelType === 'MicrosoftFoundry' && selectedModelId) {
const config = AGENT_MODEL_CONFIG[selectedModelId];
const modelName = selectedModelId;
const modelFormat = config?.format ?? 'OpenAI';
const modelVersion = config?.version;
Copy link

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

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

selectedModelId originates from the deploymentId combobox options (currently value: deployment.name), so for Foundry it is a deployment name rather than the model id used as keys in AGENT_MODEL_CONFIG. This will cause deploymentModelProperties.name to be set incorrectly and the config lookup to fail. Suggest mapping deployment name -> deployment.properties.model.name before looking up AGENT_MODEL_CONFIG (or change the combobox option values for MicrosoftFoundry to return the model id).

Copilot uses AI. Check for mistakes.
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 5, 2026

📊 Coverage Check

No source files changed in this PR.

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.

2 participants