steveandClaude Opus 5
67a73616e1
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) <[email protected]>
67 lines
3.2 KiB
Bash
67 lines
3.2 KiB
Bash
#!/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 <provider> -> 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"
|
|
}
|