fix(models): reject unknown model aliases instead of assuming OpenAI - #84
Open
Agnik47 wants to merge 1 commit into
Open
fix(models): reject unknown model aliases instead of assuming OpenAI#84Agnik47 wants to merge 1 commit into
Agnik47 wants to merge 1 commit into
Conversation
`getModelConfig` ended with an unconditional fallback returning `provider: "openai"` with the caller's string as the model id. The orchestrator derives the judge *provider* from that return value (`orchestrator/index.ts:90-91`), so any alias not in `MODEL_CONFIGS` and not matching a known prefix was silently routed to OpenAI. `--judge sonnet-4-5` (the registry key is `sonnet-4.5`), `opus4.5` or `gemini2.5-pro` therefore became an OpenAI judge using the typo verbatim. That surfaces either as a 401 when `OPENAI_API_KEY` is unset — pointing at the wrong provider entirely — or a 404 for a model the user believes is Anthropic, and only in the evaluate phase, after a full ingest has been paid for. Neither message mentions that the alias was unrecognised. The prefix branches also tested the original casing while the registry lookup used `alias.toLowerCase()`, so `GPT-4.5` missed the registry *and* the `gpt-` branch and landed in the fallback with `supportsTemperature: true` — wrong for a reasoning model. Match on `lowerAlias` in every branch, and return the normalised id rather than the caller's spelling, since providers reject a mis-cased model id. Replace the fallback with an error naming the alias and listing `listAvailableModels()`: reaching it means the alias matched no registry entry and no provider prefix, so its provider genuinely cannot be inferred and guessing is what caused the misattribution. Also resolve the answering model alongside the judge at the top of `Orchestrator.run`. It is otherwise first resolved in the answer phase, so an unusable alias would still only surface after ingest. Fixes supermemoryai#73
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #73
The bug
getModelConfig(src/utils/models.ts) ended with an unconditional fallback:The orchestrator derives the judge provider from that return value:
So any alias absent from
MODEL_CONFIGSand not matching a known prefix was routed to OpenAI with the unrecognised string used verbatim as the model id.--judge sonnet-4-5(the registry key issonnet-4.5),opus4.5,gemini2.5-pro— all became OpenAI judges.The failure then lands as either a 401 when
OPENAI_API_KEYis unset, blaming a provider the user never selected, or a 404 for a model they believe is Anthropic. Both arrive in the evaluate phase, after a full ingest has been paid for, and neither message says the alias was unrecognised.Casing made it worse. The registry lookup used
alias.toLowerCase()but every prefix branch tested the original casing, soGPT-4.5missed the registry and thegpt-branch, landing in the fallback withsupportsTemperature: true— wrong for a reasoning model.The fix
lowerAliasin every prefix branch, so casing no longer decides whether a provider is recognised.GPT-4.5must go out asgpt-4.5.listAvailableModels(). Reaching that point means the alias matched no registry entry and no provider prefix (gpt-,o1/o3/o4,claude-,gemini-), so its provider genuinely cannot be inferred — guessing is precisely what produced the misattribution.The error is raised at
Orchestrator.runstartup, before any phase executes, so an unusable judge now fails in a second rather than after a full ingest.I also added one line resolving the answering model next to the judge. It is otherwise first resolved inside the answer phase (
phases/answer.ts:28), so a typo there would still have cost a full ingest before surfacing. This is slightly beyond the reported issue but it is the same bug on the same code path, and leaving it would have made the fix only half-effective.A note on the throw
This turns a silent misroute into a hard failure, so it is worth being explicit about the blast radius. The prefix branches already cover every model family the repo supports, and all call sites are internal (
judges/{openai,anthropic,google}.ts,phases/answer.ts,orchestrator/index.ts) — none passes arbitrary user input anywhere except the--judge/--answering-modelCLI flags and the equivalent API fields, which is exactly where failing fast is wanted. A genuinely new model from a supported provider still resolves through its prefix; only strings with no inferable provider now throw.Verification
bun test— 19 new tests insrc/utils/models.test.tspass.main, 10 of the 19 fail (registry-casing, prefix-casing, id normalisation, and every unrecognised-alias case); they fail cleanly rather than hanging.gemini-3vsgemini-temperature defaults, o-series reasoning params, mixed-case inference, id normalisation, six unrecognised aliases including empty and whitespace, and a guard that every registered alias still resolves.tsc --noEmitclean for both changed files.src/utils/models.tsandsrc/orchestrator/index.tsalready failprettier --checkonmain; I left that alone and confirmed every line I added is within the configuredprintWidth.