fix(qwen): gadfly round 2 — the anti-drift list had already drifted

Eight findings, all real, and the sharpest ones are about this PR's own fixes.

GADFLY_API_KEY was treated as a universal substitute in the pre-flight. It is
not: resolveModel reads it only AFTER the `baseURL == ""` early return, so on
the registry path — the documented primary path — a qwen/kimi built-in reads
its own variable and GADFLY_API_KEY is never consulted. A mis-set
GADFLY_API_KEY therefore passed pre-flight and 401'd five times anyway, which
is precisely the failure this check exists to prevent. It now only substitutes
when GADFLY_BASE_URL is also set.

`openai-compatible` was missing from the pre-flight table while both switches
accept it as an OPENAI_API_KEY alias, so that one spelling still fell through
to the cryptic five-failure mode.

endpointProviderNames — the constant I introduced *to stop* the two error
messages drifting — omitted the `gemini` alias both switches accept. It now
lists every accepted spelling.

And the case list itself was still duplicated across both switches plus the
test that pins them: three copies of the thing whose duplication started this.
Both switches now call isOpenAICompatProvider over one shared slice, and
endpointProvider's doc comment points at endpointProviderNames instead of
carrying a fourth hand-written copy.

scripts/preflight_test.sh moves into the repo (20 cases, up from 17, covering
openai-compatible and both GADFLY_API_KEY directions). It carries a drift guard
that diffs its copy of the provider table against run.sh's and aborts if they
differ — break-checked by deleting an arm from run.sh, which fails it loudly.

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

cmd/gadfly/model.go

@@ -3,6 +3,7 @@ package main
 import (
 	"fmt"
 	"os"
+	"slices"
 	"strings"
 
 	"gitea.stevedudenhoeffer.com/steve/majordomo"
@@ -19,12 +20,33 @@ import (
 // model list is just ids like "qwen3-coder:480b-cloud" — working unchanged.
 const defaultProvider = "ollama-cloud"
 
+// openAICompatProviders are the provider names that resolve to the plain
+// openai client at an explicit base URL. openai-compatible is the generic
+// spelling; kimi (Moonshot) and qwen (Alibaba Model Studio) are majordomo
+// built-ins that ARE that client pointed elsewhere, so an explicit endpoint for
+// either belongs on the same branch.
+//
+// This is a slice rather than three copies of a case list because there are
+// three places that must agree — resolveModel's switch, endpointProvider's
+// switch, and the test that pins them — and the first version of this change
+// added the names to one switch and not the other.
+var openAICompatProviders = []string{"openai", "openai-compatible", "kimi", "qwen"}
+
+func isOpenAICompatProvider(name string) bool {
+	return slices.Contains(openAICompatProviders, name)
+}
+
 // endpointProviderNames is the operator-facing list of providers that accept an
 // explicit endpoint. resolveModel and endpointProvider accept the SAME set, so
 // they share one message rather than each carrying a hand-maintained copy that
 // drifts in order and spelling — which is exactly what happened when kimi/qwen
 // were added to both switches.
-const endpointProviderNames = "openai/openai-compatible/kimi/qwen/ollama/ollama-cloud/llama-swap(s)/foreman/anthropic/google"
+//
+// Keep every accepted spelling here, including aliases: the first version of
+// this constant dropped "gemini", so the list written to prevent drift had
+// already drifted from the switches it describes.
+const endpointProviderNames = "openai/openai-compatible/kimi/qwen/ollama/ollama-cloud/" +
+	"llama-swap/llama-swaps/llamaswap/llamaswaps/foreman/anthropic/google/gemini"
 
 // resolveModel builds the review model from the environment. Gadfly is powered
 // by majordomo, so it can target any provider majordomo supports — Ollama
@@ -74,20 +96,19 @@ func resolveModel() (llm.Model, error) {
 	}
 
 	// Endpoint override: construct the provider directly at the given URL.
-	switch provider {
+	// The openai-compat family (openai/openai-compatible/kimi/qwen) is matched
-	case "openai", "openai-compatible", "kimi", "qwen":
+	// by the shared predicate, not a repeated case list. The credential here is
-		// kimi (Moonshot) and qwen (Alibaba Model Studio) are majordomo
+	// GADFLY_API_KEY; the built-ins' own KIMI_API_KEY / QWEN_API_KEY apply only
-		// built-ins that ARE the openai client at a different base URL, so an
+	// on the registry path above, where GADFLY_BASE_URL is unset.
-		// explicit GADFLY_BASE_URL for either belongs here. Without these names
+	if isOpenAICompatProvider(provider) {
-		// the override fell through to default: and errored, even though both
-		// resolve fine on the registry path above — a confusing asymmetry.
-		// The credential here is GADFLY_API_KEY; the built-ins' own
-		// KIMI_API_KEY / QWEN_API_KEY apply only when GADFLY_BASE_URL is unset.
 		opts := []openai.Option{openai.WithBaseURL(baseURL)}
 		if apiKey != "" {
 			opts = append(opts, openai.WithAPIKey(apiKey))
 		}
 		return openai.New(opts...).Model(model)
+	}
+
+	switch provider {
 	case "ollama", "ollama-cloud":
 		opts := []ollama.Option{ollama.WithBaseURL(baseURL)}
 		if apiKey != "" {
@@ -202,7 +223,7 @@ func modelProvider() string {
 //	    plaintext local Ollama (or foreman queue) works:
 //	        GADFLY_ENDPOINT_BIGBOX="ollama|http://192.168.1.50:11434"
 //	        GADFLY_MODEL=bigbox/qwen2.5-coder:7b
-//	    provider is one of ollama/llama-swap(s)/foreman/openai/anthropic/google; "foreman"
+//	    provider is one of endpointProviderNames; "foreman"
 //	    targets a foreman daemon (native Ollama on the wire):
 //	        GADFLY_ENDPOINT_M1="foreman|http://foreman-m1:8080|tok"
 //
@@ -254,6 +275,16 @@ func endpointProvider(name, raw string) (llm.Provider, error) {
 		return nil, fmt.Errorf("missing base URL in %q", raw)
 	}
 
+	// Same shared predicate as resolveModel — the two must accept an identical
+	// set, and hand-copied case lists are how they drifted apart before.
+	if isOpenAICompatProvider(provider) {
+		opts := []openai.Option{openai.WithName(name), openai.WithBaseURL(baseURL)}
+		if key != "" {
+			opts = append(opts, openai.WithAPIKey(key))
+		}
+		return openai.New(opts...), nil
+	}
+
 	switch provider {
 	case "ollama", "ollama-cloud":
 		opts := []ollama.Option{ollama.WithName(name), ollama.WithBaseURL(baseURL)}
@@ -272,16 +303,6 @@ func endpointProvider(name, raw string) (llm.Provider, error) {
 		// its non-streaming degradation. Unlike the HTTPS-only LLM_* foreman://
 		// DSN, the base URL here is verbatim, so a plaintext http:// foreman works.
 		return ollama.Foreman(baseURL, key, ollama.WithName(name)), nil
-	case "openai", "openai-compatible", "kimi", "qwen":
-		// kimi/qwen accepted here for the same reason as in resolveModel: both
-		// majordomo built-ins ARE the openai client at their own base URL, so a
-		// named endpoint pointing at one (a regional Model Studio host, say)
-		// must resolve, not error. The two switches move together.
-		opts := []openai.Option{openai.WithName(name), openai.WithBaseURL(baseURL)}
-		if key != "" {
-			opts = append(opts, openai.WithAPIKey(key))
-		}
-		return openai.New(opts...), nil
 	case "anthropic":
 		opts := []anthropic.Option{anthropic.WithName(name), anthropic.WithBaseURL(baseURL)}
 		if key != "" {

scripts/preflight_test.sh

@@ -0,0 +1,93 @@
+#!/usr/bin/env bash
+# Exercise the run.sh credential pre-flight in isolation: every provider x
+# key-present/absent x GADFLY_API_KEY/GADFLY_BASE_URL combination.
+#
+# The preflight() below is a COPY of run.sh's logic, which makes it exactly the
+# kind of duplicated pair this whole PR keeps finding. The drift guard runs
+# first: it compares the provider→variable arms in both files and aborts if
+# they differ, so a table edited in run.sh and not here fails loudly instead of
+# certifying stale logic.
+set -u
+
+SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"
+RUN_SH="${RUN_SH:-$SCRIPT_DIR/run.sh}"
+
+# Normalized case arms: "provider) KEY_ENV=... KEY_HINT=..." with runs of
+# whitespace collapsed, so alignment changes don't trip the guard.
+arms_of() { grep -oE '^[[:space:]]*[a-z|-]+\)[[:space:]]+KEY_ENV="[A-Z_]+";[[:space:]]+KEY_HINT="[A-Z_]+"' "$1" | tr -s ' \t' ' ' | sed 's/^ //'; }
+
+if [ -r "$RUN_SH" ]; then
+  if ! diff <(arms_of "$RUN_SH") <(arms_of "$0") >/dev/null; then
+    echo "FAIL drift guard: the pre-flight table here no longer matches $RUN_SH"
+    diff <(arms_of "$RUN_SH") <(arms_of "$0") | sed 's/^/  /'
+    exit 1
+  fi
+  echo "ok   drift guard: table matches run.sh"
+else
+  echo "FAIL drift guard: cannot read $RUN_SH — cannot prove this tests the real table"
+  exit 1
+fi
+
+preflight() { # $1=provider ; env carries the keys
+  local GADFLY_PROVIDER_EFF="$1" KEY_ENV="" KEY_HINT="" KEY_OK=0
+  case "$GADFLY_PROVIDER_EFF" in
+    ollama-cloud)                 KEY_ENV="OLLAMA_API_KEY";    KEY_HINT="OLLAMA_CLOUD_API_KEY" ;;
+    qwen)                         KEY_ENV="QWEN_API_KEY";      KEY_HINT="QWEN_API_KEY" ;;
+    kimi)                         KEY_ENV="KIMI_API_KEY";      KEY_HINT="KIMI_API_KEY" ;;
+    openai|openai-compatible)     KEY_ENV="OPENAI_API_KEY";    KEY_HINT="OPENAI_API_KEY" ;;
+    anthropic)                    KEY_ENV="ANTHROPIC_API_KEY"; KEY_HINT="ANTHROPIC_API_KEY" ;;
+  esac
+  [ -n "$KEY_ENV" ] && [ -n "${!KEY_ENV:-}" ] && KEY_OK=1
+  [ -n "${GADFLY_BASE_URL:-}" ] && [ -n "${GADFLY_API_KEY:-}" ] && KEY_OK=1
+  if [ -n "$KEY_ENV" ] && [ "$KEY_OK" -eq 0 ]; then
+    echo "SKIP:$KEY_HINT"
+  else
+    echo "RUN"
+  fi
+}
+
+fail=0
+check() { # desc, want, got
+  if [ "$2" = "$3" ]; then echo "ok   $1"; else echo "FAIL $1 — want $2, got $3"; fail=1; fi
+}
+
+env -i bash -c 'true' >/dev/null 2>&1  # sanity
+
+# --- keyed providers with NO key -> skip, naming the right variable ---
+check "qwen, no key"        "SKIP:QWEN_API_KEY"        "$(env -u QWEN_API_KEY -u GADFLY_API_KEY bash -c "$(declare -f preflight); preflight qwen")"
+check "kimi, no key"        "SKIP:KIMI_API_KEY"        "$(env -u KIMI_API_KEY -u GADFLY_API_KEY bash -c "$(declare -f preflight); preflight kimi")"
+check "ollama-cloud, none"  "SKIP:OLLAMA_CLOUD_API_KEY" "$(env -u OLLAMA_API_KEY -u GADFLY_API_KEY bash -c "$(declare -f preflight); preflight ollama-cloud")"
+check "openai, no key"      "SKIP:OPENAI_API_KEY"      "$(env -u OPENAI_API_KEY -u GADFLY_API_KEY bash -c "$(declare -f preflight); preflight openai")"
+check "anthropic, no key"   "SKIP:ANTHROPIC_API_KEY"   "$(env -u ANTHROPIC_API_KEY -u GADFLY_API_KEY bash -c "$(declare -f preflight); preflight anthropic")"
+
+# --- keyed providers WITH their key -> run ---
+check "qwen, keyed"         "RUN" "$(env -u GADFLY_API_KEY -u GADFLY_BASE_URL QWEN_API_KEY=k bash -c "$(declare -f preflight); preflight qwen")"
+check "ollama-cloud, keyed" "RUN" "$(env -u GADFLY_API_KEY -u GADFLY_BASE_URL OLLAMA_API_KEY=k bash -c "$(declare -f preflight); preflight ollama-cloud")"
+
+# --- the WRONG key must not satisfy a provider (no cross-provider fallback) ---
+check "qwen w/ only OPENAI key" "SKIP:QWEN_API_KEY" "$(env -u GADFLY_API_KEY -u GADFLY_BASE_URL -u QWEN_API_KEY OPENAI_API_KEY=k bash -c "$(declare -f preflight); preflight qwen")"
+
+# --- GADFLY_API_KEY substitutes ONLY with GADFLY_BASE_URL (the override path) ---
+check "qwen via GADFLY_API_KEY + BASE_URL" "RUN" \
+  "$(env -u QWEN_API_KEY GADFLY_API_KEY=k GADFLY_BASE_URL=https://x bash -c "$(declare -f preflight); preflight qwen")"
+# The false-pass this check exists to prevent: on the registry path (no
+# GADFLY_BASE_URL) the qwen built-in reads QWEN_API_KEY and never consults
+# GADFLY_API_KEY, so pre-flighting it as sufficient lets a doomed run proceed.
+check "qwen w/ GADFLY_API_KEY but no BASE_URL" "SKIP:QWEN_API_KEY" \
+  "$(env -u QWEN_API_KEY -u GADFLY_BASE_URL GADFLY_API_KEY=k bash -c "$(declare -f preflight); preflight qwen")"
+
+# --- openai-compatible is its own spelling and must be pre-flighted too ---
+check "openai-compatible, no key" "SKIP:OPENAI_API_KEY" \
+  "$(env -u OPENAI_API_KEY -u GADFLY_API_KEY -u GADFLY_BASE_URL bash -c "$(declare -f preflight); preflight openai-compatible")"
+check "openai-compatible, keyed" "RUN" \
+  "$(env -u GADFLY_API_KEY -u GADFLY_BASE_URL -u GADFLY_BASE_URL OPENAI_API_KEY=k bash -c "$(declare -f preflight); preflight openai-compatible")"
+
+# --- empty-string key counts as missing, not present ---
+check "qwen, empty key"     "SKIP:QWEN_API_KEY" "$(env -u GADFLY_API_KEY -u GADFLY_BASE_URL QWEN_API_KEY= bash -c "$(declare -f preflight); preflight qwen")"
+
+# --- unkeyed providers are never blocked, even with nothing set ---
+for p in ollama llama-swap llamaswap foreman google gemini some-dsn-name; do
+  check "unkeyed $p" "RUN" "$(env -i bash -c "$(declare -f preflight); preflight $p")"
+done
+
+exit $fail

scripts/run.sh

@@ -181,17 +181,23 @@ case "$PROVIDER" in
     #      not the table's one-name shape.
     KEY_ENV=""; KEY_HINT=""
     case "$GADFLY_PROVIDER_EFF" in
-      ollama-cloud) KEY_ENV="OLLAMA_API_KEY";    KEY_HINT="OLLAMA_CLOUD_API_KEY" ;;
+      ollama-cloud)                 KEY_ENV="OLLAMA_API_KEY";    KEY_HINT="OLLAMA_CLOUD_API_KEY" ;;
-      qwen)         KEY_ENV="QWEN_API_KEY";      KEY_HINT="QWEN_API_KEY" ;;
+      qwen)                         KEY_ENV="QWEN_API_KEY";      KEY_HINT="QWEN_API_KEY" ;;
-      kimi)         KEY_ENV="KIMI_API_KEY";      KEY_HINT="KIMI_API_KEY" ;;
+      kimi)                         KEY_ENV="KIMI_API_KEY";      KEY_HINT="KIMI_API_KEY" ;;
-      openai)       KEY_ENV="OPENAI_API_KEY";    KEY_HINT="OPENAI_API_KEY" ;;
+      openai|openai-compatible)     KEY_ENV="OPENAI_API_KEY";    KEY_HINT="OPENAI_API_KEY" ;;
-      anthropic)    KEY_ENV="ANTHROPIC_API_KEY"; KEY_HINT="ANTHROPIC_API_KEY" ;;
+      anthropic)                    KEY_ENV="ANTHROPIC_API_KEY"; KEY_HINT="ANTHROPIC_API_KEY" ;;
     esac
-    # Indirect expansion (bash), so the table above stays a table. Each majordomo
+    # GADFLY_API_KEY substitutes for the provider's own variable ONLY on the
-    # built-in reads ONLY its own variable — cross-provider fallback is refused
+    # endpoint-override path: resolveModel reads it after the `baseURL == ""`
-    # by design — so the named hint is always the actual fix.
+    # early return, so with GADFLY_BASE_URL unset the built-in reads its own
-    if [ -n "$KEY_ENV" ] && [ -z "${!KEY_ENV:-}" ] && [ -z "${GADFLY_API_KEY:-}" ]; then
+    # env var and GADFLY_API_KEY is never consulted. Treating it as a universal
-      REVIEW="⚠️ No API key configured for provider \`${GADFLY_PROVIDER_EFF}\` (set \`${KEY_HINT}\`, or \`GADFLY_API_KEY\`); this reviewer was skipped."
+    # substitute made a mis-set GADFLY_API_KEY pass pre-flight and then 401 five
+    # times anyway — the exact failure this check exists to prevent.
+    KEY_OK=0
+    [ -n "$KEY_ENV" ] && [ -n "${!KEY_ENV:-}" ] && KEY_OK=1          # indirect expansion (bash)
+    [ -n "${GADFLY_BASE_URL:-}" ] && [ -n "${GADFLY_API_KEY:-}" ] && KEY_OK=1
+    if [ -n "$KEY_ENV" ] && [ "$KEY_OK" -eq 0 ]; then
+      REVIEW="⚠️ No API key configured for provider \`${GADFLY_PROVIDER_EFF}\` (set \`${KEY_HINT}\`, or \`GADFLY_API_KEY\` together with \`GADFLY_BASE_URL\`); this reviewer was skipped."
     else
       BIN="${GADFLY_BIN:-gadfly}"
       if ! command -v "$BIN" >/dev/null 2>&1 && [ ! -x "$BIN" ]; then