diff --git a/.gitea/workflows/build-image.yml b/.gitea/workflows/build-image.yml index 7d7d9fa..b341cc9 100644 --- a/.gitea/workflows/build-image.yml +++ b/.gitea/workflows/build-image.yml @@ -108,12 +108,21 @@ jobs: # write ~/.netrc or ~/.config/go/env. Guarded on a non-empty secret: # `grep -F ""` matches every file, so a secretless run (fork PR) would # fail here with a message accusing it of leaking nothing. - # -e, so a password beginning with "-" is a pattern and not options: - # without it the check errors out and, under `set -e`, fails the step - # with a message about grep usage rather than about credentials. - if [ -n "${REGISTRY_PASSWORD:-}" ] && grep -rqF -e "$REGISTRY_PASSWORD" "$HOME" 2>/dev/null; then - echo "::error::registry credential still present under \$HOME after scrub" - exit 1 + # -e, so a password beginning with "-" is a pattern and not options. + # And distinguish grep's three exits: 0 found, 1 clean, >=2 ERROR. As + # a bare condition an error reads as "not found" and the guard is + # skipped — a check that fails OPEN in exactly the case where it can no + # longer see the filesystem it is supposed to be searching. + if [ -n "${REGISTRY_PASSWORD:-}" ]; then + set +e + grep -rqF -e "$REGISTRY_PASSWORD" "$HOME" 2>/dev/null + rc=$? + set -e + case "$rc" in + 0) echo "::error::registry credential still present under \$HOME after scrub"; exit 1 ;; + 1) : ;; # clean + *) echo "::error::credential scrub check could not run (grep exit $rc); refusing to continue"; exit 1 ;; + esac fi # GOPROXY=off from here on: the module cache is already warm, so any diff --git a/cmd/gadfly/model.go b/cmd/gadfly/model.go index 1c5b14a..3589fb8 100644 --- a/cmd/gadfly/model.go +++ b/cmd/gadfly/model.go @@ -33,10 +33,38 @@ const defaultProvider = "ollama-cloud" // guess. var openAICompatProviders = []string{"openai", "openai-compatible", "kimi", "qwen"} +// builtinCompatProviders are the openai-compat names that belong to a DIFFERENT +// vendor. openai.New defaults its credential to OPENAI_API_KEY, so any of these +// constructed without an explicit key would put an OpenAI key on the wire to +// Moonshot or Alibaba. Membership here means "pass the key unconditionally, +// even empty" — an absent key must be a 401, never a foreign credential. +var builtinCompatProviders = []string{"kimi", "qwen"} + func isOpenAICompatProvider(name string) bool { return slices.Contains(openAICompatProviders, name) } +// openAICompatOptions builds the option set for an openai-compat provider, and +// is the ONE place the no-cross-vendor-fallback rule lives. +// +// Both resolution paths call it — resolveModel's GADFLY_BASE_URL override and +// endpointProvider's GADFLY_ENDPOINT_* parser. They had separate copies of this +// decision once, the guard was added to one of them, and the other kept leaking +// OPENAI_API_KEY to another vendor. keyHint names the variable to set when the +// key is absent, since the two paths take it from different places. +func openAICompatOptions(provider, baseURL, key, keyHint string) []openai.Option { + opts := []openai.Option{openai.WithBaseURL(baseURL)} + switch { + case slices.Contains(builtinCompatProviders, provider): + opts = append(opts, openai.WithAPIKey(key), openai.WithAPIKeyName(keyHint)) + case key != "": + opts = append(opts, openai.WithAPIKey(key)) + // openai/openai-compatible with no explicit key keep openai.New's + // OPENAI_API_KEY default: for those names it IS the right key. + } + return opts +} + // endpointProviderNames is the operator-facing list of providers that accept an // explicit endpoint. resolveModel and endpointProvider accept the SAME set, so // one message serves both rather than each carrying a copy that drifts in @@ -106,25 +134,7 @@ func resolveModel() (llm.Model, error) { // credential rule — assuming they do produces a config that passes every // check and then 401s. if isOpenAICompatProvider(provider) { - opts := []openai.Option{openai.WithBaseURL(baseURL)} - switch { - case provider == "kimi" || provider == "qwen": - // Pass the key UNCONDITIONALLY, even when empty. openai.New defaults - // its credential to OPENAI_API_KEY, so omitting the option sends an - // OpenAI key to Moonshot or Alibaba — a credential handed to the - // wrong vendor, which is exactly what majordomo's built-ins go out - // of their way to prevent. An empty key instead yields a synthetic - // 401 naming the knob that fixes it. - opts = append(opts, - openai.WithAPIKey(apiKey), - openai.WithAPIKeyName("GADFLY_API_KEY"), - ) - case apiKey != "": - opts = append(opts, openai.WithAPIKey(apiKey)) - // openai/openai-compatible with no explicit key keep the - // OPENAI_API_KEY default: for those names it IS the right key. - } - return openai.New(opts...).Model(model) + return openai.New(openAICompatOptions(provider, baseURL, apiKey, "GADFLY_API_KEY")...).Model(model) } switch provider { @@ -299,10 +309,10 @@ func endpointProvider(name, raw string) (llm.Provider, error) { // Same shared predicate as resolveModel: the two must accept an identical // set, and a hand-copied case list cannot guarantee that. if isOpenAICompatProvider(provider) { - opts := []openai.Option{openai.WithName(name), openai.WithBaseURL(baseURL)} - if key != "" { - opts = append(opts, openai.WithAPIKey(key)) - } + // The key for a named endpoint comes from the third DSN field, so that + // is what an absent one points at. + opts := append([]openai.Option{openai.WithName(name)}, + openAICompatOptions(provider, baseURL, key, "GADFLY_ENDPOINT_"+strings.ToUpper(name))...) return openai.New(opts...), nil } diff --git a/cmd/gadfly/model_test.go b/cmd/gadfly/model_test.go index f38e1f1..1ebf2ab 100644 --- a/cmd/gadfly/model_test.go +++ b/cmd/gadfly/model_test.go @@ -1,10 +1,15 @@ package main import ( + "context" + "net/http" + "net/http/httptest" "os/exec" "path/filepath" "strings" "testing" + + llm "gitea.stevedudenhoeffer.com/steve/majordomo/llm" ) func TestEndpointProvider(t *testing.T) { @@ -226,3 +231,89 @@ func TestBuiltinCompatProvidersResolveViaRegistry(t *testing.T) { }) } } + +// TestBuiltinCompatProvidersNeverInheritOpenAIKey pins the rule that has now +// been broken on one path or the other three separate times: kimi and qwen are +// other vendors, openai.New defaults its credential to OPENAI_API_KEY, and a +// provider built without an explicit key therefore puts an OpenAI key on the +// wire to Moonshot or Alibaba. +// +// Both construction paths are asserted from one loop deliberately. Each time +// this was fixed on a single path the sibling kept leaking, so a test covering +// one of them would have passed through every one of those bugs. +// +// The provider is pointed at a LOCAL server, and the test demands two things: +// that no request arrives carrying the foreign key, and that the call fails +// closed naming the variable to set. Without the second half the test would +// pass on a provider that simply did nothing. +func TestBuiltinCompatProvidersNeverInheritOpenAIKey(t *testing.T) { + const foreign = "sk-openai-must-not-travel" + + for _, provider := range builtinCompatProviders { + t.Run(provider+" via GADFLY_BASE_URL", func(t *testing.T) { + srv, seen := leakServer(t) + t.Setenv("OPENAI_API_KEY", foreign) + t.Setenv("GADFLY_PROVIDER", provider) + t.Setenv("GADFLY_BASE_URL", srv.URL+"/v1") + t.Setenv("GADFLY_API_KEY", "") // the operator forgot the key + t.Setenv("GADFLY_MODEL", "some-model") + + m, err := resolveModel() + if err != nil { + t.Fatalf("resolveModel: %v", err) + } + assertFailsClosed(t, m, seen, foreign, "GADFLY_API_KEY") + }) + + t.Run(provider+" via GADFLY_ENDPOINT_*", func(t *testing.T) { + srv, seen := leakServer(t) + t.Setenv("OPENAI_API_KEY", foreign) + p, err := endpointProvider("ep", provider+"|"+srv.URL+"/v1") // no key field + if err != nil { + t.Fatalf("endpointProvider: %v", err) + } + m, err := p.Model("some-model") + if err != nil { + t.Fatalf("Model: %v", err) + } + assertFailsClosed(t, m, seen, foreign, "GADFLY_ENDPOINT_EP") + }) + } +} + +// leakServer returns a server that records every Authorization header it is +// sent. A request arriving at all means the client did not fail closed. +func leakServer(t *testing.T) (*httptest.Server, *[]string) { + t.Helper() + var seen []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + seen = append(seen, r.Header.Get("Authorization")) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"id":"c1","object":"chat.completion","choices":[{"index":0,` + + `"message":{"role":"assistant","content":"ok"},"finish_reason":"stop"}]}`)) + })) + t.Cleanup(srv.Close) + return srv, &seen +} + +func assertFailsClosed(t *testing.T, m llm.Model, seen *[]string, foreign, wantHint string) { + t.Helper() + _, err := m.Generate(context.Background(), llm.Request{Messages: []llm.Message{llm.UserText("hi")}}) + + for _, auth := range *seen { + if strings.Contains(auth, foreign) { + t.Errorf("Authorization carried the OpenAI key to another vendor: %q", auth) + } + } + if len(*seen) > 0 { + t.Errorf("a keyless %s provider reached the network (%d request(s)) instead of failing closed", wantHint, len(*seen)) + } + // The positive half: prove it refused for the right reason, so the test + // cannot pass on a provider that quietly did nothing at all. + if err == nil { + t.Fatal("keyless provider returned no error; expected a missing-key failure") + } + if !strings.Contains(err.Error(), wantHint) { + t.Errorf("error = %v, want it to name %s so the operator knows what to set", err, wantHint) + } +} diff --git a/scripts/preflight.sh b/scripts/preflight.sh index 26fdb77..e250611 100755 --- a/scripts/preflight.sh +++ b/scripts/preflight.sh @@ -24,7 +24,15 @@ # 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="" + local provider="$1" model="${2:-}" key_env="" key_hint="" + + # Engine specs are not majordomo providers and carry their own auth. A bare + # "claude-code" has no "/" so the caller's provider falls back to + # ollama-cloud, which would skip a reviewer that authenticates with + # CLAUDE_CODE_OAUTH_TOKEN and needs no Ollama key at all. + case "$model" in + claude-code|claude-code/*|opencode/*) echo ""; return 0 ;; + esac # Only the registry path has knowable credential rules — see above. # Trim before testing: resolveModel does strings.TrimSpace on GADFLY_BASE_URL, @@ -54,22 +62,16 @@ gadfly_preflight_key() { # 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 + 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. @@ -80,17 +82,42 @@ gadfly_preflight_key() { echo "$key_hint" } -# gadfly_preflight_providers echoes every provider this file has a credential -# arm for, one per line. +# _gadfly_preflight_table is the single source for both the credential lookup +# and the provider list: "::". # -# 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. +# 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. # -# Keep in step with the case arms above — the Go test fails if a provider in -# either list is missing from the other. +# 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' \ + '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() { - printf '%s\n' ollama-cloud qwen kimi openai openai-compatible anthropic + _gadfly_preflight_table | cut -d: -f1 } diff --git a/scripts/preflight_test.sh b/scripts/preflight_test.sh index a24fdea..57b47df 100755 --- a/scripts/preflight_test.sh +++ b/scripts/preflight_test.sh @@ -25,11 +25,11 @@ check() { # description, want, got # 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 + 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' + gadfly_preflight_key '$provider' '$model' " } @@ -84,6 +84,16 @@ echo "== a whitespace-only GADFLY_BASE_URL counts as unset, as it does in Go ==" # 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)" +check "claude-code/opus, no ollama key" "" "$(GADFLY_TEST_MODEL=claude-code/opus probe ollama-cloud)" +check "opencode/x, no ollama key" "" "$(GADFLY_TEST_MODEL=opencode/x probe ollama-cloud)" +# ...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 diff --git a/scripts/run.sh b/scripts/run.sh index cbffd7d..9684056 100644 --- a/scripts/run.sh +++ b/scripts/run.sh @@ -168,7 +168,9 @@ case "$PROVIDER" in GADFLY_PROVIDER_EFF="$MODEL_PROVIDER" # Credential pre-flight — one definition, shared with preflight_test.sh. - MISSING_KEY="$(gadfly_preflight_key "$GADFLY_PROVIDER_EFF")" + # Pass the raw spec too: engine specs (claude-code/opencode) carry their + # own auth and must not be judged by the provider fallback. + MISSING_KEY="$(gadfly_preflight_key "$GADFLY_PROVIDER_EFF" "$MODEL")" if [ -n "$MISSING_KEY" ]; then REVIEW="⚠️ No API key configured for provider \`${GADFLY_PROVIDER_EFF}\` (set \`${MISSING_KEY}\`); this reviewer was skipped." else