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]>
116 lines
6.0 KiB
Bash
Executable File
116 lines
6.0 KiB
Bash
Executable File
#!/usr/bin/env bash
|
|
# Table test for the credential pre-flight in preflight.sh.
|
|
#
|
|
# It SOURCES the real implementation rather than copying it, so there is no
|
|
# second definition that can pass while production fails.
|
|
#
|
|
# Run: scripts/preflight_test.sh (exit 0 = all cases pass)
|
|
set -u
|
|
|
|
SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"
|
|
# shellcheck source=preflight.sh
|
|
. "$SCRIPT_DIR/preflight.sh"
|
|
|
|
fail=0
|
|
check() { # description, want, got
|
|
if [ "$2" = "$3" ]; then
|
|
echo "ok $1"
|
|
else
|
|
echo "FAIL $1 — want '$2', got '$3'"
|
|
fail=1
|
|
fi
|
|
}
|
|
|
|
# probe <provider> [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" model="${GADFLY_TEST_MODEL:-}"; shift
|
|
env -i PATH="$PATH" HOME="$HOME" "$@" bash -c "
|
|
set -u
|
|
. '$SCRIPT_DIR/preflight.sh'
|
|
gadfly_preflight_key '$provider' '$model'
|
|
"
|
|
}
|
|
|
|
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)"
|
|
|
|
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)"
|
|
|
|
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)"
|
|
|
|
echo "== an empty-string key counts as missing, not present =="
|
|
check "qwen, empty key" "QWEN_API_KEY" "$(probe qwen QWEN_API_KEY=)"
|
|
|
|
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)"
|
|
|
|
echo "== override path: built-ins ARE checked; others are not =="
|
|
# 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.
|
|
# A built-in reads its own key on the override path too (openAICompatOptions
|
|
# falls back to QWEN_API_KEY/KIMI_API_KEY there), so "own key or GADFLY_API_KEY"
|
|
# is statable and worth checking — leaving it unchecked let a keyless config
|
|
# sail past and fail as a 401.
|
|
check "qwen + BASE_URL, no keys" "QWEN_API_KEY" "$(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 "kimi + BASE_URL, no keys" "KIMI_API_KEY" "$(probe kimi GADFLY_BASE_URL=https://x)"
|
|
# Other providers' override-path rules are not statable, so this stays quiet.
|
|
check "openai + BASE_URL, no keys" "" "$(probe openai GADFLY_BASE_URL=https://x)"
|
|
check "anthropic + BASE_URL, none" "" "$(probe anthropic GADFLY_BASE_URL=https://x)"
|
|
|
|
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
|
|
|
|
# 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)"
|
|
|
|
echo "== a whitespace-only GADFLY_BASE_URL counts as unset, as it does in Go =="
|
|
# resolveModel TrimSpaces it and takes the registry path; if this check
|
|
# disagreed, the missing key would arrive as a bare 401 with no skip notice.
|
|
check "qwen + blank BASE_URL" "QWEN_API_KEY" "$(probe qwen GADFLY_BASE_URL=" ")"
|
|
|
|
echo "== engine specs carry their own auth and are never pre-flighted =="
|
|
# A bare "claude-code" has no "/", so the caller's provider falls back to
|
|
# ollama-cloud; judging it by that would skip a reviewer using
|
|
# CLAUDE_CODE_OAUTH_TOKEN, which needs no Ollama key.
|
|
check "bare claude-code, no ollama key" "" "$(GADFLY_TEST_MODEL=claude-code probe ollama-cloud)"
|
|
# Go trims GADFLY_MODEL, so padding must not bypass the exemption.
|
|
check "claude-code w/ whitespace" "" "$(GADFLY_TEST_MODEL=" claude-code " probe ollama-cloud)"
|
|
check "claude-code/opus, no ollama key" "" "$(GADFLY_TEST_MODEL=claude-code/opus probe ollama-cloud)"
|
|
# opencode is NOT exempt: it drives an ollama-cloud model and needs that key,
|
|
# so skipping it would disable the pre-flight for the one engine it can help.
|
|
check "opencode/x, no ollama key" "OLLAMA_CLOUD_API_KEY" "$(GADFLY_TEST_MODEL=opencode/x probe opencode)"
|
|
check "bare opencode, no ollama key" "OLLAMA_CLOUD_API_KEY" "$(GADFLY_TEST_MODEL=opencode probe ollama-cloud)"
|
|
check "open-code/x, no ollama key" "OLLAMA_CLOUD_API_KEY" "$(GADFLY_TEST_MODEL=open-code/x probe open-code)"
|
|
check "opencode/x, keyed" "" "$(GADFLY_TEST_MODEL=opencode/x probe opencode OLLAMA_API_KEY=k)"
|
|
# ...but a genuine ollama-cloud model still is.
|
|
check "ollama-cloud model, no key" "OLLAMA_CLOUD_API_KEY" "$(GADFLY_TEST_MODEL=glm-5.2:cloud probe ollama-cloud)"
|
|
|
|
if [ "$fail" -ne 0 ]; then
|
|
echo "RESULT: preflight table FAILED"
|
|
exit 1
|
|
fi
|
|
echo "RESULT: all pre-flight cases pass"
|