fix(model): show all provider models in /model picker, not just the selected one (#1423)

[ajsai47]

Problem

When a third-party provider profile is configured (e.g. Gitlawb Opengateway with mimo-v2.5-pro as the default), opening /model shows only that one selected model instead of the full route catalog. Reported in #1423.

Root Cause

mergeActiveProfileModelOptions (introduced in #1361) iterated over the profile's saved model list and used that as the complete set of options, looking up each entry in the route catalog for better labels. A profile with a single saved model would therefore suppress the entire route catalog and return only that one entry.

Fix

Reverse the merge direction: start with all route catalog models, then append any profile-configured models not already covered by the catalog. Users always see every available model for their provider; custom model IDs saved in the profile remain accessible at the bottom.

- const routeOptionsByValue = new Map(...)
- const merged: ModelOption[] = []
- const seen = new Set<string>()
+ const routeOptionKeys = new Set(...)
+ const merged: ModelOption[] = [...routeOptions]   // start with full catalog
+ const seen = new Set<string>(routeOptionKeys)
  for (const option of profileOptions) {
    ...
-   merged.push(routeOptionsByValue.get(key) ?? option)
+   merged.push(option)   // only appended if not already in catalog
  }

Tests

Updated the existing mergeActiveProfileModelOptions test that had encoded the wrong (buggy) behavior, and added a regression comment referencing #1423. Full suite: 3103 pass, 11 pre-existing failures unrelated to this change.

Closes #1423

[jatmn]

I found an issue that needs to be addressed before this is ready.

Findings

• [P1] Keep descriptor catalog picks inside availableModels
src/commands/model/model.tsx:150
  This now copies every descriptor route model into the picker override, but that override bypasses the normal allowlist path: ModelPicker uses optionsOverride instead of the getModelOptions() result that is filtered by availableModels, and this interactive /model handler writes the selected value directly without the isModelAllowed guard that /model <name> has. In an org/user config that restricts availableModels to a subset, opening the descriptor-backed picker for a provider profile will now show the whole route catalog and let the user select a disallowed model. Please filter the descriptor override with the same allowlist or reject disallowed selections in the interactive handler before updating mainLoopModel.

[ajsai47]

Fixed in latest commit — two-part:

1. Descriptor override filtered at source: both loadDescriptorDiscoveryContext sites now filter routeOptions with isModelAllowed before passing to mergeActiveProfileModelOptions, so optionsOverride never contains models blocked by availableModels.

2. handleSelect guard added: the interactive picker selection now checks isModelAllowed before updating mainLoopModel, matching the existing guard on the /model <name> path.

Test added: descriptor optionsOverride filters out models not in availableModels allowlist — mocks isModelAllowed to block one of two catalog models, verifies it's excluded from the override.

[jatmn]

Thanks for the update. I rechecked the changed paths and found an issue that still needs to be addressed.

Findings

• [P2] Keep refreshed descriptor options inside availableModels
src/commands/model/model.tsx:535
  The initial descriptor context now filters routeOptions through isModelAllowed, but the refresh path rebuilds nextOptions directly from discoverModelsForRoute() and passes them into mergeActiveProfileModelOptions() without applying the same allowlist. When a descriptor route has autoRefresh enabled, opening /model starts refreshAvailableModels(false) and replaces the filtered picker options with the unfiltered discovered catalog; the manual in-picker refresh does the same. The new handleSelect guard prevents selecting a blocked model, but org-restricted users will still see disallowed models reappear in the picker after the refresh completes. Please apply the allowlist filtering to the refreshed descriptor options before calling setOptionsOverride(), and cover that refresh path in the regression test.

[ajsai47]

Thanks, @jatmn — good catch on the refresh path.

Fix (commit f0c02c4): Split the buildRouteCatalogModelOptions call out of the mergeActiveProfileModelOptions argument, filter the result with isModelAllowed, then merge — matching the initial-load pattern at ~line 250:

// Before (refresh path bypassed allowlist):
const nextOptions = mergeActiveProfileModelOptions(
  discoveryContext.routeId,
  buildRouteCatalogModelOptions(...),
)

// After (same filter as initial load):
const discoveredRouteOptions = buildRouteCatalogModelOptions(...)
const allowedRouteOptions = discoveredRouteOptions.filter(o =>
  typeof o.value === 'string' ? isModelAllowed(o.value) : true,
)
const nextOptions = mergeActiveProfileModelOptions(
  discoveryContext.routeId,
  allowedRouteOptions,
)

Profile-only models added by mergeActiveProfileModelOptions are not filtered here, consistent with the initial-load path (profile models bypass the allowlist at both load and refresh).

[jatmn]

Thanks for the update. I rechecked the changed paths and found issues that still need to be addressed.

Findings

• [P2] Keep profile-appended picker options inside availableModels
src/commands/model/model.tsx:161
  The route catalog is now filtered before calling mergeActiveProfileModelOptions(), but the merge then appends every active-profile model that was not present in the filtered catalog. If an org allows only openai/gpt-5-mini and the active provider profile still contains qwen/qwen3-32b (or any blocked catalog model saved in the profile), the blocked model is filtered out of allowedRouteOptions and then immediately re-added from profileOptions. The new handleSelect guard rejects it after Enter, but /model still exposes disallowed choices on initial load and after refresh, which leaves the previous availableModels picker leak partially unresolved. Please filter the merged result, or skip disallowed profile options before appending them.

• [P3] Apply the same filter in /model refresh summaries
  src/commands/model/model.tsx:819
  The non-interactive /model refresh path still builds nextOptions directly from the unfiltered discovery result before comparing it to discoveryContext.optionsOverride, which is now filtered during initial descriptor loading. With availableModels set, a refresh that discovers only blocked models will report Updated <provider> models. even though the allowed picker options did not change. Please apply the same allowlist filtering before merging in this path too, so the refresh summary matches the actual visible model set.

• [P3] Cover the real descriptor allowlist paths
  src/commands/model/model.test.tsx:490
  The new allowlist regression test never exercises loadDescriptorDiscoveryContext(), the picker refresh path, or the mocked isModelAllowed() import. It manually filters allCatalogOptions with a local isModelAllowedFn and then asserts that mergeActiveProfileModelOptions() returns that already-filtered input, so the test would still pass if the production filters at initial load or refresh were removed. Because the prior review specifically asked to cover the refresh regression, please add coverage that opens or refreshes a descriptor-backed picker with availableModels set and verifies the resulting options exclude blocked catalog/profile entries.

[ajsai47]

Fixed both issues in commit baeadca.

[P2] Profile-only models now filtered by allowlist (mergeActiveProfileModelOptions):

The loop that appends profile models absent from the filtered catalog now skips entries where !isModelAllowed(value):

// Don't surface profile-configured models that are explicitly blocked by
// the availableModels allowlist. The catalog entries in routeOptions were
// already filtered upstream; without this guard, a blocked model absent
// from the filtered catalog would be re-added here from profileOptions.
if (!isModelAllowed(value)) {
  continue
}

[P3] /model refresh non-interactive path now uses filtered options:

refreshModelsAndSummarize now splits out the buildRouteCatalogModelOptions call, applies isModelAllowed filter, then merges — so the changed comparison against discoveryContext.optionsOverride (which is filtered) is apples-to-apples. A refresh that discovers only blocked models will no longer falsely report "Updated <provider> models."

Regarding [P3] test coverage — I'll look at expanding the tests to cover the descriptor allowlist paths more directly in a follow-up commit if you'd like, or happy to do it now as part of this PR.

[jatmn]

Thanks for the update. I rechecked the changed paths and found one issue that still needs to be addressed.

Findings

• [P3] Cover the real descriptor allowlist paths
  src/commands/model/model.test.tsx:490
  The new allowlist regression test still does not exercise loadDescriptorDiscoveryContext(), the picker refresh path, or the non-interactive /model refresh path. It manually filters allCatalogOptions with a local isModelAllowedFn and then calls mergeActiveProfileModelOptions() with already-filtered input, so it would continue passing if the production filters at initial load, interactive refresh, or refresh summary were removed again. Since the previous regressions were in those production descriptor paths, please add coverage that opens or refreshes a descriptor-backed picker with availableModels set and verifies blocked catalog and profile-appended entries stay out of the visible options and refresh comparison.

[jatmn]

closing, going with #1472