[Extensibility 1/4] refactor: table-driven LLM provider registry - #259
Draft
amal66 wants to merge 7 commits into
Draft
[Extensibility 1/4] refactor: table-driven LLM provider registry#259amal66 wants to merge 7 commits into
amal66 wants to merge 7 commits into
Conversation
This was referenced Jul 26, 2026
amal66
force-pushed
the
olp-pr/provider-registry
branch
from
July 26, 2026 14:33
db1bd15 to
32a5ff1
Compare
amal66
force-pushed
the
olp-pr/provider-registry
branch
from
August 3, 2026 01:56
32a5ff1 to
40d8d7d
Compare
Replace the per-provider if/else chains in lib/llm/index.ts and the env-var switch in lib/userApiKeys.ts with a provider registry (lib/llm/registry.ts) and an API-key provider table (core/apiKeyProviders.ts). Adding a provider is now a registerProvider()/registerApiKeyProvider() call — no edits to index.ts, models.ts, or userApiKeys.ts required. Ported from the amal66/mike monorepo fork (origin/main, b3166dd); mechanical translation into the backend/ layout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CEguyEgXa9JjCciXCcVemC
WHY THIS MATTERS
The LLMProviderAdapter interface asked every provider for its models
grouped into three usage tiers:
readonly models: {
readonly main: readonly string[];
readonly mid: readonly string[];
readonly low: readonly string[];
};
But nothing ever consumed that structure. The only reader,
allRegisteredModels(), immediately flattened it:
[...p.models.main, ...p.models.mid, ...p.models.low]
So the tier grouping was pure ceremony: it forced every adapter —
including third-party ones and test fixtures — to stub empty arrays
(`{ main: models, mid: [], low: [] }`) to satisfy a shape whose
distinctions were discarded on the very next line.
WHAT IS SPECULATIVE API SURFACE
Structure added because it "might be useful later" rather than because
a consumer needs it today. It is a real cost, not a free option: every
implementer must fill it in, every reader must wonder which tier
matters, and when a future need actually arrives it rarely matches the
guessed shape anyway. The YAGNI principle ("you aren't gonna need it")
says to add structure at the moment a consumer demands it — an
interface change is cheap when you control all the implementations.
HOW THE FIX WORKS
The adapter now declares one flat list:
readonly models: readonly string[];
- registry.ts: allRegisteredModels() iterates p.models directly.
- index.ts: each built-in registration spreads its tier constants into
one array, e.g. [...CLAUDE_MAIN_MODELS, ...CLAUDE_MID_MODELS,
...CLAUDE_LOW_MODELS]. Duplicates across tiers (claude-sonnet-4-6
appears in both main and mid) are harmless because the consumer
builds a Set.
- registry.test.ts: fixtures pass `models` directly, no empty-tier
stubs.
Tier information still exists where it is actually used: the exported
*_MAIN/_MID/_LOW_MODELS constants in models.ts, which the settings UI
and defaults consume. The registry only ever needed "which model IDs
exist", and now that is all it asks for.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WHY THIS MATTERS
Model→provider routing knowledge lived in THREE places at once:
1. Each adapter's matchesModel() in the registry (e.g. Claude's
`m.startsWith("claude")`).
2. A hard-coded prefix fallback inside providerForModel() that
re-implemented exactly those same heuristics:
if (model.startsWith("claude")) return "claude";
if (model.startsWith("gemini")) return "gemini";
if (model.startsWith("gpt-")) return "openai";
3. A static ALL_MODELS Set that resolveModel() consulted alongside the
registry's allRegisteredModels().
Duplicated knowledge drifts. If someone changed an adapter's
matchesModel() rule (say, to also claim "anthropic.claude-*" Bedrock
IDs), the fallback in models.ts would silently disagree, and which
answer you got would depend on whether index.ts happened to be loaded
first. That is exactly the class of bug this refactor PR exists to
prevent — extensibility is only safe when routing has ONE authority.
WHAT IS "SINGLE SOURCE OF TRUTH"
A design rule: every fact in a system should have exactly one
authoritative home, and everything else should derive from it. Here the
fact is "which provider serves model X", and its home is the provider
registry: adapters declare matchesModel() and their model lists, and
all other code asks the registry instead of keeping private copies.
The duplication existed only so llmModels.test.ts could import
models.ts without loading index.ts (which registers the built-in
providers). That is a test-structure problem, and the finding's
guidance is to fix it in the tests — never by duplicating production
routing rules.
HOW THE FIX WORKS
- providerForModel() now delegates solely to findProviderForModel();
no prefix fallback. Unknown models still throw the same
"Unknown model id" error.
- resolveModel() now validates solely against allRegisteredModels();
the static ALL_MODELS set is deleted.
- llmModels.test.ts imports from "../llm" (the package index) instead
of "../llm/models". Loading index.ts registers the built-in
providers, so the functions under test exercise the exact same
registry path production uses. Every existing assertion — including
"prefix-only inference without catalog validation", which now flows
through the adapters' matchesModel() — passes unchanged.
All production consumers (userSettings.ts, chat/streaming.ts,
routes/user.ts, routes/tabular.ts) already import from "lib/llm", so
registration is guaranteed to have run before any routing call.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WHY THIS MATTERS
On main, `Provider` was a closed union ("claude" | "gemini" | "openai"),
so TypeScript could prove a switch over providers handled every case.
This PR widens it to `string` to keep providers extensible — but that
silently disarms exhaustiveness checking. The first casualty was
providerLabel() in routes/tabular.ts:
function providerLabel(provider: Provider): string {
if (provider === "claude") return "Anthropic";
if (provider === "openai") return "OpenAI";
return "Gemini"; // <- claims EVERY other provider is Gemini
}
With the closed union, that final return really did mean "gemini" —
the compiler guaranteed no other value could reach it. With
`Provider = string`, register an "ollama" provider and the user sees
"Gemini API key is required to use llama-3..." — a plausible-looking
but wrong message that sends them to configure the wrong vendor's key.
WHAT IS EXHAUSTIVENESS CHECKING
With a closed union, TypeScript narrows the type in each branch; in a
final else the type collapses to the single remaining member (or
`never`, which the compiler flags if you add a variant and forget a
branch). Widening to `string` removes that safety net, so any
"remaining cases" fallthrough becomes a lie waiting to happen. When a
type must stay open, unknown values need honest handling — data-driven
lookup with an explicit fallback — instead of a hard-coded assumption.
HOW THE FIX WORKS
The display name becomes data the provider declares about itself, in
the one place that already knows every provider — the registry:
- LLMProviderAdapter gains `readonly label: string` ("Anthropic",
"Gemini", "OpenAI" for the built-ins). A third-party provider ships
its own label in the same registerProvider() call.
- registry.ts adds providerDisplayLabel(id): returns the registered
adapter's label, or the raw id for unknown providers — so a message
can at worst say `ollama API key is required...`, which names the
actual provider instead of blaming Gemini.
- routes/tabular.ts deletes its local providerLabel() and uses the
registry version. In practice providerForModel() throws before an
unregistered provider reaches this code, so the fallback is
defense-in-depth, not a reachable lie.
- The `Provider` doc comment now warns future readers not to write
catch-all branches over provider ids.
UserApiKeys' index signature was reviewed per the same finding and
deliberately kept: getUserApiKeys()/getUserApiKeyStatus() build their
maps dynamically from the registered-provider list, which requires an
open Record. Access flows through provider ids obtained from the
registries (not hand-typed literals), which is the mitigation the open
type needs.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WHY THIS MATTERS Two functions introduced by this PR had zero real consumers: 1. core/apiKeyProviders.ts isApiKeyProvider(value) — a boolean twin of normalizeApiKeyProvider(value), which already answers the same question (null means "not a provider") AND returns the validated id. Nothing in the codebase called it. 2. lib/llm/registry.ts registeredProviderIds() — consumed only by its own unit tests. A function whose only caller is its own test is not tested behavior, it is surface invented to have something to test. Dead exports are not free. Every public function is a promise: future maintainers must keep it working, wonder who depends on it, and search for callers before changing anything nearby. Two near-identical predicates (isApiKeyProvider vs normalizeApiKeyProvider) are also a drift hazard — if validation rules ever change in one and not the other, callers get different answers depending on which twin they happened to import. WHAT IS "DEAD SURFACE" (AND YAGNI) API surface is everything a module exports. Surface is "dead" when no production code consumes it — either literally uncalled, or called only by tests written for it. The YAGNI principle says: do not export speculative helpers; add them in the commit where a real caller appears. Deleting dead surface is cheap now and expensive later, when external code may have started depending on it. HOW THE FIX WORKS - isApiKeyProvider is deleted. Anyone needing a boolean writes `normalizeApiKeyProvider(v) !== null`, keeping one validation path. - registeredProviderIds() is deleted along with its describe block in registry.test.ts. The remaining tests all exercise API that production code actually uses (registerProvider, getRegisteredProvider, findProviderForModel, providerDisplayLabel, allRegisteredModels). Note the similarly named getRegisteredProviders() in apiKeyProviders.ts stays — it has a real consumer (userApiKeys.ts builds key/status maps from it). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WHY THIS MATTERS
The comment above the built-in provider registration claimed:
// Providers are imported above so that Vitest's vi.mock() hoisting
// works: test files mock e.g. "../claude" before this module loads,
// so the mocked function is captured here and ends up in the registry.
A search of the backend test suite shows NO test mocks "../claude",
"../gemini", "../openai", or "lib/llm" at all — route tests mock one
level higher ("../../lib/chat", "../../lib/userSettings"). The comment
described a constraint that does not exist.
Wrong comments are worse than no comments: a maintainer who trusts this
one would preserve the import structure for a mocking scheme nobody
uses, or — worse — "fix" a test by adding vi.mock("../claude") because
the comment implies that is the established pattern.
WHAT IS vi.mock HOISTING (what the comment misdescribed)
Vitest transforms each test file so vi.mock(path, factory) calls run
before the file's imports execute; any module importing that path then
receives the mock. That mechanism is real, but it only matters for
modules some test actually mocks — which is not the case for the
provider modules here.
HOW THE FIX WORKS
The comment now states the true and useful invariant: registration runs
at module load, so any code that imports "lib/llm" sees a fully
populated registry before its first dispatch or routing call. That is
the ordering guarantee resolveModel()/providerForModel() rely on since
routing became registry-only. No code change — comment only.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ation)
WHY THIS MATTERS
While this PR was in flight, main gained an Ollama provider whose model
list does not exist at build time: the backend discovers locally
installed models at runtime (GET /models/ollama) and any id of the form
"ollama/<tag>" is valid. Main expressed that in two hard-coded places:
// models.ts (main)
if (model.startsWith("ollama")) return "ollama"; // routing
if (ALL_MODELS.has(id) || id.startsWith("ollama/")) ... // validation
The rebase registered Ollama as a provider adapter, which cleanly
replaced the routing line — matchesModel() already covers "which
provider serves this id". But the validation line had no registry
equivalent: resolveModel() only accepted ids found in some adapter's
static `models` array, and Ollama's array is necessarily empty. Keeping
`id.startsWith("ollama/")` inside resolveModel() would have preserved
behavior while contradicting the whole point of this PR — models.ts
would once again hold provider-specific knowledge that drifts
independently of the adapter it describes.
WHAT IS A DYNAMIC MODEL ID
Most providers ship a fixed catalog: the adapter lists every model id it
serves, and validation is set membership. A dynamic provider (Ollama
locally, and later Bedrock/Azure-style gateways) serves ids that are
only knowable at runtime — whatever the user has pulled. For these,
"is this id valid?" is a predicate, not a set lookup. An interface that
only offers a static list forces dynamic providers to either lie
(claim nothing, breaking validation) or leak their predicate into core
files (what main did, acceptable only while providers were hard-coded).
HOW THE FIX WORKS
- LLMProviderAdapter gains an OPTIONAL hook:
isDynamicModel?(model: string): boolean
Static-catalog providers omit it; the Ollama registration implements
it as `(m) => m.startsWith("ollama/")` — the same rule main had, now
living next to the adapter it belongs to.
- registry.ts adds matchesDynamicModel(id): true when any registered
provider's isDynamicModel() claims the id.
- resolveModel() accepts an id when it is in allRegisteredModels() OR
matchesDynamicModel() claims it. No provider names appear in
models.ts.
- Tests: registry.test.ts covers the new hook (claimed and unclaimed
cases); llmModels.test.ts pins the observable behavior main
introduced — providerForModel("ollama/llama3") routes to "ollama"
and resolveModel() passes "ollama/<tag>" ids through.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
amal66
force-pushed
the
olp-pr/provider-registry
branch
from
August 6, 2026 03:45
454adca to
896ec3f
Compare
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
1 participant
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.
Summary
Replaces the per-provider
if/elsechains in the LLM dispatch layer and the per-providerswitchin the API-key lookup with a table-driven provider registry. Adding a provider (including OpenAI-compatible endpoints — the path endorsed in #20) becomes a singleregisterProvider()/registerApiKeyProvider()call, with no edits to call sites.Behavior is preserved: same three built-in providers, same env-var precedence, same model routing, same API-key status results. This is the base of a 4-PR extensibility series — demo mode, Ollama, and Vertex AI each become one self-contained file on top of it.
Changes
backend/src/lib/llm/registry.ts(new):LLMProviderAdaptercontract +registerProvider/findProviderForModel/ friends.backend/src/core/apiKeyProviders.ts(new): provider → env-var table replacing the hand-maintainedswitchandPROVIDERSarray.backend/src/lib/llm/index.ts,models.ts,userApiKeys.ts,types.ts: dispatch through the registry;Providerbecomes an open string id.registry.test.ts+models.test.ts(20 tests — registration, first-match-wins routing, model-set union, provider inference).Why
Today every new provider requires touching each dispatch site, the model tables, and the API-key switch. With the registry, local LLMs and other OpenAI-compatible endpoints plug in from a single setup file.
Testing
On this branch, on upstream's own harness:
npm testinbackend/— 20 files, 279 passed, 5 skipped.tscbuild clean.Provenance
Mechanical port of code running in amal66/mike (main). Fork-only features intertwined with this code (air-gap branches, retry/circuit-breaker wrapper) were omitted, not rewritten. Full hunk-by-hunk provenance in amal66#27.
🤖 Generated with Claude Code