-
Notifications
You must be signed in to change notification settings - Fork 395
fix: restore provider registry after cf5a430d2 emptied the default #3196
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,9 +26,11 @@ type BuildContext struct { | |
| } | ||
|
|
||
| // NewProvider creates a model provider using the build context's environment, | ||
| // gateway, and custom provider settings. | ||
| // gateway, and custom provider settings. It uses the provider registry carried | ||
| // by RuntimeConfig (populated by the team loader with the full provider set); | ||
| // 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, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [MEDIUM/LIKELY] Same silent empty-registry fallback as in
|
||
| options.WithGateway(c.ModelsGateway), | ||
| options.WithProviders(c.Providers)) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -153,6 +153,9 @@ func LoadWithConfig(ctx context.Context, agentSource config.Source, runConfig *c | |
| // Make model definitions available to toolset creators (e.g., RAG reranking) | ||
| runConfig.Models = cfg.Models | ||
| 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [MEDIUM/LIKELY]
All current callers pass |
||
|
|
||
| // Load agents | ||
| parentDir := cmp.Or(agentSource.ParentDir(), runConfig.WorkingDir) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[MEDIUM/LIKELY] Silent fallback to empty provider registry when
RuntimeConfig.ProviderRegistryis unsetc.RuntimeConfig.ProviderRegistryOrDefault()falls back toprovider.DefaultRegistry()whenRuntimeConfigis nil or itsProviderRegistryfield is not set. In non-js builds,provider.DefaultRegistry()returns an empty registry (no factories registered), so any call toNewProviderin 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 injectsproviders.NewDefaultRegistry()), so the happy path is fine. However, the fallbackprovider.DefaultRegistry()returns an empty registry in non-js builds, so any future call site that constructsManagersBuildConfigwithout going throughteamloader.LoadWithConfig+loaderdefaults.Opts()will fail silently.Consider logging a warning or returning an explicit error if
RuntimeConfigis nil, rather than silently falling back to an unusable registry.