fix: restore provider registry after cf5a430d2 emptied the default#3196
fix: restore provider registry after cf5a430d2 emptied the default#3196dgageot wants to merge 1 commit into
Conversation
RAG toolsets, the eval judge, and the js/wasm build all broke with 'unknown provider type' because the default registry is now empty. Thread the resolved registry through RuntimeConfig and repopulate the wasm-safe provider set directly in factory_js.go. Assisted-By: Claude
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The three broken provider-registry paths are correctly restored. The nil-safe ProviderRegistryOrDefault() getter, the new ProviderRegistry field on config.RuntimeConfig, and the teamloader.LoadWithConfig wiring all look sound.
Key areas checked:
teamloader.LoadWithConfigrecursive safety:loadExternalAgentexplicitly forwards the parent's registry viaWithProviderRegistry(loadOpts.providerRegistry), so recursive loads for external agent refs correctly propagate the full registry — no overwrite with the empty default.- js/wasm
factory_js.go: The missingvertexai.IsModelGardenConfigbranch is intentional and documented; Vertex AI cloud SDKs don't compile to wasm, and the comment in the new file makes this explicit. - Eval judge path (
cmd/root/eval.go):ProviderRegistryis set beforecreateJudgeModelis invoked — ordering is correct. - RAG builder/strategy: Both now consume
runConfig.ProviderRegistryOrDefault(), which falls back toDefaultRegistry()(populated in wasm, empty in non-wasm — butLoadWithConfigalways sets it before RAG toolsets are created).
No bugs found in the changed code.
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
This PR correctly restores provider registry wiring for three broken code paths (RAG builder/strategy, eval judge, js/wasm build). The structural fix is sound and all current callers are wired correctly via loaderdefaults.Opts().
Three related medium-severity concerns were identified around the empty-registry fallback behavior: ProviderRegistryOrDefault() silently falls back to an empty DefaultRegistry() in non-js builds, and teamloader.LoadWithConfig initializes loadOpts.providerRegistry to that same empty registry before callers can override it. No current production path triggers these issues, but they are latent traps for future callers.
| // without it, model creation fails with "unknown provider type". | ||
| func (c ManagersBuildConfig) NewProvider(ctx context.Context, cfg *latest.ModelConfig) (provider.Provider, error) { | ||
| return provider.New(ctx, cfg, c.Env, | ||
| return c.RuntimeConfig.ProviderRegistryOrDefault().New(ctx, cfg, c.Env, |
There was a problem hiding this comment.
[MEDIUM/LIKELY] Silent fallback to empty provider registry when RuntimeConfig.ProviderRegistry is unset
c.RuntimeConfig.ProviderRegistryOrDefault() falls back to provider.DefaultRegistry() when RuntimeConfig is nil or its ProviderRegistry field is not set. In non-js builds, provider.DefaultRegistry() returns an empty registry (no factories registered), so any call to NewProvider in that state will produce "unknown provider type" errors — exactly the runtime failure this PR is fixing elsewhere.
All current callers go through loaderdefaults.Opts() (which injects providers.NewDefaultRegistry()), so the happy path is fine. However, the fallback provider.DefaultRegistry() returns an empty registry in non-js builds, so any future call site that constructs ManagersBuildConfig without going through teamloader.LoadWithConfig + loaderdefaults.Opts() will fail silently.
Consider logging a warning or returning an explicit error if RuntimeConfig is nil, rather than silently falling back to an unusable registry.
| // without it, model creation fails with "unknown provider type". | ||
| func (c BuildContext) NewProvider(ctx context.Context, cfg *latest.ModelConfig) (provider.Provider, error) { | ||
| return provider.New(ctx, cfg, c.Env, | ||
| return c.RuntimeConfig.ProviderRegistryOrDefault().New(ctx, cfg, c.Env, |
There was a problem hiding this comment.
[MEDIUM/LIKELY] Same silent empty-registry fallback as in pkg/rag/builder.go
c.RuntimeConfig.ProviderRegistryOrDefault() falls back to provider.DefaultRegistry() (empty in non-js builds) when RuntimeConfig is nil or ProviderRegistry is unset. The same analysis applies as the parallel issue in pkg/rag/builder.go: all current production paths are safe because all callers go through loaderdefaults.Opts(), but any future caller that constructs BuildContext without a populated RuntimeConfig will silently receive a broken provider registry and fail at model creation time with "unknown provider type".
| runConfig.Providers = cfg.Providers | ||
| // Share the resolved provider registry so toolsets that build providers at | ||
| // load time (e.g. RAG embeddings/reranking) use the same one as agent models. | ||
| runConfig.ProviderRegistry = loadOpts.providerRegistry |
There was a problem hiding this comment.
[MEDIUM/LIKELY] loadOpts.providerRegistry may be empty when WithProviderRegistry is not passed; callers get a non-nil but unusable registry
loadOpts.providerRegistry is initialized to provider.DefaultRegistry() (an empty registry in non-js builds) before opts are applied. runConfig.ProviderRegistry is then set to this value. Because ProviderRegistryOrDefault() checks only != nil, a caller that omits WithProviderRegistry(...) will silently receive an empty (non-nil) registry and fail at RAG model creation time with "unknown provider type" — the same failure this PR intends to fix.
All current callers pass loaderdefaults.Opts() (which includes WithProviderRegistry(providers.NewDefaultRegistry())), so this is safe today. However the default is a footgun: consider initializing loadOpts.providerRegistry to nil instead of provider.DefaultRegistry(), so that ProviderRegistryOrDefault() can provide a meaningful fallback, or enforce that callers must always pass WithProviderRegistry.
Commit cf5a430 (
refactor: make toolsets and providers explicit) emptiedprovider.DefaultRegistry()and required callers to pass the full provider set explicitly viaproviders.NewDefaultRegistry(). The main agent model loading and runtime model switcher were updated at that time, but three code paths still fell through to the now-empty default registry and failed at runtime withunknown provider type "openai".This commit restores the three broken paths. RAG toolsets now receive the resolved provider registry via a new
ProviderRegistryfield onconfig.RuntimeConfig(with a nil-safeProviderRegistryOrDefault()getter);teamloader.LoadWithConfigpopulates it, andpkg/rag/builder.goandpkg/rag/strategy/strategy.goconsume it instead of the empty default. The js/wasm build gets its own restoreddefaultFactoriesinpkg/model/provider/factory_js.go(openai, anthropic, google) becausepkg/model/provider/providerscannot compile to wasm — it pulls in cloud SDKs that have no wasm-safe build. The eval judge inpkg/evaluation/eval.gopreviously called the empty default directly;cmd/root/eval.gonow setsrunConfig.ProviderRegistry = providers.NewDefaultRegistry()andcreateJudgeModelusesrunConfig.ProviderRegistryOrDefault().No behavior changes outside these three error paths. The fix is self-contained and all existing tests continue to pass.