Skip to content

Fix duplicate models across named custom providers#1874

Closed
hacker1e7 wants to merge 6 commits into
nesquena:masterfrom
hacker1e7:fix/default-model-slash-id-collisions
Closed

Fix duplicate models across named custom providers#1874
hacker1e7 wants to merge 6 commits into
nesquena:masterfrom
hacker1e7:fix/default-model-slash-id-collisions

Conversation

@hacker1e7
Copy link
Copy Markdown
Contributor

Summary

Fix /api/models so duplicate model IDs from different named custom providers are preserved instead of being dropped by an overly broad global de-duplication pass.

Root cause

While assembling named custom provider groups in get_available_models(), the API used a single global _seen_custom_ids set seeded from auto-detected models. That meant if two named custom providers exposed the same raw model ID (for example both exposing gpt-5.4), the first provider to be processed "claimed" the ID and later providers silently lost their copy before provider-aware namespacing/deduplication ran.

In practice this caused configs like:

  • edith -> gpt-5.4
  • super-javis -> gpt-5.4, gpt-5.5

...to incorrectly omit super-javis's gpt-5.4 entry from /api/models.

Fix

Replace the single global custom-provider seen set with a per rendered provider-group seen set:

  • unnamed / auto-detected custom entries still dedupe against the __unnamed__ bucket
  • each named custom provider slug gets its own bucket
  • later provider-aware duplicate handling (_deduplicate_model_ids()) remains responsible for cross-provider collisions and namespacing behavior

This keeps de-duplication scoped to the place where it is actually valid: inside the same rendered provider group.

Why this is the correct behavior

Provider identity matters. Two providers can legitimately expose the same upstream model ID while still being different choices (different base URLs, auth, routing, latency, quotas, or semantics). The API should preserve both entries and only disambiguate them in a provider-aware way, not erase one globally just because the bare model ID matches.

Regression coverage

Added tests/test_custom_provider_duplicate_models.py, which verifies that:

  1. two named custom providers can both expose gpt-5.4
  2. the first provider keeps its plain ID entry
  3. the second provider is still present and survives provider-aware deduplication as @custom:super-javis:gpt-5.4
  4. non-colliding sibling models like gpt-5.5 are still returned normally

Testing

pytest tests/test_custom_provider_duplicate_models.py

…ontend fallback

Backend (_build_configured_model_badges): skip badge assignment for
synthetic candidate keys like 'custom:provider/model' that aren't real
dropdown entries, preventing phantom badge keys that normalize to the same
string as bare model IDs from other providers.

Frontend (_getConfiguredModelBadge): when a providerId is specified and
no provider-specific badge match is found, return null instead of falling
back to a badge from another provider.

Together these fixes ensure that when 'custom:super-javis' is the active
provider with 'gpt-5.4' as the default model, only super-javis's own
'@Custom:super-javis:gpt-5.4' carries the PRIMARY badge, while Edith's
bare 'gpt-5.4' is correctly left unadorned.
@nesquena-hermes
Copy link
Copy Markdown
Collaborator

Summary

Investigated against origin/master and read the full PR series (4 commits). The fix is correct in concept and the per-group dedup bucket is the right scoping change. Two concerns worth surfacing before merge: a subtle behavior change to _deduplicate_model_ids() for slash-qualified IDs, and a lingering _named_custom_groups ordering issue. CI status currently shows no checks reported.

Code reference

The core fix in api/config.py:1864-1873:

_seen_custom_ids_by_group: dict[str, set[str]] = {
    "__unnamed__": {m["id"] for m in auto_detected_models}
}
for _cp in _custom_providers_cfg:
    ...
    _bucket_key = _slug or "__unnamed__"
    _bucket_seen = _seen_custom_ids_by_group.setdefault(_bucket_key, set())

That's precisely the right scoping — same-bucket dedup (within one named provider, or within the unnamed bucket) is preserved, cross-bucket de-dupe is correctly delegated to the post-process at api/config.py:880-944.

Concern 1: dedup now treats slash IDs as collidable

Before this PR, _deduplicate_model_ids() skipped slash IDs entirely:

# Before
if mid.startswith("@") or "/" in mid:
    continue

After:

# After (this PR)
if not mid or mid.startswith("@"):
    continue

This is intentional — the new test test_two_providers_same_slash_qualified_model_prefixes_second asserts that two providers exposing google/gemma-4-27b should now produce google/gemma-4-27b (Alpha) and @custom:beta:google/gemma-4-27b (Beta). Reasonable.

But this is a silent semantic change for the provider/model form — namely the provider's canonical IDs like anthropic/claude-sonnet-4.6 and openai/gpt-5.4. Today those exist on _PROVIDER_MODELS, so when a user has both Anthropic and a custom provider also exposing anthropic/claude-sonnet-4.6 (e.g. via OpenRouter or an aggregator gateway), the second occurrence will now get prefixed @<pid>:anthropic/claude-sonnet-4.6 where it previously stayed as the slash form.

That probably isn't wrong, but I'd want to confirm it doesn't break sessions with persisted bare slash IDs that were previously matched by the second provider's group. The "first occurrence wins" rule guarantees the first group's bare slash ID stays intact, but if the user's saved session model belongs to the second provider that group's option-id has now changed.

Consider documenting this as an intentional behavior change in CHANGELOG / migration notes, even if it's mostly invisible. The frontend rehydration via _findModelInDropdown(modelId, sel, preferredProviderId) should cover most legacy sessions, but worth a smoke test.

Concern 2: _named_custom_groups add-then-skip ordering

Looking at the surrounding loop at api/config.py:2843-2849 on master:

_cp_name = (_cp.get("name") or "").strip()
_slug = _custom_provider_slug_from_name(_cp_name) if _cp_name else None
if _slug and _slug not in _named_custom_groups:
    _named_custom_groups[_slug] = (_cp_name, [])

This pre-creates an empty group entry before the model-collection loop runs. If every model id in this provider was already seen in the bucket (or the bucket bug previously dropped them), the group ends up empty and gets dropped at api/config.py:2956 (if _nc_models: groups.append(...)). Per-bucket sets fix the dropping case correctly, but the pre-create + later "if not _nc_models" fallback to auto_detected_models_by_provider.get(pid, []) at api/config.py:2944-2952 is now harder to reason about for named groups whose own set legitimately deduped the model. Not a blocker, but the comment block ("issue #1619 / Opus advisor on stage-295") needs an update to reflect the bucket scoping.

Cross-repo and frontend

The frontend bridge in static/ui.js:455-465 reads optgroup.dataset.provider:

function _getOptionProviderId(opt){
  if(!opt) return '';
  const group=opt.parentElement;
  if(group && group.tagName==='OPTGROUP' && group.dataset && group.dataset.provider){
    return group.dataset.provider;
  }
  ...
}

…and the optgroup is populated with og.dataset.provider = g.provider_id at static/ui.js:626. That dataset attribute is the load-bearing piece — without it, _findModelInDropdown falls back to slicing @provider: out of the value, which only works for already-prefixed IDs. Worth a comment on og.dataset.provider = g.provider_id to flag it as the rehydration anchor.

The _getConfiguredModelBadge(modelId, badgeMap, providerId) change in static/ui.js:431-450 correctly returns null when a provider was specified but no badge matches that provider, instead of silently picking matches[0]. That's the right way to prevent the PRIMARY badge leaking across providers — Edith's bare gpt-5.4 no longer steals super-javis's PRIMARY.

Test coverage

The three new test functions cover the right surfaces:

  • test_named_custom_providers_keep_duplicate_model_ids — full integration, confirms super-javis keeps gpt-5.4 after dedup runs.
  • test_two_providers_same_slash_qualified_model_prefixes_second — locks down the slash-ID dedup behavior change.
  • test_duplicate_slash_id_primary_badge_sticks_to_matching_provider_only — exec()-based scope re-creation to test _build_configured_model_badges in isolation.

The third one is clever but brittle — exec'ing a slice of api/config.py source with a hand-rolled _norm_model_id shim means a refactor of the function's signature or inline helpers will silently fail to load. Consider extracting _build_configured_model_badges to a top-level function so the test can import it directly.

CI hasn't reported yet on this PR (statusCheckRollup empty); worth waiting for green before merge.

LGTM with the two notes above — the underlying fix is correct and the per-bucket scoping is the right architectural call.

hacker1e7 added 2 commits May 8, 2026 18:35
_deduplicate_model_ids was keeping the alphabetically-first provider's
bare IDs. When the active provider was different, selecting a bare model
from a non-active provider would silently route through the active
provider instead.

Fix: the active provider now gets first-occurrence priority for bare
model IDs. All overlapping non-active providers get @provider_id:
prefixes, so selecting them routes through the correct provider.
Three fixes in this commit:

1. resolve_model_provider(): Replace greedy first-match on custom_providers
   with a multi-pass collector. When a bare model ID exists in multiple
   custom providers, prefer the active provider; when there's exactly one
   match, verify the active provider doesn't also have it before routing.

2. _build_configured_model_badges(): Remove fallback to exact_match and
   matches[0] when provider_match fails — they may belong to a different
   provider and would leak PRIMARY badges.

3. Guard the final badges[match_id] assignment with a provider identity
   check as defense-in-depth.

Add _provider_has_model() helper to check whether a provider's catalogue
includes a given model ID, supporting both custom and built-in providers.
@nesquena-hermes
Copy link
Copy Markdown
Collaborator

Thanks @hacker1e7 — defer-flagging this PR for the next pass. The per-group dedup bucket fix is the right shape and the per-named-provider seen-set is exactly the right scoping change. Two concrete things to address before merge, plus the rebase.

Blocker 1: Behavior change to _deduplicate_model_ids() for slash-qualified model IDs

Before this PR, slash-qualified model IDs were excluded from collision tracking entirely:

# api/config.py — pre-PR
if mid.startswith("@") or "/" in mid:
    continue

After this PR, they're now included:

# api/config.py — this PR
if not mid or mid.startswith("@"):
    continue

That's intentional — your new test test_two_providers_same_slash_qualified_model_prefixes_second asserts two providers exposing google/gemma-4-27b should now produce google/gemma-4-27b (Alpha) and @custom:beta:google/gemma-4-27b (Beta).

The concern: this changes the dedup behavior for every existing user who has slash-qualified IDs from multiple providers (OpenRouter anthropic/claude-3.5-sonnet, etc.). Pre-PR, those would coexist as plain IDs in both groups. Post-PR, the second-listed provider gets @custom:<slug>: prefixed, which surfaces in the model picker UI and changes the IDs that downstream callers send to /api/chat/start.

What's needed before merge:

  1. Audit the slash-prefix dedup against existing OpenRouter / Mistral / together.ai configs. Specifically: does an existing user with two OpenRouter-shaped providers (e.g. one personal + one team account) suddenly see model IDs change in their model picker between versions? If so, that's a breaking change for stored model selections.

  2. Test for the no-collision case. Add a test asserting that when two providers expose different slash-qualified models (e.g. provider A: anthropic/claude-3.5, provider B: openai/gpt-4), neither gets prefixed — only true collisions are namespaced. The current test only covers the collision path.

  3. Document the change in the CHANGELOG entry. Anyone tracking custom-provider config from before this PR needs to know their stored model IDs may have shifted.

Blocker 2: _named_custom_groups ordering is still positional

The fix correctly scopes dedup per group, but the order in which groups are processed still determines which provider gets the plain ID and which gets @custom:<slug>: prefix. If custom_providers is reordered in config.yaml, the same model collision flips which one gets prefixed — and stored sessions that pinned gpt-5.4 to "the alpha provider" silently swap to the beta provider on next load.

Suggested: stable the ordering on config.yaml declaration order or alphabetical by slug, with a regression test pinning the order. This is small and fits in the same PR.

Mechanical: please rebase

The PR is currently CONFLICTING against origin/master. Please rebase onto current master and force-push — once the conflict resolves and the two concerns above are addressed, this is in good shape to land.

What's good as-is

The core scoping change in api/config.py:1864-1873:

_seen_custom_ids_by_group: dict[str, set[str]] = {
    "__unnamed__": {m["id"] for m in auto_detected_models}
}
for _cp in _custom_providers_cfg:
    ...
    _bucket_key = _slug or "__unnamed__"
    _bucket_seen = _seen_custom_ids_by_group.setdefault(_bucket_key, set())

is exactly right — same-bucket dedup is preserved, cross-bucket dedup is correctly delegated to _deduplicate_model_ids() at lines 880-944. The diagnosis was real and the fix shape is correct.

Tagging for re-review once the rebase + audit + ordering test land.

@nesquena-hermes
Copy link
Copy Markdown
Collaborator

Closing — superseded by #1947's narrower fix

Thanks @hacker1e7 — your diagnosis of the _seen_custom_ids global-bucket bug is correct, and the per-rendered-provider-group seen-set is the right scoping change. Re-reading the diff against current origin/master plus PR #1947 which landed yesterday with a 4-LOC version of the same fix:

Why I'm going with #1947's smaller version:

  1. The minimum-correct fix is a 4-line change in api/config.py — switch the dedup key from bare model id to f"{slug}:{model_id}". fix: show same model from different custom providers instead of deduplicating #1947 lands exactly that and nothing else.
  2. Your PR also touches _deduplicate_model_ids() behavior for slash-qualified IDs and adds a frontend _addLiveModelsToSelect change in static/ui.js — both are scope creep relative to the bug. Per the May 03 review I posted, the _deduplicate_model_ids shift is a behavior change (slash-qualified provider/model now survives where bare-id duplicates used to be removed) that needs its own justification + test, not a bundle.
  3. fix: show same model from different custom providers instead of deduplicating #1947 was simplified to backend-only after my review feedback — frontend dedup at _normId() was correctly identified as a separate concern that can be addressed independently if it actually surfaces.

Closing this PR but the analysis is preserved — the per-group bucket is the canonical fix, your PR description articulates the root cause clearly, and your three-named-provider regression test inspired the test I'm adding to #1947 before merge. Co-authored-by trailer on the #1947 merge commit attributes you alongside @happy5318.

Worth noting: if there's a real _deduplicate_model_ids() bug for slash-qualified IDs, please file it as a separate issue with a failing-test repro — happy to triage as its own PR.

nesquena-hermes added a commit that referenced this pull request May 9, 2026
CHANGELOG, ROADMAP, TESTING refresh for v0.51.31 stage release covering
12 contributor PRs:

Added (2 PRs):
- #1956 JKJameson — persistent composer draft (server-side, cross-client)
- #1957 hermes-gimmethebeans — configurable session TTL via env + settings

Fixed (10 PRs):
- #1939 ai-ag2026 — theme-color + sw cache regression coverage
- #1941 ai-ag2026 — preserve chat scroll across final render
- #1945 franksong2702 — localize session jump controls (#1938)
- #1947 happy5318 — show same model from different custom providers
  (Co-authored-by hacker1e7 for #1874 close)
- #1949 Sanjays2402 — close #1937 endless-scroll vs Start-jump race
  with generation-token + mutex
  (Co-authored-by franksong2702 + Michaelyklam)
- #1950 franksong2702 — mute stale stopped gateway heartbeat (#1944)
- #1951 amlyczz — gate goal hook on goal-related turns (#1932)
  (Co-authored-by franksong2702 for #1946 close)
- #1953 lucky-yonug — skip provider peel for custom host:port slugs
- #1960 Michaelyklam — translate hidden-files workspace label (#1841)
- #1961 sbe27 — respect image_input_mode (#1959)

Closed in favor of canonical: #1942, #1962, #1946, #1874, #1311.

Stage-326 hotfixes (per Opus advisor):
- CRITICAL #1951 PENDING_GOAL_CONTINUATION race fix (removed finally
  discard that race-erased the marker before consumer could read it)
- #1956 composer-draft input validation (50 KB text / 50 file clamp +
  type coercion to prevent unbounded session-JSON bloat)
- #1957 SESSION_TTL constant preserved as named fallback (existing
  regression tests pin it; #1957 originally deleted it)

Tests: 5006 → 5028 (+51 net new) — 0 regressions, 142.61s runtime.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants