Skip to content

fix: skip missing and embedding models when auto-starting the selected model - #244

Open
Ayush7614 wants to merge 1 commit into
AtomicBot-ai:mainfrom
Ayush7614:fix/get-model-to-start-skip-missing
Open

fix: skip missing and embedding models when auto-starting the selected model#244
Ayush7614 wants to merge 1 commit into
AtomicBot-ai:mainfrom
Ayush7614:fix/get-model-to-start-skip-missing

Conversation

@Ayush7614

Copy link
Copy Markdown
Contributor

What

getModelToStart (used on app start and by ensureModelForServer, claude-code.tsx, hermes-agent.tsx) returned the selected model verbatim, ignoring the missing flag and the embedding model:

  • findFirstLocalModel and the last-used-model path both guard against broken-link (missing: true) and embedding models — loading them only crashes (see the comment in the same file).
  • The selected-model path checked neither, so a model whose weights file was deleted could be re-selected on restart and crash at load.

Fix

The selected-model path now skips a missing or embedding selected model and falls through to findFirstLocalModel, matching the guards used by the other two selection paths.

Verification

  • New test suite (web-app/src/utils/getModelToStart.test.ts, 9 tests) covering healthy/missing/embedding selections, no-usable-model, last-used preference, and last-used-missing fallback.
  • tsc -b clean, eslint 0 errors, all 9 tests pass.

…d model

getModelToStart's selected-model path returned the selection verbatim,
ignoring the missing flag and the embedding model, while both the
findFirstLocalModel fallback and the last-used-model path already guard
against them. A deleted-weights model could therefore be re-selected on
restart and crash at load.

The selected-model path now mirrors those guards and falls through to a
healthy local model. Adds a 9-case test suite.
@Ayush7614
Ayush7614 requested a review from Vect0rM as a code owner August 19, 2026 10:24

Vect0rM commented Aug 20, 2026

Copy link
Copy Markdown
Member

Thanks @Ayush7614 — the reasoning here is right. The selected-model branch was the one path that didn't check the model was actually loadable, so a broken-link selection could crash the auto-start while a perfectly good model sat one line below in findFirstLocalModel. Making the three paths agree is the correct fix.

The change needs a rebase before I can take it, and there are two collisions worth calling out explicitly, because resolving them casually would lose something.

1. main tightened the same line

Your diff replaces:

if (provider) {

but on current main that line reads:

// A stale persisted selection may point at a deactivated provider.
if (provider && provider.active !== false) {

The active check landed after you branched. The merged version needs both conditions:

if (
  provider &&
  provider.active !== false &&
  selectedModel.id !== EMBEDDING_MODEL_ID &&
  !(selectedModel as { missing?: boolean }).missing
) {

Dropping provider.active !== false would silently re-introduce auto-starting a provider the user disabled in settings — TurboQuant ships disabled on fresh installs, so it would be visible immediately.

2. The test file already exists

web-app/src/utils/getModelToStart.test.ts is on main with five cases covering the deactivated-provider behaviour. Git reports an add/add conflict, and taking your side of it would delete that coverage.

Could you fold your two new cases into the existing file instead? It has helpers that do what your local ones do:

const makeProvider = (name: string, modelIds: string[], active = true): ModelProvider => ...
const lookup = (providers: ModelProvider[]) => (name: string) => ...

makeProvider builds models as { id }, so a missing-weights model is { id: 'x', missing: true } — you can pass those through directly.

One simplification while you're there: the file runs under jsdom, which provides a real localStorage. The existing tests just call localStorage.clear() in beforeEach and localStorage.setItem(...) where they need a last-used model, so the vi.hoisted + vi.stubGlobal block isn't needed.

Nit

No trailing newline on the new file — prettier --write sorts it.

The two cases you added (missing-weights selection, embedding-model selection) are exactly the coverage that was absent, so please do keep them. Rebase, merge the guard rather than replacing it, fold the tests in, and I'll take it 🔌


Generated by Claude Code

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