From 67a73616e101fec64564761f77d1b164195919a9 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Wed, 12 Aug 2026 17:20:44 -0400 Subject: [PATCH] =?UTF-8?q?fix(qwen):=20gadfly=20round=203=20=E2=80=94=20s?= =?UTF-8?q?top=20guarding=20a=20duplicate,=20delete=20it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Twelve findings, all real, and the two that matter are about the pre-flight I added rather than about qwen. The credential check had a false pass in the OTHER direction from round 2's: on the GADFLY_BASE_URL override path, resolveModel builds the client with GADFLY_API_KEY and never reads QWEN_API_KEY/KIMI_API_KEY, so treating the provider's own key as sufficient there let a doomed run proceed. Having now been wrong about these rules in both directions, the check no longer tries to model both paths: it covers the REGISTRY path, whose rules it can state exactly, and says nothing about the override path — which is hand-configured by definition, while the registry path is the one you hit by adding a model id to a var and forgetting the secret. The logic moves to scripts/preflight.sh, sourced by both run.sh and the test. The previous answer to "this test duplicates production logic" was a regex drift-guard, and that guard compared only the provider table — not the decision logic, which is precisely the half that carried the bug. A duplicate you guard is still a duplicate; this deletes it, and the test now runs under `set -u` like production does. Also: the test that pins the shared provider slice held its own copy of the list (now ranges the slice); endpointProviderNames had nothing tying it to the switches it describes, which is how it shipped without "gemini" (a new test asserts every advertised name resolves); two godoc lists had drifted; and the "sanity" line that asserted nothing is gone. And the repo had NO test job — `go test` and the pre-flight table both existed and neither was ever executed by CI, which reads as coverage while providing none. Added one (build/vet/gofmt/test/pre-flight), running alongside the image build rather than gating it, so red is loud without standing between a push and a rebuild. Co-Authored-By: Claude Opus 5 (1M context) --- .gitea/workflows/build-image.yml | 31 +++++++ cmd/gadfly/model.go | 16 ++-- cmd/gadfly/model_test.go | 28 +++++- scripts/preflight.sh | 66 ++++++++++++++ scripts/preflight_test.sh | 145 +++++++++++++++---------------- scripts/run.sh | 45 ++-------- 6 files changed, 212 insertions(+), 119 deletions(-) create mode 100644 scripts/preflight.sh diff --git a/.gitea/workflows/build-image.yml b/.gitea/workflows/build-image.yml index d1c0695..c1bb7fb 100644 --- a/.gitea/workflows/build-image.yml +++ b/.gitea/workflows/build-image.yml @@ -45,6 +45,37 @@ env: IMAGE_NAME: gitea.stevedudenhoeffer.com/steve/gadfly jobs: + # Runs alongside the image build rather than gating it: a red test should be + # loud on the PR without standing between Steve and a rebuild. Added because + # this repo had NO test job at all — `go test` and scripts/preflight_test.sh + # both existed and neither was ever executed by CI, which is worse than + # having no tests, since it reads as coverage. + test: + runs-on: ubuntu-latest + timeout-minutes: 15 + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-go@v5 + with: + go-version-file: go.mod + - name: Configure private module access + env: + REGISTRY_USER: ${{ secrets.REGISTRY_USER }} + REGISTRY_PASSWORD: ${{ secrets.REGISTRY_PASSWORD }} + run: | + git config --global url."https://${REGISTRY_USER}:${REGISTRY_PASSWORD}@gitea.stevedudenhoeffer.com/".insteadOf "https://gitea.stevedudenhoeffer.com/" + go env -w GOPRIVATE=gitea.stevedudenhoeffer.com/* + - name: go build + run: go build ./... + - name: go vet + run: go vet ./... + - name: gofmt + run: test -z "$(gofmt -l .)" || { gofmt -l .; exit 1; } + - name: go test + run: go test -count=1 ./... + - name: pre-flight credential table + run: bash scripts/preflight_test.sh + build-and-push: runs-on: ubuntu-latest timeout-minutes: 20 diff --git a/cmd/gadfly/model.go b/cmd/gadfly/model.go index 33f2def..b8e9b0e 100644 --- a/cmd/gadfly/model.go +++ b/cmd/gadfly/model.go @@ -62,10 +62,12 @@ const endpointProviderNames = "openai/openai-compatible/kimi/qwen/ollama/ollama- // GADFLY_BASE_URL override the backend endpoint (OpenAI/Ollama-compatible // servers, a remote Ollama, an OpenRouter-style gateway…). // When set, the provider is constructed directly at that URL. -// GADFLY_API_KEY bearer/API key for the chosen provider. Optional; when -// unset the provider falls back to its standard env var -// (OLLAMA_API_KEY / OPENAI_API_KEY / ANTHROPIC_API_KEY / -// GOOGLE_API_KEY|GEMINI_API_KEY). Local Ollama needs none. +// GADFLY_API_KEY bearer/API key for the chosen provider, used ONLY on the +// GADFLY_BASE_URL override path. With no base URL the +// provider reads its own standard variable and this is never +// consulted: OLLAMA_API_KEY / OPENAI_API_KEY / +// QWEN_API_KEY / KIMI_API_KEY / ANTHROPIC_API_KEY / +// GOOGLE_API_KEY|GEMINI_API_KEY. Local Ollama needs none. // // With GADFLY_BASE_URL unset, resolution goes through majordomo's registry, so // LLM_* env DSNs and registered aliases/tiers work too. @@ -223,8 +225,10 @@ 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 endpointProviderNames; "foreman" -// targets a foreman daemon (native Ollama on the wire): +// provider is ollama/openai/anthropic/google/foreman/llama-swap(s) or an +// openai-compat built-in (kimi, qwen) — endpointProviderNames is the +// authoritative list. "foreman" targets a foreman daemon (native Ollama +// on the wire): // GADFLY_ENDPOINT_M1="foreman|http://foreman-m1:8080|tok" // // GADFLY_ALIAS_ = "" diff --git a/cmd/gadfly/model_test.go b/cmd/gadfly/model_test.go index d8ba84a..8a81cc2 100644 --- a/cmd/gadfly/model_test.go +++ b/cmd/gadfly/model_test.go @@ -1,6 +1,9 @@ package main -import "testing" +import ( + "strings" + "testing" +) func TestEndpointProvider(t *testing.T) { t.Run("ollama http endpoint registers under its name", func(t *testing.T) { @@ -77,7 +80,9 @@ func TestEndpointProvider(t *testing.T) { // reason a user could guess — which is exactly what happened here on the first // pass. Asserting both in one table is what makes the pair fail together. func TestOpenAICompatProvidersResolveOnBothPaths(t *testing.T) { - for _, provider := range []string{"openai", "openai-compatible", "kimi", "qwen"} { + // Ranges the SHARED slice rather than a fourth copy of the names: a test + // that pins a list against drift must not be able to drift from it. + for _, provider := range openAICompatProviders { t.Run(provider+" via GADFLY_ENDPOINT_*", func(t *testing.T) { p, err := endpointProvider("ep", provider+"|https://host.example/v1|sk-x") if err != nil { @@ -99,6 +104,25 @@ func TestOpenAICompatProvidersResolveOnBothPaths(t *testing.T) { } } +// TestEndpointProviderNamesAreAllAccepted keeps the operator-facing list +// honest. endpointProviderNames exists to stop two error messages drifting +// apart, but nothing tied it to the switches it describes — and its first +// version had already dropped the "gemini" alias, so the anti-drift list was +// itself drifted. Every name it advertises must actually resolve. +func TestEndpointProviderNamesAreAllAccepted(t *testing.T) { + for _, name := range strings.Split(endpointProviderNames, "/") { + name = strings.TrimSpace(name) + if name == "" { + continue + } + t.Run(name, func(t *testing.T) { + if _, err := endpointProvider("ep", name+"|https://host.example/v1|sk-x"); err != nil { + t.Errorf("endpointProviderNames advertises %q but endpointProvider rejects it: %v", name, err) + } + }) + } +} + func TestBuildSpec(t *testing.T) { tests := []struct { name string diff --git a/scripts/preflight.sh b/scripts/preflight.sh new file mode 100644 index 0000000..c9972d1 --- /dev/null +++ b/scripts/preflight.sh @@ -0,0 +1,66 @@ +#!/usr/bin/env bash +# Credential pre-flight for the agentic reviewer, in ONE definition. +# +# Sourced by run.sh (production) and by preflight_test.sh (the table test), so +# there is no second copy to drift. An earlier version of this change had the +# logic in run.sh and a duplicate in the test reconciled by a regex diff — that +# guard only covered the provider table and not the decision logic below, which +# is precisely the half that had the bug. +# +# Why pre-flight at all, when majordomo already fails closed with a 401: +# without it a missing key surfaces as five identical per-lens agent failures +# that name no variable, and the operator reads a stack trace to learn which +# secret they forgot to forward. + +# gadfly_preflight_key -> echoes "" when the run may proceed, or the +# name of the environment variable the operator must set. +# +# Scope: the REGISTRY path only — i.e. GADFLY_BASE_URL unset. That is deliberate. +# With an explicit endpoint, resolveModel constructs the client directly and the +# credential is GADFLY_API_KEY, falling back to the client's own default +# (OPENAI_API_KEY for the openai family) — while the built-ins' own variables are +# never consulted. Checking one path's rules against the other produced a +# false-pass in BOTH directions across successive fixes here, so this checks the +# path whose rules it can state exactly and stays silent on the other. An +# override-path config is hand-written by definition; the registry path is the +# one somebody hits by adding a model id to a var and forgetting the secret. +gadfly_preflight_key() { + local provider="$1" key_env="" key_hint="" + + # Only the registry path has knowable credential rules — see above. + if [ -n "${GADFLY_BASE_URL:-}" ]; then + echo "" + return 0 + fi + + # A provider is absent from this table for one of TWO different reasons — do + # not assume the first one and add an arm: + # 1. It needs no key, or carries it 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. Adding + # `google) key_env="GOOGLE_API_KEY"` would silently skip every reviewer + # configured with GEMINI_API_KEY. Pre-flighting google needs an + # either-variable check, not this table's one-name shape. + case "$provider" 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 + + if [ -z "$key_env" ]; then + echo "" # provider needs no pre-flight + return 0 + fi + # 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. + if [ -n "${!key_env:-}" ]; then + echo "" + return 0 + fi + echo "$key_hint" +} diff --git a/scripts/preflight_test.sh b/scripts/preflight_test.sh index 8bd64bd..b9fe9ee 100644 --- a/scripts/preflight_test.sh +++ b/scripts/preflight_test.sh @@ -1,93 +1,88 @@ #!/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. +# Table test for the credential pre-flight in preflight.sh. # -# 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. +# It SOURCES the real implementation rather than copying it. An earlier version +# duplicated the logic and reconciled the copies with a regex diff — which only +# covered the provider table and not the decision logic, i.e. exactly the half +# that had the bug. Sourcing removes the second copy entirely. +# +# Run: scripts/preflight_test.sh (exit 0 = all cases pass) 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 -} +# shellcheck source=preflight.sh +. "$SCRIPT_DIR/preflight.sh" fail=0 -check() { # desc, want, got - if [ "$2" = "$3" ]; then echo "ok $1"; else echo "FAIL $1 — want $2, got $3"; fail=1; fi +check() { # description, 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 +# probe [VAR=VAL ...] — run the real function in a clean environment +# 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 + env -i PATH="$PATH" HOME="$HOME" "$@" bash -c " + set -u + . '$SCRIPT_DIR/preflight.sh' + gadfly_preflight_key '$provider' + " +} -# --- 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")" +echo "== registry path: keyed providers with no key must name their variable ==" +check "qwen, no key" "QWEN_API_KEY" "$(probe qwen)" +check "kimi, no key" "KIMI_API_KEY" "$(probe kimi)" +check "ollama-cloud, no key" "OLLAMA_CLOUD_API_KEY" "$(probe ollama-cloud)" +check "openai, no key" "OPENAI_API_KEY" "$(probe openai)" +check "openai-compatible, none" "OPENAI_API_KEY" "$(probe openai-compatible)" +check "anthropic, no key" "ANTHROPIC_API_KEY" "$(probe 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")" +echo "== registry path: the provider's own key lets it run ==" +check "qwen, keyed" "" "$(probe qwen QWEN_API_KEY=k)" +check "kimi, keyed" "" "$(probe kimi KIMI_API_KEY=k)" +check "ollama-cloud, keyed" "" "$(probe ollama-cloud OLLAMA_API_KEY=k)" +check "openai-compatible, keyed" "" "$(probe openai-compatible OPENAI_API_KEY=k)" -# --- 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")" +echo "== a wrong-provider key never satisfies a provider (no cross-fallback) ==" +check "qwen w/ only OPENAI key" "QWEN_API_KEY" "$(probe qwen OPENAI_API_KEY=k)" +check "kimi w/ only QWEN key" "KIMI_API_KEY" "$(probe kimi QWEN_API_KEY=k)" -# --- 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")" +echo "== an empty-string key counts as missing, not present ==" +check "qwen, empty key" "QWEN_API_KEY" "$(probe qwen QWEN_API_KEY=)" -# --- 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")" +echo "== GADFLY_API_KEY does NOT substitute on the registry path ==" +# resolveModel reads GADFLY_API_KEY only after its `baseURL == ""` early +# return, so on this path the built-in reads its own variable and a set +# GADFLY_API_KEY changes nothing. Treating it as sufficient was a false pass. +check "qwen w/ GADFLY_API_KEY only" "QWEN_API_KEY" "$(probe qwen GADFLY_API_KEY=k)" -# --- 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")" +echo "== override path (GADFLY_BASE_URL set) is deliberately not pre-flighted ==" +# The credential there is GADFLY_API_KEY with a client-specific fallback, and +# the built-ins' own variables are never read. Checking one path's rules +# against the other produced a false pass in BOTH directions, so this path is +# left alone rather than guessed at. +check "qwen + BASE_URL, no keys" "" "$(probe qwen GADFLY_BASE_URL=https://x)" +check "qwen + BASE_URL + own key" "" "$(probe qwen GADFLY_BASE_URL=https://x QWEN_API_KEY=k)" +check "qwen + BASE_URL + GADFLY key" "" "$(probe qwen GADFLY_BASE_URL=https://x GADFLY_API_KEY=k)" +check "openai + BASE_URL, no keys" "" "$(probe openai GADFLY_BASE_URL=https://x)" -# --- 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")" +echo "== providers needing no key are never blocked, with nothing set ==" +for p in ollama llama-swap llama-swaps llamaswap llamaswaps foreman google gemini some-dsn-name; do + check "unkeyed $p" "" "$(probe "$p")" done -exit $fail +# google is absent from the table on purpose: it accepts GOOGLE_API_KEY *or* +# GEMINI_API_KEY, so a one-name arm would skip a correctly-configured run. +check "google w/ only GEMINI_API_KEY" "" "$(probe google GEMINI_API_KEY=k)" + +if [ "$fail" -ne 0 ]; then + echo "RESULT: preflight table FAILED" + exit 1 +fi +echo "RESULT: all pre-flight cases pass" diff --git a/scripts/run.sh b/scripts/run.sh index 28ac987..cbffd7d 100644 --- a/scripts/run.sh +++ b/scripts/run.sh @@ -48,6 +48,11 @@ set -uo pipefail SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" MAX_DIFF_CHARS="${MAX_DIFF_CHARS:-60000}" +# Credential pre-flight, shared verbatim with scripts/preflight_test.sh so the +# tested logic and the running logic are the same bytes. +# shellcheck source=preflight.sh +. "$SCRIPT_DIR/preflight.sh" + : "${GITEA_API:?GITEA_API required}" : "${GITEA_TOKEN:?GITEA_TOKEN required}" : "${PR:?PR required}" @@ -162,42 +167,10 @@ case "$PROVIDER" in fi GADFLY_PROVIDER_EFF="$MODEL_PROVIDER" - # Pre-flight the credential for providers that need one. Why bother, when - # majordomo already fails closed with a 401: without this, a missing key - # surfaces as five identical per-lens agent failures that name no variable, - # and the operator has to read a stack trace to learn which secret they - # forgot to forward. GADFLY_API_KEY overrides any entry. - # - # A provider is absent from this table for one of TWO different reasons — - # do not assume the first one and add an arm: - # 1. It needs no key, or carries it in its endpoint/DSN: local ollama, - # llama-swap, foreman. - # 2. It needs a key but has more than one acceptable variable, so a - # single-variable check would skip a correctly-configured run. - # **google** is this case: it accepts GOOGLE_API_KEY *or* - # GEMINI_API_KEY. Adding `google) KEY_ENV="GOOGLE_API_KEY"` here would - # silently skip every reviewer configured with GEMINI_API_KEY. If you - # want google pre-flighted, the check has to accept either variable, - # 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|openai-compatible) KEY_ENV="OPENAI_API_KEY"; KEY_HINT="OPENAI_API_KEY" ;; - anthropic) KEY_ENV="ANTHROPIC_API_KEY"; KEY_HINT="ANTHROPIC_API_KEY" ;; - esac - # 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." + # Credential pre-flight — one definition, shared with preflight_test.sh. + MISSING_KEY="$(gadfly_preflight_key "$GADFLY_PROVIDER_EFF")" + if [ -n "$MISSING_KEY" ]; then + REVIEW="⚠️ No API key configured for provider \`${GADFLY_PROVIDER_EFF}\` (set \`${MISSING_KEY}\`); this reviewer was skipped." else BIN="${GADFLY_BIN:-gadfly}" if ! command -v "$BIN" >/dev/null 2>&1 && [ ! -x "$BIN" ]; then