From f1f2b653c33a1b618eeee975d6b1439d921d523d Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Wed, 12 Aug 2026 16:25:38 -0400 Subject: [PATCH] =?UTF-8?q?refactor:=20gadfly=20round=202=20=E2=80=94=20bo?= =?UTF-8?q?th=20halves=20of=20an=20OpenAI-compat=20built-in=20register=20t?= =?UTF-8?q?ogether?= 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