fix(qwen): one credential rule for both paths — seven findings said so

Fourteen findings, and seven of them from all four models are the same one:
endpointProvider was missing the no-cross-vendor-fallback guard I had just
added to resolveModel. I fixed a credential leak on one path and left its
sibling leaking, in the commit whose own message argued those two paths must
move together. That is the third time in this PR.

So it is no longer a rule written twice. openAICompatOptions owns it and both
paths call it; builtinCompatProviders names the vendors that must never inherit
OPENAI_API_KEY, replacing a `provider == "kimi" || provider == "qwen"` literal
that was a fourth uncounted copy of the list.

The test drives a real request at a local server and demands two things: that
no request arrives carrying the OpenAI key, AND that the call fails closed
naming the variable to set — the second half because my first draft pointed the
provider at vendor.example, so the server saw nothing and the assertion held
for a reason unrelated to the fix. Break-checked: removing the guard puts
"Bearer sk-openai-must-not-travel" on the wire to the other vendor.

The scrub check failed open. As a bare condition, a grep ERROR (exit >= 2)
reads as "not found" and skips the guard — a credential check that passes
precisely when it cannot see the filesystem it is searching. It now
distinguishes 0/1/>=2 and refuses to continue on error.

A bare "claude-code" spec has no "/", so the provider fell back to ollama-cloud
and the pre-flight would skip a reviewer that authenticates with
CLAUDE_CODE_OAUTH_TOKEN and needs no Ollama key. Engine specs are now exempt.

preflight.sh's provider list duplicated its own case arms; both now read one
table. And its comment claimed the Go cross-check fails if either list misses
an entry from the other, when only one direction is checked — the reverse is
not even desirable, since ollama-cloud and anthropic belong in that table and
not in the Go one. The comment now says what is enforced.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

cmd/gadfly/model.go

@@ -33,10 +33,38 @@ const defaultProvider = "ollama-cloud"
 // guess.
 var openAICompatProviders = []string{"openai", "openai-compatible", "kimi", "qwen"}
 
+// builtinCompatProviders are the openai-compat names that belong to a DIFFERENT
+// vendor. openai.New defaults its credential to OPENAI_API_KEY, so any of these
+// constructed without an explicit key would put an OpenAI key on the wire to
+// Moonshot or Alibaba. Membership here means "pass the key unconditionally,
+// even empty" — an absent key must be a 401, never a foreign credential.
+var builtinCompatProviders = []string{"kimi", "qwen"}
+
 func isOpenAICompatProvider(name string) bool {
 	return slices.Contains(openAICompatProviders, name)
 }
 
+// openAICompatOptions builds the option set for an openai-compat provider, and
+// is the ONE place the no-cross-vendor-fallback rule lives.
+//
+// Both resolution paths call it — resolveModel's GADFLY_BASE_URL override and
+// endpointProvider's GADFLY_ENDPOINT_* parser. They had separate copies of this
+// decision once, the guard was added to one of them, and the other kept leaking
+// OPENAI_API_KEY to another vendor. keyHint names the variable to set when the
+// key is absent, since the two paths take it from different places.
+func openAICompatOptions(provider, baseURL, key, keyHint string) []openai.Option {
+	opts := []openai.Option{openai.WithBaseURL(baseURL)}
+	switch {
+	case slices.Contains(builtinCompatProviders, provider):
+		opts = append(opts, openai.WithAPIKey(key), openai.WithAPIKeyName(keyHint))
+	case key != "":
+		opts = append(opts, openai.WithAPIKey(key))
+		// openai/openai-compatible with no explicit key keep openai.New's
+		// OPENAI_API_KEY default: for those names it IS the right key.
+	}
+	return opts
+}
+
 // endpointProviderNames is the operator-facing list of providers that accept an
 // explicit endpoint. resolveModel and endpointProvider accept the SAME set, so
 // one message serves both rather than each carrying a copy that drifts in
@@ -106,25 +134,7 @@ func resolveModel() (llm.Model, error) {
 	// credential rule — assuming they do produces a config that passes every
 	// check and then 401s.
 	if isOpenAICompatProvider(provider) {
-		opts := []openai.Option{openai.WithBaseURL(baseURL)}
-		switch {
-		case provider == "kimi" || provider == "qwen":
-			// Pass the key UNCONDITIONALLY, even when empty. openai.New defaults
-			// its credential to OPENAI_API_KEY, so omitting the option sends an
-			// OpenAI key to Moonshot or Alibaba — a credential handed to the
-			// wrong vendor, which is exactly what majordomo's built-ins go out
-			// of their way to prevent. An empty key instead yields a synthetic
-			// 401 naming the knob that fixes it.
-			opts = append(opts,
-				openai.WithAPIKey(apiKey),
-				openai.WithAPIKeyName("GADFLY_API_KEY"),
-			)
-		case apiKey != "":
-			opts = append(opts, openai.WithAPIKey(apiKey))
-			// openai/openai-compatible with no explicit key keep the
-			// OPENAI_API_KEY default: for those names it IS the right key.
-		}
-		return openai.New(opts...).Model(model)
+		return openai.New(openAICompatOptions(provider, baseURL, apiKey, "GADFLY_API_KEY")...).Model(model)
 	}
 
 	switch provider {
@@ -299,10 +309,10 @@ func endpointProvider(name, raw string) (llm.Provider, error) {
 	// Same shared predicate as resolveModel: the two must accept an identical
 	// set, and a hand-copied case list cannot guarantee that.
 	if isOpenAICompatProvider(provider) {
-		opts := []openai.Option{openai.WithName(name), openai.WithBaseURL(baseURL)}
-		if key != "" {
-			opts = append(opts, openai.WithAPIKey(key))
-		}
+		// The key for a named endpoint comes from the third DSN field, so that
+		// is what an absent one points at.
+		opts := append([]openai.Option{openai.WithName(name)},
+			openAICompatOptions(provider, baseURL, key, "GADFLY_ENDPOINT_"+strings.ToUpper(name))...)
 		return openai.New(opts...), nil
 	}
 

cmd/gadfly/model_test.go

@@ -1,10 +1,15 @@
 package main
 
 import (
+	"context"
+	"net/http"
+	"net/http/httptest"
 	"os/exec"
 	"path/filepath"
 	"strings"
 	"testing"
+
+	llm "gitea.stevedudenhoeffer.com/steve/majordomo/llm"
 )
 
 func TestEndpointProvider(t *testing.T) {
@@ -226,3 +231,89 @@ func TestBuiltinCompatProvidersResolveViaRegistry(t *testing.T) {
 		})
 	}
 }
+
+// TestBuiltinCompatProvidersNeverInheritOpenAIKey pins the rule that has now
+// been broken on one path or the other three separate times: kimi and qwen are
+// other vendors, openai.New defaults its credential to OPENAI_API_KEY, and a
+// provider built without an explicit key therefore puts an OpenAI key on the
+// wire to Moonshot or Alibaba.
+//
+// Both construction paths are asserted from one loop deliberately. Each time
+// this was fixed on a single path the sibling kept leaking, so a test covering
+// one of them would have passed through every one of those bugs.
+//
+// The provider is pointed at a LOCAL server, and the test demands two things:
+// that no request arrives carrying the foreign key, and that the call fails
+// closed naming the variable to set. Without the second half the test would
+// pass on a provider that simply did nothing.
+func TestBuiltinCompatProvidersNeverInheritOpenAIKey(t *testing.T) {
+	const foreign = "sk-openai-must-not-travel"
+
+	for _, provider := range builtinCompatProviders {
+		t.Run(provider+" via GADFLY_BASE_URL", func(t *testing.T) {
+			srv, seen := leakServer(t)
+			t.Setenv("OPENAI_API_KEY", foreign)
+			t.Setenv("GADFLY_PROVIDER", provider)
+			t.Setenv("GADFLY_BASE_URL", srv.URL+"/v1")
+			t.Setenv("GADFLY_API_KEY", "") // the operator forgot the key
+			t.Setenv("GADFLY_MODEL", "some-model")
+
+			m, err := resolveModel()
+			if err != nil {
+				t.Fatalf("resolveModel: %v", err)
+			}
+			assertFailsClosed(t, m, seen, foreign, "GADFLY_API_KEY")
+		})
+
+		t.Run(provider+" via GADFLY_ENDPOINT_*", func(t *testing.T) {
+			srv, seen := leakServer(t)
+			t.Setenv("OPENAI_API_KEY", foreign)
+			p, err := endpointProvider("ep", provider+"|"+srv.URL+"/v1") // no key field
+			if err != nil {
+				t.Fatalf("endpointProvider: %v", err)
+			}
+			m, err := p.Model("some-model")
+			if err != nil {
+				t.Fatalf("Model: %v", err)
+			}
+			assertFailsClosed(t, m, seen, foreign, "GADFLY_ENDPOINT_EP")
+		})
+	}
+}
+
+// leakServer returns a server that records every Authorization header it is
+// sent. A request arriving at all means the client did not fail closed.
+func leakServer(t *testing.T) (*httptest.Server, *[]string) {
+	t.Helper()
+	var seen []string
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		seen = append(seen, r.Header.Get("Authorization"))
+		w.Header().Set("Content-Type", "application/json")
+		_, _ = w.Write([]byte(`{"id":"c1","object":"chat.completion","choices":[{"index":0,` +
+			`"message":{"role":"assistant","content":"ok"},"finish_reason":"stop"}]}`))
+	}))
+	t.Cleanup(srv.Close)
+	return srv, &seen
+}
+
+func assertFailsClosed(t *testing.T, m llm.Model, seen *[]string, foreign, wantHint string) {
+	t.Helper()
+	_, err := m.Generate(context.Background(), llm.Request{Messages: []llm.Message{llm.UserText("hi")}})
+
+	for _, auth := range *seen {
+		if strings.Contains(auth, foreign) {
+			t.Errorf("Authorization carried the OpenAI key to another vendor: %q", auth)
+		}
+	}
+	if len(*seen) > 0 {
+		t.Errorf("a keyless %s provider reached the network (%d request(s)) instead of failing closed", wantHint, len(*seen))
+	}
+	// The positive half: prove it refused for the right reason, so the test
+	// cannot pass on a provider that quietly did nothing at all.
+	if err == nil {
+		t.Fatal("keyless provider returned no error; expected a missing-key failure")
+	}
+	if !strings.Contains(err.Error(), wantHint) {
+		t.Errorf("error = %v, want it to name %s so the operator knows what to set", err, wantHint)
+	}
+}