Skip to content

fix(core): preserve fallback models after failed discovery - #3581

Open
mikemikimike wants to merge 2 commits into
apache:mainfrom
mikemikimike:codex/issue-3320-preserve-fallback
Open

fix(core): preserve fallback models after failed discovery#3581
mikemikimike wants to merge 2 commits into
apache:mainfrom
mikemikimike:codex/issue-3320-preserve-fallback

Conversation

@mikemikimike

Copy link
Copy Markdown
Contributor

Summary

Fixes the independent fallback-model half of #3320.

When model discovery has failed or has not run yet, the catalog can contain an empty models: [] array without a successful modelSource. The picker treated that empty array as authoritative and hid the static fallback catalog, even though the connection could still use the configured fallback model.

This change:

  • infers fallback, rather than fetched, for an empty model array when no source is persisted;
  • keeps fallback models visible for failed/pending discovery;
  • preserves an explicitly fetched empty inventory as authoritative;
  • adds regression coverage for both states.

The URL-normalization question in #3320 is intentionally out of scope.

Validation

  • npm --workspace @maka/core run typecheck
  • npm --workspace @maka/core run build
  • npm --workspace @maka/core run test:dist — 638 passed
  • npx biome check packages/core/src/model-catalog.ts packages/core/src/__tests__/model-catalog.test.ts
  • npx biome lint packages/core/src/model-catalog.ts packages/core/src/__tests__/model-catalog.test.ts
  • git diff --check

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Independent review at exact head 073a9cba387d22314b3446ca13bd218ff363672c.

APPROVE. The distinction this draws is the right one, and it is drawn in the only place that can carry it.

The bug was that an empty array is truthy, so input.modelSource ?? (liveModels ? 'fetched' : 'fallback') turned "discovery has not succeeded" into "the provider enumerated nothing", and the entire static catalog vanished from the picker while authorizeConnectionModel was still happily admitting those same models. The old comment on that line already claimed to use the raw modelSource — the code did not. This PR makes the code match the comment.

I walked the four reachable input shapes at this head rather than taking the tests' word for it:

models modelSource inventory source entries
undefined undefined absent static_catalog fallback catalog
[] undefined (failed/pending discovery) absent static_catalog fallback catalog — the fix
[] 'fetched' (authoritative empty) live provider_api empty; saved default annotated not_in_live_list
non-empty either per source per source live list

The pairing holds in every row: classifyConnectionModelInventory gates on models === undefined || modelSource === undefined → 'absent', so the fallback catalog can never be labelled provider_api. I specifically probed the shape that would break that — models: undefined with an explicit modelSource: 'fetched' — and it resolves to absent/static_catalog, so provenance stays honest there too.

Worth noting for the record: the defaulted local modelSource also feeds the staleness/annotation paths at model-catalog.ts:207,219,236, so models: [] with no source now reads fallback there where it previously read fetched. That is the same correction, applied consistently, not a side effect.

The two new tests pin behaviour rather than call shape, and the second one is the load-bearing half — without it, "always fall back on empty" would pass just as well and would silently resurrect models a provider has genuinely retired.

Verification: exact-head test is completed/success. Dependency audit is absent on this head by design — it is path-filtered to package.json / lockfile / audit scripts, neither of which this PR touches.

@mikemikimike
mikemikimike force-pushed the codex/issue-3320-preserve-fallback branch from b04169e to b2a895d Compare August 23, 2026 15:49
@mikemikimike
mikemikimike force-pushed the codex/issue-3320-preserve-fallback branch from b2a895d to cb37702 Compare August 23, 2026 16:18

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed on exact head cb3770233099d5ca454effed4ceb3b930964b895. No P0–P2 in the change itself. One pre-existing [P3] inline. Not approving yet, for a gating reason rather than a code reason — details below.

The fix is correct, and for the right reason. Both original sites treated an empty array as "the provider authoritatively answered: none":

  • the modelSource inference liveModels ? 'fetched' : 'fallback'[] is truthy, so "never looked" was scored as "looked and found nothing";
  • rawModels = liveModels ?? fallback?? only guards undefined, so [] displaced the entire static fallback catalog.

Introducing the "is there an authoritative source" dimension separates the three states cleanly: undefined (never queried) → fallback; [] without an explicit modelSource: 'fetched' (failed or pending discovery) → fallback, which is the bug being fixed; [] with explicit 'fetched' (provider genuinely has none) → stays empty.

I also confirmed classifyConnectionModelInventory still receives the raw input.modelSource rather than the inferred value — that is what the original comment there was protecting, and the change does not lose it.

The three new tests pin both branches of the new condition plus one public-path case, rather than only asserting shape.

On the red check — it is not this PR's fault, but it still blocks. The failing test comes from the branch being cut from an older main that contained a broken test: at the merge base, goal-services-adapter.test.ts:64/70/76 pass a type: 'sessions_changed' field that SessionChangedEvent does not have (its fields are reason/sessionId/modelId/turnId/ts), so that base does not compile. Current main has already dropped those type fields. This PR touches only model-catalog.ts and its test — verified via git diff --name-only merge-base..head.

So: no finding recorded for it, but an unrelated red is still not green, and the approval bar is terminal-green expected checks on the exact head. Merging current main into the branch should clear it, and I will re-check the terminal state after that.

liveModels !== undefined && (liveModels.length > 0 || modelSource === 'fetched')
? liveModels
: (input.fallbackModels ?? []).map((id) => ({
id,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P3] Pre-existing, not introduced here (anchored to this hunk because the affected line at :270 is unchanged and cannot take an inline comment) — but it sits directly adjacent to what this PR fixes, so it is worth naming while the area is being touched.

A connection that succeeded at discovery (modelSource === 'fetched') but whose models were all later quarantined ends up here with [] after the filter, still carrying 'fetched'. That takes the "authoritatively empty" branch, so the picker renders empty and no fallback catalog is offered — even though the static fallback may contain usable models.

Trigger → path → outcome: a connection works normally, every one of its model ids later lands in brokenModelIds, the user opens the model picker and sees nothing.

From the user's side this is indistinguishable from the failed-discovery case this PR is fixing, but only the latter is covered. Behavior is unchanged from before this PR (the old code also reduced to rawModels = liveModels = [] here), so this is not a regression — hence P3 rather than a blocker.

Smallest fix: check for emptiness after filtering — if the pre-filter list was non-empty and the post-filter list is empty, treat it as having no usable discovery result and fall back. A production-seam test would set models such that every id hits brokenModelIds with modelSource: 'fetched', and assert fallback catalog entries are still returned.

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