diff --git a/.gitea/workflows/build-image.yml b/.gitea/workflows/build-image.yml index b341cc9..ff62f3a 100644 --- a/.gitea/workflows/build-image.yml +++ b/.gitea/workflows/build-image.yml @@ -135,6 +135,7 @@ jobs: env: { GOPROXY: "off" } run: go vet ./... - name: gofmt + env: { GOPROXY: "off" } run: test -z "$(gofmt -l .)" || { gofmt -l .; exit 1; } - name: go test env: { GOPROXY: "off" } diff --git a/README.md b/README.md index 1592557..d76a175 100644 --- a/README.md +++ b/README.md @@ -95,10 +95,16 @@ default swarm, which is Ollama Cloud and keyed by `OLLAMA_CLOUD_API_KEY`. > host; point at your own with a named endpoint, which needs no code change: > > ``` -> GADFLY_ENDPOINT_QWENWS = "qwen|https://.ap-southeast-1.maas.aliyuncs.com/compatible-mode/v1|" +> GADFLY_ENDPOINT_QWENWS = "qwen|https://.ap-southeast-1.maas.aliyuncs.com/compatible-mode/v1" > GADFLY_MODELS = "qwenws/qwen3.8-max,..." +> QWEN_API_KEY = > ``` > +> **Leave the key out of the endpoint var.** `GADFLY_ENDPOINT_*` are Gitea +> *variables*, which are not masked in logs; the third `|` field would put +> a credential there. Omit it and a `qwen`/`kimi` endpoint falls back to its own +> `QWEN_API_KEY` / `KIMI_API_KEY` secret โ€” its own vendor's key, never another's. +> > (Verified the hard way against a live deployment.) > ### ๐Ÿงช Honest status diff --git a/cmd/gadfly/model.go b/cmd/gadfly/model.go index 3589fb8..8dbd281 100644 --- a/cmd/gadfly/model.go +++ b/cmd/gadfly/model.go @@ -44,6 +44,19 @@ func isOpenAICompatProvider(name string) bool { return slices.Contains(openAICompatProviders, name) } +// isBuiltinCompatProvider mirrors isOpenAICompatProvider rather than testing +// the slice inline, so both memberships are asked the same way. +func isBuiltinCompatProvider(name string) bool { + return slices.Contains(builtinCompatProviders, name) +} + +// builtinCompatKeyEnv is the provider's own credential variable, matching the +// name majordomo's built-in reads on the registry path โ€” so the same secret +// works whether or not an explicit endpoint is configured. +func builtinCompatKeyEnv(provider string) string { + return strings.ToUpper(strings.ReplaceAll(provider, "-", "_")) + "_API_KEY" +} + // openAICompatOptions builds the option set for an openai-compat provider, and // is the ONE place the no-cross-vendor-fallback rule lives. // @@ -55,7 +68,17 @@ func isOpenAICompatProvider(name string) bool { func openAICompatOptions(provider, baseURL, key, keyHint string) []openai.Option { opts := []openai.Option{openai.WithBaseURL(baseURL)} switch { - case slices.Contains(builtinCompatProviders, provider): + case isBuiltinCompatProvider(provider): + // With no explicit key, fall back to the provider's OWN variable + // (QWEN_API_KEY, KIMI_API_KEY). That is not the cross-vendor fallback + // this function exists to prevent โ€” it is the same vendor's key โ€” and + // it lets an operator keep the credential in a masked secret while the + // endpoint URL lives in a var, which is NOT masked. + if key == "" { + if own := os.Getenv(builtinCompatKeyEnv(provider)); own != "" { + key, keyHint = own, builtinCompatKeyEnv(provider) + } + } opts = append(opts, openai.WithAPIKey(key), openai.WithAPIKeyName(keyHint)) case key != "": opts = append(opts, openai.WithAPIKey(key)) diff --git a/cmd/gadfly/model_test.go b/cmd/gadfly/model_test.go index 1ebf2ab..43db0ef 100644 --- a/cmd/gadfly/model_test.go +++ b/cmd/gadfly/model_test.go @@ -6,6 +6,7 @@ import ( "net/http/httptest" "os/exec" "path/filepath" + "runtime" "strings" "testing" @@ -185,7 +186,13 @@ func TestOpenAICompatProvidersAreFullyWired(t *testing.T) { advertised[strings.TrimSpace(n)] = true } - script := filepath.Join("..", "..", "scripts", "preflight.sh") + // Locate the script relative to THIS source file rather than the working + // directory, so moving the package does not silently break the lookup. + _, thisFile, _, ok := runtime.Caller(0) + if !ok { + t.Fatal("runtime.Caller failed; cannot locate scripts/preflight.sh") + } + script := filepath.Join(filepath.Dir(thisFile), "..", "..", "scripts", "preflight.sh") out, err := exec.Command("bash", "-c", ". "+script+"; gadfly_preflight_providers").Output() if err != nil { t.Fatalf("query gadfly_preflight_providers from %s: %v", script, err) @@ -317,3 +324,52 @@ func assertFailsClosed(t *testing.T, m llm.Model, seen *[]string, foreign, wantH t.Errorf("error = %v, want it to name %s so the operator knows what to set", err, wantHint) } } + +// TestBuiltinCompatOwnKeyFallback: with no key in the endpoint definition, a +// built-in falls back to its OWN variable (QWEN_API_KEY, KIMI_API_KEY) โ€” never +// to another vendor's. This is what lets the credential live in a masked +// secret while the endpoint URL lives in a GADFLY_ENDPOINT_* var, which Gitea +// does not mask; the README used to advise embedding the key in that var. +func TestBuiltinCompatOwnKeyFallback(t *testing.T) { + const own, foreign = "sk-qwen-own", "sk-openai-must-not-travel" + + srv, seen := leakServer(t) + t.Setenv("OPENAI_API_KEY", foreign) + t.Setenv("QWEN_API_KEY", own) + + p, err := endpointProvider("ep", "qwen|"+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) + } + if _, err := m.Generate(context.Background(), llm.Request{Messages: []llm.Message{llm.UserText("hi")}}); err != nil { + t.Fatalf("Generate: %v", err) + } + if len(*seen) == 0 { + t.Fatal("no request reached the server โ€” the own-key fallback did not take effect") + } + for _, auth := range *seen { + if strings.Contains(auth, foreign) { + t.Errorf("Authorization carried the OpenAI key: %q", auth) + } + if !strings.Contains(auth, own) { + t.Errorf("Authorization = %q, want the provider's own QWEN_API_KEY", auth) + } + } +} + +// TestBuiltinCompatProvidersAreOpenAICompat: the two slices are parallel, and a +// built-in missing from openAICompatProviders would never reach the branch that +// applies its unconditional-key rule โ€” it would fall through to the generic +// switch and silently lose the protection. +func TestBuiltinCompatProvidersAreOpenAICompat(t *testing.T) { + for _, p := range builtinCompatProviders { + if !isOpenAICompatProvider(p) { + t.Errorf("%q is in builtinCompatProviders but not openAICompatProviders, so the "+ + "no-cross-vendor-fallback branch never runs for it", p) + } + } +} diff --git a/scripts/preflight.sh b/scripts/preflight.sh index e250611..08f2312 100755 --- a/scripts/preflight.sh +++ b/scripts/preflight.sh @@ -26,12 +26,18 @@ gadfly_preflight_key() { 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. + # 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. case "$model" in - claude-code|claude-code/*|opencode/*) echo ""; return 0 ;; + claude-code|claude-code/*) echo ""; return 0 ;; esac # Only the registry path has knowable credential rules โ€” see above. @@ -46,16 +52,6 @@ gadfly_preflight_key() { 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 @@ -102,6 +98,8 @@ gadfly_preflight_key() { _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:' \ diff --git a/scripts/preflight_test.sh b/scripts/preflight_test.sh index 57b47df..c3093bc 100755 --- a/scripts/preflight_test.sh +++ b/scripts/preflight_test.sh @@ -90,7 +90,12 @@ echo "== engine specs carry their own auth and are never pre-flighted ==" # 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)" +# 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)"