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