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]>
241 lines
8.6 KiB
Go
241 lines
8.6 KiB
Go
package majordomo
|
|
|
|
import (
|
|
"context"
|
|
"errors"
|
|
"io"
|
|
"net/http"
|
|
"strings"
|
|
"testing"
|
|
|
|
"gitea.stevedudenhoeffer.com/steve/majordomo/llm"
|
|
)
|
|
|
|
// 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
|
|
// 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 ""
|
|
}
|
|
}
|
|
|
|
// 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_<dsnVar> 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_<NAME> 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")
|
|
}
|
|
})
|
|
}
|
|
}
|