Recreated from #25 to re-fire the review trigger after fixing the self-review caller (see commit 2).
What
Adds a third review harness alongside the in-process majordomo loop and the claude-code CLI shell-out: the OpenCode CLI (opencode.ai) driving an ollama-cloud model, selected by an opencode/<model> spec.
The point is to benchmark gadfly's boutique executus harness against a freely-available agentic harness on the same model — run ollama-cloud/glm-5.2 (majordomo loop) and opencode/glm-5.2 (OpenCode) side by side and compare findings. This is the wired, no-proxy version of the README's existing "alternate backends" idea.
Commits
feat(engine): add opencode CLI review engine — the feature (below).
fix(dogfood): thread dispatch pr_number into gadfly's own review caller — adversarial-review.yml was never updated for the Gitea ≥ 1.27 breaking change (go-gitea#37478): a called workflow no longer receives the caller's workflow_dispatch inputs in github.event, so a manual dispatch reached the reusable with an empty PR. Commit 64d34bd added the pr_number workflow_call input and mort's caller threads it; this brings gadfly's own caller in line so the dogfood path works on the same Gitea version. (This is why #25 was recreated — the review triggers don't include synchronize, so a fresh opened event was needed against the fixed workflow.)
How the engine works
Read-only via config, not a flag. OpenCode has no --append-system-prompt, so each pass generates an OpenCode config — the lens system prompt as a gadfly agent's prompt with edit/bash denied at both global and agent level, plus a gadfly ollama-cloud provider — and injects it via OPENCODE_CONFIG_CONTENT. That env var is the highest-precedence config source in the container, so a reviewed repo's own opencode.json can't re-enable edits on the reviewer.
Spec forms:opencode/<model> (wrapped in the generated provider), the open-code/ alias, opencode/<provider>/<model> pass-through to OpenCode's own registry/auth, and bare opencode. Model ids are taken verbatim, so colon-bearing ollama ids like qwen3-coder:480b-cloud survive (no :thinking suffix like claude-code).
Auth reuses OLLAMA_CLOUD_API_KEY (mapped to OLLAMA_API_KEY, referenced as {env:OLLAMA_API_KEY} in config — never a literal secret). Knobs mirror GADFLY_CLAUDE_*: GADFLY_OPENCODE_BIN/MODEL/BASE_URL/EXTRA_ARGS. openCodeEnv() forwards OLLAMA_API_KEY (the inverse of claudeEnv) but still withholds the Gitea/findings/Anthropic secrets.
main.go engine selection is now a switch (claude-code / opencode / majordomo); the auto-select path uses a type-check instead of a boolean so a shell-out engine can never hit the *majordomoEngine assertion. auto selection and delegate_investigation stay majordomo-only and are skipped for opencode (the CLI does its own legwork).
Dockerfile — bundles opencode-ai (npm auto-selects its musl build on alpine) with a best-effort version check + provider pre-warm that never fails the shared image build.
README.md, examples/opencode.yml (new), examples/README.md, CLAUDE.md, scripts/run.sh, entrypoint.sh, .gitea/workflows/review-reusable.yml — docs/comments per the maintenance rules.
.gitea/workflows/adversarial-review.yml — the dogfood-caller fix (commit 2).
Verification
go build, go test ./..., go vet, gofmt all clean.
Ran the actual built binary end-to-end with a fake opencode CLI: confirmed main.go routes opencode/… to the new engine, emits the correct argv (run --agent gadfly --model gadfly/qwen3-coder:480b-cloud … <task-last>), injects the expected config (env-ref API key, both-level deny, real system prompt), and consolidates output into review markdown.
Not yet verified (needs the built image / a real run)
Docker image build — needs registry secrets. npm publishes musl builds of opencode-ai (opencode-linux-x64-musl, etc.) that it auto-selects on alpine, so the musl risk is small, but confirm with docker run --entrypoint opencode <img> --version.
A real-model run — that OpenCode's --agent + deny behaves non-interactively as expected and its default text output captures cleanly. Documented honestly as "newly wired, lightly tested" in the README.
To put it on the live fleet: add an opencode/<model> entry to the GADFLY_DEFAULT_MODELS user-scope var (runtime retune, no repo change), pinning consumers to the fresh :sha-<short> since :latest is runner-cached.
_Recreated from #25 to re-fire the review trigger after fixing the self-review caller (see commit 2)._
## What
Adds a third review harness alongside the in-process majordomo loop and the claude-code CLI shell-out: the **OpenCode CLI** ([opencode.ai](https://opencode.ai)) driving an **ollama-cloud** model, selected by an `opencode/<model>` spec.
The point is to **benchmark gadfly's boutique executus harness against a freely-available agentic harness on the same model** — run `ollama-cloud/glm-5.2` (majordomo loop) and `opencode/glm-5.2` (OpenCode) side by side and compare findings. This is the wired, no-proxy version of the README's existing "alternate backends" idea.
## Commits
1. **`feat(engine): add opencode CLI review engine`** — the feature (below).
2. **`fix(dogfood): thread dispatch pr_number into gadfly's own review caller`** — `adversarial-review.yml` was never updated for the Gitea ≥ 1.27 breaking change ([go-gitea#37478](https://github.com/go-gitea/gitea/pull/37478)): a called workflow no longer receives the caller's `workflow_dispatch` inputs in `github.event`, so a manual dispatch reached the reusable with an empty PR. Commit `64d34bd` added the `pr_number` workflow_call input and mort's caller threads it; this brings gadfly's own caller in line so the dogfood path works on the same Gitea version. (This is why #25 was recreated — the review triggers don't include `synchronize`, so a fresh `opened` event was needed against the fixed workflow.)
## How the engine works
- **Read-only via config, not a flag.** OpenCode has no `--append-system-prompt`, so each pass generates an OpenCode config — the lens system prompt as a `gadfly` agent's prompt with `edit`/`bash` denied at both global and agent level, plus a `gadfly` ollama-cloud provider — and injects it via `OPENCODE_CONFIG_CONTENT`. That env var is the highest-precedence config source in the container, so **a reviewed repo's own `opencode.json` can't re-enable edits** on the reviewer.
- **Spec forms:** `opencode/<model>` (wrapped in the generated provider), the `open-code/` alias, `opencode/<provider>/<model>` pass-through to OpenCode's own registry/auth, and bare `opencode`. Model ids are taken **verbatim**, so colon-bearing ollama ids like `qwen3-coder:480b-cloud` survive (no `:thinking` suffix like claude-code).
- **Auth reuses `OLLAMA_CLOUD_API_KEY`** (mapped to `OLLAMA_API_KEY`, referenced as `{env:OLLAMA_API_KEY}` in config — never a literal secret). Knobs mirror `GADFLY_CLAUDE_*`: `GADFLY_OPENCODE_BIN/MODEL/BASE_URL/EXTRA_ARGS`. `openCodeEnv()` forwards `OLLAMA_API_KEY` (the inverse of `claudeEnv`) but still withholds the Gitea/findings/Anthropic secrets.
- `main.go` engine selection is now a switch (claude-code / opencode / majordomo); the auto-select path uses a type-check instead of a boolean so a shell-out engine can never hit the `*majordomoEngine` assertion. `auto` selection and `delegate_investigation` stay majordomo-only and are skipped for opencode (the CLI does its own legwork).
## Files
- **`cmd/gadfly/opencode.go`** (new) — the engine.
- **`cmd/gadfly/opencode_test.go`** (new) — 12 tests mirroring `engine_test.go`.
- **`cmd/gadfly/main.go`**, **`engine.go`** — engine selection switch + doc.
- **`Dockerfile`** — bundles `opencode-ai` (npm auto-selects its musl build on alpine) with a best-effort version check + provider pre-warm that never fails the shared image build.
- **`README.md`**, **`examples/opencode.yml`** (new), **`examples/README.md`**, **`CLAUDE.md`**, **`scripts/run.sh`**, **`entrypoint.sh`**, **`.gitea/workflows/review-reusable.yml`** — docs/comments per the maintenance rules.
- **`.gitea/workflows/adversarial-review.yml`** — the dogfood-caller fix (commit 2).
## Verification
- `go build`, `go test ./...`, `go vet`, `gofmt` all clean.
- Ran the **actual built binary** end-to-end with a fake `opencode` CLI: confirmed `main.go` routes `opencode/…` to the new engine, emits the correct argv (`run --agent gadfly --model gadfly/qwen3-coder:480b-cloud … <task-last>`), injects the expected config (env-ref API key, both-level deny, real system prompt), and consolidates output into review markdown.
### Not yet verified (needs the built image / a real run)
- **Docker image build** — needs registry secrets. npm publishes musl builds of `opencode-ai` (`opencode-linux-x64-musl`, etc.) that it auto-selects on alpine, so the musl risk is small, but confirm with `docker run --entrypoint opencode <img> --version`.
- **A real-model run** — that OpenCode's `--agent` + deny behaves non-interactively as expected and its default text output captures cleanly. Documented honestly as "newly wired, lightly tested" in the README.
To put it on the live fleet: add an `opencode/<model>` entry to the `GADFLY_DEFAULT_MODELS` user-scope var (runtime retune, no repo change), pinning consumers to the fresh `:sha-<short>` since `:latest` is runner-cached.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Add a third review harness alongside the in-process majordomo loop and the
claude-code CLI shell-out: the OpenCode CLI (opencode.ai) driving an ollama-cloud
model, selected by an "opencode/<model>" spec. The goal is to benchmark gadfly's
boutique executus harness against a freely-available agentic harness on the SAME
model (e.g. "ollama-cloud/glm-5.2" vs "opencode/glm-5.2").
OpenCode has no --append-system-prompt flag, so the lens system prompt and the
read-only discipline are delivered through a generated config injected via
OPENCODE_CONFIG_CONTENT: a "gadfly" agent whose prompt is the system prompt with
edit/bash denied at both the global and agent level, plus a "gadfly" ollama-cloud
provider. That env var is the highest-precedence config source in the container,
so a reviewed repo's own opencode.json can't re-enable edits on the reviewer.
Spec forms: "opencode/<model>" (wrapped in the generated provider), the
"open-code/" alias, "opencode/<provider>/<model>" pass-through to OpenCode's own
registry, and bare "opencode". Model ids are taken verbatim so colon-bearing
ollama ids (qwen3-coder:480b-cloud) survive. Auth reuses OLLAMA_CLOUD_API_KEY
(mapped to OLLAMA_API_KEY, referenced as {env:OLLAMA_API_KEY} in config, never a
literal secret). Knobs mirror GADFLY_CLAUDE_*: GADFLY_OPENCODE_BIN/MODEL/BASE_URL/
EXTRA_ARGS. openCodeEnv() forwards OLLAMA_API_KEY (the inverse of claudeEnv) but
still withholds the Gitea/findings/Anthropic secrets.
main.go engine selection is now a switch (claude-code / opencode / majordomo), and
the auto-select path uses a type-check instead of a boolean so a shell-out engine
can never hit the *majordomoEngine assertion. auto-select and delegate_investigation
stay majordomo-only and are skipped for opencode (the CLI does its own legwork).
Dockerfile bundles opencode-ai (npm auto-selects its musl build on alpine) with a
best-effort version check + provider pre-warm that never fails the shared image
build. README/examples/CLAUDE.md/scripts updated per the maintenance rules.
Tests: new opencode_test.go mirrors engine_test.go (spec/model/args/config/env-
filter + stub-CLI runtime tests). Verified end-to-end with a fake opencode CLI:
correct argv, injected config, and consolidated markdown output.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
gadfly's self-review adversarial-review.yml calls review-reusable.yml but was
never updated for the Gitea >= 1.27 breaking change (go-gitea#37478): a called
workflow no longer receives the caller's workflow_dispatch inputs in
github.event, so a manual "review PR #N" dispatch reached the reusable with an
empty PR. Commit 64d34bd added the pr_number workflow_call input and mort's
caller threads it; this brings gadfly's own caller in line so the dogfood path
works on the same Gitea version.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
The self-review kept skipping every PR with "event 'workflow_call' not handled":
review-reusable.yml pinned the reviewer image at sha-3095ebf, which predates the
entrypoint.sh reclassification (added in 9d74cb9) that maps a called workflow's
github.event_name = 'workflow_call' back to pull_request/issue_comment. Under
Gitea >= 1.27 the container therefore saw an unhandled event and self-skipped in
~1s, even though PR was populated.
Bump the pin to sha-f468fe6 (current main HEAD, which contains the fix) so the
dogfood path actually reviews. The pr_number threading in the prior commit fixes
the manual-dispatch input propagation; this fixes the pull_request path.
Co-Authored-By: Claude Opus 4.8 (1M context) <[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
1/1 reviewers finished · updated 2026-07-18 06:31:04Z
#### `opencode/glm-5.2:cloud` · opencode — ✅ done
- ✅ **security** — Minor issues
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
🪰Gadfly consensus review — 8 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
<!-- gadfly-inline-review -->
🪰 **Gadfly consensus review** — 8 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
⚪const block groups unrelated constants; doc comment visually misattaches to wrong const
maintainability · flagged by 1 model
cmd/gadfly/opencode.go:38-46 — const block groups unrelated constants; doc comment visually misattaches to the wrong const. The single const block lumps the paired name constants (openCodeProviderName/openCodeAgentName, both "gadfly") together with the unrelated defaultOpenCodeBaseURL. Because openCodeAgentName = "gadfly" is shorter than the others, gofmt aligns it under a different column, so the // defaultOpenCodeBaseURL is ollama-cloud's… comment on line 44 reads as if i…
🪰 Gadfly · advisory
⚪ **const block groups unrelated constants; doc comment visually misattaches to wrong const**
_maintainability · flagged by 1 model_
- **`cmd/gadfly/opencode.go:38-46` — const block groups unrelated constants; doc comment visually misattaches to the wrong const.** The single `const` block lumps the paired name constants (`openCodeProviderName`/`openCodeAgentName`, both `"gadfly"`) together with the unrelated `defaultOpenCodeBaseURL`. Because `openCodeAgentName = "gadfly"` is shorter than the others, gofmt aligns it under a different column, so the `// defaultOpenCodeBaseURL is ollama-cloud's…` comment on line 44 reads as if i…
<sub>🪰 Gadfly · advisory</sub>
🟠Read-only enforcement denies only edit/bash; other OpenCode permission categories left at CLI default
security · flagged by 1 model
cmd/gadfly/opencode.go:155 — read-only enforcement denies only edit and bash; other OpenCode permission/tool categories are left at the CLI default.openCodePermission (opencode.go:125-128) hardcodes exactly two fields, and config() sets Edit:"deny", Bash:"deny" at both the global and agent level (opencode.go:156-167). The claude-code engine achieves read-only via --permission-mode plan (engine.go:64,111); the opencode engine's equivalence rests on the assumption that OpenCod…
🪰 Gadfly · advisory
🟠 **Read-only enforcement denies only edit/bash; other OpenCode permission categories left at CLI default**
_security · flagged by 1 model_
- **`cmd/gadfly/opencode.go:155` — read-only enforcement denies only `edit` and `bash`; other OpenCode permission/tool categories are left at the CLI default.** `openCodePermission` (opencode.go:125-128) hardcodes exactly two fields, and `config()` sets `Edit:"deny", Bash:"deny"` at both the global and agent level (opencode.go:156-167). The claude-code engine achieves read-only via `--permission-mode plan` (engine.go:64,111); the opencode engine's equivalence rests on the assumption that OpenCod…
<sub>🪰 Gadfly · advisory</sub>
cmd/gadfly/opencode.go:185 / cmd/gadfly/engine.go:155 — The runPass implementations for claudeCodeEngine and openCodeEngine duplicate ~40 lines of identical shell-out orchestration: Setpgid + SIGKILL process-group cleanup, bytes.Buffer stdout/stderr capture, the ctx.Err() cancellation guard, and the truncateForErr fallback chain. Any fix to timeout handling or orphan-process cleanup must be edited in two places. Suggested fix: Extract a shared helper such as `runShe…
🪰 Gadfly · advisory
🟡 **runPass duplicates claudeCodeEngine shell-out orchestration; extract shared helper**
_maintainability · flagged by 1 model_
- **`cmd/gadfly/opencode.go:185` / `cmd/gadfly/engine.go:155`** — The `runPass` implementations for `claudeCodeEngine` and `openCodeEngine` duplicate ~40 lines of identical shell-out orchestration: `Setpgid` + `SIGKILL` process-group cleanup, `bytes.Buffer` stdout/stderr capture, the `ctx.Err()` cancellation guard, and the `truncateForErr` fallback chain. Any fix to timeout handling or orphan-process cleanup must be edited in two places. **Suggested fix:** Extract a shared helper such as `runShe…
<sub>🪰 Gadfly · advisory</sub>
🟡OPENCODE_CONFIG_CONTENT outranking repo opencode.json is an external CLI property, not enforced in code; repo could re-enable edits if precedence changes
maintainability, security · flagged by 1 model
cmd/gadfly/opencode.go:196 — OPENCODE_CONFIG_CONTENT outranking a repo opencode.json is an external CLI property, not enforced in code. The comments (opencode.go:26-28, :194) and README claim OPENCODE_CONFIG_CONTENT "outranks any opencode.json a reviewed repo ships," which is the load-bearing security guarantee that a repo can't re-enable edits. That precedence is a property of the external opencode CLI, not anything this code enforces — runPass only sets the env var (opencode.…
🪰 Gadfly · advisory
🟡 **OPENCODE_CONFIG_CONTENT outranking repo opencode.json is an external CLI property, not enforced in code; repo could re-enable edits if precedence changes**
_maintainability, security · flagged by 1 model_
- **`cmd/gadfly/opencode.go:196` — `OPENCODE_CONFIG_CONTENT` outranking a repo `opencode.json` is an external CLI property, not enforced in code.** The comments (opencode.go:26-28, :194) and README claim `OPENCODE_CONFIG_CONTENT` "outranks any opencode.json a reviewed repo ships," which is the load-bearing security guarantee that a repo can't re-enable edits. That precedence is a property of the external `opencode` CLI, not anything this code enforces — `runPass` only sets the env var (opencode.…
<sub>🪰 Gadfly · advisory</sub>
🟡Process-group setup/kill-on-cancel block copy-pasted from claudeCodeEngine.runPass
maintainability · flagged by 1 model
cmd/gadfly/opencode.go:200-206 copy-pastes the process-group setup/kill logic from claudeCodeEngine.runPass (cmd/gadfly/engine.go:166-172) verbatim, down to the matching 3-line comment (only "claude" → "opencode" changed in the comment text). Verified by reading both runPass implementations. Since both shell-out engines need identical "own process group + SIGKILL the group on cancel" behavior, this is a good candidate for a one-line shared helper, e.g. `killProcessGroupOnCancel(cmd *ex…
🪰 Gadfly · advisory
🟡 **Process-group setup/kill-on-cancel block copy-pasted from claudeCodeEngine.runPass**
_maintainability · flagged by 1 model_
- `cmd/gadfly/opencode.go:200-206` copy-pastes the process-group setup/kill logic from `claudeCodeEngine.runPass` (`cmd/gadfly/engine.go:166-172`) verbatim, down to the matching 3-line comment (only "claude" → "opencode" changed in the comment text). Verified by reading both `runPass` implementations. Since both shell-out engines need identical "own process group + SIGKILL the group on cancel" behavior, this is a good candidate for a one-line shared helper, e.g. `killProcessGroupOnCancel(cmd *ex…
<sub>🪰 Gadfly · advisory</sub>
🟡Raw opencode CLI stderr/stdout (possibly containing credential/auth error detail) is truncated but surfaced verbatim into the publicly-posted review comment
security · flagged by 1 model
cmd/gadfly/opencode.go:219 — runPass forwards up to 800 chars (truncateForErr, engine.go:252-258) of the opencode subprocess's raw stderr/stdout into the returned error when the CLI fails. Confirmed this error string flows unmodified into the publicly-posted PR comment: main.go:340 renders it verbatim as "⚠️ This reviewer failed to complete: %v". If the opencode CLI or the underlying ollama-cloud HTTP client ever echoes auth-failure detail containing request fragments (e.…
🪰 Gadfly · advisory
🟡 **Raw opencode CLI stderr/stdout (possibly containing credential/auth error detail) is truncated but surfaced verbatim into the publicly-posted review comment**
_security · flagged by 1 model_
- **`cmd/gadfly/opencode.go:219`** — `runPass` forwards up to 800 chars (`truncateForErr`, `engine.go:252-258`) of the opencode subprocess's raw `stderr`/`stdout` into the returned error when the CLI fails. Confirmed this error string flows unmodified into the publicly-posted PR comment: `main.go:340` renders it verbatim as `"⚠️ This reviewer failed to complete: %v"`. If the `opencode` CLI or the underlying ollama-cloud HTTP client ever echoes auth-failure detail containing request fragments (e.…
<sub>🪰 Gadfly · advisory</sub>
🟠openCodeEnv() strips provider API keys (e.g. ANTHROPIC_API_KEY), breaking the documented opencode// pass-through escape hatch
error-handling, maintainability · flagged by 3 models
Documented opencode/<provider>/<model> escape hatch can't actually authenticate for its own example. README.md:160 documents opencode/anthropic/claude-sonnet-4-6 as a working pass-through that uses "OpenCode's own provider registry/auth." But openCodeEnv() (cmd/gadfly/opencode.go:243-264) only keeps PATH/HOME/USER/LOGNAME/TMPDIR/LANG/TERM/SHELL/OLLAMA_API_KEY plus LC_/OPENCODE_/GADFLY_OPENCODE_/NODE_/XDG_ prefixes — ANTHROPIC_API_KEY (and every other provider key) is stripped f…
🪰 Gadfly · advisory
🟠 **openCodeEnv() strips provider API keys (e.g. ANTHROPIC_API_KEY), breaking the documented opencode/<provider>/<model> pass-through escape hatch**
_error-handling, maintainability · flagged by 3 models_
- **Documented `opencode/<provider>/<model>` escape hatch can't actually authenticate for its own example.** README.md:160 documents `opencode/anthropic/claude-sonnet-4-6` as a working pass-through that uses "OpenCode's own provider registry/auth." But `openCodeEnv()` (cmd/gadfly/opencode.go:243-264) only keeps `PATH/HOME/USER/LOGNAME/TMPDIR/LANG/TERM/SHELL/OLLAMA_API_KEY` plus `LC_/OPENCODE_/GADFLY_OPENCODE_/NODE_/XDG_` prefixes — `ANTHROPIC_API_KEY` (and every other provider key) is stripped f…
<sub>🪰 Gadfly · advisory</sub>
🔴openCodeEnv drops ANTHROPIC_API_KEY/OPENAI_API_KEY, breaking the documented opencode// pass-through escape hatch (e.g. opencode/anthropic/...) which relies on OpenCode's built-in providers for auth
correctness · flagged by 2 models
🪰 Gadfly · advisory
🔴 **openCodeEnv drops ANTHROPIC_API_KEY/OPENAI_API_KEY, breaking the documented opencode/<provider>/<model> pass-through escape hatch (e.g. opencode/anthropic/...) which relies on OpenCode's built-in providers for auth**
_correctness · flagged by 2 models_
<sub>🪰 Gadfly · advisory</sub>
Verdict: Blocking issues found · 9 findings (2 with multi-model agreement)
Finding
Where
Models
Lens
🟠
openCodeEnv() strips provider API keys (e.g. ANTHROPIC_API_KEY), breaking the documented opencode// pass-through escape hatch
cmd/gadfly/opencode.go:243
3/3
error-handling, maintainability
🔴
openCodeEnv drops ANTHROPIC_API_KEY/OPENAI_API_KEY, breaking the documented opencode// pass-through escape hatch (e.g. opencode/anthropic/...) which relies on OpenCode's built-in providers for auth
cmd/gadfly/opencode.go:249
2/3
correctness
7 single-model findings (lower confidence)
Finding
Where
Model
Lens
🟠
Read-only enforcement denies only edit/bash; other OpenCode permission categories left at CLI default
OPENCODE_CONFIG_CONTENT outranking repo opencode.json is an external CLI property, not enforced in code; repo could re-enable edits if precedence changes
cmd/gadfly/opencode.go:196
glm-5.2:cloud
maintainability, security
🟡
Process-group setup/kill-on-cancel block copy-pasted from claudeCodeEngine.runPass
cmd/gadfly/opencode.go:200
claude-code/sonnet
maintainability
🟡
Raw opencode CLI stderr/stdout (possibly containing credential/auth error detail) is truncated but surfaced verbatim into the publicly-posted review comment
cmd/gadfly/opencode.go:219
claude-code/sonnet
security
🟡
pre-flight Ollama key check only matches GADFLY_PROVIDER_EFF=ollama-cloud, so a missing OLLAMA_CLOUD_API_KEY for an opencode/ spec skips the friendly warning and falls through to a generic failure
scripts/run.sh:167
claude-code/sonnet
correctness
⚪
const block groups unrelated constants; doc comment visually misattaches to wrong const
cmd/gadfly/opencode.go:38
glm-5.2:cloud
maintainability
Per-model detailclaude-code/sonnet (claude-code) — Minor issues
Confirmed: .gadfly.yml's fileConfig only has Specialists/Define fields — no path to GADFLY_OPENCODE_* or GADFLY_MODEL. This matches the draft's "no repo-controlled config injection" claim.
All the "verified as sound" claims and finding 1 check out against the actual code. Finding 2 is explicitly speculative about an external CLI's undocumented tool inventory that can't be confirmed from this repo, so it's dropped per the verification gate's confirm-or-drop rule.
No injection vector: args() builds an exec.CommandContext argv slice (no shell), and config() marshals via encoding/json, so the system prompt / task text can't break out of the JSON config or the argv (opencode.go:107-114, 155-183).
API key never literal: Options.APIKey is always the string "{env:OLLAMA_API_KEY}" (opencode.go:176), confirmed by TestOpenCodeConfig.
Env allowlist is deny-by-default: openCodeEnv() (opencode.go:243-265) only forwards a fixed set + prefixes; GITEA_TOKEN, GADFLY_API_KEY, GADFLY_FINDINGS_TOKEN, ANTHROPIC_API_KEY, CLAUDE_CODE_OAUTH_TOKEN are excluded.
OPENCODE_CONFIG_CONTENT can't be double-set: explicitly filtered out of the inherited env (opencode.go:245-247) before being appended fresh (opencode.go:196).
No repo-controlled config injection: .gadfly.yml's fileConfig (specialists.go:69-77) only has Specialists/Define fields — it cannot set GADFLY_OPENCODE_BASE_URL/GADFLY_OPENCODE_EXTRA_ARGS/GADFLY_MODEL, so a malicious PR can't redirect the subprocess's model endpoint or smuggle extra CLI flags via repo content.
Findings:
cmd/gadfly/opencode.go:219 — runPass forwards up to 800 chars (truncateForErr, engine.go:252-258) of the opencode subprocess's raw stderr/stdout into the returned error when the CLI fails. Confirmed this error string flows unmodified into the publicly-posted PR comment: main.go:340 renders it verbatim as "⚠️ This reviewer failed to complete: %v". If the opencode CLI or the underlying ollama-cloud HTTP client ever echoes auth-failure detail containing request fragments (e.g. an "invalid key: sk-…" message or a URL with embedded query state), that text becomes part of a public comment. This mirrors the pre-existing claudeCodeEngine.runPass pattern (engine.go:200-212), so it isn't a new failure class, but it is a second credential (OLLAMA_CLOUD_API_KEY, mapped to OLLAMA_API_KEY per scripts/run.sh:158-161) now flowing through the same raw-error-to-public-comment path via a new shell-out CLI. Low confidence — cannot confirm what opencode run actually prints on an auth failure without executing the real binary.
🎯 Correctness — Minor issues
Both findings verified directly against the code — the whitelist in openCodeEnv(), the conditional provider block in config(), the absence of any opencode auth login/credential-persistence step in the Dockerfile/entrypoint.sh, the TestOpenCodeEnvFilters assertions, and the run.sh provider-derivation/pre-flight-check logic all match the draft's claims exactly.
VERDICT: Minor issues
cmd/gadfly/opencode.go:243-256 (openCodeEnv) contradicts the feature it's shipped to support. The README (README.md:160) and the code's own doc comment (opencode.go:68-69) document opencode/<provider>/<model> as an "escape hatch" that passes through to "OpenCode's own provider registry/auth," and bare opencode as using "OpenCode's configured default model." But openCodeEnv()'s whitelist (lines 244-256) keeps only PATH/HOME/USER/LOGNAME/TMPDIR/LANG/TERM/SHELL/OLLAMA_API_KEY plus LC_*/OPENCODE_*/GADFLY_OPENCODE_*/NODE_*/XDG_* prefixes — excluding every other provider credential (confirmed by TestOpenCodeEnvFilters in opencode_test.go:197-227, which asserts ANTHROPIC_API_KEY/CLAUDE_CODE_OAUTH_TOKEN are dropped). Nothing in the Dockerfile or entrypoint.sh persists alternate-provider credentials into an OpenCode on-disk auth store (no opencode auth login step exists anywhere in the repo). config() (opencode.go:169-181) also only emits a Provider block when e.providerModel != "", which is never true for the pass-through or bare forms (newOpenCodeEngine, opencode.go:88-98). So neither the env nor the generated config gives OpenCode any credential for a non-Ollama provider — the pass-through and bare spec forms have no working auth path and will fail at runtime for anything but the opencode/<ollama-model> form. Doesn't affect the PR's primary use case (benchmarking opencode/glm-5.2 vs ollama-cloud/glm-5.2), only the two secondary spec forms documented in the README table.
scripts/run.sh:167 the pre-flight "no key configured" guard ([ "$GADFLY_PROVIDER_EFF" = "ollama-cloud" ] && ...) only fires when GADFLY_PROVIDER_EFF equals ollama-cloud. For an opencode/<model> spec, MODEL_PROVIDER (line 70-73, case "$MODEL" in */*) MODEL_PROVIDER="${MODEL%%/*}") becomes "opencode", and GADFLY_PROVIDER_EFF="$MODEL_PROVIDER" (line 163) inherits that — so this check never fires even though the opencode engine also depends on OLLAMA_CLOUD_API_KEY/OLLAMA_API_KEY (per this PR's own comment at run.sh:32-37). A missing key falls through to the generic "Agentic reviewer … failed (exit …)" path (line 193-198) instead of the specific, friendlier message. Still advisory-only — no invariant broken — just a less helpful failure surface than the one this PR documents as reused for opencode. (Note: the bare opencode spec, with no slash, actually falls into the *) case and gets MODEL_PROVIDER="${GADFLY_PROVIDER:-ollama-cloud}", so the gap is specific to the opencode/<model> slash form.)
🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
cmd/gadfly/opencode.go:243-265 (openCodeEnv) duplicates the entire filter/loop structure of claudeEnv at cmd/gadfly/engine.go:228-248 almost verbatim — same keep(k) switch-plus-prefix pattern and the identical for _, kv := range os.Environ() { if k, _, ok := strings.Cut(kv, "="); ok && keep(k) { env = append(env, kv) } }; return env body. Verified by reading both functions side by side. The PR already extracts shared helpers for other repeated bits (envOr, truncateForErr in engine.go), so the precedent for sharing exists — this one block was copy-pasted instead. A small filterEnv(keep func(string) bool) []string in engine.go, called from both claudeEnv and openCodeEnv with their own keep closures, removes the duplication with no behavior change.
cmd/gadfly/opencode.go:200-206 copy-pastes the process-group setup/kill logic from claudeCodeEngine.runPass (cmd/gadfly/engine.go:166-172) verbatim, down to the matching 3-line comment (only "claude" → "opencode" changed in the comment text). Verified by reading both runPass implementations. Since both shell-out engines need identical "own process group + SIGKILL the group on cancel" behavior, this is a good candidate for a one-line shared helper, e.g. killProcessGroupOnCancel(cmd *exec.Cmd) in engine.go, called from both engines' runPass. Low risk today, but a future fix to the kill semantics (e.g. switching to SIGTERM-then-SIGKILL) would otherwise have to be made in two places and could easily be applied to only one.
Both are small, mechanical, low-churn duplications rather than anything structurally wrong — the rest of the new opencode.go/opencode_test.go follows the existing claude-code engine's shape well and I found nothing else in scope for this lens.
⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed through the performance lens (allocations, hot-loop work, N+1, blocking calls, missing concurrency bounds).
What I checked:
cmd/gadfly/opencode.go — runPass shells out once per lens/pass (same shape as the existing claudeCodeEngine), buffers stdout/stderr in-memory via bytes.Buffer (fine for review-sized output), and openCodeEnv() does a single O(env vars) scan per invocation — not a hot loop, called at most once per pass.
Concurrency: verified cmd/gadfly/main.go:245-308 (fanout.Run + lensConcurrency(), default 1) governs specialist parallelism uniformly for all three engines — the new opencode engine doesn't bypass or add its own concurrency path.
Provider lane capping: confirmed entrypoint.sh:240 (provider_of()) derives the provider from the text before the first /, so opencode/<model> entries correctly get their own GADFLY_PROVIDER_CONCURRENCY lane like any other provider — no unbounded parallel opencode process spawning introduced.
Dockerfile warm-up step (timeout 120 opencode run …) runs once at image-build time, not per review — not a runtime regression, and it's ; true-guarded so it can't hang the build indefinitely beyond the 120s timeout.
config()/JSON marshaling in opencode.go:155-183 builds a small, fixed-size struct per pass — no scaling concern.
Nothing in this diff introduces N+1 behavior, unbounded growth, missing limits, or blocking work on a path that runs more than once per lens/pass. The change is architecturally parallel to the existing claude-code engine and reuses its existing concurrency/lane controls.
🧯 Error handling & edge cases — Minor issues
The finding is fully confirmed against the actual code and docs.
Review — Error handling & edge cases lens
VERDICT: Minor issues
Documented opencode/<provider>/<model> escape hatch can't actually authenticate for its own example. README.md:160 documents opencode/anthropic/claude-sonnet-4-6 as a working pass-through that uses "OpenCode's own provider registry/auth." But openCodeEnv() (cmd/gadfly/opencode.go:243-264) only keeps PATH/HOME/USER/LOGNAME/TMPDIR/LANG/TERM/SHELL/OLLAMA_API_KEY plus LC_/OPENCODE_/GADFLY_OPENCODE_/NODE_/XDG_ prefixes — ANTHROPIC_API_KEY (and every other provider key) is stripped from the subprocess environment before opencode run is invoked. The README itself confirms this a few lines later ("the Gitea token, Anthropic/Claude keys, and findings token aren't handed to the CLI," README.md:184). There is also no opencode auth login or baked-in credential setup anywhere in the Dockerfile/entrypoint.sh/run.sh. So the one concrete example given for this spec form has no way to authenticate through gadfly's own plumbing: a user who follows the doc and sets GADFLY_MODELS=opencode/anthropic/claude-sonnet-4-6 (with ANTHROPIC_API_KEY set as a secret, expecting it to reach the CLI) gets a provider-auth failure from opencode, surfaced only as the generic "opencode run failed: %v: %s" (opencode.go:218-226) with no code path distinguishing "pass-through provider has no forwarded credential" from any other CLI error. This is an unhandled edge case for a spec form the diff explicitly introduces and documents as supported.
Suggested fix: either forward the specific provider's standard env var(s) when modelRef doesn't start with openCodeProviderName+"/" (true pass-through), or drop the anthropic/... example from the docs and state plainly that the pass-through form only works for providers OpenCode can already authenticate without gadfly's help.
glm-5.2:cloud (ollama-cloud) — Blocking issues found
Both findings concern behavior of the external opencode CLI that isn't vendored in this repo, but each rests on a verifiable code-level fact: openCodePermission only defines Edit/Bash (opencode.go:125-128) and config() sets exactly those two denies (opencode.go:155-168); and precedence over a repo opencode.json is not enforced anywhere in code — only asserted in comments (opencode.go:26-28, :194) and set via the env var (opencode.go:196). I confirmed both code facts. The draft correctly hedges these as defense-in-depth caveats, not hard defects. The third (operator GADFLY_OPENCODE_BIN verbatim) was explicitly "no action required" and not in the findings JSON, so it's correctly omitted.
Verdict: Minor issues
cmd/gadfly/opencode.go:155 — read-only enforcement denies only edit and bash; other OpenCode permission/tool categories are left at the CLI default.openCodePermission (opencode.go:125-128) hardcodes exactly two fields, and config() sets Edit:"deny", Bash:"deny" at both the global and agent level (opencode.go:156-167). The claude-code engine achieves read-only via --permission-mode plan (engine.go:64,111); the opencode engine's equivalence rests on the assumption that OpenCode's permission schema has only edit/bash categories. If OpenCode exposes any other actionable permission (web/fetch, MCP tools, webfetch, arbitrary tool execution), it is not denied and runs at the CLI's default for the gadfly agent — a prompt-injected/malicious review could exfiltrate via network or invoke repo-shipped MCP tools. The OpenCode source isn't vendored here, so this is medium-confidence; the fix is to confirm the full permission enum and deny every non-read category (or use an equivalent strict catch-all), not just edit/bash.
cmd/gadfly/opencode.go:196 — OPENCODE_CONFIG_CONTENT outranking a repo opencode.json is an external CLI property, not enforced in code. The comments (opencode.go:26-28, :194) and README claim OPENCODE_CONFIG_CONTENT "outranks any opencode.json a reviewed repo ships," which is the load-bearing security guarantee that a repo can't re-enable edits. That precedence is a property of the external opencode CLI, not anything this code enforces — runPass only sets the env var (opencode.go:196). If a future OpenCode build changes precedence to merge repo config on top (or honors a repo opencode.json that re-enables a permission), the deny can be undone with no code change here. Worth a defense-in-depth mitigation (e.g. removing/neutralizing any repo-root opencode.json/.opencode/ before invoking) or documenting the dependency as a pinned CLI version rather than an invariant.
🎯 Correctness — Blocking issues found
VERDICT: Blocking issues found
cmd/gadfly/opencode.go:243-265 — pass-through spec form is broken: its auth env is withheld. The opencode/<provider>/<model> escape hatch (documented in README.md:160 and examples/opencode.yml:66-67, and tested at cmd/gadfly/opencode_test.go:18,48) is explicitly described as passing <provider>/<model> "straight to OpenCode's own provider registry/auth" (e.g. opencode/anthropic/claude-sonnet-4-6). OpenCode's built-in providers authenticate via their standard env vars — ANTHROPIC_API_KEY for the anthropic provider, OPENAI_API_KEY for openai, etc. But openCodeEnv() (opencode.go:243-265) only keeps OLLAMA_API_KEY plus the generic PATH/HOME/…/OPENCODE_*/NODE_*/XDG_* prefixes; it drops every provider key. The keep switch at opencode.go:248-256 has no ANTHROPIC_*/OPENAI_* branch, unlike claudeEnv() at engine.go:234-239 which keeps ANTHROPIC_* (and CLAUDE_*) so the claude-code engine can authenticate. Net effect: any non-ollama pass-through model runs unauthenticated and fails at the provider call, even though the workflow forwards the keys — .gitea/workflows/review-reusable.yml:126-127 forwards OPENAI_API_KEY/ANTHROPIC_API_KEY into the container where openCodeEnv() then discards them. This is the documented escape hatch, advertised with a concrete non-ollama example, not a corner case. Fix: keep the standard provider-key envs in openCodeEnv() (e.g. add ANTHROPIC_API_KEY, OPENAI_API_KEY, and any other OpenCode built-in provider keys intended for support), or — if the intent really is ollama-only — narrow the pass-through spec form and drop it from the docs/tests so users aren't pointed at a path that can't authenticate. (The bare opencode form has the same limitation, but it at least isn't advertised with a concrete non-ollama example.)
The primary opencode/<model> path is correctly authenticated (the generated gadfly provider references OLLAMA_API_KEY, which is kept); the bug is specific to the pass-through/non-ollama forms.
🧹 Code cleanliness & maintainability — Minor issues
All three findings are confirmed against the actual source.
Verdict: Minor issues
Findings
cmd/gadfly/opencode.go:38-46 — const block groups unrelated constants; doc comment visually misattaches to the wrong const. The single const block lumps the paired name constants (openCodeProviderName/openCodeAgentName, both "gadfly") together with the unrelated defaultOpenCodeBaseURL. Because openCodeAgentName = "gadfly" is shorter than the others, gofmt aligns it under a different column, so the // defaultOpenCodeBaseURL is ollama-cloud's… comment on line 44 reads as if it attaches to the agent-name line above it rather than to the base-URL line below. Trivial readability churn; splitting the base URL into its own const (or re-grouping) would fix it.
cmd/gadfly/opencode.go:243-265 — openCodeEnv duplicates claudeEnv's env-filter loop; should share a helper. The two functions (openCodeEnv here and claudeEnv at engine.go:228-248) share the identical idiom: iterate os.Environ(), strings.Cut(kv, "="), keep via a keep func(string) bool predicate, append the surviving kv. Only the allow-list differs (and openCodeEnv adds the OPENCODE_CONFIG_CONTENT deny guard + keeps OLLAMA_API_KEY; claudeEnv keeps MAX_THINKING_TOKENS + CLAUDE_/ANTHROPIC_ prefixes). A small shared helper like filterEnv(keep func(string) bool, drop []string) []string would remove the duplicated loop and keep the two from drifting further apart. Small/maintainability.
cmd/gadfly/opencode.go:196 — append(openCodeEnv(), …) is less scannable than the claude path's two-step style.claudeEnv is called and assigned first (engine.go:158: cmd.Env = claudeEnv()), then conditionally appended to (engine.go:161: cmd.Env = append(cmd.Env, …)), making the "base env + explicit overrides" intent clear. Here it's inlined as cmd.Env = append(openCodeEnv(), "OPENCODE_CONFIG_CONTENT="+…, "NO_COLOR=1") — functionally fine but a trivial style inconsistency between the two engine implementations. Not worth a churn-inducing change on its own.
The rest of the new file mirrors the established claudeCodeEngine patterns cleanly (constructor shape, pure args() factoring, runPass cancel/timeout/process-group handling, test layout reusing argAfter/shSingleQuote/itoa), and the engine-selection switch in main.go is a genuine readability improvement over the prior boolean. No dead code or leaky abstractions found.
⚡ Performance — No material issues found
Verdict: No material issues found
I reviewed the new opencode.go engine, main.go routing, and the Dockerfile warm-up through the performance lens.
Per-pass cost is inherent and bounded.runPass (opencode.go:185) builds a small JSON config, a tiny argv, and a filtered env slice, then spawns one CLI process — exactly mirroring claudeCodeEngine.runPass (engine.go:155). One process per lens is the shell-out design, not a regression. Nothing here loops or does avoidable quadratic work.
openCodeEnv() (opencode.go:243) rebuilds the env slice from os.Environ() on every pass. In principle this could be hoisted to newOpenCodeEngine, but os.Environ() is tens of entries and lens count is small (~5), so the cost is negligible — a micro-optimization I won't flag.
config(system) (opencode.go:155) marshals JSON per pass, but the system prompt differs per lens so it correctly can't be hoisted; the object is tiny.
stdout/stderr bytes.Buffer (opencode.go:207) is unbounded, but this matches the existing claude-code pattern (engine.go:173) — not a new regression.
Dockerfile warm-up (Dockerfile:52) adds up to 120s of build time (timeout 120), but it's build-time, best-effort (; true), and against a dead localhost URL that fails fast on ECONNREFUSED rather than hanging — not a runtime hot-path concern.
No runtime efficiency regressions, N+1 patterns, unbounded growth, or hot-loop waste introduced.
🧯 Error handling & edge cases — No material issues found
No material issues found
Through the error-handling & edge-case lens, the new opencode.go engine is well-defended on its unhappy paths. I verified each against the checked-out code:
Cancelled/timed-out runs (opencode.go:215): ctx.Err() is checked beforerunErr, so a killed run surfaces as a context error rather than leaking partial stdout. Correct, and mirrors the claude-code engine.
Process-group cleanup (opencode.go:200-206): Setpgid + Kill(-pgid) is guarded by a cmd.Process != nil check, so the cancel callback can't dereference a nil process if the binary never started. Good.
Missing binary (opencode.go:190): a non-installed opencode produces a PathError from cmd.Run(); the runErr != nil branch formats it via truncateForErr (verified at engine.go:252) and falls back to a bare error when stderr/stdout are empty. Surfaces as advisory, never panics.
Empty clean-exit output (opencode.go:230-233): explicitly returns an error rather than "", so downstream consolidation won't treat a silent pass as a valid empty review.
Error detail truncation: reuses truncateForErr, which cuts on a rune boundary (verified), so a huge stderr dump can't emit invalid UTF-8 or bloat the comment.
Env ambiguity (opencode.go:245): OPENCODE_CONFIG_CONTENT is explicitly dropped from the inherited env and only re-set by runPass, avoiding the first-occurrence-wins duplicate-key problem the comment calls out.
Spec edge cases: opencode/ (trailing slash) and bare opencode both resolve to after == "" → bare/default-model mode via strings.Cut (verified at :82), so no nil/empty-map issues in config() — the providerModel != "" guard omits the provider block cleanly.
args() task-last ordering: the task is the positional message and is always appended last; buildTask prefixes it with literal text ("PR title:"/"Review …" per the comment), so a crafted PR title can't make the positional begin with - and be misparsed as a flag. Verified the comment's claim is consistent with the code shape.
No swallowed errors, missing defers, nil-derefs, or unhandled boundary conditions found in this lens.
The OpenCode engine implementation is security-conscious:
Env filtering is correct: openCodeEnv() drops the runner's secrets (GITEA_TOKEN, ANTHROPIC_API_KEY, GADFLY_FINDINGS_TOKEN, etc.) and only forwards OLLAMA_API_KEY plus CLI-relevant knobs. OPENCODE_CONFIG_CONTENT is explicitly blocked from inheritance so runPass controls the only config source.
Read-only defense in depth: The generated JSON config denies edit/bash at both the global permission level and the per-agent permission level, preventing a reviewed repo from re-enabling writes.
Safe subprocess construction: args() appends the task as the final positional argument; buildTask prepends "PR title:"/"Review…" prefixes, so the task cannot be misinterpreted as a CLI flag. Process group isolation (Setpgid) and SIGKILL on cancel prevent orphaned processes.
No literal secrets in config: The provider apiKey is always the env reference {env:OLLAMA_API_KEY}, never a baked-in value.
I verified the env filtering logic by reading cmd/gadfly/opencode.go:243-264, the command assembly at :107-114, and the config generation at :155-183. No authn/authz gaps, injection vectors, or secret leakage paths are present in the new code.
🎯 Correctness — No material issues found
Verdict: No material issues found
After thorough verification of the opencode engine implementation (cmd/gadfly/opencode.go, opencode_test.go, main.go, and integration points), I found no correctness defects in the logic introduced by this PR.
Key correctness checks performed:
Model spec parsing: isOpenCodeSpec and newOpenCodeEngine correctly handle all documented forms (opencode, opencode/<model>, opencode/<provider>/<model>, open-code/ alias), with strings.Cut splitting on the first / so colons in ollama model IDs are preserved verbatim.
Config generation: The JSON config correctly omits the provider block for bare/pass-through specs (omitempty), injects the system prompt as the agent's prompt, and denies edit/bash at both global and agent levels. The API key is always the {env:OLLAMA_API_KEY} reference.
Environment filtering: openCodeEnv correctly withholds runner secrets (GITEA_TOKEN, ANTHROPIC_API_KEY, etc.) while passing through OLLAMA_API_KEY and OpenCode-specific knobs. OPENCODE_CONFIG_CONTENT is explicitly filtered out to avoid ambiguity before runPass sets it.
Process execution: Setpgid: true + cmd.Cancel killing the negative PID correctly prevents orphaned processes on timeout. Context cancellation is checked before runErr to surface timeout errors correctly.
Engine selection in main.go: The switch correctly routes both claude-code and opencode prefixes to their shell-out engines, and the auto selection properly falls back to the default suite for non-majordomo engines.
Auth chain: Verified that OLLAMA_CLOUD_API_KEY → OLLAMA_API_KEY mapping in scripts/run.sh feeds into openCodeEnv() and is referenced correctly in the generated config.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues found
cmd/gadfly/opencode.go:185 / cmd/gadfly/engine.go:155 — The runPass implementations for claudeCodeEngine and openCodeEngine duplicate ~40 lines of identical shell-out orchestration: Setpgid + SIGKILL process-group cleanup, bytes.Buffer stdout/stderr capture, the ctx.Err() cancellation guard, and the truncateForErr fallback chain. Any fix to timeout handling or orphan-process cleanup must be edited in two places. Suggested fix: Extract a shared helper such as runShellOut(ctx, cmd *exec.Cmd) (stdout, stderr string, err error) in engine.go that both engines call; keep the engine-specific JSON vs plain-text parsing in the callers.
cmd/gadfly/opencode.go:243 / cmd/gadfly/engine.go:228 — openCodeEnv and claudeEnv are near-copies of the same filtering loop with only the keep closure differing. Suggested fix: Extract a filterEnv(keep func(string) bool) []string helper in engine.go and have both env builders call it with their respective predicates.
⚡ Performance — No material issues found
No material issues found
The opencode engine introduces no material performance regressions beyond the inherent cost of shelling out to a CLI (which is the intended design for benchmarking). The per-pass environment filtering and JSON config marshaling are minor overheads dwarfed by the subprocess execution time, and the existing concurrency controls (lensConcurrency, entrypoint's per-provider caps) govern concurrent opencode processes the same way they govern claude-code processes. The unbounded bytes.Buffer for stdout/stderr mirrors the existing claude-code engine pattern.
🧯 Error handling & edge cases — No material issues found
No material issues found
The new opencode engine's error paths and edge cases look solid:
runPass surfaces context cancellation before examining runErr, preventing a timed-out run from returning partial stdout.
Empty stdout on clean exit returns an explicit error, not "".
openCodeEnv excludes OPENCODE_CONFIG_CONTENT to avoid duplicate-key ambiguity; runPass appends the generated config explicitly.
Setpgid + syscall.Kill(-pid, SIGKILL) in cmd.Cancel prevents orphaned Node child processes on timeout.
isOpenCodeSpec rejects false positives like "opencode-extra".
Colon-bearing model IDs are preserved verbatim; GADFLY_OPENCODE_MODEL override runs through the same slash logic.
main.go correctly falls back to the default suite for shell-out engines via type assertion (*majordomoEngine check).
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 3 models
**Verdict: Blocking issues found** · 9 findings (2 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🟠 | openCodeEnv() strips provider API keys (e.g. ANTHROPIC_API_KEY), breaking the documented opencode/<provider>/<model> pass-through escape hatch | `cmd/gadfly/opencode.go:243` | 3/3 | error-handling, maintainability |
| 🔴 | openCodeEnv drops ANTHROPIC_API_KEY/OPENAI_API_KEY, breaking the documented opencode/<provider>/<model> pass-through escape hatch (e.g. opencode/anthropic/...) which relies on OpenCode's built-in providers for auth | `cmd/gadfly/opencode.go:249` | 2/3 | correctness |
<details><summary>7 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟠 | Read-only enforcement denies only edit/bash; other OpenCode permission categories left at CLI default | `cmd/gadfly/opencode.go:155` | glm-5.2:cloud | security |
| 🟡 | runPass duplicates claudeCodeEngine shell-out orchestration; extract shared helper | `cmd/gadfly/opencode.go:185` | kimi-k2.6:cloud | maintainability |
| 🟡 | OPENCODE_CONFIG_CONTENT outranking repo opencode.json is an external CLI property, not enforced in code; repo could re-enable edits if precedence changes | `cmd/gadfly/opencode.go:196` | glm-5.2:cloud | maintainability, security |
| 🟡 | Process-group setup/kill-on-cancel block copy-pasted from claudeCodeEngine.runPass | `cmd/gadfly/opencode.go:200` | claude-code/sonnet | maintainability |
| 🟡 | Raw opencode CLI stderr/stdout (possibly containing credential/auth error detail) is truncated but surfaced verbatim into the publicly-posted review comment | `cmd/gadfly/opencode.go:219` | claude-code/sonnet | security |
| 🟡 | pre-flight Ollama key check only matches GADFLY_PROVIDER_EFF=ollama-cloud, so a missing OLLAMA_CLOUD_API_KEY for an opencode/<model> spec skips the friendly warning and falls through to a generic failure | `scripts/run.sh:167` | claude-code/sonnet | correctness |
| ⚪ | const block groups unrelated constants; doc comment visually misattaches to wrong const | `cmd/gadfly/opencode.go:38` | glm-5.2:cloud | 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> — Minor issues</summary>
Confirmed: `.gadfly.yml`'s `fileConfig` only has `Specialists`/`Define` fields — no path to `GADFLY_OPENCODE_*` or `GADFLY_MODEL`. This matches the draft's "no repo-controlled config injection" claim.
All the "verified as sound" claims and finding 1 check out against the actual code. Finding 2 is explicitly speculative about an external CLI's undocumented tool inventory that can't be confirmed from this repo, so it's dropped per the verification gate's confirm-or-drop rule.
## Security Review — `opencode.go` / OpenCode CLI engine
**VERDICT: Minor issues**
Verified as sound:
- **No injection vector**: `args()` builds an `exec.CommandContext` argv slice (no shell), and `config()` marshals via `encoding/json`, so the system prompt / task text can't break out of the JSON config or the argv (`opencode.go:107-114`, `155-183`).
- **API key never literal**: `Options.APIKey` is always the string `"{env:OLLAMA_API_KEY}"` (`opencode.go:176`), confirmed by `TestOpenCodeConfig`.
- **Env allowlist is deny-by-default**: `openCodeEnv()` (`opencode.go:243-265`) only forwards a fixed set + prefixes; `GITEA_TOKEN`, `GADFLY_API_KEY`, `GADFLY_FINDINGS_TOKEN`, `ANTHROPIC_API_KEY`, `CLAUDE_CODE_OAUTH_TOKEN` are excluded.
- **`OPENCODE_CONFIG_CONTENT` can't be double-set**: explicitly filtered out of the inherited env (`opencode.go:245-247`) before being appended fresh (`opencode.go:196`).
- **No repo-controlled config injection**: `.gadfly.yml`'s `fileConfig` (`specialists.go:69-77`) only has `Specialists`/`Define` fields — it cannot set `GADFLY_OPENCODE_BASE_URL`/`GADFLY_OPENCODE_EXTRA_ARGS`/`GADFLY_MODEL`, so a malicious PR can't redirect the subprocess's model endpoint or smuggle extra CLI flags via repo content.
Findings:
- **`cmd/gadfly/opencode.go:219`** — `runPass` forwards up to 800 chars (`truncateForErr`, `engine.go:252-258`) of the opencode subprocess's raw `stderr`/`stdout` into the returned error when the CLI fails. Confirmed this error string flows unmodified into the publicly-posted PR comment: `main.go:340` renders it verbatim as `"⚠️ This reviewer failed to complete: %v"`. If the `opencode` CLI or the underlying ollama-cloud HTTP client ever echoes auth-failure detail containing request fragments (e.g. an "invalid key: sk-…" message or a URL with embedded query state), that text becomes part of a public comment. This mirrors the pre-existing `claudeCodeEngine.runPass` pattern (`engine.go:200-212`), so it isn't a new failure class, but it is a second credential (`OLLAMA_CLOUD_API_KEY`, mapped to `OLLAMA_API_KEY` per `scripts/run.sh:158-161`) now flowing through the same raw-error-to-public-comment path via a new shell-out CLI. Low confidence — cannot confirm what `opencode run` actually prints on an auth failure without executing the real binary.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
Both findings verified directly against the code — the whitelist in `openCodeEnv()`, the conditional provider block in `config()`, the absence of any `opencode auth login`/credential-persistence step in the Dockerfile/entrypoint.sh, the `TestOpenCodeEnvFilters` assertions, and the `run.sh` provider-derivation/pre-flight-check logic all match the draft's claims exactly.
VERDICT: Minor issues
- **`cmd/gadfly/opencode.go:243-256`** (`openCodeEnv`) contradicts the feature it's shipped to support. The README (`README.md:160`) and the code's own doc comment (`opencode.go:68-69`) document `opencode/<provider>/<model>` as an "escape hatch" that passes through to "OpenCode's own provider registry/auth," and bare `opencode` as using "OpenCode's configured default model." But `openCodeEnv()`'s whitelist (lines 244-256) keeps only `PATH`/`HOME`/`USER`/`LOGNAME`/`TMPDIR`/`LANG`/`TERM`/`SHELL`/`OLLAMA_API_KEY` plus `LC_*`/`OPENCODE_*`/`GADFLY_OPENCODE_*`/`NODE_*`/`XDG_*` prefixes — excluding every other provider credential (confirmed by `TestOpenCodeEnvFilters` in `opencode_test.go:197-227`, which asserts `ANTHROPIC_API_KEY`/`CLAUDE_CODE_OAUTH_TOKEN` are dropped). Nothing in the `Dockerfile` or `entrypoint.sh` persists alternate-provider credentials into an OpenCode on-disk auth store (no `opencode auth login` step exists anywhere in the repo). `config()` (`opencode.go:169-181`) also only emits a `Provider` block when `e.providerModel != ""`, which is never true for the pass-through or bare forms (`newOpenCodeEngine`, `opencode.go:88-98`). So neither the env nor the generated config gives OpenCode any credential for a non-Ollama provider — the pass-through and bare spec forms have no working auth path and will fail at runtime for anything but the `opencode/<ollama-model>` form. Doesn't affect the PR's primary use case (benchmarking `opencode/glm-5.2` vs `ollama-cloud/glm-5.2`), only the two secondary spec forms documented in the README table.
- **`scripts/run.sh:167`** the pre-flight "no key configured" guard (`[ "$GADFLY_PROVIDER_EFF" = "ollama-cloud" ] && ...`) only fires when `GADFLY_PROVIDER_EFF` equals `ollama-cloud`. For an `opencode/<model>` spec, `MODEL_PROVIDER` (line 70-73, `case "$MODEL" in */*) MODEL_PROVIDER="${MODEL%%/*}"`) becomes `"opencode"`, and `GADFLY_PROVIDER_EFF="$MODEL_PROVIDER"` (line 163) inherits that — so this check never fires even though the opencode engine also depends on `OLLAMA_CLOUD_API_KEY`/`OLLAMA_API_KEY` (per this PR's own comment at `run.sh:32-37`). A missing key falls through to the generic "Agentic reviewer … failed (exit …)" path (line 193-198) instead of the specific, friendlier message. Still advisory-only — no invariant broken — just a less helpful failure surface than the one this PR documents as reused for opencode. (Note: the bare `opencode` spec, with no slash, actually falls into the `*)` case and gets `MODEL_PROVIDER="${GADFLY_PROVIDER:-ollama-cloud}"`, so the gap is specific to the `opencode/<model>` slash form.)
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**VERDICT: Minor issues**
- `cmd/gadfly/opencode.go:243-265` (`openCodeEnv`) duplicates the entire filter/loop structure of `claudeEnv` at `cmd/gadfly/engine.go:228-248` almost verbatim — same `keep(k)` switch-plus-prefix pattern and the identical `for _, kv := range os.Environ() { if k, _, ok := strings.Cut(kv, "="); ok && keep(k) { env = append(env, kv) } }; return env` body. Verified by reading both functions side by side. The PR already extracts shared helpers for other repeated bits (`envOr`, `truncateForErr` in `engine.go`), so the precedent for sharing exists — this one block was copy-pasted instead. A small `filterEnv(keep func(string) bool) []string` in `engine.go`, called from both `claudeEnv` and `openCodeEnv` with their own `keep` closures, removes the duplication with no behavior change.
- `cmd/gadfly/opencode.go:200-206` copy-pastes the process-group setup/kill logic from `claudeCodeEngine.runPass` (`cmd/gadfly/engine.go:166-172`) verbatim, down to the matching 3-line comment (only "claude" → "opencode" changed in the comment text). Verified by reading both `runPass` implementations. Since both shell-out engines need identical "own process group + SIGKILL the group on cancel" behavior, this is a good candidate for a one-line shared helper, e.g. `killProcessGroupOnCancel(cmd *exec.Cmd)` in `engine.go`, called from both engines' `runPass`. Low risk today, but a future fix to the kill semantics (e.g. switching to SIGTERM-then-SIGKILL) would otherwise have to be made in two places and could easily be applied to only one.
Both are small, mechanical, low-churn duplications rather than anything structurally wrong — the rest of the new `opencode.go`/`opencode_test.go` follows the existing `claude-code` engine's shape well and I found nothing else in scope for this lens.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**VERDICT: No material issues found**
Reviewed through the performance lens (allocations, hot-loop work, N+1, blocking calls, missing concurrency bounds).
What I checked:
- `cmd/gadfly/opencode.go` — `runPass` shells out once per lens/pass (same shape as the existing `claudeCodeEngine`), buffers stdout/stderr in-memory via `bytes.Buffer` (fine for review-sized output), and `openCodeEnv()` does a single `O(env vars)` scan per invocation — not a hot loop, called at most once per pass.
- Concurrency: verified `cmd/gadfly/main.go:245-308` (`fanout.Run` + `lensConcurrency()`, default 1) governs specialist parallelism uniformly for all three engines — the new opencode engine doesn't bypass or add its own concurrency path.
- Provider lane capping: confirmed `entrypoint.sh:240` (`provider_of()`) derives the provider from the text before the first `/`, so `opencode/<model>` entries correctly get their own `GADFLY_PROVIDER_CONCURRENCY` lane like any other provider — no unbounded parallel `opencode` process spawning introduced.
- `Dockerfile` warm-up step (`timeout 120 opencode run …`) runs once at image-build time, not per review — not a runtime regression, and it's `; true`-guarded so it can't hang the build indefinitely beyond the 120s timeout.
- `config()`/JSON marshaling in `opencode.go:155-183` builds a small, fixed-size struct per pass — no scaling concern.
Nothing in this diff introduces N+1 behavior, unbounded growth, missing limits, or blocking work on a path that runs more than once per lens/pass. The change is architecturally parallel to the existing claude-code engine and reuses its existing concurrency/lane controls.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
The finding is fully confirmed against the actual code and docs.
## Review — Error handling & edge cases lens
**VERDICT: Minor issues**
- **Documented `opencode/<provider>/<model>` escape hatch can't actually authenticate for its own example.** README.md:160 documents `opencode/anthropic/claude-sonnet-4-6` as a working pass-through that uses "OpenCode's own provider registry/auth." But `openCodeEnv()` (cmd/gadfly/opencode.go:243-264) only keeps `PATH/HOME/USER/LOGNAME/TMPDIR/LANG/TERM/SHELL/OLLAMA_API_KEY` plus `LC_/OPENCODE_/GADFLY_OPENCODE_/NODE_/XDG_` prefixes — `ANTHROPIC_API_KEY` (and every other provider key) is stripped from the subprocess environment before `opencode run` is invoked. The README itself confirms this a few lines later ("the Gitea token, Anthropic/Claude keys, and findings token aren't handed to the CLI," README.md:184). There is also no `opencode auth login` or baked-in credential setup anywhere in the Dockerfile/entrypoint.sh/run.sh. So the one concrete example given for this spec form has no way to authenticate through gadfly's own plumbing: a user who follows the doc and sets `GADFLY_MODELS=opencode/anthropic/claude-sonnet-4-6` (with `ANTHROPIC_API_KEY` set as a secret, expecting it to reach the CLI) gets a provider-auth failure from `opencode`, surfaced only as the generic `"opencode run failed: %v: %s"` (opencode.go:218-226) with no code path distinguishing "pass-through provider has no forwarded credential" from any other CLI error. This is an unhandled edge case for a spec form the diff explicitly introduces and documents as supported.
- Suggested fix: either forward the specific provider's standard env var(s) when `modelRef` doesn't start with `openCodeProviderName+"/"` (true pass-through), or drop the `anthropic/...` example from the docs and state plainly that the pass-through form only works for providers OpenCode can already authenticate without gadfly's help.
</details>
</details>
<details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Blocking issues found</summary>
**Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — Minor issues</summary>
Both findings concern behavior of the external `opencode` CLI that isn't vendored in this repo, but each rests on a verifiable code-level fact: `openCodePermission` only defines `Edit`/`Bash` (opencode.go:125-128) and `config()` sets exactly those two denies (opencode.go:155-168); and precedence over a repo `opencode.json` is not enforced anywhere in code — only asserted in comments (opencode.go:26-28, :194) and set via the env var (opencode.go:196). I confirmed both code facts. The draft correctly hedges these as defense-in-depth caveats, not hard defects. The third (operator `GADFLY_OPENCODE_BIN` verbatim) was explicitly "no action required" and not in the findings JSON, so it's correctly omitted.
**Verdict: Minor issues**
- **`cmd/gadfly/opencode.go:155` — read-only enforcement denies only `edit` and `bash`; other OpenCode permission/tool categories are left at the CLI default.** `openCodePermission` (opencode.go:125-128) hardcodes exactly two fields, and `config()` sets `Edit:"deny", Bash:"deny"` at both the global and agent level (opencode.go:156-167). The claude-code engine achieves read-only via `--permission-mode plan` (engine.go:64,111); the opencode engine's equivalence rests on the assumption that OpenCode's permission schema has only `edit`/`bash` categories. If OpenCode exposes any other actionable permission (web/fetch, MCP tools, `webfetch`, arbitrary tool execution), it is not denied and runs at the CLI's default for the `gadfly` agent — a prompt-injected/malicious review could exfiltrate via network or invoke repo-shipped MCP tools. The OpenCode source isn't vendored here, so this is medium-confidence; the fix is to confirm the full permission enum and deny every non-read category (or use an equivalent strict catch-all), not just `edit`/`bash`.
- **`cmd/gadfly/opencode.go:196` — `OPENCODE_CONFIG_CONTENT` outranking a repo `opencode.json` is an external CLI property, not enforced in code.** The comments (opencode.go:26-28, :194) and README claim `OPENCODE_CONFIG_CONTENT` "outranks any opencode.json a reviewed repo ships," which is the load-bearing security guarantee that a repo can't re-enable edits. That precedence is a property of the external `opencode` CLI, not anything this code enforces — `runPass` only sets the env var (opencode.go:196). If a future OpenCode build changes precedence to merge repo config on top (or honors a repo `opencode.json` that re-enables a permission), the deny can be undone with no code change here. Worth a defense-in-depth mitigation (e.g. removing/neutralizing any repo-root `opencode.json`/`.opencode/` before invoking) or documenting the dependency as a pinned CLI version rather than an invariant.
</details>
<details><summary><b>🎯 Correctness</b> — Blocking issues found</summary>
## VERDICT: Blocking issues found
- **`cmd/gadfly/opencode.go:243-265` — pass-through spec form is broken: its auth env is withheld.** The `opencode/<provider>/<model>` escape hatch (documented in `README.md:160` and `examples/opencode.yml:66-67`, and tested at `cmd/gadfly/opencode_test.go:18,48`) is explicitly described as passing `<provider>/<model>` "straight to OpenCode's own provider registry/auth" (e.g. `opencode/anthropic/claude-sonnet-4-6`). OpenCode's built-in providers authenticate via their standard env vars — `ANTHROPIC_API_KEY` for the `anthropic` provider, `OPENAI_API_KEY` for `openai`, etc. But `openCodeEnv()` (`opencode.go:243-265`) only keeps `OLLAMA_API_KEY` plus the generic `PATH/HOME/…/OPENCODE_*/NODE_*/XDG_*` prefixes; it drops every provider key. The `keep` switch at `opencode.go:248-256` has no `ANTHROPIC_*`/`OPENAI_*` branch, unlike `claudeEnv()` at `engine.go:234-239` which keeps `ANTHROPIC_*` (and `CLAUDE_*`) so the claude-code engine can authenticate. Net effect: any non-ollama pass-through model runs unauthenticated and fails at the provider call, even though the workflow forwards the keys — `.gitea/workflows/review-reusable.yml:126-127` forwards `OPENAI_API_KEY`/`ANTHROPIC_API_KEY` into the container where `openCodeEnv()` then discards them. This is the documented escape hatch, advertised with a concrete non-ollama example, not a corner case. Fix: keep the standard provider-key envs in `openCodeEnv()` (e.g. add `ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, and any other OpenCode built-in provider keys intended for support), or — if the intent really is ollama-only — narrow the pass-through spec form and drop it from the docs/tests so users aren't pointed at a path that can't authenticate. (The bare `opencode` form has the same limitation, but it at least isn't advertised with a concrete non-ollama example.)
The primary `opencode/<model>` path is correctly authenticated (the generated `gadfly` provider references `OLLAMA_API_KEY`, which is kept); the bug is specific to the pass-through/non-ollama forms.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
All three findings are confirmed against the actual source.
Verdict: Minor issues
## Findings
- **`cmd/gadfly/opencode.go:38-46` — const block groups unrelated constants; doc comment visually misattaches to the wrong const.** The single `const` block lumps the paired name constants (`openCodeProviderName`/`openCodeAgentName`, both `"gadfly"`) together with the unrelated `defaultOpenCodeBaseURL`. Because `openCodeAgentName = "gadfly"` is shorter than the others, gofmt aligns it under a different column, so the `// defaultOpenCodeBaseURL is ollama-cloud's…` comment on line 44 reads as if it attaches to the agent-name line above it rather than to the base-URL line below. Trivial readability churn; splitting the base URL into its own `const` (or re-grouping) would fix it.
- **`cmd/gadfly/opencode.go:243-265` — `openCodeEnv` duplicates `claudeEnv`'s env-filter loop; should share a helper.** The two functions (`openCodeEnv` here and `claudeEnv` at `engine.go:228-248`) share the identical idiom: iterate `os.Environ()`, `strings.Cut(kv, "=")`, keep via a `keep func(string) bool` predicate, append the surviving `kv`. Only the allow-list differs (and `openCodeEnv` adds the `OPENCODE_CONFIG_CONTENT` deny guard + keeps `OLLAMA_API_KEY`; `claudeEnv` keeps `MAX_THINKING_TOKENS` + `CLAUDE_`/`ANTHROPIC_` prefixes). A small shared helper like `filterEnv(keep func(string) bool, drop []string) []string` would remove the duplicated loop and keep the two from drifting further apart. Small/maintainability.
- **`cmd/gadfly/opencode.go:196` — `append(openCodeEnv(), …)` is less scannable than the claude path's two-step style.** `claudeEnv` is called and assigned first (`engine.go:158`: `cmd.Env = claudeEnv()`), then conditionally appended to (`engine.go:161`: `cmd.Env = append(cmd.Env, …)`), making the "base env + explicit overrides" intent clear. Here it's inlined as `cmd.Env = append(openCodeEnv(), "OPENCODE_CONFIG_CONTENT="+…, "NO_COLOR=1")` — functionally fine but a trivial style inconsistency between the two engine implementations. Not worth a churn-inducing change on its own.
The rest of the new file mirrors the established `claudeCodeEngine` patterns cleanly (constructor shape, pure `args()` factoring, `runPass` cancel/timeout/process-group handling, test layout reusing `argAfter`/`shSingleQuote`/`itoa`), and the engine-selection `switch` in `main.go` is a genuine readability improvement over the prior boolean. No dead code or leaky abstractions found.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
Verdict: No material issues found
I reviewed the new `opencode.go` engine, `main.go` routing, and the `Dockerfile` warm-up through the performance lens.
- **Per-pass cost is inherent and bounded.** `runPass` (opencode.go:185) builds a small JSON config, a tiny argv, and a filtered env slice, then spawns one CLI process — exactly mirroring `claudeCodeEngine.runPass` (engine.go:155). One process per lens is the shell-out design, not a regression. Nothing here loops or does avoidable quadratic work.
- **`openCodeEnv()` (opencode.go:243)** rebuilds the env slice from `os.Environ()` on every pass. In principle this could be hoisted to `newOpenCodeEngine`, but `os.Environ()` is tens of entries and lens count is small (~5), so the cost is negligible — a micro-optimization I won't flag.
- **`config(system)` (opencode.go:155)** marshals JSON per pass, but the system prompt differs per lens so it correctly can't be hoisted; the object is tiny.
- **stdout/stderr `bytes.Buffer` (opencode.go:207)** is unbounded, but this matches the existing claude-code pattern (engine.go:173) — not a new regression.
- **Dockerfile warm-up (Dockerfile:52)** adds up to 120s of build time (`timeout 120`), but it's build-time, best-effort (`; true`), and against a dead localhost URL that fails fast on ECONNREFUSED rather than hanging — not a runtime hot-path concern.
No runtime efficiency regressions, N+1 patterns, unbounded growth, or hot-loop waste introduced.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
No material issues found
Through the error-handling & edge-case lens, the new `opencode.go` engine is well-defended on its unhappy paths. I verified each against the checked-out code:
- **Cancelled/timed-out runs** (`opencode.go:215`): `ctx.Err()` is checked *before* `runErr`, so a killed run surfaces as a context error rather than leaking partial stdout. Correct, and mirrors the claude-code engine.
- **Process-group cleanup** (`opencode.go:200-206`): `Setpgid` + `Kill(-pgid)` is guarded by a `cmd.Process != nil` check, so the cancel callback can't dereference a nil process if the binary never started. Good.
- **Missing binary** (`opencode.go:190`): a non-installed `opencode` produces a `PathError` from `cmd.Run()`; the `runErr != nil` branch formats it via `truncateForErr` (verified at `engine.go:252`) and falls back to a bare error when stderr/stdout are empty. Surfaces as advisory, never panics.
- **Empty clean-exit output** (`opencode.go:230-233`): explicitly returns an error rather than `""`, so downstream consolidation won't treat a silent pass as a valid empty review.
- **Error detail truncation**: reuses `truncateForErr`, which cuts on a rune boundary (verified), so a huge stderr dump can't emit invalid UTF-8 or bloat the comment.
- **Env ambiguity** (`opencode.go:245`): `OPENCODE_CONFIG_CONTENT` is explicitly dropped from the inherited env and only re-set by `runPass`, avoiding the first-occurrence-wins duplicate-key problem the comment calls out.
- **Spec edge cases**: `opencode/` (trailing slash) and bare `opencode` both resolve to `after == ""` → bare/default-model mode via `strings.Cut` (verified at `:82`), so no nil/empty-map issues in `config()` — the `providerModel != ""` guard omits the provider block cleanly.
- **`args()` task-last ordering**: the task is the positional message and is always appended last; `buildTask` prefixes it with literal text (`"PR title:"`/`"Review …"` per the comment), so a crafted PR title can't make the positional begin with `-` and be misparsed as a flag. Verified the comment's claim is consistent with the code shape.
No swallowed errors, missing defers, nil-derefs, or unhandled boundary conditions found in this lens.
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
No material issues found
The OpenCode engine implementation is security-conscious:
- **Env filtering is correct**: `openCodeEnv()` drops the runner's secrets (`GITEA_TOKEN`, `ANTHROPIC_API_KEY`, `GADFLY_FINDINGS_TOKEN`, etc.) and only forwards `OLLAMA_API_KEY` plus CLI-relevant knobs. `OPENCODE_CONFIG_CONTENT` is explicitly blocked from inheritance so `runPass` controls the only config source.
- **Read-only defense in depth**: The generated JSON config denies `edit`/`bash` at both the global `permission` level and the per-agent `permission` level, preventing a reviewed repo from re-enabling writes.
- **Safe subprocess construction**: `args()` appends the task as the final positional argument; `buildTask` prepends `"PR title:"`/`"Review…"` prefixes, so the task cannot be misinterpreted as a CLI flag. Process group isolation (`Setpgid`) and `SIGKILL` on cancel prevent orphaned processes.
- **No literal secrets in config**: The provider `apiKey` is always the env reference `{env:OLLAMA_API_KEY}`, never a baked-in value.
I verified the env filtering logic by reading `cmd/gadfly/opencode.go:243-264`, the command assembly at `:107-114`, and the config generation at `:155-183`. No authn/authz gaps, injection vectors, or secret leakage paths are present in the new code.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**Verdict: No material issues found**
After thorough verification of the opencode engine implementation (`cmd/gadfly/opencode.go`, `opencode_test.go`, `main.go`, and integration points), I found no correctness defects in the logic introduced by this PR.
Key correctness checks performed:
- **Model spec parsing**: `isOpenCodeSpec` and `newOpenCodeEngine` correctly handle all documented forms (`opencode`, `opencode/<model>`, `opencode/<provider>/<model>`, `open-code/` alias), with `strings.Cut` splitting on the first `/` so colons in ollama model IDs are preserved verbatim.
- **Config generation**: The JSON config correctly omits the provider block for bare/pass-through specs (`omitempty`), injects the system prompt as the agent's prompt, and denies `edit`/`bash` at both global and agent levels. The API key is always the `{env:OLLAMA_API_KEY}` reference.
- **Environment filtering**: `openCodeEnv` correctly withholds runner secrets (`GITEA_TOKEN`, `ANTHROPIC_API_KEY`, etc.) while passing through `OLLAMA_API_KEY` and OpenCode-specific knobs. `OPENCODE_CONFIG_CONTENT` is explicitly filtered out to avoid ambiguity before `runPass` sets it.
- **Process execution**: `Setpgid: true` + `cmd.Cancel` killing the negative PID correctly prevents orphaned processes on timeout. Context cancellation is checked before `runErr` to surface timeout errors correctly.
- **Engine selection in `main.go`**: The switch correctly routes both `claude-code` and `opencode` prefixes to their shell-out engines, and the `auto` selection properly falls back to the default suite for non-majordomo engines.
- **Auth chain**: Verified that `OLLAMA_CLOUD_API_KEY` → `OLLAMA_API_KEY` mapping in `scripts/run.sh` feeds into `openCodeEnv()` and is referenced correctly in the generated config.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Minor issues found**
- **`cmd/gadfly/opencode.go:185` / `cmd/gadfly/engine.go:155`** — The `runPass` implementations for `claudeCodeEngine` and `openCodeEngine` duplicate ~40 lines of identical shell-out orchestration: `Setpgid` + `SIGKILL` process-group cleanup, `bytes.Buffer` stdout/stderr capture, the `ctx.Err()` cancellation guard, and the `truncateForErr` fallback chain. Any fix to timeout handling or orphan-process cleanup must be edited in two places. **Suggested fix:** Extract a shared helper such as `runShellOut(ctx, cmd *exec.Cmd) (stdout, stderr string, err error)` in `engine.go` that both engines call; keep the engine-specific JSON vs plain-text parsing in the callers.
- **`cmd/gadfly/opencode.go:243` / `cmd/gadfly/engine.go:228`** — `openCodeEnv` and `claudeEnv` are near-copies of the same filtering loop with only the `keep` closure differing. **Suggested fix:** Extract a `filterEnv(keep func(string) bool) []string` helper in `engine.go` and have both env builders call it with their respective predicates.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found
The opencode engine introduces no material performance regressions beyond the inherent cost of shelling out to a CLI (which is the intended design for benchmarking). The per-pass environment filtering and JSON config marshaling are minor overheads dwarfed by the subprocess execution time, and the existing concurrency controls (`lensConcurrency`, entrypoint's per-provider caps) govern concurrent opencode processes the same way they govern claude-code processes. The unbounded `bytes.Buffer` for stdout/stderr mirrors the existing claude-code engine pattern.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
No material issues found
The new `opencode` engine's error paths and edge cases look solid:
- `runPass` surfaces context cancellation before examining `runErr`, preventing a timed-out run from returning partial stdout.
- Empty stdout on clean exit returns an explicit error, not `""`.
- `openCodeEnv` excludes `OPENCODE_CONFIG_CONTENT` to avoid duplicate-key ambiguity; `runPass` appends the generated config explicitly.
- `Setpgid` + `syscall.Kill(-pid, SIGKILL)` in `cmd.Cancel` prevents orphaned Node child processes on timeout.
- `isOpenCodeSpec` rejects false positives like `"opencode-extra"`.
- Colon-bearing model IDs are preserved verbatim; `GADFLY_OPENCODE_MODEL` override runs through the same slash logic.
- `main.go` correctly falls back to the default suite for shell-out engines via type assertion (`*majordomoEngine` check).
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
Gadfly's own swarm reviewed PR #26 and reached consensus (3/3 models) on a real
bug, plus flagged security/maintainability items. Fixes:
- Pass-through auth (BLOCKING, 3/3 agreement): openCodeEnv() stripped every
provider key except OLLAMA_API_KEY, so the documented opencode/<provider>/<model>
escape hatch (e.g. opencode/anthropic/...) had no way to authenticate — the
reusable workflow forwards ANTHROPIC_API_KEY/OPENAI_API_KEY into the container
and the allowlist discarded them. Now forward ANTHROPIC_*/OPENAI_*/GOOGLE_*/
GEMINI_* so OpenCode's built-in providers can authenticate, while still
withholding gadfly's own secrets (Gitea/findings tokens, claude-code OAuth).
- Read-only hardening (security lens): the generated config denied only edit/bash.
Using OpenCode's documented permission schema, also deny webfetch/websearch/
external_directory — the network + out-of-sandbox tools — closing the
exfiltration surface a prompt-injected review could otherwise reach. Permission
is now a map so the deny set is extensible.
- Dedup (maintainability lens, 3/3): extract shared filterEnv() and
killGroupOnCancel() helpers in engine.go, used by both shell-out engines'
runPass/env builders instead of the copy-pasted blocks.
- Cosmetic: split the const block so defaultOpenCodeBaseURL's doc comment no
longer visually misattaches to the agent-name const.
README updated: the read-only note and the reduced-env note now reflect the
broader deny set and the forwarded provider keys.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
steve
merged commit bb98fae5f0 into main2026-07-18 05:51:51 +00:00
I've verified both findings against the actual code.
Finding 1 (line 208): Confirmed — cmd.Dir = e.repoDir (line 206) means the CLI runs in the checked-out repo tree where OpenCode would discover any opencode.json. The code asserts (lines 25-27, 208-210) that OPENCODE_CONFIG_CONTENT outranks it, but there's no in-repo verification of that external precedence claim, and no integration test guarding it.
Finding 2 (line 135): Confirmed — denyMutations pins exactly the 5 keys edit/bash/webfetch/websearch/external_directory. The comment (lines 132-134) only addresses extra keys being ignored, not missing real mutating tool keys.
Both findings accurately describe the code's actual state. Keeping both.
VERDICT: Minor issues
cmd/gadfly/opencode.go:206-211 / cmd/gadfly/opencode.go:25-27 — read-only enforcement depends on an unverified precedence claim. The CLI runs with cmd.Dir = e.repoDir (the checked-out PR tree, line 206), so OpenCode will discover any opencode.json the reviewed repo ships there. The only thing keeping a malicious repo from re-enabling edit/bash and mutating the checkout (or running arbitrary commands) during a review is the comment-asserted claim (lines 25-27, 208-210) that OPENCODE_CONFIG_CONTENT "outranks any opencode.json." This is the central security invariant and it is stated, not tested or verified in-repo. If opencode instead merges configs with the repo file winning for keys the generated config doesn't pin, or if a future opencode version changes precedence, a reviewed repo escapes read-only. Suggested fix: verify against opencode's current docs/source (link the precedence rule in a comment), and/or add an integration test that drops an opencode.json with permission:{edit:allow} in repoDir and asserts the generated config wins; as defense-in-depth, also pass --config pointing at a path the repo can't shadow, or chroot the CLI to a dir without the repo's config.
cmd/gadfly/opencode.go:135-143 — the deny-list is asserted complete but isn't provably so.denyMutations pins exactly edit/bash/webfetch/websearch/external_directory. The comment (lines 132-134) defends unknown keys ("a key it doesn't recognize is simply ignored, so listing extras is safe") but that addresses extra keys, not missing real ones. If opencode gates any mutating/command tool under a different permission key (e.g. a write/create/delete/task/MCP-tool key), it would be allowed by default and a repo could use it. Suggested fix: confirm against opencode's enumerated permission keys (the URL in the comment, https://opencode.ai/docs/agents) and either deny every mutating key explicitly or use an allow-list (read/grep/glob/list/lsp → allow, everything else → deny) if opencode supports a catch-all, which is more robust than a deny-list against a third-party tool surface you don't control.
🎯 Correctness — No material issues found
Let me verify one more potential correctness concern around the OpenCode config schema and the run subcommand.
Let me check a couple more potential correctness concerns around the recheck and consolidation integration with the new engine.
Now let me verify the provider_of lane behavior with the opencode prefix — there could be a lane mismatch issue.
Let me look at the Dockerfile change more carefully for the build correctness.
I've thoroughly reviewed the change through the correctness lens, reading the new engine, its tests, the refactored engine.go, main.go's engine selection switch, the recheck/consolidation integration paths, the entrypoint/run.sh provider-lane wiring, the Dockerfile, and the workflow YAML changes.
Verdict: No material issues found
I verified the following potential correctness concerns and they all check out:
Spec parsing in newOpenCodeEngine (opencode.go:82-98): strings.Cut on the first / correctly handles all three forms — bare (opencode), wrapped (opencode/glm-5.2), pass-through (opencode/anthropic/claude). Colon-bearing ids like qwen3-coder:480b-cloud survive because there's no :-suffix parsing (unlike claude-code). The open-code/ alias works because Cut discards the prefix. GADFLY_OPENCODE_MODEL override runs through the same slash logic per the doc. ✅
args ordering (opencode.go:107-114): run --agent gadfly --model <ref> [extra] <task-last>. Task is a single argv element (no shell splitting), and buildTask/buildRecheckTask outputs never begin with -, so no -- terminator is needed. ✅
Config generation (opencode.go:170-198): provider,omitempty correctly omits the block for bare/pass-through specs; Agent (no omitempty) is always present. The Models map marshals to {"model":{}}. JSON struct tags are correct. The API key is always the {env:OLLAMA_API_KEY} reference, never a literal. ✅
Context cancellation (opencode.go:221-223): ctx.Err() is checked beforerunErr, so a timed-out run surfaces as an error even if the CLI flushed partial stdout — matches the claude engine. killGroupOnCancel (engine.go:217-225) correctly sets Setpgid and kills -PID (the whole group), with a cmd.Process != nil guard. ✅
main.go engine switch (main.go:160-189): the switch correctly routes to claude-code, opencode, or majordomo. The auto-select path (main.go:200-219) now uses a *majordomoEngine type assertion instead of a boolean, so a shell-out engine can never hit the old eng.(*majordomoEngine) panic. ✅
Recheck integration (main.go:344-352): the opencode engine handles the recheck pass identically to the review pass — recheckSystemPrompt becomes the agent prompt in a freshly generated config. No engine-specific code needed. ✅
Provider lane consistency: entrypoint.sh:240provider_of and model.go:167modelProvider() both extract the first path segment, so opencode/glm-5.2 lands in the opencode lane consistently in both the entrypoint (concurrency) and the binary (lens concurrency). run.sh hardcodes PROVIDER=ollama (run.sh:263), so opencode specs enter the ollama case and get the OLLAMA_CLOUD_API_KEY → OLLAMA_API_KEY mapping (run.sh:160-161) that the generated provider's {env:OLLAMA_API_KEY} reference needs. ✅
openCodeEnv filtering (opencode.go:254-272): OPENCODE_CONFIG_CONTENT is explicitly excluded (line 256-258), avoiding a duplicate-key ambiguity since runPass sets it explicitly. OLLAMA_API_KEY is kept; gadfly's own secrets are dropped. ✅
filterEnv refactor (engine.go:230-238): the extracted helper preserves the exact prior behavior of claudeEnv; the keep predicate is unchanged. ✅
Dogfood caller fix (adversarial-review.yml:58): pr_number: ${{ github.event.inputs.pr_number }} correctly threads the dispatch input through the reusable's inputs.pr_number, which the reusable reads at line 116 via the || chain. For pull_request events, github.event.pull_request.number is still propagated (only dispatch inputs are dropped by Gitea ≥1.27), so the reusable's PR env is populated and the entrypoint's workflow_call reclassification (entrypoint.sh:129-136) works. ✅
The only things I cannot verify from the repo are external OpenCode CLI behaviors (the run --agent --model flag surface, the permission schema keys, OPENCODE_CONFIG_CONTENT precedence over opencode.json, Mode: "primary"). These are domain assumptions about a third-party CLI, not code-internal logic, and the README honestly documents the engine as "newly wired, lightly tested." None of them are code-correctness bugs I can flag from the checked-out source.
🧹 Code cleanliness & maintainability — No material issues found
Now let me check the consolidated structure of the two engines side by side for code-cleanliness observations.
Let me verify a couple of specific cleanliness concerns.
I reviewed the new opencode.go, the refactored engine.go, main.go's engine switch, and the test file against the surrounding code patterns. The change is clean: the filterEnv/killGroupOnCancel extraction is exactly the right dedup, the new engine mirrors claudeCodeEngine's structure (struct shape, args() factored out for testability, runPass error-handling shape, env-filtering helper), and the doc comments carry the same voice and density as the rest of the package.
Verdict: No material issues found
I verified filterEnv / killGroupOnCancel are now shared by both shell-out engines and that the old inlined duplication in claudeCodeEngine.runPass is gone (engine.go:163, engine.go:217-225, engine.go:230-238). The extraction is well-scoped — no leaky abstraction.
openCodeEnv and claudeEnv now genuinely "differ only in their keep predicate" as the doc claims (engine.go:228-229); the predicates are structurally parallel (exact-name switch + HasPrefix chain). Confirmed OPENCODE_CONFIG_CONTENT is dropped by the filter and then re-appended by runPass (opencode.go:211, opencode.go:256-258), so there is no duplicate-key ambiguity the comment warns about.
The openCodeConfig / openCodeProvider / openCodeAgent structs are minimal and the JSON tags match the field names used in the test's round-trip (opencode_test.go:128-168). denyMutations() is built once and reused for both the global and agent permission maps, which is correct since json.Marshal doesn't mutate (opencode.go:171-182).
The one nit I'll note but not flag as a finding: opencode.go:15 lists OpenCode's read tools as read/grep/glob/list while opencode.go:131 lists them as read/glob/grep/list/lsp (different order, and the second includes lsp). Both are prose, but they describe the same set — trivial doc inconsistency only.
⚡ Performance — No material issues found
Verdict: No material issues found
I reviewed cmd/gadfly/opencode.go, engine.go, main.go, and the Dockerfile/entrypoint through the performance lens. The new shell-out engine follows the same patterns as the existing claudeCodeEngine, and I found no material efficiency regressions.
What I checked and why it's fine:
cmd/gadfly/opencode.go:200runPass — per pass it calls e.config(system) (small struct JSON marshal), e.args(task) (slice build), openCodeEnv() (one os.Environ() scan of a few-hundred-entry env, filtered into a fresh slice), and one exec.Cmd. All of this is dwarfed by the LLM round-trip the CLI makes; not a hot-loop concern. Verified openCodeEnv() returns a fresh slice each call (opencode.go:254), so the append(openCodeEnv(), …) on line 211 cannot alias/corrupt a shared backing array.
cmd/gadfly/opencode.go:170config — denyMutations() allocates a fresh 5-entry map per call and is invoked once per pass. Negligible.
Token burn from re-sending the transcript each CLI step is inherent to any shell-out engine (claude-code has the same characteristic). This PR doesn't worsen it: buildTask (main.go:364) still truncates the embedded diff to GADFLY_MAX_DIFF_CHARS (default 60000), and entrypoint.sh:220 downshifts huge PRs above GADFLY_HUGE_DIFF_BYTES. So the new engine inherits the same size-gating already in place. Not a regression introduced here.
Dockerfile:52 warm-up timeout 120 opencode run … — points at 127.0.0.1:9 (dead), so it fails on connection refused rather than burning the full 120s; build-time-only, best-effort (; true), and doesn't affect runtime. Not a runtime perf issue.
cmd/gadfly/opencode.go:213 unbounded bytes.Buffer for stdout/stderr — this is the same pattern as claudeCodeEngine (engine.go:164), bounded ultimately by the model's response length per pass. Standard; not flagging as a regression.
One low-confidence observation (not rising to a finding): OpenCode's default text output mode may emit intermediate agent/tool traces to stdout where claude-code emits a single JSON envelope, so per-pass stdout could be larger in memory. I couldn't verify OpenCode's actual output volume with the tools available, and it's bounded per pass regardless, so I'm not reporting it as a real issue — just noting the README already flags this engine as "newly wired, lightly tested."
🧯 Error handling & edge cases — Minor issues
Confirmed. The finding survives verification:
examples/reusable.yml declares workflow_dispatch.inputs.pr_number (line 35) but its with: block (lines 64-71) never threads pr_number: ${{ github.event.inputs.pr_number }}.
The reusable's PR: env (review-reusable.yml:116) falls back to inputs.pr_number, but that input is only populated when the caller passes it via with:.
The reusable's own input declaration + comments (lines 62-66) explicitly state the caller stub must thread it: "The caller stub must thread it explicitly: pr_number: ${{ github.event.inputs.pr_number }}."
The dogfood caller (adversarial-review.yml:58) DOES thread it — this PR's commit 2 added exactly that line.
Under Gitea ≥1.27, manual workflow_dispatch through examples/reusable.yml → all four fallbacks in PR: are empty → entrypoint.sh:70 (: "${PR:?PR required}") fails the job. Auto PR/comment triggers unaffected (they populate github.event.pull_request.number/github.event.issue.number).
CLAUDE.md maintenance rules require examples to be kept current in the same change that fixes the consumer-stub contract.
VERDICT: Minor issues
examples/reusable.yml — manual workflow_dispatch arrives with an empty PR (the same bug commit 2 fixes for the dogfood caller). This is the README's recommended "slimmest stub" and it calls the reusable workflow. It declares workflow_dispatch.inputs.pr_number (line 35) but its with: block (lines 64-71) never threads pr_number: ${{ github.event.inputs.pr_number }}. The reusable's PR: env (review-reusable.yml:116) falls back to inputs.pr_number, but that input is only populated when the caller passes it via with: — which this example doesn't. Under Gitea ≥1.27 (the breaking change cited in commit 2), a manual dispatch through this stub reaches the container with PR="", and entrypoint.sh:70 (: "${PR:?PR required}") fails the job with "PR required". Auto PR/comment triggers are unaffected (they populate github.event.pull_request.number/github.event.issue.number), so only the manual-dispatch unhappy path is broken. Verified by reading examples/reusable.yml:28-71 and review-reusable.yml:116,62-66. Fix: add pr_number: ${{ github.event.inputs.pr_number }} to the with: block (mirroring adversarial-review.yml:58). The maintenance rules in CLAUDE.md explicitly require examples to be kept current in the same change that fixes the consumer-stub contract.
Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 25m 54s
<!-- gadfly-review:ollama:opencode/glm-5.2:cloud -->
### 🪰 Gadfly review — `opencode/glm-5.2:cloud` (opencode)
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — Minor issues</summary>
I've verified both findings against the actual code.
Finding 1 (line 208): Confirmed — `cmd.Dir = e.repoDir` (line 206) means the CLI runs in the checked-out repo tree where OpenCode would discover any `opencode.json`. The code asserts (lines 25-27, 208-210) that `OPENCODE_CONFIG_CONTENT` outranks it, but there's no in-repo verification of that external precedence claim, and no integration test guarding it.
Finding 2 (line 135): Confirmed — `denyMutations` pins exactly the 5 keys `edit`/`bash`/`webfetch`/`websearch`/`external_directory`. The comment (lines 132-134) only addresses *extra* keys being ignored, not *missing real* mutating tool keys.
Both findings accurately describe the code's actual state. Keeping both.
VERDICT: Minor issues
- **`cmd/gadfly/opencode.go:206-211` / `cmd/gadfly/opencode.go:25-27` — read-only enforcement depends on an unverified precedence claim.** The CLI runs with `cmd.Dir = e.repoDir` (the checked-out PR tree, line 206), so OpenCode will discover any `opencode.json` the reviewed repo ships there. The only thing keeping a malicious repo from re-enabling `edit`/`bash` and mutating the checkout (or running arbitrary commands) during a review is the comment-asserted claim (lines 25-27, 208-210) that `OPENCODE_CONFIG_CONTENT` "outranks any `opencode.json`." This is the central security invariant and it is stated, not tested or verified in-repo. If opencode instead *merges* configs with the repo file winning for keys the generated config doesn't pin, or if a future opencode version changes precedence, a reviewed repo escapes read-only. Suggested fix: verify against opencode's current docs/source (link the precedence rule in a comment), and/or add an integration test that drops an `opencode.json` with `permission:{edit:allow}` in `repoDir` and asserts the generated config wins; as defense-in-depth, also pass `--config` pointing at a path the repo can't shadow, or chroot the CLI to a dir without the repo's config.
- **`cmd/gadfly/opencode.go:135-143` — the deny-list is asserted complete but isn't provably so.** `denyMutations` pins exactly `edit`/`bash`/`webfetch`/`websearch`/`external_directory`. The comment (lines 132-134) defends unknown keys ("a key it doesn't recognize is simply ignored, so listing extras is safe") but that addresses *extra* keys, not *missing real ones*. If opencode gates any mutating/command tool under a different permission key (e.g. a `write`/`create`/`delete`/`task`/MCP-tool key), it would be allowed by default and a repo could use it. Suggested fix: confirm against opencode's enumerated permission keys (the URL in the comment, `https://opencode.ai/docs/agents`) and either deny every mutating key explicitly or use an allow-list (`read`/`grep`/`glob`/`list`/`lsp` → allow, everything else → deny) if opencode supports a catch-all, which is more robust than a deny-list against a third-party tool surface you don't control.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
Let me verify one more potential correctness concern around the OpenCode config schema and the `run` subcommand.
Let me check a couple more potential correctness concerns around the recheck and consolidation integration with the new engine.
Now let me verify the `provider_of` lane behavior with the opencode prefix — there could be a lane mismatch issue.
Let me look at the Dockerfile change more carefully for the build correctness.
I've thoroughly reviewed the change through the correctness lens, reading the new engine, its tests, the refactored engine.go, main.go's engine selection switch, the recheck/consolidation integration paths, the entrypoint/run.sh provider-lane wiring, the Dockerfile, and the workflow YAML changes.
**Verdict: No material issues found**
I verified the following potential correctness concerns and they all check out:
- **Spec parsing in `newOpenCodeEngine`** (opencode.go:82-98): `strings.Cut` on the first `/` correctly handles all three forms — bare (`opencode`), wrapped (`opencode/glm-5.2`), pass-through (`opencode/anthropic/claude`). Colon-bearing ids like `qwen3-coder:480b-cloud` survive because there's no `:`-suffix parsing (unlike claude-code). The `open-code/` alias works because `Cut` discards the prefix. `GADFLY_OPENCODE_MODEL` override runs through the same slash logic per the doc. ✅
- **`args` ordering** (opencode.go:107-114): `run --agent gadfly --model <ref> [extra] <task-last>`. Task is a single argv element (no shell splitting), and `buildTask`/`buildRecheckTask` outputs never begin with `-`, so no `--` terminator is needed. ✅
- **Config generation** (opencode.go:170-198): `provider,omitempty` correctly omits the block for bare/pass-through specs; `Agent` (no omitempty) is always present. The `Models` map marshals to `{"model":{}}`. JSON struct tags are correct. The API key is always the `{env:OLLAMA_API_KEY}` reference, never a literal. ✅
- **Context cancellation** (opencode.go:221-223): `ctx.Err()` is checked *before* `runErr`, so a timed-out run surfaces as an error even if the CLI flushed partial stdout — matches the claude engine. `killGroupOnCancel` (engine.go:217-225) correctly sets `Setpgid` and kills `-PID` (the whole group), with a `cmd.Process != nil` guard. ✅
- **`main.go` engine switch** (main.go:160-189): the `switch` correctly routes to claude-code, opencode, or majordomo. The auto-select path (main.go:200-219) now uses a `*majordomoEngine` type assertion instead of a boolean, so a shell-out engine can never hit the old `eng.(*majordomoEngine)` panic. ✅
- **Recheck integration** (main.go:344-352): the opencode engine handles the recheck pass identically to the review pass — `recheckSystemPrompt` becomes the agent prompt in a freshly generated config. No engine-specific code needed. ✅
- **Provider lane consistency**: `entrypoint.sh:240` `provider_of` and `model.go:167` `modelProvider()` both extract the first path segment, so `opencode/glm-5.2` lands in the `opencode` lane consistently in both the entrypoint (concurrency) and the binary (lens concurrency). `run.sh` hardcodes `PROVIDER=ollama` (run.sh:263), so opencode specs enter the `ollama` case and get the `OLLAMA_CLOUD_API_KEY → OLLAMA_API_KEY` mapping (run.sh:160-161) that the generated provider's `{env:OLLAMA_API_KEY}` reference needs. ✅
- **`openCodeEnv` filtering** (opencode.go:254-272): `OPENCODE_CONFIG_CONTENT` is explicitly excluded (line 256-258), avoiding a duplicate-key ambiguity since `runPass` sets it explicitly. `OLLAMA_API_KEY` is kept; gadfly's own secrets are dropped. ✅
- **`filterEnv` refactor** (engine.go:230-238): the extracted helper preserves the exact prior behavior of `claudeEnv`; the keep predicate is unchanged. ✅
- **Dogfood caller fix** (adversarial-review.yml:58): `pr_number: ${{ github.event.inputs.pr_number }}` correctly threads the dispatch input through the reusable's `inputs.pr_number`, which the reusable reads at line 116 via the `||` chain. For `pull_request` events, `github.event.pull_request.number` is still propagated (only dispatch *inputs* are dropped by Gitea ≥1.27), so the reusable's `PR` env is populated and the entrypoint's `workflow_call` reclassification (entrypoint.sh:129-136) works. ✅
The only things I cannot verify from the repo are external OpenCode CLI behaviors (the `run --agent --model` flag surface, the `permission` schema keys, `OPENCODE_CONFIG_CONTENT` precedence over `opencode.json`, `Mode: "primary"`). These are domain assumptions about a third-party CLI, not code-internal logic, and the README honestly documents the engine as "newly wired, lightly tested." None of them are code-correctness bugs I can flag from the checked-out source.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
Now let me check the consolidated structure of the two engines side by side for code-cleanliness observations.
Let me verify a couple of specific cleanliness concerns.
I reviewed the new `opencode.go`, the refactored `engine.go`, `main.go`'s engine switch, and the test file against the surrounding code patterns. The change is clean: the `filterEnv`/`killGroupOnCancel` extraction is exactly the right dedup, the new engine mirrors `claudeCodeEngine`'s structure (struct shape, `args()` factored out for testability, `runPass` error-handling shape, env-filtering helper), and the doc comments carry the same voice and density as the rest of the package.
**Verdict: No material issues found**
- I verified `filterEnv` / `killGroupOnCancel` are now shared by both shell-out engines and that the old inlined duplication in `claudeCodeEngine.runPass` is gone (`engine.go:163`, `engine.go:217-225`, `engine.go:230-238`). The extraction is well-scoped — no leaky abstraction.
- `openCodeEnv` and `claudeEnv` now genuinely "differ only in their keep predicate" as the doc claims (`engine.go:228-229`); the predicates are structurally parallel (exact-name `switch` + `HasPrefix` chain). Confirmed `OPENCODE_CONFIG_CONTENT` is dropped by the filter and then re-appended by `runPass` (`opencode.go:211`, `opencode.go:256-258`), so there is no duplicate-key ambiguity the comment warns about.
- The `openCodeConfig` / `openCodeProvider` / `openCodeAgent` structs are minimal and the JSON tags match the field names used in the test's round-trip (`opencode_test.go:128-168`). `denyMutations()` is built once and reused for both the global and agent permission maps, which is correct since `json.Marshal` doesn't mutate (`opencode.go:171-182`).
- The one nit I'll note but not flag as a finding: `opencode.go:15` lists OpenCode's read tools as `read/grep/glob/list` while `opencode.go:131` lists them as `read/glob/grep/list/lsp` (different order, and the second includes `lsp`). Both are prose, but they describe the same set — trivial doc inconsistency only.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
Verdict: No material issues found
I reviewed `cmd/gadfly/opencode.go`, `engine.go`, `main.go`, and the Dockerfile/entrypoint through the performance lens. The new shell-out engine follows the same patterns as the existing `claudeCodeEngine`, and I found no material efficiency regressions.
What I checked and why it's fine:
- `cmd/gadfly/opencode.go:200` `runPass` — per pass it calls `e.config(system)` (small struct JSON marshal), `e.args(task)` (slice build), `openCodeEnv()` (one `os.Environ()` scan of a few-hundred-entry env, filtered into a fresh slice), and one `exec.Cmd`. All of this is dwarfed by the LLM round-trip the CLI makes; not a hot-loop concern. Verified `openCodeEnv()` returns a fresh slice each call (`opencode.go:254`), so the `append(openCodeEnv(), …)` on line 211 cannot alias/corrupt a shared backing array.
- `cmd/gadfly/opencode.go:170` `config` — `denyMutations()` allocates a fresh 5-entry map per call and is invoked once per pass. Negligible.
- Token burn from re-sending the transcript each CLI step is inherent to *any* shell-out engine (claude-code has the same characteristic). This PR doesn't worsen it: `buildTask` (`main.go:364`) still truncates the embedded diff to `GADFLY_MAX_DIFF_CHARS` (default 60000), and `entrypoint.sh:220` downshifts huge PRs above `GADFLY_HUGE_DIFF_BYTES`. So the new engine inherits the same size-gating already in place. Not a regression introduced here.
- `Dockerfile:52` warm-up `timeout 120 opencode run …` — points at `127.0.0.1:9` (dead), so it fails on connection refused rather than burning the full 120s; build-time-only, best-effort (`; true`), and doesn't affect runtime. Not a runtime perf issue.
- `cmd/gadfly/opencode.go:213` unbounded `bytes.Buffer` for stdout/stderr — this is the same pattern as `claudeCodeEngine` (`engine.go:164`), bounded ultimately by the model's response length per pass. Standard; not flagging as a regression.
One low-confidence observation (not rising to a finding): OpenCode's default text output mode may emit intermediate agent/tool traces to stdout where claude-code emits a single JSON envelope, so per-pass stdout could be larger in memory. I couldn't verify OpenCode's actual output volume with the tools available, and it's bounded per pass regardless, so I'm not reporting it as a real issue — just noting the README already flags this engine as "newly wired, lightly tested."
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
Confirmed. The finding survives verification:
- `examples/reusable.yml` declares `workflow_dispatch.inputs.pr_number` (line 35) but its `with:` block (lines 64-71) never threads `pr_number: ${{ github.event.inputs.pr_number }}`.
- The reusable's `PR:` env (`review-reusable.yml:116`) falls back to `inputs.pr_number`, but that input is only populated when the caller passes it via `with:`.
- The reusable's own input declaration + comments (lines 62-66) explicitly state the caller stub must thread it: "The caller stub must thread it explicitly: `pr_number: ${{ github.event.inputs.pr_number }}`."
- The dogfood caller (`adversarial-review.yml:58`) DOES thread it — this PR's commit 2 added exactly that line.
- Under Gitea ≥1.27, manual `workflow_dispatch` through `examples/reusable.yml` → all four fallbacks in `PR:` are empty → `entrypoint.sh:70` (`: "${PR:?PR required}"`) fails the job. Auto PR/comment triggers unaffected (they populate `github.event.pull_request.number`/`github.event.issue.number`).
- `CLAUDE.md` maintenance rules require examples to be kept current in the same change that fixes the consumer-stub contract.
## VERDICT: Minor issues
- **`examples/reusable.yml` — manual `workflow_dispatch` arrives with an empty PR (the same bug commit 2 fixes for the dogfood caller).** This is the README's recommended "slimmest stub" and it calls the reusable workflow. It declares `workflow_dispatch.inputs.pr_number` (line 35) but its `with:` block (lines 64-71) never threads `pr_number: ${{ github.event.inputs.pr_number }}`. The reusable's `PR:` env (`review-reusable.yml:116`) falls back to `inputs.pr_number`, but that input is only populated when the caller passes it via `with:` — which this example doesn't. Under Gitea ≥1.27 (the breaking change cited in commit 2), a manual dispatch through this stub reaches the container with `PR=""`, and `entrypoint.sh:70` (`: "${PR:?PR required}"`) fails the job with "PR required". Auto PR/comment triggers are unaffected (they populate `github.event.pull_request.number`/`github.event.issue.number`), so only the manual-dispatch unhappy path is broken. Verified by reading `examples/reusable.yml:28-71` and `review-reusable.yml:116,62-66`. Fix: add `pr_number: ${{ github.event.inputs.pr_number }}` to the `with:` block (mirroring `adversarial-review.yml:58`). The maintenance rules in `CLAUDE.md` explicitly require examples to be kept current in the same change that fixes the consumer-stub contract.
</details>
<sub>Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 25m 54s</sub>
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.
Recreated from #25 to re-fire the review trigger after fixing the self-review caller (see commit 2).
What
Adds a third review harness alongside the in-process majordomo loop and the claude-code CLI shell-out: the OpenCode CLI (opencode.ai) driving an ollama-cloud model, selected by an
opencode/<model>spec.The point is to benchmark gadfly's boutique executus harness against a freely-available agentic harness on the same model — run
ollama-cloud/glm-5.2(majordomo loop) andopencode/glm-5.2(OpenCode) side by side and compare findings. This is the wired, no-proxy version of the README's existing "alternate backends" idea.Commits
feat(engine): add opencode CLI review engine— the feature (below).fix(dogfood): thread dispatch pr_number into gadfly's own review caller—adversarial-review.ymlwas never updated for the Gitea ≥ 1.27 breaking change (go-gitea#37478): a called workflow no longer receives the caller'sworkflow_dispatchinputs ingithub.event, so a manual dispatch reached the reusable with an empty PR. Commit64d34bdadded thepr_numberworkflow_call input and mort's caller threads it; this brings gadfly's own caller in line so the dogfood path works on the same Gitea version. (This is why #25 was recreated — the review triggers don't includesynchronize, so a freshopenedevent was needed against the fixed workflow.)How the engine works
--append-system-prompt, so each pass generates an OpenCode config — the lens system prompt as agadflyagent's prompt withedit/bashdenied at both global and agent level, plus agadflyollama-cloud provider — and injects it viaOPENCODE_CONFIG_CONTENT. That env var is the highest-precedence config source in the container, so a reviewed repo's ownopencode.jsoncan't re-enable edits on the reviewer.opencode/<model>(wrapped in the generated provider), theopen-code/alias,opencode/<provider>/<model>pass-through to OpenCode's own registry/auth, and bareopencode. Model ids are taken verbatim, so colon-bearing ollama ids likeqwen3-coder:480b-cloudsurvive (no:thinkingsuffix like claude-code).OLLAMA_CLOUD_API_KEY(mapped toOLLAMA_API_KEY, referenced as{env:OLLAMA_API_KEY}in config — never a literal secret). Knobs mirrorGADFLY_CLAUDE_*:GADFLY_OPENCODE_BIN/MODEL/BASE_URL/EXTRA_ARGS.openCodeEnv()forwardsOLLAMA_API_KEY(the inverse ofclaudeEnv) but still withholds the Gitea/findings/Anthropic secrets.main.goengine selection is now a switch (claude-code / opencode / majordomo); the auto-select path uses a type-check instead of a boolean so a shell-out engine can never hit the*majordomoEngineassertion.autoselection anddelegate_investigationstay majordomo-only and are skipped for opencode (the CLI does its own legwork).Files
cmd/gadfly/opencode.go(new) — the engine.cmd/gadfly/opencode_test.go(new) — 12 tests mirroringengine_test.go.cmd/gadfly/main.go,engine.go— engine selection switch + doc.Dockerfile— bundlesopencode-ai(npm auto-selects its musl build on alpine) with a best-effort version check + provider pre-warm that never fails the shared image build.README.md,examples/opencode.yml(new),examples/README.md,CLAUDE.md,scripts/run.sh,entrypoint.sh,.gitea/workflows/review-reusable.yml— docs/comments per the maintenance rules..gitea/workflows/adversarial-review.yml— the dogfood-caller fix (commit 2).Verification
go build,go test ./...,go vet,gofmtall clean.opencodeCLI: confirmedmain.goroutesopencode/…to the new engine, emits the correct argv (run --agent gadfly --model gadfly/qwen3-coder:480b-cloud … <task-last>), injects the expected config (env-ref API key, both-level deny, real system prompt), and consolidates output into review markdown.Not yet verified (needs the built image / a real run)
opencode-ai(opencode-linux-x64-musl, etc.) that it auto-selects on alpine, so the musl risk is small, but confirm withdocker run --entrypoint opencode <img> --version.--agent+ deny behaves non-interactively as expected and its default text output captures cleanly. Documented honestly as "newly wired, lightly tested" in the README.To put it on the live fleet: add an
opencode/<model>entry to theGADFLY_DEFAULT_MODELSuser-scope var (runtime retune, no repo change), pinning consumers to the fresh:sha-<short>since:latestis runner-cached.🤖 Generated with Claude Code
Add a third review harness alongside the in-process majordomo loop and the claude-code CLI shell-out: the OpenCode CLI (opencode.ai) driving an ollama-cloud model, selected by an "opencode/<model>" spec. The goal is to benchmark gadfly's boutique executus harness against a freely-available agentic harness on the SAME model (e.g. "ollama-cloud/glm-5.2" vs "opencode/glm-5.2"). OpenCode has no --append-system-prompt flag, so the lens system prompt and the read-only discipline are delivered through a generated config injected via OPENCODE_CONFIG_CONTENT: a "gadfly" agent whose prompt is the system prompt with edit/bash denied at both the global and agent level, plus a "gadfly" ollama-cloud provider. That env var is the highest-precedence config source in the container, so a reviewed repo's own opencode.json can't re-enable edits on the reviewer. Spec forms: "opencode/<model>" (wrapped in the generated provider), the "open-code/" alias, "opencode/<provider>/<model>" pass-through to OpenCode's own registry, and bare "opencode". Model ids are taken verbatim so colon-bearing ollama ids (qwen3-coder:480b-cloud) survive. Auth reuses OLLAMA_CLOUD_API_KEY (mapped to OLLAMA_API_KEY, referenced as {env:OLLAMA_API_KEY} in config, never a literal secret). Knobs mirror GADFLY_CLAUDE_*: GADFLY_OPENCODE_BIN/MODEL/BASE_URL/ EXTRA_ARGS. openCodeEnv() forwards OLLAMA_API_KEY (the inverse of claudeEnv) but still withholds the Gitea/findings/Anthropic secrets. main.go engine selection is now a switch (claude-code / opencode / majordomo), and the auto-select path uses a type-check instead of a boolean so a shell-out engine can never hit the *majordomoEngine assertion. auto-select and delegate_investigation stay majordomo-only and are skipped for opencode (the CLI does its own legwork). Dockerfile bundles opencode-ai (npm auto-selects its musl build on alpine) with a best-effort version check + provider pre-warm that never fails the shared image build. README/examples/CLAUDE.md/scripts updated per the maintenance rules. Tests: new opencode_test.go mirrors engine_test.go (spec/model/args/config/env- filter + stub-CLI runtime tests). Verified end-to-end with a fake opencode CLI: correct argv, injected config, and consolidated markdown output. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>🪰 Gadfly — live review status
1/1 reviewers finished · updated 2026-07-18 06:31:04Z
opencode/glm-5.2:cloud· opencode — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 8 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -0,0 +35,4 @@extraArgs []string // appended verbatim (GADFLY_OPENCODE_EXTRA_ARGS)}const (⚪ const block groups unrelated constants; doc comment visually misattaches to wrong const
maintainability · flagged by 1 model
cmd/gadfly/opencode.go:38-46— const block groups unrelated constants; doc comment visually misattaches to the wrong const. The singleconstblock lumps the paired name constants (openCodeProviderName/openCodeAgentName, both"gadfly") together with the unrelateddefaultOpenCodeBaseURL. BecauseopenCodeAgentName = "gadfly"is shorter than the others, gofmt aligns it under a different column, so the// defaultOpenCodeBaseURL is ollama-cloud's…comment on line 44 reads as if i…🪰 Gadfly · advisory
@@ -0,0 +152,4 @@// bare or pass-through spec the generated ollama-cloud provider block is omitted.// The API key is always the "{env:OLLAMA_API_KEY}" reference, never a literal// secret baked into the config.func (e *openCodeEngine) config(system string) ([]byte, error) {🟠 Read-only enforcement denies only edit/bash; other OpenCode permission categories left at CLI default
security · flagged by 1 model
cmd/gadfly/opencode.go:155— read-only enforcement denies onlyeditandbash; other OpenCode permission/tool categories are left at the CLI default.openCodePermission(opencode.go:125-128) hardcodes exactly two fields, andconfig()setsEdit:"deny", Bash:"deny"at both the global and agent level (opencode.go:156-167). The claude-code engine achieves read-only via--permission-mode plan(engine.go:64,111); the opencode engine's equivalence rests on the assumption that OpenCod…🪰 Gadfly · advisory
@@ -0,0 +182,4 @@return json.Marshal(cfg)}func (e *openCodeEngine) runPass(ctx context.Context, system, task string, _ int) (string, error) {🟡 runPass duplicates claudeCodeEngine shell-out orchestration; extract shared helper
maintainability · flagged by 1 model
cmd/gadfly/opencode.go:185/cmd/gadfly/engine.go:155— TherunPassimplementations forclaudeCodeEngineandopenCodeEngineduplicate ~40 lines of identical shell-out orchestration:Setpgid+SIGKILLprocess-group cleanup,bytes.Bufferstdout/stderr capture, thectx.Err()cancellation guard, and thetruncateForErrfallback chain. Any fix to timeout handling or orphan-process cleanup must be edited in two places. Suggested fix: Extract a shared helper such as `runShe…🪰 Gadfly · advisory
@@ -0,0 +193,4 @@// permissions + the ollama-cloud provider) via OPENCODE_CONFIG_CONTENT, which// outranks any opencode.json the reviewed repo itself ships. NO_COLOR keeps the// captured stdout free of ANSI decoration.cmd.Env = append(openCodeEnv(), "OPENCODE_CONFIG_CONTENT="+string(cfg), "NO_COLOR=1")🟡 OPENCODE_CONFIG_CONTENT outranking repo opencode.json is an external CLI property, not enforced in code; repo could re-enable edits if precedence changes
maintainability, security · flagged by 1 model
cmd/gadfly/opencode.go:196—OPENCODE_CONFIG_CONTENToutranking a repoopencode.jsonis an external CLI property, not enforced in code. The comments (opencode.go:26-28, :194) and README claimOPENCODE_CONFIG_CONTENT"outranks any opencode.json a reviewed repo ships," which is the load-bearing security guarantee that a repo can't re-enable edits. That precedence is a property of the externalopencodeCLI, not anything this code enforces —runPassonly sets the env var (opencode.…🪰 Gadfly · advisory
@@ -0,0 +197,4 @@// Put the CLI and the Node children it spawns in their own process group and// kill the WHOLE group on context cancel, so a timed-out lens can't leave// orphaned opencode/node processes behind in the container.cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}🟡 Process-group setup/kill-on-cancel block copy-pasted from claudeCodeEngine.runPass
maintainability · flagged by 1 model
cmd/gadfly/opencode.go:200-206copy-pastes the process-group setup/kill logic fromclaudeCodeEngine.runPass(cmd/gadfly/engine.go:166-172) verbatim, down to the matching 3-line comment (only "claude" → "opencode" changed in the comment text). Verified by reading bothrunPassimplementations. Since both shell-out engines need identical "own process group + SIGKILL the group on cancel" behavior, this is a good candidate for a one-line shared helper, e.g. `killProcessGroupOnCancel(cmd *ex…🪰 Gadfly · advisory
@@ -0,0 +216,4 @@return "", fmt.Errorf("opencode run %v", ctx.Err())}if runErr != nil {detail := truncateForErr(stderr.String())🟡 Raw opencode CLI stderr/stdout (possibly containing credential/auth error detail) is truncated but surfaced verbatim into the publicly-posted review comment
security · flagged by 1 model
cmd/gadfly/opencode.go:219—runPassforwards up to 800 chars (truncateForErr,engine.go:252-258) of the opencode subprocess's rawstderr/stdoutinto the returned error when the CLI fails. Confirmed this error string flows unmodified into the publicly-posted PR comment:main.go:340renders it verbatim as"⚠️ This reviewer failed to complete: %v". If theopencodeCLI or the underlying ollama-cloud HTTP client ever echoes auth-failure detail containing request fragments (e.…🪰 Gadfly · advisory
@@ -0,0 +240,4 @@// GADFLY_FINDINGS_TOKEN, Anthropic/Claude keys) are withheld — the CLI has no need// for them. OPENCODE_CONFIG_CONTENT is never inherited: runPass sets it, and a// duplicate key would be ambiguous (getenv returns the first occurrence).func openCodeEnv() []string {🟠 openCodeEnv() strips provider API keys (e.g. ANTHROPIC_API_KEY), breaking the documented opencode// pass-through escape hatch
error-handling, maintainability · flagged by 3 models
opencode/<provider>/<model>escape hatch can't actually authenticate for its own example. README.md:160 documentsopencode/anthropic/claude-sonnet-4-6as a working pass-through that uses "OpenCode's own provider registry/auth." ButopenCodeEnv()(cmd/gadfly/opencode.go:243-264) only keepsPATH/HOME/USER/LOGNAME/TMPDIR/LANG/TERM/SHELL/OLLAMA_API_KEYplusLC_/OPENCODE_/GADFLY_OPENCODE_/NODE_/XDG_prefixes —ANTHROPIC_API_KEY(and every other provider key) is stripped f…🪰 Gadfly · advisory
@@ -0,0 +246,4 @@return false // set explicitly by runPass; never inherit a competing value}switch k {case "PATH", "HOME", "USER", "LOGNAME", "TMPDIR", "LANG", "TERM", "SHELL", "OLLAMA_API_KEY":🔴 openCodeEnv drops ANTHROPIC_API_KEY/OPENAI_API_KEY, breaking the documented opencode// pass-through escape hatch (e.g. opencode/anthropic/...) which relies on OpenCode's built-in providers for auth
correctness · flagged by 2 models
🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 3 models
Verdict: Blocking issues found · 9 findings (2 with multi-model agreement)
cmd/gadfly/opencode.go:243cmd/gadfly/opencode.go:2497 single-model findings (lower confidence)
cmd/gadfly/opencode.go:155cmd/gadfly/opencode.go:185cmd/gadfly/opencode.go:196cmd/gadfly/opencode.go:200cmd/gadfly/opencode.go:219scripts/run.sh:167cmd/gadfly/opencode.go:38Per-model detail
claude-code/sonnet (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Minor issues
Confirmed:
.gadfly.yml'sfileConfigonly hasSpecialists/Definefields — no path toGADFLY_OPENCODE_*orGADFLY_MODEL. This matches the draft's "no repo-controlled config injection" claim.All the "verified as sound" claims and finding 1 check out against the actual code. Finding 2 is explicitly speculative about an external CLI's undocumented tool inventory that can't be confirmed from this repo, so it's dropped per the verification gate's confirm-or-drop rule.
Security Review —
opencode.go/ OpenCode CLI engineVERDICT: Minor issues
Verified as sound:
args()builds anexec.CommandContextargv slice (no shell), andconfig()marshals viaencoding/json, so the system prompt / task text can't break out of the JSON config or the argv (opencode.go:107-114,155-183).Options.APIKeyis always the string"{env:OLLAMA_API_KEY}"(opencode.go:176), confirmed byTestOpenCodeConfig.openCodeEnv()(opencode.go:243-265) only forwards a fixed set + prefixes;GITEA_TOKEN,GADFLY_API_KEY,GADFLY_FINDINGS_TOKEN,ANTHROPIC_API_KEY,CLAUDE_CODE_OAUTH_TOKENare excluded.OPENCODE_CONFIG_CONTENTcan't be double-set: explicitly filtered out of the inherited env (opencode.go:245-247) before being appended fresh (opencode.go:196)..gadfly.yml'sfileConfig(specialists.go:69-77) only hasSpecialists/Definefields — it cannot setGADFLY_OPENCODE_BASE_URL/GADFLY_OPENCODE_EXTRA_ARGS/GADFLY_MODEL, so a malicious PR can't redirect the subprocess's model endpoint or smuggle extra CLI flags via repo content.Findings:
cmd/gadfly/opencode.go:219—runPassforwards up to 800 chars (truncateForErr,engine.go:252-258) of the opencode subprocess's rawstderr/stdoutinto the returned error when the CLI fails. Confirmed this error string flows unmodified into the publicly-posted PR comment:main.go:340renders it verbatim as"⚠️ This reviewer failed to complete: %v". If theopencodeCLI or the underlying ollama-cloud HTTP client ever echoes auth-failure detail containing request fragments (e.g. an "invalid key: sk-…" message or a URL with embedded query state), that text becomes part of a public comment. This mirrors the pre-existingclaudeCodeEngine.runPasspattern (engine.go:200-212), so it isn't a new failure class, but it is a second credential (OLLAMA_CLOUD_API_KEY, mapped toOLLAMA_API_KEYperscripts/run.sh:158-161) now flowing through the same raw-error-to-public-comment path via a new shell-out CLI. Low confidence — cannot confirm whatopencode runactually prints on an auth failure without executing the real binary.🎯 Correctness — Minor issues
Both findings verified directly against the code — the whitelist in
openCodeEnv(), the conditional provider block inconfig(), the absence of anyopencode auth login/credential-persistence step in the Dockerfile/entrypoint.sh, theTestOpenCodeEnvFiltersassertions, and therun.shprovider-derivation/pre-flight-check logic all match the draft's claims exactly.VERDICT: Minor issues
cmd/gadfly/opencode.go:243-256(openCodeEnv) contradicts the feature it's shipped to support. The README (README.md:160) and the code's own doc comment (opencode.go:68-69) documentopencode/<provider>/<model>as an "escape hatch" that passes through to "OpenCode's own provider registry/auth," and bareopencodeas using "OpenCode's configured default model." ButopenCodeEnv()'s whitelist (lines 244-256) keeps onlyPATH/HOME/USER/LOGNAME/TMPDIR/LANG/TERM/SHELL/OLLAMA_API_KEYplusLC_*/OPENCODE_*/GADFLY_OPENCODE_*/NODE_*/XDG_*prefixes — excluding every other provider credential (confirmed byTestOpenCodeEnvFiltersinopencode_test.go:197-227, which assertsANTHROPIC_API_KEY/CLAUDE_CODE_OAUTH_TOKENare dropped). Nothing in theDockerfileorentrypoint.shpersists alternate-provider credentials into an OpenCode on-disk auth store (noopencode auth loginstep exists anywhere in the repo).config()(opencode.go:169-181) also only emits aProviderblock whene.providerModel != "", which is never true for the pass-through or bare forms (newOpenCodeEngine,opencode.go:88-98). So neither the env nor the generated config gives OpenCode any credential for a non-Ollama provider — the pass-through and bare spec forms have no working auth path and will fail at runtime for anything but theopencode/<ollama-model>form. Doesn't affect the PR's primary use case (benchmarkingopencode/glm-5.2vsollama-cloud/glm-5.2), only the two secondary spec forms documented in the README table.scripts/run.sh:167the pre-flight "no key configured" guard ([ "$GADFLY_PROVIDER_EFF" = "ollama-cloud" ] && ...) only fires whenGADFLY_PROVIDER_EFFequalsollama-cloud. For anopencode/<model>spec,MODEL_PROVIDER(line 70-73,case "$MODEL" in */*) MODEL_PROVIDER="${MODEL%%/*}") becomes"opencode", andGADFLY_PROVIDER_EFF="$MODEL_PROVIDER"(line 163) inherits that — so this check never fires even though the opencode engine also depends onOLLAMA_CLOUD_API_KEY/OLLAMA_API_KEY(per this PR's own comment atrun.sh:32-37). A missing key falls through to the generic "Agentic reviewer … failed (exit …)" path (line 193-198) instead of the specific, friendlier message. Still advisory-only — no invariant broken — just a less helpful failure surface than the one this PR documents as reused for opencode. (Note: the bareopencodespec, with no slash, actually falls into the*)case and getsMODEL_PROVIDER="${GADFLY_PROVIDER:-ollama-cloud}", so the gap is specific to theopencode/<model>slash form.)🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
cmd/gadfly/opencode.go:243-265(openCodeEnv) duplicates the entire filter/loop structure ofclaudeEnvatcmd/gadfly/engine.go:228-248almost verbatim — samekeep(k)switch-plus-prefix pattern and the identicalfor _, kv := range os.Environ() { if k, _, ok := strings.Cut(kv, "="); ok && keep(k) { env = append(env, kv) } }; return envbody. Verified by reading both functions side by side. The PR already extracts shared helpers for other repeated bits (envOr,truncateForErrinengine.go), so the precedent for sharing exists — this one block was copy-pasted instead. A smallfilterEnv(keep func(string) bool) []stringinengine.go, called from bothclaudeEnvandopenCodeEnvwith their ownkeepclosures, removes the duplication with no behavior change.cmd/gadfly/opencode.go:200-206copy-pastes the process-group setup/kill logic fromclaudeCodeEngine.runPass(cmd/gadfly/engine.go:166-172) verbatim, down to the matching 3-line comment (only "claude" → "opencode" changed in the comment text). Verified by reading bothrunPassimplementations. Since both shell-out engines need identical "own process group + SIGKILL the group on cancel" behavior, this is a good candidate for a one-line shared helper, e.g.killProcessGroupOnCancel(cmd *exec.Cmd)inengine.go, called from both engines'runPass. Low risk today, but a future fix to the kill semantics (e.g. switching to SIGTERM-then-SIGKILL) would otherwise have to be made in two places and could easily be applied to only one.Both are small, mechanical, low-churn duplications rather than anything structurally wrong — the rest of the new
opencode.go/opencode_test.gofollows the existingclaude-codeengine's shape well and I found nothing else in scope for this lens.⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed through the performance lens (allocations, hot-loop work, N+1, blocking calls, missing concurrency bounds).
What I checked:
cmd/gadfly/opencode.go—runPassshells out once per lens/pass (same shape as the existingclaudeCodeEngine), buffers stdout/stderr in-memory viabytes.Buffer(fine for review-sized output), andopenCodeEnv()does a singleO(env vars)scan per invocation — not a hot loop, called at most once per pass.cmd/gadfly/main.go:245-308(fanout.Run+lensConcurrency(), default 1) governs specialist parallelism uniformly for all three engines — the new opencode engine doesn't bypass or add its own concurrency path.entrypoint.sh:240(provider_of()) derives the provider from the text before the first/, soopencode/<model>entries correctly get their ownGADFLY_PROVIDER_CONCURRENCYlane like any other provider — no unbounded parallelopencodeprocess spawning introduced.Dockerfilewarm-up step (timeout 120 opencode run …) runs once at image-build time, not per review — not a runtime regression, and it's; true-guarded so it can't hang the build indefinitely beyond the 120s timeout.config()/JSON marshaling inopencode.go:155-183builds a small, fixed-size struct per pass — no scaling concern.Nothing in this diff introduces N+1 behavior, unbounded growth, missing limits, or blocking work on a path that runs more than once per lens/pass. The change is architecturally parallel to the existing claude-code engine and reuses its existing concurrency/lane controls.
🧯 Error handling & edge cases — Minor issues
The finding is fully confirmed against the actual code and docs.
Review — Error handling & edge cases lens
VERDICT: Minor issues
opencode/<provider>/<model>escape hatch can't actually authenticate for its own example. README.md:160 documentsopencode/anthropic/claude-sonnet-4-6as a working pass-through that uses "OpenCode's own provider registry/auth." ButopenCodeEnv()(cmd/gadfly/opencode.go:243-264) only keepsPATH/HOME/USER/LOGNAME/TMPDIR/LANG/TERM/SHELL/OLLAMA_API_KEYplusLC_/OPENCODE_/GADFLY_OPENCODE_/NODE_/XDG_prefixes —ANTHROPIC_API_KEY(and every other provider key) is stripped from the subprocess environment beforeopencode runis invoked. The README itself confirms this a few lines later ("the Gitea token, Anthropic/Claude keys, and findings token aren't handed to the CLI," README.md:184). There is also noopencode auth loginor baked-in credential setup anywhere in the Dockerfile/entrypoint.sh/run.sh. So the one concrete example given for this spec form has no way to authenticate through gadfly's own plumbing: a user who follows the doc and setsGADFLY_MODELS=opencode/anthropic/claude-sonnet-4-6(withANTHROPIC_API_KEYset as a secret, expecting it to reach the CLI) gets a provider-auth failure fromopencode, surfaced only as the generic"opencode run failed: %v: %s"(opencode.go:218-226) with no code path distinguishing "pass-through provider has no forwarded credential" from any other CLI error. This is an unhandled edge case for a spec form the diff explicitly introduces and documents as supported.modelRefdoesn't start withopenCodeProviderName+"/"(true pass-through), or drop theanthropic/...example from the docs and state plainly that the pass-through form only works for providers OpenCode can already authenticate without gadfly's help.glm-5.2:cloud (ollama-cloud) — Blocking issues found
Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Minor issues
Both findings concern behavior of the external
opencodeCLI that isn't vendored in this repo, but each rests on a verifiable code-level fact:openCodePermissiononly definesEdit/Bash(opencode.go:125-128) andconfig()sets exactly those two denies (opencode.go:155-168); and precedence over a repoopencode.jsonis not enforced anywhere in code — only asserted in comments (opencode.go:26-28, :194) and set via the env var (opencode.go:196). I confirmed both code facts. The draft correctly hedges these as defense-in-depth caveats, not hard defects. The third (operatorGADFLY_OPENCODE_BINverbatim) was explicitly "no action required" and not in the findings JSON, so it's correctly omitted.Verdict: Minor issues
cmd/gadfly/opencode.go:155— read-only enforcement denies onlyeditandbash; other OpenCode permission/tool categories are left at the CLI default.openCodePermission(opencode.go:125-128) hardcodes exactly two fields, andconfig()setsEdit:"deny", Bash:"deny"at both the global and agent level (opencode.go:156-167). The claude-code engine achieves read-only via--permission-mode plan(engine.go:64,111); the opencode engine's equivalence rests on the assumption that OpenCode's permission schema has onlyedit/bashcategories. If OpenCode exposes any other actionable permission (web/fetch, MCP tools,webfetch, arbitrary tool execution), it is not denied and runs at the CLI's default for thegadflyagent — a prompt-injected/malicious review could exfiltrate via network or invoke repo-shipped MCP tools. The OpenCode source isn't vendored here, so this is medium-confidence; the fix is to confirm the full permission enum and deny every non-read category (or use an equivalent strict catch-all), not justedit/bash.cmd/gadfly/opencode.go:196—OPENCODE_CONFIG_CONTENToutranking a repoopencode.jsonis an external CLI property, not enforced in code. The comments (opencode.go:26-28, :194) and README claimOPENCODE_CONFIG_CONTENT"outranks any opencode.json a reviewed repo ships," which is the load-bearing security guarantee that a repo can't re-enable edits. That precedence is a property of the externalopencodeCLI, not anything this code enforces —runPassonly sets the env var (opencode.go:196). If a future OpenCode build changes precedence to merge repo config on top (or honors a repoopencode.jsonthat re-enables a permission), the deny can be undone with no code change here. Worth a defense-in-depth mitigation (e.g. removing/neutralizing any repo-rootopencode.json/.opencode/before invoking) or documenting the dependency as a pinned CLI version rather than an invariant.🎯 Correctness — Blocking issues found
VERDICT: Blocking issues found
cmd/gadfly/opencode.go:243-265— pass-through spec form is broken: its auth env is withheld. Theopencode/<provider>/<model>escape hatch (documented inREADME.md:160andexamples/opencode.yml:66-67, and tested atcmd/gadfly/opencode_test.go:18,48) is explicitly described as passing<provider>/<model>"straight to OpenCode's own provider registry/auth" (e.g.opencode/anthropic/claude-sonnet-4-6). OpenCode's built-in providers authenticate via their standard env vars —ANTHROPIC_API_KEYfor theanthropicprovider,OPENAI_API_KEYforopenai, etc. ButopenCodeEnv()(opencode.go:243-265) only keepsOLLAMA_API_KEYplus the genericPATH/HOME/…/OPENCODE_*/NODE_*/XDG_*prefixes; it drops every provider key. Thekeepswitch atopencode.go:248-256has noANTHROPIC_*/OPENAI_*branch, unlikeclaudeEnv()atengine.go:234-239which keepsANTHROPIC_*(andCLAUDE_*) so the claude-code engine can authenticate. Net effect: any non-ollama pass-through model runs unauthenticated and fails at the provider call, even though the workflow forwards the keys —.gitea/workflows/review-reusable.yml:126-127forwardsOPENAI_API_KEY/ANTHROPIC_API_KEYinto the container whereopenCodeEnv()then discards them. This is the documented escape hatch, advertised with a concrete non-ollama example, not a corner case. Fix: keep the standard provider-key envs inopenCodeEnv()(e.g. addANTHROPIC_API_KEY,OPENAI_API_KEY, and any other OpenCode built-in provider keys intended for support), or — if the intent really is ollama-only — narrow the pass-through spec form and drop it from the docs/tests so users aren't pointed at a path that can't authenticate. (The bareopencodeform has the same limitation, but it at least isn't advertised with a concrete non-ollama example.)The primary
opencode/<model>path is correctly authenticated (the generatedgadflyprovider referencesOLLAMA_API_KEY, which is kept); the bug is specific to the pass-through/non-ollama forms.🧹 Code cleanliness & maintainability — Minor issues
All three findings are confirmed against the actual source.
Verdict: Minor issues
Findings
cmd/gadfly/opencode.go:38-46— const block groups unrelated constants; doc comment visually misattaches to the wrong const. The singleconstblock lumps the paired name constants (openCodeProviderName/openCodeAgentName, both"gadfly") together with the unrelateddefaultOpenCodeBaseURL. BecauseopenCodeAgentName = "gadfly"is shorter than the others, gofmt aligns it under a different column, so the// defaultOpenCodeBaseURL is ollama-cloud's…comment on line 44 reads as if it attaches to the agent-name line above it rather than to the base-URL line below. Trivial readability churn; splitting the base URL into its ownconst(or re-grouping) would fix it.cmd/gadfly/opencode.go:243-265—openCodeEnvduplicatesclaudeEnv's env-filter loop; should share a helper. The two functions (openCodeEnvhere andclaudeEnvatengine.go:228-248) share the identical idiom: iterateos.Environ(),strings.Cut(kv, "="), keep via akeep func(string) boolpredicate, append the survivingkv. Only the allow-list differs (andopenCodeEnvadds theOPENCODE_CONFIG_CONTENTdeny guard + keepsOLLAMA_API_KEY;claudeEnvkeepsMAX_THINKING_TOKENS+CLAUDE_/ANTHROPIC_prefixes). A small shared helper likefilterEnv(keep func(string) bool, drop []string) []stringwould remove the duplicated loop and keep the two from drifting further apart. Small/maintainability.cmd/gadfly/opencode.go:196—append(openCodeEnv(), …)is less scannable than the claude path's two-step style.claudeEnvis called and assigned first (engine.go:158:cmd.Env = claudeEnv()), then conditionally appended to (engine.go:161:cmd.Env = append(cmd.Env, …)), making the "base env + explicit overrides" intent clear. Here it's inlined ascmd.Env = append(openCodeEnv(), "OPENCODE_CONFIG_CONTENT="+…, "NO_COLOR=1")— functionally fine but a trivial style inconsistency between the two engine implementations. Not worth a churn-inducing change on its own.The rest of the new file mirrors the established
claudeCodeEnginepatterns cleanly (constructor shape, pureargs()factoring,runPasscancel/timeout/process-group handling, test layout reusingargAfter/shSingleQuote/itoa), and the engine-selectionswitchinmain.gois a genuine readability improvement over the prior boolean. No dead code or leaky abstractions found.⚡ Performance — No material issues found
Verdict: No material issues found
I reviewed the new
opencode.goengine,main.gorouting, and theDockerfilewarm-up through the performance lens.runPass(opencode.go:185) builds a small JSON config, a tiny argv, and a filtered env slice, then spawns one CLI process — exactly mirroringclaudeCodeEngine.runPass(engine.go:155). One process per lens is the shell-out design, not a regression. Nothing here loops or does avoidable quadratic work.openCodeEnv()(opencode.go:243) rebuilds the env slice fromos.Environ()on every pass. In principle this could be hoisted tonewOpenCodeEngine, butos.Environ()is tens of entries and lens count is small (~5), so the cost is negligible — a micro-optimization I won't flag.config(system)(opencode.go:155) marshals JSON per pass, but the system prompt differs per lens so it correctly can't be hoisted; the object is tiny.bytes.Buffer(opencode.go:207) is unbounded, but this matches the existing claude-code pattern (engine.go:173) — not a new regression.timeout 120), but it's build-time, best-effort (; true), and against a dead localhost URL that fails fast on ECONNREFUSED rather than hanging — not a runtime hot-path concern.No runtime efficiency regressions, N+1 patterns, unbounded growth, or hot-loop waste introduced.
🧯 Error handling & edge cases — No material issues found
No material issues found
Through the error-handling & edge-case lens, the new
opencode.goengine is well-defended on its unhappy paths. I verified each against the checked-out code:opencode.go:215):ctx.Err()is checked beforerunErr, so a killed run surfaces as a context error rather than leaking partial stdout. Correct, and mirrors the claude-code engine.opencode.go:200-206):Setpgid+Kill(-pgid)is guarded by acmd.Process != nilcheck, so the cancel callback can't dereference a nil process if the binary never started. Good.opencode.go:190): a non-installedopencodeproduces aPathErrorfromcmd.Run(); therunErr != nilbranch formats it viatruncateForErr(verified atengine.go:252) and falls back to a bare error when stderr/stdout are empty. Surfaces as advisory, never panics.opencode.go:230-233): explicitly returns an error rather than"", so downstream consolidation won't treat a silent pass as a valid empty review.truncateForErr, which cuts on a rune boundary (verified), so a huge stderr dump can't emit invalid UTF-8 or bloat the comment.opencode.go:245):OPENCODE_CONFIG_CONTENTis explicitly dropped from the inherited env and only re-set byrunPass, avoiding the first-occurrence-wins duplicate-key problem the comment calls out.opencode/(trailing slash) and bareopencodeboth resolve toafter == ""→ bare/default-model mode viastrings.Cut(verified at:82), so no nil/empty-map issues inconfig()— theproviderModel != ""guard omits the provider block cleanly.args()task-last ordering: the task is the positional message and is always appended last;buildTaskprefixes it with literal text ("PR title:"/"Review …"per the comment), so a crafted PR title can't make the positional begin with-and be misparsed as a flag. Verified the comment's claim is consistent with the code shape.No swallowed errors, missing defers, nil-derefs, or unhandled boundary conditions found in this lens.
kimi-k2.6:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
The OpenCode engine implementation is security-conscious:
openCodeEnv()drops the runner's secrets (GITEA_TOKEN,ANTHROPIC_API_KEY,GADFLY_FINDINGS_TOKEN, etc.) and only forwardsOLLAMA_API_KEYplus CLI-relevant knobs.OPENCODE_CONFIG_CONTENTis explicitly blocked from inheritance sorunPasscontrols the only config source.edit/bashat both the globalpermissionlevel and the per-agentpermissionlevel, preventing a reviewed repo from re-enabling writes.args()appends the task as the final positional argument;buildTaskprepends"PR title:"/"Review…"prefixes, so the task cannot be misinterpreted as a CLI flag. Process group isolation (Setpgid) andSIGKILLon cancel prevent orphaned processes.apiKeyis always the env reference{env:OLLAMA_API_KEY}, never a baked-in value.I verified the env filtering logic by reading
cmd/gadfly/opencode.go:243-264, the command assembly at:107-114, and the config generation at:155-183. No authn/authz gaps, injection vectors, or secret leakage paths are present in the new code.🎯 Correctness — No material issues found
Verdict: No material issues found
After thorough verification of the opencode engine implementation (
cmd/gadfly/opencode.go,opencode_test.go,main.go, and integration points), I found no correctness defects in the logic introduced by this PR.Key correctness checks performed:
isOpenCodeSpecandnewOpenCodeEnginecorrectly handle all documented forms (opencode,opencode/<model>,opencode/<provider>/<model>,open-code/alias), withstrings.Cutsplitting on the first/so colons in ollama model IDs are preserved verbatim.omitempty), injects the system prompt as the agent's prompt, and deniesedit/bashat both global and agent levels. The API key is always the{env:OLLAMA_API_KEY}reference.openCodeEnvcorrectly withholds runner secrets (GITEA_TOKEN,ANTHROPIC_API_KEY, etc.) while passing throughOLLAMA_API_KEYand OpenCode-specific knobs.OPENCODE_CONFIG_CONTENTis explicitly filtered out to avoid ambiguity beforerunPasssets it.Setpgid: true+cmd.Cancelkilling the negative PID correctly prevents orphaned processes on timeout. Context cancellation is checked beforerunErrto surface timeout errors correctly.main.go: The switch correctly routes bothclaude-codeandopencodeprefixes to their shell-out engines, and theautoselection properly falls back to the default suite for non-majordomo engines.OLLAMA_CLOUD_API_KEY→OLLAMA_API_KEYmapping inscripts/run.shfeeds intoopenCodeEnv()and is referenced correctly in the generated config.🧹 Code cleanliness & maintainability — Minor issues
Minor issues found
cmd/gadfly/opencode.go:185/cmd/gadfly/engine.go:155— TherunPassimplementations forclaudeCodeEngineandopenCodeEngineduplicate ~40 lines of identical shell-out orchestration:Setpgid+SIGKILLprocess-group cleanup,bytes.Bufferstdout/stderr capture, thectx.Err()cancellation guard, and thetruncateForErrfallback chain. Any fix to timeout handling or orphan-process cleanup must be edited in two places. Suggested fix: Extract a shared helper such asrunShellOut(ctx, cmd *exec.Cmd) (stdout, stderr string, err error)inengine.gothat both engines call; keep the engine-specific JSON vs plain-text parsing in the callers.cmd/gadfly/opencode.go:243/cmd/gadfly/engine.go:228—openCodeEnvandclaudeEnvare near-copies of the same filtering loop with only thekeepclosure differing. Suggested fix: Extract afilterEnv(keep func(string) bool) []stringhelper inengine.goand have both env builders call it with their respective predicates.⚡ Performance — No material issues found
No material issues found
The opencode engine introduces no material performance regressions beyond the inherent cost of shelling out to a CLI (which is the intended design for benchmarking). The per-pass environment filtering and JSON config marshaling are minor overheads dwarfed by the subprocess execution time, and the existing concurrency controls (
lensConcurrency, entrypoint's per-provider caps) govern concurrent opencode processes the same way they govern claude-code processes. The unboundedbytes.Bufferfor stdout/stderr mirrors the existing claude-code engine pattern.🧯 Error handling & edge cases — No material issues found
No material issues found
The new
opencodeengine's error paths and edge cases look solid:runPasssurfaces context cancellation before examiningrunErr, preventing a timed-out run from returning partial stdout."".openCodeEnvexcludesOPENCODE_CONFIG_CONTENTto avoid duplicate-key ambiguity;runPassappends the generated config explicitly.Setpgid+syscall.Kill(-pid, SIGKILL)incmd.Cancelprevents orphaned Node child processes on timeout.isOpenCodeSpecrejects false positives like"opencode-extra".GADFLY_OPENCODE_MODELoverride runs through the same slash logic.main.gocorrectly falls back to the default suite for shell-out engines via type assertion (*majordomoEnginecheck).Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
🪰 Gadfly review —
opencode/glm-5.2:cloud(opencode)Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Minor issues
I've verified both findings against the actual code.
Finding 1 (line 208): Confirmed —
cmd.Dir = e.repoDir(line 206) means the CLI runs in the checked-out repo tree where OpenCode would discover anyopencode.json. The code asserts (lines 25-27, 208-210) thatOPENCODE_CONFIG_CONTENToutranks it, but there's no in-repo verification of that external precedence claim, and no integration test guarding it.Finding 2 (line 135): Confirmed —
denyMutationspins exactly the 5 keysedit/bash/webfetch/websearch/external_directory. The comment (lines 132-134) only addresses extra keys being ignored, not missing real mutating tool keys.Both findings accurately describe the code's actual state. Keeping both.
VERDICT: Minor issues
cmd/gadfly/opencode.go:206-211/cmd/gadfly/opencode.go:25-27— read-only enforcement depends on an unverified precedence claim. The CLI runs withcmd.Dir = e.repoDir(the checked-out PR tree, line 206), so OpenCode will discover anyopencode.jsonthe reviewed repo ships there. The only thing keeping a malicious repo from re-enablingedit/bashand mutating the checkout (or running arbitrary commands) during a review is the comment-asserted claim (lines 25-27, 208-210) thatOPENCODE_CONFIG_CONTENT"outranks anyopencode.json." This is the central security invariant and it is stated, not tested or verified in-repo. If opencode instead merges configs with the repo file winning for keys the generated config doesn't pin, or if a future opencode version changes precedence, a reviewed repo escapes read-only. Suggested fix: verify against opencode's current docs/source (link the precedence rule in a comment), and/or add an integration test that drops anopencode.jsonwithpermission:{edit:allow}inrepoDirand asserts the generated config wins; as defense-in-depth, also pass--configpointing at a path the repo can't shadow, or chroot the CLI to a dir without the repo's config.cmd/gadfly/opencode.go:135-143— the deny-list is asserted complete but isn't provably so.denyMutationspins exactlyedit/bash/webfetch/websearch/external_directory. The comment (lines 132-134) defends unknown keys ("a key it doesn't recognize is simply ignored, so listing extras is safe") but that addresses extra keys, not missing real ones. If opencode gates any mutating/command tool under a different permission key (e.g. awrite/create/delete/task/MCP-tool key), it would be allowed by default and a repo could use it. Suggested fix: confirm against opencode's enumerated permission keys (the URL in the comment,https://opencode.ai/docs/agents) and either deny every mutating key explicitly or use an allow-list (read/grep/glob/list/lsp→ allow, everything else → deny) if opencode supports a catch-all, which is more robust than a deny-list against a third-party tool surface you don't control.🎯 Correctness — No material issues found
Let me verify one more potential correctness concern around the OpenCode config schema and the
runsubcommand.Let me check a couple more potential correctness concerns around the recheck and consolidation integration with the new engine.
Now let me verify the
provider_oflane behavior with the opencode prefix — there could be a lane mismatch issue.Let me look at the Dockerfile change more carefully for the build correctness.
I've thoroughly reviewed the change through the correctness lens, reading the new engine, its tests, the refactored engine.go, main.go's engine selection switch, the recheck/consolidation integration paths, the entrypoint/run.sh provider-lane wiring, the Dockerfile, and the workflow YAML changes.
Verdict: No material issues found
I verified the following potential correctness concerns and they all check out:
Spec parsing in
newOpenCodeEngine(opencode.go:82-98):strings.Cuton the first/correctly handles all three forms — bare (opencode), wrapped (opencode/glm-5.2), pass-through (opencode/anthropic/claude). Colon-bearing ids likeqwen3-coder:480b-cloudsurvive because there's no:-suffix parsing (unlike claude-code). Theopen-code/alias works becauseCutdiscards the prefix.GADFLY_OPENCODE_MODELoverride runs through the same slash logic per the doc. ✅argsordering (opencode.go:107-114):run --agent gadfly --model <ref> [extra] <task-last>. Task is a single argv element (no shell splitting), andbuildTask/buildRecheckTaskoutputs never begin with-, so no--terminator is needed. ✅Config generation (opencode.go:170-198):
provider,omitemptycorrectly omits the block for bare/pass-through specs;Agent(no omitempty) is always present. TheModelsmap marshals to{"model":{}}. JSON struct tags are correct. The API key is always the{env:OLLAMA_API_KEY}reference, never a literal. ✅Context cancellation (opencode.go:221-223):
ctx.Err()is checked beforerunErr, so a timed-out run surfaces as an error even if the CLI flushed partial stdout — matches the claude engine.killGroupOnCancel(engine.go:217-225) correctly setsSetpgidand kills-PID(the whole group), with acmd.Process != nilguard. ✅main.goengine switch (main.go:160-189): theswitchcorrectly routes to claude-code, opencode, or majordomo. The auto-select path (main.go:200-219) now uses a*majordomoEnginetype assertion instead of a boolean, so a shell-out engine can never hit the oldeng.(*majordomoEngine)panic. ✅Recheck integration (main.go:344-352): the opencode engine handles the recheck pass identically to the review pass —
recheckSystemPromptbecomes the agent prompt in a freshly generated config. No engine-specific code needed. ✅Provider lane consistency:
entrypoint.sh:240provider_ofandmodel.go:167modelProvider()both extract the first path segment, soopencode/glm-5.2lands in theopencodelane consistently in both the entrypoint (concurrency) and the binary (lens concurrency).run.shhardcodesPROVIDER=ollama(run.sh:263), so opencode specs enter theollamacase and get theOLLAMA_CLOUD_API_KEY → OLLAMA_API_KEYmapping (run.sh:160-161) that the generated provider's{env:OLLAMA_API_KEY}reference needs. ✅openCodeEnvfiltering (opencode.go:254-272):OPENCODE_CONFIG_CONTENTis explicitly excluded (line 256-258), avoiding a duplicate-key ambiguity sincerunPasssets it explicitly.OLLAMA_API_KEYis kept; gadfly's own secrets are dropped. ✅filterEnvrefactor (engine.go:230-238): the extracted helper preserves the exact prior behavior ofclaudeEnv; the keep predicate is unchanged. ✅Dogfood caller fix (adversarial-review.yml:58):
pr_number: ${{ github.event.inputs.pr_number }}correctly threads the dispatch input through the reusable'sinputs.pr_number, which the reusable reads at line 116 via the||chain. Forpull_requestevents,github.event.pull_request.numberis still propagated (only dispatch inputs are dropped by Gitea ≥1.27), so the reusable'sPRenv is populated and the entrypoint'sworkflow_callreclassification (entrypoint.sh:129-136) works. ✅The only things I cannot verify from the repo are external OpenCode CLI behaviors (the
run --agent --modelflag surface, thepermissionschema keys,OPENCODE_CONFIG_CONTENTprecedence overopencode.json,Mode: "primary"). These are domain assumptions about a third-party CLI, not code-internal logic, and the README honestly documents the engine as "newly wired, lightly tested." None of them are code-correctness bugs I can flag from the checked-out source.🧹 Code cleanliness & maintainability — No material issues found
Now let me check the consolidated structure of the two engines side by side for code-cleanliness observations.
Let me verify a couple of specific cleanliness concerns.
I reviewed the new
opencode.go, the refactoredengine.go,main.go's engine switch, and the test file against the surrounding code patterns. The change is clean: thefilterEnv/killGroupOnCancelextraction is exactly the right dedup, the new engine mirrorsclaudeCodeEngine's structure (struct shape,args()factored out for testability,runPasserror-handling shape, env-filtering helper), and the doc comments carry the same voice and density as the rest of the package.Verdict: No material issues found
filterEnv/killGroupOnCancelare now shared by both shell-out engines and that the old inlined duplication inclaudeCodeEngine.runPassis gone (engine.go:163,engine.go:217-225,engine.go:230-238). The extraction is well-scoped — no leaky abstraction.openCodeEnvandclaudeEnvnow genuinely "differ only in their keep predicate" as the doc claims (engine.go:228-229); the predicates are structurally parallel (exact-nameswitch+HasPrefixchain). ConfirmedOPENCODE_CONFIG_CONTENTis dropped by the filter and then re-appended byrunPass(opencode.go:211,opencode.go:256-258), so there is no duplicate-key ambiguity the comment warns about.openCodeConfig/openCodeProvider/openCodeAgentstructs are minimal and the JSON tags match the field names used in the test's round-trip (opencode_test.go:128-168).denyMutations()is built once and reused for both the global and agent permission maps, which is correct sincejson.Marshaldoesn't mutate (opencode.go:171-182).opencode.go:15lists OpenCode's read tools asread/grep/glob/listwhileopencode.go:131lists them asread/glob/grep/list/lsp(different order, and the second includeslsp). Both are prose, but they describe the same set — trivial doc inconsistency only.⚡ Performance — No material issues found
Verdict: No material issues found
I reviewed
cmd/gadfly/opencode.go,engine.go,main.go, and the Dockerfile/entrypoint through the performance lens. The new shell-out engine follows the same patterns as the existingclaudeCodeEngine, and I found no material efficiency regressions.What I checked and why it's fine:
cmd/gadfly/opencode.go:200runPass— per pass it callse.config(system)(small struct JSON marshal),e.args(task)(slice build),openCodeEnv()(oneos.Environ()scan of a few-hundred-entry env, filtered into a fresh slice), and oneexec.Cmd. All of this is dwarfed by the LLM round-trip the CLI makes; not a hot-loop concern. VerifiedopenCodeEnv()returns a fresh slice each call (opencode.go:254), so theappend(openCodeEnv(), …)on line 211 cannot alias/corrupt a shared backing array.cmd/gadfly/opencode.go:170config—denyMutations()allocates a fresh 5-entry map per call and is invoked once per pass. Negligible.buildTask(main.go:364) still truncates the embedded diff toGADFLY_MAX_DIFF_CHARS(default 60000), andentrypoint.sh:220downshifts huge PRs aboveGADFLY_HUGE_DIFF_BYTES. So the new engine inherits the same size-gating already in place. Not a regression introduced here.Dockerfile:52warm-uptimeout 120 opencode run …— points at127.0.0.1:9(dead), so it fails on connection refused rather than burning the full 120s; build-time-only, best-effort (; true), and doesn't affect runtime. Not a runtime perf issue.cmd/gadfly/opencode.go:213unboundedbytes.Bufferfor stdout/stderr — this is the same pattern asclaudeCodeEngine(engine.go:164), bounded ultimately by the model's response length per pass. Standard; not flagging as a regression.One low-confidence observation (not rising to a finding): OpenCode's default text output mode may emit intermediate agent/tool traces to stdout where claude-code emits a single JSON envelope, so per-pass stdout could be larger in memory. I couldn't verify OpenCode's actual output volume with the tools available, and it's bounded per pass regardless, so I'm not reporting it as a real issue — just noting the README already flags this engine as "newly wired, lightly tested."
🧯 Error handling & edge cases — Minor issues
Confirmed. The finding survives verification:
examples/reusable.ymldeclaresworkflow_dispatch.inputs.pr_number(line 35) but itswith:block (lines 64-71) never threadspr_number: ${{ github.event.inputs.pr_number }}.PR:env (review-reusable.yml:116) falls back toinputs.pr_number, but that input is only populated when the caller passes it viawith:.pr_number: ${{ github.event.inputs.pr_number }}."adversarial-review.yml:58) DOES thread it — this PR's commit 2 added exactly that line.workflow_dispatchthroughexamples/reusable.yml→ all four fallbacks inPR:are empty →entrypoint.sh:70(: "${PR:?PR required}") fails the job. Auto PR/comment triggers unaffected (they populategithub.event.pull_request.number/github.event.issue.number).CLAUDE.mdmaintenance rules require examples to be kept current in the same change that fixes the consumer-stub contract.VERDICT: Minor issues
examples/reusable.yml— manualworkflow_dispatcharrives with an empty PR (the same bug commit 2 fixes for the dogfood caller). This is the README's recommended "slimmest stub" and it calls the reusable workflow. It declaresworkflow_dispatch.inputs.pr_number(line 35) but itswith:block (lines 64-71) never threadspr_number: ${{ github.event.inputs.pr_number }}. The reusable'sPR:env (review-reusable.yml:116) falls back toinputs.pr_number, but that input is only populated when the caller passes it viawith:— which this example doesn't. Under Gitea ≥1.27 (the breaking change cited in commit 2), a manual dispatch through this stub reaches the container withPR="", andentrypoint.sh:70(: "${PR:?PR required}") fails the job with "PR required". Auto PR/comment triggers are unaffected (they populategithub.event.pull_request.number/github.event.issue.number), so only the manual-dispatch unhappy path is broken. Verified by readingexamples/reusable.yml:28-71andreview-reusable.yml:116,62-66. Fix: addpr_number: ${{ github.event.inputs.pr_number }}to thewith:block (mirroringadversarial-review.yml:58). The maintenance rules inCLAUDE.mdexplicitly require examples to be kept current in the same change that fixes the consumer-stub contract.Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 25m 54s