fix: prevent BYOK model from conflicting with same-named Copilot model in picker

[rsd-darshan]

Problem

When a BYOK model and a native Copilot model share the same modelName, the model picker dropdown used getModelName() as the item ID. This caused both models to map to the same dropdown item, so selecting one would always activate the other.

Fix

Use CopilotModel.getModelKey() (which returns providerName + "_" + id for BYOK models, or just id for native models) as the dropdown item ID. This is already the map key used throughout ModelService, so no new ID format is introduced.

Changes

• ModelPickerGroupsBuilder: Use model.getModelKey() as the DropdownItem ID instead of model.getModelName()
• ModelService.setActiveModel: Accepts a model key and does a direct O(1) map lookup (simplified from the previous loop-based approach)
• ModelService.bindModelPicker: Uses activeModel.getModelKey() when syncing the picker selection
• ModelService.setFallBackModelAsActiveModel: Passes fallbackModel.getModelKey() to setActiveModel
• ModelService.findModelKeyByName: Used only by the custom-mode event handler (which receives a model name from the event payload). A TODO(#261) documents the residual same-name ambiguity in that path, which requires a protocol change to fix properly.

Notes

• No persistence migration needed — the previous code already wrote the composite key to UserPreference.setChatModel, so existing preferences remain valid.
• A unit test (ModelPickerGroupsBuilderTest) asserts that two models sharing a modelName produce DropdownItems with distinct IDs.

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Fixes model picker selection conflicts when a BYOK model shares the same display name as a Copilot-native model by introducing a unique picker identity and using it consistently across picker item creation, selection display, and model lookup.

Changes:

• Added ModelUtils.getPickerId() to generate unique IDs for picker items (BYOK: providerName_modelName, native: modelName).
• Updated model lookup in ModelService#setActiveModel to match by picker ID.
• Updated picker binding and dropdown item construction to use the new picker ID.

Reviewed changes

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

File	Description
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/utils/ModelUtils.java	Adds getPickerId() helper to generate unique picker IDs.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/ModelService.java	Uses picker ID for active model lookup and selected item display.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ModelPickerGroupsBuilder.java	Uses picker ID as dropdown item id to prevent collisions.

Comments suppressed due to low confidence (1)

com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/ModelService.java:1

• setActiveModel(String modelName) now effectively expects a picker ID (sometimes providerName_modelName) rather than a plain model name. This is a behavioral/API contract change that can break existing call sites that pass getModelName(). Consider (a) renaming the parameter/method to reflect the new meaning (e.g., setActiveModelByPickerId(String pickerId)), or (b) supporting both formats for backward compatibility (first match by picker ID, then fallback match by getModelName() when no picker-id match is found).

// Copyright (c) Microsoft Corporation.

[jdneo]

Hi @rsd-darshan, thank you for your contribution. Pls check the copilot review first :)

[rsd-darshan]

@jdneo addressed the Copilot review comments — switched to using the model's existing getModelKey() method for the picker ID, which avoids both the concatenation collision and the blank-provider-name edge case. Conversations resolved.

[jdneo]

@ethanyhou Currently we have fallback model related logics in ModelService. Is it still require after TBB rolls out?

[ethanyhou]

Review (Google CL guidelines)

Overall the fix is on the right track and clearly improves code health: getModelKey() already exists on CopilotModel and is the natural unique identifier for the picker — replacing getModelName() with it as the dropdown id is the correct, minimal fix for #167. Nice job reusing the existing helper rather than inventing a new ID format (the later refactor commit a8f7a38b is a good cleanup).

Blocking

1. customModeModelChangedEventHandler retains the same-name ambiguity the rest of the PR fixes (ModelService.java ~L137–155 + findModelKeyByName ~L334). The event payload is a model name, and findModelKeyByName returns the first map entry whose getModelName() matches — so if a BYOK model shares a name with a Copilot-native model (the exact scenario in #167), this path can still activate the wrong one. Two options, either is fine:

   • Document the limitation with a // TODO referencing a follow-up bug to extend the custom-mode protocol to carry the composite key (or provider + name), or
   • Disambiguate using modelFamily already parsed out of "<modelName> (<modelFamily>)" (currently the family is computed and discarded — see openParenIndex logic).
     Per the "no deferred cleanup without a filed bug" guideline, please at minimum file an issue and reference it in a code comment.

2. No tests. This is a regression-prone area (selection identity flowing through dropdown id ↔ map key ↔ persisted preference), and the bug it fixes is exactly the kind a unit test catches cheaply. ModelPickerGroupsBuilder.buildModelDropdownItems returns plain DropdownItems and should be testable in com.microsoft.copilot.eclipse.ui.test without SWT. Please add at least one test asserting that two CopilotModels sharing a modelName produce two DropdownItems with distinct ids. Bonus: a ModelService.setActiveModel test covering same-named BYOK + native models.

Non-blocking

3. Nit: PR description is stale. It still describes a ModelUtils.getPickerId() helper returning providerName_modelName, but the implemented code uses the existing CopilotModel.getModelKey() (which is providerName + "_" + id, not providerName + "_" + modelName). Please update the description before merge so reviewers and future archaeologists aren’t misled.

4. Nit: setActiveModel can be simplified now that lookup is O(1). The compositeKey/foundModel/model triad is leftover from the loop-based version:

   public void setActiveModel(String modelKey) {
     CopilotModel model = modelObservable.getValue().get(modelKey);
     if (model == null) {
       return;
     }
     UserPreference preference = getUserPreference();
     preference.setChatModel(modelKey);
     CompletableFuture.runAsync(this::persistUserPreference);
     ensureRealm(() -> activeModelObservable.setValue(model));
   }

5. FYI: No persistence migration is needed — the previous code already wrote the composite key (compositeKey = entry.getKey()) to UserPreference.setChatModel, so existing preferences remain valid. Worth noting explicitly in the commit message / PR description so reviewers don't have to verify it themselves.

6. Praise: Good instinct dropping the misleading comment // setActiveModel will be called after models are loaded — both old and new code look up against modelObservable.getValue() at call time, so the comment was incorrect.

Checklist

• Design sound, change belongs here
• Fixes the reported bug (unique picker IDs)
• Same-name ambiguity in custom-mode event path — see #1
• Tests — see #2
• No persistence migration needed
• Style/consistency OK

[rsd-darshan]

@ethanyhou thanks for the detailed review — addressed all points:

Blocking:

1. Added TODO(#261) to findModelKeyByName documenting the same-name ambiguity in the custom-mode event path. Filed customModeModelChangedEventHandler may activate wrong model when BYOK and native models share a name #261 to track the protocol-level fix.
2. Added ModelPickerGroupsBuilderTest with two tests: one asserting same-named BYOK + native models produce DropdownItems with distinct IDs, one asserting the ID is the model key not the model name.

Non-blocking:
3. Simplified setActiveModel to a clean O(1) direct map lookup — removed the leftover compositeKey/foundModel/model triad.
4. Updated the PR description to accurately reflect the implemented approach (no more stale getPickerId() reference).
5. No persistence migration note added to the PR description.

[jdneo]

@ethanyhou Currently we have fallback model related logics in ModelService. Is it still require after TBB rolls out?

@ethanyhou echo this again, please help confirm this.

[ethanyhou]

@jdneo Thanks for the reminder, fallback is not required after TBB.

[rsd-darshan]

Happy to remove the fallback model logic as part of this PR if that's preferred — just let me know.

[rsd-darshan]

Went ahead and removed the legacy TBB fallback logic — getFallbackModel, setFallBackModelAsActiveModel, the fallbackModel field in ModelService, and the 402 replay block in ChatContentViewer (both were already marked with TODO: Remove after TBB is officially released).