refactor: gadfly round 3 — one table owns the OpenAI-compat contract
CI / Tidy (pull_request) Successful in 9m21s
CI / Build & Test (pull_request) Successful in 10m23s

Three findings, and the first two are the same recurring shape.

envKeyForProvider (env.go) is now the single definition of the LLM_<NAME>
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_<NAME> 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) <[email protected]>
This commit is contained in:
2026-08-12 16:40:53 -04:00
co-authored by Claude Opus 5
parent f1f2b653c3
commit 8670ed22be
6 changed files with 211 additions and 291 deletions
+8 -137
View File
@@ -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://[email protected]/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_<NAME> 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