fix: address Gadfly findings on kimi provider
- kimi:// DSN scheme: missing-credential hint now names the LLM_<NAME> env var that defines the provider (its token comes from the DSN, not KIMI_API_KEY), matching providerFor's lazy-resolution key form. Fixes the correctness/error-handling findings that the old hint misdirected users to set KIMI_API_KEY when the fix is adding a token to the DSN. - parse_test.go: add kimi to TestBuiltinsResolve. (llama-swap stays excluded and is now documented — its no-URL built-in errors at Model() construction, not just on use, so it can't resolve there; the finding's llama-swap half was a false lead the test surfaced.) - Add TestKimiSchemeMissingToken covering the corrected hint. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
This commit is contained in:
+7
-2
@@ -2,6 +2,7 @@ package majordomo
|
|||||||
|
|
||||||
import (
|
import (
|
||||||
"net/http"
|
"net/http"
|
||||||
|
"strings"
|
||||||
|
|
||||||
"gitea.stevedudenhoeffer.com/steve/majordomo/llm"
|
"gitea.stevedudenhoeffer.com/steve/majordomo/llm"
|
||||||
"gitea.stevedudenhoeffer.com/steve/majordomo/provider/anthropic"
|
"gitea.stevedudenhoeffer.com/steve/majordomo/provider/anthropic"
|
||||||
@@ -95,13 +96,17 @@ func registerBuiltins(r *Registry, httpClient *http.Client) {
|
|||||||
openai.WithAPIKeyName("KIMI_API_KEY"),
|
openai.WithAPIKeyName("KIMI_API_KEY"),
|
||||||
)...)
|
)...)
|
||||||
// kimi:// DSN scheme: an OpenAI-compatible target labeled kimi, base URL
|
// kimi:// DSN scheme: an OpenAI-compatible target labeled kimi, base URL
|
||||||
// from the DSN host (e.g. kimi://[email protected]/v1 for China).
|
// from the DSN host (e.g. kimi://[email protected]/v1 for China). Its
|
||||||
|
// credential is the DSN token, not KIMI_API_KEY, so the missing-key hint
|
||||||
|
// names the LLM_<NAME> 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) {
|
r.schemes[ProviderKimi] = func(name string, dsn DSN) (llm.Provider, error) {
|
||||||
return openai.New(openaiOpts(
|
return openai.New(openaiOpts(
|
||||||
openai.WithName(name),
|
openai.WithName(name),
|
||||||
openai.WithBaseURL(dsn.BaseURL()),
|
openai.WithBaseURL(dsn.BaseURL()),
|
||||||
openai.WithAPIKey(dsn.Token),
|
openai.WithAPIKey(dsn.Token),
|
||||||
openai.WithAPIKeyName("KIMI_API_KEY"),
|
openai.WithAPIKeyName("LLM_"+strings.ToUpper(strings.ReplaceAll(name, "-", "_"))),
|
||||||
)...), nil
|
)...), nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -136,3 +136,36 @@ func TestKimiScheme(t *testing.T) {
|
|||||||
t.Errorf("Authorization = %q, want %q", 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_<NAME> env var, never KIMI_API_KEY (which does nothing for a
|
||||||
|
// DSN-defined provider).
|
||||||
|
func TestKimiSchemeMissingToken(t *testing.T) {
|
||||||
|
rt := &captureRT{body: kimiResponse}
|
||||||
|
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")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
+3
-1
@@ -213,7 +213,9 @@ func TestBuiltinsResolve(t *testing.T) {
|
|||||||
r := newTestRegistry(t)
|
r := newTestRegistry(t)
|
||||||
// All built-in provider names resolve even before their client
|
// All built-in provider names resolve even before their client
|
||||||
// implementations land (stub providers error only on use).
|
// implementations land (stub providers error only on use).
|
||||||
for _, name := range []string{"openai", "anthropic", "google", "ollama", "ollama-cloud", "foreman"} {
|
// Note: llama-swap is intentionally excluded — its no-URL built-in errors
|
||||||
|
// at Model() construction (not just on use), so it can't resolve here.
|
||||||
|
for _, name := range []string{"openai", "kimi", "anthropic", "google", "ollama", "ollama-cloud", "foreman"} {
|
||||||
if _, err := r.Parse(name + "/anything"); err != nil {
|
if _, err := r.Parse(name + "/anything"); err != nil {
|
||||||
t.Errorf("Parse(%s/anything): %v", name, err)
|
t.Errorf("Parse(%s/anything): %v", name, err)
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user