d8b023efd673835f4f9f45f3290df0e37c8f1430
7
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  steveandClaude Opus 5
|
d8b023efd6 |
fix(qwen): the own-key fallback made the override path checkable
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) <[email protected]>
|
||
|
steveandClaude Opus 5
|
e67f95d777 |
fix(qwen): un-exempt opencode, and keep the Qwen key in a secret
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) <[email protected]> |
||
|
steveandClaude Opus 5
|
3af0f09387 |
fix(qwen): one credential rule for both paths — seven findings said so
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) <[email protected]> |
||
|
steveandClaude Opus 5
|
0abcd16e9e |
fix(ci): make the credential scrub failure-safe, and stop the lists drifting
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) <[email protected]> |
||
|
steveandClaude Opus 5
|
14f8533e38 |
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]>
|
||
|
steveandClaude Opus 5
|
67a73616e1 |
fix(qwen): gadfly round 3 — stop guarding a duplicate, delete it
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) <[email protected]> |
||
|
steveandClaude Opus 5
|
1d6eaa08c5 |
fix(qwen): gadfly round 2 — the anti-drift list had already drifted
Build & push image / build-and-push (pull_request) Successful in 3s
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) <[email protected]> |