From 0f40b21d79aa51766971a3587b223f364ff46c89 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Wed, 12 Aug 2026 16:30:29 -0400 Subject: [PATCH 01/12] feat(qwen): let Qwen (and Kimi) join the swarm MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit majordomo now ships qwen and kimi as built-ins that ARE the openai client at their own base URL, so "qwen/qwen3.8-max" works as a GADFLY_MODELS entry once the key reaches the container. This wires up the parts that key has to pass through. Two provider switches had to learn the names, not one. resolveModel's GADFLY_BASE_URL override was the obvious one; endpointProvider's GADFLY_ENDPOINT_* parser is its sibling, and I fixed the first and missed the second on the first pass — a config that resolves one way and errors the other for no reason a user could guess. TestOpenAICompatProvidersResolveOnBothPaths now asserts both from one table so the pair fails together; break-checked in both directions. QWEN_API_KEY (and KIMI_API_KEY) are declared as workflow_call secrets and forwarded to the container, with gadfly's own stub forwarding QWEN_API_KEY so a qwen entry can join the default swarm by editing GADFLY_DEFAULT_MODELS alone — no workflow edit, no re-release. The run.sh credential pre-flight is now a provider→variable table instead of an ollama-cloud special case. Without it a forgotten key surfaces as five identical per-lens agent failures naming no variable, and the operator reads a stack trace to find out which secret they missed. Google stays out of the table on purpose: it accepts either GOOGLE_API_KEY or GEMINI_API_KEY, and a one-var entry would wrongly skip a correctly-configured run. Verified across 17 provider x key-state combinations, including that a wrong-provider key never satisfies qwen (majordomo refuses cross-provider fallback) and that unkeyed providers are never blocked. Co-Authored-By: Claude Opus 5 (1M context) --- .gitea/workflows/adversarial-review.yml | 5 ++++ .gitea/workflows/review-reusable.yml | 15 ++++++++++++ README.md | 13 ++++++++++- cmd/gadfly/model.go | 19 +++++++++++---- cmd/gadfly/model_test.go | 31 +++++++++++++++++++++++++ entrypoint.sh | 10 +++++++- scripts/run.sh | 24 +++++++++++++++---- 7 files changed, 107 insertions(+), 10 deletions(-) diff --git a/.gitea/workflows/adversarial-review.yml b/.gitea/workflows/adversarial-review.yml index 1a329d0..dbd530f 100644 --- a/.gitea/workflows/adversarial-review.yml +++ b/.gitea/workflows/adversarial-review.yml @@ -46,6 +46,11 @@ jobs: secrets: OLLAMA_CLOUD_API_KEY: ${{ secrets.OLLAMA_CLOUD_API_KEY }} CLAUDE_CODE_OAUTH_TOKEN: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} + # Forwarded so a "qwen/" entry can join the swarm by editing the + # GADFLY_DEFAULT_MODELS var alone — no workflow edit, no re-release. + # Empty until the repo secret exists: that's a 401 on that one model, + # not a broken review. + QWEN_API_KEY: ${{ secrets.QWEN_API_KEY }} GADFLY_FINDINGS_URL: ${{ secrets.GADFLY_FINDINGS_URL }} GADFLY_FINDINGS_TOKEN: ${{ secrets.GADFLY_FINDINGS_TOKEN }} with: 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..825513b 100644 --- a/README.md +++ b/README.md @@ -73,11 +73,22 @@ 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`. + > ### 🧪 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..c25dd88 100644 --- a/cmd/gadfly/model.go +++ b/cmd/gadfly/model.go @@ -68,7 +68,14 @@ func resolveModel() (llm.Model, error) { // Endpoint override: construct the provider directly at the given URL. switch provider { - case "openai", "openai-compatible": + case "openai", "openai-compatible", "kimi", "qwen": + // kimi (Moonshot) and qwen (Alibaba Model Studio) are majordomo + // built-ins that ARE the openai client at a different base URL, so an + // explicit GADFLY_BASE_URL for either belongs here. Without these names + // the override fell through to default: and errored, even though both + // resolve fine on the registry path above — a confusing asymmetry. + // The credential here is GADFLY_API_KEY; the built-ins' own + // KIMI_API_KEY / QWEN_API_KEY apply only when GADFLY_BASE_URL is unset. opts := []openai.Option{openai.WithBaseURL(baseURL)} if apiKey != "" { opts = append(opts, openai.WithAPIKey(apiKey)) @@ -108,7 +115,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 openai/openai-compatible/kimi/qwen/ollama/llama-swap/foreman/anthropic/google, or unset GADFLY_BASE_URL to resolve via majordomo)", provider) } } @@ -258,7 +265,11 @@ 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": + case "openai", "openai-compatible", "kimi", "qwen": + // kimi/qwen accepted here for the same reason as in resolveModel: both + // majordomo built-ins ARE the openai client at their own base URL, so a + // named endpoint pointing at one (a regional Model Studio host, say) + // must resolve, not error. The two switches move together. opts := []openai.Option{openai.WithName(name), openai.WithBaseURL(baseURL)} if key != "" { opts = append(opts, openai.WithAPIKey(key)) @@ -277,6 +288,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 ollama/llama-swap(s)/foreman/openai/openai-compatible/kimi/qwen/anthropic/google)", provider) } } diff --git a/cmd/gadfly/model_test.go b/cmd/gadfly/model_test.go index d77c68f..d8ba84a 100644 --- a/cmd/gadfly/model_test.go +++ b/cmd/gadfly/model_test.go @@ -68,6 +68,37 @@ 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. Adding a name to one and not the other yields a +// provider that works when configured one way and errors the other, for no +// reason a user could guess — which is exactly what happened here on the first +// pass. Asserting both in one table is what makes the pair fail together. +func TestOpenAICompatProvidersResolveOnBothPaths(t *testing.T) { + for _, provider := range []string{"openai", "openai-compatible", "kimi", "qwen"} { + 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) + } + }) + } +} + func TestBuildSpec(t *testing.T) { tests := []struct { name string 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/scripts/run.sh b/scripts/run.sh index 6dc2944..6230a23 100644 --- a/scripts/run.sh +++ b/scripts/run.sh @@ -162,10 +162,26 @@ 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." + # Pre-flight the credential for providers that need one. Why bother, when + # majordomo already fails closed with a 401: without this, a missing key + # surfaces as five identical per-lens agent failures that name no variable, + # and the operator has to read a stack trace to learn which secret they + # forgot to forward. Providers absent from this table need no key (local + # ollama, llama-swap) or carry it in their endpoint/DSN (foreman), and are + # left alone. GADFLY_API_KEY overrides any of them. + KEY_ENV=""; KEY_HINT="" + case "$GADFLY_PROVIDER_EFF" in + ollama-cloud) KEY_ENV="OLLAMA_API_KEY"; KEY_HINT="OLLAMA_CLOUD_API_KEY" ;; + qwen) KEY_ENV="QWEN_API_KEY"; KEY_HINT="QWEN_API_KEY" ;; + kimi) KEY_ENV="KIMI_API_KEY"; KEY_HINT="KIMI_API_KEY" ;; + openai) KEY_ENV="OPENAI_API_KEY"; KEY_HINT="OPENAI_API_KEY" ;; + anthropic) KEY_ENV="ANTHROPIC_API_KEY"; KEY_HINT="ANTHROPIC_API_KEY" ;; + esac + # Indirect expansion (bash), so the table above stays a table. 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" ] && [ -z "${!KEY_ENV:-}" ] && [ -z "${GADFLY_API_KEY:-}" ]; then + REVIEW="⚠️ No API key configured for provider \`${GADFLY_PROVIDER_EFF}\` (set \`${KEY_HINT}\`, or \`GADFLY_API_KEY\`); this reviewer was skipped." else BIN="${GADFLY_BIN:-gadfly}" if ! command -v "$BIN" >/dev/null 2>&1 && [ ! -x "$BIN" ]; then From 2367e696b586b570930ad406e15eb5624f40beb4 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Wed, 12 Aug 2026 16:43:00 -0400 Subject: [PATCH 02/12] =?UTF-8?q?fix(qwen):=20gadfly=20round=201=20?= =?UTF-8?q?=E2=80=94=20three=20real=20findings,=20all=20sibling=20drift?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pre-flight comment was the worst of them, and three models agreed. It said providers absent from the table "need no key or carry it in their endpoint/DSN" — false for google, which needs a key and is absent for an entirely different reason: it accepts GOOGLE_API_KEY *or* GEMINI_API_KEY, so a single-variable arm would silently skip a correctly-configured reviewer. That reasoning was in the PR description and not in the code, so the comment invited exactly the wrong edit. It now states both exclusion reasons and names google's. Forwarded KIMI_API_KEY alongside QWEN_API_KEY in the dogfooding stub. This PR argues that sibling call sites must move together, and I declared both secrets in the reusable workflow and forwarded one — a config that looks complete and 401s on the model you didn't wire. The two endpoint-provider error messages listed the same accepted set in different order and spelling. Both functions accept an identical set, so they now share one endpointProviderNames constant and cannot disagree. Co-Authored-By: Claude Opus 5 (1M context) --- .gitea/workflows/adversarial-review.yml | 13 +++++++++---- cmd/gadfly/model.go | 11 +++++++++-- scripts/run.sh | 16 +++++++++++++--- 3 files changed, 31 insertions(+), 9 deletions(-) diff --git a/.gitea/workflows/adversarial-review.yml b/.gitea/workflows/adversarial-review.yml index dbd530f..b1ac715 100644 --- a/.gitea/workflows/adversarial-review.yml +++ b/.gitea/workflows/adversarial-review.yml @@ -46,11 +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/" entry can join the swarm by editing the - # GADFLY_DEFAULT_MODELS var alone — no workflow edit, no re-release. - # Empty until the repo secret exists: that's a 401 on that one model, - # not a broken review. + # 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/cmd/gadfly/model.go b/cmd/gadfly/model.go index c25dd88..e3ff47c 100644 --- a/cmd/gadfly/model.go +++ b/cmd/gadfly/model.go @@ -19,6 +19,13 @@ import ( // model list is just ids like "qwen3-coder:480b-cloud" — working unchanged. const defaultProvider = "ollama-cloud" +// endpointProviderNames is the operator-facing list of providers that accept an +// explicit endpoint. resolveModel and endpointProvider accept the SAME set, so +// they share one message rather than each carrying a hand-maintained copy that +// drifts in order and spelling — which is exactly what happened when kimi/qwen +// were added to both switches. +const endpointProviderNames = "openai/openai-compatible/kimi/qwen/ollama/ollama-cloud/llama-swap(s)/foreman/anthropic/google" + // 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 @@ -115,7 +122,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/kimi/qwen/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) } } @@ -288,6 +295,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/kimi/qwen/anthropic/google)", provider) + return nil, fmt.Errorf("unknown provider %q (use %s)", provider, endpointProviderNames) } } diff --git a/scripts/run.sh b/scripts/run.sh index 6230a23..c09caca 100644 --- a/scripts/run.sh +++ b/scripts/run.sh @@ -166,9 +166,19 @@ case "$PROVIDER" in # majordomo already fails closed with a 401: without this, a missing key # surfaces as five identical per-lens agent failures that name no variable, # and the operator has to read a stack trace to learn which secret they - # forgot to forward. Providers absent from this table need no key (local - # ollama, llama-swap) or carry it in their endpoint/DSN (foreman), and are - # left alone. GADFLY_API_KEY overrides any of them. + # forgot to forward. GADFLY_API_KEY overrides any entry. + # + # 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 has more than one acceptable variable, so a + # single-variable check would skip a correctly-configured run. + # **google** is this case: it accepts GOOGLE_API_KEY *or* + # GEMINI_API_KEY. Adding `google) KEY_ENV="GOOGLE_API_KEY"` here would + # silently skip every reviewer configured with GEMINI_API_KEY. If you + # want google pre-flighted, the check has to accept either variable, + # not the table's one-name shape. KEY_ENV=""; KEY_HINT="" case "$GADFLY_PROVIDER_EFF" in ollama-cloud) KEY_ENV="OLLAMA_API_KEY"; KEY_HINT="OLLAMA_CLOUD_API_KEY" ;; From 1d6eaa08c5986b5b330f61da003f4c84ce89e38b Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Wed, 12 Aug 2026 16:59:01 -0400 Subject: [PATCH 03/12] =?UTF-8?q?fix(qwen):=20gadfly=20round=202=20?= =?UTF-8?q?=E2=80=94=20the=20anti-drift=20list=20had=20already=20drifted?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eight findings, all real, and the sharpest ones are about this PR's own fixes. GADFLY_API_KEY was treated as a universal substitute in the pre-flight. It is not: resolveModel reads it only AFTER the `baseURL == ""` early return, so on the registry path — the documented primary path — a qwen/kimi built-in reads its own variable and GADFLY_API_KEY is never consulted. A mis-set GADFLY_API_KEY therefore passed pre-flight and 401'd five times anyway, which is precisely the failure this check exists to prevent. It now only substitutes when GADFLY_BASE_URL is also set. `openai-compatible` was missing from the pre-flight table while both switches accept it as an OPENAI_API_KEY alias, so that one spelling still fell through to the cryptic five-failure mode. endpointProviderNames — the constant I introduced *to stop* the two error messages drifting — omitted the `gemini` alias both switches accept. It now lists every accepted spelling. And the case list itself was still duplicated across both switches plus the test that pins them: three copies of the thing whose duplication started this. Both switches now call isOpenAICompatProvider over one shared slice, and endpointProvider's doc comment points at endpointProviderNames instead of carrying a fourth hand-written copy. scripts/preflight_test.sh moves into the repo (20 cases, up from 17, covering openai-compatible and both GADFLY_API_KEY directions). It carries a drift guard that diffs its copy of the provider table against run.sh's and aborts if they differ — break-checked by deleting an arm from run.sh, which fails it loudly. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/gadfly/model.go | 63 +++++++++++++++++--------- scripts/preflight_test.sh | 93 +++++++++++++++++++++++++++++++++++++++ scripts/run.sh | 26 ++++++----- 3 files changed, 151 insertions(+), 31 deletions(-) create mode 100644 scripts/preflight_test.sh diff --git a/cmd/gadfly/model.go b/cmd/gadfly/model.go index e3ff47c..33f2def 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,12 +20,33 @@ 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. +// +// This is a slice rather than three copies of a case list because there are +// three places that must agree — resolveModel's switch, endpointProvider's +// switch, and the test that pins them — and the first version of this change +// added the names to one switch and not the other. +var openAICompatProviders = []string{"openai", "openai-compatible", "kimi", "qwen"} + +func isOpenAICompatProvider(name string) bool { + return slices.Contains(openAICompatProviders, name) +} + // endpointProviderNames is the operator-facing list of providers that accept an // explicit endpoint. resolveModel and endpointProvider accept the SAME set, so // they share one message rather than each carrying a hand-maintained copy that // drifts in order and spelling — which is exactly what happened when kimi/qwen // were added to both switches. -const endpointProviderNames = "openai/openai-compatible/kimi/qwen/ollama/ollama-cloud/llama-swap(s)/foreman/anthropic/google" +// +// Keep every accepted spelling here, including aliases: the first version of +// this constant dropped "gemini", so the list written to prevent drift had +// already drifted from the switches it describes. +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 @@ -74,20 +96,19 @@ func resolveModel() (llm.Model, error) { } // Endpoint override: construct the provider directly at the given URL. - switch provider { - case "openai", "openai-compatible", "kimi", "qwen": - // kimi (Moonshot) and qwen (Alibaba Model Studio) are majordomo - // built-ins that ARE the openai client at a different base URL, so an - // explicit GADFLY_BASE_URL for either belongs here. Without these names - // the override fell through to default: and errored, even though both - // resolve fine on the registry path above — a confusing asymmetry. - // The credential here is GADFLY_API_KEY; the built-ins' own - // KIMI_API_KEY / QWEN_API_KEY apply only when GADFLY_BASE_URL is unset. + // The openai-compat family (openai/openai-compatible/kimi/qwen) is matched + // by the shared predicate, not a repeated case list. The credential here is + // GADFLY_API_KEY; the built-ins' own KIMI_API_KEY / QWEN_API_KEY apply only + // on the registry path above, where GADFLY_BASE_URL is unset. + if isOpenAICompatProvider(provider) { opts := []openai.Option{openai.WithBaseURL(baseURL)} if apiKey != "" { opts = append(opts, openai.WithAPIKey(apiKey)) } return openai.New(opts...).Model(model) + } + + switch provider { case "ollama", "ollama-cloud": opts := []ollama.Option{ollama.WithBaseURL(baseURL)} if apiKey != "" { @@ -202,7 +223,7 @@ 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" +// provider is one of endpointProviderNames; "foreman" // targets a foreman daemon (native Ollama on the wire): // GADFLY_ENDPOINT_M1="foreman|http://foreman-m1:8080|tok" // @@ -254,6 +275,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 hand-copied case lists are how they drifted apart before. + if isOpenAICompatProvider(provider) { + opts := []openai.Option{openai.WithName(name), openai.WithBaseURL(baseURL)} + if key != "" { + opts = append(opts, openai.WithAPIKey(key)) + } + return openai.New(opts...), nil + } + switch provider { case "ollama", "ollama-cloud": opts := []ollama.Option{ollama.WithName(name), ollama.WithBaseURL(baseURL)} @@ -272,16 +303,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", "kimi", "qwen": - // kimi/qwen accepted here for the same reason as in resolveModel: both - // majordomo built-ins ARE the openai client at their own base URL, so a - // named endpoint pointing at one (a regional Model Studio host, say) - // must resolve, not error. The two switches move together. - 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 != "" { diff --git a/scripts/preflight_test.sh b/scripts/preflight_test.sh new file mode 100644 index 0000000..8bd64bd --- /dev/null +++ b/scripts/preflight_test.sh @@ -0,0 +1,93 @@ +#!/usr/bin/env bash +# Exercise the run.sh credential pre-flight in isolation: every provider x +# key-present/absent x GADFLY_API_KEY/GADFLY_BASE_URL combination. +# +# The preflight() below is a COPY of run.sh's logic, which makes it exactly the +# kind of duplicated pair this whole PR keeps finding. The drift guard runs +# first: it compares the provider→variable arms in both files and aborts if +# they differ, so a table edited in run.sh and not here fails loudly instead of +# certifying stale logic. +set -u + +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +RUN_SH="${RUN_SH:-$SCRIPT_DIR/run.sh}" + +# Normalized case arms: "provider) KEY_ENV=... KEY_HINT=..." with runs of +# whitespace collapsed, so alignment changes don't trip the guard. +arms_of() { grep -oE '^[[:space:]]*[a-z|-]+\)[[:space:]]+KEY_ENV="[A-Z_]+";[[:space:]]+KEY_HINT="[A-Z_]+"' "$1" | tr -s ' \t' ' ' | sed 's/^ //'; } + +if [ -r "$RUN_SH" ]; then + if ! diff <(arms_of "$RUN_SH") <(arms_of "$0") >/dev/null; then + echo "FAIL drift guard: the pre-flight table here no longer matches $RUN_SH" + diff <(arms_of "$RUN_SH") <(arms_of "$0") | sed 's/^/ /' + exit 1 + fi + echo "ok drift guard: table matches run.sh" +else + echo "FAIL drift guard: cannot read $RUN_SH — cannot prove this tests the real table" + exit 1 +fi + +preflight() { # $1=provider ; env carries the keys + local GADFLY_PROVIDER_EFF="$1" KEY_ENV="" KEY_HINT="" KEY_OK=0 + case "$GADFLY_PROVIDER_EFF" in + ollama-cloud) KEY_ENV="OLLAMA_API_KEY"; KEY_HINT="OLLAMA_CLOUD_API_KEY" ;; + qwen) KEY_ENV="QWEN_API_KEY"; KEY_HINT="QWEN_API_KEY" ;; + kimi) KEY_ENV="KIMI_API_KEY"; KEY_HINT="KIMI_API_KEY" ;; + openai|openai-compatible) KEY_ENV="OPENAI_API_KEY"; KEY_HINT="OPENAI_API_KEY" ;; + anthropic) KEY_ENV="ANTHROPIC_API_KEY"; KEY_HINT="ANTHROPIC_API_KEY" ;; + esac + [ -n "$KEY_ENV" ] && [ -n "${!KEY_ENV:-}" ] && KEY_OK=1 + [ -n "${GADFLY_BASE_URL:-}" ] && [ -n "${GADFLY_API_KEY:-}" ] && KEY_OK=1 + if [ -n "$KEY_ENV" ] && [ "$KEY_OK" -eq 0 ]; then + echo "SKIP:$KEY_HINT" + else + echo "RUN" + fi +} + +fail=0 +check() { # desc, want, got + if [ "$2" = "$3" ]; then echo "ok $1"; else echo "FAIL $1 — want $2, got $3"; fail=1; fi +} + +env -i bash -c 'true' >/dev/null 2>&1 # sanity + +# --- keyed providers with NO key -> skip, naming the right variable --- +check "qwen, no key" "SKIP:QWEN_API_KEY" "$(env -u QWEN_API_KEY -u GADFLY_API_KEY bash -c "$(declare -f preflight); preflight qwen")" +check "kimi, no key" "SKIP:KIMI_API_KEY" "$(env -u KIMI_API_KEY -u GADFLY_API_KEY bash -c "$(declare -f preflight); preflight kimi")" +check "ollama-cloud, none" "SKIP:OLLAMA_CLOUD_API_KEY" "$(env -u OLLAMA_API_KEY -u GADFLY_API_KEY bash -c "$(declare -f preflight); preflight ollama-cloud")" +check "openai, no key" "SKIP:OPENAI_API_KEY" "$(env -u OPENAI_API_KEY -u GADFLY_API_KEY bash -c "$(declare -f preflight); preflight openai")" +check "anthropic, no key" "SKIP:ANTHROPIC_API_KEY" "$(env -u ANTHROPIC_API_KEY -u GADFLY_API_KEY bash -c "$(declare -f preflight); preflight anthropic")" + +# --- keyed providers WITH their key -> run --- +check "qwen, keyed" "RUN" "$(env -u GADFLY_API_KEY -u GADFLY_BASE_URL QWEN_API_KEY=k bash -c "$(declare -f preflight); preflight qwen")" +check "ollama-cloud, keyed" "RUN" "$(env -u GADFLY_API_KEY -u GADFLY_BASE_URL OLLAMA_API_KEY=k bash -c "$(declare -f preflight); preflight ollama-cloud")" + +# --- the WRONG key must not satisfy a provider (no cross-provider fallback) --- +check "qwen w/ only OPENAI key" "SKIP:QWEN_API_KEY" "$(env -u GADFLY_API_KEY -u GADFLY_BASE_URL -u QWEN_API_KEY OPENAI_API_KEY=k bash -c "$(declare -f preflight); preflight qwen")" + +# --- GADFLY_API_KEY substitutes ONLY with GADFLY_BASE_URL (the override path) --- +check "qwen via GADFLY_API_KEY + BASE_URL" "RUN" \ + "$(env -u QWEN_API_KEY GADFLY_API_KEY=k GADFLY_BASE_URL=https://x bash -c "$(declare -f preflight); preflight qwen")" +# The false-pass this check exists to prevent: on the registry path (no +# GADFLY_BASE_URL) the qwen built-in reads QWEN_API_KEY and never consults +# GADFLY_API_KEY, so pre-flighting it as sufficient lets a doomed run proceed. +check "qwen w/ GADFLY_API_KEY but no BASE_URL" "SKIP:QWEN_API_KEY" \ + "$(env -u QWEN_API_KEY -u GADFLY_BASE_URL GADFLY_API_KEY=k bash -c "$(declare -f preflight); preflight qwen")" + +# --- openai-compatible is its own spelling and must be pre-flighted too --- +check "openai-compatible, no key" "SKIP:OPENAI_API_KEY" \ + "$(env -u OPENAI_API_KEY -u GADFLY_API_KEY -u GADFLY_BASE_URL bash -c "$(declare -f preflight); preflight openai-compatible")" +check "openai-compatible, keyed" "RUN" \ + "$(env -u GADFLY_API_KEY -u GADFLY_BASE_URL -u GADFLY_BASE_URL OPENAI_API_KEY=k bash -c "$(declare -f preflight); preflight openai-compatible")" + +# --- empty-string key counts as missing, not present --- +check "qwen, empty key" "SKIP:QWEN_API_KEY" "$(env -u GADFLY_API_KEY -u GADFLY_BASE_URL QWEN_API_KEY= bash -c "$(declare -f preflight); preflight qwen")" + +# --- unkeyed providers are never blocked, even with nothing set --- +for p in ollama llama-swap llamaswap foreman google gemini some-dsn-name; do + check "unkeyed $p" "RUN" "$(env -i bash -c "$(declare -f preflight); preflight $p")" +done + +exit $fail diff --git a/scripts/run.sh b/scripts/run.sh index c09caca..28ac987 100644 --- a/scripts/run.sh +++ b/scripts/run.sh @@ -181,17 +181,23 @@ case "$PROVIDER" in # not the table's one-name shape. KEY_ENV=""; KEY_HINT="" case "$GADFLY_PROVIDER_EFF" in - ollama-cloud) KEY_ENV="OLLAMA_API_KEY"; KEY_HINT="OLLAMA_CLOUD_API_KEY" ;; - qwen) KEY_ENV="QWEN_API_KEY"; KEY_HINT="QWEN_API_KEY" ;; - kimi) KEY_ENV="KIMI_API_KEY"; KEY_HINT="KIMI_API_KEY" ;; - openai) KEY_ENV="OPENAI_API_KEY"; KEY_HINT="OPENAI_API_KEY" ;; - anthropic) KEY_ENV="ANTHROPIC_API_KEY"; KEY_HINT="ANTHROPIC_API_KEY" ;; + ollama-cloud) KEY_ENV="OLLAMA_API_KEY"; KEY_HINT="OLLAMA_CLOUD_API_KEY" ;; + qwen) KEY_ENV="QWEN_API_KEY"; KEY_HINT="QWEN_API_KEY" ;; + kimi) KEY_ENV="KIMI_API_KEY"; KEY_HINT="KIMI_API_KEY" ;; + openai|openai-compatible) KEY_ENV="OPENAI_API_KEY"; KEY_HINT="OPENAI_API_KEY" ;; + anthropic) KEY_ENV="ANTHROPIC_API_KEY"; KEY_HINT="ANTHROPIC_API_KEY" ;; esac - # Indirect expansion (bash), so the table above stays a table. 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" ] && [ -z "${!KEY_ENV:-}" ] && [ -z "${GADFLY_API_KEY:-}" ]; then - REVIEW="⚠️ No API key configured for provider \`${GADFLY_PROVIDER_EFF}\` (set \`${KEY_HINT}\`, or \`GADFLY_API_KEY\`); this reviewer was skipped." + # GADFLY_API_KEY substitutes for the provider's own variable ONLY on the + # endpoint-override path: resolveModel reads it after the `baseURL == ""` + # early return, so with GADFLY_BASE_URL unset the built-in reads its own + # env var and GADFLY_API_KEY is never consulted. Treating it as a universal + # substitute made a mis-set GADFLY_API_KEY pass pre-flight and then 401 five + # times anyway — the exact failure this check exists to prevent. + KEY_OK=0 + [ -n "$KEY_ENV" ] && [ -n "${!KEY_ENV:-}" ] && KEY_OK=1 # indirect expansion (bash) + [ -n "${GADFLY_BASE_URL:-}" ] && [ -n "${GADFLY_API_KEY:-}" ] && KEY_OK=1 + if [ -n "$KEY_ENV" ] && [ "$KEY_OK" -eq 0 ]; then + REVIEW="⚠️ No API key configured for provider \`${GADFLY_PROVIDER_EFF}\` (set \`${KEY_HINT}\`, or \`GADFLY_API_KEY\` together with \`GADFLY_BASE_URL\`); this reviewer was skipped." else BIN="${GADFLY_BIN:-gadfly}" if ! command -v "$BIN" >/dev/null 2>&1 && [ ! -x "$BIN" ]; then From 67a73616e101fec64564761f77d1b164195919a9 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Wed, 12 Aug 2026 17:20:44 -0400 Subject: [PATCH 04/12] =?UTF-8?q?fix(qwen):=20gadfly=20round=203=20?= =?UTF-8?q?=E2=80=94=20stop=20guarding=20a=20duplicate,=20delete=20it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Twelve findings, all real, and the two that matter are about the pre-flight I added rather than about qwen. The credential check had a false pass in the OTHER direction from round 2's: on the GADFLY_BASE_URL override path, resolveModel builds the client with GADFLY_API_KEY and never reads QWEN_API_KEY/KIMI_API_KEY, so treating the provider's own key as sufficient there let a doomed run proceed. Having now been wrong about these rules in both directions, the check no longer tries to model both paths: it covers the REGISTRY path, whose rules it can state exactly, and says nothing about the override path — which is hand-configured by definition, while the registry path is the one you hit by adding a model id to a var and forgetting the secret. The logic moves to scripts/preflight.sh, sourced by both run.sh and the test. The previous answer to "this test duplicates production logic" was a regex drift-guard, and that guard compared only the provider table — not the decision logic, which is precisely the half that carried the bug. A duplicate you guard is still a duplicate; this deletes it, and the test now runs under `set -u` like production does. Also: the test that pins the shared provider slice held its own copy of the list (now ranges the slice); endpointProviderNames had nothing tying it to the switches it describes, which is how it shipped without "gemini" (a new test asserts every advertised name resolves); two godoc lists had drifted; and the "sanity" line that asserted nothing is gone. And the repo had NO test job — `go test` and the pre-flight table both existed and neither was ever executed by CI, which reads as coverage while providing none. Added one (build/vet/gofmt/test/pre-flight), running alongside the image build rather than gating it, so red is loud without standing between a push and a rebuild. Co-Authored-By: Claude Opus 5 (1M context) --- .gitea/workflows/build-image.yml | 31 +++++++ cmd/gadfly/model.go | 16 ++-- cmd/gadfly/model_test.go | 28 +++++- scripts/preflight.sh | 66 ++++++++++++++ scripts/preflight_test.sh | 145 +++++++++++++++---------------- scripts/run.sh | 45 ++-------- 6 files changed, 212 insertions(+), 119 deletions(-) create mode 100644 scripts/preflight.sh diff --git a/.gitea/workflows/build-image.yml b/.gitea/workflows/build-image.yml index d1c0695..c1bb7fb 100644 --- a/.gitea/workflows/build-image.yml +++ b/.gitea/workflows/build-image.yml @@ -45,6 +45,37 @@ 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 + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-go@v5 + with: + go-version-file: go.mod + - name: Configure private module access + env: + REGISTRY_USER: ${{ secrets.REGISTRY_USER }} + REGISTRY_PASSWORD: ${{ secrets.REGISTRY_PASSWORD }} + run: | + git config --global url."https://${REGISTRY_USER}:${REGISTRY_PASSWORD}@gitea.stevedudenhoeffer.com/".insteadOf "https://gitea.stevedudenhoeffer.com/" + go env -w GOPRIVATE=gitea.stevedudenhoeffer.com/* + - name: go build + run: go build ./... + - name: go vet + run: go vet ./... + - name: gofmt + run: test -z "$(gofmt -l .)" || { gofmt -l .; exit 1; } + - name: go test + 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/cmd/gadfly/model.go b/cmd/gadfly/model.go index 33f2def..b8e9b0e 100644 --- a/cmd/gadfly/model.go +++ b/cmd/gadfly/model.go @@ -62,10 +62,12 @@ const endpointProviderNames = "openai/openai-compatible/kimi/qwen/ollama/ollama- // 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. @@ -223,8 +225,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 endpointProviderNames; "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_ = "" diff --git a/cmd/gadfly/model_test.go b/cmd/gadfly/model_test.go index d8ba84a..8a81cc2 100644 --- a/cmd/gadfly/model_test.go +++ b/cmd/gadfly/model_test.go @@ -1,6 +1,9 @@ package main -import "testing" +import ( + "strings" + "testing" +) func TestEndpointProvider(t *testing.T) { t.Run("ollama http endpoint registers under its name", func(t *testing.T) { @@ -77,7 +80,9 @@ func TestEndpointProvider(t *testing.T) { // reason a user could guess — which is exactly what happened here on the first // pass. Asserting both in one table is what makes the pair fail together. func TestOpenAICompatProvidersResolveOnBothPaths(t *testing.T) { - for _, provider := range []string{"openai", "openai-compatible", "kimi", "qwen"} { + // Ranges the SHARED slice rather than a fourth copy of the names: 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 { @@ -99,6 +104,25 @@ func TestOpenAICompatProvidersResolveOnBothPaths(t *testing.T) { } } +// TestEndpointProviderNamesAreAllAccepted keeps the operator-facing list +// honest. endpointProviderNames exists to stop two error messages drifting +// apart, but nothing tied it to the switches it describes — and its first +// version had already dropped the "gemini" alias, so the anti-drift list was +// itself drifted. Every name it advertises must actually resolve. +func TestEndpointProviderNamesAreAllAccepted(t *testing.T) { + for _, name := range strings.Split(endpointProviderNames, "/") { + name = strings.TrimSpace(name) + if name == "" { + continue + } + t.Run(name, 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) + } + }) + } +} + func TestBuildSpec(t *testing.T) { tests := []struct { name string diff --git a/scripts/preflight.sh b/scripts/preflight.sh new file mode 100644 index 0000000..c9972d1 --- /dev/null +++ b/scripts/preflight.sh @@ -0,0 +1,66 @@ +#!/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 +# there is no second copy to drift. An earlier version of this change had the +# logic in run.sh and a duplicate in the test reconciled by a regex diff — that +# guard only covered the provider table and not the decision logic below, which +# is precisely the half that had the bug. +# +# 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 — i.e. GADFLY_BASE_URL unset. That is deliberate. +# With an explicit endpoint, resolveModel constructs the client directly and the +# credential is GADFLY_API_KEY, falling back to the client's own default +# (OPENAI_API_KEY for the openai family) — while the built-ins' own variables are +# never consulted. Checking one path's rules against the other produced a +# false-pass in BOTH directions across successive fixes here, so this checks the +# path whose rules it can state exactly and stays silent on the other. An +# override-path config is hand-written by definition; the registry path is the +# one somebody hits by adding a model id to a var and forgetting the secret. +gadfly_preflight_key() { + local provider="$1" key_env="" key_hint="" + + # Only the registry path has knowable credential rules — see above. + if [ -n "${GADFLY_BASE_URL:-}" ]; then + echo "" + 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. + case "$provider" in + ollama-cloud) key_env="OLLAMA_API_KEY"; key_hint="OLLAMA_CLOUD_API_KEY" ;; + qwen) key_env="QWEN_API_KEY"; key_hint="QWEN_API_KEY" ;; + kimi) key_env="KIMI_API_KEY"; key_hint="KIMI_API_KEY" ;; + openai|openai-compatible) key_env="OPENAI_API_KEY"; key_hint="OPENAI_API_KEY" ;; + anthropic) key_env="ANTHROPIC_API_KEY"; key_hint="ANTHROPIC_API_KEY" ;; + esac + + if [ -z "$key_env" ]; then + echo "" # provider needs no pre-flight + return 0 + fi + # 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" +} diff --git a/scripts/preflight_test.sh b/scripts/preflight_test.sh index 8bd64bd..b9fe9ee 100644 --- a/scripts/preflight_test.sh +++ b/scripts/preflight_test.sh @@ -1,93 +1,88 @@ #!/usr/bin/env bash -# Exercise the run.sh credential pre-flight in isolation: every provider x -# key-present/absent x GADFLY_API_KEY/GADFLY_BASE_URL combination. +# Table test for the credential pre-flight in preflight.sh. # -# The preflight() below is a COPY of run.sh's logic, which makes it exactly the -# kind of duplicated pair this whole PR keeps finding. The drift guard runs -# first: it compares the provider→variable arms in both files and aborts if -# they differ, so a table edited in run.sh and not here fails loudly instead of -# certifying stale logic. +# It SOURCES the real implementation rather than copying it. An earlier version +# duplicated the logic and reconciled the copies with a regex diff — which only +# covered the provider table and not the decision logic, i.e. exactly the half +# that had the bug. Sourcing removes the second copy entirely. +# +# Run: scripts/preflight_test.sh (exit 0 = all cases pass) set -u SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" -RUN_SH="${RUN_SH:-$SCRIPT_DIR/run.sh}" - -# Normalized case arms: "provider) KEY_ENV=... KEY_HINT=..." with runs of -# whitespace collapsed, so alignment changes don't trip the guard. -arms_of() { grep -oE '^[[:space:]]*[a-z|-]+\)[[:space:]]+KEY_ENV="[A-Z_]+";[[:space:]]+KEY_HINT="[A-Z_]+"' "$1" | tr -s ' \t' ' ' | sed 's/^ //'; } - -if [ -r "$RUN_SH" ]; then - if ! diff <(arms_of "$RUN_SH") <(arms_of "$0") >/dev/null; then - echo "FAIL drift guard: the pre-flight table here no longer matches $RUN_SH" - diff <(arms_of "$RUN_SH") <(arms_of "$0") | sed 's/^/ /' - exit 1 - fi - echo "ok drift guard: table matches run.sh" -else - echo "FAIL drift guard: cannot read $RUN_SH — cannot prove this tests the real table" - exit 1 -fi - -preflight() { # $1=provider ; env carries the keys - local GADFLY_PROVIDER_EFF="$1" KEY_ENV="" KEY_HINT="" KEY_OK=0 - case "$GADFLY_PROVIDER_EFF" in - ollama-cloud) KEY_ENV="OLLAMA_API_KEY"; KEY_HINT="OLLAMA_CLOUD_API_KEY" ;; - qwen) KEY_ENV="QWEN_API_KEY"; KEY_HINT="QWEN_API_KEY" ;; - kimi) KEY_ENV="KIMI_API_KEY"; KEY_HINT="KIMI_API_KEY" ;; - openai|openai-compatible) KEY_ENV="OPENAI_API_KEY"; KEY_HINT="OPENAI_API_KEY" ;; - anthropic) KEY_ENV="ANTHROPIC_API_KEY"; KEY_HINT="ANTHROPIC_API_KEY" ;; - esac - [ -n "$KEY_ENV" ] && [ -n "${!KEY_ENV:-}" ] && KEY_OK=1 - [ -n "${GADFLY_BASE_URL:-}" ] && [ -n "${GADFLY_API_KEY:-}" ] && KEY_OK=1 - if [ -n "$KEY_ENV" ] && [ "$KEY_OK" -eq 0 ]; then - echo "SKIP:$KEY_HINT" - else - echo "RUN" - fi -} +# shellcheck source=preflight.sh +. "$SCRIPT_DIR/preflight.sh" fail=0 -check() { # desc, want, got - if [ "$2" = "$3" ]; then echo "ok $1"; else echo "FAIL $1 — want $2, got $3"; fail=1; fi +check() { # description, want, got + if [ "$2" = "$3" ]; then + echo "ok $1" + else + echo "FAIL $1 — want '$2', got '$3'" + fail=1 + fi } -env -i bash -c 'true' >/dev/null 2>&1 # sanity +# 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"; shift + env -i PATH="$PATH" HOME="$HOME" "$@" bash -c " + set -u + . '$SCRIPT_DIR/preflight.sh' + gadfly_preflight_key '$provider' + " +} -# --- keyed providers with NO key -> skip, naming the right variable --- -check "qwen, no key" "SKIP:QWEN_API_KEY" "$(env -u QWEN_API_KEY -u GADFLY_API_KEY bash -c "$(declare -f preflight); preflight qwen")" -check "kimi, no key" "SKIP:KIMI_API_KEY" "$(env -u KIMI_API_KEY -u GADFLY_API_KEY bash -c "$(declare -f preflight); preflight kimi")" -check "ollama-cloud, none" "SKIP:OLLAMA_CLOUD_API_KEY" "$(env -u OLLAMA_API_KEY -u GADFLY_API_KEY bash -c "$(declare -f preflight); preflight ollama-cloud")" -check "openai, no key" "SKIP:OPENAI_API_KEY" "$(env -u OPENAI_API_KEY -u GADFLY_API_KEY bash -c "$(declare -f preflight); preflight openai")" -check "anthropic, no key" "SKIP:ANTHROPIC_API_KEY" "$(env -u ANTHROPIC_API_KEY -u GADFLY_API_KEY bash -c "$(declare -f preflight); preflight anthropic")" +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)" -# --- keyed providers WITH their key -> run --- -check "qwen, keyed" "RUN" "$(env -u GADFLY_API_KEY -u GADFLY_BASE_URL QWEN_API_KEY=k bash -c "$(declare -f preflight); preflight qwen")" -check "ollama-cloud, keyed" "RUN" "$(env -u GADFLY_API_KEY -u GADFLY_BASE_URL OLLAMA_API_KEY=k bash -c "$(declare -f preflight); preflight ollama-cloud")" +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)" -# --- the WRONG key must not satisfy a provider (no cross-provider fallback) --- -check "qwen w/ only OPENAI key" "SKIP:QWEN_API_KEY" "$(env -u GADFLY_API_KEY -u GADFLY_BASE_URL -u QWEN_API_KEY OPENAI_API_KEY=k bash -c "$(declare -f preflight); preflight qwen")" +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)" -# --- GADFLY_API_KEY substitutes ONLY with GADFLY_BASE_URL (the override path) --- -check "qwen via GADFLY_API_KEY + BASE_URL" "RUN" \ - "$(env -u QWEN_API_KEY GADFLY_API_KEY=k GADFLY_BASE_URL=https://x bash -c "$(declare -f preflight); preflight qwen")" -# The false-pass this check exists to prevent: on the registry path (no -# GADFLY_BASE_URL) the qwen built-in reads QWEN_API_KEY and never consults -# GADFLY_API_KEY, so pre-flighting it as sufficient lets a doomed run proceed. -check "qwen w/ GADFLY_API_KEY but no BASE_URL" "SKIP:QWEN_API_KEY" \ - "$(env -u QWEN_API_KEY -u GADFLY_BASE_URL GADFLY_API_KEY=k bash -c "$(declare -f preflight); preflight qwen")" +echo "== an empty-string key counts as missing, not present ==" +check "qwen, empty key" "QWEN_API_KEY" "$(probe qwen QWEN_API_KEY=)" -# --- openai-compatible is its own spelling and must be pre-flighted too --- -check "openai-compatible, no key" "SKIP:OPENAI_API_KEY" \ - "$(env -u OPENAI_API_KEY -u GADFLY_API_KEY -u GADFLY_BASE_URL bash -c "$(declare -f preflight); preflight openai-compatible")" -check "openai-compatible, keyed" "RUN" \ - "$(env -u GADFLY_API_KEY -u GADFLY_BASE_URL -u GADFLY_BASE_URL OPENAI_API_KEY=k bash -c "$(declare -f preflight); preflight openai-compatible")" +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)" -# --- empty-string key counts as missing, not present --- -check "qwen, empty key" "SKIP:QWEN_API_KEY" "$(env -u GADFLY_API_KEY -u GADFLY_BASE_URL QWEN_API_KEY= bash -c "$(declare -f preflight); preflight qwen")" +echo "== override path (GADFLY_BASE_URL set) is deliberately not pre-flighted ==" +# 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. +check "qwen + BASE_URL, no keys" "" "$(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 "openai + BASE_URL, no keys" "" "$(probe openai GADFLY_BASE_URL=https://x)" -# --- unkeyed providers are never blocked, even with nothing set --- -for p in ollama llama-swap llamaswap foreman google gemini some-dsn-name; do - check "unkeyed $p" "RUN" "$(env -i bash -c "$(declare -f preflight); preflight $p")" +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 -exit $fail +# 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)" + +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 28ac987..cbffd7d 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,42 +167,10 @@ case "$PROVIDER" in fi GADFLY_PROVIDER_EFF="$MODEL_PROVIDER" - # Pre-flight the credential for providers that need one. Why bother, when - # majordomo already fails closed with a 401: without this, a missing key - # surfaces as five identical per-lens agent failures that name no variable, - # and the operator has to read a stack trace to learn which secret they - # forgot to forward. GADFLY_API_KEY overrides any entry. - # - # 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 has more than one acceptable variable, so a - # single-variable check would skip a correctly-configured run. - # **google** is this case: it accepts GOOGLE_API_KEY *or* - # GEMINI_API_KEY. Adding `google) KEY_ENV="GOOGLE_API_KEY"` here would - # silently skip every reviewer configured with GEMINI_API_KEY. If you - # want google pre-flighted, the check has to accept either variable, - # not the table's one-name shape. - KEY_ENV=""; KEY_HINT="" - case "$GADFLY_PROVIDER_EFF" in - ollama-cloud) KEY_ENV="OLLAMA_API_KEY"; KEY_HINT="OLLAMA_CLOUD_API_KEY" ;; - qwen) KEY_ENV="QWEN_API_KEY"; KEY_HINT="QWEN_API_KEY" ;; - kimi) KEY_ENV="KIMI_API_KEY"; KEY_HINT="KIMI_API_KEY" ;; - openai|openai-compatible) KEY_ENV="OPENAI_API_KEY"; KEY_HINT="OPENAI_API_KEY" ;; - anthropic) KEY_ENV="ANTHROPIC_API_KEY"; KEY_HINT="ANTHROPIC_API_KEY" ;; - esac - # GADFLY_API_KEY substitutes for the provider's own variable ONLY on the - # endpoint-override path: resolveModel reads it after the `baseURL == ""` - # early return, so with GADFLY_BASE_URL unset the built-in reads its own - # env var and GADFLY_API_KEY is never consulted. Treating it as a universal - # substitute made a mis-set GADFLY_API_KEY pass pre-flight and then 401 five - # times anyway — the exact failure this check exists to prevent. - KEY_OK=0 - [ -n "$KEY_ENV" ] && [ -n "${!KEY_ENV:-}" ] && KEY_OK=1 # indirect expansion (bash) - [ -n "${GADFLY_BASE_URL:-}" ] && [ -n "${GADFLY_API_KEY:-}" ] && KEY_OK=1 - if [ -n "$KEY_ENV" ] && [ "$KEY_OK" -eq 0 ]; then - REVIEW="⚠️ No API key configured for provider \`${GADFLY_PROVIDER_EFF}\` (set \`${KEY_HINT}\`, or \`GADFLY_API_KEY\` together with \`GADFLY_BASE_URL\`); this reviewer was skipped." + # Credential pre-flight — one definition, shared with preflight_test.sh. + MISSING_KEY="$(gadfly_preflight_key "$GADFLY_PROVIDER_EFF")" + 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 From ebfaeba07e92f41db6237e073a1c91648a515ada Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Wed, 12 Aug 2026 17:22:08 -0400 Subject: [PATCH 05/12] docs(qwen): warn that Model Studio keys are endpoint-scoped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A workspace-scoped Qwen endpoint rejects a key issued for the shared international host with a genuine 'Incorrect API key provided', so a valid key reads as invalid and the obvious next move — checking the key — confirms it is fine and leads nowhere. Watched this cost real debugging time on a live deployment today; gadfly would hit it identically. Documents the GADFLY_ENDPOINT_* form, which reaches a workspace host with no code change. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/README.md b/README.md index 825513b..1592557 100644 --- a/README.md +++ b/README.md @@ -86,6 +86,21 @@ gets you a skip notice naming it, not a mis-keyed call. Note `kimi/` (Moo 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,..." +> ``` +> +> (Verified the hard way against a live deployment.) + > ### 🧪 Honest status > Only the **Ollama** paths above are actually exercised. The OpenAI / Qwen / Kimi / > Anthropic / Google From 14f8533e38680bd619c00ac5cf0ff8b6b4e82b17 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Wed, 12 Aug 2026 17:28:08 -0400 Subject: [PATCH 06/12] fix(ci): scrub the registry credential before running repo code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both Claude reviewers caught this independently, and they are right. The test job I added wrote a PUSH-CAPABLE REGISTRY_PASSWORD into a plaintext ~/.gitconfig and then ran `go build`/`go vet`/`go test` — repository code — on pull_request events. This repo is public, so a fork PR could ship a test whose only job is to print that file. The image build had already answered this question correctly: its credentials are BuildKit secrets scoped to the module-download RUN and are never present while code executes. I bolted on a job that skipped the boundary its neighbour maintains. Dependencies are now fetched in their own step which deletes ~/.gitconfig before anything else runs, and asserts the scrub — against the whole home directory, not against the file it just removed, because the credential can also land in ~/.netrc or ~/.config/go/env. Verified the assertion is not vacuous: planting the secret in ~/.netrc trips it. Later steps run with GOPROXY=off, so any attempt to reach the network fails loudly rather than quietly hunting for the credential that is now gone. Also from round 4: TestEndpointProviderNamesAreAllAccepted pinned only endpointProvider, while the constant is the error text for BOTH resolution paths — it now asserts each advertised name resolves either way (break-checked by dropping the gemini alias from resolveModel alone). preflight.sh documents that ollama-cloud is checked on OLLAMA_API_KEY but hinted as OLLAMA_CLOUD_API_KEY because run.sh copies one to the other first, an ordering dependency that was invisible from the file. And the comments that narrated this PR's own edit history ("the first version of this change...") are rewritten as invariants. That history stops being true the moment this merges, and the repo's doc policy says as much. Co-Authored-By: Claude Opus 5 (1M context) --- .gitea/workflows/build-image.yml | 28 +++++++++++++++++++++++-- cmd/gadfly/model.go | 35 +++++++++++++++++--------------- cmd/gadfly/model_test.go | 34 +++++++++++++++++++++---------- scripts/preflight.sh | 31 ++++++++++++++++------------ scripts/preflight_test.sh | 6 ++---- 5 files changed, 88 insertions(+), 46 deletions(-) diff --git a/.gitea/workflows/build-image.yml b/.gitea/workflows/build-image.yml index c1bb7fb..634e9f0 100644 --- a/.gitea/workflows/build-image.yml +++ b/.gitea/workflows/build-image.yml @@ -58,20 +58,44 @@ jobs: - uses: actions/setup-go@v5 with: go-version-file: go.mod - - name: Configure private module access + # 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: | - git config --global url."https://${REGISTRY_USER}:${REGISTRY_PASSWORD}@gitea.stevedudenhoeffer.com/".insteadOf "https://gitea.stevedudenhoeffer.com/" go env -w GOPRIVATE=gitea.stevedudenhoeffer.com/* + git config --global url."https://${REGISTRY_USER}:${REGISTRY_PASSWORD}@gitea.stevedudenhoeffer.com/".insteadOf "https://gitea.stevedudenhoeffer.com/" + go mod download + rm -f "$HOME/.gitconfig" + # Prove the scrub worked, and prove it against the whole home dir — + # checking only the file just deleted would pass no matter what, and + # the credential can also reach ~/.netrc or ~/.config/go/env. + test ! -e "$HOME/.gitconfig" + if grep -rqF "${REGISTRY_PASSWORD}" "$HOME" 2>/dev/null; then + echo "::error::registry credential still present under \$HOME after scrub" + exit 1 + 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 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 diff --git a/cmd/gadfly/model.go b/cmd/gadfly/model.go index b8e9b0e..0f8cce4 100644 --- a/cmd/gadfly/model.go +++ b/cmd/gadfly/model.go @@ -26,10 +26,11 @@ const defaultProvider = "ollama-cloud" // built-ins that ARE that client pointed elsewhere, so an explicit endpoint for // either belongs on the same branch. // -// This is a slice rather than three copies of a case list because there are -// three places that must agree — resolveModel's switch, endpointProvider's -// switch, and the test that pins them — and the first version of this change -// added the names to one switch and not the other. +// 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"} func isOpenAICompatProvider(name string) bool { @@ -38,13 +39,13 @@ func isOpenAICompatProvider(name string) bool { // endpointProviderNames is the operator-facing list of providers that accept an // explicit endpoint. resolveModel and endpointProvider accept the SAME set, so -// they share one message rather than each carrying a hand-maintained copy that -// drifts in order and spelling — which is exactly what happened when kimi/qwen -// were added to both switches. +// one message serves both rather than each carrying a copy that drifts in +// order and spelling. // -// Keep every accepted spelling here, including aliases: the first version of -// this constant dropped "gemini", so the list written to prevent drift had -// already drifted from the switches it describes. +// 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" @@ -98,10 +99,12 @@ func resolveModel() (llm.Model, error) { } // Endpoint override: construct the provider directly at the given URL. - // The openai-compat family (openai/openai-compatible/kimi/qwen) is matched - // by the shared predicate, not a repeated case list. The credential here is - // GADFLY_API_KEY; the built-ins' own KIMI_API_KEY / QWEN_API_KEY apply only - // on the registry path above, where GADFLY_BASE_URL is unset. + // 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) { opts := []openai.Option{openai.WithBaseURL(baseURL)} if apiKey != "" { @@ -279,8 +282,8 @@ 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 hand-copied case lists are how they drifted apart before. + // 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 != "" { diff --git a/cmd/gadfly/model_test.go b/cmd/gadfly/model_test.go index 8a81cc2..4cf4145 100644 --- a/cmd/gadfly/model_test.go +++ b/cmd/gadfly/model_test.go @@ -75,13 +75,13 @@ func TestEndpointProvider(t *testing.T) { // 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. Adding a name to one and not the other yields a -// provider that works when configured one way and errors the other, for no -// reason a user could guess — which is exactly what happened here on the first -// pass. Asserting both in one table is what makes the pair fail together. +// 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 rather than a fourth copy of the names: a test - // that pins a list against drift must not be able to drift from it. + // 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") @@ -105,21 +105,33 @@ func TestOpenAICompatProvidersResolveOnBothPaths(t *testing.T) { } // TestEndpointProviderNamesAreAllAccepted keeps the operator-facing list -// honest. endpointProviderNames exists to stop two error messages drifting -// apart, but nothing tied it to the switches it describes — and its first -// version had already dropped the "gemini" alias, so the anti-drift list was -// itself drifted. Every name it advertises must actually resolve. +// 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 } - t.Run(name, func(t *testing.T) { + // 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) + } + }) } } diff --git a/scripts/preflight.sh b/scripts/preflight.sh index c9972d1..392a4ab 100644 --- a/scripts/preflight.sh +++ b/scripts/preflight.sh @@ -2,10 +2,8 @@ # Credential pre-flight for the agentic reviewer, in ONE definition. # # Sourced by run.sh (production) and by preflight_test.sh (the table test), so -# there is no second copy to drift. An earlier version of this change had the -# logic in run.sh and a duplicate in the test reconciled by a regex diff — that -# guard only covered the provider table and not the decision logic below, which -# is precisely the half that had the bug. +# 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 @@ -15,15 +13,16 @@ # 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 — i.e. GADFLY_BASE_URL unset. That is deliberate. -# With an explicit endpoint, resolveModel constructs the client directly and the -# credential is GADFLY_API_KEY, falling back to the client's own default -# (OPENAI_API_KEY for the openai family) — while the built-ins' own variables are -# never consulted. Checking one path's rules against the other produced a -# false-pass in BOTH directions across successive fixes here, so this checks the -# path whose rules it can state exactly and stays silent on the other. An -# override-path config is hand-written by definition; the registry path is the -# one somebody hits by adding a model id to a var and forgetting the secret. +# 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" key_env="" key_hint="" @@ -43,6 +42,12 @@ gadfly_preflight_key() { # `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 + # 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"; key_hint="OLLAMA_CLOUD_API_KEY" ;; qwen) key_env="QWEN_API_KEY"; key_hint="QWEN_API_KEY" ;; diff --git a/scripts/preflight_test.sh b/scripts/preflight_test.sh index b9fe9ee..baad214 100644 --- a/scripts/preflight_test.sh +++ b/scripts/preflight_test.sh @@ -1,10 +1,8 @@ #!/usr/bin/env bash # Table test for the credential pre-flight in preflight.sh. # -# It SOURCES the real implementation rather than copying it. An earlier version -# duplicated the logic and reconciled the copies with a regex diff — which only -# covered the provider table and not the decision logic, i.e. exactly the half -# that had the bug. Sourcing removes the second copy entirely. +# 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 From 0abcd16e9ea841b2a1a5c741fa8728b48c0b567f Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Wed, 12 Aug 2026 17:40:03 -0400 Subject: [PATCH 07/12] fix(ci): make the credential scrub failure-safe, and stop the lists drifting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 5, and the best findings are again about the fix from round 4. The scrub only ran on success. `set -e` aborts the step when `go mod download` fails, so the cleanup line after it never executed — leaving a push-capable credential on a long-lived self-hosted runner for whatever job landed there next. It is now a `trap ... EXIT`, verified against a simulated failure. It also scrubbed the wrong file in principle: `git config --global` writes to GIT_CONFIG_GLOBAL, else $XDG_CONFIG_HOME/git/config when that exists, else ~/.gitconfig — so deleting ~/.gitconfig can scrub a path the credential was never in. The step now names GIT_CONFIG_GLOBAL itself, leaving exactly one file to remove. And the verification failed open in the case that matters most: `grep -F ""` matches every file, so a run WITHOUT the secret — a fork PR, the threat model — failed the check with a message accusing it of leaking a credential it never had. Guarded on a non-empty secret. Credentials move to an Authorization header instead of being embedded in the URL, so a password containing @ : / or # can no longer break URL parsing in a way that reads as a bad password. Two list-drift holes closed with one test that reads across languages: TestOpenAICompatProvidersAreFullyWired asserts every openAICompatProviders entry is both advertised in endpointProviderNames and has a credential arm in scripts/preflight.sh. Adding a compat provider touches three places in two languages and nothing connected them. Break-checked in both directions. Finally, a whitespace-only GADFLY_BASE_URL disagreed across the boundary: Go TrimSpaces it and takes the registry path, bash called it "set" and skipped the pre-flight, so the missing key arrived as a bare 401 with no notice. Both now agree on what unset means. Co-Authored-By: Claude Opus 5 (1M context) --- .gitea/workflows/build-image.yml | 38 ++++++++++++++++++++++++++------ cmd/gadfly/model_test.go | 37 +++++++++++++++++++++++++++++++ scripts/preflight.sh | 8 ++++++- scripts/preflight_test.sh | 5 +++++ 4 files changed, 80 insertions(+), 8 deletions(-) mode change 100644 => 100755 scripts/preflight.sh mode change 100644 => 100755 scripts/preflight_test.sh diff --git a/.gitea/workflows/build-image.yml b/.gitea/workflows/build-image.yml index 634e9f0..fb8ddd9 100644 --- a/.gitea/workflows/build-image.yml +++ b/.gitea/workflows/build-image.yml @@ -70,15 +70,39 @@ jobs: 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/* - git config --global url."https://${REGISTRY_USER}:${REGISTRY_PASSWORD}@gitea.stevedudenhoeffer.com/".insteadOf "https://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 "$HOME/.gitconfig" - # Prove the scrub worked, and prove it against the whole home dir — - # checking only the file just deleted would pass no matter what, and - # the credential can also reach ~/.netrc or ~/.config/go/env. - test ! -e "$HOME/.gitconfig" - if grep -rqF "${REGISTRY_PASSWORD}" "$HOME" 2>/dev/null; then + + 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. + if [ -n "${REGISTRY_PASSWORD:-}" ] && grep -rqF "$REGISTRY_PASSWORD" "$HOME" 2>/dev/null; then echo "::error::registry credential still present under \$HOME after scrub" exit 1 fi diff --git a/cmd/gadfly/model_test.go b/cmd/gadfly/model_test.go index 4cf4145..20d42d1 100644 --- a/cmd/gadfly/model_test.go +++ b/cmd/gadfly/model_test.go @@ -1,6 +1,9 @@ package main import ( + "os" + "path/filepath" + "regexp" "strings" "testing" ) @@ -156,3 +159,37 @@ 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. +func TestOpenAICompatProvidersAreFullyWired(t *testing.T) { + advertised := make(map[string]bool) + for _, n := range strings.Split(endpointProviderNames, "/") { + advertised[strings.TrimSpace(n)] = true + } + + preflight, err := os.ReadFile(filepath.Join("..", "..", "scripts", "preflight.sh")) + if err != nil { + t.Fatalf("read preflight.sh: %v", err) + } + + 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) + } + // The arm may be shared ("openai|openai-compatible)"), so match the + // bare name as a case alternative rather than a whole line. + if !regexp.MustCompile(`(?m)^\s*(\w[\w-]*\|)*` + regexp.QuoteMeta(p) + `(\|[\w-]+)*\)`).Match(preflight) { + 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) + } + } +} diff --git a/scripts/preflight.sh b/scripts/preflight.sh old mode 100644 new mode 100755 index 392a4ab..f8586ec --- a/scripts/preflight.sh +++ b/scripts/preflight.sh @@ -27,7 +27,13 @@ gadfly_preflight_key() { local provider="$1" key_env="" key_hint="" # Only the registry path has knowable credential rules — see above. - if [ -n "${GADFLY_BASE_URL:-}" ]; then + # 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 echo "" return 0 fi diff --git a/scripts/preflight_test.sh b/scripts/preflight_test.sh old mode 100644 new mode 100755 index baad214..a24fdea --- a/scripts/preflight_test.sh +++ b/scripts/preflight_test.sh @@ -79,6 +79,11 @@ done # 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=" ")" + if [ "$fail" -ne 0 ]; then echo "RESULT: preflight table FAILED" exit 1 From a3d3a45e7e4d0f0d9333481ebc0c870beae03063 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Wed, 12 Aug 2026 17:51:55 -0400 Subject: [PATCH 08/12] fix(qwen): bump majordomo, and stop handing keys to the wrong vendor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 6, and one finding exposed something no reviewer mentioned: the majordomo bump this whole PR depends on was never made. Every test here builds the openai client directly, so all of them passed against a majordomo release that had never heard of qwen — a plain "qwen/" in GADFLY_MODELS, the primary way anyone will use this, would not have resolved at all. A compile error caught it, which is luck. TestBuiltinCompatProvidersResolveViaRegistry now exercises that path; the build is what guards the dep itself, since the old release cannot compile the code below. On the endpoint-override path, kimi and qwen fell through to openai.New's OPENAI_API_KEY default whenever GADFLY_API_KEY was unset — sending an OpenAI key to Moonshot or Alibaba. That is a credential handed to the wrong vendor, and it is the exact failure majordomo's built-ins are written to prevent; I reintroduced it one layer up. Both now pass the key unconditionally, so an absent key is a 401 naming GADFLY_API_KEY rather than a foreign credential on the wire. The test job scrubbed the registry credential and left the checkout token in .git/config, readable by the `go test` it then runs — fixing one credential while its neighbour sat in the open. persist-credentials: false; nothing in that job talks to git after checkout. The cross-language wiring test now QUERIES preflight.sh via a new gadfly_preflight_providers function instead of regexing its case statement. Parsing made that file's formatting a contract no linter enforces, where a harmless reformat breaks a test in another language. Two models flagged it. Also: grep for the scrub check takes -e, so a password starting with a hyphen is not read as options; and key_hint stopped repeating key_env in four of five arms. Co-Authored-By: Claude Opus 5 (1M context) --- .gitea/workflows/build-image.yml | 6 ++++ cmd/gadfly/model.go | 16 ++++++++++- cmd/gadfly/model_test.go | 47 +++++++++++++++++++++++++++----- go.mod | 2 +- go.sum | 4 +-- scripts/preflight.sh | 29 ++++++++++++++++---- 6 files changed, 88 insertions(+), 16 deletions(-) diff --git a/.gitea/workflows/build-image.yml b/.gitea/workflows/build-image.yml index fb8ddd9..d88e8e5 100644 --- a/.gitea/workflows/build-image.yml +++ b/.gitea/workflows/build-image.yml @@ -55,6 +55,12 @@ jobs: timeout-minutes: 15 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 diff --git a/cmd/gadfly/model.go b/cmd/gadfly/model.go index 0f8cce4..1c5b14a 100644 --- a/cmd/gadfly/model.go +++ b/cmd/gadfly/model.go @@ -107,8 +107,22 @@ func resolveModel() (llm.Model, error) { // check and then 401s. if isOpenAICompatProvider(provider) { opts := []openai.Option{openai.WithBaseURL(baseURL)} - if apiKey != "" { + 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) } diff --git a/cmd/gadfly/model_test.go b/cmd/gadfly/model_test.go index 20d42d1..f38e1f1 100644 --- a/cmd/gadfly/model_test.go +++ b/cmd/gadfly/model_test.go @@ -1,9 +1,8 @@ package main import ( - "os" + "os/exec" "path/filepath" - "regexp" "strings" "testing" ) @@ -169,15 +168,29 @@ func TestBuildSpec(t *testing.T) { // - 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 } - preflight, err := os.ReadFile(filepath.Join("..", "..", "scripts", "preflight.sh")) + script := filepath.Join("..", "..", "scripts", "preflight.sh") + out, err := exec.Command("bash", "-c", ". "+script+"; gadfly_preflight_providers").Output() if err != nil { - t.Fatalf("read preflight.sh: %v", err) + 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 { @@ -185,11 +198,31 @@ func TestOpenAICompatProvidersAreFullyWired(t *testing.T) { t.Errorf("openAICompatProviders has %q but endpointProviderNames does not list it — "+ "the error message operators read would omit a name that works", p) } - // The arm may be shared ("openai|openai-compatible)"), so match the - // bare name as a case alternative rather than a whole line. - if !regexp.MustCompile(`(?m)^\s*(\w[\w-]*\|)*` + regexp.QuoteMeta(p) + `(\|[\w-]+)*\)`).Match(preflight) { + 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) + } + }) + } +} 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 index f8586ec..26fdb77 100755 --- a/scripts/preflight.sh +++ b/scripts/preflight.sh @@ -55,12 +55,16 @@ gadfly_preflight_key() { # 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"; key_hint="OLLAMA_CLOUD_API_KEY" ;; - qwen) key_env="QWEN_API_KEY"; key_hint="QWEN_API_KEY" ;; - kimi) key_env="KIMI_API_KEY"; key_hint="KIMI_API_KEY" ;; - openai|openai-compatible) key_env="OPENAI_API_KEY"; key_hint="OPENAI_API_KEY" ;; - anthropic) key_env="ANTHROPIC_API_KEY"; key_hint="ANTHROPIC_API_KEY" ;; + 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 echo "" # provider needs no pre-flight @@ -75,3 +79,18 @@ gadfly_preflight_key() { fi echo "$key_hint" } + +# gadfly_preflight_providers echoes every provider this file has a credential +# arm for, one per line. +# +# 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. +# +# Keep in step with the case arms above — the Go test fails if a provider in +# either list is missing from the other. +gadfly_preflight_providers() { + printf '%s\n' ollama-cloud qwen kimi openai openai-compatible anthropic +} From 274451e89c48e01621f3f0724018fbc9cbadcdf3 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Wed, 12 Aug 2026 17:52:57 -0400 Subject: [PATCH 09/12] fix(ci): actually add grep -e (a3d3a45 claimed it and did not) The previous commit message listed this fix; the edit never made it into the tree. Checked because I have done exactly that before, and the message is not the change. Verified rather than assumed, and the behaviour is worth recording: with a password beginning with "-", `grep -rqF "$PW"` returns 0 against a directory containing no such string, so the scrub check would have reported a leak that did not exist and failed the step with a misleading message. With -e it correctly returns 1 when clean and 0 when the credential really is present. Co-Authored-By: Claude Opus 5 (1M context) --- .gitea/workflows/build-image.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.gitea/workflows/build-image.yml b/.gitea/workflows/build-image.yml index d88e8e5..7d7d9fa 100644 --- a/.gitea/workflows/build-image.yml +++ b/.gitea/workflows/build-image.yml @@ -108,7 +108,10 @@ 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. - if [ -n "${REGISTRY_PASSWORD:-}" ] && grep -rqF "$REGISTRY_PASSWORD" "$HOME" 2>/dev/null; then + # -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 fi From 3af0f09387253aab2819106543df941a15e950e2 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Wed, 12 Aug 2026 18:09:18 -0400 Subject: [PATCH 10/12] =?UTF-8?q?fix(qwen):=20one=20credential=20rule=20fo?= =?UTF-8?q?r=20both=20paths=20=E2=80=94=20seven=20findings=20said=20so?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourteen findings, and seven of them from all four models are the same one: endpointProvider was missing the no-cross-vendor-fallback guard I had just added to resolveModel. I fixed a credential leak on one path and left its sibling leaking, in the commit whose own message argued those two paths must move together. That is the third time in this PR. So it is no longer a rule written twice. openAICompatOptions owns it and both paths call it; builtinCompatProviders names the vendors that must never inherit OPENAI_API_KEY, replacing a `provider == "kimi" || provider == "qwen"` literal that was a fourth uncounted copy of the list. The test drives a real request at a local server and demands two things: that no request arrives carrying the OpenAI key, AND that the call fails closed naming the variable to set — the second half because my first draft pointed the provider at vendor.example, so the server saw nothing and the assertion held for a reason unrelated to the fix. Break-checked: removing the guard puts "Bearer sk-openai-must-not-travel" on the wire to the other vendor. The scrub check failed open. As a bare condition, a grep ERROR (exit >= 2) reads as "not found" and skips the guard — a credential check that passes precisely when it cannot see the filesystem it is searching. It now distinguishes 0/1/>=2 and refuses to continue on error. A bare "claude-code" spec has no "/", so the provider fell back to ollama-cloud and the pre-flight would skip a reviewer that authenticates with CLAUDE_CODE_OAUTH_TOKEN and needs no Ollama key. Engine specs are now exempt. preflight.sh's provider list duplicated its own case arms; both now read one table. And its comment claimed the Go cross-check fails if either list misses an entry from the other, when only one direction is checked — the reverse is not even desirable, since ollama-cloud and anthropic belong in that table and not in the Go one. The comment now says what is enforced. Co-Authored-By: Claude Opus 5 (1M context) --- .gitea/workflows/build-image.yml | 21 +++++--- cmd/gadfly/model.go | 56 ++++++++++++-------- cmd/gadfly/model_test.go | 91 ++++++++++++++++++++++++++++++++ scripts/preflight.sh | 75 +++++++++++++++++--------- scripts/preflight_test.sh | 14 ++++- scripts/run.sh | 4 +- 6 files changed, 205 insertions(+), 56 deletions(-) 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 From e67f95d7776344174ec66509a97a47c85192994e Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Wed, 12 Aug 2026 18:23:33 -0400 Subject: [PATCH 11/12] fix(qwen): un-exempt opencode, and keep the Qwen key in a secret MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings this round contradicted each other — one asked me to extend the engine-spec exemption to a bare "opencode", the other said opencode should not be exempt at all. The code settles it: that engine drives an ollama-cloud model through the bundled CLI and authenticates with OLLAMA_API_KEY, so it needs exactly the key the pre-flight checks. Exempting it, which I did last round, switched the check off for the one engine it could still help. Only claude-code is exempt now — it carries CLAUDE_CODE_OAUTH_TOKEN and needs no Ollama key — and opencode/open-code get table rows so both spellings are covered. The README told operators to embed the Qwen key in a GADFLY_ENDPOINT_* var, while the workflow that forwards those vars warns in its own comments that vars are NOT masked. Rather than only rewording the docs, a keyless kimi/qwen endpoint now falls back to its own QWEN_API_KEY / KIMI_API_KEY — the same vendor's key, so the no-cross-vendor rule is untouched — which lets the URL live in a var and the credential in a secret. Break-checked by pointing that fallback at OPENAI_API_KEY: the leak test catches it. Smaller: isBuiltinCompatProvider mirrors isOpenAICompatProvider instead of an inline slices.Contains, with a test that every builtin is also in the compat list (a builtin missing from it would never reach the branch that protects it); the preflight.sh rationale is stated once rather than in two comment blocks; the Go test locates the shell script relative to its own source file; and the gofmt step takes GOPROXY=off like its neighbours. Co-Authored-By: Claude Opus 5 (1M context) --- .gitea/workflows/build-image.yml | 1 + README.md | 8 ++++- cmd/gadfly/model.go | 25 +++++++++++++- cmd/gadfly/model_test.go | 58 +++++++++++++++++++++++++++++++- scripts/preflight.sh | 28 +++++++-------- scripts/preflight_test.sh | 7 +++- 6 files changed, 108 insertions(+), 19 deletions(-) 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)" From d8b023efd673835f4f9f45f3290df0e37c8f1430 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Wed, 12 Aug 2026 18:36:12 -0400 Subject: [PATCH 12/12] fix(qwen): the own-key fallback made the override path checkable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .gitea/workflows/build-image.yml | 4 ++++ cmd/gadfly/model.go | 11 +++++++---- cmd/gadfly/model_test.go | 20 ++++++++++++++------ scripts/preflight.sh | 30 ++++++++++++++++++++++-------- scripts/preflight_test.sh | 17 +++++++++++++---- 5 files changed, 60 insertions(+), 22 deletions(-) diff --git a/.gitea/workflows/build-image.yml b/.gitea/workflows/build-image.yml index ff62f3a..5ffc8a4 100644 --- a/.gitea/workflows/build-image.yml +++ b/.gitea/workflows/build-image.yml @@ -53,6 +53,10 @@ jobs: 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: diff --git a/cmd/gadfly/model.go b/cmd/gadfly/model.go index 8dbd281..389623b 100644 --- a/cmd/gadfly/model.go +++ b/cmd/gadfly/model.go @@ -74,12 +74,15 @@ func openAICompatOptions(provider, baseURL, key, keyHint string) []openai.Option // 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 == "" { - if own := os.Getenv(builtinCompatKeyEnv(provider)); own != "" { - key, keyHint = own, builtinCompatKeyEnv(provider) - } + key = os.Getenv(builtinCompatKeyEnv(provider)) } - opts = append(opts, openai.WithAPIKey(key), openai.WithAPIKeyName(keyHint)) + 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 diff --git a/cmd/gadfly/model_test.go b/cmd/gadfly/model_test.go index 43db0ef..8c57826 100644 --- a/cmd/gadfly/model_test.go +++ b/cmd/gadfly/model_test.go @@ -269,7 +269,7 @@ func TestBuiltinCompatProvidersNeverInheritOpenAIKey(t *testing.T) { if err != nil { t.Fatalf("resolveModel: %v", err) } - assertFailsClosed(t, m, seen, foreign, "GADFLY_API_KEY") + assertFailsClosed(t, m, seen, foreign, "QWEN_API_KEY", "KIMI_API_KEY") }) t.Run(provider+" via GADFLY_ENDPOINT_*", func(t *testing.T) { @@ -283,7 +283,7 @@ func TestBuiltinCompatProvidersNeverInheritOpenAIKey(t *testing.T) { if err != nil { t.Fatalf("Model: %v", err) } - assertFailsClosed(t, m, seen, foreign, "GADFLY_ENDPOINT_EP") + assertFailsClosed(t, m, seen, foreign, "QWEN_API_KEY", "KIMI_API_KEY") }) } } @@ -303,7 +303,7 @@ func leakServer(t *testing.T) (*httptest.Server, *[]string) { return srv, &seen } -func assertFailsClosed(t *testing.T, m llm.Model, seen *[]string, foreign, wantHint string) { +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")}}) @@ -313,15 +313,23 @@ func assertFailsClosed(t *testing.T, m llm.Model, seen *[]string, foreign, wantH } } if len(*seen) > 0 { - t.Errorf("a keyless %s provider reached the network (%d request(s)) instead of failing closed", wantHint, len(*seen)) + 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") } - if !strings.Contains(err.Error(), wantHint) { - t.Errorf("error = %v, want it to name %s so the operator knows what to set", err, wantHint) + // 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) } } diff --git a/scripts/preflight.sh b/scripts/preflight.sh index 08f2312..e323a23 100755 --- a/scripts/preflight.sh +++ b/scripts/preflight.sh @@ -36,28 +36,42 @@ gadfly_preflight_key() { # 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 - # Only the registry path has knowable credential rules — see above. # 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 - echo "" + # 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 - # 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 - # 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. local row row="$(_gadfly_preflight_table | awk -F: -v p="$provider" '$1 == p {print; exit}')" if [ -z "$row" ]; then diff --git a/scripts/preflight_test.sh b/scripts/preflight_test.sh index c3093bc..64e6ef6 100755 --- a/scripts/preflight_test.sh +++ b/scripts/preflight_test.sh @@ -60,15 +60,22 @@ echo "== GADFLY_API_KEY does NOT substitute on the registry path ==" # 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 (GADFLY_BASE_URL set) is deliberately not pre-flighted ==" +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. -check "qwen + BASE_URL, no keys" "" "$(probe qwen GADFLY_BASE_URL=https://x)" -check "qwen + BASE_URL + own key" "" "$(probe qwen GADFLY_BASE_URL=https://x QWEN_API_KEY=k)" +# 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 "openai + BASE_URL, no keys" "" "$(probe openai GADFLY_BASE_URL=https://x)" +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 @@ -89,6 +96,8 @@ echo "== engine specs carry their own auth and are never pre-flighted ==" # 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.