From fcbb01b729813ffd463f5de3a896e0f58c46d19d Mon Sep 17 00:00:00 2001
From: Steve Dudenhoeffer <steve@stevedudenhoeffer.com>
Date: Sat, 18 Jul 2026 03:24:48 -0400
Subject: [PATCH] fix: address Gadfly findings on kimi provider
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- 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) <noreply@anthropic.com>
---
 builtin.go           |  9 +++++++--
 builtin_kimi_test.go | 33 +++++++++++++++++++++++++++++++++
 parse_test.go        |  4 +++-
 3 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/builtin.go b/builtin.go
index 02fa3c6..1b39c05 100644
--- a/builtin.go
+++ b/builtin.go
@@ -2,6 +2,7 @@ package majordomo
 
 import (
 	"net/http"
+	"strings"
 
 	"gitea.stevedudenhoeffer.com/steve/majordomo/llm"
 	"gitea.stevedudenhoeffer.com/steve/majordomo/provider/anthropic"
@@ -95,13 +96,17 @@ 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).
+	// 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_<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) {
 		return openai.New(openaiOpts(
 			openai.WithName(name),
 			openai.WithBaseURL(dsn.BaseURL()),
 			openai.WithAPIKey(dsn.Token),
-			openai.WithAPIKeyName("KIMI_API_KEY"),
+			openai.WithAPIKeyName("LLM_"+strings.ToUpper(strings.ReplaceAll(name, "-", "_"))),
 		)...), nil
 	}
 
diff --git a/builtin_kimi_test.go b/builtin_kimi_test.go
index 18b41dd..7140ba0 100644
--- a/builtin_kimi_test.go
+++ b/builtin_kimi_test.go
@@ -136,3 +136,36 @@ func TestKimiScheme(t *testing.T) {
 		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")
+	}
+}
diff --git a/parse_test.go b/parse_test.go
index 53d2bc3..e338490 100644
--- a/parse_test.go
+++ b/parse_test.go
@@ -213,7 +213,9 @@ func TestBuiltinsResolve(t *testing.T) {
 	r := newTestRegistry(t)
 	// All built-in provider names resolve even before their client
 	// 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 {
 			t.Errorf("Parse(%s/anything): %v", name, err)
 		}