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]>

.gitea/workflows/build-image.yml

@@ -108,12 +108,21 @@ jobs:
           # write ~/.netrc or ~/.config/go/env. Guarded on a non-empty secret:
           # `grep -F ""` matches every file, so a secretless run (fork PR) would
           # fail here with a message accusing it of leaking nothing.
-          # -e, so a password beginning with "-" is a pattern and not options:
-          # without it the check errors out and, under `set -e`, fails the step
-          # with a message about grep usage rather than about credentials.
-          if [ -n "${REGISTRY_PASSWORD:-}" ] && grep -rqF -e "$REGISTRY_PASSWORD" "$HOME" 2>/dev/null; then
-            echo "::error::registry credential still present under \$HOME after scrub"
-            exit 1
+          # -e, so a password beginning with "-" is a pattern and not options.
+          # And distinguish grep's three exits: 0 found, 1 clean, >=2 ERROR. As
+          # a bare condition an error reads as "not found" and the guard is
+          # skipped — a check that fails OPEN in exactly the case where it can no
+          # longer see the filesystem it is supposed to be searching.
+          if [ -n "${REGISTRY_PASSWORD:-}" ]; then
+            set +e
+            grep -rqF -e "$REGISTRY_PASSWORD" "$HOME" 2>/dev/null
+            rc=$?
+            set -e
+            case "$rc" in
+              0) echo "::error::registry credential still present under \$HOME after scrub"; exit 1 ;;
+              1) : ;;  # clean
+              *) echo "::error::credential scrub check could not run (grep exit $rc); refusing to continue"; exit 1 ;;
+            esac
           fi
 
       # GOPROXY=off from here on: the module cache is already warm, so any

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)
+	}
+}

scripts/preflight.sh

@@ -24,7 +24,15 @@
 # override-path config is hand-written, while the registry path is what somebody
 # hits by adding a model id to a var and forgetting the secret.
 gadfly_preflight_key() {
-  local provider="$1" key_env="" key_hint=""
+  local provider="$1" model="${2:-}" key_env="" key_hint=""
+
+  # Engine specs are not majordomo providers and carry their own auth. A bare
+  # "claude-code" has no "/" so the caller's provider falls back to
+  # ollama-cloud, which would skip a reviewer that authenticates with
+  # CLAUDE_CODE_OAUTH_TOKEN and needs no Ollama key at all.
+  case "$model" in
+    claude-code|claude-code/*|opencode/*) echo ""; return 0 ;;
+  esac
 
   # Only the registry path has knowable credential rules — see above.
   # Trim before testing: resolveModel does strings.TrimSpace on GADFLY_BASE_URL,
@@ -54,22 +62,16 @@ gadfly_preflight_key() {
   # variable the operator actually sets; the check reads the one the code uses.
   # If that copy ever moves after this call, this arm reports a missing key for
   # a configured run.
-  case "$provider" in
-    ollama-cloud)              key_env="OLLAMA_API_KEY" ;;
-    qwen)                      key_env="QWEN_API_KEY" ;;
-    kimi)                      key_env="KIMI_API_KEY" ;;
-    openai|openai-compatible)  key_env="OPENAI_API_KEY" ;;
-    anthropic)                 key_env="ANTHROPIC_API_KEY" ;;
-  esac
-  # The hint is the variable the operator sets, which equals the one the code
-  # reads everywhere except ollama-cloud (see the note above).
-  key_hint="$key_env"
-  [ "$provider" = "ollama-cloud" ] && key_hint="OLLAMA_CLOUD_API_KEY"
-
-  if [ -z "$key_env" ]; then
+  local row
+  row="$(_gadfly_preflight_table | awk -F: -v p="$provider" '$1 == p {print; exit}')"
+  if [ -z "$row" ]; then
     echo ""                       # provider needs no pre-flight
     return 0
   fi
+  key_env="$(printf '%s' "$row" | cut -d: -f2)"
+  key_hint="$(printf '%s' "$row" | cut -d: -f3)"
+  [ -n "$key_hint" ] || key_hint="$key_env"
+
   # Indirect expansion (bash). Each majordomo built-in reads ONLY its own
   # variable — cross-provider fallback is refused by design — so the named hint
   # is always the actual fix.
@@ -80,17 +82,42 @@ gadfly_preflight_key() {
   echo "$key_hint"
 }
 
-# gadfly_preflight_providers echoes every provider this file has a credential
-# arm for, one per line.
+# _gadfly_preflight_table is the single source for both the credential lookup
+# and the provider list: "<provider>:<env-var-read>:<env-var-to-suggest>".
 #
-# It exists so callers can ASK which providers are covered instead of parsing
-# the case statement. A Go test cross-checks this list against the provider
-# table in cmd/gadfly/model.go; having it regex this file would make the shell
-# formatting a contract no linter enforces, where a reformat breaks a test in
-# another language for no visible reason.
+# The third field is normally empty, meaning "same as the second". ollama-cloud
+# is the exception: run.sh copies the consumer-facing OLLAMA_CLOUD_API_KEY onto
+# the OLLAMA_API_KEY the provider reads BEFORE calling in here, so the check and
+# the hint name different variables on purpose. If that copy ever moves after
+# the call, this arm reports a missing key for a configured run.
 #
-# Keep in step with the case arms above — the Go test fails if a provider in
-# either list is missing from the other.
+# A provider absent from this table is absent for one of TWO reasons — do not
+# assume the first and add a row:
+#   1. It needs no key, or carries one in its endpoint/DSN: local ollama,
+#      llama-swap, foreman.
+#   2. It needs a key but accepts more than one variable, so a single-name check
+#      would skip a correctly-configured run. **google** is this case
+#      (GOOGLE_API_KEY *or* GEMINI_API_KEY); pre-flighting it needs an
+#      either-variable check, not this table's one-name shape.
+_gadfly_preflight_table() {
+  printf '%s\n' \
+    'ollama-cloud:OLLAMA_API_KEY:OLLAMA_CLOUD_API_KEY' \
+    'qwen:QWEN_API_KEY:' \
+    'kimi:KIMI_API_KEY:' \
+    'openai:OPENAI_API_KEY:' \
+    'openai-compatible:OPENAI_API_KEY:' \
+    'anthropic:ANTHROPIC_API_KEY:'
+}
+
+# gadfly_preflight_providers echoes every provider covered above, one per line.
+# Callers ASK rather than parse: a Go test cross-checks this against the
+# openai-compat provider table in cmd/gadfly/model.go, and regexing this file
+# would make its formatting a contract no linter enforces.
+#
+# The cross-check runs ONE direction — every openai-compat provider in Go must
+# appear here. The reverse is not required and must not be asserted:
+# ollama-cloud and anthropic belong in this table and are deliberately not in
+# that Go list.
 gadfly_preflight_providers() {
-  printf '%s\n' ollama-cloud qwen kimi openai openai-compatible anthropic
+  _gadfly_preflight_table | cut -d: -f1
 }

scripts/preflight_test.sh

@@ -25,11 +25,11 @@ check() { # description, want, got
 # under the same shell options production uses (set -u), so an unset-variable
 # bug surfaces here instead of in a live review.
 probe() {
-  local provider="$1"; shift
+  local provider="$1" model="${GADFLY_TEST_MODEL:-}"; shift
   env -i PATH="$PATH" HOME="$HOME" "$@" bash -c "
     set -u
     . '$SCRIPT_DIR/preflight.sh'
-    gadfly_preflight_key '$provider'
+    gadfly_preflight_key '$provider' '$model'
   "
 }
 
@@ -84,6 +84,16 @@ echo "== a whitespace-only GADFLY_BASE_URL counts as unset, as it does in Go =="
 # disagreed, the missing key would arrive as a bare 401 with no skip notice.
 check "qwen + blank BASE_URL" "QWEN_API_KEY" "$(probe qwen GADFLY_BASE_URL="   ")"
 
+echo "== engine specs carry their own auth and are never pre-flighted =="
+# A bare "claude-code" has no "/", so the caller's provider falls back to
+# ollama-cloud; judging it by that would skip a reviewer using
+# CLAUDE_CODE_OAUTH_TOKEN, which needs no Ollama key.
+check "bare claude-code, no ollama key" "" "$(GADFLY_TEST_MODEL=claude-code probe ollama-cloud)"
+check "claude-code/opus, no ollama key" "" "$(GADFLY_TEST_MODEL=claude-code/opus probe ollama-cloud)"
+check "opencode/x, no ollama key"       "" "$(GADFLY_TEST_MODEL=opencode/x probe ollama-cloud)"
+# ...but a genuine ollama-cloud model still is.
+check "ollama-cloud model, no key"      "OLLAMA_CLOUD_API_KEY" "$(GADFLY_TEST_MODEL=glm-5.2:cloud probe ollama-cloud)"
+
 if [ "$fail" -ne 0 ]; then
   echo "RESULT: preflight table FAILED"
   exit 1

scripts/run.sh

@@ -168,7 +168,9 @@ case "$PROVIDER" in
     GADFLY_PROVIDER_EFF="$MODEL_PROVIDER"
 
     # Credential pre-flight — one definition, shared with preflight_test.sh.
-    MISSING_KEY="$(gadfly_preflight_key "$GADFLY_PROVIDER_EFF")"
+    # Pass the raw spec too: engine specs (claude-code/opencode) carry their
+    # own auth and must not be judged by the provider fallback.
+    MISSING_KEY="$(gadfly_preflight_key "$GADFLY_PROVIDER_EFF" "$MODEL")"
     if [ -n "$MISSING_KEY" ]; then
       REVIEW="⚠️ No API key configured for provider \`${GADFLY_PROVIDER_EFF}\` (set \`${MISSING_KEY}\`); this reviewer was skipped."
     else