Nearly every consumer re-pin of review-reusable.yml has been a reviewer image bump, not a structural change (c9dab69, 0d51879, 463aa01, 5007597, 84b891b, 8f5adc9…). The tag was baked into a uses: docker://…:sha-<short> step, and a uses: ref is parsed before any expression context exists — it can't read a variable, so each image release meant editing the stub in every consuming repo.
Change
Run the reviewer as the job container instead of a uses: docker:// step — container.imagedoes accept expressions — with an explicit run: /entrypoint.sh step. The tag now resolves per-run, following the same priority chain as the rest of the swarm config:
inputs.reviewer_tag → user var GADFLY_REVIEWER_TAG → baked fallback (sha-b37cd09, unchanged)
A Gadfly release becomes: build the image → update one user-level variable. Every consumer pinned to this file's @<sha> picks it up on its next review with no re-pin, riding the same vars-are-injected-per-run mechanism as GADFLY_DEFAULT_*. Workflow re-pins remain only for structural yml changes — this PR is one, intended to be the last routine one.
Notes
Entrypoint contract unchanged: identical env block, same /entrypoint.sh brains (bash + the entrypoint already live in the image; ENTRYPOINT was /entrypoint.sh).
Keep the variable on immutable sha- tags, never :latest (act_runner caches :latest and often does not re-pull a moved one).
Tradeoff (noted in comments): vars are unmasked and move without a commit trail — same blast radius as editing this file, minus the audit trail. Acceptable since only the owner edits user vars.
README (image/pinning paragraph + Central config table) and examples/reusable.yml updated per the maintenance rule.
This PR's own gadfly review runs through the local reusable ref, so it exercises the new container: path end-to-end on the act_runner.
## Problem
Nearly every consumer re-pin of `review-reusable.yml` has been a reviewer **image** bump, not a structural change (`c9dab69`, `0d51879`, `463aa01`, `5007597`, `84b891b`, `8f5adc9`…). The tag was baked into a `uses: docker://…:sha-<short>` step, and a `uses:` ref is parsed before any expression context exists — it can't read a variable, so each image release meant editing the stub in every consuming repo.
## Change
Run the reviewer as the **job container** instead of a `uses: docker://` step — `container.image` *does* accept expressions — with an explicit `run: /entrypoint.sh` step. The tag now resolves per-run, following the same priority chain as the rest of the swarm config:
```
inputs.reviewer_tag → user var GADFLY_REVIEWER_TAG → baked fallback (sha-b37cd09, unchanged)
```
A Gadfly release becomes: build the image → update **one** user-level variable. Every consumer pinned to this file's `@<sha>` picks it up on its next review with no re-pin, riding the same vars-are-injected-per-run mechanism as `GADFLY_DEFAULT_*`. Workflow re-pins remain only for structural yml changes — this PR is one, intended to be the last routine one.
## Notes
- Entrypoint contract unchanged: identical env block, same `/entrypoint.sh` brains (bash + the entrypoint already live in the image; `ENTRYPOINT` was `/entrypoint.sh`).
- Keep the variable on immutable `sha-` tags, never `:latest` (act_runner caches `:latest` and often does not re-pull a moved one).
- Tradeoff (noted in comments): vars are unmasked and move without a commit trail — same blast radius as editing this file, minus the audit trail. Acceptable since only the owner edits user vars.
- README (image/pinning paragraph + Central config table) and `examples/reusable.yml` updated per the maintenance rule.
- This PR's own gadfly review runs through the **local** reusable ref, so it exercises the new `container:` path end-to-end on the act_runner.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Nearly every consumer re-pin of review-reusable.yml was a reviewer IMAGE bump,
not a structural change — but the tag was baked into a `uses: docker://` step,
which is parsed before any expression context exists and so can't read a
variable. Run the reviewer as the JOB container instead (container.image DOES
accept expressions) with an explicit `run: /entrypoint.sh` step: the tag now
resolves per-run via inputs.reviewer_tag → user var GADFLY_REVIEWER_TAG → the
baked fallback pin (sha-b37cd09, unchanged).
A Gadfly release becomes: build the image, update the ONE user-level variable —
every consumer pinned to this file's @<sha> picks it up on its next review with
no re-pin. Workflow re-pins remain only for structural yml changes (this is one
— intended to be the last routine one).
- Entrypoint contract unchanged: same env block, same /entrypoint.sh brains
(bash + entrypoint already live in the image; ENTRYPOINT was /entrypoint.sh).
- Keep the variable on immutable sha- tags, never :latest (act_runner caches
:latest and often does not re-pull a moved one).
- Tradeoff noted in comments: vars are unmasked and move without a commit
trail — same blast radius as editing this file, minus the audit trail.
- README (image/pinning para + Central config table) and examples/reusable.yml
updated per the maintenance rule.
Co-Authored-By: Claude Fable 5 <[email protected]>
Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
<!-- gadfly-status-board -->
## 🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-23 02:50:35Z
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `opencode/glm-5.2:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `opencode/kimi-k2.6:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
🪰Gadfly consensus review — 1 inline finding on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
<!-- gadfly-inline-review -->
🪰 **Gadfly consensus review** — 1 inline finding on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
🟡Reviewer-tag runtime-resolution priority chain is explained in full prose twice in the same file
maintainability · flagged by 1 model
🪰 Gadfly · advisory
🟡 **Reviewer-tag runtime-resolution priority chain is explained in full prose twice in the same file**
_maintainability · flagged by 1 model_
<sub>🪰 Gadfly · advisory</sub>
My security review of this PR is complete — verdict: No material issues found. The change moves the reviewer image tag resolution from a commit-baked uses: docker://...@sha to a runtime-resolved container: image: expression; I verified there's no injection vector, no secret leakage into the image-selection expression, and the one real trust-boundary tradeoff (unaudited var-based image selection) is already explicitly disclosed and accepted in the PR description itself. Since this was a review task with no code to implement, I recorded the outcome in the plan file rather than proposing changes — let me know if you'd like me to actually apply any changes or dig into a different lens (correctness, performance, etc.).
🎯 Correctness — No material issues found
VERDICT: No material issues found
I reviewed this PR specifically for logic bugs in the runtime-tag resolution mechanism (the container.image expression, the input/var/fallback precedence chain, and the switch from a uses: docker:// step to a container: + run: /entrypoint.sh step).
Verification performed:
Read the full .gitea/workflows/review-reusable.yml post-diff: the container.image expression ${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }} follows the exact same ||-fallback idiom already used elsewhere in the same file (e.g. GADFLY_MODELS: ${{ inputs.models || vars.GADFLY_DEFAULT_MODELS }}), so the precedence logic is internally consistent.
Read Dockerfile: bash is installed and ENTRYPOINT ["/entrypoint.sh"] is chmod'd executable, so run: /entrypoint.sh as a job-container step works regardless of the step's default shell (sh vs bash) — the shebang (#!/usr/bin/env bash) is honored by the kernel either way.
Read entrypoint.sh: it is fully env-driven (no positional args), uses a self-managed WORKDIR="/tmp/gadfly" rather than $GITHUB_WORKSPACE, and its process-management (pkill -f/wait) doesn't depend on being container PID 1 — so moving it from the container's own entrypoint invocation to a run: step inside a job container doesn't change its behavior.
Cross-checked README.md and examples/reusable.yml updates against the workflow file — the documented precedence chain (reviewer_tag input → GADFLY_REVIEWER_TAG var → sha-b37cd09 fallback) matches the actual expression.
Confirmed adversarial-review.yml (gadfly's own dogfood caller) doesn't pass reviewer_tag, so it correctly falls through to var/default — no inconsistency there.
Confirmed the other examples/*.yml full-stub files (still using uses: docker://...:latest) are a separate, pre-existing, unaffected code path — not touched by or in scope of this diff.
No logic bugs, no incorrect fallback ordering, no broken cross-file assumption found.
🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
.gitea/workflows/review-reusable.yml:27-34 vs :106-121 — the reviewer-tag runtime-resolution priority chain (inputs.reviewer_tag → user var GADFLY_REVIEWER_TAG → baked fallback) and the rationale for why per-run resolution matters ("update ONE variable, no consumer re-pin") are spelled out in full prose twice in the same file: the top-of-file header comment (27-31) and the new 16-line comment block directly above container: (106-121, specifically 106-111). They're worded differently ("resolves at runtime" vs. "resolve per-run") but state the identical priority chain and identical consequence. The per-input annotation at line 66 is a short one-liner consistent with the file's existing convention for other inputs and isn't itself excessive. A future change to the priority chain (e.g. adding an org-level tier) requires updating both prose blocks to stay consistent, and they've already begun to drift in phrasing. The 106-121 block does contain some content not in the header (the container-vs-uses: rationale, the blast-radius/no-audit-trail caveat, and the fallback-pin changelog), so trimming should keep that unique content and only fold the repeated priority-chain sentence into a pointer back to the header.
⚡ Performance — No material issues found
VERDICT: No material issues found
The draft is not a review — it's a clarification request (the drafting model got stuck on an apparent Plan Mode conflict and never analyzed the diff, so it produced no bullets and no gadfly-findings block). There are no findings to verify or carry forward.
🧯 Error handling & edge cases — No material issues found
Review — Error handling & edge cases lens
VERDICT: No material issues found
This PR is a workflow/doc restructuring (moving the reviewer image from a uses: docker://… step to a container: job image with an explicit run: /entrypoint.sh step, plus a || fallback chain for the tag). I checked the areas most likely to introduce error-handling regressions:
Fallback chain correctness: image: gitea.stevedudenhoeffer.com/steve/gadfly:${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }} (.gitea/workflows/review-reusable.yml:123) always resolves to a non-empty tag — the literal fallback guarantees the image ref is never malformed even if both the input and var are unset/empty. This mirrors the pre-existing GADFLY_MODELS: ${{ inputs.models || vars.GADFLY_DEFAULT_MODELS }} pattern already in the same file (line 170), so the ||-on-empty-string behavior is an established, already-proven idiom here, not new risk.
Env/secret forwarding preserved: diffed the full step body (lines 124–184) against the pre-change file — every secret (OLLAMA_CLOUD_API_KEY, ANTHROPIC_API_KEY, CLAUDE_CODE_OAUTH_TOKEN, etc.) and config env var that was attached to the old uses: docker:// step is still attached to the new run: step, unchanged. Nothing silently dropped in the migration.
entrypoint.sh's cleanup/watchdog logic is untouched by this diff (verified by reading entrypoint.sh:370-394, entrypoint.sh:406-414): the PR-budget watchdog (pkill -f), the lane waits, and the disarm-marker race guard are all invocation-model-agnostic — they operate on process trees inside the container via bash job control, not on how the container itself was started (uses: vs container:+run:). I don't see a code path where switching to job-container execution changes whether those background jobs (status board, per-model reviews, watchdog subshell) get reaped on the happy path or on die()/non-zero exit.
The reviewer_tag input default is "", consistent with every other optional input in the block (models, specialists, etc.), so an unset input degrades the same way the rest of the swarm config already does.
One thing I could not fully verify with static reading (flagging as a caveat, not a finding, since I have no way to confirm act_runner's actual behavior from the repo alone): whether a job timeout-minutes cancellation or container-pull failure under the new container:+docker exec execution model reaps entrypoint.sh's background children identically to the old single-step docker:// model. The PR description says this path was exercised end-to-end on the act_runner, which covers the happy path; I have no evidence either way for the cancellation/timeout edge case, and didn't find a concrete defect to point at, so I'm not reporting it as a finding.
glm-5.2:cloud (ollama-cloud) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
The change's security-relevant surface is the new reviewer_tag input and the vars.GADFLY_REVIEWER_TAG variable both flowing into container.image at .gitea/workflows/review-reusable.yml:123:
Image-redirection scope is bounded. The registry/repository prefix gitea.stevedudenhoeffer.com/steve/gadfly: is a literal in the expression — a consumer setting reviewer_tag via with: can only control the tag of the owner's existing image, not the registry or image name. There's no path/namespace traversal that lets a caller redirect to an attacker-controlled image (Docker reference syntax doesn't allow a tag to rewrite the repo path). Verified at review-reusable.yml:123.
Whoever controls the owner's user vars can redirect the reviewer image with no commit trail — this is an acknowledged threat model, documented in the new comments at review-reusable.yml:113-117 and the README. The blast radius is "same as editing this file, minus the audit trail," which the PR accepts as in-scope-by-design. No unmasked-secret leak is introduced; GADFLY_REVIEWER_TAG is config, not a credential.
No new secret exposure. The env: block (review-reusable.yml:127-184) is unchanged from the prior docker:// step form — same GITEA_TOKEN, same forwarded provider secrets, same endpoint vars. Moving the image from a uses: docker:// step to the job container does not alter the credential set available to the entrypoint. Verified by reading the full env block.
Consumer-forced :latest is possible but advisory-only. A consuming repo could pass reviewer_tag: "latest", which the comments warn against (act_runner caches :latest and may run stale). This is a reliability/staleness concern, not a privilege escalation, and the review is explicitly advisory ("never blocks a merge"), so I'm not counting it as a security finding.
Nothing in the authn/authz, injection, secret-leakage, or untrusted-input lanes rises to a material issue here.
🎯 Correctness — No material issues found
Verdict: No material issues found
I verified the change through the correctness lens and could not confirm a real defect introduced by this diff. Specifics I checked against the checked-out code:
Equivalent entrypoint contract (Dockerfile:58-60, entrypoint.sh:1,56): the image's ENTRYPOINT ["/entrypoint.sh"] is exactly what the new step invokes via run: /entrypoint.sh. The old uses: docker://… step ran the image's ENTRYPOINT; the new path calls the same script explicitly. The env: block moved from the uses: step to the run: step — same scope, same vars reach /entrypoint.sh. No semantic drift in what the reviewer executes.
Tag fallback chain is correct (review-reusable.yml:123): ${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }} mirrors the established inputs.x || vars.GADFLY_DEFAULT_X idiom (e.g. line 170-177). Empty-string is falsy in Gitea's ||, so the "" default on reviewer_tag falls through correctly. The baked fallback sha-b37cd09 is identical to the old docker://…:sha-b37cd09 pin — no stale/drifted pin introduced, and the comment's "PR #27" attribution matches the pre-existing comment.
Backwards compatible (review-reusable.yml:66): reviewer_tag is a new optional string input defaulting to ""; existing callers (e.g. .gitea/workflows/adversarial-review.yml:51-55, examples/reusable.yml:65) don't pass it and still resolve to the baked pin. No caller breakage.
Docs/contract consistency: README.md:404-408,419 and examples/reusable.yml:20-22 describe the same priority chain and the "empty ⇒ baked fallback" semantics that the YAML implements. The README table row matches the input comment. The full-stub examples (adversarial-review.yml, claude-code.yml, etc.) use uses: docker:// directly and are intentionally a different path — correctly untouched by this PR.
Unverified verification gap (not a confirmed finding): The change's correctness rests on two act_runner behaviors I could not confirm from this repository — (1) that jobs.<id>.container.image actually evaluates ${{ }} expressions at run time (the PR's whole premise), and (2) that a job container whose image ENTRYPOINT is a one-shot script (/entrypoint.sh that exits) is kept alive for the run: step rather than the entrypoint firing once on container start and then the run: step double-firing or failing. The PR description states the dogfood run "exercises the new container: path end-to-end on the act_runner," which is the strongest evidence both hold; I could not independently verify act_runner internals here. If that end-to-end run did not actually cover a consumer pinned at @<sha>withGADFLY_REVIEWER_TAG set (vs. just the local fallback), it would be worth confirming both explicitly. I am not confident enough in either being broken to report it as a finding.
🧹 Code cleanliness & maintainability — No material issues found
The diff is clean through the maintainability lens. Verified against the checked-out code: the new container: block follows the same input → vars → fallback resolution pattern used by the surrounding GADFLY_MODELS/GADFLY_SPECIALISTS env lines, the baked fallback tag matches the prior pin, and the README/examples comments stay consistent with the new mechanism.
VERDICT: No material issues found
The change converts a uses: docker://…:sha-<short> step into a job-container: + explicit run: /entrypoint.sh step. Through the maintainability lens this is a net positive: the tag resolution now matches the established inputs.x || vars.GADFLY_DEFAULT_X pattern already used for the config env vars on lines 170–177, eliminating the special-cased per-release edit that the README calls out. No duplicated logic or divergent fallback constant is introduced — the sha-b37cd09 fallback appears exactly once (.gitea/workflows/review-reusable.yml:123) after previously living once in the uses: line, so there's no new drift risk.
Comment hygiene is good and follows the surrounding style: the new container: block comment at lines 106–121 explains why (expression context availability) rather than what, and the fallback-pin comment preserves the prior PR provenance note. The on.inputs.reviewer_tag doc at line 66 mirrors the wording of the other input docs (same "empty falls back to user var …, then …" chain) rather than inventing a new convention.
README and examples/reusable.yml comments were updated consistently (verified at README.md:404-408,419 and examples/reusable.yml:20-22), so the maintenance rule about keeping the docs in sync with the reusable is honored; no stale references to the old uses: form remain in the touched files.
No dead code introduced: the old step comment block (lines 100–105 in the old file) is removed in lockstep with the step it described.
⚡ Performance — No material issues found
Verdict: No material issues found
Through the performance lens, this change is neutral:
Image pull cost is unchanged. Both the old uses: docker://…:sha-b37cd09 step and the new container.image: …:${{ expr }} resolve to a single immutable image ref per run. act_runner caches by ref either way; the expression only selects which ref, it doesn't force a re-pull of a cached one. A new release tag means one new pull — exactly as before, when each release re-pinned the uses: line.
No extra steps or work added. The diff replaces one step (uses:) with one step (run: /entrypoint.sh). Step count and entrypoint invocations are identical (verified at review-reusable.yml:124-126).
No new per-run overhead, loops, allocations, or unbounded growth. This is a workflow YAML change; there are no hot paths, N+1 patterns, or pagination concerns introduced.
Workspace/clone behavior unchanged. There is no actions/checkout step before or after — the reviewer operates via the Gitea API, not the mounted workspace — so switching from a uses: docker:// action to a job container: adds no clone/mount overhead that wasn't already present.
I verified the job body (review-reusable.yml:102-184) to confirm no additional steps, env-driven repeated work, or checkout were introduced alongside the container switch.
🧯 Error handling & edge cases — No material issues found
Verdict: No material issues found
Through the error-handling / edge-case lens, the diff's unhappy paths are handled soundly:
Fallback chain (review-reusable.yml:123): ${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }} — an empty input, an unset/empty user var, and a missing var all collapse to the baked sha- fallback. || treats both unset and empty-string as falsy, so there's no "set-but-empty" hole that would resolve to a bare : and fail the pull. Verified at line 66 (default: "") and line 123.
No shell-injection / empty-arg surface: run: /entrypoint.sh is a literal command with no input interpolation (review-reusable.yml:126), so a malicious or malformed reviewer_tag value can't escape into a shell — worst case is a bad image ref that fails the pull with a clear runner error.
Missing/malformed image tag: a typo'd GADFLY_REVIEWER_TAG (e.g. a non-existent sha-xxxx) fails at container pull before any step runs — a clean, loud failure, not a swallowed error. The step has no if: that would mask it.
container: vs uses: docker:// parity for the entrypoint: the env block (lines 127–184) is unchanged and identical, and /entrypoint.sh is invoked explicitly, so the env-driven contract the entrypoint relies on is preserved. No new nil/empty env var introduced (all inputs default to "" and the entrypoint already treats empty as "use image default" per the file's own comments).
README/examples: no error-handling surface — pure doc.
One thing I considered and could not verify with the available tools (flagging as unverified, not a finding): whether act_runner auto-authenticates pulls from the private gitea.stevedudenhoeffer.com registry for a container: image the same way it does for uses: docker://. If it does not, a pull would fail with an auth error. This would be a regression specific to the uses: docker:// → container: switch, but it's outside what I can confirm from the repo, and the PR description states the local review already exercises the container: path end-to-end — so I'm treating it as not-material rather than a finding.
kimi-k2.6:cloud (ollama-cloud) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
🎯 Correctness — No material issues found
No material issues found
I've carefully traced the semantic transformation through the correctness lens. The change replaces a uses: docker://… step with a job-level container: + explicit run: /entrypoint.sh step. Here's what I verified:
Expression resolution logic is correct: inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' correctly implements the documented priority chain. An empty-string default on reviewer_tag is falsy in GitHub/Gitea Actions expressions, so it falls through to the variable, then the baked fallback. This was confirmed by reading the full file and checking the inputs block (.gitea/workflows/review-reusable.yml:66).
Semantic equivalence of execution: The old uses: docker://… ran the image's ENTRYPOINT (/entrypoint.sh) with the env: block injected. The new container: + run: /entrypoint.sh executes the same absolute path inside the same container image with the identical env: block. The entrypoint contract is preserved (confirmed by PR description and comments at lines 106-123).
Environment forwarding unchanged: All secrets.* and vars.* references in the env: block remain on the step and resolve identically in a container job as they did in a uses: step. No env variable references were altered or dropped.
Image reference syntax is correct: container.image takes a bare registry reference (no docker:// prefix), and the expression interpolation ${{ … }} in that field is standard supported syntax. The fallback string 'sha-b37cd09' is a valid tag literal.
No logic bugs, incorrect fall-through behavior, or semantic breakage introduced.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
The change is clean and consistent across all touched files. The runtime tag resolution chain (inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09') is applied uniformly in the YAML, README, and example stub. The fallback pin sha-b37cd09 is referenced consistently everywhere it appears. Comments are substantive and explain the caching/trust tradeoffs rather than stating the obvious. No dead code, no confusing names, no copy-paste that should be shared, no structural smells.
⚡ Performance — No material issues found
No material issues found
Reviewed the change through the performance lens. The move from a uses: docker://… step to a container: job container is architecturally neutral for performance:
No additional overhead: In both the old and new design, the gadfly image must be pulled and started before the entrypoint executes. A single-step job inside a job container is equivalent to a single uses: docker:// step in terms of container lifecycle.
No extra resolution cost: The expression inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' evaluates once at job startup — a trivial string fallback chain with no hot-path impact.
No N+1 or cache-thrashing in the workflow: The change adds no loops, repeated API calls, or unbounded data structures. Image-cache behavior with dynamic tags is the intended feature (immutable sha- tags are enforced by convention, not code, but that's outside performance scope).
No redundant work: The run: /entrypoint.sh step is functionally identical to the prior ENTRYPOINT-driven invocation; the same env block is forwarded, and the entrypoint already self-manages cloning and execution.
The change successfully centralizes image-tag updates without introducing any measurable efficiency regression.
🧯 Error handling & edge cases — No material issues found
Verdict: No material issues found
The PR cleanly migrates the reviewer execution from a uses: docker:// step to a job-level container: + explicit run: /entrypoint.sh step, with the image tag resolved at runtime via ${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }}.
Verified via repository read of review-reusable.yml, entrypoint.sh, and Dockerfile:
Fallback chain is robust: inputs.reviewer_tag (default "", falsy) → vars.GADFLY_REVIEWER_TAG (unset/empty is falsy) → hardcoded 'sha-b37cd09'. The || semantics in Gitea Actions expressions correctly skip empty strings and fall through to the baked fallback.
Entrypoint contract preserved: The image's ENTRYPOINT ["/entrypoint.sh"] is explicitly invoked by the run: step, so the same bash process runs with the identical env: block. No behavioral change in signal handling, process management (pkill, background &/wait), or $WORKDIR usage.
Error handling unchanged: entrypoint.sh retains set -uo pipefail, explicit || true / || log "..." fallbacks for API calls, and die for truly fatal conditions (missing GITEA_API, GITEA_TOKEN, PR, EVENT_NAME, unresolvable branch). Optional envs (IS_DRAFT, COMMENT_BODY, COMMENT_ID, etc.) all use ${VAR:-} defaults.
No new unhandled edge cases: The only new user-controlled input is reviewer_tag; an invalid tag (whitespace, nonexistent digest) fails at image-pull time with a clear runner error, which is the same failure mode as a bad hardcoded tag in the old approach. No other inputs gain new failure modes.
opencode/glm-5.2:cloud (opencode) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Verdict: No material issues found
I reviewed the change through the security lens (authn/authz, injection, supply-chain image integrity, secret handling, input validation of untrusted data).
What I checked:
review-reusable.yml:122-126 — the new container.image expression ${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }}. The registry/repo prefix gitea.stevedudenhoeffer.com/steve/gadfly: is hardcoded; only the tag segment is variable. So a caller or var-editor can select a different tag of the same image, not redirect to an arbitrary registry/image. No shell context is involved (it's a container image ref pulled by the runner, not a command), so there's no command/SQL/path injection surface. Verified by reading the full file.
review-reusable.yml:66 — reviewer_tag is a workflow_call input sourced from the consumer's stub. The consumer repo controls its own with: values, so a malicious reviewer_tag only affects that consumer's own job (self-harm, not a cross-trust escalation). examples/reusable.yml does not forward reviewer_tag, so consumers inherit the var/fallback chain by default.
entrypoint.sh and Dockerfile:60 — the ENTRYPOINT is /entrypoint.sh, and the step explicitly run: /entrypoint.sh, so moving from uses: docker:// to a job container preserves the same entrypoint contract. No new code-exec surface introduced; the same env: block (GITEA_TOKEN, forwarded provider secrets) is passed to the step identically. Verified.
permissions block (lines 97-100) is unchanged; the container runs with the same contents:read / issues:write / pull-requests:write as before.
The one security-relevant tradeoff (not a defect): the image tag now mutates via an owner-scope variable with no commit trail, so anyone who can edit owner-level vars can redirect every consumer's reviewer image to a different tag of the gadfly image. The PR explicitly documents this at review-reusable.yml:113-117 ("same blast radius as editing this file, but without a commit trail"). The blast radius is bounded — the fixed prefix confines redirection to tags of the same image repo (an attacker still needs registry-push creds to plant a malicious tag), and the trust set (owner-var editors) is essentially the same as repo committers. This is an accepted, in-code-documented supply-chain tradeoff, not an unmitigated vulnerability, so I'm not flagging it as a finding requiring a fix.
No sha--pattern enforcement exists on the variable, but that's an operational foot-gun (stale :latest caching) for the owner only, not a security issue.
🎯 Correctness — No material issues found
The docker-reviewer PR is correct as a self-contained change. The adversarial-review.yml caller forwards pr_number but NOT reviewer_tag — so inputs.reviewer_tag is empty, falling back to vars.GADFLY_REVIEWER_TAG then the baked sha-b37cd09 fallback. This is the intended priority chain, and the existing behavior (fixed tag) is preserved when no var is set.
Let me verify the vars context is accessible inside a reusable workflow.
Verdict: No material issues found
Through the correctness lens, the change from a uses: docker://…:tag step to a container: job container is sound:
Entrypoint contract preserved. The env block is byte-identical, and /entrypoint.sh is invoked explicitly via run: /entrypoint.sh. The image's ENTRYPOINT ["/entrypoint.sh"] (Dockerfile:60) is effectively ignored when used as a job container (the runner uses the container's shell for run: steps), so the explicit invocation is correct and not double-run. Verified Dockerfile:60 and entrypoint.sh:56 (set -uo pipefail under bash, which the image installs at Dockerfile:30).
No working-directory regression. All paths the entrypoint and run.sh use are absolute or derived from WORKDIR="${WORKDIR:-/tmp/gadfly}" (entrypoint.sh:63), not from the container's cwd. The repo is cloned to ${WORKDIR}/repo (entrypoint.sh:178) and GADFLY_REPO_DIR is set explicitly (entrypoint.sh:275), so run.sh's cwd-relative fallback (run.sh:174) never triggers. The container: switch changes neither.
Priority chain semantics correct.image: …:${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }} — empty strings fall through via ||, and the baked fallback reproduces the prior pinned tag exactly, so a caller that sets neither (e.g. this repo's own adversarial-review.yml, which forwards no reviewer_tag — verified at .gitea/workflows/adversarial-review.yml:51-58) gets identical behavior to before.
Advisory-only invariant intact. The step's exit code is still the entrypoint's exit code (non-triggers exit 0; config errors non-zero), unchanged by the uses:→container: swap.
vars context in a called workflow is already relied on in this same file (vars.GADFLY_DEFAULT_*, lines 170/171/177), so vars.GADFLY_REVIEWER_TAG resolving at runtime is consistent with established behavior.
README and examples/reusable.yml updates match the workflow change (verified README.md:404-408, 419; examples/reusable.yml:19-22).
🧹 Code cleanliness & maintainability — No material issues found
VERDICT: No material issues found
Through the code-cleanliness/maintainability lens the change is clean:
The new container.image expression uses the exact inputs.x || vars.Y || 'fallback' fallback pattern already established throughout the env: block (e.g. line 170 GADFLY_MODELS: ${{ inputs.models || vars.GADFLY_DEFAULT_MODELS }}), so it follows the surrounding convention rather than introducing a new idiom.
The header comments (lines 27-34, 106-121) and the inline input-block comment (line 49, 66) explain why the container: form was chosen over uses: docker:// and document the priority chain — this is the kind of non-obvious rationale that aids future maintainers; no dead/duplicated prose.
The fallback pin sha-b37cd09 appears in both the comment (line 119) and the image line (line 123); that's intentional explanatory duplication of a magic value (mirrors how the prior uses: block documented its sha), not copy-paste drift.
README's Central config via variables table (line 419) and the new structural-vs-routine paragraph (lines 404-408) and examples/reusable.yml header (lines 20-22) are all updated consistently per the maintenance rule — no stale doc left behind.
examples/reusable.yml does not thread reviewer_tag in its with: block, but that's correct: the input defaults to "" and the user-var GADFLY_REVIEWER_TAG is the intended per-fleet mechanism, matching how the example treats the other GADFLY_DEFAULT_* knobs (it doesn't forward those either).
Verified by reading .gitea/workflows/review-reusable.yml, examples/reusable.yml, README.md, entrypoint.sh, and Dockerfile in full.
⚡ Performance — No material issues found
Through the Performance lens, this change converts a uses: docker:// step into a container: job container and moves the image tag to a runtime-resolved expression. I verified the surrounding workflow, the Dockerfile entrypoint contract, and entrypoint.sh to assess any efficiency impact.
Findings:
No material performance regression. The diff touches only workflow orchestration (.gitea/workflows/review-reusable.yml, README, example). It introduces no code on a hot path, no loops, no allocations, no queries, and no repeated work. The expression ${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }} is evaluated once at job setup — negligible.
Image pull behavior is unchanged in substance. Both uses: docker://<image>:<tag> and container.image pull the image once per job; the caching concern (:latest not re-pulled) is preserved by keeping the resolution on immutable sha- tags, which is explicitly documented in the added comments. No new unbounded or repeated pull is introduced.
No new per-step overhead. There is a single step (run: /entrypoint.sh); running it inside the job container vs. a uses: step container is effectively equivalent — one container lifecycle per job either way. entrypoint.sh does its own shallow clone to /tmp/gadfly/repo (git clone --depth=1) and does not depend on the runner workspace layout, so the container-mode workspace mounting doesn't add extra I/O.
Entrypoint contract verified unchanged.Dockerfile sets ENTRYPOINT ["/entrypoint.sh"] and the env block forwarded to the step is identical to before; the binary path (/usr/local/bin/gadfly) and scripts dir (/app/scripts) referenced by entrypoint.sh still exist in the image (Dockerfile lines 56-59). No extra filesystem traversal or missing-path retries are introduced.
Verified by reading: review-reusable.yml (full), entrypoint.sh (full), Dockerfile (full), adversarial-review.yml (caller), build-image.yml, and examples/reusable.yml.
VERDICT: No material issues found
🧯 Error handling & edge cases — No material issues found
I've reviewed the diff through the error-handling & edge-cases lens.
VERDICT: No material issues found
The change is a workflow YAML mechanical refactor: swapping a uses: docker://… step for a container: job with an explicit run: /entrypoint.sh. Verified against the actual code:
Entrypoint contract intact.Dockerfile:58-60 bakes /entrypoint.sh and sets ENTRYPOINT ["/entrypoint.sh"]. The new run: /entrypoint.sh (.gitea/workflows/review-reusable.yml:126) invokes the same script the ENTRYPOINT would have run under uses: docker://, so the bash brains (gating, clone, model loop, consolidation) run identically. No new unhappy path is introduced by the move itself.
Env block preserved. The diff only shows the changed lines, but reading the full file (lines 127-184) confirms the entire env: block (event context, forwarded secrets, endpoint vars, config inputs) is unchanged and still attached to the /entrypoint.sh step. entrypoint.sh's required-var guards (: "${GITEA_API:?...}", etc., lines 68-71) still fire with the same inputs.
Tag-resolution fallback chain.${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }} has a concrete baked fallback, so an unset input + unset var does not produce an empty/invalid image ref — it resolves to the same immutable pin the old uses: line used. No empty-image-pull edge case.
No swallowed errors / missing cleanup introduced. The PR doesn't touch entrypoint.sh or any script; the advisory-only invariant, watchdog disarm, and board teardown all live unchanged in entrypoint.sh (lines 411-426).
No edge-case, error-handling, or cleanup regressions in my lane.
opencode/kimi-k2.6:cloud (opencode) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
Security analysis: The change moves the reviewer from a uses: docker:// step to a container: job with runtime-resolved image tag. There are no authn/authz gaps, injection vectors, or secret leaks introduced. The env: block and secret forwarding are identical to the prior implementation. The run: /entrypoint.sh uses an absolute path, avoiding any workspace-path confusion. The new inputs.reviewer_tag input allows callers to specify a tag, but callers already control their own workflows; this does not expand attack surface in a reusable workflow_call context. The documented trade-off about user/org variables (GADFLY_REVIEWER_TAG) being editable without a commit trail is acknowledged inline (.gitea/workflows/review-reusable.yml:115-117) and is an accepted operational choice, not a vulnerability. The fallback pin remains hardcoded, ensuring a deterministic default.
🎯 Correctness — No material issues found
Verdict: No material issues found
I scrutinized the change for logic bugs, semantic correctness, and expression-evaluation issues. The refactoring from a uses: docker://… step to a job-level container: + explicit run: /entrypoint.sh step is semantically equivalent:
entrypoint.sh is executable, has a bash shebang, and is copied to /entrypoint.sh by the Dockerfile (verified at Dockerfile:58 and Dockerfile:59). Running it explicitly via run: produces the same behavior as the image's ENTRYPOINT ["/entrypoint.sh"].
The environment block is still attached to the single step that executes the reviewer, so all variables (GITEA_API, GITEA_TOKEN, GADFLY_MODELS, etc.) reach the script unchanged.
The tag-resolution expression ${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }} correctly implements the documented priority chain. vars.* is already used throughout this reusable workflow (e.g., vars.GADFLY_DEFAULT_MODELS, vars.GADFLY_ENDPOINT_NETHERSTORM at lines 156–165 of review-reusable.yml), so vars.GADFLY_REVIEWER_TAG is consistent with existing patterns.
The fallback tag sha-b37cd09 is unchanged from the previous hardcoded pin; no drift in default behavior.
The adversarial-review.yml caller does not pass reviewer_tag, which correctly triggers the fallback chain.
No logic errors, incorrect thresholds, broken formulas, or semantic misbehaviors introduced.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
⚡ Performance — No material issues found
No material issues found
From the Performance lens, this change is strictly neutral. It is a workflow YAML restructuring with no impact on runtime efficiency:
The change moves the reviewer from a step-level uses: docker://… to a job-level container: with one run: /entrypoint.sh step. For a single-step job, container startup and teardown overhead is identical — one image pull, one container lifecycle in both cases.
The inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' expression is evaluated once at job scheduling time; there is no hot-path re-evaluation, loop, or query introduced.
The entrypoint contract and env block are unchanged, so all runtime behavior (model execution, lens passes, compaction, token budgets) is identical. No allocations, copies, or blocking patterns are added.
🧯 Error handling & edge cases — No material issues found
No material issues found
Reviewed through the error-handling & edge-cases lens. The change moves the reviewer from a uses: docker://… step to a job-level container: + explicit run: /entrypoint.sh, with the image tag resolved dynamically via ${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }}.
The fallback chain (input → variable → literal) correctly handles empty/null values; the baked-in literal ensures the tag is never empty.
The entrypoint.sh contract is unchanged: it still shallow-clones, gates triggers, and exits 0 for non-review events. The env block passed to the step is identical to the old uses: step env block.
No new nil/null dereferences, unbounded iterations, or integer paths are introduced by this diff.
The step continues to fail hard on missing required env (e.g., PR, GITEA_API), which is consistent with the existing behavior and is intentional for config errors per the entrypoint comments.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 5 models
**Verdict: Minor issues** · 1 finding (0 with multi-model agreement)
<details><summary>1 single-model finding (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟡 | Reviewer-tag runtime-resolution priority chain is explained in full prose twice in the same file | `.gitea/workflows/review-reusable.yml:106` | claude-code/sonnet | maintainability |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
My security review of this PR is complete — verdict: **No material issues found**. The change moves the reviewer image tag resolution from a commit-baked `uses: docker://...@sha` to a runtime-resolved `container: image:` expression; I verified there's no injection vector, no secret leakage into the image-selection expression, and the one real trust-boundary tradeoff (unaudited var-based image selection) is already explicitly disclosed and accepted in the PR description itself. Since this was a review task with no code to implement, I recorded the outcome in the plan file rather than proposing changes — let me know if you'd like me to actually apply any changes or dig into a different lens (correctness, performance, etc.).
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**VERDICT: No material issues found**
I reviewed this PR specifically for logic bugs in the runtime-tag resolution mechanism (the `container.image` expression, the input/var/fallback precedence chain, and the switch from a `uses: docker://` step to a `container:` + `run: /entrypoint.sh` step).
Verification performed:
- Read the full `.gitea/workflows/review-reusable.yml` post-diff: the `container.image` expression `${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }}` follows the exact same `||`-fallback idiom already used elsewhere in the same file (e.g. `GADFLY_MODELS: ${{ inputs.models || vars.GADFLY_DEFAULT_MODELS }}`), so the precedence logic is internally consistent.
- Read `Dockerfile`: `bash` is installed and `ENTRYPOINT ["/entrypoint.sh"]` is chmod'd executable, so `run: /entrypoint.sh` as a job-container step works regardless of the step's default shell (`sh` vs `bash`) — the shebang (`#!/usr/bin/env bash`) is honored by the kernel either way.
- Read `entrypoint.sh`: it is fully env-driven (no positional args), uses a self-managed `WORKDIR="/tmp/gadfly"` rather than `$GITHUB_WORKSPACE`, and its process-management (`pkill -f`/`wait`) doesn't depend on being container PID 1 — so moving it from the container's own entrypoint invocation to a `run:` step inside a job container doesn't change its behavior.
- Cross-checked README.md and examples/reusable.yml updates against the workflow file — the documented precedence chain (`reviewer_tag` input → `GADFLY_REVIEWER_TAG` var → `sha-b37cd09` fallback) matches the actual expression.
- Confirmed `adversarial-review.yml` (gadfly's own dogfood caller) doesn't pass `reviewer_tag`, so it correctly falls through to var/default — no inconsistency there.
- Confirmed the other `examples/*.yml` full-stub files (still using `uses: docker://...:latest`) are a separate, pre-existing, unaffected code path — not touched by or in scope of this diff.
No logic bugs, no incorrect fallback ordering, no broken cross-file assumption found.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**VERDICT: Minor issues**
- `.gitea/workflows/review-reusable.yml:27-34` vs `:106-121` — the reviewer-tag runtime-resolution priority chain (`inputs.reviewer_tag → user var GADFLY_REVIEWER_TAG → baked fallback`) and the rationale for why per-run resolution matters ("update ONE variable, no consumer re-pin") are spelled out in full prose twice in the same file: the top-of-file header comment (27-31) and the new 16-line comment block directly above `container:` (106-121, specifically 106-111). They're worded differently ("resolves at runtime" vs. "resolve per-run") but state the identical priority chain and identical consequence. The per-input annotation at line 66 is a short one-liner consistent with the file's existing convention for other inputs and isn't itself excessive. A future change to the priority chain (e.g. adding an org-level tier) requires updating both prose blocks to stay consistent, and they've already begun to drift in phrasing. The 106-121 block does contain some content not in the header (the container-vs-`uses:` rationale, the blast-radius/no-audit-trail caveat, and the fallback-pin changelog), so trimming should keep that unique content and only fold the repeated priority-chain sentence into a pointer back to the header.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
VERDICT: No material issues found
The draft is not a review — it's a clarification request (the drafting model got stuck on an apparent Plan Mode conflict and never analyzed the diff, so it produced no bullets and no `gadfly-findings` block). There are no findings to verify or carry forward.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
## Review — Error handling & edge cases lens
**VERDICT: No material issues found**
This PR is a workflow/doc restructuring (moving the reviewer image from a `uses: docker://…` step to a `container:` job image with an explicit `run: /entrypoint.sh` step, plus a `||` fallback chain for the tag). I checked the areas most likely to introduce error-handling regressions:
- **Fallback chain correctness**: `image: gitea.stevedudenhoeffer.com/steve/gadfly:${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }}` (`.gitea/workflows/review-reusable.yml:123`) always resolves to a non-empty tag — the literal fallback guarantees the image ref is never malformed even if both the input and var are unset/empty. This mirrors the pre-existing `GADFLY_MODELS: ${{ inputs.models || vars.GADFLY_DEFAULT_MODELS }}` pattern already in the same file (line 170), so the `||`-on-empty-string behavior is an established, already-proven idiom here, not new risk.
- **Env/secret forwarding preserved**: diffed the full step body (lines 124–184) against the pre-change file — every secret (`OLLAMA_CLOUD_API_KEY`, `ANTHROPIC_API_KEY`, `CLAUDE_CODE_OAUTH_TOKEN`, etc.) and config env var that was attached to the old `uses: docker://` step is still attached to the new `run:` step, unchanged. Nothing silently dropped in the migration.
- **entrypoint.sh's cleanup/watchdog logic is untouched by this diff** (verified by reading `entrypoint.sh:370-394`, `entrypoint.sh:406-414`): the PR-budget watchdog (`pkill -f`), the lane `wait`s, and the disarm-marker race guard are all invocation-model-agnostic — they operate on process trees inside the container via bash job control, not on how the container itself was started (`uses:` vs `container:`+`run:`). I don't see a code path where switching to job-container execution changes whether those background jobs (status board, per-model reviews, watchdog subshell) get reaped on the happy path or on `die()`/non-zero exit.
- The reviewer_tag `input` default is `""`, consistent with every other optional input in the block (`models`, `specialists`, etc.), so an unset input degrades the same way the rest of the swarm config already does.
One thing I could **not** fully verify with static reading (flagging as a caveat, not a finding, since I have no way to confirm act_runner's actual behavior from the repo alone): whether a job `timeout-minutes` cancellation or container-pull failure under the new `container:`+`docker exec` execution model reaps entrypoint.sh's background children identically to the old single-step `docker://` model. The PR description says this path was exercised end-to-end on the act_runner, which covers the happy path; I have no evidence either way for the cancellation/timeout edge case, and didn't find a concrete defect to point at, so I'm not reporting it as a finding.
</details>
</details>
<details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — No material issues found</summary>
**Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
## VERDICT: No material issues found
The change's security-relevant surface is the new `reviewer_tag` input and the `vars.GADFLY_REVIEWER_TAG` variable both flowing into `container.image` at `.gitea/workflows/review-reusable.yml:123`:
```yaml
image: gitea.stevedudenhoeffer.com/steve/gadfly:${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }}
```
I checked this through the security lens:
- **Image-redirection scope is bounded.** The registry/repository prefix `gitea.stevedudenhoeffer.com/steve/gadfly:` is a literal in the expression — a consumer setting `reviewer_tag` via `with:` can only control the *tag* of the owner's existing image, not the registry or image name. There's no path/namespace traversal that lets a caller redirect to an attacker-controlled image (Docker reference syntax doesn't allow a tag to rewrite the repo path). Verified at `review-reusable.yml:123`.
- **Whoever controls the owner's user vars can redirect the reviewer image with no commit trail** — this is an acknowledged threat model, documented in the new comments at `review-reusable.yml:113-117` and the README. The blast radius is "same as editing this file, minus the audit trail," which the PR accepts as in-scope-by-design. No unmasked-secret leak is introduced; `GADFLY_REVIEWER_TAG` is config, not a credential.
- **No new secret exposure.** The `env:` block (`review-reusable.yml:127-184`) is unchanged from the prior `docker://` step form — same `GITEA_TOKEN`, same forwarded provider secrets, same endpoint vars. Moving the image from a `uses: docker://` step to the job container does not alter the credential set available to the entrypoint. Verified by reading the full env block.
- **Consumer-forced `:latest` is possible but advisory-only.** A consuming repo could pass `reviewer_tag: "latest"`, which the comments warn against (act_runner caches `:latest` and may run stale). This is a reliability/staleness concern, not a privilege escalation, and the review is explicitly advisory ("never blocks a merge"), so I'm not counting it as a security finding.
Nothing in the authn/authz, injection, secret-leakage, or untrusted-input lanes rises to a material issue here.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**Verdict: No material issues found**
I verified the change through the correctness lens and could not confirm a real defect introduced by this diff. Specifics I checked against the checked-out code:
- **Equivalent entrypoint contract** (`Dockerfile:58-60`, `entrypoint.sh:1,56`): the image's `ENTRYPOINT ["/entrypoint.sh"]` is exactly what the new step invokes via `run: /entrypoint.sh`. The old `uses: docker://…` step ran the image's ENTRYPOINT; the new path calls the same script explicitly. The `env:` block moved from the `uses:` step to the `run:` step — same scope, same vars reach `/entrypoint.sh`. No semantic drift in what the reviewer executes.
- **Tag fallback chain is correct** (`review-reusable.yml:123`): `${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }}` mirrors the established `inputs.x || vars.GADFLY_DEFAULT_X` idiom (e.g. line 170-177). Empty-string is falsy in Gitea's `||`, so the "" default on `reviewer_tag` falls through correctly. The baked fallback `sha-b37cd09` is identical to the old `docker://…:sha-b37cd09` pin — no stale/drifted pin introduced, and the comment's "PR #27" attribution matches the pre-existing comment.
- **Backwards compatible** (`review-reusable.yml:66`): `reviewer_tag` is a new optional string input defaulting to `""`; existing callers (e.g. `.gitea/workflows/adversarial-review.yml:51-55`, `examples/reusable.yml:65`) don't pass it and still resolve to the baked pin. No caller breakage.
- **Docs/contract consistency**: `README.md:404-408,419` and `examples/reusable.yml:20-22` describe the same priority chain and the "empty ⇒ baked fallback" semantics that the YAML implements. The README table row matches the input comment. The full-stub examples (`adversarial-review.yml`, `claude-code.yml`, etc.) use `uses: docker://` directly and are intentionally a different path — correctly untouched by this PR.
**Unverified verification gap (not a confirmed finding):** The change's correctness rests on two `act_runner` behaviors I could not confirm from this repository — (1) that `jobs.<id>.container.image` actually evaluates `${{ }}` expressions at run time (the PR's whole premise), and (2) that a job container whose image `ENTRYPOINT` is a one-shot script (`/entrypoint.sh` that exits) is kept alive for the `run:` step rather than the entrypoint firing once on container start and then the `run:` step double-firing or failing. The PR description states the dogfood run "exercises the new `container:` path end-to-end on the act_runner," which is the strongest evidence both hold; I could not independently verify `act_runner` internals here. If that end-to-end run did not actually cover a consumer pinned at `@<sha>` *with* `GADFLY_REVIEWER_TAG` set (vs. just the local fallback), it would be worth confirming both explicitly. I am not confident enough in either being broken to report it as a finding.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
The diff is clean through the maintainability lens. Verified against the checked-out code: the new `container:` block follows the same `input → vars → fallback` resolution pattern used by the surrounding `GADFLY_MODELS`/`GADFLY_SPECIALISTS` env lines, the baked fallback tag matches the prior pin, and the README/examples comments stay consistent with the new mechanism.
VERDICT: No material issues found
- The change converts a `uses: docker://…:sha-<short>` step into a job-`container:` + explicit `run: /entrypoint.sh` step. Through the maintainability lens this is a net positive: the tag resolution now matches the established `inputs.x || vars.GADFLY_DEFAULT_X` pattern already used for the config env vars on lines 170–177, eliminating the special-cased per-release edit that the README calls out. No duplicated logic or divergent fallback constant is introduced — the `sha-b37cd09` fallback appears exactly once (`.gitea/workflows/review-reusable.yml:123`) after previously living once in the `uses:` line, so there's no new drift risk.
- Comment hygiene is good and follows the surrounding style: the new `container:` block comment at lines 106–121 explains *why* (expression context availability) rather than *what*, and the fallback-pin comment preserves the prior PR provenance note. The `on.inputs.reviewer_tag` doc at line 66 mirrors the wording of the other input docs (same "empty falls back to user var …, then …" chain) rather than inventing a new convention.
- README and `examples/reusable.yml` comments were updated consistently (verified at `README.md:404-408,419` and `examples/reusable.yml:20-22`), so the maintenance rule about keeping the docs in sync with the reusable is honored; no stale references to the old `uses:` form remain in the touched files.
- No dead code introduced: the old step comment block (lines 100–105 in the old file) is removed in lockstep with the step it described.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**Verdict: No material issues found**
Through the performance lens, this change is neutral:
- **Image pull cost is unchanged.** Both the old `uses: docker://…:sha-b37cd09` step and the new `container.image: …:${{ expr }}` resolve to a single immutable image ref per run. act_runner caches by ref either way; the expression only selects which ref, it doesn't force a re-pull of a cached one. A new release tag means one new pull — exactly as before, when each release re-pinned the `uses:` line.
- **No extra steps or work added.** The diff replaces one step (`uses:`) with one step (`run: /entrypoint.sh`). Step count and entrypoint invocations are identical (verified at `review-reusable.yml:124-126`).
- **No new per-run overhead, loops, allocations, or unbounded growth.** This is a workflow YAML change; there are no hot paths, N+1 patterns, or pagination concerns introduced.
- **Workspace/clone behavior unchanged.** There is no `actions/checkout` step before or after — the reviewer operates via the Gitea API, not the mounted workspace — so switching from a `uses: docker://` action to a job `container:` adds no clone/mount overhead that wasn't already present.
I verified the job body (`review-reusable.yml:102-184`) to confirm no additional steps, env-driven repeated work, or checkout were introduced alongside the container switch.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
**Verdict: No material issues found**
Through the error-handling / edge-case lens, the diff's unhappy paths are handled soundly:
- **Fallback chain (`review-reusable.yml:123`)**: `${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }}` — an empty input, an unset/empty user var, and a missing var all collapse to the baked `sha-` fallback. `||` treats both unset and empty-string as falsy, so there's no "set-but-empty" hole that would resolve to a bare `:` and fail the pull. Verified at line 66 (`default: ""`) and line 123.
- **No shell-injection / empty-arg surface**: `run: /entrypoint.sh` is a literal command with no input interpolation (`review-reusable.yml:126`), so a malicious or malformed `reviewer_tag` value can't escape into a shell — worst case is a bad image ref that fails the pull with a clear runner error.
- **Missing/malformed image tag**: a typo'd `GADFLY_REVIEWER_TAG` (e.g. a non-existent `sha-xxxx`) fails at container pull before any step runs — a clean, loud failure, not a swallowed error. The step has no `if:` that would mask it.
- **`container:` vs `uses: docker://` parity for the entrypoint**: the env block (lines 127–184) is unchanged and identical, and `/entrypoint.sh` is invoked explicitly, so the env-driven contract the entrypoint relies on is preserved. No new nil/empty env var introduced (all inputs default to `""` and the entrypoint already treats empty as "use image default" per the file's own comments).
- **README/examples**: no error-handling surface — pure doc.
One thing I considered and could **not verify** with the available tools (flagging as unverified, not a finding): whether `act_runner` auto-authenticates pulls from the private `gitea.stevedudenhoeffer.com` registry for a `container:` image the same way it does for `uses: docker://`. If it does not, a pull would fail with an auth error. This would be a regression specific to the `uses: docker://` → `container:` switch, but it's outside what I can confirm from the repo, and the PR description states the local review already exercises the `container:` path end-to-end — so I'm treating it as not-material rather than a finding.
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — No material issues found</summary>
**Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**No material issues found**
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
No material issues found
I've carefully traced the semantic transformation through the correctness lens. The change replaces a `uses: docker://…` step with a job-level `container:` + explicit `run: /entrypoint.sh` step. Here's what I verified:
- **Expression resolution logic is correct**: `inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09'` correctly implements the documented priority chain. An empty-string default on `reviewer_tag` is falsy in GitHub/Gitea Actions expressions, so it falls through to the variable, then the baked fallback. This was confirmed by reading the full file and checking the `inputs` block (`.gitea/workflows/review-reusable.yml:66`).
- **Semantic equivalence of execution**: The old `uses: docker://…` ran the image's `ENTRYPOINT` (`/entrypoint.sh`) with the `env:` block injected. The new `container:` + `run: /entrypoint.sh` executes the same absolute path inside the same container image with the identical `env:` block. The entrypoint contract is preserved (confirmed by PR description and comments at lines 106-123).
- **Environment forwarding unchanged**: All `secrets.*` and `vars.*` references in the `env:` block remain on the step and resolve identically in a container job as they did in a `uses:` step. No env variable references were altered or dropped.
- **Image reference syntax is correct**: `container.image` takes a bare registry reference (no `docker://` prefix), and the expression interpolation `${{ … }}` in that field is standard supported syntax. The fallback string `'sha-b37cd09'` is a valid tag literal.
No logic bugs, incorrect fall-through behavior, or semantic breakage introduced.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
**No material issues found**
The change is clean and consistent across all touched files. The runtime tag resolution chain (`inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09'`) is applied uniformly in the YAML, README, and example stub. The fallback pin `sha-b37cd09` is referenced consistently everywhere it appears. Comments are substantive and explain the caching/trust tradeoffs rather than stating the obvious. No dead code, no confusing names, no copy-paste that should be shared, no structural smells.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**No material issues found**
Reviewed the change through the **performance** lens. The move from a `uses: docker://…` step to a `container:` job container is architecturally neutral for performance:
- **No additional overhead**: In both the old and new design, the gadfly image must be pulled and started before the entrypoint executes. A single-step job inside a job container is equivalent to a single `uses: docker://` step in terms of container lifecycle.
- **No extra resolution cost**: The expression `inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09'` evaluates once at job startup — a trivial string fallback chain with no hot-path impact.
- **No N+1 or cache-thrashing in the workflow**: The change adds no loops, repeated API calls, or unbounded data structures. Image-cache behavior with dynamic tags is the intended feature (immutable `sha-` tags are enforced by convention, not code, but that's outside performance scope).
- **No redundant work**: The `run: /entrypoint.sh` step is functionally identical to the prior `ENTRYPOINT`-driven invocation; the same env block is forwarded, and the entrypoint already self-manages cloning and execution.
The change successfully centralizes image-tag updates without introducing any measurable efficiency regression.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
**Verdict:** No material issues found
The PR cleanly migrates the reviewer execution from a `uses: docker://` step to a job-level `container:` + explicit `run: /entrypoint.sh` step, with the image tag resolved at runtime via `${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }}`.
Verified via repository read of `review-reusable.yml`, `entrypoint.sh`, and `Dockerfile`:
- **Fallback chain is robust**: `inputs.reviewer_tag` (default `""`, falsy) → `vars.GADFLY_REVIEWER_TAG` (unset/empty is falsy) → hardcoded `'sha-b37cd09'`. The `||` semantics in Gitea Actions expressions correctly skip empty strings and fall through to the baked fallback.
- **Entrypoint contract preserved**: The image's `ENTRYPOINT ["/entrypoint.sh"]` is explicitly invoked by the `run:` step, so the same bash process runs with the identical `env:` block. No behavioral change in signal handling, process management (`pkill`, background `&`/`wait`), or `$WORKDIR` usage.
- **Error handling unchanged**: `entrypoint.sh` retains `set -uo pipefail`, explicit `|| true` / `|| log "..."` fallbacks for API calls, and `die` for truly fatal conditions (missing `GITEA_API`, `GITEA_TOKEN`, `PR`, `EVENT_NAME`, unresolvable branch). Optional envs (`IS_DRAFT`, `COMMENT_BODY`, `COMMENT_ID`, etc.) all use `${VAR:-}` defaults.
- **No new unhandled edge cases**: The only new user-controlled input is `reviewer_tag`; an invalid tag (whitespace, nonexistent digest) fails at image-pull time with a clear runner error, which is the same failure mode as a bad hardcoded tag in the old approach. No other inputs gain new failure modes.
</details>
</details>
<details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — No material issues found</summary>
**Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
Verdict: No material issues found
I reviewed the change through the security lens (authn/authz, injection, supply-chain image integrity, secret handling, input validation of untrusted data).
What I checked:
- `review-reusable.yml:122-126` — the new `container.image` expression `${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }}`. The registry/repo prefix `gitea.stevedudenhoeffer.com/steve/gadfly:` is hardcoded; only the **tag** segment is variable. So a caller or var-editor can select a different *tag of the same image*, not redirect to an arbitrary registry/image. No shell context is involved (it's a container image ref pulled by the runner, not a command), so there's no command/SQL/path injection surface. Verified by reading the full file.
- `review-reusable.yml:66` — `reviewer_tag` is a `workflow_call` input sourced from the consumer's stub. The consumer repo controls its own `with:` values, so a malicious `reviewer_tag` only affects that consumer's own job (self-harm, not a cross-trust escalation). `examples/reusable.yml` does not forward `reviewer_tag`, so consumers inherit the var/fallback chain by default.
- `entrypoint.sh` and `Dockerfile:60` — the `ENTRYPOINT` is `/entrypoint.sh`, and the step explicitly `run: /entrypoint.sh`, so moving from `uses: docker://` to a job container preserves the same entrypoint contract. No new code-exec surface introduced; the same `env:` block (GITEA_TOKEN, forwarded provider secrets) is passed to the step identically. Verified.
- `permissions` block (lines 97-100) is unchanged; the container runs with the same `contents:read / issues:write / pull-requests:write` as before.
The one security-relevant *tradeoff* (not a defect): the image tag now mutates via an owner-scope variable with no commit trail, so anyone who can edit owner-level vars can redirect every consumer's reviewer image to a different tag of the gadfly image. The PR explicitly documents this at `review-reusable.yml:113-117` ("same blast radius as editing this file, but without a commit trail"). The blast radius is bounded — the fixed prefix confines redirection to tags of the *same* image repo (an attacker still needs registry-push creds to plant a malicious tag), and the trust set (owner-var editors) is essentially the same as repo committers. This is an accepted, in-code-documented supply-chain tradeoff, not an unmitigated vulnerability, so I'm not flagging it as a finding requiring a fix.
No `sha-`-pattern enforcement exists on the variable, but that's an operational foot-gun (stale `:latest` caching) for the owner only, not a security issue.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
The docker-reviewer PR is correct as a self-contained change. The adversarial-review.yml caller forwards `pr_number` but NOT `reviewer_tag` — so `inputs.reviewer_tag` is empty, falling back to `vars.GADFLY_REVIEWER_TAG` then the baked `sha-b37cd09` fallback. This is the intended priority chain, and the existing behavior (fixed tag) is preserved when no var is set.
Let me verify the `vars` context is accessible inside a reusable workflow.
Verdict: No material issues found
Through the correctness lens, the change from a `uses: docker://…:tag` step to a `container:` job container is sound:
- **Entrypoint contract preserved.** The env block is byte-identical, and `/entrypoint.sh` is invoked explicitly via `run: /entrypoint.sh`. The image's `ENTRYPOINT ["/entrypoint.sh"]` (Dockerfile:60) is effectively ignored when used as a job container (the runner uses the container's shell for `run:` steps), so the explicit invocation is correct and not double-run. Verified Dockerfile:60 and entrypoint.sh:56 (`set -uo pipefail` under `bash`, which the image installs at Dockerfile:30).
- **No working-directory regression.** All paths the entrypoint and run.sh use are absolute or derived from `WORKDIR="${WORKDIR:-/tmp/gadfly}"` (entrypoint.sh:63), not from the container's cwd. The repo is cloned to `${WORKDIR}/repo` (entrypoint.sh:178) and `GADFLY_REPO_DIR` is set explicitly (entrypoint.sh:275), so run.sh's cwd-relative fallback (run.sh:174) never triggers. The `container:` switch changes neither.
- **Priority chain semantics correct.** `image: …:${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }}` — empty strings fall through via `||`, and the baked fallback reproduces the prior pinned tag exactly, so a caller that sets neither (e.g. this repo's own `adversarial-review.yml`, which forwards no `reviewer_tag` — verified at .gitea/workflows/adversarial-review.yml:51-58) gets identical behavior to before.
- **Advisory-only invariant intact.** The step's exit code is still the entrypoint's exit code (non-triggers exit 0; config errors non-zero), unchanged by the `uses:`→`container:` swap.
- **`vars` context in a called workflow** is already relied on in this same file (`vars.GADFLY_DEFAULT_*`, lines 170/171/177), so `vars.GADFLY_REVIEWER_TAG` resolving at runtime is consistent with established behavior.
README and `examples/reusable.yml` updates match the workflow change (verified README.md:404-408, 419; examples/reusable.yml:19-22).
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
VERDICT: No material issues found
Through the code-cleanliness/maintainability lens the change is clean:
- The new `container.image` expression uses the exact `inputs.x || vars.Y || 'fallback'` fallback pattern already established throughout the `env:` block (e.g. line 170 `GADFLY_MODELS: ${{ inputs.models || vars.GADFLY_DEFAULT_MODELS }}`), so it follows the surrounding convention rather than introducing a new idiom.
- The header comments (lines 27-34, 106-121) and the inline input-block comment (line 49, 66) explain *why* the `container:` form was chosen over `uses: docker://` and document the priority chain — this is the kind of non-obvious rationale that aids future maintainers; no dead/duplicated prose.
- The fallback pin `sha-b37cd09` appears in both the comment (line 119) and the image line (line 123); that's intentional explanatory duplication of a magic value (mirrors how the prior `uses:` block documented its sha), not copy-paste drift.
- README's `Central config via variables` table (line 419) and the new structural-vs-routine paragraph (lines 404-408) and `examples/reusable.yml` header (lines 20-22) are all updated consistently per the maintenance rule — no stale doc left behind.
- `examples/reusable.yml` does not thread `reviewer_tag` in its `with:` block, but that's correct: the input defaults to `""` and the user-var `GADFLY_REVIEWER_TAG` is the intended per-fleet mechanism, matching how the example treats the other `GADFLY_DEFAULT_*` knobs (it doesn't forward those either).
Verified by reading `.gitea/workflows/review-reusable.yml`, `examples/reusable.yml`, `README.md`, `entrypoint.sh`, and `Dockerfile` in full.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
Through the Performance lens, this change converts a `uses: docker://` step into a `container:` job container and moves the image tag to a runtime-resolved expression. I verified the surrounding workflow, the Dockerfile entrypoint contract, and `entrypoint.sh` to assess any efficiency impact.
Findings:
- **No material performance regression.** The diff touches only workflow orchestration (`.gitea/workflows/review-reusable.yml`, README, example). It introduces no code on a hot path, no loops, no allocations, no queries, and no repeated work. The expression `${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }}` is evaluated once at job setup — negligible.
- **Image pull behavior is unchanged in substance.** Both `uses: docker://<image>:<tag>` and `container.image` pull the image once per job; the caching concern (`:latest` not re-pulled) is preserved by keeping the resolution on immutable `sha-` tags, which is explicitly documented in the added comments. No new unbounded or repeated pull is introduced.
- **No new per-step overhead.** There is a single step (`run: /entrypoint.sh`); running it inside the job container vs. a `uses:` step container is effectively equivalent — one container lifecycle per job either way. `entrypoint.sh` does its own shallow clone to `/tmp/gadfly/repo` (`git clone --depth=1`) and does not depend on the runner workspace layout, so the container-mode workspace mounting doesn't add extra I/O.
- **Entrypoint contract verified unchanged.** `Dockerfile` sets `ENTRYPOINT ["/entrypoint.sh"]` and the env block forwarded to the step is identical to before; the binary path (`/usr/local/bin/gadfly`) and scripts dir (`/app/scripts`) referenced by `entrypoint.sh` still exist in the image (Dockerfile lines 56-59). No extra filesystem traversal or missing-path retries are introduced.
Verified by reading: `review-reusable.yml` (full), `entrypoint.sh` (full), `Dockerfile` (full), `adversarial-review.yml` (caller), `build-image.yml`, and `examples/reusable.yml`.
VERDICT: No material issues found
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
I've reviewed the diff through the error-handling & edge-cases lens.
VERDICT: No material issues found
The change is a workflow YAML mechanical refactor: swapping a `uses: docker://…` step for a `container:` job with an explicit `run: /entrypoint.sh`. Verified against the actual code:
- **Entrypoint contract intact.** `Dockerfile:58-60` bakes `/entrypoint.sh` and sets `ENTRYPOINT ["/entrypoint.sh"]`. The new `run: /entrypoint.sh` (`.gitea/workflows/review-reusable.yml:126`) invokes the same script the `ENTRYPOINT` would have run under `uses: docker://`, so the bash brains (gating, clone, model loop, consolidation) run identically. No new unhappy path is introduced by the move itself.
- **Env block preserved.** The diff only shows the changed lines, but reading the full file (lines 127-184) confirms the entire `env:` block (event context, forwarded secrets, endpoint vars, config inputs) is unchanged and still attached to the `/entrypoint.sh` step. `entrypoint.sh`'s required-var guards (`: "${GITEA_API:?...}"`, etc., lines 68-71) still fire with the same inputs.
- **Tag-resolution fallback chain.** `${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }}` has a concrete baked fallback, so an unset input + unset var does not produce an empty/invalid image ref — it resolves to the same immutable pin the old `uses:` line used. No empty-image-pull edge case.
- **No swallowed errors / missing cleanup introduced.** The PR doesn't touch `entrypoint.sh` or any script; the advisory-only invariant, watchdog disarm, and board teardown all live unchanged in `entrypoint.sh` (lines 411-426).
No edge-case, error-handling, or cleanup regressions in my lane.
</details>
</details>
<details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — No material issues found</summary>
**Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
No material issues found
* Security analysis: The change moves the reviewer from a `uses: docker://` step to a `container:` job with runtime-resolved image tag. There are no authn/authz gaps, injection vectors, or secret leaks introduced. The `env:` block and secret forwarding are identical to the prior implementation. The `run: /entrypoint.sh` uses an absolute path, avoiding any workspace-path confusion. The new `inputs.reviewer_tag` input allows callers to specify a tag, but callers already control their own workflows; this does not expand attack surface in a reusable `workflow_call` context. The documented trade-off about user/org variables (`GADFLY_REVIEWER_TAG`) being editable without a commit trail is acknowledged inline (`.gitea/workflows/review-reusable.yml:115-117`) and is an accepted operational choice, not a vulnerability. The fallback pin remains hardcoded, ensuring a deterministic default.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**Verdict: No material issues found**
I scrutinized the change for logic bugs, semantic correctness, and expression-evaluation issues. The refactoring from a `uses: docker://…` step to a job-level `container:` + explicit `run: /entrypoint.sh` step is semantically equivalent:
- `entrypoint.sh` is executable, has a bash shebang, and is copied to `/entrypoint.sh` by the Dockerfile (verified at `Dockerfile:58` and `Dockerfile:59`). Running it explicitly via `run:` produces the same behavior as the image's `ENTRYPOINT ["/entrypoint.sh"]`.
- The environment block is still attached to the single step that executes the reviewer, so all variables (`GITEA_API`, `GITEA_TOKEN`, `GADFLY_MODELS`, etc.) reach the script unchanged.
- The tag-resolution expression `${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }}` correctly implements the documented priority chain. `vars.*` is already used throughout this reusable workflow (e.g., `vars.GADFLY_DEFAULT_MODELS`, `vars.GADFLY_ENDPOINT_NETHERSTORM` at lines 156–165 of `review-reusable.yml`), so `vars.GADFLY_REVIEWER_TAG` is consistent with existing patterns.
- The fallback tag `sha-b37cd09` is unchanged from the previous hardcoded pin; no drift in default behavior.
- The `adversarial-review.yml` caller does not pass `reviewer_tag`, which correctly triggers the fallback chain.
No logic errors, incorrect thresholds, broken formulas, or semantic misbehaviors introduced.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
No material issues found
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found
From the **Performance** lens, this change is strictly neutral. It is a workflow YAML restructuring with no impact on runtime efficiency:
- The change moves the reviewer from a step-level `uses: docker://…` to a job-level `container:` with one `run: /entrypoint.sh` step. For a single-step job, container startup and teardown overhead is identical — one image pull, one container lifecycle in both cases.
- The `inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09'` expression is evaluated once at job scheduling time; there is no hot-path re-evaluation, loop, or query introduced.
- The entrypoint contract and env block are unchanged, so all runtime behavior (model execution, lens passes, compaction, token budgets) is identical. No allocations, copies, or blocking patterns are added.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
**No material issues found**
Reviewed through the error-handling & edge-cases lens. The change moves the reviewer from a `uses: docker://…` step to a job-level `container:` + explicit `run: /entrypoint.sh`, with the image tag resolved dynamically via `${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }}`.
- The fallback chain (`input → variable → literal`) correctly handles empty/null values; the baked-in literal ensures the tag is never empty.
- The `entrypoint.sh` contract is unchanged: it still shallow-clones, gates triggers, and exits 0 for non-review events. The env block passed to the step is identical to the old `uses:` step env block.
- No new nil/null dereferences, unbounded iterations, or integer paths are introduced by this diff.
- The step continues to fail hard on missing required env (e.g., `PR`, `GITEA_API`), which is consistent with the existing behavior and is intentional for config errors per the entrypoint comments.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
steve
merged commit d272695c16 into main2026-07-27 03:32:45 +00:00
steve
deleted branch reviewer-tag-runtime-var2026-07-27 03:32:46 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Problem
Nearly every consumer re-pin of
review-reusable.ymlhas been a reviewer image bump, not a structural change (c9dab69,0d51879,463aa01,5007597,84b891b,8f5adc9…). The tag was baked into auses: docker://…:sha-<short>step, and auses:ref is parsed before any expression context exists — it can't read a variable, so each image release meant editing the stub in every consuming repo.Change
Run the reviewer as the job container instead of a
uses: docker://step —container.imagedoes accept expressions — with an explicitrun: /entrypoint.shstep. The tag now resolves per-run, following the same priority chain as the rest of the swarm config:A Gadfly release becomes: build the image → update one user-level variable. Every consumer pinned to this file's
@<sha>picks it up on its next review with no re-pin, riding the same vars-are-injected-per-run mechanism asGADFLY_DEFAULT_*. Workflow re-pins remain only for structural yml changes — this PR is one, intended to be the last routine one.Notes
/entrypoint.shbrains (bash + the entrypoint already live in the image;ENTRYPOINTwas/entrypoint.sh).sha-tags, never:latest(act_runner caches:latestand often does not re-pull a moved one).examples/reusable.ymlupdated per the maintenance rule.container:path end-to-end on the act_runner.🤖 Generated with Claude Code
🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-23 02:50:35Z
claude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneopencode/glm-5.2:cloud· opencode — ✅ doneopencode/kimi-k2.6:cloud· opencode — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 1 inline finding on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -98,13 +103,27 @@ jobs:review:runs-on: ubuntu-latesttimeout-minutes: ${{ inputs.timeout_minutes }}# The reviewer runs as the JOB container (steps exec inside it), not a🟡 Reviewer-tag runtime-resolution priority chain is explained in full prose twice in the same file
maintainability · flagged by 1 model
🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 5 models
Verdict: Minor issues · 1 finding (0 with multi-model agreement)
1 single-model finding (lower confidence)
.gitea/workflows/review-reusable.yml:106Per-model detail
claude-code/sonnet (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
My security review of this PR is complete — verdict: No material issues found. The change moves the reviewer image tag resolution from a commit-baked
uses: docker://...@shato a runtime-resolvedcontainer: image:expression; I verified there's no injection vector, no secret leakage into the image-selection expression, and the one real trust-boundary tradeoff (unaudited var-based image selection) is already explicitly disclosed and accepted in the PR description itself. Since this was a review task with no code to implement, I recorded the outcome in the plan file rather than proposing changes — let me know if you'd like me to actually apply any changes or dig into a different lens (correctness, performance, etc.).🎯 Correctness — No material issues found
VERDICT: No material issues found
I reviewed this PR specifically for logic bugs in the runtime-tag resolution mechanism (the
container.imageexpression, the input/var/fallback precedence chain, and the switch from auses: docker://step to acontainer:+run: /entrypoint.shstep).Verification performed:
.gitea/workflows/review-reusable.ymlpost-diff: thecontainer.imageexpression${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }}follows the exact same||-fallback idiom already used elsewhere in the same file (e.g.GADFLY_MODELS: ${{ inputs.models || vars.GADFLY_DEFAULT_MODELS }}), so the precedence logic is internally consistent.Dockerfile:bashis installed andENTRYPOINT ["/entrypoint.sh"]is chmod'd executable, sorun: /entrypoint.shas a job-container step works regardless of the step's default shell (shvsbash) — the shebang (#!/usr/bin/env bash) is honored by the kernel either way.entrypoint.sh: it is fully env-driven (no positional args), uses a self-managedWORKDIR="/tmp/gadfly"rather than$GITHUB_WORKSPACE, and its process-management (pkill -f/wait) doesn't depend on being container PID 1 — so moving it from the container's own entrypoint invocation to arun:step inside a job container doesn't change its behavior.reviewer_taginput →GADFLY_REVIEWER_TAGvar →sha-b37cd09fallback) matches the actual expression.adversarial-review.yml(gadfly's own dogfood caller) doesn't passreviewer_tag, so it correctly falls through to var/default — no inconsistency there.examples/*.ymlfull-stub files (still usinguses: docker://...:latest) are a separate, pre-existing, unaffected code path — not touched by or in scope of this diff.No logic bugs, no incorrect fallback ordering, no broken cross-file assumption found.
🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
.gitea/workflows/review-reusable.yml:27-34vs:106-121— the reviewer-tag runtime-resolution priority chain (inputs.reviewer_tag → user var GADFLY_REVIEWER_TAG → baked fallback) and the rationale for why per-run resolution matters ("update ONE variable, no consumer re-pin") are spelled out in full prose twice in the same file: the top-of-file header comment (27-31) and the new 16-line comment block directly abovecontainer:(106-121, specifically 106-111). They're worded differently ("resolves at runtime" vs. "resolve per-run") but state the identical priority chain and identical consequence. The per-input annotation at line 66 is a short one-liner consistent with the file's existing convention for other inputs and isn't itself excessive. A future change to the priority chain (e.g. adding an org-level tier) requires updating both prose blocks to stay consistent, and they've already begun to drift in phrasing. The 106-121 block does contain some content not in the header (the container-vs-uses:rationale, the blast-radius/no-audit-trail caveat, and the fallback-pin changelog), so trimming should keep that unique content and only fold the repeated priority-chain sentence into a pointer back to the header.⚡ Performance — No material issues found
VERDICT: No material issues found
The draft is not a review — it's a clarification request (the drafting model got stuck on an apparent Plan Mode conflict and never analyzed the diff, so it produced no bullets and no
gadfly-findingsblock). There are no findings to verify or carry forward.🧯 Error handling & edge cases — No material issues found
Review — Error handling & edge cases lens
VERDICT: No material issues found
This PR is a workflow/doc restructuring (moving the reviewer image from a
uses: docker://…step to acontainer:job image with an explicitrun: /entrypoint.shstep, plus a||fallback chain for the tag). I checked the areas most likely to introduce error-handling regressions:image: gitea.stevedudenhoeffer.com/steve/gadfly:${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }}(.gitea/workflows/review-reusable.yml:123) always resolves to a non-empty tag — the literal fallback guarantees the image ref is never malformed even if both the input and var are unset/empty. This mirrors the pre-existingGADFLY_MODELS: ${{ inputs.models || vars.GADFLY_DEFAULT_MODELS }}pattern already in the same file (line 170), so the||-on-empty-string behavior is an established, already-proven idiom here, not new risk.OLLAMA_CLOUD_API_KEY,ANTHROPIC_API_KEY,CLAUDE_CODE_OAUTH_TOKEN, etc.) and config env var that was attached to the olduses: docker://step is still attached to the newrun:step, unchanged. Nothing silently dropped in the migration.entrypoint.sh:370-394,entrypoint.sh:406-414): the PR-budget watchdog (pkill -f), the lanewaits, and the disarm-marker race guard are all invocation-model-agnostic — they operate on process trees inside the container via bash job control, not on how the container itself was started (uses:vscontainer:+run:). I don't see a code path where switching to job-container execution changes whether those background jobs (status board, per-model reviews, watchdog subshell) get reaped on the happy path or ondie()/non-zero exit.inputdefault is"", consistent with every other optional input in the block (models,specialists, etc.), so an unset input degrades the same way the rest of the swarm config already does.One thing I could not fully verify with static reading (flagging as a caveat, not a finding, since I have no way to confirm act_runner's actual behavior from the repo alone): whether a job
timeout-minutescancellation or container-pull failure under the newcontainer:+docker execexecution model reaps entrypoint.sh's background children identically to the old single-stepdocker://model. The PR description says this path was exercised end-to-end on the act_runner, which covers the happy path; I have no evidence either way for the cancellation/timeout edge case, and didn't find a concrete defect to point at, so I'm not reporting it as a finding.glm-5.2:cloud (ollama-cloud) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
The change's security-relevant surface is the new
reviewer_taginput and thevars.GADFLY_REVIEWER_TAGvariable both flowing intocontainer.imageat.gitea/workflows/review-reusable.yml:123:I checked this through the security lens:
gitea.stevedudenhoeffer.com/steve/gadfly:is a literal in the expression — a consumer settingreviewer_tagviawith:can only control the tag of the owner's existing image, not the registry or image name. There's no path/namespace traversal that lets a caller redirect to an attacker-controlled image (Docker reference syntax doesn't allow a tag to rewrite the repo path). Verified atreview-reusable.yml:123.review-reusable.yml:113-117and the README. The blast radius is "same as editing this file, minus the audit trail," which the PR accepts as in-scope-by-design. No unmasked-secret leak is introduced;GADFLY_REVIEWER_TAGis config, not a credential.env:block (review-reusable.yml:127-184) is unchanged from the priordocker://step form — sameGITEA_TOKEN, same forwarded provider secrets, same endpoint vars. Moving the image from auses: docker://step to the job container does not alter the credential set available to the entrypoint. Verified by reading the full env block.:latestis possible but advisory-only. A consuming repo could passreviewer_tag: "latest", which the comments warn against (act_runner caches:latestand may run stale). This is a reliability/staleness concern, not a privilege escalation, and the review is explicitly advisory ("never blocks a merge"), so I'm not counting it as a security finding.Nothing in the authn/authz, injection, secret-leakage, or untrusted-input lanes rises to a material issue here.
🎯 Correctness — No material issues found
Verdict: No material issues found
I verified the change through the correctness lens and could not confirm a real defect introduced by this diff. Specifics I checked against the checked-out code:
Dockerfile:58-60,entrypoint.sh:1,56): the image'sENTRYPOINT ["/entrypoint.sh"]is exactly what the new step invokes viarun: /entrypoint.sh. The olduses: docker://…step ran the image's ENTRYPOINT; the new path calls the same script explicitly. Theenv:block moved from theuses:step to therun:step — same scope, same vars reach/entrypoint.sh. No semantic drift in what the reviewer executes.review-reusable.yml:123):${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }}mirrors the establishedinputs.x || vars.GADFLY_DEFAULT_Xidiom (e.g. line 170-177). Empty-string is falsy in Gitea's||, so the "" default onreviewer_tagfalls through correctly. The baked fallbacksha-b37cd09is identical to the olddocker://…:sha-b37cd09pin — no stale/drifted pin introduced, and the comment's "PR #27" attribution matches the pre-existing comment.review-reusable.yml:66):reviewer_tagis a new optional string input defaulting to""; existing callers (e.g..gitea/workflows/adversarial-review.yml:51-55,examples/reusable.yml:65) don't pass it and still resolve to the baked pin. No caller breakage.README.md:404-408,419andexamples/reusable.yml:20-22describe the same priority chain and the "empty ⇒ baked fallback" semantics that the YAML implements. The README table row matches the input comment. The full-stub examples (adversarial-review.yml,claude-code.yml, etc.) useuses: docker://directly and are intentionally a different path — correctly untouched by this PR.Unverified verification gap (not a confirmed finding): The change's correctness rests on two
act_runnerbehaviors I could not confirm from this repository — (1) thatjobs.<id>.container.imageactually evaluates${{ }}expressions at run time (the PR's whole premise), and (2) that a job container whose imageENTRYPOINTis a one-shot script (/entrypoint.shthat exits) is kept alive for therun:step rather than the entrypoint firing once on container start and then therun:step double-firing or failing. The PR description states the dogfood run "exercises the newcontainer:path end-to-end on the act_runner," which is the strongest evidence both hold; I could not independently verifyact_runnerinternals here. If that end-to-end run did not actually cover a consumer pinned at@<sha>withGADFLY_REVIEWER_TAGset (vs. just the local fallback), it would be worth confirming both explicitly. I am not confident enough in either being broken to report it as a finding.🧹 Code cleanliness & maintainability — No material issues found
The diff is clean through the maintainability lens. Verified against the checked-out code: the new
container:block follows the sameinput → vars → fallbackresolution pattern used by the surroundingGADFLY_MODELS/GADFLY_SPECIALISTSenv lines, the baked fallback tag matches the prior pin, and the README/examples comments stay consistent with the new mechanism.VERDICT: No material issues found
uses: docker://…:sha-<short>step into a job-container:+ explicitrun: /entrypoint.shstep. Through the maintainability lens this is a net positive: the tag resolution now matches the establishedinputs.x || vars.GADFLY_DEFAULT_Xpattern already used for the config env vars on lines 170–177, eliminating the special-cased per-release edit that the README calls out. No duplicated logic or divergent fallback constant is introduced — thesha-b37cd09fallback appears exactly once (.gitea/workflows/review-reusable.yml:123) after previously living once in theuses:line, so there's no new drift risk.container:block comment at lines 106–121 explains why (expression context availability) rather than what, and the fallback-pin comment preserves the prior PR provenance note. Theon.inputs.reviewer_tagdoc at line 66 mirrors the wording of the other input docs (same "empty falls back to user var …, then …" chain) rather than inventing a new convention.examples/reusable.ymlcomments were updated consistently (verified atREADME.md:404-408,419andexamples/reusable.yml:20-22), so the maintenance rule about keeping the docs in sync with the reusable is honored; no stale references to the olduses:form remain in the touched files.⚡ Performance — No material issues found
Verdict: No material issues found
Through the performance lens, this change is neutral:
uses: docker://…:sha-b37cd09step and the newcontainer.image: …:${{ expr }}resolve to a single immutable image ref per run. act_runner caches by ref either way; the expression only selects which ref, it doesn't force a re-pull of a cached one. A new release tag means one new pull — exactly as before, when each release re-pinned theuses:line.uses:) with one step (run: /entrypoint.sh). Step count and entrypoint invocations are identical (verified atreview-reusable.yml:124-126).actions/checkoutstep before or after — the reviewer operates via the Gitea API, not the mounted workspace — so switching from auses: docker://action to a jobcontainer:adds no clone/mount overhead that wasn't already present.I verified the job body (
review-reusable.yml:102-184) to confirm no additional steps, env-driven repeated work, or checkout were introduced alongside the container switch.🧯 Error handling & edge cases — No material issues found
Verdict: No material issues found
Through the error-handling / edge-case lens, the diff's unhappy paths are handled soundly:
review-reusable.yml:123):${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }}— an empty input, an unset/empty user var, and a missing var all collapse to the bakedsha-fallback.||treats both unset and empty-string as falsy, so there's no "set-but-empty" hole that would resolve to a bare:and fail the pull. Verified at line 66 (default: "") and line 123.run: /entrypoint.shis a literal command with no input interpolation (review-reusable.yml:126), so a malicious or malformedreviewer_tagvalue can't escape into a shell — worst case is a bad image ref that fails the pull with a clear runner error.GADFLY_REVIEWER_TAG(e.g. a non-existentsha-xxxx) fails at container pull before any step runs — a clean, loud failure, not a swallowed error. The step has noif:that would mask it.container:vsuses: docker://parity for the entrypoint: the env block (lines 127–184) is unchanged and identical, and/entrypoint.shis invoked explicitly, so the env-driven contract the entrypoint relies on is preserved. No new nil/empty env var introduced (all inputs default to""and the entrypoint already treats empty as "use image default" per the file's own comments).One thing I considered and could not verify with the available tools (flagging as unverified, not a finding): whether
act_runnerauto-authenticates pulls from the privategitea.stevedudenhoeffer.comregistry for acontainer:image the same way it does foruses: docker://. If it does not, a pull would fail with an auth error. This would be a regression specific to theuses: docker://→container:switch, but it's outside what I can confirm from the repo, and the PR description states the local review already exercises thecontainer:path end-to-end — so I'm treating it as not-material rather than a finding.kimi-k2.6:cloud (ollama-cloud) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
🎯 Correctness — No material issues found
No material issues found
I've carefully traced the semantic transformation through the correctness lens. The change replaces a
uses: docker://…step with a job-levelcontainer:+ explicitrun: /entrypoint.shstep. Here's what I verified:inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09'correctly implements the documented priority chain. An empty-string default onreviewer_tagis falsy in GitHub/Gitea Actions expressions, so it falls through to the variable, then the baked fallback. This was confirmed by reading the full file and checking theinputsblock (.gitea/workflows/review-reusable.yml:66).uses: docker://…ran the image'sENTRYPOINT(/entrypoint.sh) with theenv:block injected. The newcontainer:+run: /entrypoint.shexecutes the same absolute path inside the same container image with the identicalenv:block. The entrypoint contract is preserved (confirmed by PR description and comments at lines 106-123).secrets.*andvars.*references in theenv:block remain on the step and resolve identically in a container job as they did in auses:step. No env variable references were altered or dropped.container.imagetakes a bare registry reference (nodocker://prefix), and the expression interpolation${{ … }}in that field is standard supported syntax. The fallback string'sha-b37cd09'is a valid tag literal.No logic bugs, incorrect fall-through behavior, or semantic breakage introduced.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
The change is clean and consistent across all touched files. The runtime tag resolution chain (
inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09') is applied uniformly in the YAML, README, and example stub. The fallback pinsha-b37cd09is referenced consistently everywhere it appears. Comments are substantive and explain the caching/trust tradeoffs rather than stating the obvious. No dead code, no confusing names, no copy-paste that should be shared, no structural smells.⚡ Performance — No material issues found
No material issues found
Reviewed the change through the performance lens. The move from a
uses: docker://…step to acontainer:job container is architecturally neutral for performance:uses: docker://step in terms of container lifecycle.inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09'evaluates once at job startup — a trivial string fallback chain with no hot-path impact.sha-tags are enforced by convention, not code, but that's outside performance scope).run: /entrypoint.shstep is functionally identical to the priorENTRYPOINT-driven invocation; the same env block is forwarded, and the entrypoint already self-manages cloning and execution.The change successfully centralizes image-tag updates without introducing any measurable efficiency regression.
🧯 Error handling & edge cases — No material issues found
Verdict: No material issues found
The PR cleanly migrates the reviewer execution from a
uses: docker://step to a job-levelcontainer:+ explicitrun: /entrypoint.shstep, with the image tag resolved at runtime via${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }}.Verified via repository read of
review-reusable.yml,entrypoint.sh, andDockerfile:inputs.reviewer_tag(default"", falsy) →vars.GADFLY_REVIEWER_TAG(unset/empty is falsy) → hardcoded'sha-b37cd09'. The||semantics in Gitea Actions expressions correctly skip empty strings and fall through to the baked fallback.ENTRYPOINT ["/entrypoint.sh"]is explicitly invoked by therun:step, so the same bash process runs with the identicalenv:block. No behavioral change in signal handling, process management (pkill, background&/wait), or$WORKDIRusage.entrypoint.shretainsset -uo pipefail, explicit|| true/|| log "..."fallbacks for API calls, anddiefor truly fatal conditions (missingGITEA_API,GITEA_TOKEN,PR,EVENT_NAME, unresolvable branch). Optional envs (IS_DRAFT,COMMENT_BODY,COMMENT_ID, etc.) all use${VAR:-}defaults.reviewer_tag; an invalid tag (whitespace, nonexistent digest) fails at image-pull time with a clear runner error, which is the same failure mode as a bad hardcoded tag in the old approach. No other inputs gain new failure modes.opencode/glm-5.2:cloud (opencode) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Verdict: No material issues found
I reviewed the change through the security lens (authn/authz, injection, supply-chain image integrity, secret handling, input validation of untrusted data).
What I checked:
review-reusable.yml:122-126— the newcontainer.imageexpression${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }}. The registry/repo prefixgitea.stevedudenhoeffer.com/steve/gadfly:is hardcoded; only the tag segment is variable. So a caller or var-editor can select a different tag of the same image, not redirect to an arbitrary registry/image. No shell context is involved (it's a container image ref pulled by the runner, not a command), so there's no command/SQL/path injection surface. Verified by reading the full file.review-reusable.yml:66—reviewer_tagis aworkflow_callinput sourced from the consumer's stub. The consumer repo controls its ownwith:values, so a maliciousreviewer_tagonly affects that consumer's own job (self-harm, not a cross-trust escalation).examples/reusable.ymldoes not forwardreviewer_tag, so consumers inherit the var/fallback chain by default.entrypoint.shandDockerfile:60— theENTRYPOINTis/entrypoint.sh, and the step explicitlyrun: /entrypoint.sh, so moving fromuses: docker://to a job container preserves the same entrypoint contract. No new code-exec surface introduced; the sameenv:block (GITEA_TOKEN, forwarded provider secrets) is passed to the step identically. Verified.permissionsblock (lines 97-100) is unchanged; the container runs with the samecontents:read / issues:write / pull-requests:writeas before.The one security-relevant tradeoff (not a defect): the image tag now mutates via an owner-scope variable with no commit trail, so anyone who can edit owner-level vars can redirect every consumer's reviewer image to a different tag of the gadfly image. The PR explicitly documents this at
review-reusable.yml:113-117("same blast radius as editing this file, but without a commit trail"). The blast radius is bounded — the fixed prefix confines redirection to tags of the same image repo (an attacker still needs registry-push creds to plant a malicious tag), and the trust set (owner-var editors) is essentially the same as repo committers. This is an accepted, in-code-documented supply-chain tradeoff, not an unmitigated vulnerability, so I'm not flagging it as a finding requiring a fix.No
sha--pattern enforcement exists on the variable, but that's an operational foot-gun (stale:latestcaching) for the owner only, not a security issue.🎯 Correctness — No material issues found
The docker-reviewer PR is correct as a self-contained change. The adversarial-review.yml caller forwards
pr_numberbut NOTreviewer_tag— soinputs.reviewer_tagis empty, falling back tovars.GADFLY_REVIEWER_TAGthen the bakedsha-b37cd09fallback. This is the intended priority chain, and the existing behavior (fixed tag) is preserved when no var is set.Let me verify the
varscontext is accessible inside a reusable workflow.Verdict: No material issues found
Through the correctness lens, the change from a
uses: docker://…:tagstep to acontainer:job container is sound:/entrypoint.shis invoked explicitly viarun: /entrypoint.sh. The image'sENTRYPOINT ["/entrypoint.sh"](Dockerfile:60) is effectively ignored when used as a job container (the runner uses the container's shell forrun:steps), so the explicit invocation is correct and not double-run. Verified Dockerfile:60 and entrypoint.sh:56 (set -uo pipefailunderbash, which the image installs at Dockerfile:30).WORKDIR="${WORKDIR:-/tmp/gadfly}"(entrypoint.sh:63), not from the container's cwd. The repo is cloned to${WORKDIR}/repo(entrypoint.sh:178) andGADFLY_REPO_DIRis set explicitly (entrypoint.sh:275), so run.sh's cwd-relative fallback (run.sh:174) never triggers. Thecontainer:switch changes neither.image: …:${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }}— empty strings fall through via||, and the baked fallback reproduces the prior pinned tag exactly, so a caller that sets neither (e.g. this repo's ownadversarial-review.yml, which forwards noreviewer_tag— verified at .gitea/workflows/adversarial-review.yml:51-58) gets identical behavior to before.uses:→container:swap.varscontext in a called workflow is already relied on in this same file (vars.GADFLY_DEFAULT_*, lines 170/171/177), sovars.GADFLY_REVIEWER_TAGresolving at runtime is consistent with established behavior.README and
examples/reusable.ymlupdates match the workflow change (verified README.md:404-408, 419; examples/reusable.yml:19-22).🧹 Code cleanliness & maintainability — No material issues found
VERDICT: No material issues found
Through the code-cleanliness/maintainability lens the change is clean:
container.imageexpression uses the exactinputs.x || vars.Y || 'fallback'fallback pattern already established throughout theenv:block (e.g. line 170GADFLY_MODELS: ${{ inputs.models || vars.GADFLY_DEFAULT_MODELS }}), so it follows the surrounding convention rather than introducing a new idiom.container:form was chosen overuses: docker://and document the priority chain — this is the kind of non-obvious rationale that aids future maintainers; no dead/duplicated prose.sha-b37cd09appears in both the comment (line 119) and the image line (line 123); that's intentional explanatory duplication of a magic value (mirrors how the prioruses:block documented its sha), not copy-paste drift.Central config via variablestable (line 419) and the new structural-vs-routine paragraph (lines 404-408) andexamples/reusable.ymlheader (lines 20-22) are all updated consistently per the maintenance rule — no stale doc left behind.examples/reusable.ymldoes not threadreviewer_tagin itswith:block, but that's correct: the input defaults to""and the user-varGADFLY_REVIEWER_TAGis the intended per-fleet mechanism, matching how the example treats the otherGADFLY_DEFAULT_*knobs (it doesn't forward those either).Verified by reading
.gitea/workflows/review-reusable.yml,examples/reusable.yml,README.md,entrypoint.sh, andDockerfilein full.⚡ Performance — No material issues found
Through the Performance lens, this change converts a
uses: docker://step into acontainer:job container and moves the image tag to a runtime-resolved expression. I verified the surrounding workflow, the Dockerfile entrypoint contract, andentrypoint.shto assess any efficiency impact.Findings:
.gitea/workflows/review-reusable.yml, README, example). It introduces no code on a hot path, no loops, no allocations, no queries, and no repeated work. The expression${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }}is evaluated once at job setup — negligible.uses: docker://<image>:<tag>andcontainer.imagepull the image once per job; the caching concern (:latestnot re-pulled) is preserved by keeping the resolution on immutablesha-tags, which is explicitly documented in the added comments. No new unbounded or repeated pull is introduced.run: /entrypoint.sh); running it inside the job container vs. auses:step container is effectively equivalent — one container lifecycle per job either way.entrypoint.shdoes its own shallow clone to/tmp/gadfly/repo(git clone --depth=1) and does not depend on the runner workspace layout, so the container-mode workspace mounting doesn't add extra I/O.DockerfilesetsENTRYPOINT ["/entrypoint.sh"]and the env block forwarded to the step is identical to before; the binary path (/usr/local/bin/gadfly) and scripts dir (/app/scripts) referenced byentrypoint.shstill exist in the image (Dockerfile lines 56-59). No extra filesystem traversal or missing-path retries are introduced.Verified by reading:
review-reusable.yml(full),entrypoint.sh(full),Dockerfile(full),adversarial-review.yml(caller),build-image.yml, andexamples/reusable.yml.VERDICT: No material issues found
🧯 Error handling & edge cases — No material issues found
I've reviewed the diff through the error-handling & edge-cases lens.
VERDICT: No material issues found
The change is a workflow YAML mechanical refactor: swapping a
uses: docker://…step for acontainer:job with an explicitrun: /entrypoint.sh. Verified against the actual code:Dockerfile:58-60bakes/entrypoint.shand setsENTRYPOINT ["/entrypoint.sh"]. The newrun: /entrypoint.sh(.gitea/workflows/review-reusable.yml:126) invokes the same script theENTRYPOINTwould have run underuses: docker://, so the bash brains (gating, clone, model loop, consolidation) run identically. No new unhappy path is introduced by the move itself.env:block (event context, forwarded secrets, endpoint vars, config inputs) is unchanged and still attached to the/entrypoint.shstep.entrypoint.sh's required-var guards (: "${GITEA_API:?...}", etc., lines 68-71) still fire with the same inputs.${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }}has a concrete baked fallback, so an unset input + unset var does not produce an empty/invalid image ref — it resolves to the same immutable pin the olduses:line used. No empty-image-pull edge case.entrypoint.shor any script; the advisory-only invariant, watchdog disarm, and board teardown all live unchanged inentrypoint.sh(lines 411-426).No edge-case, error-handling, or cleanup regressions in my lane.
opencode/kimi-k2.6:cloud (opencode) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
uses: docker://step to acontainer:job with runtime-resolved image tag. There are no authn/authz gaps, injection vectors, or secret leaks introduced. Theenv:block and secret forwarding are identical to the prior implementation. Therun: /entrypoint.shuses an absolute path, avoiding any workspace-path confusion. The newinputs.reviewer_taginput allows callers to specify a tag, but callers already control their own workflows; this does not expand attack surface in a reusableworkflow_callcontext. The documented trade-off about user/org variables (GADFLY_REVIEWER_TAG) being editable without a commit trail is acknowledged inline (.gitea/workflows/review-reusable.yml:115-117) and is an accepted operational choice, not a vulnerability. The fallback pin remains hardcoded, ensuring a deterministic default.🎯 Correctness — No material issues found
Verdict: No material issues found
I scrutinized the change for logic bugs, semantic correctness, and expression-evaluation issues. The refactoring from a
uses: docker://…step to a job-levelcontainer:+ explicitrun: /entrypoint.shstep is semantically equivalent:entrypoint.shis executable, has a bash shebang, and is copied to/entrypoint.shby the Dockerfile (verified atDockerfile:58andDockerfile:59). Running it explicitly viarun:produces the same behavior as the image'sENTRYPOINT ["/entrypoint.sh"].GITEA_API,GITEA_TOKEN,GADFLY_MODELS, etc.) reach the script unchanged.${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }}correctly implements the documented priority chain.vars.*is already used throughout this reusable workflow (e.g.,vars.GADFLY_DEFAULT_MODELS,vars.GADFLY_ENDPOINT_NETHERSTORMat lines 156–165 ofreview-reusable.yml), sovars.GADFLY_REVIEWER_TAGis consistent with existing patterns.sha-b37cd09is unchanged from the previous hardcoded pin; no drift in default behavior.adversarial-review.ymlcaller does not passreviewer_tag, which correctly triggers the fallback chain.No logic errors, incorrect thresholds, broken formulas, or semantic misbehaviors introduced.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
⚡ Performance — No material issues found
No material issues found
From the Performance lens, this change is strictly neutral. It is a workflow YAML restructuring with no impact on runtime efficiency:
uses: docker://…to a job-levelcontainer:with onerun: /entrypoint.shstep. For a single-step job, container startup and teardown overhead is identical — one image pull, one container lifecycle in both cases.inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09'expression is evaluated once at job scheduling time; there is no hot-path re-evaluation, loop, or query introduced.🧯 Error handling & edge cases — No material issues found
No material issues found
Reviewed through the error-handling & edge-cases lens. The change moves the reviewer from a
uses: docker://…step to a job-levelcontainer:+ explicitrun: /entrypoint.sh, with the image tag resolved dynamically via${{ inputs.reviewer_tag || vars.GADFLY_REVIEWER_TAG || 'sha-b37cd09' }}.input → variable → literal) correctly handles empty/null values; the baked-in literal ensures the tag is never empty.entrypoint.shcontract is unchanged: it still shallow-clones, gates triggers, and exits 0 for non-review events. The env block passed to the step is identical to the olduses:step env block.PR,GITEA_API), which is consistent with the existing behavior and is intentional for config errors per the entrypoint comments.Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.