diff --git a/internal/gateway/discover.go b/internal/gateway/discover.go index e951a12..3c5db64 100644 --- a/internal/gateway/discover.go +++ b/internal/gateway/discover.go @@ -187,21 +187,31 @@ func discoveryCandidates(raw string) []discoveryCandidate { stripped := trimVersionSuffix(raw) hasVersion := stripped != raw - // Preference 0: the URL exactly as typed, catalog at the canonical path. - // When the typed base already ends in "/v1" this is "…/v1/v1/models", which - // is precisely the doubled form that 404s — so the canonical path is only - // offered against a base that does not already carry a version. - if !hasVersion { - add(raw, discoveryModelsPath, 0) - } else { - // Base already has "/v1"; ask for the bare "/models" relative to it. - add(raw, "/models", 0) - } - // Preference 1: host root + canonical versioned path. - add(stripped, discoveryModelsPath, 1) - // Preference 2: host root + bare path (vendors that imply the version). + // Both catalog paths are always tried, whichever form the operator typed. + // + // The earlier version offered "/models" only when the base already carried a + // version segment. That left a bare host ("https://api.vendor.com") with a + // SINGLE candidate and therefore no fallback at all — so a vendor serving + // its catalog at the bare "/models" was reported as unreachable. Every + // test used a "/v1"-suffixed base and so never saw it; siliconflow's + // "/v1/models" happens to answer, which is why the gap survived review. + // + // Preference only decides WHICH URL is reported when several work, never + // which ones are attempted. + // + // Order matters: a base that already ends in a version segment must be + // paired with the RELATIVE "/models" first, because pairing it with + // "/v1/models" produces "…/v1/v1/models" — a URL that is 404 by construction. + // Trying it first would spend a round trip on a certain miss and, worse, + // make the reported "tried" list lead with nonsense. if hasVersion { + add(raw, "/models", 0) + add(raw, discoveryModelsPath, 1) // the doubled form; kept only as a last resort add(stripped, "/models", 2) + add(stripped, discoveryModelsPath, 3) + } else { + add(raw, discoveryModelsPath, 0) + add(raw, "/models", 1) } // Stable insertion sort by preference; add() above is not ordered. diff --git a/internal/gateway/discover_endpoint_test.go b/internal/gateway/discover_endpoint_test.go new file mode 100644 index 0000000..8865921 --- /dev/null +++ b/internal/gateway/discover_endpoint_test.go @@ -0,0 +1,153 @@ +package gateway + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// TestComposeImageEndpoint_NoVersionBaseLeavesDefault is the counterpart of the +// /v1 case: when the base carries no version segment, discovery must emit "" so +// the gateway's own default "/v1/images/generations" composes. +// +// This is the shape every currently-working source uses — siliconflow, the only +// source in production with kind:image, has base_url +// "https://api.siliconflow.cn" — so writing a non-empty value here would break +// exactly the configuration that is known to work today. +func TestComposeImageEndpoint_NoVersionBaseLeavesDefault(t *testing.T) { + if got := composeImageEndpoint("https://api.siliconflow.cn"); got != "" { + t.Errorf("composeImageEndpoint = %q, want \"\" so the built-in default composes", got) + } + // And the composition it implies must be the vendor's real path. + base := "https://api.siliconflow.cn" + composed := base + "/v1/images/generations" + if !strings.HasSuffix(composed, "/v1/images/generations") { + t.Errorf("composed URL %q lost the image path", composed) + } +} + +// TestDiscovery_BareHostNeedsVersionOnEndpointWhenCatalogIsVersioned +// distinguishes two cases that look identical from the outside. +// +// bare host + catalog at "/models" → chat is "/chat/completions" (the +// adapter default, no endpoint written) +// bare host + catalog at "/v1/models" → chat is "/v1/chat/completions", +// so an endpoint MUST be written +// +// The catalog path is the only evidence of whether the upstream wants the +// version segment, and getting this backwards sends every chat request to a +// path the vendor does not serve. +func TestDiscovery_BareHostNeedsVersionOnEndpointWhenCatalogIsVersioned(t *testing.T) { + cases := []struct { + name string + catalogPath string + wantEndpoint string + wantImage string + }{ + { + name: "catalog at the bare /models needs no endpoint", + catalogPath: "/models", + wantEndpoint: "", + wantImage: "", + }, + { + name: "catalog at /v1/models requires /v1 on the chat endpoint", + catalogPath: "/v1/models", + wantEndpoint: "/v1/chat/completions", + wantImage: "", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != tc.catalogPath { + w.WriteHeader(http.StatusNotFound) + return + } + _, _ = w.Write([]byte(`{"data":[{"id":"m1"},{"id":"m2"}]}`)) + })) + defer srv.Close() + + res, err := DiscoverModels(context.Background(), srv.URL, "k") + if err != nil { + t.Fatalf("discovery failed: %v", err) + } + if res.BaseURL != srv.URL { + t.Errorf("base = %q, want %q", res.BaseURL, srv.URL) + } + if res.Endpoint != tc.wantEndpoint { + t.Errorf("endpoint = %q, want %q", res.Endpoint, tc.wantEndpoint) + } + if res.ImageEndpoint != tc.wantImage { + t.Errorf("image_endpoint = %q, want %q", res.ImageEndpoint, tc.wantImage) + } + // Whatever it wrote, the chat URL must reach the same version + // prefix the catalog was served under. + ep := res.Endpoint + if ep == "" { + ep = "/chat/completions" + } + if got := res.BaseURL + ep; strings.Contains(got, "/v1/v1") { + t.Errorf("chat URL %q has a doubled version", got) + } + }) + } +} + +// TestComposeEndpoints_BothFormsProduceWorkingURLs is the end-to-end +// invariant across the two shapes, stated as the URLs the gateway will build. +// +// A base with a version and no image_endpoint produces "/v1/v1/…" — the exact +// 404 observed before composeImageEndpoint existed. A base without one and a +// non-empty image_endpoint produces a path missing the version. Both are +// silent, expensive mistakes, so they are asserted literally. +func TestComposeEndpoints_BothFormsProduceWorkingURLs(t *testing.T) { + cases := []struct { + name string + base string + wantChat string + wantImage string + }{ + { + name: "bare host: adapter and image defaults compose", + base: "https://h", + wantChat: "https://h/chat/completions", + // openai's adapter endpoint is /chat/completions (verified in + // internal/lua/adapters/openai.lua: adapter.endpoint). + wantImage: "https://h/v1/images/generations", + }, + { + name: "versioned base: the version moves onto both endpoints", + base: "https://h/v1", + wantChat: "https://h/v1/chat/completions", + wantImage: "https://h/v1/images/generations", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ep := composeEndpoint("/models", tc.base, tc.base) + if ep == "" { + ep = "/chat/completions" // the adapter default + } + iep := composeImageEndpoint(tc.base) + if iep == "" { + iep = "/v1/images/generations" // the gateway default + } + gotChat := tc.base + ep + gotImage := tc.base + iep + if gotChat != tc.wantChat { + t.Errorf("chat URL = %q, want %q", gotChat, tc.wantChat) + } + if gotImage != tc.wantImage { + t.Errorf("image URL = %q, want %q", gotImage, tc.wantImage) + } + for _, u := range []string{gotChat, gotImage} { + if strings.Contains(u, "/v1/v1") { + t.Errorf("%q has a doubled version segment", u) + } + } + }) + } +} diff --git a/internal/gateway/discover_test.go b/internal/gateway/discover_test.go index c937a0d..d465b43 100644 --- a/internal/gateway/discover_test.go +++ b/internal/gateway/discover_test.go @@ -310,9 +310,6 @@ func TestDiscoveryCandidates_DedupesAndPrefersTyped(t *testing.T) { if len(cands) == 0 { t.Fatal("no candidates") } - if cands[0].modelsPath != "/models" || cands[0].baseURL != "https://h/v1" { - t.Errorf("first candidate = %+v, want the typed base with a bare /models path", cands[0]) - } seen := map[string]bool{} for _, c := range cands { // Dedup is on the requested URL, so that is the key to assert on. @@ -322,17 +319,22 @@ func TestDiscoveryCandidates_DedupesAndPrefersTyped(t *testing.T) { } seen[k] = true } + // The versioned spelling of the typed URL must be reachable: on the real + // amr-link endpoint that is what answers, and it is the preferred candidate. if !seen["https://h/v1/models"] { - t.Error("the typed base with a bare /models path must be tried; it is the first candidate and the one that answers on the real amr-link endpoint") + t.Error("the typed base with a versioned catalog path must be tried; that is what answers on amr-link") } + // Both catalog forms must be attempted for a bare host too. Offering only + // "/v1/models" there left a vendor serving a bare "/models" looking + // unreachable. if !seen["https://h/models"] { - t.Error("host root with a bare /models path was never tried; vendors that imply the version would be missed") + t.Error("host root with a bare /models path was never tried") } - // The two candidate PAIRS that both spell "https://h/v1/models" collapse to - // one request. That is the point of deduping on the final URL: listing the - // same address twice in an error message reads as two attempts. - if seen["https://h|v1/models"] && seen["https://h/models"] { - t.Error("both spellings of the versioned URL are present; one is redundant") + // The first candidate is preferred, so its URL is what gets reported when + // several answer. + if cands[0].baseURL+cands[0].modelsPath != "https://h/v1/models" { + t.Errorf("first candidate requests %q, want the canonical versioned form", + cands[0].baseURL+cands[0].modelsPath) } // The bare "/models" form must exist too — some gateways serve it there. hasBare := false