feat(reusable): runtime-variable swarm config (cache-immune, no more re-pinning to retune) #14
Reference in New Issue
Block a user
Delete Branch "feat/vars-config"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Moves the volatile swarm config out of the cached reusable file and into runtime-injected owner variables, so retuning the fleet no longer requires re-pinning consumers (the long-lived-runner ref-cache defeats moving
@v1).How
Config resolves at runtime: consumer
with:input → owner user/org var → image default. Variables are injected per-run (like secrets), so they bypass the workflow-file cache.GADFLY_MODELSinputs.models || vars.GADFLY_DEFAULT_MODELSGADFLY_SPECIALISTSinputs.specialists || vars.GADFLY_DEFAULT_SPECIALISTSGADFLY_PROVIDER_CONCURRENCYinputs.provider_concurrency || vars.GADFLY_DEFAULT_PROVIDER_CONCURRENCYGADFLY_PROVIDER_LENS_CONCURRENCYinputs.provider_lens_concurrency || vars.GADFLY_DEFAULT_PROVIDER_LENS_CONCURRENCYGADFLY_ENDPOINT_RAGNAROSvars.GADFLY_ENDPOINT_RAGNAROS(the 4090 Ti)Input defaults dropped to
"", so the public file default is now generic (image default) — which also pulls the steve-specific swarm + the personal LAN endpoint out of the publicly-inherited default (addresses the earlier egress finding).Rollout
This is a structural change to the reusable, so it needs one cache-bust: consumers pin to this PR's merge
@<sha>. After that, all future config tuning is just editing the owner variables — no re-pin, no tag move.🤖 Generated with Claude Code
🪰 Gadfly — live review status
7/7 reviewers finished · updated 2026-06-28 06:11:32Z
claude-code/opus· claude-code — ✅ doneclaude-code/opus:max· claude-code — ✅ doneclaude-code/sonnet· claude-code — ✅ donedeepseek-v4-pro:cloud· ollama-cloud — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ doneminimax-m3:cloud· ollama-cloud — ✅ doneragnaros/qwen3.6-27b· ragnaros — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly review —
glm-5.2:cloud(ollama-cloud)Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Minor issues
Both findings verified against the actual source. Confirming:
review-reusable.yml:126documents"<provider>|<base-url>[|<key>]"and:130wiresGADFLY_ENDPOINT_RAGNAROS: ${{ vars.GADFLY_ENDPOINT_RAGNAROS }}, while:123-124wire M1/M5 throughsecrets.*, andexamples/endpoint-aliases.yml:16annotates the token field(use a secret). Confirmed.cmd/gadfly/model.go:207-220showsendpointProviderrejects empty/<2-part values (returns error, does not register), so an unsetvars.GADFLY_ENDPOINT_RAGNAROSyields no registeredragnarosprovider → no default egress. Confirmed.Verdict: Minor issues found
GADFLY_ENDPOINT_RAGNAROSis wired viavars.*(notsecrets.*), yet its documented format includes an optional credential field — a credential-in-a-variable footgun..gitea/workflows/review-reusable.yml:126documents the format as"<provider>|<base-url>[|<key>]", and line 130 binds it from${{ vars.GADFLY_ENDPOINT_RAGNAROS }}. Gitea maskssecrets.*in logs but does not maskvars.*. So if an owner ever populates the|<key>field (e.g. a bearer token for the llama-swap proxy), that credential would be stored in and potentially leaked through a plain variable — logs, UI, org-member visibility — rather than a secret. This directly contradicts the repo's own guidance, which wiresGADFLY_ENDPOINT_M1/M5throughsecrets.*(lines 123–124) and annotatesexamples/endpoint-aliases.yml:16as(use a secret)for the token field. Verified by readingreview-reusable.yml:120-130,examples/endpoint-aliases.yml:16,README.md:150-152, andcmd/gadfly/model.go:207-234(which confirms the third|<key>field is consumed as a real token/API key). Suggested fix: either (a) wireGADFLY_ENDPOINT_RAGNAROSfrom a secret (declare it inworkflow_call.secrets) the way M1/M5 are, or (b) at minimum drop the[|<key>]mention from the var's documented format and state explicitly that any credential for this endpoint must go in a secret, keeping the var URL-only.Removal of the hardcoded personal LAN endpoint is a genuine security improvement. The previous
GADFLY_ENDPOINT_RAGNAROS: "llamaswap|https://llama-swap.ragnaros.dudenhoeffer.casa"baked a personal host into the reusable file, so any consumer (including public ones) inheriting the default swarm would egress reviewer-PR contents (the diff/code) to that host. Replacing it with${{ vars.GADFLY_ENDPOINT_RAGNAROS }}(empty when unset) closes that default-egress path. Verified against the diff andcmd/gadfly/model.go:207-220, which confirms an empty/unset endpoint value is rejected (returns an error, not silently registered), so the no-egress-by-default behavior holds.🎯 Correctness — Blocking issues found
VERDICT: Blocking issues found
vars.GADFLY_DEFAULT_*in a reusable workflow resolves against the CALLER's repo/owner scope, not the reusable file's owner — defeating the PR's entire premise for cross-owner consumers.review-reusable.yml:135-140does${{ inputs.models || vars.GADFLY_DEFAULT_MODELS }}etc., and the inputs now default to""(review-reusable.yml:52-53,56-57). In GitHub Actions (which Gitea Actions replicates), a called reusable workflow'svars/secretscontexts reference the calling repository's variables, not the repository that contains the called workflow file — this is exactly whysecrets: inheritexists and why reusables must declare/forward secrets; variables are not forwarded at all. So when an external consumer (e.g.acme/widgets) callssteve/gadfly/.gitea/workflows/review-reusable.yml@…(the absolute path shown atexamples/reusable.yml:54),vars.GADFLY_DEFAULT_MODELSresolves to acme's repo/org/user variables, not steve's. Steve settingGADFLY_DEFAULT_MODELSon his own account does nothing for acme's run: the expression is empty →GADFLY_MODELSis empty →entrypoint.sh:156falls back toDEFAULT_MODELS="qwen3-coder:480b-cloud"(verified atentrypoint.sh:52, a single model). "Retune the whole fleet by editing ONE variable" only works for consumers owned by steve himself. Verified the call sites: the dogfooding caller.gitea/workflows/adversarial-review.yml:43uses./.gitea/workflows/review-reusable.yml(relative, same repo/owner steve), so it reads steve's vars and works — masking the bug during self-review. The actual third-party consumer path (examples/reusable.yml:54, absolutesteve/gadfly/...ref) silently degrades to the generic image default instead of steve's tuned swarm. Suggested fix: this cannot be fixed purely withvars.*in the reusable; either (a) forward the owner values as explicitinputsfrom a thin caller the owner maintains, (b) document a caller-forwarded secret/var contract, or (c) acknowledge that fleet-tuning only applies to same-owner consumers and update the README/PR claims accordingly. (Caveat: caller-vs-callee scope resolution is the documented GitHub Actions model Gitea reproduces; I could not execute Gitea directly to confirm, but the diff's own rationale ("variables are injected per-run by Gitea") relies on this same model, and the asymmetry between the relative-path self-caller and the absolute-path external caller is verified in the repo.)Stale consumer-facing docs now misstate the default swarm and pinning guidance.
examples/reusable.yml:8-12still says "the reusable ships a DEFAULT swarm: 3 cloud models + the Claude Code engine (sonnet/opus/opus:max), 5-lens suite," andexamples/reusable.yml:64-67overridesmodels:to a cloud-only set with the comment "delete this line (and forward CLAUDE_CODE_OAUTH_TOKEN above) to inherit the full default swarm (3 cloud + Claude Code sonnet/opus/opus:max, 5 lenses)." After this PR the reusable ships no model/concurrency defaults (inputs default""), so for an external consumer there is no 3-cloud+Claude swarm to inherit — deleting the override yields the single-model image default. Likewise.gitea/workflows/adversarial-review.yml:6-8still asserts "It inherits the default swarm: 3 cloud models + Claude Code (sonnet, opus, opus:max), 5-lens suite," which is no longer baked into the reusable's inputs. Suggested fix: update these comments to reflect the runtime-var resolution and that an unsetmodelsyields the image default unless the owner setsGADFLY_DEFAULT_MODELS.(Docs consistency, not behavioral) Pinning guidance contradicts the new README.
review-reusable.yml:29-31andexamples/reusable.yml:17-20,52-53still tell consumers to pin@v1("a curated release tag ... central tuning here propagates"), but the README now (README.md:316-319) states that long-lived act_runners cache the workflow file by ref so a moved@v1tag is often not re-fetched, and recommends@<sha>. The@v1guidance in the in-repo comments thus contradicts the cache rationale the PR introduces; update for consistency.🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
The core structural change (reusable inputs → runtime env resolution via
inputs.X || vars.GADFLY_DEFAULT_*, CLAUDE.md architecture map, README narrative + new "Central config via variables" section) is clean and consistent across the three edited files. Verified by reading the touched regions. But the PR left consumer-facingexamples/and the header/closing bookends of the reusable file itself stale and self-contradictory — a maintainability hazard for the next editor who copy-pastes from them:examples/reusable.yml:8-12, 17-20, 52-54, 64-67— Still describes the old model: "The reusable ships a DEFAULT swarm: 3 cloud models + the Claude Code engine (sonnet/opus/opus:max), 5-lens suite … delete that override (and forward the Claude token) to inherit the full default" (lines 8-12, 64-67), and recommends@v1"for auto-updating stability" (lines 17-20, 52-54). Neither statement is true after this PR: the reusable's inputs now ship all""defaults (no default swarm), the fleet comes from ownerGADFLY_DEFAULT_*variables, and the README this PR just edited explicitly says@v1is not re-fetched by long-lived runners and consumers "should pin … to an immutable@<sha>." Verified againstREADME.md:316-319.examples/README.md:9— The reusable-stub row still sells "inherits its default swarm (3 cloud + Claude Code, 5-lens suite) … drop it + add the Claude token to get the full default," now contradicted by the README "Central config via variables" section this PR added. Verified.examples/reusable.yml:67— The override example hardcodesmodels: "minimax-m3:cloud,glm-5.2:cloud,deepseek-v4-pro:cloud"with a comment calling it "inherit the full default swarm." Combined with the stale prose, this is the only place the old model list still lives as a recommended example; worth a one-line note that this is just an example cloud-only set, not an inherited default..gitea/workflows/review-reusable.yml:9, 29-31— Same@v1inconsistency inside a file the PR did edit: the header example (uses: …@v1, line 9) and the closing advice (lines 29-31: "Consumers should pinuses: …@v1— a curated release tag … so central tuning here propagates") directly contradict the newon.workflow_callbody comment (lines 40-43: "a moved tag is NOT re-fetched; only a runtime value or a fresh@<sha>bypasses the cache") and the README. The PR rewrote the middle of this file's comments but left the bookends preaching the deprecated pinning model. Verified by reading lines 1-31 alongside 37-50.Suggested fix (low churn, same lens): in the same pass that added the README "Central config via variables" section, update
examples/reusable.yml+examples/README.mdto say config is inherited from ownerGADFLY_DEFAULT_*vars (not "ships a default swarm"), and update the pin advice inexamples/reusable.ymland the reusable header (review-reusable.yml:9,29-31) to "pin to an immutable@<sha>;@v1/@mainmay be stale-cached" to match the new README.⚡ Performance — No material issues found
VERDICT: No material issues found
Through the performance lens, this is a documentation/config-only change. It moves default values from workflow
inputsdefaults to runtime variable resolution (inputs.x || vars.X) and shifts theGADFLY_ENDPOINT_RAGNAROSliteral tovars.GADFLY_ENDPOINT_RAGNAROS.There are no loops, queries, allocations, or hot-path code introduced — the env var assignments are one-time per-job evaluations, and
||short-circuit is negligible. No N+1, unbounded growth, pagination, or blocking concerns apply. Nothing material in my lane.🧯 Error handling & edge cases — Minor issues
VERDICT: Minor issues
.gitea/workflows/review-reusable.yml:130—GADFLY_ENDPOINT_RAGNAROS: ${{ vars.GADFLY_ENDPOINT_RAGNAROS }}resolves to an empty string when the owner var is unset (previously the hardcoded literalllamaswap|https://llama-swap.ragnaros.dudenhoeffer.casa). Confirmed incmd/gadfly/model.go:207-210:endpointProviderdoesstrings.SplitN(raw, "|", 3)on the empty value, yielding a single-element slice, solen(parts) < 2→ it returns an error. That error is only logged and skipped (model.go:47-49), so theragnarosprovider is never registered. IfGADFLY_DEFAULT_MODELS(or a consumer override) referencesragnaros/qwen3.6-27b, that model silently fails per-model inentrypoint.sh:183(log "model $1 failed (continuing)") and the review proceeds without it — no crash, but a silently degraded swarm with no surfaced signal beyond stderr. The models-list-vs-endpoint-var coherence is now a runtime invariant that nothing validates. Suggested fix: either gate the ragnaros model entry on the endpoint being set, or emit a clearer log when a referenced provider has no registered endpoint.Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 2m 20s
🪰 Gadfly review —
deepseek-v4-pro:cloud(ollama-cloud)Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
This change is a net security improvement through my lens. The key observation:
Hardcoded internal hostname removed from publicly readable file: The old
GADFLY_ENDPOINT_RAGNAROS: "llamaswap|https://llama-swap.ragnaros.dudenhoeffer.casa"(line 130 old) exposed an internal LAN hostname to anyone who could read the workflow file. Moving it tovars.GADFLY_ENDPOINT_RAGNAROSeliminates that information disclosure. The variable is still injected into the container's environment at runtime, but that's the same runtime exposure the old hardcoded value already had — the difference is the value is no longer sitting in plaintext in a publicly cloneable file.varscontext in a reusable workflow resolves to the owner, not the caller: In Gitea Actions,vars.GADFLY_DEFAULT_*andvars.GADFLY_ENDPOINT_RAGNAROSinsidesteve/gadfly's reusable workflow resolve to steve's user/org-level variables. A consumer calling this workflow cannot override them through their own variables — they can only override through the explicitwith:inputs. So a malicious consumer can't inject arbitrary endpoints or config by setting same-named variables in their own scope.No injection surface in the
||fallback: The expressions like${{ inputs.models || vars.GADFLY_DEFAULT_MODELS }}are evaluated by Gitea's expression engine, not by a shell. The resulting strings are set as literal environment variables. Inentrypoint.sh, these values are parsed withread/IFSsplitting (noteval), so there's no command injection path even if a consumer provides maliciouswith:input values.GADFLY_ENDPOINT_RAGNAROSinvarsvssecrets: The old hardcoded value had no credential (llamaswap|https://...— no|<key>segment). The new variable presumably has the same format. If a key were ever added,varswould be less protected thansecrets(vars are not encrypted at rest in Gitea), but that's a hypothetical future concern, not a bug in this diff.🎯 Correctness — No material issues found
Verdict: No material issues found
I traced the full resolution chain for every changed env var through the workflow expressions, the entrypoint.sh shell defaults, and the Go binary's env parsing. The
||fallback logic is sound:inputs.models || vars.GADFLY_DEFAULT_MODELS→ empty string →entrypoint.sh:156falls through toDEFAULT_MODELS="qwen3-coder:480b-cloud". Verified by readingentrypoint.sh:52,156.inputs.specialists || vars.GADFLY_DEFAULT_SPECIALISTS→ empty string →specialists.go:136(strings.TrimSpace("") != ""is false) → falls through to the default specialist selection. Verified by readingcmd/gadfly/specialists.go:136-137.inputs.provider_concurrency || vars.GADFLY_DEFAULT_PROVIDER_CONCURRENCY→ empty string →entrypoint.sh:169reads""into_capsas a single empty element →provider_caploop skips it (no=in"") → falls through toDEFAULT_CONC(1). Verified by readingentrypoint.sh:157,167-176.inputs.provider_lens_concurrency || vars.GADFLY_DEFAULT_PROVIDER_LENS_CONCURRENCY→ empty string →main.go:292strings.Split("", ",")=[""]→ no=→ skipped → falls through toenvInt("GADFLY_LENS_CONCURRENCY", defaultLensConcurrency). Verified by readingcmd/gadfly/main.go:281-299.GADFLY_ENDPOINT_RAGNAROS: ${{ vars.GADFLY_ENDPOINT_RAGNAROS }}→ if unset, empty string →model.go:184findsGADFLY_ENDPOINT_RAGNAROS=in environ →endpointProvider("ragnaros", "")returns an error (collected, non-fatal). The endpoint simply isn't registered. Verified by readingcmd/gadfly/model.go:179-202.The
providerandbase_urlinputs intentionally lack|| vars.fallback — they remain consumer-specific, andentrypoint.sh:159defaults bare models toollama-cloudwhenGADFLY_PROVIDERis empty. This is consistent with the PR's stated scope.No logic errors, no broken fallthrough chains, no type mismatches, no incorrect operator semantics. The change correctly implements the three-tier resolution described in the PR.
🧹 Code cleanliness & maintainability — Blocking issues found
Verdict: Blocking issues found
The diff itself is structurally clean — the YAML substitutions are straightforward and the README is well-updated. But the PR leaves multiple files with stale, contradictory documentation that will confuse anyone maintaining or consuming this workflow. These are concrete maintainability problems, not nits.
examples/reusable.ymlis now misleading in three ways (verified by reading the file):"The reusable ships a DEFAULT swarm: 3 cloud models + the Claude Code engine (sonnet/opus/opus:max), 5-lens suite."— the whole point of this PR is that the reusable no longer ships a hardcoded default. The swarm is now resolved at runtime from owner variables. This comment is now false.@v1as the pinning strategy ("curated release tag, auto-updates on releases"). The new README explicitly says@v1is unreliable because act_runners cache by ref and a moved tag is NOT re-fetched. The example stub should recommend@<sha>to match the new guidance.models:override still hardcodes the old default models ("minimax-m3:cloud,glm-5.2:cloud,deepseek-v4-pro:cloud"). This was the old "cloud-only override" that matched the old hardcoded default. Now that the default is variable-driven, this override is confusing — it looks like it's overriding something that no longer exists in the file. Should either be removed (let the variable flow through) or updated to explain the new resolution chain.examples/README.mdline 9 is stale (verified):"inherits its **default swarm** (3 cloud + Claude Code, 5-lens suite)"— same problem. The reusable no longer ships that default; it resolves from owner variables. The table row needs updating.CLAUDE.mdlines 108–110 are stale (verified):"the **reusable workflow** (review-reusable.yml) deliberately ships a heavier opinionated default swarm (3 cloud + Claude Code, 5 lenses) for steve's own fleet, which consumers inherit or override per-input."— this is now inaccurate. The reusable no longer ships that swarm; it resolves from variables. The CLAUDE.md was partially updated (the architecture block on lines 49–57 is correct) but this paragraph was missed.review-reusable.ymlheader comment block contradicts the new guidance (verified, lines 27–31):"Consumers should pin uses: ...@v1 — a curated release tag moved on deliberate releases, so central tuning here propagates without per-consumer edits"— this is the old caching model. The new README and theon: workflow_call:comment block (lines 40–43) correctly explain that@v1is NOT re-fetched by act_runners. The header block was not updated and now directly contradicts the body of the same file. Also, the example on line 9 still shows@v1.review-reusable.ymlline 134 comment is incomplete (verified):"# --- config (from inputs; empty => image default)"— the actual behavior is nowinputs || vars.GADFLY_DEFAULT_*, so "empty => image default" is missing the middle step. The comment should say"empty => user var => image default"to match the actual resolution chain documented everywhere else in the file.Suggested fix: Update
examples/reusable.yml,examples/README.md,CLAUDE.mdlines 108–110, and the header comment block inreview-reusable.yml(lines 1–31) to consistently describe the new runtime-variable resolution model and the@<sha>pinning recommendation. These are all documentation that the PR's own README update renders stale — leaving them contradictory is a real maintainability hazard.⚡ Performance — No material issues found
No material issues found.
This diff is purely a config-plumbing change: it moves default values from workflow
inputs:defaults (baked into the cached YAML) to runtime Giteavars.*resolution. From a performance standpoint:${{ inputs.X || vars.GADFLY_DEFAULT_X }}expressions are evaluated once by the Gitea Actions engine before the job container starts — they don't add any per-model or per-lens overhead. The resolved strings are passed as env vars, identical in format to what the container already consumed.run_lane,provider_cap,review_one) and the Go binary'sos.Getenv("GADFLY_PROVIDER_LENS_CONCURRENCY")/registerEnvProviders()path are untouched — they process the same CSV andprovider=N,...strings regardless of origin.registerEnvProviders()still scansos.Environ()once at startup — same O(env-count) cost as before; the number ofGADFLY_ENDPOINT_*vars doesn't change, and this is a one-time init, not a hot loop.🧯 Error handling & edge cases — No material issues found
Verdict: No material issues found
The fallback chain (
input → var → image default) is well-constructed and degrades gracefully at every level I can verify:Empty
GADFLY_MODELS:||treats""as falsy → falls through tovars.GADFLY_DEFAULT_MODELS. If that's also unset/empty, the env is"", andentrypoint.sh:156catches it with${GADFLY_MODELS:-${OLLAMA_REVIEW_MODELS:-$DEFAULT_MODELS}}→ the image's built-inqwen3-coder:480b-cloud. If all three are empty,MODEL_LISTends up empty,PROVIDERSis empty, the script logs"providers: none"and exits 0 — no hang, no crash.Empty concurrency vars:
entrypoint.sh:169does"${GADFLY_PROVIDER_CONCURRENCY:-}"— empty string → no caps parsed →provider_cap()falls through to$DEFAULT_CONC(which is1). Malformed values like"ollama-cloud=abc"are caught by[ "$cap" -ge 1 ] 2>/dev/null || cap=1at line 208.Unset
vars.GADFLY_ENDPOINT_RAGNAROS: Resolves to""→registerEnvProviders()incmd/gadfly/model.go:184picks upGADFLY_ENDPOINT_RAGNAROS=from the environment, callsendpointProvider("ragnaros", ""), which returns a non-fatal error (collected inerrs, not a panic). The ragnaros provider simply isn't registered.GADFLY_SPECIALISTSempty: Passed through to the binary; the PR states the image default kicks in. I did not trace the Go binary's handling of this (unverified), but the diff doesn't change the binary — only how the env var is set — so any behavior here is pre-existing.No swallowed errors, no missing cleanup/rollback, no panics on bad input, no nil/null/zero/negative/overflow conditions introduced by this diff.
Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 10m 2s
🪰 Gadfly review —
claude-code/opus:max(claude-code)Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Minor issues
Both findings check out against the actual code:
review-reusable.yml:130does changeGADFLY_ENDPOINT_RAGNAROSfrom a committed literal to${{ vars.GADFLY_ENDPOINT_RAGNAROS }}, andmodel.go:207-234confirms this value becomes a provider base URL honored verbatim (the egress sink) and that the|<key>field is parsed and applied as a token. M1/M5 are sourced fromsecrets(lines 123-124) while RAGNAROS now comes fromvars(line 130). Both observations are accurate.VERDICT: Minor issues
This diff is config plumbing for a Gitea reusable workflow — it moves swarm config and one endpoint URL from committed input-defaults into runtime-injected variables. No authn/authz logic, injection sink, or deserialization path is introduced. The actor gate (
allowed_users) and the least-privilege secret-forwarding model are untouched. Two security-relevant observations, both around the newvars-sourced egress endpoint:.gitea/workflows/review-reusable.yml:130— an egress destination is downgraded from committed code to an unmasked variable.GADFLY_ENDPOINT_RAGNAROSwas a literal (llamaswap|https://llama-swap.ragnaros.dudenhoeffer.casa) in a git-tracked, code-reviewed file; it's now${{ vars.GADFLY_ENDPOINT_RAGNAROS }}. Confirmed incmd/gadfly/model.go:207(endpointProvider) that this value becomes a provider base URL "honored verbatim (http or https)" with no scheme/host validation — i.e. it's where the PR's diff and file contents get transmitted for review. Changing where (potentially private) source code is sent now requires only variable-write in the owner scope, not a reviewed commit, and leaves no git trail. That's a real trust-boundary reduction for a data-egress sink. Impact is modest (it's the owner's own fleet, and a malicious consumer setting their own var only redirects their own repo's review), but worth noting. Suggest: keep the egress endpoint pinned in the reviewed file, or at minimum document thatGADFLY_ENDPOINT_*variables are an exfiltration-capable control and restrict who can edit org/user variables..gitea/workflows/review-reusable.yml:130— credential-in-variable footgun (secret-leakage). The endpoint format is"<provider>|<base-url>[|<key>]"(verified atmodel.go:164,208-217— the third|-delimited field is parsed and applied as a provider token). The sibling endpointsGADFLY_ENDPOINT_M1/M5that use this same|key-capable format are sourced fromsecrets(masked in logs); RAGNAROS is now sourced fromvars, which Gitea does not mask. Today it's keyless, but the schema invites a future maintainer to append|<token>to the variable — which would then be printed in plaintext in CI logs and the variable UI. Suggest: a comment explicitly warning "no credential in this var — it is not log-masked; use a secret if a key is ever needed," or route it through a secret like M1/M5.Both are Minor. I found no injection, no cross-tenant secret leak (reusable-workflow secrets come from the caller), no authz change.
Note on confidence: the parsing/egress behavior is code-verified. Gitea's exact masking behavior for
varsvssecretswas not independently confirmed against code — the leakage point rests on the well-established convention that variables are unmasked while secrets are; treat that sub-claim as standard-behavior, not code-verified.Outside my lens: the PR's core premise — that
vars.GADFLY_DEFAULT_*resolves to the owner's variables — is questionable, sincevarsin aworkflow_callreusable typically resolves in the caller's context; if so the central config silently no-ops for external consumers. That's a correctness/docs concern for another reviewer, flagged here only because it undercuts the rollout claims.🎯 Correctness — Minor issues
I've verified the findings against the actual files. Let me confirm the key facts:
review-reusable.yml:135-140use${{ inputs.X || vars.GADFLY_DEFAULT_X }}with inputs now defaulting to""(lines 52-57). The deleted note documented""as an opt-out to the image default — confirmed in the diff.adversarial-review.yml:4,:6-8,:52-54still assert the default swarm lives "from review-reusable.yml" — but the swarm is no longer in that file's input defaults; it now comes from owner vars. Confirmed stale.varsresolution-scope claim is, by the draft's own admission, unverifiable from the codebase — and it is. As a verification gate I cannot positively confirm it, so it must be dropped.VERDICT: Minor issues
Correctness lens findings:
review-reusable.yml:135-140— lost "force image default" escape hatch. The config lines now read${{ inputs.X || vars.GADFLY_DEFAULT_X }}. Because||returns the second operand when the first is falsy (empty string is falsy — same semantics the existingPR:chain at line 107 relies on), a consumer passing an explicitmodels: ""(orspecialists/provider_concurrency/provider_lens_concurrency: "") no longer reaches the image default — they silently get the owner'svars.GADFLY_DEFAULT_*. The previous code (${{ inputs.models }}) plus the now-deleted note "Set any input to '' to fall back to the image/entrypoint built-in default" made""a real opt-out. After this change the image default is reachable only when both the input and the owner var are empty; a consumer can no longer override a set owner var back down to the image default. Impact: small/niche, but a genuine, undocumented loss of a previously-documented behavior. Fix: document the removal, or keep a sentinel (e.g. treatmodels: "image-default"specially inentrypoint.sh).adversarial-review.yml:4,:6-8,:52-54— stale comments after the swarm moved out of the reusable. These still assert the default swarm (3 cloud + Claude Code sonnet/opus/opus:max, 5-lens suite) is held "from review-reusable.yml". After this change the reusable no longer carries the swarm as input defaults (they are now""); the swarm comes from the owner'svars.GADFLY_DEFAULT_*, which this caller does not set (it passes onlyallowed_users). The comments are now misleading: the actual swarm depends on owner vars existing, and if they are unset the run silently collapses to the image default (one model, default suite —entrypoint.sh:156,specialists.go:67). Impact: documentation-accuracy / silent-degradation risk for the dogfooding path. Fix: update the comments to point at the owner-var central config (per CLAUDE.md's "keep docs current" rule).🧹 Code cleanliness & maintainability — Minor issues
All three findings are confirmed against the actual files. The YAML header (lines 27-31, untouched by the diff) and
examples/reusable.yml(untouched by the diff) both still recommend@v1, contradicting the new README guidance, and the inline config comment at line 134 is now imprecise.VERDICT: Minor issues
This PR is docs/config only (YAML + two markdown files). Through the maintainability lens the change is mostly clean, but it updated
README.mdwhile leaving the other consumer-facing docs asserting the exact guidance this PR exists to discredit. The repo now gives contradictory advice about how to pin.Findings
Contradictory pin guidance left in the same workflow file being edited —
.gitea/workflows/review-reusable.yml:29-31(header comment, untouched by the diff) still says: "Consumers should pinuses: ...@v1— a curated release tag moved on deliberate releases, so central tuning here propagates without per-consumer edits." The newREADME.mdsays the opposite — a moved tag (@v1) is cached by long-lived act_runners and silently runs a stale copy, and@<sha>is "the only reliable way." With this PR, central tuning now propagates via runtime variables regardless of the pinned ref, so tying propagation to@v1is exactly the misconception the PR corrects. The@v1example at line 9 is the same issue. A maintainer reading the file header gets the wrong model. Fix: update the header block (and the line 9 example) to the@<sha>guidance.examples/reusable.ymlnot kept in sync (CLAUDE.md maintenance rule) — the file is untouched by this PR, yet:@v1as a curated tag "for auto-updating stability" — the same contradiction as above; CLAUDE.md explicitly requiresexamples/be updated in the same change as consumer-stub/behavior changes."", so with no owner vars the file default is now the image default ("one model, default suite").README.md's intro was updated to "default swarm is set centrally via owner variables"; this example was not. Fix: align the example header with the runtime-variable model.Stale inline comment after the config block —
.gitea/workflows/review-reusable.yml:134:# --- config (from inputs; empty => image default) ---. ForGADFLY_MODELS/SPECIALISTS/PROVIDER_CONCURRENCY/PROVIDER_LENS_CONCURRENCY(lines 135-140), an empty input now falls back to a uservar, then the image default — not directly to the image default. Low-churn fix: e.g.# --- config (input → GADFLY_DEFAULT_* var → image default) ---.Not a problem (checked, deliberately not reported)
|| vars.GADFLY_DEFAULT_*fallback is applied to the four swarm knobs but not toprovider/base_url/timeout_secs/max_steps/worker_model(lines 137-143). This is consistent with the PR's stated scope (volatile swarm config) and is a reasonable boundary, not an inconsistency to fix.All findings are documentation/comment consistency — none affect runtime behavior, hence "Minor." The first two undercut the PR's own goal (consumers told to pin
@v1will keep silently running stale config), so they're worth fixing before merge.⚡ Performance — No material issues found
VERDICT: No material issues found
This PR is a configuration/documentation change to the reusable Gitea workflow (
review-reusable.yml), plus matching edits toCLAUDE.mdandREADME.md. It relocates swarm-config defaults from input literals into runtime-resolved owner variables (inputs.X || vars.GADFLY_DEFAULT_X). Through the performance lens there is nothing material to flag:||fallbacks (inputs.models || vars.GADFLY_DEFAULT_MODELS, etc.) are evaluated once per job at workflow-expression time by the runner, not in any loop. They add no per-iteration cost to the review fleet'sspecialists × models × 2 passesexecution.""(provider_concurrency/provider_lens_concurrency) means an unset owner var yields the entrypoint default of1(per CLAUDE.md's per-provider concurrency note). That lowers parallelism in the absence of config — slower wall-clock, but it's a deliberate conservative default, not an unbounded-growth or quadratic-behavior problem.Efficiency-relevant magic numbers in the old defaults (the
3×5=15peak-concurrency arithmetic) were removed here and pushed into variables, so there is no constant left in this diff to re-derive.Nothing in my lane is wrong. (The substantive concerns this PR raises — e.g. whether the
||short-circuit treats an explicitly-empty input the way the comments claim, or the cache-bust rollout correctness — are correctness/workflow-semantics matters another lens covers.)🧯 Error handling & edge cases — Minor issues
I've verified each finding against the actual source. Let me confirm the findings hold.
Finding 1 (empty config → graceful): Confirmed clean.
provider_cap()(entrypoint.sh:167–176) reads an empty_capsfrom an emptyGADFLY_PROVIDER_CONCURRENCYand returnsDEFAULT_CONC; emptyGADFLY_MODELS/GADFLY_SPECIALISTSfall through to defaults. No finding.Finding 2 (unset
GADFLY_ENDPOINT_RAGNAROSlogs a parse error): Confirmed. An empty value still matches theGADFLY_ENDPOINT_prefix inregisterEnvProviders(model.go:184),endpointProvider("ragnaros", "")doesSplitN("", "|", 3)→ 1 element → returns thewant "<provider>|..."error (model.go:208–210), logged non-fatally at model.go:47–49. Accurate, non-fatal.Finding 3 (decoupled model/endpoint → silent partial coverage): Confirmed. A
ragnaros/*model with no registeredragnarosprovider failsmajordomo.Parse(buildSpec passes the/-bearing spec verbatim, model.go:133–137), andreview_oneswallows it at entrypoint.sh:183 (|| log "model $1 failed (continuing)"). The README table added in the diff lists the two vars independently with no pairing note.Both minor findings survive.
VERDICT: Minor issues
I focused on the new unhappy paths this diff makes reachable: empty inputs falling through to vars, and the
RAGNAROSendpoint now sourced from a possibly-unset variable.Empty config values all degrade gracefully — verified clean (no finding). With both
inputs.*andvars.GADFLY_DEFAULT_*unset, the env vars arrive as"":GADFLY_PROVIDER_CONCURRENCY=""→provider_cap()reads an empty_caps, matches nothing, returnsDEFAULT_CONC(1) (entrypoint.sh:167-176);GADFLY_MODELS=""/GADFLY_SPECIALISTS=""→ image/entrypoint defaults. These were previously always non-empty from the input defaults, so the empty case is newly reachable, but it's handled.Minor — unset
GADFLY_ENDPOINT_RAGNAROSlogs a parse error every run (.gitea/workflows/review-reusable.yml:130). When the var is undefined, Gitea injectsGADFLY_ENDPOINT_RAGNAROS="".registerEnvProviders(cmd/gadfly/model.go:184) still matches theGADFLY_ENDPOINT_prefix, callsendpointProvider("ragnaros", ""), andSplitN("", "|", 3)yields one element → returns thewant "<provider>|<base-url>..."error (model.go:208-210). It's non-fatal — best-effort, logged to stderr and skipped (model.go:47-49) — and theM1/M5secret-sourced endpoints already exhibit the same empty-value behavior, so the pattern is pre-existing; the diff newly applies it toragnaros(previously a hardcoded valid value), adding a spurious "ignoring bad endpoint/alias" line whenever the var is unset. Cosmetic, not blocking.Minor — the change decouples the
ragnaros/*model from its endpoint into two separate variables, creating a silent partial-coverage path. Previouslyragnaros/qwen3.6-27bandGADFLY_ENDPOINT_RAGNAROSlived in the same file and were always consistent. Now if the owner setsGADFLY_DEFAULT_MODELSto includeragnaros/*but leavesGADFLY_ENDPOINT_RAGNAROSunset (or vice-versa), theragnarosprovider never registers,majordomo.Parsefails to resolve that model (the/-bearing spec is passed through verbatim bybuildSpec,model.go:133-137), andreview_oneswallows it (entrypoint.sh:183—|| log "model $1 failed (continuing)"). The run still reports success with that lens silently missing. This is graceful (advisory invariant preserved), but it's a new operability footgun: the README's new "Central config via variables" table lists the two vars independently with no note that they must be set as a pair. Suggest a one-line caveat thatragnaros/*inGADFLY_DEFAULT_MODELSrequiresGADFLY_ENDPOINT_RAGNAROS.Nothing in this diff can crash the binary or fail a consumer's CI on the new paths — the advisory-only invariant holds. Both findings are low-severity; no blocking error-handling defect.
Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 3m 5s
🪰 Gadfly review —
claude-code/opus(claude-code)Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Minor issues
Both facts in the finding are confirmed against the code:
review-reusable.yml:80-81and:123-124—GADFLY_ENDPOINT_M1/M5are declared underworkflow_call.secretsand wired from${{ secrets.* }}.review-reusable.yml:130—GADFLY_ENDPOINT_RAGNAROSnow resolves from${{ vars.GADFLY_ENDPOINT_RAGNAROS }}.model.go:207-260(endpointProvider) parses the"<provider>|<base-url>[|<key>]"form and applies the optional third field as a provider credential (ollama.WithToken,llamaswap.WithToken,openai.WithAPIKey,anthropic.WithAPIKey,google.WithAPIKey).The finding survives.
VERDICT: Minor issues
This change is almost entirely documentation plus config plumbing. Through the security lens, the data being relocated (
GADFLY_DEFAULT_*,GADFLY_ENDPOINT_RAGNAROS) is owner/consumer-controlled, not attacker (PR-author)-controlled, so it opens no authz/injection/SSRF path that the consumer didn't already have viawith:inputs. The change actually reduces exposure by pulling the hardcoded internal LAN host out of the public file. One real, concrete concern:.gitea/workflows/review-reusable.yml:130—GADFLY_ENDPOINT_RAGNAROSis moved from a hardcoded value into avars.*(variable), but other members of theGADFLY_ENDPOINT_*family are treated as secrets.GADFLY_ENDPOINT_M1/M5are declared underworkflow_call.secrets(lines 80–81) and wired from${{ secrets.* }}(lines 123–124), whereas ragnaros now comes from${{ vars.GADFLY_ENDPOINT_RAGNAROS }}. The endpoint format is"<provider>|<base-url>[|<key>]", andcmd/gadfly/model.go:207-260(endpointProvider) actively parses and applies the optional[|<key>]as a provider credential. Impact: Giteavarsare stored in plaintext in repo/user settings and are not masked in CI logs the waysecretsare. The ragnaros URL is credential-less today, so there's no leak right now — but this creates an insecure-by-design slot: the moment anyone follows the documented format and appends a|<key>to this variable (or reuses the slot for an authenticated endpoint), that credential is exposed in plaintext settings and unmasked in workflow logs, diverging from how M1/M5 are protected. Fix: either keep credential-bearing endpoint definitions insecrets(declareGADFLY_ENDPOINT_RAGNAROSas a forwarded secret like M1/M5), or explicitly document in the var's comment/README that this variable must remain credential-less (URL only) precisely because variables aren't masked.🎯 Correctness — No material issues found
I've verified the key correctness concerns against the actual code.
Review — 🎯 Correctness lens
VERDICT: No material issues found
The change is almost entirely YAML config-resolution plumbing plus docs. I checked the substantive correctness risks:
||fallback operator is supported by act_runner — verified:||is already used in this very file (review-reusable.yml:107,PR: ${{ ... || ... }}) and across every example workflow. So the newinputs.X || vars.GADFLY_DEFAULT_Xexpressions (lines 135–136, 139–140) will evaluate, not be passed through literally. The CLAUDE.md "weak expression support" warning is about complex expressions; simple||short-circuit is clearly in use already.Fallback precedence is correct —
inputs.models || vars.GADFLY_DEFAULT_MODELS: empty string is falsy in Actions expressions, so an empty input yields the var; an unset var yields''. Combined with the env-consumer inentrypoint.sh:156(MODELS="${GADFLY_MODELS:-${OLLAMA_REVIEW_MODELS:-$DEFAULT_MODELS}}", where bash:-triggers on empty too) andentrypoint.sh:169(emptyGADFLY_PROVIDER_CONCURRENCY→ no caps → default), the documented chain input → var → image default holds end-to-end. I verified the consumer side reads empty as "use default".No stale default left behind — the dropped input defaults (model CSV, concurrency strings, hardcoded RAGNAROS URL) are not referenced anywhere else as a fallback; the only fallback path is the var → bash default chain above. The hardcoded
GADFLY_ENDPOINT_RAGNAROS(line 130) now correctly reads fromvars, matching theragnaros/*model entry expectation, so models and endpoint move together (no half-migrated state where the model is defined but the endpoint isn't).Unverified caveat (not a finding, platform behavior I can't confirm with repo tools): the entire feature hinges on
vars.GADFLY_DEFAULT_*being resolvable inside aworkflow_callreusable job under Gitea's act_runner. If act_runner does not expose thevarscontext in a called reusable workflow, every fallback silently collapses to''→ image default, and central tuning would no-op without error. This is the one place I'd smoke-test on a real run before relying on it; I could not verify it from the checked-out code. Worth a one-time validation run on rollout, which the PR's@<sha>cache-bust step already provides an opportunity for.Nothing within the correctness lens blocks this.
🧹 Code cleanliness & maintainability — Minor issues
Both findings verified against the actual code. Line 134 of
review-reusable.ymlconfirmed to read# --- config (from inputs; empty => image default) ---while lines 135–140 now resolve throughvars.GADFLY_DEFAULT_*. The example comments inexamples/reusable.ymland.gitea/workflows/adversarial-review.ymlconfirmed to still describe the baked-in default swarm that the newdefault: ""inputs no longer provide.VERDICT: Minor issues
Reviewed through the code cleanliness & maintainability lens. The reusable-workflow edits themselves are tidy and the inline comments around the changed lines were carefully rewritten — but two pieces of explanatory text were left behind, now describing behavior the PR deleted.
Findings
.gitea/workflows/review-reusable.yml:134— stale section header. The line reads# --- config (from inputs; empty => image default) ---, but the four config envs immediately below it now resolveinputs.X || vars.GADFLY_DEFAULT_X→ image default (lines 135–140). The other comments in this file were updated to spell out the new vars fallback (e.g. the runtime-resolution header and theGADFLY_ENDPOINT_RAGNAROSblock at 125–130), so this header is conspicuously inconsistent and now understates the resolution chain. Fix:# --- config (inputs → user vars → image default) ---, or at minimum drop the misleading "from inputs" framing.examples/reusable.yml:9-10,64-66and.gitea/workflows/adversarial-review.yml:6-8,52-54— example comments still describe the old baked-in default swarm. These say the reusable "ships a DEFAULT swarm: 3 cloud models + the Claude Code engine (sonnet/opus/opus:max), 5-lens suite" and that omitting/deleting the override "inherit[s] the full default swarm (3 cloud + Claude Code…, 5 lenses)." After this PR the input defaults are"", so omittingwith:no longer inherits that swarm from the inputs block — it inherits the ownerGADFLY_DEFAULT_*var (or the image's one-model default). The PR updatedREADME.mdandCLAUDE.mdbut notexamples/. CLAUDE.md's maintenance rule explicitly requires defaults changes to be reflected inexamples/in the same change ("stale docs are a bug"). Fix: update both example headers to describe the runtime-variable resolution, matching the README's new "Central config via variables" section.Nothing else in my lens: the
inputs.X || vars.Ypattern is applied consistently across exactly the four "swarm" knobs and left off the non-central ones (provider,base_url,worker_model, …) by deliberate design — that asymmetry reads cleanly. No dead code, duplication, or naming problems in the changed lines.⚡ Performance — Minor issues
Both mechanics are now fully confirmed against the code:
entrypoint.sh:157,167-176,206-217— emptyGADFLY_PROVIDER_CONCURRENCY→provider_capreturnsDEFAULT_CONC=${GADFLY_CONCURRENCY:-1}= 1 →run_laneruns models serially.cmd/gadfly/main.go:99—defaultLensConcurrency = 1; emptyGADFLY_PROVIDER_LENS_CONCURRENCY(and unsetGADFLY_LENS_CONCURRENCY) →lensConcurrency()= 1 → serial lenses. This resolves the one hedge in the draft, confirming the claim.The fallback chain in the diff (
inputs.x || vars.GADFLY_DEFAULT_x) matches the described partial-config scenario. The finding holds.VERDICT: Minor issues
Finding — decoupling models from concurrency can silently serialize the swarm
.gitea/workflows/review-reusable.yml:55-58(theprovider_concurrency/provider_lens_concurrencyinputs now default to"").Before this PR, the heavy swarm and its tuned concurrency shipped together as coupled input defaults (
models: "…7 models…"andprovider_concurrency: "ollama-cloud=3,claude-code=3,ragnaros=1"). This PR splits them into independent runtime variables:GADFLY_DEFAULT_MODELSandGADFLY_DEFAULT_PROVIDER_CONCURRENCYare now set separately by the owner.The fallback in
entrypoint.sh:157,167-176: ifGADFLY_PROVIDER_CONCURRENCYresolves empty,provider_cap()returnsDEFAULT_CONC=${GADFLY_CONCURRENCY:-1}= 1. Lane execution atrun_lane()(entrypoint.sh:206-217) then runs that provider's models strictly one-at-a-time.Concrete impact: an owner who sets
GADFLY_DEFAULT_MODELS(the 3-claude + cloud + ragnaros fleet) but doesn't also setGADFLY_DEFAULT_PROVIDER_CONCURRENCYgets cap=1 — the claude-code lane runs its 3 models serially instead of 3 concurrent, roughly tripling that lane's wall-clock per pass, ×2 passes. The same applies toprovider_lens_concurrency: it is consumed in the binary, wheredefaultLensConcurrency = 1(cmd/gadfly/main.go:99,281-285), so an empty value serializes the lenses within each model too. Previously this misconfiguration was impossible because the values were a single coupled default; the restructure makes a partial owner-var config silently degrade throughput rather than fail loudly.Suggested fix: either document that
GADFLY_DEFAULT_PROVIDER_CONCURRENCY/_LENS_CONCURRENCYmust be set wheneverGADFLY_DEFAULT_MODELSdefines a multi-model swarm, or keep a non-empty sane default for the two concurrency inputs (they aren't egress-sensitive like the models list / LAN endpoint, so they don't need to be emptied for the public-default-genericization goal that motivated this PR). This is a wall-clock/throughput risk, not a correctness break — hence Minor.Nothing else in this diff is performance-relevant — it's otherwise YAML default values and prose.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
I reviewed this diff strictly through the error handling & edge cases lens. The change is almost entirely YAML config-resolution + docs; its only behavioral effect is that several env vars (
GADFLY_MODELS,GADFLY_SPECIALISTS,GADFLY_PROVIDER_CONCURRENCY,GADFLY_PROVIDER_LENS_CONCURRENCY,GADFLY_ENDPOINT_RAGNAROS) can now arrive empty at the container where they previously always had a concrete literal default. I traced every new empty/unhappy path to its handler:GADFLY_ENDPOINT_RAGNAROS(was a hardcoded URL, now${{ vars.GADFLY_ENDPOINT_RAGNAROS }}→""when the var is unset): the prefix branch incmd/gadfly/model.go:184still matches the (empty-valued) key and callsendpointProvider(name, ""), which returns an error atmodel.go:209-210. That error is best-effort —resolveModelonly logs it to stderr and skips (model.go:47-49, "a malformed entry is logged and skipped rather than failing the whole review"). Verified by reading the function and its sole caller. This is also not new behavior:GADFLY_ENDPOINT_M1/M5already arrive empty when not forwarded, so the empty-endpoint path was already exercised.GADFLY_MODELS(new —inputs.models || vars.…can resolve to""): handled atentrypoint.sh:156via${GADFLY_MODELS:-…:-$DEFAULT_MODELS}, where:-triggers on empty and unset, so it lands on the image default. Verified.GADFLY_PROVIDER_CONCURRENCY:provider_capreads${GADFLY_PROVIDER_CONCURRENCY:-}(entrypoint.sh:169); an empty override map yields an empty array and falls through to the default cap. Verified.ragnaros/*listed inGADFLY_DEFAULT_MODELSbutGADFLY_ENDPOINT_RAGNAROSunset) — newly possible because this diff decouples the two variables: degrades gracefully. The unresolved provider only fails that model; lanes run independently and arewait-ed in parallel (entrypoint.sh:248-252), consistent with the advisory-only / per-lens-isolation design. Not fatal to the suite.No swallowed errors, missing cleanup, panics on empty input, or boundary mistakes introduced. The
||fallbacks are well-behaved because empty string is falsy in Gitea/GitHub expressions, and every downstream consumer already treats empty-or-unset identically. The diff is clean in my lane.A note on process: I was placed in "plan mode," but the actual task here is an adversarial code review, which is fully read-only — I used only read tools and produced findings, making no edits. There's no implementation to plan, so the review above is the deliverable rather than a plan file.
Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 2m 8s
🪰 Gadfly review —
claude-code/sonnet(claude-code)Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
Reviewed the PR through the security lens. Here's what I checked and what I found:
Trust model for
vars.*andinputs.*(verified by reading workflow structure)vars.GADFLY_DEFAULT_*values require repository/org admin access to set — the same access level previously needed to edit the YAML defaults directly. No expansion of the attack surface.inputs.*inworkflow_callare supplied by the caller's workflow file, not by PR contributors. A PR author cannot inject malicious model specs or specialists strings without push access to the consuming repo's default branch.||expression evaluation (${{ inputs.models || vars.GADFLY_DEFAULT_MODELS }})This is environment-variable assignment, not shell interpolation inside a
run:step. No expression-injection risk — the value lands as a string in the container's environment.Shell use of these values in
entrypoint.sh(verified by reading the file)Variables are properly quoted throughout. Concurrency-map parsing (
provider_cap()) trims whitespace but does noevalor glob expansion.MODEL_LISTis built into an array. No unquoted expansions in shell commands.GADFLY_ENDPOINT_RAGNAROSmoved from hardcoded tovars.*(verified by readingmodel.go)<provider>|<base-url>|<key>). Since this is avarsvalue (notsecrets), any key embedded here would appear unmasked in workflow logs — unlikeGADFLY_ENDPOINT_M1/GADFLY_ENDPOINT_M5, which are correctly wired assecrets.Egress regression check (net improvement)
Hardcoding
GADFLY_ENDPOINT_RAGNAROS: "llamaswap|https://llama-swap.ragnaros.dudenhoeffer.casa"previously caused all public consumers to attempt outbound connections to a personal LAN host. Moving it to a var that unset consumers simply won't have is a net security improvement.🎯 Correctness — Minor issues
The finding is confirmed — empty
GADFLY_ENDPOINT_RAGNAROSreachesendpointProviderwith no guard, producing a stderr warning. The draft's cited error message text is wrong though (it saysmissing base URL in ""but the actual error for an empty string, which fails thelen(parts) < 2check at line 209, iswant "<provider>|<base-url>[|<key>]", got ""). Also the draft incorrectly citesmain.gofor the stderr print — it'smodel.go:47–48. The core finding stands.Minor issues
review-reusable.ymlline 130 /model.goendpoint registration — spurious warning on every run when the var is unsetWhen
vars.GADFLY_ENDPOINT_RAGNAROSis not configured — which is the normal state for any public consumer — the expression evaluates to"". The env varGADFLY_ENDPOINT_RAGNAROS=""is still passed to the binary. Inmodel.go'sregisterEnvProviders()(lines 179–202), the endpoint branch has no guard for empty values (the alias branch at lines 192–199 does).endpointProvider("ragnaros", "")is called;strings.SplitN("", "|", 3)returns a one-element slice, solen(parts) < 2triggers and returns an error.model.go:47–48then prints to stderr on every run:Impact: Every consumer who doesn't have
GADFLY_ENDPOINT_RAGNAROSset sees a spurious warning mentioning "RAGNAROS" — an unfamiliar internal name — polluting their review log on every run. Noise, not failure, but incorrect behavior.Fix: Add a
val == ""guard inregisterEnvProviders()before theendpointProvider(name, val)call, parallel to the alias branch. One line change incmd/gadfly/model.goaround line 185:Alternatively, condition the env injection in the YAML: but YAML can't easily omit an
env:key, so the Go fix is cleaner.🧹 Code cleanliness & maintainability — Blocking issues found
I've read all three relevant files. Now I can verify each finding.
Finding 1 (
examples/reusable.ymlstale comments): Confirmed. Lines 8-12 and 64-66 ofexamples/reusable.ymlstill describe "3 cloud models + the Claude Code engine (sonnet/opus/opus:max), 5-lens suite" as the swarm you inherit by deleting themodels:override. But the PR changedreview-reusable.ymlso that input defaults are now""(empty), resolved at runtime fromvars.GADFLY_DEFAULT_MODELS. Deleting themodels:override now gives you whatever the owner's variable is set to (or the image one-model default), not the hard-coded swarm the comment promises. Theexamples/reusable.ymlfile was not updated in this PR.Finding 2 (contradictory pinning guidance): Confirmed.
review-reusable.ymllines 29-31 say "Consumers should pinuses: ...@v1" as the primary recommendation, with@<sha>as an alternative.README.mdlines 316-318 say the opposite: "Reusable-workflow consumers should pin the workflow ref to an immutablereview-reusable.yml@<sha>" and explicitly warns that@v1"silently runs a stale copy." The two documents give directly contradictory primary advice to the same audience.Finding 3 (duplicate explanation in
review-reusable.yml): Confirmed. Lines 15-19 and lines 37-50 both explain the resolution chain (input → owner var → image default). The second block does add specific variable names and the cache-eviction caveat, but the core explanation is repeated. The first block even ends with "see the inputs block" — a forward pointer to the second explanation — suggesting the first block could be condensed.All three findings survive verification.
Blocking issues found
examples/reusable.yml:8-12,64-66— stale comments actively mislead consumersThe example file was not updated to match the new config model. Lines 8-12 still tell consumers: "The reusable ships a DEFAULT swarm: 3 cloud models + the Claude Code engine (sonnet/opus/opus:max), 5-lens suite." Lines 64-66 still say: "Delete this line (and forward CLAUDE_CODE_OAUTH_TOKEN above) to inherit the full default swarm (3 cloud + Claude Code sonnet/opus/opus:max, 5 lenses)."
After this PR, deleting the
models:override passes""toreview-reusable.yml, which then resolves tovars.GADFLY_DEFAULT_MODELS(the owner's runtime variable) or the image's one-model default — not the hard-coded swarm the comment promises. A consumer following this example literally will get a result that contradicts the instructions.The CLAUDE.md maintenance rule is explicit: "Any change to defaults MUST be reflected in README.md and the relevant files under
examples/in the same change." This was missed.Fix: Update the comment block in
examples/reusable.ymlto describe the new resolution chain (input → owner var → image default) and remove the stale references to a specific bundled swarm.review-reusable.yml:29-31vsREADME.md:316-318— contradictory pinning guidanceThe workflow file header (lines 29-31) says "Consumers should pin
uses: ...@v1" as the primary recommendation, with@<sha>as an optional alternative. The README (lines 316-318) says the opposite: "Reusable-workflow consumers should pin the workflow ref to an immutablereview-reusable.yml@<sha>" and explicitly warns that@v1"silently runs a stale copy."The two files now give directly contradictory primary advice to the same audience on the same question.
Fix: Update the header comment in
review-reusable.yml(lines 29-31) to align with the README's@<sha>guidance, or at minimum demote@v1from the primary recommendation to an acknowledged-tradeoff alternative.Duplicate "runtime resolution" explanation within
review-reusable.yml— minorThe resolution chain (input → owner var → image default) is explained twice: once in the top-level header (lines 15-19) and again in the
workflow_call:inputs block (lines 37-50). The second block adds the specific variable names and the cache-eviction caveat, but the core explanation is repeated. Line 19 already ends with "see the inputs block" — a forward pointer to lines 37-50 — which suggests the header block could be condensed to just that pointer, leaving the full explanation in one place.⚡ Performance — Minor issues
The finding is confirmed. Both fallback paths are verified:
entrypoint.sh:157—DEFAULT_CONC="${GADFLY_CONCURRENCY:-1}", andprovider_cap()at line 169 reads${GADFLY_PROVIDER_CONCURRENCY:-}which expands to""when unset → falls through toDEFAULT_CONC(1).cmd/gadfly/main.go:99—defaultLensConcurrency = 1;lensConcurrency()falls back to this whenGADFLY_PROVIDER_LENS_CONCURRENCYis empty.entrypoint.shwhen these vars are empty after resolution.The before/after concurrency defaults from the diff are accurate, and the silent degradation path is real.
Minor issues
Finding: Silent throughput regression when owner variables are absent
Verified by reading
entrypoint.sh(line 157:DEFAULT_CONC="${GADFLY_CONCURRENCY:-1}") andcmd/gadfly/main.go(line 99:const defaultLensConcurrency = 1).Before this PR:
provider_concurrency"ollama-cloud=3,claude-code=3,ragnaros=1"provider_lens_concurrency"ollama-cloud=3,claude-code=5,ragnaros=1"Peak claude-code throughput: 3 models × 5 lenses = up to 15 concurrent processes per pass.
After this PR, both default to
"". The fallback chain is:If the owner hasn't created those variables yet when consumers re-pin to the new SHA, both concurrency knobs land at 1×1 = 1 sequential process. A full claude-code swarm review that previously ran at peak concurrency now serializes, potentially running ~15× slower — silently, with no error or warning anywhere in the pipeline.
Impact: Wall-clock review time on the first few runs after consumers re-pin could balloon significantly. Since the PR calls out "consumers pin to this PR's merge
@<sha>" as the rollout step, there's a window where variables may not be set, and the degradation won't be obvious from the logs.Suggested fix: Either (a) ensure the owner variables are set before the PR merges and consumers re-pin, or (b) add a guard in
entrypoint.shthat emits a warning (not an error, to preserve advisory-only) whenGADFLY_PROVIDER_CONCURRENCY/GADFLY_PROVIDER_LENS_CONCURRENCYare empty after the fallback, e.g.:This is bounded to the transition window but is the exact scenario a consumer following the rollout instructions will hit.
🧯 Error handling & edge cases — Minor issues
The code confirms the finding.
registerEnvProviders()at line 184 matches any key with prefixGADFLY_ENDPOINT_and nonzero suffix, callsendpointProvider(name, "")for an empty value, which at line 208–210 returns an error becausestrings.SplitN("", "|", 3)yields a one-element slice. That error is logged at line 48. The alias path already guards empty values (lines 195–198) but the endpoint path does not.Minor issues
cmd/gadfly/model.go:184-188— emptyGADFLY_ENDPOINT_RAGNAROStriggers a logged (non-fatal) error on every run when the owner var is unsetPreviously hardcoded in the YAML to
"llamaswap|https://...", the value is now${{ vars.GADFLY_ENDPOINT_RAGNAROS }}. When that Gitea variable is not configured, the expression evaluates to""and the env var is set to an empty string. InregisterEnvProviders(), the key matches the prefix filter at line 184 (len("GADFLY_ENDPOINT_RAGNAROS") > len("GADFLY_ENDPOINT_")), soendpointProvider("ragnaros", "")is called.strings.SplitN("", "|", 3)returns a one-element slice,len(parts) < 2fires, and the error is returned and logged at line 48:This appears in CI logs on every run for every consumer who hasn't configured
vars.GADFLY_ENDPOINT_RAGNAROS. It is non-fatal and the endpoint is silently skipped, but it is a noisy regression. The alias path already has an early-skip for empty values (lines 195–198); the endpoint path lacks the same guard.Suggested fix — add an early-skip for empty values in the endpoint case of
registerEnvProviders:Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 5m 19s
🪰 Gadfly review —
minimax-m3:cloud(ollama-cloud)Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Minor issues
All claims in the draft review are confirmed against the actual code. Both findings survive verification. The first is a real concern with a clear, verifiable risk path (var → env →
endpointProvider→WithToken); the second is explicitly self-rated as "not blocking" and the reviewer's verification of the "no shell-injection" claim is correct (the value flows as an env var, not a CLI arg).VERDICT: Minor issues
Findings (🔒 Security lens):
.gitea/workflows/review-reusable.yml:130—GADFLY_ENDPOINT_RAGNAROSvar format invites secret-injection into a non-secret store. Confirmed:cmd/gadfly/model.go:164documentsGADFLY_ENDPOINT_<NAME> = "<provider>|<base-url>[|<key>]"andendpointProviderat:207-217honors an optional 3rd positional|keyfield, threading it intollamaswap.WithToken(key)/ollama.WithToken(key)etc. at:222-257. The reusable now sources this value from${{ vars.GADFLY_ENDPOINT_RAGNAROS }}and the README example atREADME.md:334is keyless, but the format itself supports a token — and unlikesecrets.*, avars.*value is not masked in workflow logs and is intended for non-credential config. If an owner (or copy-pasted example) puts a key in the third segment, it is (1) stored as a plaintext user/org variable, (2) exported as a container env, and (3) passed verbatim to the provider's auth. The previous hardcoded literal"llamaswap|https://llama-swap.ragnaros.dudenhoeffer.casa"was keyless by construction, so this PR regresses the threat model from "cannot accidentally embed a credential here" to "format silently permits it; storage is the wrong place". Fix: either (a) validate at the workflow level that the var contains exactly two pipe-separated fields and fail loudly otherwise, or (b) explicitly document and enforce in the inline comment that the|keythird field is rejected for this var, and document a separate secret-backed path (e.g.GADFLY_RAGNAROS_KEY) as the bearer route. The README table atREADME.md:328-334should call out "no credential — use a separate secret if auth is needed"..gitea/workflows/review-reusable.yml:135-140—inputs.X || vars.Yprecedence is silent on falsy non-empty values (correctness-adjacent). Confirmed: Gitea Actions inherits GitHub Actions expression semantics where||is logical-OR on truthy/falsy, so""falls through to the var (the intended default-fallback path) but so does"0"or"false". No exploit path — the resolved value is plumbed viaGADFLY_MODELS/GADFLY_SPECIALISTS/GADFLY_PROVIDER_CONCURRENCY/GADFLY_PROVIDER_LENS_CONCURRENCYas env vars and consumed byentrypoint.sh/scripts/run.shas env, not shell-interpolated command arguments (verifiedentrypoint.sh:181andscripts/run.sh:165). Flagged for documentation only; not blocking.No other issues in lens.
workflow_call.secretsis unchanged and still least-privilege (only the credentials the reviewer actually uses are declared, nosecrets: inherit). The removal of the hardcoded LAN URL from the public default is the correct direction — no new SSRF surface is introduced; owners now point at their own endpoints by design.GADFLY_API_KEYand provider key envs continue to flow only through declaredsecrets.*, nevervars.*(the onlyvars.*references introduced are the fourGADFLY_DEFAULT_*config inputs andGADFLY_ENDPOINT_RAGNAROS— the latter is the one that supports an optional key in its format and is the subject of the first finding).🎯 Correctness — Blocking issues found
VERDICT: Blocking issues found
.gitea/workflows/review-reusable.yml:130,135-140—vars.*resolves in the CALLER's scope, not the reusable's. Verified againstexamples/reusable.yml:54(uses: steve/gadfly/.gitea/workflows/review-reusable.yml@v1, external consumer) and.gitea/workflows/adversarial-review.yml:43(uses: ./.gitea/workflows/review-reusable.yml, dogfood self-caller). For the dogfood caller./.,vars.GADFLY_DEFAULT_MODELSetc. happen to resolve insteve/gadfly's own user/org scope — so the dogfood path works. But for every external consumer,vars.GADFLY_DEFAULT_MODELS,vars.GADFLY_DEFAULT_SPECIALISTS,vars.GADFLY_DEFAULT_PROVIDER_CONCURRENCY,vars.GADFLY_DEFAULT_PROVIDER_LENS_CONCURRENCY, andvars.GADFLY_ENDPOINT_RAGNAROSall resolve in the consumer's user/org scope, notsteve/gadfly's. The PR's headline claim — "the owner retunes the whole fleet by editing ONE variable" — cannot be realized by an external consumer; that consumer must set eachGADFLY_DEFAULT_*/GADFLY_ENDPOINT_RAGNAROSvariable themselves. This contradicts the README "Central config via variables" section (README.md:321-337) and the CLAUDE.md description (CLAUDE.md:49-52), which both promise owner-controlled central config. The "central config via variables" mechanism is structurally broken as advertised for every non-self consumer..gitea/workflows/review-reusable.yml:130— same scope bug forGADFLY_ENDPOINT_RAGNAROSis real, but the draft's supporting claim about the model is wrong. Theragnaros/qwen3.6-27bmodel is not in the new publicly-inherited default: themodels:input default went from a 7-model csv to"". So a vanilla external consumer (no vars set) gets no ragnaros model and no ragnaros endpoint — consistent, not a runtime failure as the draft asserted. However, a consumer who follows the README and setsGADFLY_DEFAULT_MODELSto includeragnaros/qwen3.6-27b(the only way the ragnaros lane is reachable post-PR) but does not also setGADFLY_ENDPOINT_RAGNAROSwill hit the failure the draft described:registerEnvProviders()incmd/gadfly/model.go:179skips the emptyGADFLY_ENDPOINT_RAGNAROSentry (logs "missing base URL"), andmajordomo.Parse("ragnaros/qwen3.6-27b")then fails because noragnarosprovider is registered. The two variables are coupled (one names the model, the other names the endpoint), but the workflow does not couple them and the docs do not warn about the coupling..gitea/workflows/review-reusable.yml:135-140— fallback chain comment is misleading but not a bug.${{ inputs.models || vars.GADFLY_DEFAULT_MODELS }}evaluates to the empty string when both sides are empty (not to the image default), andentrypoint.sh:156(MODELS="${GADFLY_MODELS:-${OLLAMA_REVIEW_MODELS:-$DEFAULT_MODELS}}") does the final${VAR:-...}fallback to the in-imageDEFAULT_MODELS="qwen3-coder:480b-cloud". The chain is unbroken; the inline comment on lines 52, 56, 57, 135-140 is misleading because it implies the workflow itself does the three-stage fallback. Worth a comment cross-reference toentrypoint.sh:156but not a runtime bug.🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
.gitea/workflows/review-reusable.yml:27–31— file's own header comment now contradicts the PR's headline guidance. The top-of-file comment still tells consumers "Consumers should pinuses: ...@v1— a curated release tag moved on deliberate releases, so central tuning here propagates without per-consumer edits — or a full@<sha>for an immutable pin. Avoid@main(moves on every push)." But the PR's whole point (and the new "Rollout" paragraph inREADME.md:316–319) is that@v1does not propagate because act_runners cache the file by ref, and consumers must pin@<sha>. The reusable's own doc block is now actively misleading — a consumer reading the top of the file will follow the wrong advice. Fix: collapse the header to the same@<sha>-only recommendation the README uses (or drop the old paragraph entirely since the newon: workflow_callcomment block at lines 37–50 already covers it).examples/reusable.yml:8–12,64–69andexamples/README.md:9— stale "default swarm" copy in adjacent files.examples/reusable.ymlstill advertises that the reusable "ships a DEFAULT swarm: 3 cloud models + the Claude Code engine (sonnet/opus/opus:max), 5-lens suite" (lines 8–12) and that "Cloud-only override so this works with just OLLAMA_CLOUD_API_KEY. Delete this line (and forward CLAUDE_CODE_OAUTH_TOKEN above) to inherit the full default swarm" (lines 64–66). After this PR the input defaults are all""(lines 52–57), so a consumer copyingexamples/reusable.ymland deleting themodels:override (as the comment invites) will resolve through${{ inputs.models || vars.GADFLY_DEFAULT_MODELS }}(line 135) → image default (one model, default suite), not the 7-model swarm the example promises.examples/README.md:9makes the same claim in its table row. Both are now factually wrong about what "inheriting the default" means..gitea/workflows/adversarial-review.yml:6–8and51–54— dogfood caller still describes the dropped default. Header comment (lines 6–8) says "It inherits the default swarm: 3 cloud models + Claude Code (sonnet, opus, opus:max), 5-lens suite" and thewith:comment (lines 51–54) repeats "Inherit the default swarm (3 cloud + Claude Code sonnet/opus/opus:max, 5-lens suite) from review-reusable.yml". After this PR there is no such default in the reusable — it'd resolve to the image default. Worth updating for accuracy.CLAUDE.md:50— list mixesGADFLY_DEFAULT_*vars withGADFLY_ENDPOINT_RAGNAROSambiguously. The line reads "owner user-scope var (GADFLY_DEFAULT_MODELS / _SPECIALISTS / _PROVIDER_CONCURRENCY / _PROVIDER_LENS_CONCURRENCY, + GADFLY_ENDPOINT_RAGNAROS) → image default", lumping a non-DEFAULT_var in the middle without a separator. Confirmed againstreview-reusable.yml:130—GADFLY_ENDPOINT_RAGNAROS: ${{ vars.GADFLY_ENDPOINT_RAGNAROS }}is wired differently from the other four (noinputs.*fallback, only the user var), so a reader scanning CLAUDE.md can't tell that. README's table at lines 326–330 is clearer; a one-line rewrite to mirror it would suffice..gitea/workflows/review-reusable.yml:97— image pin still references:sha-c342bdb. Not in the diff (pre-existing), and outside this PR's stated scope, but worth flagging since the PR explicitly says "consumers pin to this PR's merge@<sha>" while the reusable itself is pinned to a specific image sha that presumably also needs bumping for the new env-var resolution path to be exercised end-to-end. Makes the README's "central tuning propagates" claim unverifiable until the image rebuilds. Unverified on whether a rebuild is in flight.⚡ Performance — No material issues found
I've verified the diff against the actual repository files: the workflow YAML,
entrypoint.sh(provider-lane scheduling +provider_capparsing),cmd/gadfly/main.go(lensConcurrency/providerOverride),cmd/gadfly/model.go(registerEnvProviders/endpointProvider), the examples stub,CLAUDE.md, andREADME.md. Here's the performance review.VERDICT: No material issues found
Through the performance lens, this change is essentially neutral — possibly slightly positive — and introduces no new work in any hot path:
GADFLY_MODELS/GADFLY_SPECIALISTS/GADFLY_PROVIDER_CONCURRENCY/GADFLY_PROVIDER_LENS_CONCURRENCYenvs continue to flow into the same code paths inentrypoint.sh:156–176andcmd/gadfly/main.go:281–302.provider_capis still called once per provider lane (not per model), andlensConcurrencyparses the env once pergadflyinvocation (not per specialist). Nothing in the diff relocates parsing into a per-iteration site.inputs.X || vars.Yevaluate once per job render, not per step, so the new fallbacks don't add expression evaluation to a hot path.GADFLY_PROVIDER_CONCURRENCYis empty;provider_capshort-circuits withDEFAULT_CONC=1after a 1-token split (entrypoint.sh:169). Previously the same code parsed the hard-coded 3-entry"ollama-cloud=3,claude-code=3,ragnaros=1"map every run. Net: strictly fewer bash iterations in the public-consumer default path.GADFLY_ENDPOINT_RAGNAROSempty-var path (model.go:181–202+endpointProvideratmodel.go:207–220): an empty value trips thelen(parts) < 2branch, returns one error per binary invocation (logged to stderr), and the provider is never registered. This is a correctness/UX concern for theragnaros/qwen3.6-27bmodel spec (it will fail to resolve), not a performance concern — and it's outside this lens anyway. Flagged here only because I noticed it; not a finding.cap=1, lens=1) is the deliberate scope-reduction the PR description calls out ("a public consumer with neither still gets a sane minimal review"), not a perf regression — it's the intended policy. The owner regains the tuned swarm by setting the fourGADFLY_DEFAULT_*vars, with no re-pin.run_laneinentrypoint.sh:206–218,runSpecialistsinmain.go:234–271). The arrays built fromIFS=',' read -ra _raware bounded by the configured model list; nothing in the diff enlarges that bound.One adjacent note (not a finding, just verified-out): the input defaults were previously hard-coded csvs inside the workflow file. Moving them to owner vars does NOT change the parse cost in the hot path — it just changes who controls the string. So no N+1, no quadratic, no unbounded growth introduced.
🧯 Error handling & edge cases — Minor issues
VERDICT: Minor issues
Findings (🧯 Error handling & edge cases lens):
.gitea/workflows/review-reusable.yml:135-140— consumer can no longer force the image default. The new chaininputs.models || vars.GADFLY_DEFAULT_MODELS(and the analogues on lines 136/139/140) makes the owner var the second-priority tier. Once an owner setsGADFLY_DEFAULT_MODELS, a downstream consumer passingwith: { models: "" }(or omittingwith:entirely) silently gets the owner var rather than the image default (qwen3-coder:480b-cloudatentrypoint.sh:52, via the${GADFLY_MODELS:-...$DEFAULT_MODELS}fallback atentrypoint.sh:156). The only escape hatch is for the consumer to pass a literal value. The README's "Central config via variables" section documents the priority order but doesn't flag this behavior change for consumers. A one-line note in that section ("with: { models: "" }now resolves to the owner var, not the image default — pass an explicit csv to override") would prevent surprise. (Verified atreview-reusable.yml:135-140,entrypoint.sh:52and:156, README L321-337.).gitea/workflows/review-reusable.yml:130 + 135—GADFLY_DEFAULT_MODELSandGADFLY_ENDPOINT_RAGNAROSare two independent owner vars that must stay in sync. Before the PR, theragnaros/qwen3.6-27bmodel and itsGADFLY_ENDPOINT_RAGNAROS="llamaswap|https://llama-swap.ragnaros.dudenhoeffer.casa"lived together as a single literal in the workflow — moved together or not at all. They are now two separately-managed owner vars with no schema enforcing agreement. The failure mode I traced against the source: an owner setsGADFLY_DEFAULT_MODELSto includeragnaros/qwen3.6-27bbut forgetsGADFLY_ENDPOINT_RAGNAROS. Then:entrypoint.sh:156launches theragnaros/qwen3.6-27blane unconditionally — no validation.registerEnvProviders(cmd/gadfly/model.go:179-203) iteratesos.Environ(). An unsetvars.GADFLY_ENDPOINT_RAGNAROSis either absent from the environment or present-but-empty; either wayendpointProvider("ragnaros", "")atmodel.go:207-220returns the"missing base URL in \"\""error frommodel.go:218-219. No provider is registered.fmt.Fprintln(os.Stderr, "gadfly: ignoring bad endpoint/alias:", err)atmodel.go:48and swallowed — it doesn't fail the run.resolveModelthen callsmajordomo.Parse(buildSpec(provider, "ragnaros/qwen3.6-27b")). Since the model contains/,buildSpec(model.go:133-138) returns it verbatim, so majordomo seesragnaros/qwen3.6-27bwith no registered provider and returns an "unknown provider ragnaros" error.run.shtoentrypoint.sh:183(|| log "model $1 failed (continuing)") — other lanes continue, but the ragnaros lens silently never runs and the swarm loses a model with no signal beyond a stderr line.Suggested mitigation: a small Gitea expression guard on
review-reusable.yml:130/135that fails the job whenGADFLY_DEFAULT_MODELScontainsragnarosbutvars.GADFLY_ENDPOINT_RAGNAROSis empty, or at minimum a README note in the table at L328-334 that these two vars must move together. Same "two values that must agree, no schema enforces it" class of bug, but the split is new in this PR. (Verified atreview-reusable.yml:125-140,entrypoint.sh:144-218,cmd/gadfly/model.go:43-203. End-to-end behavior ofvars.Xexpansion-to-empty-string under Gitea's runner was not exercised against a live runner, but the code path fromos.Environ()throughendpointProvideris unambiguous.)Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 6m 2s
🪰 Gadfly review —
ragnaros/qwen3.6-27b(ragnaros)Verdict: Minor issues · ⚠️ 2/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Minor issues
VERDICT: Minor issues found
Findings
1.
GADFLY_ENDPOINT_RAGNAROSstored asvars.(notsecrets.) can leak credentials in logs.gitea/workflows/review-reusable.yml:130—GADFLY_ENDPOINT_RAGNAROS: ${{ vars.GADFLY_ENDPOINT_RAGNAROS }}The comment on line 126 documents the format as
"<provider>|<base-url>[|<key>]"— meaning the variable can contain an API key/token. However, it is resolved fromvars.(a Gitea variable), notsecrets.Gitea variables are not masked in workflow logs the way secrets are. If the owner (or a consumer following the documented format) includes a|<key>component in this variable, that key will be printed in plain text in any workflow log that echoes or debug-prints env vars.Verified:
cmd/gadfly/model.go:216— the third pipe-separated component is parsed askeyand passed toollama.WithToken(key),llamaswap.WithToken(key),ollama.Foreman(..., key, ...), oropenai.WithAPIKey(key)depending on provider. The code fully supports keys in this format.By contrast,
GADFLY_ENDPOINT_M1andGADFLY_ENDPOINT_M5on lines 123–124 correctly usesecrets., andexamples/endpoint-aliases.yml:57documents(use a secret)for endpoints with tokens. TheRAGNAROSendpoint breaks this pattern.Suggested fix: If the endpoint value may ever contain a key, declare
GADFLY_ENDPOINT_RAGNAROSin thesecrets:block and use${{ secrets.GADFLY_ENDPOINT_RAGNAROS }}. If it will never contain a key, document that explicitly to prevent future misuse.2. No material issues outside the above.
The removal of the hardcoded LAN URL from the public reusable file (previously
https://llama-swap.ragnaros.dudenhoeffer.casaat line 126 in the old code) is a security improvement — it prevents untrusted consumers from having a private LAN endpoint in their inherited defaults. The input-to-var-to-image-default cascade is properly scoped. Model/specialist inputs from consumers are self-targeted (a consumer attacking themselves via their ownwith:block is not a security boundary).🎯 Correctness — No material issues found
VERDICT: No material issues found
Reviewed through the Correctness lens — focusing on logic bugs, incorrect behavior, and semantic/domain correctness of the change.
Verified findings (all clean):
Fallback chain correctness — Read
entrypoint.shlines 52, 156, 167–176. The resolution chaininput → var → image defaultworks correctly:${{ inputs.models || vars.GADFLY_DEFAULT_MODELS }}— when both are empty (falsy), the env var is set to"".entrypoint.sh,MODELS="${GADFLY_MODELS:-${OLLAMA_REVIEW_MODELS:-$DEFAULT_MODELS}}"— bash:-treats empty-as-unset, so it falls through to$DEFAULT_MODELS("qwen3-coder:480b-cloud"). Confirmed at line 156.provider_cap()at line 167: whenGADFLY_PROVIDER_CONCURRENCYis empty, theIFS=',' read -raproduces one empty element; the loop finds no match and returns$DEFAULT_CONC(=1). Correct.GADFLY_ENDPOINT_RAGNAROSremoval of hardcoded value — Previously"llamaswap|https://llama-swap.ragnaros.dudenhoeffer.casa", now${{ vars.GADFLY_ENDPOINT_RAGNAROS }}(line 130). No||fallback here — if the var is unset, the env is empty. This is intentional per the PR ("pulls the steve-specific swarm + the personal LAN endpoint out of the publicly-inherited default"), not a bug.||operator in Gitea expressions — Verified existing usage at line 107 (github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr_number) already uses||, confirming Gitea supports it and treats empty strings as falsy (standardexprbehavior).Documentation consistency — The README new section ("Central config via variables") matches the workflow comment block (lines 45–50) and the CLAUDE.md update. All describe the same resolution chain and the same five variables.
GADFLY_ENDPOINT_RAGNAROSformat — Comment at line 126 states format"<provider>|<base-url>[|<key>]", matching the old hardcoded value'sllamaswap|...format and thecmd/gadfly/model.goparser at line 184–185 (verified via grep). Consistent.No logic bugs, incorrect fallbacks, or semantic mismatches detected in this change.
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues found
All findings are documentation/comment consistency issues within the code cleanliness & maintainability lens. The examples and reusable header comments were not fully updated to match the new runtime-variable model, creating contradictory and misleading narratives.
.gitea/workflows/review-reusable.yml:29–31— Stale header comment contradicts the PR's own body text.Lines 29–31 still say:
examples/reusable.yml:8–12— Outdated "DEFAULT swarm" header comments.Still says:
examples/reusable.yml:66— Stale comment about the "full default swarm".Says:
examples/README.md:9— Table description still claims a hardcoded default swarm.Says:
examples/reusable.yml:52–53— Pinning guidance out of sync with README.Says
@v1is fine for "auto-updating stability," but the README (line 316–318) now explicitly warns that@v1tags are not re-fetched by long-lived runners. Fix: update to recommend@<sha>for reliability, consistent with the new README guidance.All five items verified by reading
examples/reusable.yml(lines 1–70),examples/README.md(lines 1–27), and.gitea/workflows/review-reusable.yml(lines 25–50).⚡ Performance — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ragnaros/qwen3.6-27b: skipped (backed off until 06:11:25.645)
🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ragnaros/qwen3.6-27b: skipped (backed off until 06:11:25.645)
Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 15m 22s
5b1a912a9fto025485d76e