fix(ci): scrub the registry credential before running repo code

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) <[email protected]>

.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

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 != "" {

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)
+			}
+		})
 	}
 }
 

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 <provider> -> 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" ;;

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