From 8670ed22be6918eb29f652749a34308ad2a07c13 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Wed, 12 Aug 2026 16:40:53 -0400 Subject: [PATCH] =?UTF-8?q?refactor:=20gadfly=20round=203=20=E2=80=94=20on?= =?UTF-8?q?e=20table=20owns=20the=20OpenAI-compat=20contract?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings, and the first two are the same recurring shape. envKeyForProvider (env.go) is now the single definition of the LLM_ form. It lived in two places — lazy resolution in registry.go and the missing-key hint in openaiCompatScheme — with a comment on the second asserting it matched the first. A comment is not enforcement: if either had drifted, a keyless DSN target would have named a variable that does nothing, and nothing would have failed. The kimi and qwen test files had become near-identical, which is round 1's finding at the level above it: I deduped the fixtures, then left two parallel suites asserting the same four things. They are now ONE table in builtin_openaicompat_test.go — endpoint + credential, missing key fails closed naming its own variable and never reaching the network, the name:// DSN reaching another host, and a keyless DSN naming LLM_ instead of the built-in's key. Adding an OpenAI-compat built-in is a table row that immediately owes all four; builtin_kimi_test.go is deleted because the table covers it. Only genuinely qwen-specific tests remain in the qwen file: the reverse credential leak and the reasoning_effort wire claim ADR-0027 rests on. Also trimmed ProviderQwen's doc comment, which restated the ADR-0027 rationale already given at the registration site. The break-check suite caught its own rot again — two mutations went stale when these tests were renamed, and the landed-check reported them loudly instead of passing them off as green. Now 9 cases, including one that drifts envKeyForProvider to prove the shared helper is load-bearing. 9/9 apply and are caught. Co-Authored-By: Claude Opus 5 (1M context) --- builtin.go | 10 +- builtin_kimi_test.go | 142 -------------------------- builtin_openaicompat_test.go | 190 ++++++++++++++++++++++++++++++++++- builtin_qwen_test.go | 145 ++------------------------ env.go | 13 +++ registry.go | 2 +- 6 files changed, 211 insertions(+), 291 deletions(-) delete mode 100644 builtin_kimi_test.go diff --git a/builtin.go b/builtin.go index 5d89ba1..b0301b1 100644 --- a/builtin.go +++ b/builtin.go @@ -2,7 +2,6 @@ package majordomo import ( "net/http" - "strings" "gitea.stevedudenhoeffer.com/steve/majordomo/llm" "gitea.stevedudenhoeffer.com/steve/majordomo/provider/anthropic" @@ -22,11 +21,8 @@ const ( // ProviderQwen is Alibaba's Qwen models over Model Studio's // OpenAI-compatible Chat Completions endpoint. Reuses the openai client // (like kimi and llama-swap); keyed by QWEN_API_KEY, default base URL - // qwenBaseURL. Why OpenAI-compat and not the Anthropic-compat endpoint - // Model Studio also exposes: see ADR-0027 — the OpenAI surface is the - // first-class one there (reasoning_effort, json_schema structured output, - // cached_tokens accounting all ride it), while the Anthropic shim exists - // mainly to host Claude Code. + // qwenBaseURL. ADR-0027 records why the OpenAI surface and not the + // Anthropic-compatible one Model Studio also exposes. ProviderQwen = "qwen" ProviderAnthropic = "anthropic" ProviderGoogle = "google" @@ -67,7 +63,7 @@ func openaiCompatScheme(wrap func(...openai.Option) []openai.Option) SchemeFacto openai.WithName(name), openai.WithBaseURL(dsn.BaseURL()), openai.WithAPIKey(dsn.Token), - openai.WithAPIKeyName("LLM_"+strings.ToUpper(strings.ReplaceAll(name, "-", "_"))), + openai.WithAPIKeyName(envKeyForProvider(name)), )...), nil } } diff --git a/builtin_kimi_test.go b/builtin_kimi_test.go deleted file mode 100644 index 3281a2e..0000000 --- a/builtin_kimi_test.go +++ /dev/null @@ -1,142 +0,0 @@ -package majordomo - -import ( - "context" - "errors" - "net/http" - "strings" - "testing" - - "gitea.stevedudenhoeffer.com/steve/majordomo/llm" -) - -// TestKimiBuiltin: the built-in "kimi" provider resolves in Parse, targets -// Moonshot's default endpoint, and authenticates with KIMI_API_KEY. -func TestKimiBuiltin(t *testing.T) { - rt := &captureRT{body: chatCompletionOK} - r := newTestRegistry(t, - WithEnvLookup(singleKeyEnv("KIMI_API_KEY", "kimi-secret")), - WithHTTPClient(&http.Client{Transport: rt}), - ) - - if p, ok := r.Provider(ProviderKimi); !ok { - t.Fatal("built-in kimi provider not registered") - } else if p.Name() != ProviderKimi { - t.Errorf("name = %q, want %q", p.Name(), ProviderKimi) - } - - m, err := r.Parse("kimi/kimi-k2-0711-preview") - if err != nil { - t.Fatalf("Parse: %v", err) - } - if got := targetsOf(t, m); len(got) != 1 || got[0] != "kimi/kimi-k2-0711-preview" { - t.Fatalf("targets = %v", got) - } - - if _, err := m.Generate(context.Background(), llm.Request{Messages: []llm.Message{llm.UserText("hi")}}); err != nil { - t.Fatalf("Generate: %v", err) - } - if rt.req == nil { - t.Fatal("no request captured") - } - if want := "https://api.moonshot.ai/v1/chat/completions"; rt.req.URL.String() != want { - t.Errorf("URL = %q, want %q", rt.req.URL.String(), want) - } - if want := "Bearer kimi-secret"; rt.req.Header.Get("Authorization") != want { - t.Errorf("Authorization = %q, want %q", rt.req.Header.Get("Authorization"), want) - } -} - -// TestKimiBuiltinMissingKey: with no KIMI_API_KEY the built-in fails fast with a -// synthetic 401 whose hint names KIMI_API_KEY — never OPENAI_API_KEY (proving -// the credential does not fall through to the openai client's default), and -// without hitting the network. -func TestKimiBuiltinMissingKey(t *testing.T) { - rt := &captureRT{body: chatCompletionOK} - r := newTestRegistry(t, WithHTTPClient(&http.Client{Transport: rt})) - - m, err := r.Parse("kimi/kimi-k2-0711-preview") - if err != nil { - t.Fatalf("Parse: %v", err) - } - _, err = m.Generate(context.Background(), llm.Request{Messages: []llm.Message{llm.UserText("hi")}}) - apiErr, ok := errors.AsType[*llm.APIError](err) - if !ok { - t.Fatalf("err = %v (%T), want *llm.APIError", err, err) - } - if apiErr.Status != http.StatusUnauthorized || apiErr.Code != "missing_api_key" { - t.Errorf("Status/Code = %d/%q, want 401/missing_api_key", apiErr.Status, apiErr.Code) - } - if !strings.Contains(apiErr.Message, "KIMI_API_KEY") { - t.Errorf("message = %q, want it to name KIMI_API_KEY", apiErr.Message) - } - if strings.Contains(apiErr.Message, "OPENAI_API_KEY") { - t.Errorf("message = %q, must not name OPENAI_API_KEY", apiErr.Message) - } - if rt.req != nil { - t.Error("network was hit despite missing key") - } -} - -// TestKimiScheme: a kimi:// LLM_* DSN defines a named provider on any Moonshot -// host (here the China endpoint) that is first-class in Parse and carries the -// DSN token as its bearer credential. -func TestKimiScheme(t *testing.T) { - rt := &captureRT{body: chatCompletionOK} - r := newTestRegistry(t, WithHTTPClient(&http.Client{Transport: rt})) - if err := r.LoadEnv(map[string]string{ - "LLM_KCN": "kimi://tok@api.moonshot.cn/v1", - }); err != nil { - t.Fatalf("LoadEnv: %v", err) - } - - m, err := r.Parse("kcn/moonshot-v1-8k") - if err != nil { - t.Fatalf("Parse: %v", err) - } - if _, err := m.Generate(context.Background(), llm.Request{Messages: []llm.Message{llm.UserText("hi")}}); err != nil { - t.Fatalf("Generate: %v", err) - } - if rt.req == nil { - t.Fatal("no request captured") - } - if want := "https://api.moonshot.cn/v1/chat/completions"; rt.req.URL.String() != want { - t.Errorf("URL = %q, want %q", rt.req.URL.String(), want) - } - if want := "Bearer tok"; rt.req.Header.Get("Authorization") != want { - t.Errorf("Authorization = %q, want %q", rt.req.Header.Get("Authorization"), want) - } -} - -// TestKimiSchemeMissingToken: a kimi:// DSN with no token is fixed by adding one -// to the DSN, not by setting KIMI_API_KEY — so the missing-key hint names the -// defining LLM_ env var, never KIMI_API_KEY (which does nothing for a -// DSN-defined provider). -func TestKimiSchemeMissingToken(t *testing.T) { - rt := &captureRT{body: chatCompletionOK} - r := newTestRegistry(t, WithHTTPClient(&http.Client{Transport: rt})) - if err := r.LoadEnv(map[string]string{ - "LLM_KCN": "kimi://api.moonshot.cn/v1", // no token - }); err != nil { - t.Fatalf("LoadEnv: %v", err) - } - - m, err := r.Parse("kcn/moonshot-v1-8k") - if err != nil { - t.Fatalf("Parse: %v", err) - } - _, err = m.Generate(context.Background(), llm.Request{Messages: []llm.Message{llm.UserText("hi")}}) - apiErr, ok := errors.AsType[*llm.APIError](err) - if !ok { - t.Fatalf("err = %v (%T), want *llm.APIError", err, err) - } - if !strings.Contains(apiErr.Message, "LLM_KCN") { - t.Errorf("message = %q, want it to name LLM_KCN", apiErr.Message) - } - if strings.Contains(apiErr.Message, "KIMI_API_KEY") { - t.Errorf("message = %q, must not name KIMI_API_KEY for a DSN provider", apiErr.Message) - } - if rt.req != nil { - t.Error("network was hit despite missing token") - } -} diff --git a/builtin_openaicompat_test.go b/builtin_openaicompat_test.go index 9f66438..1290e2e 100644 --- a/builtin_openaicompat_test.go +++ b/builtin_openaicompat_test.go @@ -1,15 +1,21 @@ package majordomo import ( + "context" + "errors" "io" "net/http" "strings" + "testing" + + "gitea.stevedudenhoeffer.com/steve/majordomo/llm" ) -// Shared fixtures for the built-ins that are "the openai client pointed -// somewhere else" (kimi, qwen, ...). They live here rather than in any one -// provider's test file so a new OpenAI-compat built-in has nothing to -// copy — the same reason openaiCompatScheme exists on the production side. +// Shared fixtures and the shared contract for the built-ins that are "the +// openai client pointed somewhere else" (kimi, qwen, ...). They live here +// rather than in any one provider's test file so a new OpenAI-compat built-in +// has nothing to copy — the same reason registerOpenAICompatBuiltin exists on +// the production side. // chatCompletionOK is a minimal valid Chat Completions body, so Generate // returns a non-empty response (an empty one would trigger failover, not a @@ -56,3 +62,179 @@ func singleKeyEnv(key, value string) func(string) string { return "" } } + +// openAICompatBuiltin describes one built-in for the shared contract below. +// Adding an OpenAI-compat built-in means adding a row here — not copying a +// test file, which is how kimi's and qwen's suites became near-identical. +type openAICompatBuiltin struct { + name string // registry name and spec prefix + keyEnv string // the credential variable this built-in reads + model string // a current model id for that endpoint + wantURL string // chat-completions URL the default endpoint must produce + + // The name:// DSN case: an alternate host (regional/China endpoint) + // reached through an LLM_ definition. + dsnVar string + dsnHost string + wantDSNURL string +} + +var openAICompatBuiltins = []openAICompatBuiltin{ + { + name: ProviderKimi, + keyEnv: "KIMI_API_KEY", + model: "kimi-k2-0711-preview", + wantURL: "https://api.moonshot.ai/v1/chat/completions", + dsnVar: "LLM_KCN", + dsnHost: "api.moonshot.cn/v1", + wantDSNURL: "https://api.moonshot.cn/v1/chat/completions", + }, + { + name: ProviderQwen, + keyEnv: "QWEN_API_KEY", + model: "qwen3.8-max", + wantURL: "https://dashscope-intl.aliyuncs.com/compatible-mode/v1/chat/completions", + dsnVar: "LLM_QCN", + dsnHost: "dashscope.aliyuncs.com/compatible-mode/v1", + wantDSNURL: "https://dashscope.aliyuncs.com/compatible-mode/v1/chat/completions", + }, +} + +// TestOpenAICompatBuiltins is the whole contract an OpenAI-compat built-in +// owes, asserted identically for every one of them: it resolves in Parse and +// targets its own endpoint with its own key; a missing key fails closed naming +// the right variable and never reaching the network; its name:// DSN reaches +// any other host on the DSN token; and a keyless DSN names the LLM_ that +// actually fixes it rather than the built-in's variable, which does nothing +// for a DSN-defined provider. +func TestOpenAICompatBuiltins(t *testing.T) { + for _, tc := range openAICompatBuiltins { + t.Run(tc.name+"/builtin", func(t *testing.T) { + rt := &captureRT{body: chatCompletionOK} + secret := tc.name + "-secret" + r := newTestRegistry(t, + WithEnvLookup(singleKeyEnv(tc.keyEnv, secret)), + WithHTTPClient(&http.Client{Transport: rt}), + ) + + if p, ok := r.Provider(tc.name); !ok { + t.Fatalf("built-in %q not registered", tc.name) + } else if p.Name() != tc.name { + t.Errorf("name = %q, want %q", p.Name(), tc.name) + } + + spec := tc.name + "/" + tc.model + m, err := r.Parse(spec) + if err != nil { + t.Fatalf("Parse(%q): %v", spec, err) + } + if got := targetsOf(t, m); len(got) != 1 || got[0] != spec { + t.Fatalf("targets = %v, want [%q]", got, spec) + } + + if _, err := m.Generate(context.Background(), llm.Request{Messages: []llm.Message{llm.UserText("hi")}}); err != nil { + t.Fatalf("Generate: %v", err) + } + if rt.req == nil { + t.Fatal("no request captured") + } + if rt.req.URL.String() != tc.wantURL { + t.Errorf("URL = %q, want %q", rt.req.URL.String(), tc.wantURL) + } + if want := "Bearer " + secret; rt.req.Header.Get("Authorization") != want { + t.Errorf("Authorization = %q, want %q", rt.req.Header.Get("Authorization"), want) + } + }) + + t.Run(tc.name+"/builtin missing key", func(t *testing.T) { + rt := &captureRT{body: chatCompletionOK} + r := newTestRegistry(t, WithHTTPClient(&http.Client{Transport: rt})) + + m, err := r.Parse(tc.name + "/" + tc.model) + if err != nil { + t.Fatalf("Parse: %v", err) + } + _, err = m.Generate(context.Background(), llm.Request{Messages: []llm.Message{llm.UserText("hi")}}) + apiErr, ok := errors.AsType[*llm.APIError](err) + if !ok { + t.Fatalf("err = %v (%T), want *llm.APIError", err, err) + } + if apiErr.Status != http.StatusUnauthorized || apiErr.Code != "missing_api_key" { + t.Errorf("Status/Code = %d/%q, want 401/missing_api_key", apiErr.Status, apiErr.Code) + } + if !strings.Contains(apiErr.Message, tc.keyEnv) { + t.Errorf("message = %q, want it to name %s", apiErr.Message, tc.keyEnv) + } + // The load-bearing half: openai.New defaults its key to + // OPENAI_API_KEY, so a built-in that stopped passing WithAPIKey + // unconditionally would authenticate as OpenAI instead of failing. + if strings.Contains(apiErr.Message, "OPENAI_API_KEY") { + t.Errorf("message = %q, must not name OPENAI_API_KEY", apiErr.Message) + } + if rt.req != nil { + t.Error("network was hit despite missing key") + } + }) + + t.Run(tc.name+"/dsn scheme", func(t *testing.T) { + rt := &captureRT{body: chatCompletionOK} + r := newTestRegistry(t, WithHTTPClient(&http.Client{Transport: rt})) + if err := r.LoadEnv(map[string]string{ + tc.dsnVar: tc.name + "://tok@" + tc.dsnHost, + }); err != nil { + t.Fatalf("LoadEnv: %v", err) + } + + dsnName := strings.ToLower(strings.TrimPrefix(tc.dsnVar, "LLM_")) + m, err := r.Parse(dsnName + "/" + tc.model) + if err != nil { + t.Fatalf("Parse: %v", err) + } + if _, err := m.Generate(context.Background(), llm.Request{Messages: []llm.Message{llm.UserText("hi")}}); err != nil { + t.Fatalf("Generate: %v", err) + } + if rt.req == nil { + t.Fatal("no request captured") + } + if rt.req.URL.String() != tc.wantDSNURL { + t.Errorf("URL = %q, want %q", rt.req.URL.String(), tc.wantDSNURL) + } + if want := "Bearer tok"; rt.req.Header.Get("Authorization") != want { + t.Errorf("Authorization = %q, want %q", rt.req.Header.Get("Authorization"), want) + } + }) + + t.Run(tc.name+"/dsn scheme missing token", func(t *testing.T) { + rt := &captureRT{body: chatCompletionOK} + r := newTestRegistry(t, WithHTTPClient(&http.Client{Transport: rt})) + if err := r.LoadEnv(map[string]string{ + tc.dsnVar: tc.name + "://" + tc.dsnHost, // no token + }); err != nil { + t.Fatalf("LoadEnv: %v", err) + } + + dsnName := strings.ToLower(strings.TrimPrefix(tc.dsnVar, "LLM_")) + m, err := r.Parse(dsnName + "/" + tc.model) + if err != nil { + t.Fatalf("Parse: %v", err) + } + _, err = m.Generate(context.Background(), llm.Request{Messages: []llm.Message{llm.UserText("hi")}}) + apiErr, ok := errors.AsType[*llm.APIError](err) + if !ok { + t.Fatalf("err = %v (%T), want *llm.APIError", err, err) + } + // A keyless DSN is fixed by adding a token to that DSN, so the + // hint must name the defining variable — never the built-in's own + // key, which does nothing for a DSN-defined provider. + if !strings.Contains(apiErr.Message, tc.dsnVar) { + t.Errorf("message = %q, want it to name %s", apiErr.Message, tc.dsnVar) + } + if strings.Contains(apiErr.Message, tc.keyEnv) { + t.Errorf("message = %q, must not name %s for a DSN provider", apiErr.Message, tc.keyEnv) + } + if rt.req != nil { + t.Error("network was hit despite missing token") + } + }) + } +} diff --git a/builtin_qwen_test.go b/builtin_qwen_test.go index afe7cb3..0419e30 100644 --- a/builtin_qwen_test.go +++ b/builtin_qwen_test.go @@ -3,88 +3,22 @@ package majordomo import ( "context" "encoding/json" - "errors" "net/http" - "strings" "testing" "gitea.stevedudenhoeffer.com/steve/majordomo/llm" ) -// TestQwenBuiltin: the built-in "qwen" provider resolves in Parse, targets -// Model Studio's international OpenAI-compatible endpoint, and authenticates -// with QWEN_API_KEY. -func TestQwenBuiltin(t *testing.T) { - rt := &captureRT{body: chatCompletionOK} - r := newTestRegistry(t, - WithEnvLookup(singleKeyEnv("QWEN_API_KEY", "qwen-secret")), - WithHTTPClient(&http.Client{Transport: rt}), - ) - - if p, ok := r.Provider(ProviderQwen); !ok { - t.Fatal("built-in qwen provider not registered") - } else if p.Name() != ProviderQwen { - t.Errorf("name = %q, want %q", p.Name(), ProviderQwen) - } - - m, err := r.Parse("qwen/qwen3.8-max") - if err != nil { - t.Fatalf("Parse: %v", err) - } - if got := targetsOf(t, m); len(got) != 1 || got[0] != "qwen/qwen3.8-max" { - t.Fatalf("targets = %v", got) - } - - if _, err := m.Generate(context.Background(), llm.Request{Messages: []llm.Message{llm.UserText("hi")}}); err != nil { - t.Fatalf("Generate: %v", err) - } - if rt.req == nil { - t.Fatal("no request captured") - } - if want := "https://dashscope-intl.aliyuncs.com/compatible-mode/v1/chat/completions"; rt.req.URL.String() != want { - t.Errorf("URL = %q, want %q", rt.req.URL.String(), want) - } - if want := "Bearer qwen-secret"; rt.req.Header.Get("Authorization") != want { - t.Errorf("Authorization = %q, want %q", rt.req.Header.Get("Authorization"), want) - } -} - -// TestQwenBuiltinMissingKey: with no QWEN_API_KEY the built-in fails fast with -// a synthetic 401 whose hint names QWEN_API_KEY — never OPENAI_API_KEY (proving -// the credential does not fall through to the openai client's default), and -// without hitting the network. -func TestQwenBuiltinMissingKey(t *testing.T) { - rt := &captureRT{body: chatCompletionOK} - r := newTestRegistry(t, WithHTTPClient(&http.Client{Transport: rt})) - - m, err := r.Parse("qwen/qwen3.8-max") - if err != nil { - t.Fatalf("Parse: %v", err) - } - _, err = m.Generate(context.Background(), llm.Request{Messages: []llm.Message{llm.UserText("hi")}}) - apiErr, ok := errors.AsType[*llm.APIError](err) - if !ok { - t.Fatalf("err = %v (%T), want *llm.APIError", err, err) - } - if apiErr.Status != http.StatusUnauthorized || apiErr.Code != "missing_api_key" { - t.Errorf("Status/Code = %d/%q, want 401/missing_api_key", apiErr.Status, apiErr.Code) - } - if !strings.Contains(apiErr.Message, "QWEN_API_KEY") { - t.Errorf("message = %q, want it to name QWEN_API_KEY", apiErr.Message) - } - if strings.Contains(apiErr.Message, "OPENAI_API_KEY") { - t.Errorf("message = %q, must not name OPENAI_API_KEY", apiErr.Message) - } - if rt.req != nil { - t.Error("network was hit despite missing key") - } -} +// The contract qwen shares with every other OpenAI-compat built-in (endpoint, +// credential isolation, its qwen:// DSN) is asserted by the table in +// builtin_openaicompat_test.go. What remains here is qwen-specific: the +// reverse-leak direction, and the wire claim ADR-0027 turns on. // TestQwenBuiltinKeyDoesNotLeakToOpenAI: QWEN_API_KEY is the qwen built-in's -// credential and nothing else's. Why this direction too: the fallthrough guard -// only proves qwen never borrows OPENAI_API_KEY; this proves the reverse — a -// registry that can see QWEN_API_KEY must not hand it to the openai built-in, -// which would send an Alibaba key to api.openai.com. +// credential and nothing else's. Why this direction too: the shared table's +// missing-key case only proves qwen never borrows OPENAI_API_KEY; this proves +// the reverse — a registry that can see QWEN_API_KEY must not hand it to the +// openai built-in, which would send an Alibaba key to api.openai.com. func TestQwenBuiltinKeyDoesNotLeakToOpenAI(t *testing.T) { // Set before newTestRegistry: the openai built-in reads OPENAI_API_KEY at // construction. Giving it a real key is what keeps this test honest — a @@ -114,69 +48,6 @@ func TestQwenBuiltinKeyDoesNotLeakToOpenAI(t *testing.T) { } } -// TestQwenScheme: a qwen:// LLM_* DSN defines a named provider on any Model -// Studio host (here the China endpoint) that is first-class in Parse and -// carries the DSN token as its bearer credential. -func TestQwenScheme(t *testing.T) { - rt := &captureRT{body: chatCompletionOK} - r := newTestRegistry(t, WithHTTPClient(&http.Client{Transport: rt})) - if err := r.LoadEnv(map[string]string{ - "LLM_QCN": "qwen://tok@dashscope.aliyuncs.com/compatible-mode/v1", - }); err != nil { - t.Fatalf("LoadEnv: %v", err) - } - - m, err := r.Parse("qcn/qwen3.7-plus") - if err != nil { - t.Fatalf("Parse: %v", err) - } - if _, err := m.Generate(context.Background(), llm.Request{Messages: []llm.Message{llm.UserText("hi")}}); err != nil { - t.Fatalf("Generate: %v", err) - } - if rt.req == nil { - t.Fatal("no request captured") - } - if want := "https://dashscope.aliyuncs.com/compatible-mode/v1/chat/completions"; rt.req.URL.String() != want { - t.Errorf("URL = %q, want %q", rt.req.URL.String(), want) - } - if want := "Bearer tok"; rt.req.Header.Get("Authorization") != want { - t.Errorf("Authorization = %q, want %q", rt.req.Header.Get("Authorization"), want) - } -} - -// TestQwenSchemeMissingToken: a qwen:// DSN with no token is fixed by adding -// one to the DSN, not by setting QWEN_API_KEY — so the missing-key hint names -// the defining LLM_ env var, never QWEN_API_KEY (which does nothing for a -// DSN-defined provider). -func TestQwenSchemeMissingToken(t *testing.T) { - rt := &captureRT{body: chatCompletionOK} - r := newTestRegistry(t, WithHTTPClient(&http.Client{Transport: rt})) - if err := r.LoadEnv(map[string]string{ - "LLM_QCN": "qwen://dashscope.aliyuncs.com/compatible-mode/v1", // no token - }); err != nil { - t.Fatalf("LoadEnv: %v", err) - } - - m, err := r.Parse("qcn/qwen3.7-plus") - if err != nil { - t.Fatalf("Parse: %v", err) - } - _, err = m.Generate(context.Background(), llm.Request{Messages: []llm.Message{llm.UserText("hi")}}) - apiErr, ok := errors.AsType[*llm.APIError](err) - if !ok { - t.Fatalf("err = %v (%T), want *llm.APIError", err, err) - } - if !strings.Contains(apiErr.Message, "LLM_QCN") { - t.Errorf("message = %q, want it to name LLM_QCN", apiErr.Message) - } - if strings.Contains(apiErr.Message, "QWEN_API_KEY") { - t.Errorf("message = %q, must not name QWEN_API_KEY for a DSN provider", apiErr.Message) - } - if rt.req != nil { - t.Error("network was hit despite missing token") - } -} - // TestQwenReasoningEffortReachesWire is the load-bearing test for ADR-0027's // central claim: Model Studio's OpenAI-compatible surface takes reasoning as a // top-level "reasoning_effort" body field, which the openai client already diff --git a/env.go b/env.go index bf7d9a8..3397a9b 100644 --- a/env.go +++ b/env.go @@ -40,6 +40,19 @@ type DSN struct { // env-defined providers always speak TLS). func (d DSN) BaseURL() string { return "https://" + d.Host } +// envKeyForProvider returns the LLM_* variable that defines the provider named +// name (LLM_M1 → "m1", so "my-prov" → LLM_MY_PROV). +// +// This is the single definition on purpose. Two call sites need byte-identical +// output and would drift apart in silence: lazy resolution reads this variable +// to find an unregistered provider, and openaiCompatScheme names it in the +// missing-key hint so a keyless DSN target tells the operator which variable to +// set. Those two were separate copies with a comment asserting they matched — +// a comment is not enforcement, this function is. +func envKeyForProvider(name string) string { + return "LLM_" + strings.ToUpper(strings.ReplaceAll(name, "-", "_")) +} + // ParseDSN parses a raw DSN string. The algorithm matches go-llm exactly: // split on "://", then an optional "@" separates the token from the host; // trailing slashes on the host are trimmed. diff --git a/registry.go b/registry.go index e657e2a..f5bd162 100644 --- a/registry.go +++ b/registry.go @@ -263,7 +263,7 @@ func (r *Registry) providerFor(name string) (llm.Provider, error) { return nil, envErr } - envKey := "LLM_" + strings.ToUpper(strings.ReplaceAll(name, "-", "_")) + envKey := envKeyForProvider(name) envVal := r.envLookup(envKey) if envVal == "" { return nil, fmt.Errorf("%w: %q (checked registry and %s env var)", ErrUnknownProvider, name, envKey)