steveandClaude Opus 5
d8b023efd6
Round 9's best finding is that my own round-8 change falsified a rationale I
wrote in round 5. The pre-flight skips the endpoint-override path because "a
built-in's own variable is never consulted there" — then I gave kimi/qwen an
own-key fallback that consults exactly that variable on exactly that path. So a
keyless override config sailed past the check and failed as a 401, which is the
failure the check exists to replace.
Now that the rule is statable for those two providers, they are checked on both
paths ("own key or GADFLY_API_KEY"), while everything else stays silent on the
override path because its rules still are not.
The missing-key hint on the GADFLY_ENDPOINT_* path named the endpoint variable
— telling a keyless operator to put a credential in a Gitea var, which is not
masked, and contradicting the README warning added one round earlier. It now
always names the provider's own masked secret.
Also: the model argument is trimmed, since Go trims GADFLY_MODEL and padding
would otherwise slip past the claude-code exemption; the test job takes
`permissions: contents: read`, being the one job that executes PR-authored
code; and the ollama-cloud rationale is stated once.
Deliberately not taken, with reasons rather than silence: the credential-scrub
bash could be extracted to a testable script like preflight.sh was — fair, and
a follow-up, since moving it now would be a fresh untested surface at merge
time. `tr -d [:space:]` strips POSIX whitespace where Go strips Unicode, which
differs only for a GADFLY_BASE_URL made entirely of non-ASCII spaces. And the
two provider tests overlap but assert different contracts that should be able
to fail independently.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
136 lines
6.3 KiB
Bash
Executable File
136 lines
6.3 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" model="${2:-}" key_env="" key_hint=""
|
|
|
|
# claude-code carries its OWN auth (CLAUDE_CODE_OAUTH_TOKEN, else
|
|
# ANTHROPIC_API_KEY) and needs no Ollama key. A bare "claude-code" has no "/",
|
|
# so the caller's provider falls back to ollama-cloud and the table below
|
|
# would skip a perfectly configured reviewer.
|
|
#
|
|
# opencode is deliberately NOT exempt: that engine drives an ollama-cloud
|
|
# model through the bundled CLI and authenticates with OLLAMA_API_KEY, so it
|
|
# needs exactly the key the table checks. Exempting it — which an earlier
|
|
# version of this guard did — turns the pre-flight off for the one engine
|
|
# whose missing key it could still catch.
|
|
model="$(printf '%s' "$model" | tr -d '[:space:]')" # Go trims GADFLY_MODEL
|
|
case "$model" in
|
|
claude-code|claude-code/*) echo ""; return 0 ;;
|
|
esac
|
|
|
|
# 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
|
|
# Endpoint-override path. Most providers take their credential from
|
|
# GADFLY_API_KEY here with a client-specific fallback, and those rules are
|
|
# not worth restating — this stays silent for them.
|
|
#
|
|
# The built-ins are the exception, and only since they gained an own-key
|
|
# fallback: a keyless kimi/qwen endpoint reads QWEN_API_KEY / KIMI_API_KEY
|
|
# on THIS path too, so "own key or GADFLY_API_KEY" is a rule that can be
|
|
# stated exactly. Leaving them unchecked here would let a keyless override
|
|
# config sail past the pre-flight and fail as a 401 — the failure the
|
|
# pre-flight exists to replace.
|
|
case "$provider" in
|
|
qwen|kimi) ;;
|
|
*) echo ""; return 0 ;;
|
|
esac
|
|
local own_env="$(printf '%s' "$provider" | tr '[:lower:]-' '[:upper:]_')_API_KEY"
|
|
if [ -n "${!own_env:-}" ] || [ -n "${GADFLY_API_KEY:-}" ]; then
|
|
echo ""
|
|
else
|
|
echo "$own_env"
|
|
fi
|
|
return 0
|
|
fi
|
|
|
|
local row
|
|
row="$(_gadfly_preflight_table | awk -F: -v p="$provider" '$1 == p {print; exit}')"
|
|
if [ -z "$row" ]; then
|
|
echo "" # provider needs no pre-flight
|
|
return 0
|
|
fi
|
|
key_env="$(printf '%s' "$row" | cut -d: -f2)"
|
|
key_hint="$(printf '%s' "$row" | cut -d: -f3)"
|
|
[ -n "$key_hint" ] || key_hint="$key_env"
|
|
|
|
# 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_table is the single source for both the credential lookup
|
|
# and the provider list: "<provider>:<env-var-read>:<env-var-to-suggest>".
|
|
#
|
|
# The third field is normally empty, meaning "same as the second". ollama-cloud
|
|
# is the exception: run.sh copies the consumer-facing OLLAMA_CLOUD_API_KEY onto
|
|
# the OLLAMA_API_KEY the provider reads BEFORE calling in here, so the check and
|
|
# the hint name different variables on purpose. If that copy ever moves after
|
|
# the call, this arm reports a missing key for a configured run.
|
|
#
|
|
# A provider absent from this table is absent for one of TWO reasons — do not
|
|
# assume the first and add a row:
|
|
# 1. It needs no key, or carries one 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); pre-flighting it needs an
|
|
# either-variable check, not this table's one-name shape.
|
|
_gadfly_preflight_table() {
|
|
printf '%s\n' \
|
|
'ollama-cloud:OLLAMA_API_KEY:OLLAMA_CLOUD_API_KEY' \
|
|
'opencode:OLLAMA_API_KEY:OLLAMA_CLOUD_API_KEY' \
|
|
'open-code:OLLAMA_API_KEY:OLLAMA_CLOUD_API_KEY' \
|
|
'qwen:QWEN_API_KEY:' \
|
|
'kimi:KIMI_API_KEY:' \
|
|
'openai:OPENAI_API_KEY:' \
|
|
'openai-compatible:OPENAI_API_KEY:' \
|
|
'anthropic:ANTHROPIC_API_KEY:'
|
|
}
|
|
|
|
# gadfly_preflight_providers echoes every provider covered above, one per line.
|
|
# Callers ASK rather than parse: a Go test cross-checks this against the
|
|
# openai-compat provider table in cmd/gadfly/model.go, and regexing this file
|
|
# would make its formatting a contract no linter enforces.
|
|
#
|
|
# The cross-check runs ONE direction — every openai-compat provider in Go must
|
|
# appear here. The reverse is not required and must not be asserted:
|
|
# ollama-cloud and anthropic belong in this table and are deliberately not in
|
|
# that Go list.
|
|
gadfly_preflight_providers() {
|
|
_gadfly_preflight_table | cut -d: -f1
|
|
}
|