From b962f99f3a9ddcaa150bff2d68d62bec3169c681 Mon Sep 17 00:00:00 2001 From: mlsmaycon Date: Mon, 24 Aug 2026 08:59:55 +0000 Subject: [PATCH] [management] Say what went wrong when loading a provider's models fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pressing "Load models from provider" against a key the vendor refuses answered "internal server error". Every outcome on that path is the operator's own key or upstream, so a 500 was wrong twice over: it told them the server broke, and it named nothing they could act on. The failures are already typed and already have sentences written for them — the save-time check translates the same set. Discovery now runs them through the same classifier, so the button reports a refused credential or an unreachable url in the words the form uses elsewhere. ErrNoDiscovery and ErrInvalidRequest pass through untouched. The handler maps both already, and a provider with no listing endpoint is a fact about the catalog entry rather than a failure — the caller falls back to the catalog's own models instead of showing an error at all. --- .../modules/agentnetwork/credentialcheck.go | 26 ++++++ .../agentnetwork/credentialcheck_test.go | 79 +++++++++++++++++++ .../internals/modules/agentnetwork/manager.go | 6 +- 3 files changed, 110 insertions(+), 1 deletion(-) diff --git a/management/internals/modules/agentnetwork/credentialcheck.go b/management/internals/modules/agentnetwork/credentialcheck.go index a49cc1d41..8306e1ee0 100644 --- a/management/internals/modules/agentnetwork/credentialcheck.go +++ b/management/internals/modules/agentnetwork/credentialcheck.go @@ -49,6 +49,32 @@ func (m *managerImpl) checkProviderCredential(ctx context.Context, provider *typ return status.Errorf(status.InvalidArgument, "%s", message) } +// discoveryFailure renders a failed model listing for the operator who pressed +// the button. Every outcome here is something they did or configured — a key +// the vendor refused, an upstream that does not answer — so it owes them the +// same sentence a refused save gives, not the generic 500 an unclassified +// error turns into. +// +// ErrNoDiscovery and ErrInvalidRequest pass through untouched: the handler +// already maps them, and "this provider has no listing endpoint" is a fact +// about the catalog rather than a failure to report as one. +func discoveryFailure(ctx context.Context, catalogID string, err error) error { + if errors.Is(err, modeldiscovery.ErrNoDiscovery) || errors.Is(err, modeldiscovery.ErrInvalidRequest) { + return err + } + + message, _ := credentialCheckFailure(err) + if message == "" { + return err + } + + // The operator's message carries no status code, so the vendor's number is + // recorded here or nowhere. + log.WithContext(ctx).Infof("agent network model discovery for %s failed: %v", catalogID, err) + + return status.Errorf(status.InvalidArgument, "%s", message) +} + // credentialCheckFailure renders a discovery failure as the sentence the // provider form shows, and reports whether it should block the write. // diff --git a/management/internals/modules/agentnetwork/credentialcheck_test.go b/management/internals/modules/agentnetwork/credentialcheck_test.go index 3ea85ba81..dc0cf133c 100644 --- a/management/internals/modules/agentnetwork/credentialcheck_test.go +++ b/management/internals/modules/agentnetwork/credentialcheck_test.go @@ -377,3 +377,82 @@ func TestCreateProvider_AProviderWeCannotCheckStillSaves(t *testing.T) { }) } } + +// TestDiscoveryFailure_TellsTheOperatorWhatWentWrong covers the button, not the +// save. Pressing "Load models from provider" against a bad key used to answer +// "internal server error", which names neither the thing that failed nor +// anything the operator could act on — every outcome here is their key or their +// URL. +func TestDiscoveryFailure_TellsTheOperatorWhatWentWrong(t *testing.T) { + cases := map[string]struct { + err error + want string + }{ + "refused credential": { + err: &modeldiscovery.VendorStatusError{Provider: "Bedrock", Status: 403}, + want: "the provider rejected the credential", + }, + "upstream that is not the api": { + err: &modeldiscovery.VendorStatusError{Provider: "OpenAI", Status: 404}, + want: "the upstream url did not answer a model listing", + }, + "upstream that does not resolve": { + err: &modeldiscovery.UnreachableError{ + Provider: "OpenAI", + Err: &net.DNSError{Err: "no such host", Name: "api.example.com", IsNotFound: true}, + }, + want: "the upstream url could not be reached: no such host", + }, + "vendor having a bad day": { + err: &modeldiscovery.VendorStatusError{Provider: "Anthropic", Status: 503}, + want: "the provider returned an error", + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + err := discoveryFailure(context.Background(), "openai_api", tc.err) + require.EqualError(t, err, tc.want) + + var sErr *status.Error + require.ErrorAs(t, err, &sErr) + require.Equal(t, status.InvalidArgument, sErr.Type(), + "a failure the operator caused must not read as a server fault") + }) + } +} + +// TestDiscoveryFailure_LeavesTheCatalogFactsAlone keeps the two outcomes the +// handler already maps. A provider with no listing endpoint is a fact about the +// catalog entry, and the caller falls back to the catalog's own models rather +// than showing an error at all — rewriting it as a refusal would turn a normal +// path into one. +func TestDiscoveryFailure_LeavesTheCatalogFactsAlone(t *testing.T) { + for name, err := range map[string]error{ + "no listing endpoint": modeldiscovery.ErrNoDiscovery, + "bad request": fmt.Errorf("%w: unknown catalog provider", modeldiscovery.ErrInvalidRequest), + } { + t.Run(name, func(t *testing.T) { + require.Equal(t, err, discoveryFailure(context.Background(), "openai_api", err), + "the handler's own mapping must still see the original error") + }) + } +} + +// TestDiscoverProviderModels_SurfacesTheVendorRefusal drives the manager rather +// than the classifier, so a future refactor that stops translating on this path +// fails here rather than silently going back to 500s. +func TestDiscoverProviderModels_SurfacesTheVendorRefusal(t *testing.T) { + ctx := context.Background() + f := newBootstrapFixture(t) + f.vendor.err = &modeldiscovery.VendorStatusError{Provider: "OpenAI", Status: 401} + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Create, true) + + _, err := f.manager.DiscoverProviderModels(ctx, "account1", "user1", modeldiscovery.Request{ + CatalogID: "openai_api", + UpstreamURL: "https://api.openai.com", + APIKey: "sk-wrong", + }, "") + + require.EqualError(t, err, "the provider rejected the credential") +} diff --git a/management/internals/modules/agentnetwork/manager.go b/management/internals/modules/agentnetwork/manager.go index 34e5c3220..fe646d555 100644 --- a/management/internals/modules/agentnetwork/manager.go +++ b/management/internals/modules/agentnetwork/manager.go @@ -231,7 +231,11 @@ func (m *managerImpl) DiscoverProviderModels(ctx context.Context, accountID, use req.APIKey = record.APIKey } - return m.modelDiscovery.Fetch(ctx, req) + models, err := m.modelDiscovery.Fetch(ctx, req) + if err != nil { + return nil, discoveryFailure(ctx, req.CatalogID, err) + } + return models, nil } // CreateProvider persists a new provider for the account. Providers have no