diff --git a/cmd/gadfly/model.go b/cmd/gadfly/model.go index e3ff47c..33f2def 100644 --- a/cmd/gadfly/model.go +++ b/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 { - case "openai", "openai-compatible", "kimi", "qwen": - // kimi (Moonshot) and qwen (Alibaba Model Studio) are majordomo - // built-ins that ARE the openai client at a different base URL, so an - // explicit GADFLY_BASE_URL for either belongs here. Without these names - // 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. + // The openai-compat family (openai/openai-compatible/kimi/qwen) is matched + // by the shared predicate, not a repeated case list. The credential here is + // GADFLY_API_KEY; the built-ins' own KIMI_API_KEY / QWEN_API_KEY apply only + // on the registry path above, where GADFLY_BASE_URL is unset. + if isOpenAICompatProvider(provider) { 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 != "" { diff --git a/scripts/preflight_test.sh b/scripts/preflight_test.sh new file mode 100644 index 0000000..8bd64bd --- /dev/null +++ b/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 diff --git a/scripts/run.sh b/scripts/run.sh index c09caca..28ac987 100644 --- a/scripts/run.sh +++ b/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" ;; - qwen) KEY_ENV="QWEN_API_KEY"; KEY_HINT="QWEN_API_KEY" ;; - kimi) KEY_ENV="KIMI_API_KEY"; KEY_HINT="KIMI_API_KEY" ;; - openai) KEY_ENV="OPENAI_API_KEY"; KEY_HINT="OPENAI_API_KEY" ;; - anthropic) KEY_ENV="ANTHROPIC_API_KEY"; KEY_HINT="ANTHROPIC_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" ;; + 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 - # Indirect expansion (bash), so the table above stays a table. 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. - if [ -n "$KEY_ENV" ] && [ -z "${!KEY_ENV:-}" ] && [ -z "${GADFLY_API_KEY:-}" ]; then - REVIEW="⚠️ No API key configured for provider \`${GADFLY_PROVIDER_EFF}\` (set \`${KEY_HINT}\`, or \`GADFLY_API_KEY\`); this reviewer was skipped." + # GADFLY_API_KEY substitutes for the provider's own variable ONLY on the + # endpoint-override path: resolveModel reads it after the `baseURL == ""` + # early return, so with GADFLY_BASE_URL unset the built-in reads its own + # env var and GADFLY_API_KEY is never consulted. Treating it as a universal + # 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