From 02cd561eafcea1d8f3095e6c173103d17aebbe4f Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Wed, 12 Aug 2026 16:07:41 -0400 Subject: [PATCH 1/6] feat(qwen): Alibaba Qwen built-in over Model Studio's OpenAI-compatible mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the `qwen` built-in provider and the `qwen://` DSN scheme, keyed by QWEN_API_KEY and defaulting to Model Studio's international host. Like kimi (ADR-0026) it is `provider/openai` pointed elsewhere — no new client. Model Studio serves the same models over two protocols, so the real decision was which wire format to speak. ADR-0027 records why it is the OpenAI one: down the anthropic client `ReasoningEffort` is ignored by design, structured output rides the first-party `output_config.format` mechanism the shim does not implement, and cached-token accounting reads Anthropic-only usage fields. Each of those fails silently rather than loudly, which is what makes the choice worth writing down. The shim stays reachable ad hoc via an `anthropic://` DSN. The kimi and qwen DSN factories were byte-identical, so they now share one `openaiCompatScheme` helper: the "credential comes from the DSN token, and the missing-key hint names LLM_" rules hold by construction instead of by copy. Tests are hermetic and break-checked (all six fail on a deliberate mutation), including the reverse credential leak — a visible QWEN_API_KEY must not authenticate the openai built-in — and reasoning_effort asserted on the wire body, which is the ADR's load-bearing claim. Co-Authored-By: Claude Opus 5 (1M context) --- .env.example | 1 + README.md | 24 +++- builtin.go | 70 ++++++++-- builtin_kimi_test.go | 19 ++- builtin_qwen_test.go | 235 ++++++++++++++++++++++++++++++++++ docs/adr/0027-qwen-builtin.md | 102 +++++++++++++++ docs/adr/README.md | 1 + progress.md | 29 +++++ 8 files changed, 461 insertions(+), 20 deletions(-) create mode 100644 builtin_qwen_test.go create mode 100644 docs/adr/0027-qwen-builtin.md diff --git a/.env.example b/.env.example index 955c225..b9dd263 100644 --- a/.env.example +++ b/.env.example @@ -7,6 +7,7 @@ OLLAMA_API_KEY=your-ollama-cloud-key-here # Built-in provider keys (each optional; only needed for the providers you use). #OPENAI_API_KEY=sk-... #KIMI_API_KEY=sk-... # Moonshot AI (Kimi); provider name "kimi" +#QWEN_API_KEY=sk-... # Alibaba Model Studio (Qwen); provider name "qwen" #ANTHROPIC_API_KEY=sk-ant-... #GOOGLE_API_KEY=... diff --git a/README.md b/README.md index 85ff031..6869dd7 100644 --- a/README.md +++ b/README.md @@ -122,6 +122,7 @@ Chains are health-tracked per target: |----------|-----------|-------------|------------------| | OpenAI (+compatible) | `openai` | `OPENAI_API_KEY` | https://api.openai.com/v1 | | Kimi (Moonshot AI) | `kimi` | `KIMI_API_KEY` | https://api.moonshot.ai/v1 | +| Qwen (Alibaba) | `qwen` | `QWEN_API_KEY` | https://dashscope-intl.aliyuncs.com/compatible-mode/v1 | | Anthropic (+compatible) | `anthropic` | `ANTHROPIC_API_KEY` | https://api.anthropic.com | | Google (Gemini) | `google` | `GOOGLE_API_KEY` / `GEMINI_API_KEY` | Gemini API (official SDK) | | Ollama Cloud | `ollama-cloud` | `OLLAMA_API_KEY` | https://ollama.com | @@ -134,6 +135,19 @@ the openai client (like llama-swap). The `kimi` built-in defaults to the international endpoint; reach the China endpoint (or any other host) with a `kimi://` DSN, e.g. `LLM_KCN=kimi://token@api.moonshot.cn/v1`. +Qwen is the same shape: Alibaba Model Studio's OpenAI-compatible mode, reusing +the openai client. The `qwen` built-in defaults to the international +(Singapore) host; reach the China host or a workspace-scoped regional one with +a `qwen://` DSN, e.g. +`LLM_QCN=qwen://token@dashscope.aliyuncs.com/compatible-mode/v1`. Model Studio +also fronts the same models with an Anthropic-compatible `/v1/messages` shim — +majordomo does **not** use it, because on that surface `reasoning_effort` is +dropped, `Request.Schema` stops being enforced, and cached-token accounting +disappears; see [ADR-0027](docs/adr/0027-qwen-builtin.md). Two Alibaba-side +quirks are worth knowing: thinking is on by default for some models (e.g. +`qwen3.7-plus`), and the Qwen3 open-source models require streaming while +thinking, so buffered `Generate` calls want a Max/Plus model. + OpenAI-compatible / Anthropic-compatible endpoints: construct the provider with a name and base URL and register it — @@ -165,7 +179,7 @@ m, _ := reg.Parse("m5/qwen3:30b,m1/qwen3:30b,thinking") ``` DSN format: `scheme://[token@]host[/path]`, scheme ∈ `foreman`, `ollama`, -`ollama-cloud`, `openai`, `kimi`, `anthropic`, `google`/`gemini`, `llama-swap`, +`ollama-cloud`, `openai`, `kimi`, `qwen`, `anthropic`, `google`/`gemini`, `llama-swap`, `llama-swaps`, or any scheme you add with `RegisterScheme`. The token is the credential (bearer token / API key); the base URL is always `https://host[/path]` — except `llama-swap`, which builds `http://host[:port]` since it's local-first @@ -407,6 +421,7 @@ to build one. |----------------------|:---:|:---:|:---:|:---:|:---:|:---:|:---:| | OpenAI (+compatible) | ✅ | ✅ | ✅ | ✅ | ✅ | ✅ | ✅ | | Kimi (Moonshot AI) | ✅ | ✅ | ✅ | ✅ | ✅ | ✅³ | ✅ | +| Qwen (Alibaba) | ✅ | ✅ | ✅ | ✅ | ✅⁴ | ✅⁴ | ✅ | | Anthropic (+compat) | ✅ | ✅ | ✅ | ✅ | ✅ | ✅ | ✅ | | Google (Gemini) | ✅ | ✅ | ✅ | ✅ | ✅ | ✅ | ✅ | | Ollama Cloud | ✅ | ✅ | ✅ | ✅ | ✅ | ✅ | ✅ | @@ -431,6 +446,13 @@ probe and management methods on `*llamaswap.Provider`. level; whether a call succeeds depends on the Moonshot model — only the vision variants (e.g. `moonshot-v1-8k-vision-preview`) accept images. +⁴ Qwen also reuses the openai client (ADR-0027), so both columns are present at +the client level and gated by the Model Studio model you name: `json_schema` +structured output is on the Max/Plus families, image inputs on the `qwen-vl-*` +/ `qwen3-vl-*` models. `reasoning_effort` rides through as a top-level field — +one reason the built-in speaks OpenAI-compat rather than Model Studio's +Anthropic-compat shim. + Notes: Ollama has no native tool_choice — `"none"` drops the tools; `"required"`/named choices are best-effort ignored there. Ollama Cloud ignores the `format` field (verified live), so the provider also states diff --git a/builtin.go b/builtin.go index 1b39c05..7b9996a 100644 --- a/builtin.go +++ b/builtin.go @@ -18,7 +18,16 @@ const ( // ProviderKimi is Moonshot AI's Kimi models over their OpenAI-compatible // Chat Completions endpoint. Reuses the openai client (like llama-swap); // keyed by KIMI_API_KEY, default base URL kimiBaseURL. - ProviderKimi = "kimi" + ProviderKimi = "kimi" + // 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. + ProviderQwen = "qwen" ProviderAnthropic = "anthropic" ProviderGoogle = "google" ProviderOllama = "ollama" @@ -37,6 +46,32 @@ const ( // China endpoint (api.moonshot.cn/v1) is reachable via a kimi:// LLM_* DSN. const kimiBaseURL = "https://api.moonshot.ai/v1" +// qwenBaseURL is Alibaba Model Studio's international (Singapore) endpoint in +// OpenAI-compatible mode. The China endpoint +// (dashscope.aliyuncs.com/compatible-mode/v1) and any regional host are +// reachable via a qwen:// LLM_* DSN. +const qwenBaseURL = "https://dashscope-intl.aliyuncs.com/compatible-mode/v1" + +// openaiCompatScheme builds the DSN factory shared by every built-in that is +// "the openai client pointed somewhere else" (kimi, qwen, ...). The provider +// is named after the LLM_ var that defined it, takes its credential from +// the DSN token — not the built-in's own env var, which does nothing for a +// DSN-defined provider — and so names that same LLM_ var in the +// missing-key hint, matching the lazy-resolution key form in providerFor. +// +// wrap is the caller's option-decorator (it injects the registry's HTTP +// client), so a DSN provider is built exactly like the eager built-ins. +func openaiCompatScheme(wrap func(...openai.Option) []openai.Option) SchemeFactory { + return func(name string, dsn DSN) (llm.Provider, error) { + return openai.New(wrap( + openai.WithName(name), + openai.WithBaseURL(dsn.BaseURL()), + openai.WithAPIKey(dsn.Token), + openai.WithAPIKeyName("LLM_"+strings.ToUpper(strings.ReplaceAll(name, "-", "_"))), + )...), nil + } +} + // registerBuiltins installs the built-in providers and env-DSN scheme // factories into a fresh registry. httpClient, when non-nil, is used by // every provider and factory the registry itself constructs. @@ -96,19 +131,26 @@ func registerBuiltins(r *Registry, httpClient *http.Client) { openai.WithAPIKeyName("KIMI_API_KEY"), )...) // kimi:// DSN scheme: an OpenAI-compatible target labeled kimi, base URL - // from the DSN host (e.g. kimi://tok@api.moonshot.cn/v1 for China). Its - // credential is the DSN token, not KIMI_API_KEY, so the missing-key hint - // names the LLM_ env var that defines this provider (matching the - // lazy-resolution key form in providerFor) — the fix for a keyless target - // here is adding a token to that DSN. - r.schemes[ProviderKimi] = func(name string, dsn DSN) (llm.Provider, error) { - return openai.New(openaiOpts( - openai.WithName(name), - openai.WithBaseURL(dsn.BaseURL()), - openai.WithAPIKey(dsn.Token), - openai.WithAPIKeyName("LLM_"+strings.ToUpper(strings.ReplaceAll(name, "-", "_"))), - )...), nil - } + // from the DSN host (e.g. kimi://tok@api.moonshot.cn/v1 for China). + r.schemes[ProviderKimi] = openaiCompatScheme(openaiOpts) + + // Qwen (Alibaba Model Studio): same shape as kimi — an OpenAI-compatible + // Chat Completions endpoint, so it reuses the openai client rather than a + // new package. Model Studio also exposes an Anthropic-compatible endpoint; + // ADR-0027 records why the OpenAI one is the built-in. Same unconditional + // WithAPIKey + WithAPIKeyName discipline as kimi: an unset QWEN_API_KEY + // must never fall through to OPENAI_API_KEY, and the missing-key error + // must name the variable the operator actually has to set. + r.providers[ProviderQwen] = openai.New(openaiOpts( + openai.WithName(ProviderQwen), + openai.WithBaseURL(qwenBaseURL), + openai.WithAPIKey(r.envLookup("QWEN_API_KEY")), + openai.WithAPIKeyName("QWEN_API_KEY"), + )...) + // qwen:// DSN scheme: an OpenAI-compatible target labeled qwen on any + // Model Studio host (e.g. qwen://tok@dashscope.aliyuncs.com/compatible-mode/v1 + // for China, or a workspace-scoped regional host). + r.schemes[ProviderQwen] = openaiCompatScheme(openaiOpts) // llama-swap: OpenAI-compatible chat + image generation + management // endpoints over a model-swapping proxy. Chat reuses the openai client diff --git a/builtin_kimi_test.go b/builtin_kimi_test.go index 7140ba0..8c9a9ca 100644 --- a/builtin_kimi_test.go +++ b/builtin_kimi_test.go @@ -16,16 +16,25 @@ import ( const kimiResponse = `{"id":"c1","object":"chat.completion","choices":[` + `{"index":0,"message":{"role":"assistant","content":"ok"},"finish_reason":"stop"}]}` -// captureRT records the last request and returns a canned response without -// touching the network, so these tests stay hermetic while still exercising -// the real openai client the kimi built-in reuses (base URL + auth header). +// captureRT records the last request (and the bytes of its body) and returns a +// canned response without touching the network, so these tests stay hermetic +// while still exercising the real openai client the kimi and qwen built-ins +// reuse: base URL, auth header, and the JSON actually put on the wire. type captureRT struct { - req *http.Request - body string + req *http.Request + reqBody []byte + body string } func (c *captureRT) RoundTrip(r *http.Request) (*http.Response, error) { c.req = r + // Drain and close the request body: a RoundTripper owns it, and those + // bytes are what wire-shape assertions read. + c.reqBody = nil + if r.Body != nil { + c.reqBody, _ = io.ReadAll(r.Body) + _ = r.Body.Close() + } return &http.Response{ StatusCode: http.StatusOK, Body: io.NopCloser(strings.NewReader(c.body)), diff --git a/builtin_qwen_test.go b/builtin_qwen_test.go new file mode 100644 index 0000000..063d5f4 --- /dev/null +++ b/builtin_qwen_test.go @@ -0,0 +1,235 @@ +package majordomo + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "strings" + "testing" + + "gitea.stevedudenhoeffer.com/steve/majordomo/llm" +) + +// qwenResponse is a minimal valid Chat Completions body so Generate returns a +// non-empty response (an empty one would trigger failover, not a clean pass). +const qwenResponse = `{"id":"c1","object":"chat.completion","choices":[` + + `{"index":0,"message":{"role":"assistant","content":"ok"},"finish_reason":"stop"}]}` + +// 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: qwenResponse} + r := newTestRegistry(t, + WithEnvLookup(func(k string) string { + if k == "QWEN_API_KEY" { + return "qwen-secret" + } + return "" + }), + 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-max") + if err != nil { + t.Fatalf("Parse: %v", err) + } + if got := targetsOf(t, m); len(got) != 1 || got[0] != "qwen/qwen3-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: qwenResponse} + r := newTestRegistry(t, WithHTTPClient(&http.Client{Transport: rt})) + + m, err := r.Parse("qwen/qwen3-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") + } +} + +// 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. +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 + // keyless openai target would 401 before any request, and the assertion + // below would pass without a single byte reaching the wire. + t.Setenv("OPENAI_API_KEY", "openai-secret") + + rt := &captureRT{body: qwenResponse} + r := newTestRegistry(t, + WithEnvLookup(func(k string) string { + if k == "QWEN_API_KEY" { + return "qwen-secret" + } + return "" + }), + WithHTTPClient(&http.Client{Transport: rt}), + ) + + m, err := r.Parse("openai/gpt-4o-mini") + 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 := "Bearer openai-secret"; rt.req.Header.Get("Authorization") != want { + t.Errorf("Authorization = %q, want %q — the qwen credential must not reach the openai built-in", + rt.req.Header.Get("Authorization"), want) + } +} + +// 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: qwenResponse} + 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/qwen-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: qwenResponse} + 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/qwen-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 +// sends — so llm.WithReasoningEffort survives the trip on qwen with no +// qwen-specific code. Routing qwen through the anthropic client instead would +// drop it silently (provider/anthropic ignores ReasoningEffort by design), and +// that difference would be invisible without asserting on the wire body. +func TestQwenReasoningEffortReachesWire(t *testing.T) { + rt := &captureRT{body: qwenResponse} + r := newTestRegistry(t, + WithEnvLookup(func(k string) string { + if k == "QWEN_API_KEY" { + return "qwen-secret" + } + return "" + }), + WithHTTPClient(&http.Client{Transport: rt}), + ) + + m, err := r.Parse("qwen/qwen3-max") + if err != nil { + t.Fatalf("Parse: %v", err) + } + _, err = m.Generate(context.Background(), llm.Request{ + Messages: []llm.Message{llm.UserText("hi")}, + ReasoningEffort: "high", + }) + if err != nil { + t.Fatalf("Generate: %v", err) + } + if rt.reqBody == nil { + t.Fatal("no request body captured") + } + var sent map[string]any + if err := json.Unmarshal(rt.reqBody, &sent); err != nil { + t.Fatalf("decode request body: %v", err) + } + if got := sent["reasoning_effort"]; got != "high" { + t.Errorf("reasoning_effort = %v, want %q (body: %s)", got, "high", rt.reqBody) + } +} diff --git a/docs/adr/0027-qwen-builtin.md b/docs/adr/0027-qwen-builtin.md new file mode 100644 index 0000000..e21c6a7 --- /dev/null +++ b/docs/adr/0027-qwen-builtin.md @@ -0,0 +1,102 @@ +# ADR-0027: Qwen (Alibaba) built-in provider — OpenAI-compat, not Anthropic-compat + +**Status:** Accepted — 2026-08-12 + +## Context + +Alibaba's Qwen models (`qwen3.8-max`, `qwen3.7-plus`, the `qwen3-vl-*` vision +variants, …) are served from Model Studio / DashScope, and mort wants them as a +first-class failover tier with a dedicated `QWEN_API_KEY` — the same ergonomics +ADR-0026 gave Kimi. + +Unlike Kimi, Model Studio exposes the same models over **two** protocols: + +| | OpenAI-compatible | Anthropic-compatible | +|---|---|---| +| Base URL | `https://dashscope-intl.aliyuncs.com/compatible-mode/v1` | `https://dashscope-intl.aliyuncs.com/apps/anthropic` | +| Endpoints | full Chat Completions surface | `/v1/messages` only (no `/v1/models`) | +| Purpose | the documented developer API | a shim, documented around hosting Claude Code | + +So the question this ADR answers is not "which client do we reuse" but +"which of Alibaba's two wire protocols does the built-in speak". + +## Decision + +**The `qwen` built-in and the `qwen://` DSN scheme speak OpenAI-compat**, over +`provider/openai` — no new package, mirroring ADR-0026 (kimi) and ADR-0015 +(llama-swap chat). Default base URL is the international host; the China host +(`dashscope.aliyuncs.com/compatible-mode/v1`) and workspace-scoped regional +hosts are reachable with a `qwen://` DSN. + +Credential handling is copied from kimi verbatim, because both of its rules +are load-bearing: `WithAPIKey` is passed unconditionally (even empty) so an +unset `QWEN_API_KEY` can never fall through to `openai.New`'s `OPENAI_API_KEY` +default, and `WithAPIKeyName("QWEN_API_KEY")` makes the synthetic-401 hint name +the variable the operator actually has to set. + +The kimi and qwen DSN factories were identical, so they now share one +`openaiCompatScheme` helper — the next OpenAI-compat built-in gets the +credential and key-hint rules by construction rather than by copy. + +### Why not the Anthropic-compatible endpoint + +Every concrete difference favors OpenAI-compat *for this codebase*: + +- **Reasoning survives the trip.** Model Studio takes `reasoning_effort` as a + top-level field on the OpenAI surface, which `provider/openai` already sends + — `llm.WithReasoningEffort` works on qwen with zero qwen-specific code + (`TestQwenReasoningEffortReachesWire` asserts it on the wire). Down the + anthropic client it would be dropped in silence: `provider/anthropic` + deliberately ignores `Request.ReasoningEffort`, because first-party Claude + has no such knob. +- **Structured output would regress.** `provider/anthropic` implements + `Request.Schema` with the first-party GA `output_config.format` mechanism. + Alibaba's shim does not implement it; a compat endpoint that ignores an + unknown field returns unconstrained prose while still reporting success. + The OpenAI path sends `response_format: json_schema`, which Model Studio + supports natively on the Max/Plus families. +- **Cache accounting already lands.** Model Studio's implicit prefix cache + reports hits in `usage.prompt_tokens_details.cached_tokens`, which the openai + client already maps to `llm.Usage.CacheReadTokens`. The anthropic client + reads `cache_read_input_tokens`, a field the shim has no reason to emit. +- **Thinking content is discarded on the anthropic path anyway.** + `provider/anthropic` skips `thinking` blocks in both the buffered and + streaming decoders, so the shim's headline feature — first-class + `thinking: {type: "enabled", budget_tokens: N}` — buys majordomo nothing + today. +- **Smaller blast radius.** The anthropic client has no `WithAPIKeyName` + option, so a keyless qwen would tell the operator to set `ANTHROPIC_API_KEY`; + fixing that means changing the first-party Anthropic client to serve a + third-party shim. +- **It is the less-exercised surface.** The Anthropic endpoint is documented as + Messages-only, with a temperature range that differs from Anthropic's own + ([0, 2) vs [0.0, 1.0]) — i.e. it is Qwen semantics wearing an Anthropic + envelope, not an Anthropic-equivalent target. + +The one thing the Anthropic surface offers that OpenAI-compat does not is +explicit `cache_control` breakpoints reached through `Request.PromptCache`. +That is not a reason to route Qwen through it: Model Studio's implicit cache is +automatic and already metered, and if explicit breakpoints ever matter they +belong in `provider/openai` (Model Studio accepts `cache_control` on content +blocks there too), where every OpenAI-compat target would get them. + +## Consequences + +- `qwen/` is first-class in Parse, chains, aliases, and health/failover + with no consumer wiring; model ids pass through verbatim (no catalog). +- Chat, streaming, tools, structured output, reasoning effort, and cached-token + accounting all ride the openai client and inherit its fixes. +- Image *inputs* work at the client level, but only the `qwen-vl-*` / + `qwen3-vl-*` models accept them (matrix footnote ³, shared with kimi). +- Two model-side quirks are Alibaba's, not majordomo's, and are left to the + caller rather than papered over: thinking is **on by default** on some models + (e.g. `qwen3.7-plus`), and Qwen3 *open-source* models require streaming when + thinking is enabled — a buffered `Generate` against one of those needs a + model that supports non-streaming thinking (the Max/Plus families do). +- If a future consumer genuinely needs the Anthropic surface, it is reachable + today without library changes: + `LLM_QWEN_ANTHROPIC=anthropic://token@dashscope-intl.aliyuncs.com/apps/anthropic` + — with the reasoning/structured-output caveats above. +- Second third-party built-in after kimi. The ADR-0026 bar still holds: a named + consumer needs it in-config. `RegisterProvider`/`LLM_*` remain the path for + everything else. diff --git a/docs/adr/README.md b/docs/adr/README.md index b75b582..5965921 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -30,3 +30,4 @@ One decision per file, append-only; supersede rather than rewrite. | [0024](0024-audio-wave3-surfaces.md) | Wave-3 audio surfaces (stems, SFX, speech enhance, voice clone, translate) | Accepted | | [0025](0025-videogen-wave3-surfaces.md) | Wave-3 video surfaces (lipsync, video matte, video upscale, chain jobs) | Accepted | | [0026](0026-kimi-builtin.md) | Kimi (Moonshot AI) built-in provider — reuse openai client, KIMI_API_KEY | Accepted | +| [0027](0027-qwen-builtin.md) | Qwen (Alibaba) built-in provider — OpenAI-compat, not Model Studio's Anthropic-compat endpoint | Accepted | diff --git a/progress.md b/progress.md index 300080d..df2331a 100644 --- a/progress.md +++ b/progress.md @@ -285,3 +285,32 @@ tests flush out. (footnote ³), `.env.example`, ADR-0026 (+ index; also backfilled the missing 0024/0025 index rows). - Consumer: mort names Kimi as a failover tier. + +## 2026-08-12 — Qwen (Alibaba) built-in provider (ADR-0027) + +- New built-in `qwen` provider + `qwen://` DSN scheme over Alibaba Model + Studio's OpenAI-compatible mode, reusing `provider/openai` (no new client, + mirrors kimi/llama-swap). Default base URL + `https://dashscope-intl.aliyuncs.com/compatible-mode/v1`; China/regional + hosts via `LLM_QCN=qwen://token@dashscope.aliyuncs.com/compatible-mode/v1`. +- Credential is `QWEN_API_KEY` (via the registry's injected envLookup). + `WithAPIKey` passed unconditionally so an unset key cannot fall through to + `OPENAI_API_KEY`; `WithAPIKeyName` names `QWEN_API_KEY` in the 401 hint. +- **Chose OpenAI-compat over Model Studio's Anthropic-compatible + `/apps/anthropic` shim** (ADR-0027): on the anthropic client + `ReasoningEffort` is ignored by design, `Request.Schema` rides + `output_config.format` (which the shim does not implement), and cached-token + accounting reads Anthropic-only usage fields. The shim is still reachable + ad hoc via an `anthropic://` DSN. +- kimi and qwen DSN factories were byte-identical, so they now share one + `openaiCompatScheme` helper — the credential + key-hint rules come by + construction, not by copy. +- Hermetic tests: built-in base URL + bearer, missing key names QWEN_API_KEY + with no OPENAI fallthrough and no network hit, the reverse leak (a visible + QWEN_API_KEY must not authenticate the openai built-in), `qwen://` round-trip + against the China host, its keyless hint naming LLM_QCN, and + `reasoning_effort` asserted on the wire body (the ADR's load-bearing claim). + captureRT now records the request body; all six break-checked. +- Docs in sync: README built-in table + Qwen paragraph + DSN scheme list + + support matrix (footnote ⁴), `.env.example`, ADR-0027 (+ index). +- Consumer: mort wants Qwen as a failover tier. From 31d6b593565fd7437160899486cd3618736f80a0 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Wed, 12 Aug 2026 16:16:54 -0400 Subject: [PATCH 2/6] =?UTF-8?q?refactor(test):=20gadfly=20round=201=20?= =?UTF-8?q?=E2=80=94=20share=20the=20OpenAI-compat=20test=20fixtures?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both findings were the same one, and both were fair: the PR that retires two byte-identical DSN factories into openaiCompatScheme then copy-pasted the test fixtures. qwenResponse was byte-identical to kimiResponse, and the single-key env-lookup closure appeared three times in the new file (plus a fourth in the kimi file, which neither reviewer was looking at). Fixed for the class rather than for qwen: captureRT, the canned Chat Completions body (now chatCompletionOK), and a new singleKeyEnv helper move to builtin_openaicompat_test.go, owned by no single provider. The kimi tests adopt them too, so the next OpenAI-compat built-in has nothing left to copy — the same argument the production helper makes. Also aligned the test model ids to the current Model Studio names (qwen3.8-max / qwen3.7-plus), which the docs already cited. One reviewer called those ids fictional and named the 2025 ones instead; they shipped 2026-08-03 and 2026-05-21 respectively, so that finding is stale model knowledge, not a defect — but having tests and prose name the same models removes the smell that prompted it. A dotted id also now proves it passes through verbatim. Break-checked again after the refactor: all six mutations still fail their test. Co-Authored-By: Claude Opus 5 (1M context) --- builtin_kimi_test.go | 48 ++++------------------------- builtin_openaicompat_test.go | 58 ++++++++++++++++++++++++++++++++++++ builtin_qwen_test.go | 50 ++++++++++--------------------- 3 files changed, 78 insertions(+), 78 deletions(-) create mode 100644 builtin_openaicompat_test.go diff --git a/builtin_kimi_test.go b/builtin_kimi_test.go index 8c9a9ca..3281a2e 100644 --- a/builtin_kimi_test.go +++ b/builtin_kimi_test.go @@ -3,7 +3,6 @@ package majordomo import ( "context" "errors" - "io" "net/http" "strings" "testing" @@ -11,49 +10,12 @@ import ( "gitea.stevedudenhoeffer.com/steve/majordomo/llm" ) -// kimiResponse is a minimal valid Chat Completions body so Generate returns a -// non-empty response (an empty one would trigger failover, not a clean pass). -const kimiResponse = `{"id":"c1","object":"chat.completion","choices":[` + - `{"index":0,"message":{"role":"assistant","content":"ok"},"finish_reason":"stop"}]}` - -// captureRT records the last request (and the bytes of its body) and returns a -// canned response without touching the network, so these tests stay hermetic -// while still exercising the real openai client the kimi and qwen built-ins -// reuse: base URL, auth header, and the JSON actually put on the wire. -type captureRT struct { - req *http.Request - reqBody []byte - body string -} - -func (c *captureRT) RoundTrip(r *http.Request) (*http.Response, error) { - c.req = r - // Drain and close the request body: a RoundTripper owns it, and those - // bytes are what wire-shape assertions read. - c.reqBody = nil - if r.Body != nil { - c.reqBody, _ = io.ReadAll(r.Body) - _ = r.Body.Close() - } - return &http.Response{ - StatusCode: http.StatusOK, - Body: io.NopCloser(strings.NewReader(c.body)), - Header: make(http.Header), - Request: r, - }, nil -} - // 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: kimiResponse} + rt := &captureRT{body: chatCompletionOK} r := newTestRegistry(t, - WithEnvLookup(func(k string) string { - if k == "KIMI_API_KEY" { - return "kimi-secret" - } - return "" - }), + WithEnvLookup(singleKeyEnv("KIMI_API_KEY", "kimi-secret")), WithHTTPClient(&http.Client{Transport: rt}), ) @@ -90,7 +52,7 @@ func TestKimiBuiltin(t *testing.T) { // 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: kimiResponse} + rt := &captureRT{body: chatCompletionOK} r := newTestRegistry(t, WithHTTPClient(&http.Client{Transport: rt})) m, err := r.Parse("kimi/kimi-k2-0711-preview") @@ -120,7 +82,7 @@ func TestKimiBuiltinMissingKey(t *testing.T) { // 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: kimiResponse} + 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", @@ -151,7 +113,7 @@ func TestKimiScheme(t *testing.T) { // defining LLM_ env var, never KIMI_API_KEY (which does nothing for a // DSN-defined provider). func TestKimiSchemeMissingToken(t *testing.T) { - rt := &captureRT{body: kimiResponse} + 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 diff --git a/builtin_openaicompat_test.go b/builtin_openaicompat_test.go new file mode 100644 index 0000000..9f66438 --- /dev/null +++ b/builtin_openaicompat_test.go @@ -0,0 +1,58 @@ +package majordomo + +import ( + "io" + "net/http" + "strings" +) + +// 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. + +// chatCompletionOK is a minimal valid Chat Completions body, so Generate +// returns a non-empty response (an empty one would trigger failover, not a +// clean pass). +const chatCompletionOK = `{"id":"c1","object":"chat.completion","choices":[` + + `{"index":0,"message":{"role":"assistant","content":"ok"},"finish_reason":"stop"}]}` + +// captureRT records the last request (and the bytes of its body) and returns a +// canned response without touching the network, so these tests stay hermetic +// while still exercising the real openai client the built-ins reuse: base URL, +// auth header, and the JSON actually put on the wire. +type captureRT struct { + req *http.Request + reqBody []byte + body string +} + +func (c *captureRT) RoundTrip(r *http.Request) (*http.Response, error) { + c.req = r + // Drain and close the request body: a RoundTripper owns it, and those + // bytes are what wire-shape assertions read. + c.reqBody = nil + if r.Body != nil { + c.reqBody, _ = io.ReadAll(r.Body) + _ = r.Body.Close() + } + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader(c.body)), + Header: make(http.Header), + Request: r, + }, nil +} + +// singleKeyEnv builds a WithEnvLookup function that knows exactly one variable +// and returns "" for everything else. The empty default has teeth: a built-in +// that reached for any other variable name gets nothing, so the request 401s +// and the test fails rather than quietly authenticating off the wrong key. +func singleKeyEnv(key, value string) func(string) string { + return func(k string) string { + if k == key { + return value + } + return "" + } +} diff --git a/builtin_qwen_test.go b/builtin_qwen_test.go index 063d5f4..afe7cb3 100644 --- a/builtin_qwen_test.go +++ b/builtin_qwen_test.go @@ -11,23 +11,13 @@ import ( "gitea.stevedudenhoeffer.com/steve/majordomo/llm" ) -// qwenResponse is a minimal valid Chat Completions body so Generate returns a -// non-empty response (an empty one would trigger failover, not a clean pass). -const qwenResponse = `{"id":"c1","object":"chat.completion","choices":[` + - `{"index":0,"message":{"role":"assistant","content":"ok"},"finish_reason":"stop"}]}` - // 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: qwenResponse} + rt := &captureRT{body: chatCompletionOK} r := newTestRegistry(t, - WithEnvLookup(func(k string) string { - if k == "QWEN_API_KEY" { - return "qwen-secret" - } - return "" - }), + WithEnvLookup(singleKeyEnv("QWEN_API_KEY", "qwen-secret")), WithHTTPClient(&http.Client{Transport: rt}), ) @@ -37,11 +27,11 @@ func TestQwenBuiltin(t *testing.T) { t.Errorf("name = %q, want %q", p.Name(), ProviderQwen) } - m, err := r.Parse("qwen/qwen3-max") + 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-max" { + if got := targetsOf(t, m); len(got) != 1 || got[0] != "qwen/qwen3.8-max" { t.Fatalf("targets = %v", got) } @@ -64,10 +54,10 @@ func TestQwenBuiltin(t *testing.T) { // 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: qwenResponse} + rt := &captureRT{body: chatCompletionOK} r := newTestRegistry(t, WithHTTPClient(&http.Client{Transport: rt})) - m, err := r.Parse("qwen/qwen3-max") + m, err := r.Parse("qwen/qwen3.8-max") if err != nil { t.Fatalf("Parse: %v", err) } @@ -102,14 +92,9 @@ func TestQwenBuiltinKeyDoesNotLeakToOpenAI(t *testing.T) { // below would pass without a single byte reaching the wire. t.Setenv("OPENAI_API_KEY", "openai-secret") - rt := &captureRT{body: qwenResponse} + rt := &captureRT{body: chatCompletionOK} r := newTestRegistry(t, - WithEnvLookup(func(k string) string { - if k == "QWEN_API_KEY" { - return "qwen-secret" - } - return "" - }), + WithEnvLookup(singleKeyEnv("QWEN_API_KEY", "qwen-secret")), WithHTTPClient(&http.Client{Transport: rt}), ) @@ -133,7 +118,7 @@ func TestQwenBuiltinKeyDoesNotLeakToOpenAI(t *testing.T) { // 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: qwenResponse} + 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", @@ -141,7 +126,7 @@ func TestQwenScheme(t *testing.T) { t.Fatalf("LoadEnv: %v", err) } - m, err := r.Parse("qcn/qwen-plus") + m, err := r.Parse("qcn/qwen3.7-plus") if err != nil { t.Fatalf("Parse: %v", err) } @@ -164,7 +149,7 @@ func TestQwenScheme(t *testing.T) { // 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: qwenResponse} + 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 @@ -172,7 +157,7 @@ func TestQwenSchemeMissingToken(t *testing.T) { t.Fatalf("LoadEnv: %v", err) } - m, err := r.Parse("qcn/qwen-plus") + m, err := r.Parse("qcn/qwen3.7-plus") if err != nil { t.Fatalf("Parse: %v", err) } @@ -200,18 +185,13 @@ func TestQwenSchemeMissingToken(t *testing.T) { // drop it silently (provider/anthropic ignores ReasoningEffort by design), and // that difference would be invisible without asserting on the wire body. func TestQwenReasoningEffortReachesWire(t *testing.T) { - rt := &captureRT{body: qwenResponse} + rt := &captureRT{body: chatCompletionOK} r := newTestRegistry(t, - WithEnvLookup(func(k string) string { - if k == "QWEN_API_KEY" { - return "qwen-secret" - } - return "" - }), + WithEnvLookup(singleKeyEnv("QWEN_API_KEY", "qwen-secret")), WithHTTPClient(&http.Client{Transport: rt}), ) - m, err := r.Parse("qwen/qwen3-max") + m, err := r.Parse("qwen/qwen3.8-max") if err != nil { t.Fatalf("Parse: %v", err) } From f1f2b653c33a1b618eeee975d6b1439d921d523d Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Wed, 12 Aug 2026 16:25:38 -0400 Subject: [PATCH 3/6] =?UTF-8?q?refactor:=20gadfly=20round=202=20=E2=80=94?= =?UTF-8?q?=20both=20halves=20of=20an=20OpenAI-compat=20built-in=20registe?= =?UTF-8?q?r=20together?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same class of finding as round 1, one level in: I factored the DSN-scheme half of the kimi/qwen duplication into openaiCompatScheme and left the eager provider half copy-pasted, so a third built-in still had six lines to clone — including both credential rules, which is exactly the pair you do not want re-typed. registerOpenAICompatBuiltin now installs both halves from one call. The rules that matter hold by construction for every future caller: WithAPIKey passed unconditionally (an unset key must not fall through to OPENAI_API_KEY), and WithAPIKeyName naming that same variable in the 401 hint. Registering kimi and qwen is now one line each. Also fixed a cross-reference the ADR got wrong: Qwen's image-input caveat is README matrix footnote ⁴, not ³ — ³ is kimi's. I wrote "³, shared with kimi" in the ADR and then gave Qwen its own footnote in the README. The break-check harness needed fixing before any of this could be trusted: three of its mutations targeted lines this refactor moved, so they matched nothing, the code was never broken, and the suite reported "test still passed" — identical output to a test that genuinely misses the bug. Mutations are now verified to have landed (sha before/after) and the suite fails loudly if one doesn't. Two new cases cover the helper: dropping the unconditional WithAPIKey, and dropping the scheme-half registration. 8/8 apply and are caught. Co-Authored-By: Claude Opus 5 (1M context) --- builtin.go | 69 ++++++++++++++++++----------------- docs/adr/0027-qwen-builtin.md | 2 +- 2 files changed, 37 insertions(+), 34 deletions(-) diff --git a/builtin.go b/builtin.go index 7b9996a..5d89ba1 100644 --- a/builtin.go +++ b/builtin.go @@ -72,6 +72,29 @@ func openaiCompatScheme(wrap func(...openai.Option) []openai.Option) SchemeFacto } } +// registerOpenAICompatBuiltin installs BOTH halves of an OpenAI-compat +// built-in: the eager provider under name (credential from keyEnv) and the +// matching name:// DSN scheme. Why both in one call: the two halves are a pair +// — a built-in whose scheme is missing resolves as a spec but not from an +// LLM_* DSN, and the credential rules below have to hold identically in each. +// Adding the next one is a single line rather than six lines to copy. +// +// The two credential rules, holding by construction for every caller: +// - WithAPIKey is passed UNCONDITIONALLY, even when the lookup comes back +// empty. openai.New defaults its key to OPENAI_API_KEY, so anything less +// lets an unset keyEnv silently authenticate as OpenAI. +// - WithAPIKeyName makes the synthetic-401 hint name keyEnv, so a keyless +// call tells the operator the variable that actually fixes it. +func registerOpenAICompatBuiltin(r *Registry, wrap func(...openai.Option) []openai.Option, name, baseURL, keyEnv string) { + r.providers[name] = openai.New(wrap( + openai.WithName(name), + openai.WithBaseURL(baseURL), + openai.WithAPIKey(r.envLookup(keyEnv)), + openai.WithAPIKeyName(keyEnv), + )...) + r.schemes[name] = openaiCompatScheme(wrap) +} + // registerBuiltins installs the built-in providers and env-DSN scheme // factories into a fresh registry. httpClient, when non-nil, is used by // every provider and factory the registry itself constructs. @@ -118,39 +141,19 @@ func registerBuiltins(r *Registry, httpClient *http.Client) { )...), nil } - // Kimi (Moonshot AI): OpenAI-compatible Chat Completions, so it reuses the - // openai client (like llama-swap). Defaults to Moonshot's international - // endpoint and the KIMI_API_KEY credential. WithAPIKey is passed - // unconditionally — even empty — so an unset KIMI_API_KEY can never fall - // through to the openai client's OPENAI_API_KEY default; WithAPIKeyName - // makes the missing-key error name KIMI_API_KEY. - r.providers[ProviderKimi] = openai.New(openaiOpts( - openai.WithName(ProviderKimi), - openai.WithBaseURL(kimiBaseURL), - openai.WithAPIKey(r.envLookup("KIMI_API_KEY")), - openai.WithAPIKeyName("KIMI_API_KEY"), - )...) - // kimi:// DSN scheme: an OpenAI-compatible target labeled kimi, base URL - // from the DSN host (e.g. kimi://tok@api.moonshot.cn/v1 for China). - r.schemes[ProviderKimi] = openaiCompatScheme(openaiOpts) - - // Qwen (Alibaba Model Studio): same shape as kimi — an OpenAI-compatible - // Chat Completions endpoint, so it reuses the openai client rather than a - // new package. Model Studio also exposes an Anthropic-compatible endpoint; - // ADR-0027 records why the OpenAI one is the built-in. Same unconditional - // WithAPIKey + WithAPIKeyName discipline as kimi: an unset QWEN_API_KEY - // must never fall through to OPENAI_API_KEY, and the missing-key error - // must name the variable the operator actually has to set. - r.providers[ProviderQwen] = openai.New(openaiOpts( - openai.WithName(ProviderQwen), - openai.WithBaseURL(qwenBaseURL), - openai.WithAPIKey(r.envLookup("QWEN_API_KEY")), - openai.WithAPIKeyName("QWEN_API_KEY"), - )...) - // qwen:// DSN scheme: an OpenAI-compatible target labeled qwen on any - // Model Studio host (e.g. qwen://tok@dashscope.aliyuncs.com/compatible-mode/v1 - // for China, or a workspace-scoped regional host). - r.schemes[ProviderQwen] = openaiCompatScheme(openaiOpts) + // Third-party endpoints that ARE the openai client at another base URL — + // no new package, mirroring llama-swap's chat path. Each gets the eager + // built-in plus its name:// DSN scheme, and the credential rules hold by + // construction (see registerOpenAICompatBuiltin). + // + // kimi (ADR-0026): Moonshot's international endpoint; China host via + // kimi://tok@api.moonshot.cn/v1. + registerOpenAICompatBuiltin(r, openaiOpts, ProviderKimi, kimiBaseURL, "KIMI_API_KEY") + // qwen (ADR-0027): Alibaba Model Studio's international host. Model Studio + // also exposes an Anthropic-compatible endpoint; the ADR records why the + // OpenAI one is the built-in. China / workspace-scoped regional hosts via + // qwen://tok@dashscope.aliyuncs.com/compatible-mode/v1. + registerOpenAICompatBuiltin(r, openaiOpts, ProviderQwen, qwenBaseURL, "QWEN_API_KEY") // llama-swap: OpenAI-compatible chat + image generation + management // endpoints over a model-swapping proxy. Chat reuses the openai client diff --git a/docs/adr/0027-qwen-builtin.md b/docs/adr/0027-qwen-builtin.md index e21c6a7..3a495d0 100644 --- a/docs/adr/0027-qwen-builtin.md +++ b/docs/adr/0027-qwen-builtin.md @@ -87,7 +87,7 @@ blocks there too), where every OpenAI-compat target would get them. - Chat, streaming, tools, structured output, reasoning effort, and cached-token accounting all ride the openai client and inherit its fixes. - Image *inputs* work at the client level, but only the `qwen-vl-*` / - `qwen3-vl-*` models accept them (matrix footnote ³, shared with kimi). + `qwen3-vl-*` models accept them (matrix footnote ⁴; ³ is kimi's). - Two model-side quirks are Alibaba's, not majordomo's, and are left to the caller rather than papered over: thinking is **on by default** on some models (e.g. `qwen3.7-plus`), and Qwen3 *open-source* models require streaming when From 8670ed22be6918eb29f652749a34308ad2a07c13 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Wed, 12 Aug 2026 16:40:53 -0400 Subject: [PATCH 4/6] =?UTF-8?q?refactor:=20gadfly=20round=203=20=E2=80=94?= =?UTF-8?q?=20one=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) From 0760cf96d4d5f880e2ee66349ad1ddfb5d77337a Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Wed, 12 Aug 2026 16:53:05 -0400 Subject: [PATCH 5/6] =?UTF-8?q?docs:=20gadfly=20round=204=20=E2=80=94=20tw?= =?UTF-8?q?o=20doc-comment=20fixes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DSN.Scheme's list named kimi but not qwen — the same sibling drift this PR keeps finding, in a doc comment this time (I updated the README's scheme list and not this one). Added qwen, and llama-swap/llama-swaps while there, since they were missing too. envKeyForProvider's example ran backwards: "LLM_M1 → m1" describes registry naming, not what the function does. Now reads name → variable in one direction. Not taking the third: the const block's alignment is gofmt's own output (gofmt -l is empty), and the uneven padding is forced by the doc comments that split the block into alignment groups. glm-5.2 reached that same conclusion in round 2 before flagging it here. Co-Authored-By: Claude Opus 5 (1M context) --- env.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/env.go b/env.go index 3397a9b..20c274b 100644 --- a/env.go +++ b/env.go @@ -26,8 +26,9 @@ var ErrUnknownProvider = errors.New("unknown provider") // authenticated with the bearer token "test-token". type DSN struct { // Scheme selects the provider implementation: "foreman", "ollama", - // "ollama-cloud", "openai", "kimi", "anthropic", "google"/"gemini", or - // any custom scheme registered with RegisterScheme. + // "ollama-cloud", "openai", "kimi", "qwen", "anthropic", + // "google"/"gemini", "llama-swap"/"llama-swaps", or any custom scheme + // registered with RegisterScheme. Scheme string // Token is the provider secret (bearer token or API key); empty = none. Token string @@ -41,7 +42,7 @@ type DSN struct { 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). +// name: "m1" → LLM_M1, "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 From f8ced9c6297d3f3fd3c0a26c5a9793195dedad01 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Wed, 12 Aug 2026 16:57:33 -0400 Subject: [PATCH 6/6] docs(progress): describe the shape this PR actually landed in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The progress entry was written before four review rounds reshaped the change: it credited openaiCompatScheme alone, listed the tests as six per-provider cases, and mentioned a captureRT detail that has since moved. Rewritten to match what merges — registerOpenAICompatBuiltin owning both halves, envKeyForProvider as the single LLM_ definition, and the shared table that every OpenAI-compat built-in is now checked against. Co-Authored-By: Claude Opus 5 (1M context) --- progress.md | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/progress.md b/progress.md index df2331a..e69f874 100644 --- a/progress.md +++ b/progress.md @@ -302,15 +302,22 @@ tests flush out. `output_config.format` (which the shim does not implement), and cached-token accounting reads Anthropic-only usage fields. The shim is still reachable ad hoc via an `anthropic://` DSN. -- kimi and qwen DSN factories were byte-identical, so they now share one - `openaiCompatScheme` helper — the credential + key-hint rules come by - construction, not by copy. -- Hermetic tests: built-in base URL + bearer, missing key names QWEN_API_KEY - with no OPENAI fallthrough and no network hit, the reverse leak (a visible - QWEN_API_KEY must not authenticate the openai built-in), `qwen://` round-trip - against the China host, its keyless hint naming LLM_QCN, and - `reasoning_effort` asserted on the wire body (the ADR's load-bearing claim). - captureRT now records the request body; all six break-checked. +- `registerOpenAICompatBuiltin` installs BOTH halves of an OpenAI-compat + built-in (eager provider + `name://` DSN scheme via the shared + `openaiCompatScheme`), so the two credential rules — unconditional + `WithAPIKey`, and `WithAPIKeyName` naming that same variable — hold by + construction. kimi and qwen are one line each. +- `envKeyForProvider` is the single definition of the `LLM_` form, + shared by lazy resolution (`registry.go`) and the DSN missing-key hint. They + were separate copies with a comment asserting they matched. +- The shared contract is ONE table (`builtin_openaicompat_test.go`), run + identically for every OpenAI-compat built-in: endpoint + bearer, missing key + fails closed naming its own variable with no network hit, the `name://` DSN + reaching another host, and a keyless DSN naming `LLM_` rather than the + built-in's key. Adding a built-in is a table row that immediately owes all + four; `builtin_kimi_test.go` was retired into it. Qwen-only tests: the + reverse credential leak, and `reasoning_effort` asserted on the wire body + (the ADR's load-bearing claim). - Docs in sync: README built-in table + Qwen paragraph + DSN scheme list + support matrix (footnote ⁴), `.env.example`, ADR-0027 (+ index). - Consumer: mort wants Qwen as a failover tier.