diff --git a/.gitea/workflows/adversarial-review.yml b/.gitea/workflows/adversarial-review.yml index 1a329d0..b1ac715 100644 --- a/.gitea/workflows/adversarial-review.yml +++ b/.gitea/workflows/adversarial-review.yml @@ -46,6 +46,16 @@ jobs: secrets: OLLAMA_CLOUD_API_KEY: ${{ secrets.OLLAMA_CLOUD_API_KEY }} CLAUDE_CODE_OAUTH_TOKEN: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} + # Forwarded so a "qwen/" or "kimi/" entry can join the + # swarm by editing the GADFLY_DEFAULT_MODELS var alone — no workflow + # edit, no re-release. Both are forwarded together on purpose: the + # reusable workflow declares both, and forwarding only one is a config + # that looks complete and 401s on the model you didn't wire. Empty until + # the repo secret exists, which is a 401 on that one model, not a broken + # review. NB kimi/ is Moonshot's own API — a different route than + # the kimi-k2.6:cloud swarm entry, which rides OLLAMA_CLOUD_API_KEY. + QWEN_API_KEY: ${{ secrets.QWEN_API_KEY }} + KIMI_API_KEY: ${{ secrets.KIMI_API_KEY }} GADFLY_FINDINGS_URL: ${{ secrets.GADFLY_FINDINGS_URL }} GADFLY_FINDINGS_TOKEN: ${{ secrets.GADFLY_FINDINGS_TOKEN }} with: diff --git a/.gitea/workflows/build-image.yml b/.gitea/workflows/build-image.yml index d1c0695..5ffc8a4 100644 --- a/.gitea/workflows/build-image.yml +++ b/.gitea/workflows/build-image.yml @@ -45,6 +45,108 @@ env: IMAGE_NAME: gitea.stevedudenhoeffer.com/steve/gadfly jobs: + # Runs alongside the image build rather than gating it: a red test should be + # loud on the PR without standing between Steve and a rebuild. Added because + # this repo had NO test job at all — `go test` and scripts/preflight_test.sh + # both existed and neither was ever executed by CI, which is worse than + # having no tests, since it reads as coverage. + test: + runs-on: ubuntu-latest + timeout-minutes: 15 + # This job executes repository code (`go test`) on pull_request, so it gets + # the narrowest token the platform will give it. Nothing here writes. + permissions: + contents: read + steps: + - uses: actions/checkout@v4 + with: + # Scrubbing the registry credential while leaving the checkout token + # in .git/config would just move the prize: `go test` below runs + # repository code with the workspace readable. Nothing in this job + # talks to git after checkout, so the token has no reason to persist. + persist-credentials: false + - uses: actions/setup-go@v5 + with: + go-version-file: go.mod + # Fetch dependencies, then DESTROY the credential before any step that + # executes repository code. REGISTRY_PASSWORD is push-capable, this repo + # is public so pull_request runs can carry attacker-authored code, and + # `go test` runs that code — a plaintext ~/.gitconfig left in place is a + # credential any test could print. The image build faces the same + # question and answers it the same way: its creds are BuildKit secrets + # scoped to the module-download RUN, never present while code runs. + - name: Fetch private modules + env: + REGISTRY_USER: ${{ secrets.REGISTRY_USER }} + REGISTRY_PASSWORD: ${{ secrets.REGISTRY_PASSWORD }} + run: | + set -euo pipefail + + # Own the config path outright. `git config --global` writes to + # GIT_CONFIG_GLOBAL, else $XDG_CONFIG_HOME/git/config when that + # directory exists, else ~/.gitconfig — so "delete ~/.gitconfig" + # scrubs a file the credential may never have been in. Naming the + # path leaves exactly one file to remove. + export GIT_CONFIG_GLOBAL="$(mktemp)" + + # Scrub on ANY exit, not just success. `set -e` means a failed + # `go mod download` aborts this step, and a cleanup written as the + # next line would never run — leaving a push-capable credential on a + # long-lived self-hosted runner for whatever job lands there next. + trap 'rm -f "$GIT_CONFIG_GLOBAL"' EXIT + + go env -w GOPRIVATE=gitea.stevedudenhoeffer.com/* + # Basic-auth header rather than credentials inside the URL: a + # password containing @ : / or # breaks URL parsing, and the failure + # would look like a bad password rather than a quoting bug. + git config --global \ + "http.https://gitea.stevedudenhoeffer.com/.extraheader" \ + "Authorization: Basic $(printf '%s:%s' "$REGISTRY_USER" "$REGISTRY_PASSWORD" | base64 | tr -d '\n')" + go mod download + + rm -f "$GIT_CONFIG_GLOBAL" + test ! -e "$GIT_CONFIG_GLOBAL" + + # Prove the scrub across the whole home dir, not just the file we + # deleted — that check would pass no matter what, and git/go can also + # 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. + # 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 + # attempt to reach the network is a bug — and it fails loudly instead of + # quietly looking for the credential that is now gone. + - name: go build + env: { GOPROXY: "off" } + run: go build ./... + - name: go vet + 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" } + run: go test -count=1 ./... + - name: pre-flight credential table + run: bash scripts/preflight_test.sh + build-and-push: runs-on: ubuntu-latest timeout-minutes: 20 diff --git a/.gitea/workflows/review-reusable.yml b/.gitea/workflows/review-reusable.yml index f1e7504..47bbdd6 100644 --- a/.gitea/workflows/review-reusable.yml +++ b/.gitea/workflows/review-reusable.yml @@ -82,6 +82,15 @@ on: OPENAI_API_KEY: { required: false } ANTHROPIC_API_KEY: { required: false } GOOGLE_API_KEY: { required: false } + # Alibaba Model Studio (Qwen), for GADFLY_MODELS entries like + # "qwen/qwen3.8-max". NOT interchangeable with OPENAI_API_KEY: majordomo's + # qwen built-in reads QWEN_API_KEY only and deliberately refuses to fall + # back to the OpenAI key, so an unforwarded secret is a 401, not a + # mis-billed OpenAI call. + QWEN_API_KEY: { required: false } + # Moonshot (Kimi) over its own API — distinct from the ollama-cloud + # "kimi-k2.6:cloud" entry, which is keyed by OLLAMA_CLOUD_API_KEY. + KIMI_API_KEY: { required: false } GADFLY_API_KEY: { required: false } CLAUDE_CODE_OAUTH_TOKEN: { required: false } GADFLY_FINDINGS_URL: { required: false } @@ -127,6 +136,12 @@ jobs: OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} GOOGLE_API_KEY: ${{ secrets.GOOGLE_API_KEY }} + # Qwen (Alibaba Model Studio) and Kimi (Moonshot) over their own APIs, + # for GADFLY_MODELS entries like "qwen/qwen3.8-max". Each built-in + # reads ONLY its own variable — no cross-provider fallback — so a + # missing line here is a clean 401, never a silently mis-keyed call. + QWEN_API_KEY: ${{ secrets.QWEN_API_KEY }} + KIMI_API_KEY: ${{ secrets.KIMI_API_KEY }} GADFLY_API_KEY: ${{ secrets.GADFLY_API_KEY }} CLAUDE_CODE_OAUTH_TOKEN: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} # Named LAN endpoints, defined in user/org vars (format diff --git a/README.md b/README.md index 3a333b3..d76a175 100644 --- a/README.md +++ b/README.md @@ -73,11 +73,43 @@ majordomo failover chain / alias) is used verbatim. | **[llama-swap](https://github.com/mostlygeek/llama-swap)** (model-swapping proxy) | `llama-swap`/`llama-swaps` (un-hyphenated `llamaswap`/`llamaswaps` also accepted) + `GADFLY_BASE_URL` or a `GADFLY_ENDPOINT_*` entry, or an `LLM_*` `llama-swap://` / `llama-swaps://` DSN | optional bearer | ⚠️ wired, **untested** | | **OpenAI-compatible** (incl. local Ollama's `/v1`) | `openai` + `GADFLY_BASE_URL` | `OPENAI_API_KEY` (any non-empty for Ollama) | ✅ tested against Ollama | | **OpenAI** | `openai` | `OPENAI_API_KEY` | ⚠️ wired, **untested** | +| **Qwen** (Alibaba Model Studio) | `qwen` | `QWEN_API_KEY` | ⚠️ wired, **untested** | +| **Kimi** (Moonshot) | `kimi` | `KIMI_API_KEY` | ⚠️ wired, **untested** | | **Anthropic** | `anthropic` | `ANTHROPIC_API_KEY` | ⚠️ wired, **untested** | | **Google (Gemini)** | `google` | `GOOGLE_API_KEY` / `GEMINI_API_KEY` | ⚠️ wired, **untested** | +Qwen and Kimi are majordomo built-ins that speak the OpenAI protocol at their own +endpoints, so `qwen/qwen3.8-max` or `kimi/kimi-k2-0711-preview` work as +`GADFLY_MODELS` entries with only the matching key set. Each reads **only** its own +variable — no cross-provider fallback — so forgetting to forward `QWEN_API_KEY` +gets you a skip notice naming it, not a mis-keyed call. Note `kimi/` (Moonshot's +API, `KIMI_API_KEY`) is a different route than the `kimi-k2.6:cloud` entry in the +default swarm, which is Ollama Cloud and keyed by `OLLAMA_CLOUD_API_KEY`. + +> **Qwen keys are endpoint-scoped, and the failure looks like a bad key.** +> Alibaba Model Studio issues *workspace-scoped* endpoints of the form +> `https://..maas.aliyuncs.com/compatible-mode/v1`. A key +> issued for one host is rejected by another with a genuine +> `401 Incorrect API key provided` — so a perfectly good key reads as invalid if +> the endpoint doesn't match. The built-in defaults to the shared international +> 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_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 -> Only the **Ollama** paths above are actually exercised. The OpenAI / Anthropic / Google +> Only the **Ollama** paths above are actually exercised. The OpenAI / Qwen / Kimi / +> Anthropic / Google > providers come "for free" from majordomo's abstraction and *should* work, but I haven't > spent money verifying them — treat them as untested. The OpenAI-**compatible** path **is** > tested, because you can point it at a local Ollama (`GADFLY_BASE_URL=http://localhost:11434/v1`) diff --git a/cmd/gadfly/model.go b/cmd/gadfly/model.go index 546641c..389623b 100644 --- a/cmd/gadfly/model.go +++ b/cmd/gadfly/model.go @@ -3,6 +3,7 @@ package main import ( "fmt" "os" + "slices" "strings" "gitea.stevedudenhoeffer.com/steve/majordomo" @@ -19,6 +20,89 @@ import ( // model list is just ids like "qwen3-coder:480b-cloud" — working unchanged. const defaultProvider = "ollama-cloud" +// openAICompatProviders are the provider names that resolve to the plain +// openai client at an explicit base URL. openai-compatible is the generic +// spelling; kimi (Moonshot) and qwen (Alibaba Model Studio) are majordomo +// built-ins that ARE that client pointed elsewhere, so an explicit endpoint for +// either belongs on the same branch. +// +// One slice, because three places must agree: resolveModel's endpoint +// override, endpointProvider's GADFLY_ENDPOINT_* parser, and the test that +// pins them. A name accepted by one and rejected by another is a config that +// works when written one way and errors the other, for no reason a user could +// 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) +} + +// 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. +// +// 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 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. + // + // The hint always names that secret, never the caller's keyHint: on the + // GADFLY_ENDPOINT_* path the caller's is the endpoint variable, and + // pointing a keyless operator at it advises them to put a credential + // somewhere Gitea does not mask. + if key == "" { + key = os.Getenv(builtinCompatKeyEnv(provider)) + } + opts = append(opts, openai.WithAPIKey(key), openai.WithAPIKeyName(builtinCompatKeyEnv(provider))) + 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 +// order and spelling. +// +// Every accepted spelling belongs here, aliases included — +// TestEndpointProviderNamesAreAllAccepted asserts that each name listed +// actually resolves, so an omission fails the build rather than misleading an +// operator who is already debugging. +const endpointProviderNames = "openai/openai-compatible/kimi/qwen/ollama/ollama-cloud/" + + "llama-swap/llama-swaps/llamaswap/llamaswaps/foreman/anthropic/google/gemini" + // resolveModel builds the review model from the environment. Gadfly is powered // by majordomo, so it can target any provider majordomo supports — Ollama // (local or cloud), OpenAI, Anthropic, Google, or any OpenAI/Ollama-compatible @@ -33,10 +117,12 @@ const defaultProvider = "ollama-cloud" // GADFLY_BASE_URL override the backend endpoint (OpenAI/Ollama-compatible // servers, a remote Ollama, an OpenRouter-style gateway…). // When set, the provider is constructed directly at that URL. -// GADFLY_API_KEY bearer/API key for the chosen provider. Optional; when -// unset the provider falls back to its standard env var -// (OLLAMA_API_KEY / OPENAI_API_KEY / ANTHROPIC_API_KEY / -// GOOGLE_API_KEY|GEMINI_API_KEY). Local Ollama needs none. +// GADFLY_API_KEY bearer/API key for the chosen provider, used ONLY on the +// GADFLY_BASE_URL override path. With no base URL the +// provider reads its own standard variable and this is never +// consulted: OLLAMA_API_KEY / OPENAI_API_KEY / +// QWEN_API_KEY / KIMI_API_KEY / ANTHROPIC_API_KEY / +// GOOGLE_API_KEY|GEMINI_API_KEY. Local Ollama needs none. // // With GADFLY_BASE_URL unset, resolution goes through majordomo's registry, so // LLM_* env DSNs and registered aliases/tiers work too. @@ -67,13 +153,17 @@ func resolveModel() (llm.Model, error) { } // Endpoint override: construct the provider directly at the given URL. + // The openai-compat family is matched by the shared predicate, not a + // repeated case list. The credential on THIS path is GADFLY_API_KEY; the + // built-ins' own KIMI_API_KEY / QWEN_API_KEY are read only on the registry + // path above, where GADFLY_BASE_URL is unset. The two paths never share a + // credential rule — assuming they do produces a config that passes every + // check and then 401s. + if isOpenAICompatProvider(provider) { + return openai.New(openAICompatOptions(provider, baseURL, apiKey, "GADFLY_API_KEY")...).Model(model) + } + switch provider { - case "openai", "openai-compatible": - opts := []openai.Option{openai.WithBaseURL(baseURL)} - if apiKey != "" { - opts = append(opts, openai.WithAPIKey(apiKey)) - } - return openai.New(opts...).Model(model) case "ollama", "ollama-cloud": opts := []ollama.Option{ollama.WithBaseURL(baseURL)} if apiKey != "" { @@ -108,7 +198,7 @@ func resolveModel() (llm.Model, error) { } return google.New(opts...).Model(model) default: - return nil, fmt.Errorf("GADFLY_BASE_URL is set but GADFLY_PROVIDER %q has no endpoint-override support (use openai/openai-compatible/ollama/llama-swap/foreman/anthropic/google, or unset GADFLY_BASE_URL to resolve via majordomo)", provider) + return nil, fmt.Errorf("GADFLY_BASE_URL is set but GADFLY_PROVIDER %q has no endpoint-override support (use %s, or unset GADFLY_BASE_URL to resolve via majordomo)", provider, endpointProviderNames) } } @@ -188,8 +278,10 @@ func modelProvider() string { // plaintext local Ollama (or foreman queue) works: // GADFLY_ENDPOINT_BIGBOX="ollama|http://192.168.1.50:11434" // GADFLY_MODEL=bigbox/qwen2.5-coder:7b -// provider is one of ollama/llama-swap(s)/foreman/openai/anthropic/google; "foreman" -// targets a foreman daemon (native Ollama on the wire): +// provider is ollama/openai/anthropic/google/foreman/llama-swap(s) or an +// openai-compat built-in (kimi, qwen) — endpointProviderNames is the +// authoritative list. "foreman" targets a foreman daemon (native Ollama +// on the wire): // GADFLY_ENDPOINT_M1="foreman|http://foreman-m1:8080|tok" // // GADFLY_ALIAS_ = "" @@ -240,6 +332,16 @@ func endpointProvider(name, raw string) (llm.Provider, error) { return nil, fmt.Errorf("missing base URL in %q", raw) } + // Same shared predicate as resolveModel: the two must accept an identical + // set, and a hand-copied case list cannot guarantee that. + if isOpenAICompatProvider(provider) { + // 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 + } + switch provider { case "ollama", "ollama-cloud": opts := []ollama.Option{ollama.WithName(name), ollama.WithBaseURL(baseURL)} @@ -258,12 +360,6 @@ func endpointProvider(name, raw string) (llm.Provider, error) { // its non-streaming degradation. Unlike the HTTPS-only LLM_* foreman:// // DSN, the base URL here is verbatim, so a plaintext http:// foreman works. return ollama.Foreman(baseURL, key, ollama.WithName(name)), nil - case "openai", "openai-compatible": - opts := []openai.Option{openai.WithName(name), openai.WithBaseURL(baseURL)} - if key != "" { - opts = append(opts, openai.WithAPIKey(key)) - } - return openai.New(opts...), nil case "anthropic": opts := []anthropic.Option{anthropic.WithName(name), anthropic.WithBaseURL(baseURL)} if key != "" { @@ -277,6 +373,6 @@ func endpointProvider(name, raw string) (llm.Provider, error) { } return google.New(opts...), nil default: - return nil, fmt.Errorf("unknown provider %q (use ollama/llama-swap(s)/foreman/openai/openai-compatible/anthropic/google)", provider) + return nil, fmt.Errorf("unknown provider %q (use %s)", provider, endpointProviderNames) } } diff --git a/cmd/gadfly/model_test.go b/cmd/gadfly/model_test.go index d77c68f..8c57826 100644 --- a/cmd/gadfly/model_test.go +++ b/cmd/gadfly/model_test.go @@ -1,6 +1,17 @@ package main -import "testing" +import ( + "context" + "net/http" + "net/http/httptest" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" + + llm "gitea.stevedudenhoeffer.com/steve/majordomo/llm" +) func TestEndpointProvider(t *testing.T) { t.Run("ollama http endpoint registers under its name", func(t *testing.T) { @@ -68,6 +79,70 @@ func TestEndpointProvider(t *testing.T) { } } +// TestOpenAICompatProvidersResolveOnBothPaths pins the two provider switches +// together. kimi and qwen are majordomo built-ins that ARE the openai client at +// a different base URL, and two independent places have to know it: +// resolveModel's GADFLY_BASE_URL override, and endpointProvider's +// GADFLY_ENDPOINT_* parser. A name accepted by one and rejected by the other is +// a provider that works when configured one way and errors the other, for no +// reason a user could guess. Asserting both from one table makes the pair fail +// together. +func TestOpenAICompatProvidersResolveOnBothPaths(t *testing.T) { + // Ranges the SHARED slice: a test that pins a list against drift must not + // be able to drift from it. + for _, provider := range openAICompatProviders { + t.Run(provider+" via GADFLY_ENDPOINT_*", func(t *testing.T) { + p, err := endpointProvider("ep", provider+"|https://host.example/v1|sk-x") + if err != nil { + t.Fatalf("endpointProvider(%q): %v", provider, err) + } + if p.Name() != "ep" { + t.Errorf("Name() = %q, want %q", p.Name(), "ep") + } + }) + t.Run(provider+" via GADFLY_BASE_URL", func(t *testing.T) { + t.Setenv("GADFLY_PROVIDER", provider) + t.Setenv("GADFLY_BASE_URL", "https://host.example/v1") + t.Setenv("GADFLY_API_KEY", "sk-x") + t.Setenv("GADFLY_MODEL", "some-model") + if _, err := resolveModel(); err != nil { + t.Fatalf("resolveModel with GADFLY_PROVIDER=%q: %v", provider, err) + } + }) + } +} + +// TestEndpointProviderNamesAreAllAccepted keeps the operator-facing list +// honest: every name endpointProviderNames advertises must actually resolve. +// The constant is read by somebody whose config just failed, so a name listed +// there and rejected by the code sends them to debug a spelling that was never +// going to work. +func TestEndpointProviderNamesAreAllAccepted(t *testing.T) { + for _, name := range strings.Split(endpointProviderNames, "/") { + name = strings.TrimSpace(name) + if name == "" { + continue + } + // Both switches, not one: this constant is the error text for BOTH + // GADFLY_ENDPOINT_* and GADFLY_BASE_URL, so a name accepted by only + // half of them still misleads whichever operator hits the other path. + t.Run(name+" via GADFLY_ENDPOINT_*", func(t *testing.T) { + if _, err := endpointProvider("ep", name+"|https://host.example/v1|sk-x"); err != nil { + t.Errorf("endpointProviderNames advertises %q but endpointProvider rejects it: %v", name, err) + } + }) + t.Run(name+" via GADFLY_BASE_URL", func(t *testing.T) { + t.Setenv("GADFLY_PROVIDER", name) + t.Setenv("GADFLY_BASE_URL", "https://host.example/v1") + t.Setenv("GADFLY_API_KEY", "sk-x") + t.Setenv("GADFLY_MODEL", "some-model") + if _, err := resolveModel(); err != nil { + t.Errorf("endpointProviderNames advertises %q but resolveModel rejects it: %v", name, err) + } + }) + } +} + func TestBuildSpec(t *testing.T) { tests := []struct { name string @@ -89,3 +164,220 @@ func TestBuildSpec(t *testing.T) { }) } } + +// TestOpenAICompatProvidersAreFullyWired closes the two remaining ways this +// provider family can be half-added. Adding one means touching three places in +// two languages, and nothing but this test connects them: +// +// - endpointProviderNames is the operator-facing list. A provider the code +// accepts but the list omits sends someone debugging a name that works. +// - scripts/preflight.sh needs a credential arm, or a missing key for that +// provider skips the pre-flight and arrives as five unexplained per-lens +// failures — the exact thing the pre-flight exists to replace. +// +// The shell side is queried, not parsed: preflight.sh exports +// gadfly_preflight_providers precisely so this test asks it what it covers. +// Regexing the case statement would make that file's formatting a contract no +// linter enforces, and a harmless reformat would fail a test in another +// language. +func TestOpenAICompatProvidersAreFullyWired(t *testing.T) { + advertised := make(map[string]bool) + for _, n := range strings.Split(endpointProviderNames, "/") { + advertised[strings.TrimSpace(n)] = true + } + + // 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) + } + preflighted := make(map[string]bool) + for _, line := range strings.Fields(string(out)) { + preflighted[line] = true + } + if len(preflighted) == 0 { + t.Fatal("gadfly_preflight_providers returned nothing — this test would pass vacuously") + } + + for _, p := range openAICompatProviders { + if !advertised[p] { + t.Errorf("openAICompatProviders has %q but endpointProviderNames does not list it — "+ + "the error message operators read would omit a name that works", p) + } + if !preflighted[p] { + t.Errorf("openAICompatProviders has %q but scripts/preflight.sh has no credential arm for it — "+ + "a missing key for %s would skip the pre-flight and surface as unexplained lens failures", p, p) + } + } +} + +// TestBuiltinCompatProvidersResolveViaRegistry exercises the PRIMARY path: +// a plain "qwen/" in GADFLY_MODELS, with no GADFLY_BASE_URL, resolved +// through majordomo's registry rather than constructed here. +// +// Every other test in this file builds the client directly, so all of them +// passed against a majordomo release that had never heard of qwen — the +// dependency bump this feature depends on was missing and nothing said so. A +// compile error eventually caught it, which is luck, not cover. +func TestBuiltinCompatProvidersResolveViaRegistry(t *testing.T) { + for _, spec := range []string{"qwen/qwen3.8-max", "kimi/kimi-k2-0711-preview"} { + t.Run(spec, func(t *testing.T) { + t.Setenv("GADFLY_MODEL", spec) + t.Setenv("GADFLY_BASE_URL", "") + t.Setenv("GADFLY_PROVIDER", "") + if _, err := resolveModel(); err != nil { + t.Fatalf("resolveModel(%q): %v — the pinned majordomo may not "+ + "provide this built-in; a bump is required, not just gadfly-side wiring", spec, err) + } + }) + } +} + +// 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, "QWEN_API_KEY", "KIMI_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, "QWEN_API_KEY", "KIMI_API_KEY") + }) + } +} + +// 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 string, wantAnyHint ...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 provider reached the network (%d request(s)) instead of failing closed", 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") + } + // The hint must name a MASKED secret the operator can set, never the + // unmasked GADFLY_ENDPOINT_* variable. + named := false + for _, h := range wantAnyHint { + if strings.Contains(err.Error(), h) { + named = true + } + } + if !named { + t.Errorf("error = %v, want it to name one of %v so the operator knows what to set", err, wantAnyHint) + } +} + +// 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/entrypoint.sh b/entrypoint.sh index 43147de..a9e3a7f 100644 --- a/entrypoint.sh +++ b/entrypoint.sh @@ -33,9 +33,17 @@ # Optional config: # GADFLY_MODELS comma-separated model ids/specs (alias: OLLAMA_REVIEW_MODELS) # GADFLY_PROVIDER majordomo provider for bare model ids (default ollama-cloud; -# e.g. "ollama" local, "openai", "anthropic", "google") +# e.g. "ollama" local, "openai", "anthropic", "google", +# "qwen" Alibaba Model Studio, "kimi" Moonshot) # GADFLY_BASE_URL override backend endpoint (OpenAI/Ollama-compatible servers) # GADFLY_API_KEY provider key (else provider's standard env: OPENAI_API_KEY, …) +# QWEN_API_KEY Alibaba Model Studio key, for GADFLY_MODELS entries like +# "qwen/qwen3.8-max". Read ONLY by the qwen provider — it +# does not fall back to OPENAI_API_KEY, so a forgotten key +# is a clean skip notice naming this variable, not a 401. +# KIMI_API_KEY Moonshot key, same deal for "kimi/". Distinct from +# the ollama-cloud "kimi-k2.6:cloud" entry, which is keyed +# by OLLAMA_CLOUD_API_KEY. # CLAUDE_CODE_OAUTH_TOKEN auth for the claude-code engine (GADFLY_MODELS entry # "claude-code"/"claude-code/"); Pro/Max subscription # token from `claude setup-token`. Else ANTHROPIC_API_KEY. diff --git a/go.mod b/go.mod index efecde8..1534410 100644 --- a/go.mod +++ b/go.mod @@ -4,7 +4,7 @@ go 1.26.2 require ( gitea.stevedudenhoeffer.com/steve/executus v0.1.4 - gitea.stevedudenhoeffer.com/steve/majordomo v0.0.0-20260627225659-aa25b2c33462 + gitea.stevedudenhoeffer.com/steve/majordomo v0.0.0-20260812210334-f837115a55c9 gopkg.in/yaml.v3 v3.0.1 ) diff --git a/go.sum b/go.sum index 581c478..d8be64c 100644 --- a/go.sum +++ b/go.sum @@ -6,8 +6,8 @@ cloud.google.com/go/compute/metadata v0.9.0 h1:pDUj4QMoPejqq20dK0Pg2N4yG9zIkYGdB cloud.google.com/go/compute/metadata v0.9.0/go.mod h1:E0bWwX5wTnLPedCKqk3pJmVgCBSM6qQI1yTBdEb3C10= gitea.stevedudenhoeffer.com/steve/executus v0.1.4 h1:4F99uCV3OVaE9ITFp0FjPiYxLUQO+WpE+wU2HCnpXNM= gitea.stevedudenhoeffer.com/steve/executus v0.1.4/go.mod h1:WQP/lH+meU06OSNF0TQO/wQLcJCrMwpi0EMj5vSpVtk= -gitea.stevedudenhoeffer.com/steve/majordomo v0.0.0-20260627225659-aa25b2c33462 h1:1crjE1YkWHLZ91tUDOxN/Y5cuOnJ56e0U9UADoFfEPY= -gitea.stevedudenhoeffer.com/steve/majordomo v0.0.0-20260627225659-aa25b2c33462/go.mod h1:UZLveG17SmENt4sne2RSLIbioix30RZbRIQUzBAnOyY= +gitea.stevedudenhoeffer.com/steve/majordomo v0.0.0-20260812210334-f837115a55c9 h1:ExY2S6RN1UaA97ju4jzkuEGpfBx0p3vv9FY8B7Npy2I= +gitea.stevedudenhoeffer.com/steve/majordomo v0.0.0-20260812210334-f837115a55c9/go.mod h1:UZLveG17SmENt4sne2RSLIbioix30RZbRIQUzBAnOyY= github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs= github.com/cespare/xxhash/v2 v2.3.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= diff --git a/scripts/preflight.sh b/scripts/preflight.sh new file mode 100755 index 0000000..e323a23 --- /dev/null +++ b/scripts/preflight.sh @@ -0,0 +1,135 @@ +#!/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 -> 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: "::". +# +# 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 +} diff --git a/scripts/preflight_test.sh b/scripts/preflight_test.sh new file mode 100755 index 0000000..64e6ef6 --- /dev/null +++ b/scripts/preflight_test.sh @@ -0,0 +1,115 @@ +#!/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 [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" diff --git a/scripts/run.sh b/scripts/run.sh index 6dc2944..9684056 100644 --- a/scripts/run.sh +++ b/scripts/run.sh @@ -48,6 +48,11 @@ set -uo pipefail SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" MAX_DIFF_CHARS="${MAX_DIFF_CHARS:-60000}" +# Credential pre-flight, shared verbatim with scripts/preflight_test.sh so the +# tested logic and the running logic are the same bytes. +# shellcheck source=preflight.sh +. "$SCRIPT_DIR/preflight.sh" + : "${GITEA_API:?GITEA_API required}" : "${GITEA_TOKEN:?GITEA_TOKEN required}" : "${PR:?PR required}" @@ -162,10 +167,12 @@ case "$PROVIDER" in fi GADFLY_PROVIDER_EFF="$MODEL_PROVIDER" - # Only the default cloud provider strictly needs a key up front; local Ollama - # and other providers either need none or read their own standard env var. - if [ "$GADFLY_PROVIDER_EFF" = "ollama-cloud" ] && [ -z "${OLLAMA_API_KEY:-}" ] && [ -z "${GADFLY_API_KEY:-}" ]; then - REVIEW="⚠️ No Ollama Cloud key configured (set \`OLLAMA_CLOUD_API_KEY\`) and \`GADFLY_PROVIDER\` is the default \`ollama-cloud\`; this reviewer was skipped." + # Credential pre-flight — one definition, shared with preflight_test.sh. + # 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 BIN="${GADFLY_BIN:-gadfly}" if ! command -v "$BIN" >/dev/null 2>&1 && [ ! -x "$BIN" ]; then