From 2d5e0dc05dbbbcc50c20a6e00d27adee4a893d64 Mon Sep 17 00:00:00 2001 From: yujiezhang-ops Date: Fri, 7 Aug 2026 14:52:43 +0800 Subject: [PATCH] fix: probe a Provider's reviewed chat model, not the first ID it lists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #93. With an empty probe model the connection test used the live /v1/models listing outright, so whatever an aggregator returned first became the model that got a chat payload. The catalogues of the Providers shipped by default are mostly generators: a t2v model rejecting a chat request produced a failure the user read as "my key is broken", during first-run setup. resolveProviderModel now prefers the Provider's manifest model when the endpoint actually serves it. That value is a reviewed, known-chat ID; the previous order reached it only when discovery failed, so the good value was bypassed exactly when the list was available to poison it. PickChatModel remains the last resort, for a custom endpoint or a Provider that no longer serves its manifest model. A model the user typed still wins over both: that field is their override, and a failure on it is the answer they asked for. nonChatModel gains video, audio and music terms — wan, kling, sora, veo, hailuo, seedance, cogvideox, t2v/i2v/v2v, bark, music, voice and others. This is defence in depth rather than the fix: a denylist over third-party IDs always lags, which is why the ordering above matters more. The pattern is tested in both directions, because a term broad enough to catch a generator family can also swallow a real chat model and push the probe onto models[0], reintroducing the bug from the other side. The probe result now reports which model was used and whether OneAgent chose it, and a failure on an auto-selected model names it and points at the override field. Before this the message could not distinguish a rejected key from a model we picked badly — the same text for two unrelated causes. It stays quiet when the key itself was rejected, which is about the key whatever model carried it. Verified the report's classification table against the live regex before changing anything: all ten video and audio IDs it lists were indeed treated as chat models, and PickChatModel did return the t2v model when it came first. Co-Authored-By: Claude Fable 5 --- .../OneAgent/internal/binding/models.ts | 16 ++++++ .../src/components/ConnectionStatus.test.tsx | 50 +++++++++++++++++ frontend/src/components/ConnectionStatus.tsx | 13 ++++- frontend/src/i18n.tsx | 4 ++ frontend/src/styles/app.css | 22 ++++++++ internal/app/agent.go | 4 +- internal/app/install.go | 2 +- internal/app/provider.go | 40 +++++++++++--- internal/app/provider_test.go | 54 +++++++++++++++++++ internal/binding/services.go | 9 ++++ internal/provider/provider.go | 8 ++- internal/provider/provider_test.go | 32 +++++++++++ 12 files changed, 242 insertions(+), 12 deletions(-) create mode 100644 frontend/src/components/ConnectionStatus.test.tsx diff --git a/frontend/bindings/github.com/MaimoryLab/OneAgent/internal/binding/models.ts b/frontend/bindings/github.com/MaimoryLab/OneAgent/internal/binding/models.ts index be58c2a2..fdc84dd6 100644 --- a/frontend/bindings/github.com/MaimoryLab/OneAgent/internal/binding/models.ts +++ b/frontend/bindings/github.com/MaimoryLab/OneAgent/internal/binding/models.ts @@ -105,6 +105,14 @@ export interface ModelsResponse { "retryable": boolean; "protocol"?: string | null; "protocols"?: { [_ in string]?: ProbeResponse } | null; + + /** + * Model is the ID that was probed, and AutoSelectedModel says OneAgent chose + * it rather than the user. A failure on a model we picked is not evidence + * about the user's key, and the UI has to be able to say which it is. + */ + "model"?: string; + "auto_selected_model"?: boolean; "models": string[] | null; } @@ -136,6 +144,14 @@ export interface ProbeResponse { "retryable": boolean; "protocol"?: string | null; "protocols"?: { [_ in string]?: ProbeResponse } | null; + + /** + * Model is the ID that was probed, and AutoSelectedModel says OneAgent chose + * it rather than the user. A failure on a model we picked is not evidence + * about the user's key, and the UI has to be able to say which it is. + */ + "model"?: string; + "auto_selected_model"?: boolean; } export interface ProviderIDRequest { diff --git a/frontend/src/components/ConnectionStatus.test.tsx b/frontend/src/components/ConnectionStatus.test.tsx new file mode 100644 index 00000000..3942d403 --- /dev/null +++ b/frontend/src/components/ConnectionStatus.test.tsx @@ -0,0 +1,50 @@ +import { render, screen } from "@testing-library/react"; +import { describe, expect, it } from "vitest"; + +import { I18nProvider } from "../i18n"; +import type { ProbeResponse } from "../types/api"; +import { ConnectionStatus } from "./ConnectionStatus"; + +function probe(overrides: Partial = {}): ProbeResponse { + return { + ok: false, reachable: true, status: 400, message: "模型请求被拒绝", + error_code: "PROTOCOL_UNSUPPORTED", retryable: false, ...overrides, + } as ProbeResponse; +} + +// jsdom reports navigator.language as en-US, so I18nProvider resolves to English +// and the assertions below match the translated strings rather than the zh keys. +function show(result: ProbeResponse) { + render(); +} + +describe("ConnectionStatus", () => { + // A Provider's catalogue is mostly image, video and audio generators. One of + // those rejecting a chat payload used to be indistinguishable from a bad key, + // so the user read a wrong verdict about their credentials during setup. + it("names a model it chose itself when the probe fails", () => { + show(probe({ model: "wan-ai/wan2.1-t2v-14b", auto_selected_model: true })); + expect(screen.getByText(/wan-ai\/wan2\.1-t2v-14b/)).toBeTruthy(); + expect(screen.getByText(/OneAgent chose the model/)).toBeTruthy(); + }); + + it("stays quiet about the model the user chose themselves", () => { + // Their override, so the failure is the answer they asked for; explaining our + // choice would be both wrong and confusing. + show(probe({ model: "kwai/kling-v1-video", auto_selected_model: false })); + expect(screen.getByText("模型请求被拒绝")).toBeTruthy(); + expect(screen.queryByText(/OneAgent chose the model/)).toBeNull(); + }); + + it("does not blame the model when the key itself was rejected", () => { + // A rejected key is about the key whatever model carried the request. + show(probe({ error_code: "API_KEY_REJECTED", message: "API Key 无效", model: "wan-ai/wan2.1-t2v-14b", auto_selected_model: true })); + expect(screen.getByText("API Key 无效")).toBeTruthy(); + expect(screen.queryByText(/OneAgent chose the model/)).toBeNull(); + }); + + it("reports a successful probe with the provider's own message", () => { + show(probe({ ok: true, status: 200, message: "连接正常", error_code: null })); + expect(screen.getByText("连接正常")).toBeTruthy(); + }); +}); diff --git a/frontend/src/components/ConnectionStatus.tsx b/frontend/src/components/ConnectionStatus.tsx index 5e1a49ae..3c0c1a5a 100644 --- a/frontend/src/components/ConnectionStatus.tsx +++ b/frontend/src/components/ConnectionStatus.tsx @@ -31,10 +31,21 @@ export function ConnectionStatus({ state, result }: { state: AsyncState; result: ); } const rejected = result?.error_code === "API_KEY_REJECTED"; + // A model OneAgent chose is named in the failure. A Provider's catalogue is + // mostly image, video and audio generators, and one of those rejecting a chat + // payload otherwise reads as a broken key -- a wrong verdict about the user's + // credentials. Not shown when the key itself was rejected, which is about the + // key whatever model was used, nor when the user named the model themselves. + const blamedModel = !rejected && result?.auto_selected_model ? result.model : ""; return (
{rejected ? : } - {result?.message || t("连接失败")} + + {result?.message || t("连接失败")} + {blamedModel ? ( + {t("测试使用的模型 {model} 由 OneAgent 自动选择，可能不支持对话。可在上方自定义模型名称后重试", { model: blamedModel })} + ) : null} +
); } diff --git a/frontend/src/i18n.tsx b/frontend/src/i18n.tsx index 189b9e28..c1323ca6 100644 --- a/frontend/src/i18n.tsx +++ b/frontend/src/i18n.tsx @@ -105,6 +105,10 @@ const english = { "尚未测试连接": "Connection not tested", "正在验证端点和 Key": "Validating endpoint and key", "连接失败": "Connection failed", + // Shown when the probe failed on a model OneAgent picked rather than one the + // user typed. Without it, a video or image model rejecting a chat request reads + // as a rejected API key. + "测试使用的模型 {model} 由 OneAgent 自动选择，可能不支持对话。可在上方自定义模型名称后重试": "OneAgent chose the model {model} for this test, and it may not support chat. Enter a model name above and try again", "查看安装日志": "View installation log", "任务中心": "Task center", "有任务正在运行": "A task is running", diff --git a/frontend/src/styles/app.css b/frontend/src/styles/app.css index b4fb2573..d9918f73 100644 --- a/frontend/src/styles/app.css +++ b/frontend/src/styles/app.css @@ -801,6 +801,28 @@ .inline-status.status-warning { color: var(--orange); background: transparent; } .inline-status.status-error { color: var(--red); } +/* A failure can carry a second line explaining that the probed model was chosen + for the user. align-items moves to the top so the icon stays beside the first + line rather than centring against a two-line block. */ +.inline-status:has(small) { + align-items: flex-start; + padding: 9px 0; +} + +.inline-status > span { + min-width: 0; + display: grid; + gap: 3px; +} + +/* Inherits neither the colour nor the weight of the error above it: the + explanation is context, not a second alarm. */ +.inline-status small { + color: var(--text-secondary); + font-size: 12px; + line-height: 1.45; +} + .model-picker { width: 100%; display: grid; diff --git a/internal/app/agent.go b/internal/app/agent.go index 300ebb1d..f41035cf 100644 --- a/internal/app/agent.go +++ b/internal/app/agent.go @@ -101,7 +101,9 @@ func (u *UseCases) activateAgentLocked(ctx context.Context, options ActivateAgen return ActivateAgentResult{}, err } - model, err := u.resolveProviderModel(ctx, target, apiKey, options.Model) + // The auto-selected flag only matters to the probe, which has to explain a + // failure; activation just needs a usable model. + model, _, err := u.resolveProviderModel(ctx, target, apiKey, options.Model) if err != nil { return ActivateAgentResult{}, err } diff --git a/internal/app/install.go b/internal/app/install.go index fbb2f80b..539a67ff 100644 --- a/internal/app/install.go +++ b/internal/app/install.go @@ -194,7 +194,7 @@ func (u *UseCases) validateInstall(ctx context.Context, manifest catalog.Manifes if u.provider == nil { return options, oneerrors.New(oneerrors.InternalError, "Model discovery is not configured", oneerrors.WithStatus(501)) } - model, err := u.resolveProviderModel(ctx, target, options.APIKey, "") + model, _, err := u.resolveProviderModel(ctx, target, options.APIKey, "") if err != nil { return options, err } diff --git a/internal/app/provider.go b/internal/app/provider.go index f2847da6..4859947e 100644 --- a/internal/app/provider.go +++ b/internal/app/provider.go @@ -3,6 +3,7 @@ package app import ( "context" "fmt" + "slices" "sort" "strings" "sync" @@ -25,6 +26,13 @@ type ProviderProbeOptions struct { type ProviderProbeResult struct { Primary provider.ProbeResult Protocols map[string]provider.ProbeResult + // Model is the ID actually probed, and AutoSelectedModel says we chose it + // rather than the user. Together they let a failure distinguish "your key is + // wrong" from "we picked a model this endpoint does not serve for chat" -- + // indistinguishable before, which is what made a bad auto-pick read as a + // credential problem. + Model string + AutoSelectedModel bool } // SaveProviderResult reports which Agents were rewritten after the edit so the @@ -50,7 +58,7 @@ func (u *UseCases) ProbeProvider(ctx context.Context, options ProviderProbeOptio if apiKey == "" { apiKey = target.APIKey } - model, err := u.resolveProviderModel(ctx, target, apiKey, options.Model) + model, autoSelected, err := u.resolveProviderModel(ctx, target, apiKey, options.Model) if err != nil { return ProviderProbeResult{}, err } @@ -73,7 +81,7 @@ func (u *UseCases) ProbeProvider(ctx context.Context, options ProviderProbeOptio } } primary.OK = allOK - return ProviderProbeResult{Primary: primary, Protocols: results}, nil + return ProviderProbeResult{Primary: primary, Protocols: results, Model: model, AutoSelectedModel: autoSelected}, nil } func (u *UseCases) probeProtocols(ctx context.Context, protocols []string, apiKey, model string, baseFor func(string) string) (map[string]provider.ProbeResult, error) { @@ -265,21 +273,37 @@ func (u *UseCases) DeleteProvider(ctx context.Context, providerID string) error return u.providers.Delete(ctx, providerID) } -func (u *UseCases) resolveProviderModel(ctx context.Context, target provider.Entry, apiKey, model string) (string, error) { +// resolveProviderModel decides which model the connection probe sends a chat +// payload to. The second return value reports whether the choice was ours rather +// than the user's, so a failure can say which of the two it is describing. +// +// Preference order, and why: a model the user typed wins outright, because the +// probe model is explicitly their override and a failure on it is the answer they +// asked for. Otherwise the Provider's manifest model wins over anything picked out +// of the live catalogue — it is a reviewed, known-chat ID for that Provider, while +// the catalogue of an aggregator is mostly video, image and audio generators whose +// names no denylist will ever fully enumerate. PickChatModel is the last resort, +// for a custom endpoint or a Provider whose manifest model it no longer serves. +func (u *UseCases) resolveProviderModel(ctx context.Context, target provider.Entry, apiKey, model string) (string, bool, error) { if model = strings.TrimSpace(model); model != "" { - return model, nil + return model, false, nil } if apiKey == "" { - return target.FallbackModel, nil + return target.FallbackModel, true, nil } listing, err := u.provider.ListModels(ctx, "custom", apiKey, target.BaseURL) if err != nil { - return "", err + return "", true, err } if listing.OK && len(listing.Models) > 0 { - return provider.PickChatModel(listing.Models), nil + // Only when the Provider actually serves it: probing a manifest model the + // endpoint has dropped would fail for a reason the user cannot act on. + if fallback := strings.TrimSpace(target.FallbackModel); fallback != "" && slices.Contains(listing.Models, fallback) { + return fallback, true, nil + } + return provider.PickChatModel(listing.Models), true, nil } - return target.FallbackModel, nil + return target.FallbackModel, true, nil } func protocolsForAgents(agentIDs []string) ([]string, error) { diff --git a/internal/app/provider_test.go b/internal/app/provider_test.go index 9eff56a7..4e15231e 100644 --- a/internal/app/provider_test.go +++ b/internal/app/provider_test.go @@ -304,3 +304,57 @@ func TestDeleteIgnoresStaleBindingWithoutAgentConfig(t *testing.T) { t.Fatalf("stale binding blocked Provider deletion: %v", err) } } + +// A built-in Provider's catalogue is mostly not chat models. With an empty probe +// model the live list used to win outright, so a video generator returned first +// became the model the connection test sent a chat payload to -- and the failure +// read as "your key is broken" during first-run setup. +func TestProbeProviderPrefersTheManifestModelOverAVideoModel(t *testing.T) { + var probed string + core := providerUseCases(t, appProviderDoer(func(request *http.Request) (*http.Response, error) { + if request.URL.Path == "/openai/v1/models" { + // Ordered as an aggregator really does: generators first. + return appProviderResponse(http.StatusOK, `{"data":[ + {"id":"wan-ai/wan2.1-t2v-14b"}, + {"id":"kwai/kling-v1-video"}, + {"id":"deepseek/deepseek-v4-flash"}, + {"id":"deepseek/deepseek-v4-pro"} + ]}`), nil + } + body, _ := io.ReadAll(request.Body) + probed = string(body) + return appProviderResponse(http.StatusNoContent, ""), nil + })) + result, err := core.ProbeProvider(context.Background(), ProviderProbeOptions{ + Provider: "ppio", APIKey: "key", AgentIDs: []string{"opencode"}, + }) + if err != nil || !result.Primary.OK { + t.Fatalf("probe = %#v, err=%v", result, err) + } + // The manifest's reviewed chat model, not the first survivor of the denylist. + if !strings.Contains(probed, "deepseek/deepseek-v4-flash") { + t.Fatalf("probed payload did not use the manifest model: %s", probed) + } +} + +// A model the user typed is their override and must be probed verbatim, even when +// it is one the denylist would reject: a failure they asked for is information. +func TestProbeProviderKeepsAModelTheUserTyped(t *testing.T) { + var probed string + core := providerUseCases(t, appProviderDoer(func(request *http.Request) (*http.Response, error) { + if request.URL.Path == "/openai/v1/models" { + t.Fatal("discovery ran even though the user named a model") + } + body, _ := io.ReadAll(request.Body) + probed = string(body) + return appProviderResponse(http.StatusNoContent, ""), nil + })) + if _, err := core.ProbeProvider(context.Background(), ProviderProbeOptions{ + Provider: "ppio", APIKey: "key", Model: "kwai/kling-v1-video", AgentIDs: []string{"opencode"}, + }); err != nil { + t.Fatal(err) + } + if !strings.Contains(probed, "kwai/kling-v1-video") { + t.Fatalf("user's model was replaced: %s", probed) + } +} diff --git a/internal/binding/services.go b/internal/binding/services.go index 53c16146..b1fdf21f 100644 --- a/internal/binding/services.go +++ b/internal/binding/services.go @@ -202,6 +202,10 @@ func (s *ProviderService) Probe(ctx context.Context, request ProbeRequest) (Prob return ProbeResponse{}, err } response := probeResponse(result.Primary) + // Set on the top-level response only: the per-protocol entries all probed the + // same model, so repeating it there would suggest they could differ. + response.Model = result.Model + response.AutoSelectedModel = result.AutoSelectedModel response.Protocols = make(map[string]ProbeResponse, len(result.Protocols)) for protocolID, protocolResult := range result.Protocols { response.Protocols[protocolID] = probeResponse(protocolResult) @@ -511,6 +515,11 @@ type ProbeResponse struct { Retryable bool `json:"retryable"` Protocol *string `json:"protocol,omitempty"` Protocols map[string]ProbeResponse `json:"protocols,omitempty"` + // Model is the ID that was probed, and AutoSelectedModel says OneAgent chose + // it rather than the user. A failure on a model we picked is not evidence + // about the user's key, and the UI has to be able to say which it is. + Model string `json:"model,omitempty"` + AutoSelectedModel bool `json:"auto_selected_model,omitempty"` } type ModelsResponse struct { diff --git a/internal/provider/provider.go b/internal/provider/provider.go index a7b65f02..6c6ce4f9 100644 --- a/internal/provider/provider.go +++ b/internal/provider/provider.go @@ -18,7 +18,13 @@ const ( ProtocolResponses = catalog.ProtocolResponses ) -var nonChatModel = regexp.MustCompile(`(?i)(^|[-_/.])(embed(ding)?s?|rerank(er)?s?|ocr|whisper|asr|tts|speech|vl|vision|image|sdx?|flux|guard(rail)?s?|moderation|sql)([-_/.]|$)`) +// nonChatModel matches IDs that clearly name a non-chat endpoint. It is defence +// in depth, not the primary guard: a denylist over third-party model IDs always +// lags the vendors, so callers that have a reviewed model for the Provider should +// prefer that (see app.resolveProviderModel). The video, audio and music terms +// were added after a t2v model returned first by an aggregator got probed with a +// chat payload and the failure read as a bad API key. +var nonChatModel = regexp.MustCompile(`(?i)(^|[-_/.])(embed(ding)?s?|rerank(er)?s?|ocr|whisper|asr|tts|speech|voice|audio|music|bark|vl|vision|image|img|sdx?|flux|video|t2v|i2v|v2v|t2i|i2t|t2a|sora|veo|kling|hailuo|seedance|cogvideox?|wan[0-9.]*|guard(rail)?s?|moderation|sql)([-_/.]|$)`) // ValidateBaseURL accepts an explicit HTTP(S) origin or path and rejects the // forms that could smuggle credentials or control characters into requests. diff --git a/internal/provider/provider_test.go b/internal/provider/provider_test.go index 8aee87a5..4e620b38 100644 --- a/internal/provider/provider_test.go +++ b/internal/provider/provider_test.go @@ -94,3 +94,35 @@ func TestInvalidProtocolErrorIsStable(t *testing.T) { t.Fatalf("invalid protocol error = %v", err) } } + +// The generator IDs the aggregators OneAgent ships actually return. Before the +// video and audio terms were added, every one of these was treated as a chat +// model, so an aggregator listing one first had it probed with a chat payload. +func TestPickChatModelSkipsGeneratorModels(t *testing.T) { + generators := []string{ + "wan-ai/wan2.1-t2v-14b", "kwai/kling-v1-video", "sora-2", "veo-3.0-generate-001", + "zai-org/cogvideox-5b", "tencent/hunyuan-video", "bytedance/seedance-1-0-pro", + "minimaxai/minimax-hailuo-02", "stabilityai/stable-video-diffusion", "suno/bark", + "black-forest-labs/flux-1-schnell", "qwen/qwen-image-edit", "minimaxai/minimax-speech-02", + } + for _, id := range generators { + if got := PickChatModel([]string{id, "deepseek/deepseek-v4-pro"}); got != "deepseek/deepseek-v4-pro" { + t.Errorf("PickChatModel picked the generator %q", got) + } + } +} + +// The other half of the denylist's contract, and the reason it cannot simply be +// made broader: a term like "wan" or "veo" that matched real chat model families +// would push the probe onto models[0] and reintroduce the bug from the other side. +func TestPickChatModelKeepsRealChatModels(t *testing.T) { + for _, id := range []string{ + "deepseek/deepseek-v4-pro", "deepseek/deepseek-v4-flash", "gpt-5.6-terra", "claude-fable-5", + "qwen/qwen3-235b-a22b", "moonshotai/kimi-k2", "meta-llama/llama-4-maverick", + "zai-org/glm-4.6", "minimaxai/minimax-m2", "openai/gpt-oss-120b", + } { + if got := PickChatModel([]string{id}); got != id { + t.Errorf("chat model %q was classified as non-chat", id) + } + } +}