steveandClaude Opus 5
a3d3a45e7e
Round 6, and one finding exposed something no reviewer mentioned: the majordomo bump this whole PR depends on was never made. Every test here builds the openai client directly, so all of them passed against a majordomo release that had never heard of qwen — a plain "qwen/<model>" in GADFLY_MODELS, the primary way anyone will use this, would not have resolved at all. A compile error caught it, which is luck. TestBuiltinCompatProvidersResolveViaRegistry now exercises that path; the build is what guards the dep itself, since the old release cannot compile the code below. On the endpoint-override path, kimi and qwen fell through to openai.New's OPENAI_API_KEY default whenever GADFLY_API_KEY was unset — sending an OpenAI key to Moonshot or Alibaba. That is a credential handed to the wrong vendor, and it is the exact failure majordomo's built-ins are written to prevent; I reintroduced it one layer up. Both now pass the key unconditionally, so an absent key is a 401 naming GADFLY_API_KEY rather than a foreign credential on the wire. The test job scrubbed the registry credential and left the checkout token in .git/config, readable by the `go test` it then runs — fixing one credential while its neighbour sat in the open. persist-credentials: false; nothing in that job talks to git after checkout. The cross-language wiring test now QUERIES preflight.sh via a new gadfly_preflight_providers function instead of regexing its case statement. Parsing made that file's formatting a contract no linter enforces, where a harmless reformat breaks a test in another language. Two models flagged it. Also: grep for the scrub check takes -e, so a password starting with a hyphen is not read as options; and key_hint stopped repeating key_env in four of five arms. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
97 lines
4.7 KiB
Bash
Executable File
97 lines
4.7 KiB
Bash
Executable File
#!/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
|
|
# the tested bytes and the running bytes are the same. Keep it that way: a test
|
|
# that reimplements this logic can agree with a stale copy of it.
|
|
#
|
|
# 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 — GADFLY_BASE_URL unset — and deliberately so.
|
|
# The two resolution paths have DIFFERENT credential rules: with an explicit
|
|
# endpoint the credential is GADFLY_API_KEY (falling back to the client's own
|
|
# default, OPENAI_API_KEY for the openai family) and a built-in's own variable
|
|
# is never consulted; without one, the reverse. Applying either path's rule to
|
|
# the other yields a check that passes a run which then 401s — the precise
|
|
# failure this exists to prevent. So it covers the path whose rules it can state
|
|
# exactly and stays silent on the other. That is also the useful half: an
|
|
# 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=""
|
|
|
|
# Only the registry path has knowable credential rules — see above.
|
|
# Trim before testing: resolveModel does strings.TrimSpace on GADFLY_BASE_URL,
|
|
# so a whitespace-only value takes the REGISTRY path there. Testing the raw
|
|
# value here would call it "set", skip the check, and let the missing key
|
|
# arrive as a 401 with no notice — the two must agree on what "unset" means.
|
|
local base_url
|
|
base_url="$(printf '%s' "${GADFLY_BASE_URL:-}" | tr -d '[:space:]')"
|
|
if [ -n "$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.
|
|
# ollama-cloud is checked on OLLAMA_API_KEY but hinted as OLLAMA_CLOUD_API_KEY:
|
|
# run.sh copies the consumer-facing OLLAMA_CLOUD_API_KEY secret into the
|
|
# OLLAMA_API_KEY the provider reads, BEFORE calling this. The hint names the
|
|
# 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
|
|
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"
|
|
}
|
|
|
|
# gadfly_preflight_providers echoes every provider this file has a credential
|
|
# arm for, one per line.
|
|
#
|
|
# 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.
|
|
#
|
|
# Keep in step with the case arms above — the Go test fails if a provider in
|
|
# either list is missing from the other.
|
|
gadfly_preflight_providers() {
|
|
printf '%s\n' ollama-cloud qwen kimi openai openai-compatible anthropic
|
|
}
|