33 Commits
Author SHA1 Message Date
steveandClaude Opus 5 14f8533e38 fix(ci): scrub the registry credential before running repo code
Build & push image / build-and-push (pull_request) Successful in 4s
Build & push image / test (pull_request) Successful in 9m47s
Both Claude reviewers caught this independently, and they are right. The test
job I added wrote a PUSH-CAPABLE REGISTRY_PASSWORD into a plaintext
~/.gitconfig and then ran `go build`/`go vet`/`go test` — repository code — on
pull_request events. This repo is public, so a fork PR could ship a test whose
only job is to print that file. The image build had already answered this
question correctly: its credentials are BuildKit secrets scoped to the
module-download RUN and are never present while code executes. I bolted on a
job that skipped the boundary its neighbour maintains.

Dependencies are now fetched in their own step which deletes ~/.gitconfig
before anything else runs, and asserts the scrub — against the whole home
directory, not against the file it just removed, because the credential can
also land in ~/.netrc or ~/.config/go/env. Verified the assertion is not
vacuous: planting the secret in ~/.netrc trips it. Later steps run with
GOPROXY=off, so any attempt to reach the network fails loudly rather than
quietly hunting for the credential that is now gone.

Also from round 4: TestEndpointProviderNamesAreAllAccepted pinned only
endpointProvider, while the constant is the error text for BOTH resolution
paths — it now asserts each advertised name resolves either way (break-checked
by dropping the gemini alias from resolveModel alone). preflight.sh documents
that ollama-cloud is checked on OLLAMA_API_KEY but hinted as
OLLAMA_CLOUD_API_KEY because run.sh copies one to the other first, an ordering
dependency that was invisible from the file.

And the comments that narrated this PR's own edit history ("the first version
of this change...") are rewritten as invariants. That history stops being true
the moment this merges, and the repo's doc policy says as much.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
2026-08-12 17:28:08 -04:00
steveandClaude Opus 5 ebfaeba07e docs(qwen): warn that Model Studio keys are endpoint-scoped
A workspace-scoped Qwen endpoint rejects a key issued for the shared
international host with a genuine 'Incorrect API key provided', so a valid key
reads as invalid and the obvious next move — checking the key — confirms it is
fine and leads nowhere. Watched this cost real debugging time on a live
deployment today; gadfly would hit it identically. Documents the
GADFLY_ENDPOINT_* form, which reaches a workspace host with no code change.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
2026-08-12 17:22:08 -04:00
steveandClaude Opus 5 67a73616e1 fix(qwen): gadfly round 3 — stop guarding a duplicate, delete it
Build & push image / build-and-push (pull_request) Successful in 5s
Build & push image / test (pull_request) Canceled after 7m27s
Twelve findings, all real, and the two that matter are about the pre-flight I
added rather than about qwen.

The credential check had a false pass in the OTHER direction from round 2's: on
the GADFLY_BASE_URL override path, resolveModel builds the client with
GADFLY_API_KEY and never reads QWEN_API_KEY/KIMI_API_KEY, so treating the
provider's own key as sufficient there let a doomed run proceed. Having now
been wrong about these rules in both directions, the check no longer tries to
model both paths: it covers the REGISTRY path, whose rules it can state
exactly, and says nothing about the override path — which is hand-configured by
definition, while the registry path is the one you hit by adding a model id to
a var and forgetting the secret.

The logic moves to scripts/preflight.sh, sourced by both run.sh and the test.
The previous answer to "this test duplicates production logic" was a regex
drift-guard, and that guard compared only the provider table — not the decision
logic, which is precisely the half that carried the bug. A duplicate you guard
is still a duplicate; this deletes it, and the test now runs under `set -u`
like production does.

Also: the test that pins the shared provider slice held its own copy of the
list (now ranges the slice); endpointProviderNames had nothing tying it to the
switches it describes, which is how it shipped without "gemini" (a new test
asserts every advertised name resolves); two godoc lists had drifted; and the
"sanity" line that asserted nothing is gone.

And the repo had NO test job — `go test` and the pre-flight table both existed
and neither was ever executed by CI, which reads as coverage while providing
none. Added one (build/vet/gofmt/test/pre-flight), running alongside the image
build rather than gating it, so red is loud without standing between a push and
a rebuild.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
2026-08-12 17:20:44 -04:00
steveandClaude Opus 5 1d6eaa08c5 fix(qwen): gadfly round 2 — the anti-drift list had already drifted
Build & push image / build-and-push (pull_request) Successful in 3s
Eight findings, all real, and the sharpest ones are about this PR's own fixes.

GADFLY_API_KEY was treated as a universal substitute in the pre-flight. It is
not: resolveModel reads it only AFTER the `baseURL == ""` early return, so on
the registry path — the documented primary path — a qwen/kimi built-in reads
its own variable and GADFLY_API_KEY is never consulted. A mis-set
GADFLY_API_KEY therefore passed pre-flight and 401'd five times anyway, which
is precisely the failure this check exists to prevent. It now only substitutes
when GADFLY_BASE_URL is also set.

`openai-compatible` was missing from the pre-flight table while both switches
accept it as an OPENAI_API_KEY alias, so that one spelling still fell through
to the cryptic five-failure mode.

endpointProviderNames — the constant I introduced *to stop* the two error
messages drifting — omitted the `gemini` alias both switches accept. It now
lists every accepted spelling.

And the case list itself was still duplicated across both switches plus the
test that pins them: three copies of the thing whose duplication started this.
Both switches now call isOpenAICompatProvider over one shared slice, and
endpointProvider's doc comment points at endpointProviderNames instead of
carrying a fourth hand-written copy.

scripts/preflight_test.sh moves into the repo (20 cases, up from 17, covering
openai-compatible and both GADFLY_API_KEY directions). It carries a drift guard
that diffs its copy of the provider table against run.sh's and aborts if they
differ — break-checked by deleting an arm from run.sh, which fails it loudly.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
2026-08-12 16:59:01 -04:00
steveandClaude Opus 5 2367e696b5 fix(qwen): gadfly round 1 — three real findings, all sibling drift
Build & push image / build-and-push (pull_request) Successful in 3s
The pre-flight comment was the worst of them, and three models agreed. It said
providers absent from the table "need no key or carry it in their endpoint/DSN"
— false for google, which needs a key and is absent for an entirely different
reason: it accepts GOOGLE_API_KEY *or* GEMINI_API_KEY, so a single-variable arm
would silently skip a correctly-configured reviewer. That reasoning was in the
PR description and not in the code, so the comment invited exactly the wrong
edit. It now states both exclusion reasons and names google's.

Forwarded KIMI_API_KEY alongside QWEN_API_KEY in the dogfooding stub. This PR
argues that sibling call sites must move together, and I declared both secrets
in the reusable workflow and forwarded one — a config that looks complete and
401s on the model you didn't wire.

The two endpoint-provider error messages listed the same accepted set in
different order and spelling. Both functions accept an identical set, so they
now share one endpointProviderNames constant and cannot disagree.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
2026-08-12 16:43:00 -04:00
steveandClaude Opus 5 0f40b21d79 feat(qwen): let Qwen (and Kimi) join the swarm
Build & push image / build-and-push (pull_request) Successful in 14s
Gadfly review (reusable) / review (pull_request) Successful in 8m46s
Adversarial Review (Gadfly) / review (pull_request) Successful in 8m46s
majordomo now ships qwen and kimi as built-ins that ARE the openai client at
their own base URL, so "qwen/qwen3.8-max" works as a GADFLY_MODELS entry once
the key reaches the container. This wires up the parts that key has to pass
through.

Two provider switches had to learn the names, not one. resolveModel's
GADFLY_BASE_URL override was the obvious one; endpointProvider's
GADFLY_ENDPOINT_* parser is its sibling, and I fixed the first and missed the
second on the first pass — a config that resolves one way and errors the other
for no reason a user could guess. TestOpenAICompatProvidersResolveOnBothPaths
now asserts both from one table so the pair fails together; break-checked in
both directions.

QWEN_API_KEY (and KIMI_API_KEY) are declared as workflow_call secrets and
forwarded to the container, with gadfly's own stub forwarding QWEN_API_KEY so a
qwen entry can join the default swarm by editing GADFLY_DEFAULT_MODELS alone —
no workflow edit, no re-release.

The run.sh credential pre-flight is now a provider→variable table instead of an
ollama-cloud special case. Without it a forgotten key surfaces as five
identical per-lens agent failures naming no variable, and the operator reads a
stack trace to find out which secret they missed. Google stays out of the table
on purpose: it accepts either GOOGLE_API_KEY or GEMINI_API_KEY, and a one-var
entry would wrongly skip a correctly-configured run. Verified across 17
provider x key-state combinations, including that a wrong-provider key never
satisfies qwen (majordomo refuses cross-provider fallback) and that unkeyed
providers are never blocked.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
2026-08-12 16:30:29 -04:00
steveandClaude Opus 4.8 c9dab69d14 chore(reusable): pin the reviewer image to sha-b37cd09 [skip ci]
Adopt the provider-wide lens budget build (PR #27) for gadfly's own reviews
and for consumers pinning this reusable. No image rebuild needed — the tag
already exists — so skip CI.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
2026-07-18 12:50:20 -04:00
steve b37cd09dc9 Merge pull request 'feat(concurrency): provider-wide lens budget, drop the model cap' (#27) from feat/provider-wide-lens-budget into main
Build & push image / build-and-push (push) Successful in 6s
2026-07-18 16:48:26 +00:00
steveandClaude Opus 4.8 6a74b64c7a fix(concurrency): address gadfly review — fail-open pool + doc drift
Build & push image / build-and-push (pull_request) Successful in 4s
Gadfly's own review of #27 surfaced a real robustness cluster (5 models,
error-handling) plus stale comments I missed.

Robustness — the flock permit pool could hang forever:
- tryAcquire swallowed os.OpenFile errors and treated every flock error as
  "busy", so a broken/missing pool dir (or a filesystem without flock) would
  spin-poll indefinitely; the fanout context is uncancellable and the per-lens
  timeout only starts AFTER acquire returns. Can't trigger in the deploy
  (entrypoint mkdir -p's the dir) but fixed defensively.
- tryAcquire now returns a structural error, distinguished from a healthy-full
  pool (EWOULDBLOCK = busy → keep polling). acquire FAILS OPEN on a structural
  error: logs once and runs the lens unthrottled rather than hanging the review.
- acquire uses time.NewTimer + Stop() (no per-poll timer leak on cancellation).
- activeLensSem warns on stderr when GADFLY_LENS_SEM_DIR is set but the size is
  invalid (was a silent degrade to unthrottled).
- New test: a broken pool dir fails open promptly.

Doc drift (stale references to the removed model cap):
- main.go defaultLensConcurrency + runSpecialists doc, entrypoint.sh status
  pre-seed + lane-launch comments, and the pre-existing lens_concurrency_test.go
  header all updated to the provider-wide-budget wording.

Accepted (graded real, not changed): all-models-start-at-once startup burst
(intended tradeoff) and index-0 sweep bias (cosmetic). One false positive
(one model using the whole budget is the intended lone-model behavior).

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
2026-07-18 12:47:36 -04:00
steveandClaude Opus 4.8 74831368ab feat(concurrency): provider-wide lens budget, drop the model cap
Build & push image / build-and-push (pull_request) Successful in 4s
Gadfly review (reusable) / review (pull_request) Successful in 14m47s
Adversarial Review (Gadfly) / review (pull_request) Successful in 14m47s
Concurrency was two multiplicative gates in two processes: entrypoint.sh
capped MODELS-at-once per provider (GADFLY_PROVIDER_CONCURRENCY) while each
model's binary separately capped its own lenses (GADFLY_LENS_CONCURRENCY).
A model therefore held its whole model-slot until its LAST lens finished,
stalling the next model even with idle lens capacity.

Collapse to one throttle: a provider-wide lens budget shared across all of
that provider's models. entrypoint now runs every model in a lane at once and
seeds a single cross-process permit pool per lane (a dir of N flock files,
sized by GADFLY_PROVIDER_LENS_CONCURRENCY -> GADFLY_LENS_CONCURRENCY). Each
lens pass (review+recheck) acquires a permit before it runs and releases it
after, so a model winding down immediately yields its freed permits to
another model's queued lenses. flock auto-releases on process death, so a
killed/crashed model can't leak budget.

- cmd/gadfly/lenssem.go: the flock permit pool (+ lenssem_test.go).
- main.go: runSpecialists holds a shared permit per lens; fanout sized to the
  budget so a lone model can use all of it. Falls back to the in-process limit
  when no pool is set (local runs, tests).
- entrypoint.sh: drop provider_cap/DEFAULT_CONC; run_lane runs all models and
  seeds the per-lane pool.
- GADFLY_PROVIDER_CONCURRENCY / GADFLY_CONCURRENCY are now ignored; the
  reusable workflow marks provider_concurrency deprecated and stops forwarding
  it. Docs (README, CLAUDE.md, examples) updated per the maintenance rule.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
2026-07-18 12:22:11 -04:00
steveandClaude Opus 4.8 0d51879450 chore(reusable): bump reviewer image to sha-bb98fae (opencode-capable)
Build & push image / build-and-push (push) Successful in 5s
sha-bb98fae is the image built from the PR #26 merge, so it bundles the opencode
CLI and can run the new opencode/<model> engine. Consumers pinned to the gadfly
ref of this commit inherit it. Previous pin (sha-f468fe6) predated opencode.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
2026-07-18 01:56:17 -04:00
steve bb98fae5f0 Merge pull request 'feat(engine): add opencode CLI review engine' (#26) from feat/opencode-engine into main
Build & push image / build-and-push (push) Successful in 41s
2026-07-18 05:51:51 +00:00
steveandClaude Opus 4.8 2477e50230 fix(opencode): address gadfly's dogfood review
Build & push image / build-and-push (pull_request) Successful in 8s
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]>
2026-07-18 01:29:29 -04:00
steveandClaude Opus 4.8 463aa01ddb fix(dogfood): bump reusable image pin past the Gitea 1.27 reclassification
Build & push image / build-and-push (pull_request) Successful in 4s
Gadfly review (reusable) / review (pull_request) Successful in 5m25s
Adversarial Review (Gadfly) / review (pull_request) Successful in 5m25s
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]>
2026-07-18 01:14:27 -04:00
steveandClaude Opus 4.8 3973e469a8 fix(dogfood): thread dispatch pr_number into gadfly's own review caller
Gadfly review (reusable) / review (pull_request) Successful in 1s
Adversarial Review (Gadfly) / review (pull_request) Successful in 1s
Build & push image / build-and-push (pull_request) Successful in 2m13s
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]>
2026-07-18 01:07:56 -04:00
steveandClaude Opus 4.8 5ab4074e9c feat(engine): add opencode CLI review engine
Gadfly review (reusable) / review (pull_request) Successful in 5s
Adversarial Review (Gadfly) / review (pull_request) Successful in 5s
Build & push image / build-and-push (pull_request) Successful in 2m43s
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]>
2026-07-18 01:00:54 -04:00
steve f468fe6245 Merge pull request 'fix(reusable): thread the dispatch pr_number as a workflow_call input' (#24) from fix/dispatch-pr-input into main
Build & push image / build-and-push (push) Successful in 3s
2026-07-16 03:34:29 +00:00
steveandClaude Fable 5 64d34bd33b fix(reusable): thread the dispatch pr_number as a workflow_call input
Gadfly review (reusable) / review (pull_request) Successful in 1s
Adversarial Review (Gadfly) / review (pull_request) Successful in 1s
Build & push image / build-and-push (pull_request) Successful in 3s
Gitea >= 1.27 does not propagate the caller's workflow_dispatch inputs
into a called workflow's github.event (same rework that changed
event_name), so a manual 'review PR #N' dispatch arrived with an empty
PR and died at the entrypoint's 'PR required' check. Accept pr_number as
an explicit workflow_call input and fold it into the PR env fallback
chain; caller stubs pass github.event.inputs.pr_number through.

Co-Authored-By: Claude Fable 5 <[email protected]>
2026-07-15 23:34:07 -04:00
steve d8580bc193 Merge pull request 'chore(reusable): replace the retired ragnaros endpoint with netherstorm' (#23) from chore/ragnaros-to-netherstorm into main
Build & push image / build-and-push (push) Successful in 18s
2026-07-16 03:32:18 +00:00
steveandClaude Fable 5 24d1ee1ebd chore(reusable): replace the retired ragnaros endpoint with netherstorm
Build & push image / build-and-push (pull_request) Successful in 3s
Gadfly review (reusable) / review (pull_request) Successful in 5s
Adversarial Review (Gadfly) / review (pull_request) Successful in 5s
GADFLY_ENDPOINT_RAGNAROS is empty in Gitea vars and no ragnaros/<model>
is in the pool; netherstorm is the live local GPU endpoint. Requested by
Steve.

Co-Authored-By: Claude Fable 5 <[email protected]>
2026-07-15 23:31:57 -04:00
steve 56392368dd Merge pull request 'chore(reusable): forward GADFLY_ENDPOINT_NETHERSTORM' (#22) from chore/forward-netherstorm-endpoint into main
Build & push image / build-and-push (push) Successful in 8s
2026-07-16 03:30:43 +00:00
steveandClaude Fable 5 4ff63d988a chore(reusable): forward GADFLY_ENDPOINT_NETHERSTORM
Gadfly review (reusable) / review (pull_request) Successful in 0s
Adversarial Review (Gadfly) / review (pull_request) Successful in 0s
Build & push image / build-and-push (pull_request) Successful in 26s
A reusable workflow can't enumerate arbitrary vars.GADFLY_ENDPOINT_*;
the netherstorm endpoint var was added after the forwarding list, so
every netherstorm/<model> reviewer failed with 'unknown provider:
netherstorm' — the correctly-formatted var never reached the container.

Co-Authored-By: Claude Fable 5 <[email protected]>
2026-07-15 23:30:17 -04:00
steve 256344d3e1 Merge pull request 'fix: handle Gitea 1.27's workflow_call event name in the trigger gate' (#21) from fix/gitea-127-workflow-call into main
Build & push image / build-and-push (push) Successful in 2m30s
2026-07-16 03:00:40 +00:00
steveandClaude Fable 5 9d74cb9c81 fix: handle Gitea 1.27's workflow_call event name in the trigger gate
Gadfly review (reusable) / review (pull_request) Successful in 28s
Adversarial Review (Gadfly) / review (pull_request) Successful in 28s
Build & push image / build-and-push (pull_request) Successful in 2m39s
Gitea 1.27 (breaking change go-gitea#37478) runs a called (reusable)
workflow with github.event_name = 'workflow_call' instead of propagating
the caller's event. Every consumer stub forwards EVENT_NAME from
github.event_name, so since the server upgrade every review arrived as an
unhandled event and self-skipped in one second while reporting success —
mort PRs #1445-#1447 all went unreviewed.

Reclassify workflow_call from the forwarded payload: a non-empty
COMMENT_BODY can only come from issue_comment (trigger-phrase + actor
gates still apply); otherwise a PR number means a pull_request-shaped
trigger. Neither → the existing unhandled-event skip. Pre-1.27 servers
are unaffected.

Co-Authored-By: Claude Fable 5 <[email protected]>
2026-07-15 23:00:09 -04:00
steve ac6ce06cdd feat: re-platform agentic review onto executus + large-PR cost controls (#20)
Build & push image / build-and-push (push) Successful in 33s
Makes gadfly a consumer of executus (run.Executor compaction/bounding/budget/critic + fanout) and fixes the large-PR token burn in size-gated layers: paginated get_diff, downshift above GADFLY_HUGE_DIFF_BYTES, and a swarm-wide GADFLY_PR_BUDGET_SECS backstop. Small PRs untouched; advisory-only and the static binary preserved. Dogfood swarm reviewed it (6 models, 21 real findings graded + folded in).

Co-authored-by: Steve Dudenhoeffer <[email protected]>
Co-committed-by: Steve Dudenhoeffer <[email protected]>
2026-06-30 15:41:03 +00:00
steveandClaude Opus 4.8 5007597cf9 chore(reusable): bump image pin to sha-3095ebf (inline PR review live)
Phase 3: gadfly's own multi-model reviews now also post a COMMENT-state PR
review with inline comments anchored to changed lines. External consumers
re-pin separately.

[skip ci]

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
2026-06-28 22:00:39 -04:00
steve 3095ebff23 feat: inline COMMENT-state PR review (findings anchored to changed lines) (#18)
Build & push image / build-and-push (push) Successful in 8s
Co-authored-by: Steve Dudenhoeffer <[email protected]>
Co-committed-by: Steve Dudenhoeffer <[email protected]>
2026-06-29 01:59:36 +00:00
steveandClaude Opus 4.8 8f5adc91b2 chore(reusable): bump image pin to sha-88f74aa (consensus consolidation live)
Phase 2: gadfly's own multi-model reviews now post ONE cross-model consensus
comment instead of N per-model comments. External consumers re-pin separately.

[skip ci]

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
2026-06-28 18:57:38 -04:00
steve 88f74aa768 feat: cross-model consensus consolidation (one ranked comment, not N walls) (#17)
Build & push image / build-and-push (push) Successful in 9s
Co-authored-by: Steve Dudenhoeffer <[email protected]>
Co-committed-by: Steve Dudenhoeffer <[email protected]>
2026-06-28 22:56:15 +00:00
steveandClaude Opus 4.8 84b891b1ba chore(reusable): bump image pin to sha-5397160 (structured findings contract)
Makes the Phase 1 gadfly-findings contract live for gadfly's own dogfood
reviews (the local-ref reusable). External consumers re-pin separately.

[skip ci]

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
2026-06-28 18:25:16 -04:00
steve 53971603d3 feat: structured findings contract (machine-readable gadfly-findings block) (#16)
Build & push image / build-and-push (push) Successful in 5s
Co-authored-by: Steve Dudenhoeffer <[email protected]>
Co-committed-by: Steve Dudenhoeffer <[email protected]>
2026-06-28 22:23:02 +00:00
steve f49699fc12 Merge pull request 'docs: correct examples/reusable.yml pin guidance (prefer @sha; runners cache @v1)' (#15) from test/trigger-check into main
Reviewed-on: #15
2026-06-28 22:09:07 +00:00
Steve DudenhoefferandClaude Opus 4.8 6e87a3e73f docs: correct examples/reusable.yml pin guidance (runners cache @v1; prefer @sha)
Adversarial Review (Gadfly) / review (pull_request) Successful in 3m4s
The @v1 comment claimed it auto-updates on releases, but long-lived act_runners
cache the reusable by ref so a moved tag isn't re-fetched. Recommend an
immutable @<sha>; routine tuning rides owner variables.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
2026-06-28 02:10:35 -04:00
42 changed files with 4451 additions and 452 deletions
+13
View File
@@ -46,6 +46,16 @@ jobs:
secrets:
OLLAMA_CLOUD_API_KEY: ${{ secrets.OLLAMA_CLOUD_API_KEY }}
CLAUDE_CODE_OAUTH_TOKEN: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }}
# Forwarded so a "qwen/<model>" or "kimi/<model>" entry can join the
# swarm by editing the GADFLY_DEFAULT_MODELS var alone — no workflow
# edit, no re-release. Both are forwarded together on purpose: the
# reusable workflow declares both, and forwarding only one is a config
# that looks complete and 401s on the model you didn't wire. Empty until
# the repo secret exists, which is a 401 on that one model, not a broken
# review. NB kimi/<model> is Moonshot's own API — a different route than
# the kimi-k2.6:cloud swarm entry, which rides OLLAMA_CLOUD_API_KEY.
QWEN_API_KEY: ${{ secrets.QWEN_API_KEY }}
KIMI_API_KEY: ${{ secrets.KIMI_API_KEY }}
GADFLY_FINDINGS_URL: ${{ secrets.GADFLY_FINDINGS_URL }}
GADFLY_FINDINGS_TOKEN: ${{ secrets.GADFLY_FINDINGS_TOKEN }}
with:
@@ -53,3 +63,6 @@ jobs:
# 5-lens suite) from review-reusable.yml. Only the consumer-specific
# allow-list is set here.
allowed_users: "steve,fizi,dazed"
# Gitea >= 1.27 does not propagate dispatch inputs into a called workflow's
# github.event — thread the PR number explicitly (empty on non-dispatch events).
pr_number: ${{ github.event.inputs.pr_number }}
+55
View File
@@ -45,6 +45,61 @@ env:
IMAGE_NAME: gitea.stevedudenhoeffer.com/steve/gadfly
jobs:
# Runs alongside the image build rather than gating it: a red test should be
# loud on the PR without standing between Steve and a rebuild. Added because
# this repo had NO test job at all — `go test` and scripts/preflight_test.sh
# both existed and neither was ever executed by CI, which is worse than
# having no tests, since it reads as coverage.
test:
runs-on: ubuntu-latest
timeout-minutes: 15
steps:
- uses: actions/checkout@v4
- uses: actions/setup-go@v5
with:
go-version-file: go.mod
# Fetch dependencies, then DESTROY the credential before any step that
# executes repository code. REGISTRY_PASSWORD is push-capable, this repo
# is public so pull_request runs can carry attacker-authored code, and
# `go test` runs that code — a plaintext ~/.gitconfig left in place is a
# credential any test could print. The image build faces the same
# question and answers it the same way: its creds are BuildKit secrets
# scoped to the module-download RUN, never present while code runs.
- name: Fetch private modules
env:
REGISTRY_USER: ${{ secrets.REGISTRY_USER }}
REGISTRY_PASSWORD: ${{ secrets.REGISTRY_PASSWORD }}
run: |
go env -w GOPRIVATE=gitea.stevedudenhoeffer.com/*
git config --global url."https://${REGISTRY_USER}:${REGISTRY_PASSWORD}@gitea.stevedudenhoeffer.com/".insteadOf "https://gitea.stevedudenhoeffer.com/"
go mod download
rm -f "$HOME/.gitconfig"
# Prove the scrub worked, and prove it against the whole home dir —
# checking only the file just deleted would pass no matter what, and
# the credential can also reach ~/.netrc or ~/.config/go/env.
test ! -e "$HOME/.gitconfig"
if grep -rqF "${REGISTRY_PASSWORD}" "$HOME" 2>/dev/null; then
echo "::error::registry credential still present under \$HOME after scrub"
exit 1
fi
# GOPROXY=off from here on: the module cache is already warm, so any
# attempt to reach the network is a bug — and it fails loudly instead of
# quietly looking for the credential that is now gone.
- name: go build
env: { GOPROXY: "off" }
run: go build ./...
- name: go vet
env: { GOPROXY: "off" }
run: go vet ./...
- name: gofmt
run: test -z "$(gofmt -l .)" || { gofmt -l .; exit 1; }
- name: go test
env: { GOPROXY: "off" }
run: go test -count=1 ./...
- name: pre-flight credential table
run: bash scripts/preflight_test.sh
build-and-push:
runs-on: ubuntu-latest
timeout-minutes: 20
+48 -13
View File
@@ -44,8 +44,8 @@ on:
#
# Owner-set user-scope variables (see README "Central config via variables"):
# GADFLY_DEFAULT_MODELS, GADFLY_DEFAULT_SPECIALISTS,
# GADFLY_DEFAULT_PROVIDER_CONCURRENCY, GADFLY_DEFAULT_PROVIDER_LENS_CONCURRENCY,
# GADFLY_ENDPOINT_RAGNAROS (the 4090 Ti endpoint).
# GADFLY_DEFAULT_PROVIDER_LENS_CONCURRENCY (the provider-wide lens budget),
# GADFLY_ENDPOINT_NETHERSTORM (the local GPU box endpoint).
# An unset variable + no input → the image default (one model, default suite),
# so a public consumer with neither still gets a sane minimal review.
inputs:
@@ -53,13 +53,20 @@ on:
specialists: { type: string, default: "" } # GADFLY_SPECIALISTS — empty falls back to user var GADFLY_DEFAULT_SPECIALISTS
provider: { type: string, default: "" } # GADFLY_PROVIDER
base_url: { type: string, default: "" } # GADFLY_BASE_URL
provider_concurrency: { type: string, default: "" } # GADFLY_PROVIDER_CONCURRENCY — empty falls back to user var GADFLY_DEFAULT_PROVIDER_CONCURRENCY
provider_lens_concurrency: { type: string, default: "" } # GADFLY_PROVIDER_LENS_CONCURRENCY — empty falls back to user var GADFLY_DEFAULT_PROVIDER_LENS_CONCURRENCY
provider_concurrency: { type: string, default: "" } # DEPRECATED / ignored — the per-provider MODEL cap was removed; the lens budget below is the single throttle. Kept so existing callers don't error.
provider_lens_concurrency: { type: string, default: "" } # GADFLY_PROVIDER_LENS_CONCURRENCY — the per-provider lens budget (shared across the provider's models); empty falls back to user var GADFLY_DEFAULT_PROVIDER_LENS_CONCURRENCY
timeout_secs: { type: string, default: "600" } # GADFLY_TIMEOUT_SECS (per lens)
max_steps: { type: string, default: "14" } # GADFLY_MAX_STEPS
worker_model: { type: string, default: "" } # GADFLY_WORKER_MODEL
allowed_users: { type: string, default: "" } # GADFLY_ALLOWED_USERS (consumer-specific; set in your stub)
# Gitea >= 1.27 does not propagate the CALLER's workflow_dispatch inputs into a
# called workflow's github.event, so a manual "review PR #N" dispatch arrived
# here with an empty PR and died at the entrypoint's "PR required" check. The
# caller stub must thread it explicitly: `pr_number: ${{ github.event.inputs.pr_number }}`.
pr_number: { type: string, default: "" }
trigger_phrase: { type: string, default: "" } # GADFLY_TRIGGER_PHRASE
consolidate: { type: string, default: "" } # GADFLY_CONSOLIDATE — "" => auto (one consensus comment for >=2 models); "0" => one comment per model
inline_review: { type: string, default: "" } # GADFLY_INLINE_REVIEW — "" => on (post a COMMENT-state PR review with inline comments on changed lines); "0" => off
# Job wall-clock cap. 90 as a default: the 5-lens suite across a slow lane
# (claude-code with extended thinking) over two passes can run long.
timeout_minutes: { type: number, default: 90 }
@@ -75,6 +82,15 @@ on:
OPENAI_API_KEY: { required: false }
ANTHROPIC_API_KEY: { required: false }
GOOGLE_API_KEY: { required: false }
# Alibaba Model Studio (Qwen), for GADFLY_MODELS entries like
# "qwen/qwen3.8-max". NOT interchangeable with OPENAI_API_KEY: majordomo's
# qwen built-in reads QWEN_API_KEY only and deliberately refuses to fall
# back to the OpenAI key, so an unforwarded secret is a 401, not a
# mis-billed OpenAI call.
QWEN_API_KEY: { required: false }
# Moonshot (Kimi) over its own API — distinct from the ollama-cloud
# "kimi-k2.6:cloud" entry, which is keyed by OLLAMA_CLOUD_API_KEY.
KIMI_API_KEY: { required: false }
GADFLY_API_KEY: { required: false }
CLAUDE_CODE_OAUTH_TOKEN: { required: false }
GADFLY_FINDINGS_URL: { required: false }
@@ -92,7 +108,12 @@ jobs:
runs-on: ubuntu-latest
timeout-minutes: ${{ inputs.timeout_minutes }}
steps:
- uses: docker://gitea.stevedudenhoeffer.com/steve/gadfly:sha-c342bdb
# Pin the reviewer image to an immutable sha (act_runner caches :latest, so a
# moved :latest is often NOT re-pulled). sha-b37cd09 adds the provider-wide
# lens budget (PR #27: one shared lens-permit pool per provider, the model cap
# removed) on top of the opencode CLI engine (PR #26) and the Gitea >= 1.27
# workflow_call reclassification. Bump per Gadfly release.
- uses: docker://gitea.stevedudenhoeffer.com/steve/gadfly:sha-b37cd09
env:
# --- event context (from the CALLER's github.*) -------------------
GITEA_API: ${{ github.server_url }}/api/v1/repos/${{ github.repository }}
@@ -102,17 +123,25 @@ jobs:
# forwarding, since the auto token isn't a forwarded workflow_call secret.
GITEA_TOKEN: ${{ github.token }}
EVENT_NAME: ${{ github.event_name }}
PR: ${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr_number }}
PR: ${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr_number || inputs.pr_number }}
PR_BRANCH: ${{ github.head_ref }}
IS_DRAFT: ${{ github.event.pull_request.draft }}
COMMENT_BODY: ${{ github.event.comment.body }}
COMMENT_ID: ${{ github.event.comment.id }}
ACTOR: ${{ github.actor }}
# --- provider auth (forwarded workflow_call secrets; empty if the caller doesn't forward it) -
# OLLAMA_CLOUD_API_KEY powers both the ollama-cloud majordomo path AND
# the opencode engine (GADFLY_MODELS entry "opencode/<model>").
OLLAMA_CLOUD_API_KEY: ${{ secrets.OLLAMA_CLOUD_API_KEY }}
OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}
ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }}
GOOGLE_API_KEY: ${{ secrets.GOOGLE_API_KEY }}
# Qwen (Alibaba Model Studio) and Kimi (Moonshot) over their own APIs,
# for GADFLY_MODELS entries like "qwen/qwen3.8-max". Each built-in
# reads ONLY its own variable — no cross-provider fallback — so a
# missing line here is a clean 401, never a silently mis-keyed call.
QWEN_API_KEY: ${{ secrets.QWEN_API_KEY }}
KIMI_API_KEY: ${{ secrets.KIMI_API_KEY }}
GADFLY_API_KEY: ${{ secrets.GADFLY_API_KEY }}
CLAUDE_CODE_OAUTH_TOKEN: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }}
# Named LAN endpoints, defined in user/org vars (format
@@ -122,12 +151,14 @@ jobs:
# token, keep that one a secret instead.
GADFLY_ENDPOINT_M1: ${{ vars.GADFLY_ENDPOINT_M1 }}
GADFLY_ENDPOINT_M5: ${{ vars.GADFLY_ENDPOINT_M5 }}
# ragnaros = the 4090 Ti via its llama-swap proxy. Defined in the user
# var GADFLY_ENDPOINT_RAGNAROS (format "<provider>|<base-url>[|<key>]")
# so the URL can change without editing this file; the matching model is
# ragnaros/qwen3.6-27b in GADFLY_DEFAULT_MODELS. NB: use the un-hyphenated
# `llamaswap` provider spelling in the var — the pinned image needs it.
GADFLY_ENDPOINT_RAGNAROS: ${{ vars.GADFLY_ENDPOINT_RAGNAROS }}
# netherstorm = the local GPU box via its llama-swap proxy. Defined in the
# user var GADFLY_ENDPOINT_NETHERSTORM (format "<provider>|<base-url>[|<key>]",
# e.g. "llamaswap|https://llama-swap.netherstorm...") so the URL can change
# without editing this file; the matching model is netherstorm/qwen3.6-27b in
# GADFLY_DEFAULT_MODELS. NB: use the un-hyphenated `llamaswap` provider
# spelling in the var — the pinned image needs it. Without this line the var
# is silently dropped at the workflow_call boundary ('unknown provider').
GADFLY_ENDPOINT_NETHERSTORM: ${{ vars.GADFLY_ENDPOINT_NETHERSTORM }}
# --- findings telemetry (optional) --------------------------------
GADFLY_FINDINGS_URL: ${{ secrets.GADFLY_FINDINGS_URL }}
GADFLY_FINDINGS_TOKEN: ${{ secrets.GADFLY_FINDINGS_TOKEN }}
@@ -136,10 +167,14 @@ jobs:
GADFLY_SPECIALISTS: ${{ inputs.specialists || vars.GADFLY_DEFAULT_SPECIALISTS }}
GADFLY_PROVIDER: ${{ inputs.provider }}
GADFLY_BASE_URL: ${{ inputs.base_url }}
GADFLY_PROVIDER_CONCURRENCY: ${{ inputs.provider_concurrency || vars.GADFLY_DEFAULT_PROVIDER_CONCURRENCY }}
# NB: GADFLY_PROVIDER_CONCURRENCY (the old model cap) is intentionally no
# longer forwarded — entrypoint.sh ignores it. The lens budget is the one
# throttle now, shared across a provider's models.
GADFLY_PROVIDER_LENS_CONCURRENCY: ${{ inputs.provider_lens_concurrency || vars.GADFLY_DEFAULT_PROVIDER_LENS_CONCURRENCY }}
GADFLY_TIMEOUT_SECS: ${{ inputs.timeout_secs }}
GADFLY_MAX_STEPS: ${{ inputs.max_steps }}
GADFLY_WORKER_MODEL: ${{ inputs.worker_model }}
GADFLY_ALLOWED_USERS: ${{ inputs.allowed_users }}
GADFLY_TRIGGER_PHRASE: ${{ inputs.trigger_phrase }}
GADFLY_CONSOLIDATE: ${{ inputs.consolidate }}
GADFLY_INLINE_REVIEW: ${{ inputs.inline_review }}
+46 -10
View File
@@ -22,15 +22,24 @@ verifies each one against the actual code, and posts its findings as a comment.
4. **Provider-agnostic.** Powered by [majordomo](https://gitea.stevedudenhoeffer.com/steve/majordomo),
so it can target Ollama (local/cloud), OpenAI, Anthropic, Google, or any
OpenAI/Ollama-compatible endpoint. Don't re-hardcode a single provider.
5. **Portable & self-contained.** `cmd/gadfly` depends only on the Go stdlib + majordomo. Keep
it that way — no heavyweight deps, no coupling to any one consumer repo (e.g. mort).
5. **Portable & self-contained.** `cmd/gadfly` depends only on the Go stdlib, majordomo, and
[executus](https://gitea.stevedudenhoeffer.com/steve/executus) (whose *core*`run`/`compact`/
`model`/`fanout`/`tool` — is itself majordomo+stdlib only, so the binary stays static; do NOT
pull executus's `contrib/store` or any battery that drags in a DB driver). No heavyweight deps,
no coupling to any one consumer repo (e.g. mort). Gadfly is executus's canonical *light* consumer.
## Architecture
```
cmd/gadfly/ the reviewer binary — pure producer of review markdown (stdout)
main.go orchestration: loop specialists, each a review pass + adversarial recheck
engine.go reviewEngine abstraction: majordomo agent loop vs claude-code CLI shell-out
main.go orchestration: fan specialists out (executus/fanout), each a review pass + recheck
engine.go reviewEngine abstraction: executus run.Executor (majordomo agent loop +
compaction/bounding/budget/critic) vs claude-code / opencode CLI shell-outs
opencode.go the opencode CLI engine: ollama-cloud model through the OpenCode harness
(read-only via a generated OPENCODE_CONFIG_CONTENT agent+provider); for
benchmarking the boutique harness vs a free one on the same model
executus.go executus wiring: tool.Registry over the repo tools, the run.Executor build
(compact + model context-limit threshold + per-PR budget + wrap-up critic)
specialists.go specialist lenses: built-ins, default suite, env + .gadfly.yml resolution
auto.go dynamic `auto` selection: a selector model picks lenses per-diff (may invent)
delegate.go worker-tier delegate_investigation tool (cheap sub-agent does legwork)
@@ -48,7 +57,7 @@ Dockerfile multi-stage; private-module creds via BuildKit secrets ne
.gitea/workflows/build-image.yml push main → :latest; tag v* → :<tag>+:latest; PR → build-only
.gitea/workflows/review-reusable.yml reusable (workflow_call) review job; resolves swarm config at
RUNTIME: consumer `with:` input → owner user-scope var (GADFLY_DEFAULT_MODELS /
_SPECIALISTS / _PROVIDER_CONCURRENCY / _PROVIDER_LENS_CONCURRENCY, +
_SPECIALISTS / _PROVIDER_LENS_CONCURRENCY, +
GADFLY_ENDPOINT_RAGNAROS) → image default. Vars are injected per-run, so editing
one var retunes the whole fleet even though long-lived act_runners CACHE this file
by ref (a moved tag is NOT re-fetched — only a runtime value or a fresh @<sha>
@@ -69,7 +78,7 @@ verdict. Verdict is one of: `No material issues found` / `Minor issues` / `Block
## Build / test
```sh
go build ./cmd/gadfly # needs read access to the private majordomo module
go build ./cmd/gadfly # needs read access to the private majordomo + executus modules
go test ./...
gofmt -l cmd/ # must be clean
docker build -t gadfly:dev --secret id=REGISTRY_USER,env=REGISTRY_USER --secret id=REGISTRY_PASSWORD,env=REGISTRY_PASSWORD .
@@ -145,7 +154,34 @@ are actually exercised. OpenAI/Anthropic/Google come from majordomo's abstractio
NOT re-pulled, so the job silently runs the previous image. For a run that must use a specific
build (e.g. validating a just-pushed fix), pin the consumer stub to the immutable
`:sha-<short>` tag the build publishes, not `:latest`.
- **Concurrency is per-provider** (`entrypoint.sh`): each provider is a lane, lanes run in
parallel, `cap` (from `GADFLY_PROVIDER_CONCURRENCY` else `GADFLY_CONCURRENCY`, default 1) bounds
models-at-once within a lane. The review timeout (`GADFLY_TIMEOUT_SECS`) is **per-lens**, not
shared across the suite — a slow model can't starve later lenses (the original timeout bug).
- **Concurrency is one per-provider lens budget** (`entrypoint.sh` + `cmd/gadfly/lenssem.go`):
each provider is a lane, lanes run in parallel, and within a lane ALL of the provider's models
run at once — the only throttle is a **provider-wide lens budget** (max lens passes in flight,
a lens = one specialist's review+recheck). The budget comes from
`GADFLY_PROVIDER_LENS_CONCURRENCY` (`provider=N` map) else `GADFLY_LENS_CONCURRENCY` (default 1).
Because models are separate processes, the budget is a **cross-process permit pool**: entrypoint
seeds a per-lane dir of N flock files; each model's binary acquires one before a lens pass and
releases it after (flock auto-drops on process death). This replaced the old two-level
`GADFLY_PROVIDER_CONCURRENCY` MODEL cap × per-model `GADFLY_LENS_CONCURRENCY`, which multiplied
and let a model hold its slot through its last lens — stalling the next model with idle lens
capacity. Those two model-cap vars are now **ignored**. The review timeout
(`GADFLY_TIMEOUT_SECS`) is **per-lens**, not shared across the suite — a slow lens can't starve
the others (the original timeout bug).
- **Large-PR token burn**: the agent loop re-sends the whole transcript every step, so a giant
diff (the old `get_diff` dumped it untruncated, and it was embedded in both the review and
recheck task) was re-transmitted ~steps × lenses × passes × models times — a ~250 K-token PR
could drain a metered usage block in minutes. Fixed in three size-gated layers (small PRs
untouched): paginated `get_diff` + `executus/compact` compaction in the binary; an
`entrypoint.sh` downshift above `GADFLY_HUGE_DIFF_BYTES` (one cheap model, fewer lenses/steps,
no recheck); and a swarm-wide `GADFLY_PR_BUDGET_SECS` wall-clock backstop. Compaction's threshold
is intentionally LOW (`GADFLY_COMPACT_RATIO` 0.45, not executus's 0.7) because the burning
transcript on the embedded path rarely reaches 0.7×context.
- **executus re-platform**: the in-process review path runs through `executus/run`'s `run.Executor`
(compaction, run-bounding, `Ports.Budget`, the wrap-up nudge as `Ports.Critic`), wiring it in
`cmd/gadfly/executus.go`. Gadfly KEEPS its own `model.go` resolution (so `GADFLY_ENDPOINT_<NAME>`
http aliases + the claude-code engine survive) and only hands `run.Executor` the already-resolved
model via a trivial resolver — do NOT route review-model resolution through
`model.ParseModelForContext` (it bypasses gadfly's endpoint aliases). `run.Result` exposes no
transcript, so the old transcript-based forced-finalization fallback is gone; the wrap-up critic
nudge is the remaining "always emit something" mechanism. The claude-code engine still shells out
and is unaffected.
+24 -1
View File
@@ -24,12 +24,35 @@ RUN --mount=type=cache,target=/go/pkg/mod \
go build -trimpath -ldflags="-s -w" -o /out/gadfly ./cmd/gadfly
FROM alpine:3.20
RUN apk add --no-cache bash git curl jq ca-certificates nodejs npm
# procps provides pkill/pgrep, which entrypoint.sh's per-PR wall-clock backstop
# (GADFLY_PR_BUDGET_SECS) uses to stop the review subtrees — busybox's applets
# are not guaranteed to include them.
RUN apk add --no-cache bash git curl jq ca-certificates nodejs npm procps
# Bundle the Claude Code CLI so the `claude-code` review engine works out of the
# box (GADFLY_MODELS=claude-code or claude-code/<model>). This adds Node + the
# CLI to the image (notably larger); ollama-only users pay the size but nothing
# else. Auth is provided at runtime via CLAUDE_CODE_OAUTH_TOKEN / ANTHROPIC_API_KEY.
RUN npm install -g @anthropic-ai/claude-code && npm cache clean --force
# Bundle the OpenCode CLI (opencode.ai) for the `opencode` review engine
# (GADFLY_MODELS=opencode/<model>): a freely-available agentic harness driving an
# ollama-cloud model, used to benchmark it against gadfly's own executus harness
# on the same model. Auth reuses OLLAMA_CLOUD_API_KEY at runtime. opencode ships a
# compiled (Bun) binary; it publishes musl variants (opencode-linux-*-musl) that
# npm auto-selects on alpine via the package "libc" field. libstdc++/libgcc are
# the Bun binary's runtime deps; gcompat is a belt-and-suspenders fallback in case
# npm ever resolves a glibc build here.
RUN apk add --no-cache gcompat libstdc++ libgcc \
&& npm install -g opencode-ai \
&& npm cache clean --force
# Best-effort: confirm the binary runs and pre-warm the openai-compatible provider
# package into opencode's cache so a review doesn't pay a first-run npm fetch. The
# warm-up model call intentionally fails against a dead URL. Never fail the build:
# a musl/runtime quirk here must not break the shared image for ollama/claude
# users — a broken opencode engine degrades to a normal (advisory) pass error.
RUN opencode --version >/dev/null 2>&1 \
&& OPENCODE_CONFIG_CONTENT='{"provider":{"gadfly":{"npm":"@ai-sdk/openai-compatible","options":{"baseURL":"http://127.0.0.1:9/v1"},"models":{"x":{}}}}}' \
timeout 120 opencode run --model gadfly/x "warm" >/dev/null 2>&1 \
; true
COPY --from=build /out/gadfly /usr/local/bin/gadfly
COPY scripts /app/scripts
COPY entrypoint.sh /entrypoint.sh
+184 -34
View File
@@ -73,11 +73,37 @@ majordomo failover chain / alias) is used verbatim.
| **[llama-swap](https://github.com/mostlygeek/llama-swap)** (model-swapping proxy) | `llama-swap`/`llama-swaps` (un-hyphenated `llamaswap`/`llamaswaps` also accepted) + `GADFLY_BASE_URL` or a `GADFLY_ENDPOINT_*` entry, or an `LLM_*` `llama-swap://` / `llama-swaps://` DSN | optional bearer | ⚠️ wired, **untested** |
| **OpenAI-compatible** (incl. local Ollama's `/v1`) | `openai` + `GADFLY_BASE_URL` | `OPENAI_API_KEY` (any non-empty for Ollama) | ✅ tested against Ollama |
| **OpenAI** | `openai` | `OPENAI_API_KEY` | ⚠️ wired, **untested** |
| **Qwen** (Alibaba Model Studio) | `qwen` | `QWEN_API_KEY` | ⚠️ wired, **untested** |
| **Kimi** (Moonshot) | `kimi` | `KIMI_API_KEY` | ⚠️ wired, **untested** |
| **Anthropic** | `anthropic` | `ANTHROPIC_API_KEY` | ⚠️ wired, **untested** |
| **Google (Gemini)** | `google` | `GOOGLE_API_KEY` / `GEMINI_API_KEY` | ⚠️ wired, **untested** |
Qwen and Kimi are majordomo built-ins that speak the OpenAI protocol at their own
endpoints, so `qwen/qwen3.8-max` or `kimi/kimi-k2-0711-preview` work as
`GADFLY_MODELS` entries with only the matching key set. Each reads **only** its own
variable — no cross-provider fallback — so forgetting to forward `QWEN_API_KEY`
gets you a skip notice naming it, not a mis-keyed call. Note `kimi/<model>` (Moonshot's
API, `KIMI_API_KEY`) is a different route than the `kimi-k2.6:cloud` entry in the
default swarm, which is Ollama Cloud and keyed by `OLLAMA_CLOUD_API_KEY`.
> **Qwen keys are endpoint-scoped, and the failure looks like a bad key.**
> Alibaba Model Studio issues *workspace-scoped* endpoints of the form
> `https://<workspace>.<region>.maas.aliyuncs.com/compatible-mode/v1`. A key
> issued for one host is rejected by another with a genuine
> `401 Incorrect API key provided` — so a perfectly good key reads as invalid if
> the endpoint doesn't match. The built-in defaults to the shared international
> host; point at your own with a named endpoint, which needs no code change:
>
> ```
> GADFLY_ENDPOINT_QWENWS = "qwen|https://<workspace>.ap-southeast-1.maas.aliyuncs.com/compatible-mode/v1|<key>"
> GADFLY_MODELS = "qwenws/qwen3.8-max,..."
> ```
>
> (Verified the hard way against a live deployment.)
> ### 🧪 Honest status
> Only the **Ollama** paths above are actually exercised. The OpenAI / Anthropic / Google
> Only the **Ollama** paths above are actually exercised. The OpenAI / Qwen / Kimi /
> Anthropic / Google
> providers come "for free" from majordomo's abstraction and *should* work, but I haven't
> spent money verifying them — treat them as untested. The OpenAI-**compatible** path **is**
> tested, because you can point it at a local Ollama (`GADFLY_BASE_URL=http://localhost:11434/v1`)
@@ -140,6 +166,58 @@ as an example, not wired or tested here.
> specialist selection and the `delegate_investigation` worker are majordomo-only and are skipped
> with this engine (Claude Code does its own legwork).
### OpenCode engine (`opencode`)
The same shell-out idea, but with a **freely-available** harness: Gadfly can review through the
**[OpenCode](https://opencode.ai) CLI**, which — like Claude Code — brings its own read tools and
verifies findings against the checked-out repo, but drives an **ollama-cloud** model. The point is
to benchmark gadfly's boutique executus harness against a good open harness *on the same model*:
run `ollama-cloud/glm-5.2` (majordomo loop) and `opencode/glm-5.2` (OpenCode) side by side and
compare their findings. This is the wired, no-proxy version of the "alternate backends" comparison
described above. The CLI is bundled in the image (Node + `opencode-ai`).
Select it as a model id:
| Spec | Meaning |
|------|---------|
| `opencode/glm-5.2` | serve `glm-5.2` via ollama-cloud through OpenCode |
| `open-code/glm-5.2` | accepted alias spelling (`opencode` is canonical) |
| `opencode/qwen3-coder:480b-cloud` | model ids are taken **verbatim** — colons are preserved (no `:thinking` suffix here, unlike claude-code) |
| `opencode/<provider>/<model>` | escape hatch: pass `<provider>/<model>` straight to OpenCode's own provider registry/auth (e.g. `opencode/anthropic/claude-sonnet-4-6`) |
| `opencode` | bare: OpenCode's configured default model |
```yaml
GADFLY_MODELS: "ollama-cloud/glm-5.2,opencode/glm-5.2" # the benchmark pairing
```
Auth reuses **`OLLAMA_CLOUD_API_KEY`** (the same secret the ollama-cloud path uses; it's mapped to
`OLLAMA_API_KEY`, which the generated provider references as `{env:OLLAMA_API_KEY}` — never a literal
secret in config). Tuning knobs (all optional):
| Env | Default | Meaning |
|-----|---------|---------|
| `GADFLY_OPENCODE_MODEL` | *(from the spec suffix)* | overrides the model |
| `GADFLY_OPENCODE_BASE_URL` | `https://ollama.com/v1` | ollama-cloud endpoint; point at a local Ollama (`http://localhost:11434/v1`) or any OpenAI-compatible server |
| `GADFLY_OPENCODE_EXTRA_ARGS` | *(unset)* | extra `opencode run` args, **whitespace-split**, appended before the positional task |
| `GADFLY_OPENCODE_BIN` | `opencode` | CLI binary path |
> **Read-only is enforced through config, not a flag.** OpenCode has no `--append-system-prompt`, so
> Gadfly generates a per-lens config — the lens system prompt as a `gadfly` agent's prompt, with the
> mutating and network tools (`edit`/`bash`/`webfetch`/`websearch`/`external_directory`) denied at both
> the global and agent level — and injects it via `OPENCODE_CONFIG_CONTENT`. That env var is the
> highest-precedence config source in the container, so it **outranks any `opencode.json` a reviewed
> repo ships** — a repo can't re-enable edits on the reviewer. The subprocess runs with a **reduced
> environment**: the provider keys OpenCode needs to authenticate (`OLLAMA_API_KEY` for the primary
> path, plus `ANTHROPIC_*`/`OPENAI_*`/`GOOGLE_*`/`GEMINI_*` for the `opencode/<provider>/<model>`
> pass-through) alongside `PATH`/`HOME`/locale/`OPENCODE_*`/`GADFLY_OPENCODE_*` — but **not** gadfly's
> own secrets (the Gitea token, the findings token, or the claude-code subscription token), which the
> CLI has no use for.
> **Newly wired, lightly tested.** Like the claude-code engine, `auto` specialist selection and the
> `delegate_investigation` worker are majordomo-only and are skipped here (OpenCode does its own
> legwork). Output capture reads OpenCode's default text output, so treat the engine as new and
> sanity-check a run before trusting a benchmark.
### Endpoint aliases via env vars
For multiple named backends (e.g. a couple of Ollama boxes on your LAN), register them by
@@ -220,38 +298,32 @@ Unset = no delegation (current behavior).
### Concurrency (per-provider lanes)
With multiple models, each **provider** is its own lane and lanes run in **parallel**, so a fast
cloud provider isn't stuck behind a slow local box. Within a lane, at most `cap` models run at
once — `cap` comes from `GADFLY_PROVIDER_CONCURRENCY` (a `provider=N` map) else `GADFLY_CONCURRENCY`
(default `1`). The timeout is **per-lens** (`GADFLY_TIMEOUT_SECS`), so a slow model on one lens
can't starve the others.
cloud provider isn't stuck behind a slow local box. There is **one throttle**: a per-provider
**lens budget** — the max number of lens passes (a lens = one specialist's review+recheck) in
flight at once for that provider. Every model in the lane runs concurrently and its lenses draw
from that single shared budget, so nothing else caps how many models run. The budget comes from
`GADFLY_PROVIDER_LENS_CONCURRENCY` (a `provider=N` map) else the `GADFLY_LENS_CONCURRENCY` scalar
(default `1`). The timeout is **per-lens** (`GADFLY_TIMEOUT_SECS`), so a slow lens can't starve
the others.
```yaml
# One local box (serial — it serves one model at a time) + 3 cloud reviews at once,
# both lanes running concurrently:
GADFLY_PROVIDER_CONCURRENCY: "ollama-cloud=3,m1pro=1"
# The local box gets 1 lens at a time (serial); the cloud lane runs up to 3 lens passes at once,
# shared across ALL its models. Both lanes run concurrently.
GADFLY_PROVIDER_LENS_CONCURRENCY: "ollama-cloud=3,m1pro=1"
GADFLY_MODELS: "m1pro/qwen3:14b,qwen3-coder:480b-cloud,gpt-oss:120b-cloud"
```
A model's provider is the spec's first segment (`m1pro/…``m1pro`), or `GADFLY_PROVIDER`/
`ollama-cloud` for a bare id. Default (`cap 1`) keeps a single-provider pool fully sequential.
`ollama-cloud` for a bare id. The budget is **shared across the provider's models**: with a
budget of 3 and two cloud models, you get 3 lens passes in flight in any mix — as one model
finishes a lens, the freed slot immediately goes to another model's next lens, so a model
winding down to its last lens never stalls the others (the pre-2026-07 design capped *models*
separately and did stall — that `GADFLY_PROVIDER_CONCURRENCY`/`GADFLY_CONCURRENCY` model cap is
**gone**; those vars are now ignored). Default (budget `1`) keeps a provider fully sequential.
**Lens fan-out (within a model).** By default the specialist lenses run **sequentially** inside
each model (`GADFLY_LENS_CONCURRENCY=1`). Raise it to overlap the independent per-lens
review+recheck passes — the model then posts its consolidated comment as soon as its lenses
finish (so with sequential models, results stream in per model and per-model timings stay
clean). Like the model cap, it's **per-provider configurable**: `GADFLY_PROVIDER_LENS_CONCURRENCY`
takes a `provider=N` map keyed by the **same provider lanes** as `GADFLY_PROVIDER_CONCURRENCY`,
falling back to the `GADFLY_LENS_CONCURRENCY` scalar (default `1`). **It multiplies with the
model cap:** total in-flight requests ≈ *models-at-once × lenses-at-once*, so to fan lenses out
without oversubscribing a backend, keep its model cap low and raise its lens cap:
```yaml
# Per provider: cloud runs one model at a time but fans its 3 lenses out (3 concurrent requests);
# the slow local box stays fully serial. Both provider lanes still run in parallel.
GADFLY_PROVIDER_CONCURRENCY: "ollama-cloud=1,m1=1"
GADFLY_PROVIDER_LENS_CONCURRENCY: "ollama-cloud=3,m1=1"
GADFLY_SPECIALISTS: "security,correctness,error-handling"
```
> Under the hood the shared budget is a small cross-process permit pool (flock files, seeded per
> lane by `entrypoint.sh`); permits release automatically if a model process dies, so a crashed
> lens can't leak budget.
### Live status board
@@ -277,6 +349,43 @@ every `GADFLY_STATUS_POLL_SECS` (default 12s) until the swarm finishes. It's adv
best-effort — the per-model findings comments are unaffected — and entirely separate from those.
Turn it off with `GADFLY_STATUS_BOARD=0`.
### Consensus consolidation
With **two or more models**, posting one comment each means a reader faces N walls of prose that
mostly agree. Instead Gadfly consolidates: every model writes its findings to a shared file, and
after the whole swarm finishes a single pass clusters those findings by location, counts **how
many models independently flagged each one**, and posts **one consensus comment**:
```
## 🪰 Gadfly review — consensus across 7 models
**Verdict: Blocking issues found** · 9 findings (3 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🔴 | Auth bypass: token not verified | `auth/login.go:42` | 6/7 | security |
| 🟠 | Unbounded retry loop | `sync/worker.go:88` | 3/7 | error-handling |
<details><summary>4 single-model findings (lower confidence)</summary> … </details>
<details><summary>Per-model detail</summary> … each model's full review, folded … </details>
```
Cross-model agreement is the strongest real-vs-false-positive signal available, so findings are
ranked by it (a lone low-severity finding folds away; a lone *critical* still surfaces). The
per-model comments are suppressed in this mode — each model's full review is preserved, folded,
inside the consensus comment — and nothing is lost: if consolidation can't run, Gadfly falls
back to posting the per-model comments. Controlled by `GADFLY_CONSOLIDATE`: `auto` (default — on
for ≥2 models), `1` (force on), `0` (force off, one comment per model). Single-model runs are
unaffected.
**Inline PR review.** Alongside the consensus comment, Gadfly also posts a single Gitea **pull
review** (state `COMMENT` — advisory, **never** request-changes or approve, so it can't block a
merge) whose inline comments anchor each consensus finding to the exact changed line it's about.
Only findings that land on a line in the diff are anchored (Gitea rejects comments off the diff);
the rest stay in the consensus comment. A re-run replaces the previous review instead of stacking.
It's the "reviewer integrated with Gitea" without the blocking — turn it off with
`GADFLY_INLINE_REVIEW=0`.
### Triggers
1. A **new/reopened/ready** non-draft PR — automatic.
@@ -329,8 +438,7 @@ on its next review **without** a re-pin or a tag move:
|---|---|
| `GADFLY_DEFAULT_MODELS` | `GADFLY_MODELS` (csv) |
| `GADFLY_DEFAULT_SPECIALISTS` | the lens suite |
| `GADFLY_DEFAULT_PROVIDER_CONCURRENCY` | models-at-once per provider |
| `GADFLY_DEFAULT_PROVIDER_LENS_CONCURRENCY` | lenses-at-once per provider |
| `GADFLY_DEFAULT_PROVIDER_LENS_CONCURRENCY` | the per-provider lens budget (lens passes in flight per provider, shared across its models) |
| `GADFLY_ENDPOINT_RAGNAROS` | a named endpoint, e.g. `llamaswap\|https://host` |
Adding a *new* named endpoint still needs a one-line reusable edit (Gitea can't auto-expose arbitrary
@@ -347,21 +455,32 @@ The reviewer binary reads these (the stub/entrypoint set sane defaults):
| `GADFLY_BASE_URL` | — | override endpoint (OpenAI/Ollama-compatible servers) |
| `GADFLY_API_KEY` | — | provider key; falls back to the provider's standard env |
| `claude-code` model id | — | route a model through the bundled Claude Code CLI (`claude-code` / `claude-code/<model>`); see [Claude Code engine](#claude-code-engine-claude-code) for its `GADFLY_CLAUDE_*` knobs |
| `opencode` model id | — | route an ollama-cloud model through the bundled OpenCode CLI (`opencode/<model>`); see [OpenCode engine](#opencode-engine-opencode) for its `GADFLY_OPENCODE_*` knobs |
| `GADFLY_SPECIALISTS` | default suite | csv of lenses, `all`, or `auto` (dynamic selection) |
| `GADFLY_SELECTOR_MODEL` | review model | model that picks lenses in `auto` mode |
| `GADFLY_WORKER_MODEL` | — | cheap model for `delegate_investigation`; unset = no delegation |
| `GADFLY_WORKER_MAX_STEPS` | 8 | tool-step cap for a delegated worker run |
| `GADFLY_CONCURRENCY` | 1 | default max models run at once **per provider** |
| `GADFLY_PROVIDER_CONCURRENCY` | — | per-provider overrides, e.g. `ollama-cloud=3,m1pro=1` |
| `GADFLY_LENS_CONCURRENCY` | 1 | specialist lenses run at once **within a model** (× model cap = total in-flight) |
| `GADFLY_PROVIDER_LENS_CONCURRENCY` | — | per-provider lens overrides, same lanes as `GADFLY_PROVIDER_CONCURRENCY`, e.g. `ollama-cloud=3,m1=1` |
| `GADFLY_LENS_CONCURRENCY` | 1 | **per-provider lens budget** — lens passes in flight per provider, shared across all its models (all a provider's models run at once; this is the only throttle) |
| `GADFLY_PROVIDER_LENS_CONCURRENCY` | — | per-provider lens-budget overrides, a `provider=N` map, e.g. `ollama-cloud=3,m1=1` |
| `GADFLY_CONCURRENCY` / `GADFLY_PROVIDER_CONCURRENCY` | | **removed** (was the per-provider models-at-once cap; now ignored — the lens budget is the single throttle) |
| `GADFLY_MAX_STEPS` | 24 | review-pass tool-step cap |
| `GADFLY_TIMEOUT_SECS` | 300 | deadline **per specialist lens** (review+recheck) |
| `GADFLY_RECHECK` | on | set `0`/`false` to skip the recheck pass |
| `GADFLY_RECHECK_MAX_STEPS` | 16 | recheck-pass step cap |
| `GADFLY_MAX_DIFF_CHARS` | 60000 | diff chars embedded in the prompt (full diff via `get_diff`) |
| `GADFLY_MAX_DIFF_CHARS` | 60000 | diff chars embedded in the **review** prompt (the full diff is reachable via the paginated `get_diff` tool, scoped per file with its `path` arg) |
| `GADFLY_RECHECK_DIFF_CHARS` | 20000 | diff chars embedded in the **recheck** prompt (smaller — the recheck pages `get_diff` for the hunks it verifies) |
| `GADFLY_COMPACT` | on | context compaction (via [executus](https://gitea.stevedudenhoeffer.com/steve/executus)): fold the transcript's runaway middle into a summary as it nears the model's context window, so a big diff + accumulating tool output can't balloon every step. `0` disables |
| `GADFLY_COMPACT_RATIO` | 0.45 | fraction of the model's context window at which compaction fires |
| `GADFLY_COMPACT_MODEL` | worker, else review model | cheap model the compactor uses to summarize the folded middle |
| `GADFLY_COMPACT_KEEP_RECENT` | 8 | most-recent messages kept verbatim during compaction |
| `GADFLY_COMPACT_SUMMARY_WORDS` | 200 | word cap on the compaction summary |
| `GADFLY_MODEL_CONTEXT_TOKENS` | *(auto)* | override the model's context-window size (tokens) for the compaction threshold; set it for self-hosted endpoints executus can't introspect (Ollama Cloud models resolve automatically) |
| `GADFLY_PR_TOKEN_BUDGET` | — | per-model token ceiling for this PR; once spent, remaining lenses/passes are skipped (advisory). 0 = off |
| `GADFLY_PR_TIME_BUDGET_SECS` | — | per-model wall-clock ceiling for this PR (advisory). 0 = off |
| `GADFLY_STATUS_BOARD` | on | set `0` to disable the live status-board comment |
| `GADFLY_STATUS_POLL_SECS` | 12 | how often the status board re-renders/upserts |
| `GADFLY_CONSOLIDATE` | `auto` | cross-model consensus comment: `auto` (on for ≥2 models), `1` (force on), `0` (off — one comment per model) |
| `GADFLY_INLINE_REVIEW` | on | when consolidating, also post a `COMMENT`-state PR review with inline comments on changed lines; `0` disables |
| `GADFLY_TRIGGER_PHRASE` | `@gadfly review` | comment phrase that re-triggers |
| `GADFLY_ALLOWED_USERS` | *(collaborators)* | comma-separated allow-list for comment triggers |
| `GADFLY_FINDINGS_URL` | — | gadfly-reports store base URL; set to enable findings telemetry (off when empty) |
@@ -369,6 +488,37 @@ The reviewer binary reads these (the stub/entrypoint set sane defaults):
| `GADFLY_REPO` | *(from `GITEA_API`)* | `owner/repo` slug stamped on emitted runs/findings (set by `entrypoint.sh`) |
| `GADFLY_PR` | *(from event)* | PR number stamped on emitted runs/findings (set by `entrypoint.sh`) |
### Large-PR cost controls
A very large diff is the one thing that can blow the budget: every review step
re-sends it, multiplied across models × lenses × passes × steps (a single
~250 K-token PR can otherwise burn a whole metered usage block). Gadfly handles
big PRs in three layers, all **size-gated so small PRs are untouched**:
1. **Paginated `get_diff` + compaction** (reviewer binary, on by default) —
`get_diff` returns a paginated, optionally per-file window instead of the whole
diff, and once a transcript nears the model's context window its middle is
folded into a summary (powered by [executus](https://gitea.stevedudenhoeffer.com/steve/executus)'s
`compact`). Tune with the `GADFLY_COMPACT_*` knobs above.
2. **Downshift** (`entrypoint.sh`) — above `GADFLY_HUGE_DIFF_BYTES` the whole fleet
collapses to a single cheap model + a focused lens subset, fewer steps, and no
recheck. A finished shallow review beats a budget-nuking one, and the posted
comment says so.
3. **Hard backstop** (`entrypoint.sh`) — `GADFLY_PR_BUDGET_SECS` is a wall-clock
ceiling across the *entire* fleet; on expiry the review is stopped and whatever
was found so far is posted. Like everything else, it never fails CI.
| Env | Default | Meaning |
|-----|---------|---------|
| `GADFLY_HUGE_DIFF_BYTES` | 600000 | downshift the fleet when the PR diff exceeds this many bytes (0 = never downshift) |
| `GADFLY_HUGE_DIFF_MODELS` | first model | model(s) to run on a downshifted huge PR |
| `GADFLY_HUGE_DIFF_SPECIALISTS` | `security,correctness,error-handling` | lenses on a downshifted huge PR |
| `GADFLY_HUGE_DIFF_MAX_STEPS` | 12 | review step cap on a huge PR |
| `GADFLY_HUGE_DIFF_RECHECK_MAX_STEPS` | 8 | recheck step cap on a huge PR |
| `GADFLY_HUGE_DIFF_RECHECK` | 0 | run the recheck pass on a huge PR (off by default) |
| `GADFLY_HUGE_DIFF_MAX_DIFF_CHARS` | 20000 | embedded review-diff chars on a huge PR |
| `GADFLY_PR_BUDGET_SECS` | — | swarm-wide wall-clock backstop; stops the whole fleet when reached (0 = off) |
## Findings telemetry (optional)
Gadfly can record what it found so model quality can be tracked over time. It is
@@ -392,7 +542,7 @@ code.
## Building locally
```sh
go build ./cmd/gadfly # needs read access to the private majordomo module
go build ./cmd/gadfly # needs read access to the private majordomo + executus modules
go test ./...
```
+477
View File
@@ -0,0 +1,477 @@
package main
// Cross-model consensus consolidation. The swarm runs each model independently
// (entrypoint.sh fans them out across provider lanes); historically each model
// posted its OWN comment, so a reader faced N walls of prose that mostly agreed.
//
// Instead, every model writes its findings to a shared directory
// (GADFLY_FINDINGS_OUT, one JSON file per model), and after the whole swarm
// finishes a single consolidation pass (GADFLY_CONSOLIDATE_DIR) clusters those
// findings by location, counts how many models independently flagged each one,
// and renders ONE comment: an agreement-ranked table up top (cross-model
// agreement is the strongest real-vs-false-positive signal we have), with each
// model's full review folded below for drill-down.
//
// This file owns: the per-model artifact (modelFindings), writing it
// (writeFindingsOut), reading the directory back, clustering, and rendering the
// consensus markdown (renderConsensus). It depends only on the stdlib.
import (
"encoding/json"
"errors"
"fmt"
"os"
"path/filepath"
"sort"
"strings"
)
// consensusMarker tags the single consolidated comment so entrypoint.sh can
// upsert it in place across re-runs (mirrors run.sh's per-model marker).
const consensusMarker = "<!-- gadfly-consensus -->"
// modelFindings is the per-model artifact written to GADFLY_FINDINGS_OUT. It
// carries enough to rebuild the consolidated comment without re-running models:
// the structured findings (for clustering) plus the full rendered review (for
// the folded per-model drill-down).
type modelFindings struct {
Model string `json:"model"`
Provider string `json:"provider"`
Verdict string `json:"verdict"` // worst lens verdict, as a label
Errored bool `json:"errored"` // the model produced no usable review (every lens failed, or the run crashed)
Markdown string `json:"markdown"` // full rendered per-model review (findings block already stripped)
Findings []outFinding `json:"findings"`
}
// outFinding is one finding in the per-model artifact (a flattened `finding`
// plus its lens).
type outFinding struct {
Lens string `json:"lens"`
File string `json:"file"`
Line int `json:"line"`
Severity string `json:"severity"`
Confidence string `json:"confidence"`
Title string `json:"title"`
Detail string `json:"detail"`
}
// collectFindings returns a lens result's findings with severity always filled
// in: per-finding when the structured block supplied it, else derived from the
// lens verdict (so heuristic-scraped findings still carry a canonical word). A
// clean or errored lens yields nothing. Shared by the telemetry emit and the
// per-model findings file so both agree on what a finding is.
func collectFindings(r specialistResult) []finding {
if r.errored || r.verdict == verdictClean {
return nil
}
fs := extractStructuredFindingsOrScrape(r)
lensSev := r.verdict.severity()
for i := range fs {
if fs[i].severity == "" {
fs[i].severity = lensSev
}
}
return fs
}
// writeFindingsOut writes this model's findings + rendered review to
// GADFLY_FINDINGS_OUT for the later consolidation pass. No-op unless the env is
// set. Best-effort: any error is logged to stderr and never affects the review
// (it runs after the markdown is already on stdout).
func writeFindingsOut(results []specialistResult) {
path := strings.TrimSpace(os.Getenv("GADFLY_FINDINGS_OUT"))
if path == "" {
return
}
mf := modelFindings{
Model: strings.TrimSpace(os.Getenv("GADFLY_MODEL")),
Provider: modelProvider(),
Verdict: worstVerdict(results).label(),
Errored: allErrored(results),
Markdown: renderConsolidated(results),
}
for _, r := range results {
for _, f := range collectFindings(r) {
mf.Findings = append(mf.Findings, outFinding{
Lens: r.spec.Name,
File: f.file,
Line: f.line,
Severity: f.severity,
Confidence: f.confidence,
Title: f.title,
Detail: f.detail,
})
}
}
data, err := json.Marshal(mf)
if err != nil {
fmt.Fprintln(os.Stderr, "gadfly: marshal findings out:", err)
return
}
// Defensive: make sure the parent dir exists (entrypoint creates it, but a
// missing dir would otherwise silently drop this model from the consensus).
if dir := filepath.Dir(path); dir != "" {
_ = os.MkdirAll(dir, 0o755)
}
if err := os.WriteFile(path, data, 0o644); err != nil {
fmt.Fprintln(os.Stderr, "gadfly: write findings out:", err)
}
}
// allErrored reports whether every lens of a review failed (so the model
// produced no usable findings). Such a model is recorded but excluded from the
// consensus agreement denominator — counting it would dilute every ratio with a
// model that never actually reviewed.
func allErrored(results []specialistResult) bool {
if len(results) == 0 {
return true
}
for _, r := range results {
if !r.errored {
return false
}
}
return true
}
// runConsolidate is the consolidation entry point (GADFLY_CONSOLIDATE_DIR set):
// read every per-model artifact in the directory, render the consensus comment
// to stdout. Errors are fatal to THIS process only — entrypoint.sh treats a
// failed consolidation as advisory and falls back to per-model comments.
func runConsolidate() error {
dir := strings.TrimSpace(os.Getenv("GADFLY_CONSOLIDATE_DIR"))
if dir == "" {
return errors.New("GADFLY_CONSOLIDATE_DIR is empty")
}
entries, err := os.ReadDir(dir)
if err != nil {
return fmt.Errorf("read consolidate dir: %w", err)
}
var models []modelFindings
for _, e := range entries {
if e.IsDir() || !strings.HasSuffix(e.Name(), ".json") {
continue
}
data, err := os.ReadFile(filepath.Join(dir, e.Name()))
if err != nil {
fmt.Fprintln(os.Stderr, "gadfly: read", e.Name(), err)
continue
}
var mf modelFindings
if err := json.Unmarshal(data, &mf); err != nil {
fmt.Fprintln(os.Stderr, "gadfly: parse", e.Name(), err)
continue
}
if strings.TrimSpace(mf.Model) == "" {
continue
}
models = append(models, mf)
}
if len(models) == 0 {
return errors.New("no model findings to consolidate")
}
// Cluster once, then render the consensus comment and (best-effort) the inline
// PR review from the same clusters so the two views can't drift.
clusters := clusterFindings(models)
// Lead with the marker so entrypoint.sh can upsert this comment in place
// (same pattern as run.sh's per-model marker); it appends the advisory footer.
fmt.Println(consensusMarker)
fmt.Println(renderConsensus(models, clusters))
// Inline PR review (COMMENT state) anchoring findings to changed lines.
// Best-effort: no-op without diff/API creds, never affects stdout/exit.
postInlineReview(clusters)
return nil
}
// cluster is a group of findings (across models) judged to be the same issue:
// same file, lines within lineTolerance of the cluster's current span. The span
// [line,maxLine] slides as members join, so a chain of nearby findings merges
// instead of splitting once it drifts past the first line.
type cluster struct {
file string
line int // representative (smallest) line
maxLine int // largest line in the cluster — the span's upper edge
severity string
title string
detail string // highest-severity report's detail; rendered in the inline review comment
models map[string]bool
lenses map[string]bool
}
// findingRef is one model's finding (carrying which model reported it), used
// while grouping findings into clusters.
type findingRef struct {
f outFinding
model string
}
// lineTolerance: a finding in the same file within this many lines of a
// cluster's current span is treated as the same issue (models often cite a line
// or two apart).
const lineTolerance = 3
// renderConsensus builds the single consolidated comment body from every model's
// findings. It does NOT emit the marker or advisory footer — entrypoint.sh wraps
// it (mirroring run.sh's per-model framing).
func renderConsensus(models []modelFindings, clusters []cluster) string {
// effective = models that actually produced a review. Errored models are
// shown (folded, below) but excluded from the agreement denominator so a
// failed model doesn't dilute every ratio.
effective := 0
for _, m := range models {
if !m.Errored {
effective++
}
}
errored := len(models) - effective
worst := verdictClean
for _, m := range models {
if v := parseVerdict(m.Verdict); v > worst {
worst = v
}
}
// Partition in one pass: "headline" findings (multi-model agreement, OR a
// lone CRITICAL) vs folded "single-model" lower-confidence findings. Also
// count multi-model agreements for the summary line.
var headline, folded []cluster
agreed := 0
for _, c := range clusters {
if len(c.models) >= 2 {
agreed++
}
if len(c.models) >= 2 || sevRank(c.severity) >= sevRank("critical") {
headline = append(headline, c)
} else {
folded = append(folded, c)
}
}
var b strings.Builder
fmt.Fprintf(&b, "## 🪰 Gadfly review — consensus across %d model%s", effective, plural(effective))
if errored > 0 {
fmt.Fprintf(&b, " (%d failed)", errored)
}
b.WriteString("\n\n")
fmt.Fprintf(&b, "**Verdict: %s** · %d finding%s (%d with multi-model agreement)\n",
worst.label(), len(clusters), plural(len(clusters)), agreed)
if len(headline) > 0 {
b.WriteString("\n| | Finding | Where | Models | Lens |\n|--|--|--|--|--|\n")
for _, c := range headline {
fmt.Fprintf(&b, "| %s | %s | `%s` | %d/%d | %s |\n",
sevIcon(c.severity), mdCell(c.title), mdCell(location(c.file, c.line)),
len(c.models), effective, mdCell(lensList(c.lenses)))
}
} else if len(clusters) == 0 {
b.WriteString("\nNo material issues found by consensus.\n")
}
// else: only single-model findings — they're shown folded below, so don't
// claim "no material issues" (there are some, just none with consensus).
if len(folded) > 0 {
fmt.Fprintf(&b, "\n<details><summary>%d single-model finding%s (lower confidence)</summary>\n\n",
len(folded), plural(len(folded)))
b.WriteString("| | Finding | Where | Model | Lens |\n|--|--|--|--|--|\n")
for _, c := range folded {
fmt.Fprintf(&b, "| %s | %s | `%s` | %s | %s |\n",
sevIcon(c.severity), mdCell(c.title), mdCell(location(c.file, c.line)),
mdCell(oneModel(c.models)), mdCell(lensList(c.lenses)))
}
b.WriteString("\n</details>\n")
}
// Per-model full reviews, folded for drill-down (nothing is lost).
b.WriteString("\n<details><summary>Per-model detail</summary>\n")
for _, m := range models {
body := strings.TrimSpace(m.Markdown)
if body == "" {
body = "_(no output)_"
}
verdict := m.Verdict
if m.Errored {
verdict = "⚠️ reviewer failed"
}
fmt.Fprintf(&b, "\n<details><summary><b>%s</b> (%s) — %s</summary>\n\n%s\n\n</details>\n",
mdCell(m.Model), mdCell(m.Provider), verdict, body)
}
b.WriteString("\n</details>")
return b.String()
}
// clusterFindings groups every model's findings into cross-model clusters,
// sorted by agreement (desc), then severity (desc), then location.
func clusterFindings(models []modelFindings) []cluster {
// Group by file, then greedily merge by line proximity.
byFile := map[string][]findingRef{}
for _, m := range models {
for _, f := range m.Findings {
if strings.TrimSpace(f.File) == "" {
continue
}
byFile[f.File] = append(byFile[f.File], findingRef{f, m.Model})
}
}
var clusters []cluster
for file, items := range byFile {
sort.SliceStable(items, func(i, j int) bool { return items[i].f.Line < items[j].f.Line })
// Cluster within THIS file only (clusters never span files), so the inner
// scan is over same-file clusters, not every cluster seen so far.
var fileClusters []cluster
for _, it := range items {
placed := false
for ci := range fileClusters {
c := &fileClusters[ci]
// Join if the line falls within the cluster's span, widened by the
// tolerance on both edges — so the window slides as the span grows.
if it.f.Line >= c.line-lineTolerance && it.f.Line <= c.maxLine+lineTolerance {
mergeIntoCluster(c, it.f, it.model)
placed = true
break
}
}
if !placed {
c := cluster{
file: file,
line: it.f.Line,
maxLine: it.f.Line,
severity: it.f.Severity,
title: it.f.Title,
detail: it.f.Detail,
models: map[string]bool{},
lenses: map[string]bool{},
}
mergeIntoCluster(&c, it.f, it.model)
fileClusters = append(fileClusters, c)
}
}
clusters = append(clusters, fileClusters...)
}
sort.SliceStable(clusters, func(i, j int) bool {
if len(clusters[i].models) != len(clusters[j].models) {
return len(clusters[i].models) > len(clusters[j].models)
}
if sevRank(clusters[i].severity) != sevRank(clusters[j].severity) {
return sevRank(clusters[i].severity) > sevRank(clusters[j].severity)
}
if clusters[i].file != clusters[j].file {
return clusters[i].file < clusters[j].file
}
return clusters[i].line < clusters[j].line
})
return clusters
}
// mergeIntoCluster folds one finding into a cluster: union the model/lens sets,
// widen the [line,maxLine] span, and keep the highest-severity report's title.
func mergeIntoCluster(c *cluster, f outFinding, model string) {
if model != "" {
c.models[model] = true
}
if f.Lens != "" {
c.lenses[f.Lens] = true
}
if f.Line > 0 && (c.line == 0 || f.Line < c.line) {
c.line = f.Line
}
if f.Line > c.maxLine {
c.maxLine = f.Line
}
// Backfill an empty title/detail from any report, regardless of severity, so a
// higher-severity-but-terse finding doesn't leave the cluster without context.
if strings.TrimSpace(c.title) == "" && strings.TrimSpace(f.Title) != "" {
c.title = f.Title
}
if strings.TrimSpace(c.detail) == "" && strings.TrimSpace(f.Detail) != "" {
c.detail = f.Detail
}
// A strictly-higher-severity report takes over the title/detail.
if sevRank(f.Severity) > sevRank(c.severity) {
c.severity = f.Severity
if strings.TrimSpace(f.Title) != "" {
c.title = f.Title
}
if strings.TrimSpace(f.Detail) != "" {
c.detail = f.Detail
}
}
}
// sevRank orders the canonical severity words for sorting/comparison.
func sevRank(s string) int {
switch strings.ToLower(strings.TrimSpace(s)) {
case "critical":
return 5
case "high":
return 4
case "medium":
return 3
case "small":
return 2
case "trivial":
return 1
default:
return 0
}
}
// sevIcon is the at-a-glance severity badge for the consensus table.
func sevIcon(s string) string {
switch strings.ToLower(strings.TrimSpace(s)) {
case "critical", "high":
return "🔴"
case "medium":
return "🟠"
case "small":
return "🟡"
default:
return "⚪"
}
}
func location(file string, line int) string {
if line > 0 {
return fmt.Sprintf("%s:%d", file, line)
}
return file
}
func lensList(lenses map[string]bool) string {
out := make([]string, 0, len(lenses))
for l := range lenses {
out = append(out, l)
}
sort.Strings(out)
return strings.Join(out, ", ")
}
func oneModel(models map[string]bool) string {
for m := range models {
return m
}
return ""
}
// mdCell makes a string safe for a one-line markdown table cell: collapse
// newlines, escape pipes (which delimit columns), and neutralize backticks
// (a stray one would break an inline-code span — a backslash can't escape it
// inside code, so replace with an apostrophe). Inputs are model-influenced, so
// this keeps a malformed file path or title from breaking the table.
func mdCell(s string) string {
s = strings.ReplaceAll(s, "\n", " ")
s = strings.ReplaceAll(s, "|", "\\|")
s = strings.ReplaceAll(s, "`", "'")
return strings.TrimSpace(s)
}
func plural(n int) string {
if n == 1 {
return ""
}
return "s"
}
+217
View File
@@ -0,0 +1,217 @@
package main
import (
"encoding/json"
"os"
"path/filepath"
"strings"
"testing"
)
func TestClusterFindingsAgreementAndTolerance(t *testing.T) {
models := []modelFindings{
{Model: "m1", Verdict: "Blocking issues found", Findings: []outFinding{
{Lens: "security", File: "a.go", Line: 10, Severity: "high", Title: "auth bypass"},
{Lens: "perf", File: "b.go", Line: 5, Severity: "trivial", Title: "tiny nit"},
}},
{Model: "m2", Verdict: "Minor issues", Findings: []outFinding{
{Lens: "security", File: "a.go", Line: 11, Severity: "critical", Title: "auth bypass (crit)"}, // within tolerance of a.go:10
}},
{Model: "m3", Verdict: "Minor issues", Findings: []outFinding{
{Lens: "correctness", File: "a.go", Line: 10, Severity: "medium", Title: "auth bypass"},
}},
}
clusters := clusterFindings(models)
if len(clusters) != 2 {
t.Fatalf("want 2 clusters (a.go:10±, b.go:5), got %d: %+v", len(clusters), clusters)
}
// First cluster (highest agreement) is the a.go auth one: 3 models, severity
// escalated to critical, representative line the smallest (10).
c := clusters[0]
if len(c.models) != 3 {
t.Errorf("want 3 models on the top cluster, got %d", len(c.models))
}
if c.severity != "critical" {
t.Errorf("want escalated severity critical, got %q", c.severity)
}
if c.line != 10 {
t.Errorf("want representative line 10, got %d", c.line)
}
if !c.lenses["security"] || !c.lenses["correctness"] {
t.Errorf("want union of lenses, got %v", c.lenses)
}
}
func TestRenderConsensusFoldsSingleModelNits(t *testing.T) {
models := []modelFindings{
{Model: "m1", Provider: "p", Verdict: "Blocking issues found", Markdown: "m1 detail", Findings: []outFinding{
{Lens: "security", File: "a.go", Line: 10, Severity: "high", Title: "auth bypass"},
{Lens: "perf", File: "b.go", Line: 5, Severity: "trivial", Title: "tiny nit"},
}},
{Model: "m2", Provider: "p", Verdict: "Minor issues", Markdown: "m2 detail", Findings: []outFinding{
{Lens: "security", File: "a.go", Line: 10, Severity: "high", Title: "auth bypass"},
}},
}
out := renderConsensus(models, clusterFindings(models))
// Headline table: the agreed finding with a 2/2 badge.
if !strings.Contains(out, "2/2") {
t.Errorf("expected a 2/2 agreement badge in headline:\n%s", out)
}
if !strings.Contains(out, "auth bypass") || !strings.Contains(out, "a.go:10") {
t.Errorf("headline missing the consensus finding:\n%s", out)
}
// The lone trivial finding is folded, not in the headline table.
if !strings.Contains(out, "single-model finding") {
t.Errorf("expected a folded single-model section:\n%s", out)
}
// Per-model detail is preserved (folded).
if !strings.Contains(out, "m1 detail") || !strings.Contains(out, "m2 detail") {
t.Errorf("per-model detail not preserved:\n%s", out)
}
}
func TestRenderConsensusHighSeverityLoneFindingStaysHeadline(t *testing.T) {
// A single model, single critical finding must still surface in the headline
// (not be folded as "low confidence").
models := []modelFindings{
{Model: "solo", Verdict: "Blocking issues found", Markdown: "x", Findings: []outFinding{
{Lens: "security", File: "a.go", Line: 1, Severity: "critical", Title: "rce"},
}},
}
out := renderConsensus(models, clusterFindings(models))
headline := out
if i := strings.Index(out, "single-model finding"); i >= 0 {
headline = out[:i]
}
if !strings.Contains(headline, "rce") {
t.Errorf("lone critical should be in the headline, not folded:\n%s", out)
}
}
func TestClusterSlidingWindowMergesChain(t *testing.T) {
// Findings at 10, 13, 16 (each 3 apart) from three models must merge into ONE
// cluster — the window slides with the span instead of anchoring at line 10.
models := []modelFindings{
{Model: "m1", Findings: []outFinding{{Lens: "x", File: "a.go", Line: 10, Severity: "medium", Title: "t"}}},
{Model: "m2", Findings: []outFinding{{Lens: "x", File: "a.go", Line: 13, Severity: "medium", Title: "t"}}},
{Model: "m3", Findings: []outFinding{{Lens: "x", File: "a.go", Line: 16, Severity: "medium", Title: "t"}}},
}
clusters := clusterFindings(models)
if len(clusters) != 1 {
t.Fatalf("chain 10/13/16 should merge into 1 cluster, got %d", len(clusters))
}
if len(clusters[0].models) != 3 {
t.Errorf("want 3 models in the merged cluster, got %d", len(clusters[0].models))
}
}
func TestRenderConsensusExcludesErroredFromDenominator(t *testing.T) {
models := []modelFindings{
{Model: "m1", Verdict: "Minor issues", Markdown: "a", Findings: []outFinding{
{Lens: "security", File: "a.go", Line: 9, Severity: "medium", Title: "leak"}}},
{Model: "m2", Verdict: "Minor issues", Markdown: "b", Findings: []outFinding{
{Lens: "security", File: "a.go", Line: 9, Severity: "medium", Title: "leak"}}},
{Model: "broken", Verdict: "reviewer failed", Errored: true, Markdown: "boom"},
}
out := renderConsensus(models, clusterFindings(models))
// Denominator is the 2 effective models, not 3; the failure is noted.
if !strings.Contains(out, "2/2") {
t.Errorf("errored model must be excluded from the denominator (want 2/2):\n%s", out)
}
if !strings.Contains(out, "1 failed") {
t.Errorf("expected a '1 failed' note:\n%s", out)
}
if !strings.Contains(out, "reviewer failed") {
t.Errorf("errored model should still appear (folded) as failed:\n%s", out)
}
}
func TestRenderConsensusLoneHighFolds(t *testing.T) {
// A single-model HIGH (not critical) folds — only consensus or a lone CRITICAL
// earns the headline, so a lone Blocking-lens finding doesn't reintroduce noise.
models := []modelFindings{
{Model: "solo", Verdict: "Blocking issues found", Markdown: "x", Findings: []outFinding{
{Lens: "security", File: "a.go", Line: 1, Severity: "high", Title: "maybe-bug"}}},
}
out := renderConsensus(models, clusterFindings(models))
head := out
if i := strings.Index(out, "single-model finding"); i >= 0 {
head = out[:i]
}
if strings.Contains(head, "maybe-bug") {
t.Errorf("a lone HIGH should fold, not headline:\n%s", out)
}
}
func TestWriteAndConsolidateRoundTrip(t *testing.T) {
dir := t.TempDir()
// Two model artifacts on disk.
write := func(name string, mf modelFindings) {
data, _ := json.Marshal(mf)
if err := os.WriteFile(filepath.Join(dir, name), data, 0o644); err != nil {
t.Fatal(err)
}
}
write("m1.json", modelFindings{Model: "m1", Provider: "ollama", Verdict: "Minor issues", Markdown: "md1",
Findings: []outFinding{{Lens: "security", File: "x.go", Line: 3, Severity: "medium", Title: "leak"}}})
write("m2.json", modelFindings{Model: "m2", Provider: "ollama", Verdict: "Minor issues", Markdown: "md2",
Findings: []outFinding{{Lens: "security", File: "x.go", Line: 3, Severity: "high", Title: "leak"}}})
// A junk file must be skipped, not crash consolidation.
if err := os.WriteFile(filepath.Join(dir, "notes.txt"), []byte("ignore me"), 0o644); err != nil {
t.Fatal(err)
}
t.Setenv("GADFLY_CONSOLIDATE_DIR", dir)
// runConsolidate prints to stdout; capture it.
out := captureStdout(t, func() {
if err := runConsolidate(); err != nil {
t.Fatalf("runConsolidate: %v", err)
}
})
if !strings.HasPrefix(strings.TrimSpace(out), consensusMarker) {
t.Errorf("consolidated output must lead with the marker:\n%s", out)
}
if !strings.Contains(out, "2/2") || !strings.Contains(out, "x.go:3") {
t.Errorf("expected the agreed x.go:3 finding at 2/2:\n%s", out)
}
}
func TestRunConsolidateEmptyDirErrors(t *testing.T) {
t.Setenv("GADFLY_CONSOLIDATE_DIR", t.TempDir())
if err := runConsolidate(); err == nil {
t.Error("want an error for an empty consolidate dir (entrypoint falls back)")
}
}
// captureStdout redirects os.Stdout for the duration of fn and returns what was
// written.
func captureStdout(t *testing.T, fn func()) string {
t.Helper()
orig := os.Stdout
r, w, err := os.Pipe()
if err != nil {
t.Fatal(err)
}
os.Stdout = w
defer func() { os.Stdout = orig }() // restore even if fn panics
done := make(chan string)
go func() {
var sb strings.Builder
buf := make([]byte, 4096)
for {
n, err := r.Read(buf)
if n > 0 {
sb.Write(buf[:n])
}
if err != nil {
break
}
}
r.Close()
done <- sb.String()
}()
fn()
w.Close()
return <-done
}
+19 -1
View File
@@ -28,6 +28,22 @@ func (v verdict) label() string {
}
}
// severity maps a verdict to a canonical severity word (the same vocabulary as
// the structured findings: critical/high/medium/small/trivial). Used as the
// raw_severity for heuristic-scraped findings, which carry no per-finding
// severity of their own — so the telemetry store sees one consistent vocabulary
// instead of a mix of canonical words and full verdict phrases.
func (v verdict) severity() string {
switch v {
case verdictBlocking:
return "high"
case verdictMinor:
return "small"
default:
return "trivial"
}
}
// parseVerdict extracts a specialist's self-reported verdict from its output.
// The base prompt tells each lens to lead with one of the three phrases.
func parseVerdict(out string) verdict {
@@ -100,7 +116,9 @@ func renderConsolidated(results []specialistResult) string {
headline, len(results), strings.Join(specialistNames(results), ", "))
for _, r := range results {
body := strings.TrimSpace(r.out)
// Strip the machine-readable ```gadfly-findings block — it's for tooling
// (telemetry/consolidation), not for human readers of the comment.
body := strings.TrimSpace(stripFindingsBlock(r.out))
if body == "" {
body = "_(no output)_"
}
+24 -23
View File
@@ -10,11 +10,14 @@ package main
// the review markdown consumed by run.sh) and never changes the exit code.
// - It depends only on the Go stdlib (net/http).
//
// Findings are extracted heuristically from each lens's markdown: a "path:line"
// reference (e.g. run/executor.go:166) anchors a finding, whose title is the
// nearest preceding markdown heading / numbered item / bold lead-in (else the
// first sentence of the finding's paragraph). This is best-effort signal for the
// store to aggregate — not a structured contract the reviewer guarantees.
// Findings come PRIMARILY from each lens's machine-readable ```gadfly-findings
// block (findings.go: exact file/line + per-finding severity/confidence). When a
// model emits no parseable block, emit falls back to a heuristic prose scrape: a
// "path:line" reference (e.g. run/executor.go:166) anchors a finding, whose title
// is the nearest preceding markdown heading / numbered item / bold lead-in (else
// the first sentence of the finding's paragraph). The scrape is best-effort
// signal for the store to aggregate, not a structured contract the reviewer
// guarantees.
import (
"bytes"
@@ -50,11 +53,16 @@ var pathLineRe = regexp.MustCompile(`([A-Za-z0-9_./-]+\.[A-Za-z0-9]+):(\d+)`)
var numberedRe = regexp.MustCompile(`^\d+[.)]\s+(.+)$`)
// finding is one extracted issue: where it points and a derived human title.
// severity/confidence are populated from the structured ```gadfly-findings block
// (extractStructuredFindings); they are empty for findings recovered by the
// heuristic prose scrape (parseFindings), which has no per-finding signal.
type finding struct {
file string
line int
title string
detail string
file string
line int
title string
detail string
severity string // per-finding: critical/high/medium/small/trivial ("" if heuristic)
confidence string // per-finding: high/medium/low ("" if heuristic)
}
// runPayload is the POST /runs body. Field names match the gadfly-reports API EXACTLY.
@@ -86,6 +94,7 @@ type reportPayload struct {
Provider string `json:"provider"`
RunID string `json:"run_id"`
RawSeverity string `json:"raw_severity"`
Confidence string `json:"confidence"` // per-finding high/medium/low ("" if heuristic)
Detail string `json:"detail"`
}
@@ -116,21 +125,12 @@ func emit(results []specialistResult, elapsed time.Duration) {
// InputTokens/OutputTokens/CostUSD stay nil -> JSON null (not metered).
}
// collectFindings (consensus.go) skips clean/errored lenses and fills in each
// finding's severity (per-finding when structured, else derived from the lens
// verdict), so every reportPayload carries a canonical raw_severity.
var reports []reportPayload
for _, r := range results {
if r.errored {
continue // a failed lens contributes no findings
}
// A lens that reports "No material issues found" has nothing to flag —
// its path:line references are verification notes ("verified X at
// file:line is safe"), not problems. Extracting them pollutes the
// findings store with false positives and unfairly penalizes thorough
// reviewers that do clean passes, so a clean lens emits no findings.
if r.verdict == verdictClean {
continue
}
sev := r.verdict.label()
for _, f := range parseFindings(r.spec, r.out) {
for _, f := range collectFindings(r) {
reports = append(reports, reportPayload{
Repo: repo,
PR: pr,
@@ -141,7 +141,8 @@ func emit(results []specialistResult, elapsed time.Duration) {
Model: model,
Provider: provider,
RunID: runID,
RawSeverity: sev,
RawSeverity: f.severity,
Confidence: f.confidence,
Detail: f.detail,
})
}
+6 -3
View File
@@ -157,7 +157,7 @@ func TestEmit_PostsRunsAndReports(t *testing.T) {
if len(reportBody) != 2 {
t.Fatalf("/reports array length = %d, want 2", len(reportBody))
}
for _, k := range []string{"repo", "pr", "lens", "file", "line", "title", "model", "provider", "run_id", "raw_severity", "detail"} {
for _, k := range []string{"repo", "pr", "lens", "file", "line", "title", "model", "provider", "run_id", "raw_severity", "confidence", "detail"} {
if _, ok := reportBody[0][k]; !ok {
t.Errorf("/reports[0] missing field %q (got keys %v)", k, keysOf(reportBody[0]))
}
@@ -171,8 +171,11 @@ func TestEmit_PostsRunsAndReports(t *testing.T) {
if reportBody[0]["line"] != float64(166) {
t.Errorf("reports[0].line = %v, want 166", reportBody[0]["line"])
}
if reportBody[0]["raw_severity"] != "Blocking issues found" {
t.Errorf("reports[0].raw_severity = %v, want 'Blocking issues found'", reportBody[0]["raw_severity"])
// No structured block in sampleLensMarkdown, so this is a heuristic-scraped
// finding: raw_severity is the canonical word derived from the lens verdict
// (Blocking -> "high"), not the full verdict phrase.
if reportBody[0]["raw_severity"] != "high" {
t.Errorf("reports[0].raw_severity = %v, want 'high'", reportBody[0]["raw_severity"])
}
if reportBody[0]["run_id"] != "owner/repo#7:ollama-cloud/qwen3" {
t.Errorf("reports[0].run_id = %v, want owner/repo#7:ollama-cloud/qwen3", reportBody[0]["run_id"])
+42 -25
View File
@@ -19,28 +19,35 @@ import (
// the model's text answer. It is the one primitive both review passes use — the
// draft review and the adversarial recheck — so the rest of the pipeline
// (specialist composition, recheck orchestration, consolidation, emit) is
// engine-agnostic. Two implementations:
// engine-agnostic. Three implementations:
//
// - majordomoEngine: the original path — a majordomo tool-using agent loop
// (read_file/grep/… over a sandboxed repoFS).
// - claudeCodeEngine: shells out to the `claude` CLI in print mode, which
// brings its OWN repo tools; gadfly just feeds it the prompt and reads back
// the final text.
// - openCodeEngine (opencode.go): shells out to the `opencode` CLI likewise,
// but driving an ollama-cloud model — for benchmarking the two harnesses on
// the same model.
//
// maxSteps is the tool-step budget for engines that have one (majordomo); the
// claude-code engine manages its own loop and ignores it.
// shell-out engines manage their own loop and ignore it.
type reviewEngine interface {
runPass(ctx context.Context, system, task string, maxSteps int) (string, error)
}
// majordomoEngine drives the in-process majordomo agent over the repo sandbox.
// majordomoEngine drives the in-process review path over the repo sandbox. It no
// longer calls majordomo's agent loop directly: each pass runs through an
// executus run.Executor (see executus.go), which adds context compaction, run
// bounding, the per-PR budget gate, and the wrap-up critic. mdl is retained only
// so auto-select can fall back to the review model as its selector.
type majordomoEngine struct {
mdl llm.Model
fsTools *repoFS
rex *reviewExecutor
mdl llm.Model
}
func (e *majordomoEngine) runPass(ctx context.Context, system, task string, maxSteps int) (string, error) {
return runAgent(ctx, e.mdl, e.fsTools, system, task, maxSteps)
return e.rex.run(ctx, system, task, maxSteps)
}
// claudeCodeEngine reviews by shelling out to the `claude` CLI (Claude Code) in
@@ -153,16 +160,7 @@ func (e *claudeCodeEngine) runPass(ctx context.Context, system, task string, _ i
// Force an extended-thinking budget for this run (a "...:max" spec).
cmd.Env = append(cmd.Env, "MAX_THINKING_TOKENS="+strconv.Itoa(e.thinkingTokens))
}
// 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 claude/node processes behind in the container.
cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
cmd.Cancel = func() error {
if cmd.Process != nil {
_ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL)
}
return nil
}
killGroupOnCancel(cmd)
var stdout, stderr bytes.Buffer
cmd.Stdout = &stdout
cmd.Stderr = &stderr
@@ -213,13 +211,39 @@ func (e *claudeCodeEngine) runPass(ctx context.Context, system, task string, _ i
return "", fmt.Errorf("claude -p produced no parseable output")
}
// killGroupOnCancel puts cmd in its own process group and, on context cancel,
// SIGKILLs the whole group — so a timed-out shell-out CLI (claude/opencode) can't
// leave orphaned Node children behind in the container. Call before cmd.Run.
func killGroupOnCancel(cmd *exec.Cmd) {
cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
cmd.Cancel = func() error {
if cmd.Process != nil {
_ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL)
}
return nil
}
}
// filterEnv returns the current process environment reduced to the variables for
// which keep returns true. Shared by the shell-out engines' minimal-env builders
// (claudeEnv, openCodeEnv), which differ only in their keep predicate.
func filterEnv(keep func(string) bool) []string {
var env []string
for _, kv := range os.Environ() {
if k, _, ok := strings.Cut(kv, "="); ok && keep(k) {
env = append(env, kv)
}
}
return env
}
// claudeEnv builds a minimal environment for the `claude` subprocess: only what
// the CLI needs (PATH/HOME, its auth tokens, locale, Node/XDG/GADFLY_CLAUDE_*
// knobs), deliberately dropping the rest of the runner's secrets — GITEA_TOKEN,
// GADFLY_FINDINGS_TOKEN, provider keys — so they never reach the third-party
// CLI. Defense in depth: the parent already holds them, but the CLI has no need.
func claudeEnv() []string {
keep := func(k string) bool {
return filterEnv(func(k string) bool {
switch k {
case "PATH", "HOME", "USER", "LOGNAME", "TMPDIR", "LANG", "TERM", "SHELL", "MAX_THINKING_TOKENS":
return true
@@ -230,14 +254,7 @@ func claudeEnv() []string {
strings.HasPrefix(k, "GADFLY_CLAUDE_") ||
strings.HasPrefix(k, "NODE_") ||
strings.HasPrefix(k, "XDG_")
}
var env []string
for _, kv := range os.Environ() {
if k, _, ok := strings.Cut(kv, "="); ok && keep(k) {
env = append(env, kv)
}
}
return env
})
}
// truncateForErr caps CLI error detail so a stderr dump can't bloat the comment,
+371
View File
@@ -0,0 +1,371 @@
package main
// executus.go wires gadfly's agentic review path onto the executus run kernel
// (gitea.stevedudenhoeffer.com/steve/executus), layered above majordomo. The
// majordomoEngine no longer drives majordomo's agent loop directly; it builds a
// run.Executor that gives gadfly, for free:
//
// - context compaction (executus/compact): once the transcript a step would
// SEND crosses a token threshold derived from the model's real context
// window, the runaway middle is folded into a one-paragraph summary by a
// cheap summarizer model — so a big diff + accumulating read_file/grep
// results can't balloon every re-sent step (the large-PR burn).
// - run bounding + a per-PR spend budget (executus/run Ports.Budget): a hard
// token/seconds ceiling so a pathological PR can't drain the usage block.
// - the wrap-up nudge, re-expressed as an executus Critic (Ports.Critic): the
// steer that tells a step-hungry model to stop investigating and write its
// answer is now the critic seam, not a bespoke RunOption.
//
// Everything degrades to today's behavior when unconfigured: nil summarizer or a
// 0 context window disables compaction; nil budget disables the ceiling; the
// claude-code engine shells out and is unaffected by any of this.
//
// gadfly keeps its own model.go resolution (so GADFLY_ENDPOINT_<NAME> http
// aliases, failover chains, and the claude-code engine all survive) — the
// run.Executor is handed gadfly's already-resolved model via a trivial resolver,
// not routed through executus's tier table.
import (
"context"
"errors"
"fmt"
"os"
"strconv"
"strings"
"sync"
"sync/atomic"
"time"
llm "gitea.stevedudenhoeffer.com/steve/majordomo/llm"
"gitea.stevedudenhoeffer.com/steve/executus/compact"
"gitea.stevedudenhoeffer.com/steve/executus/model"
exrun "gitea.stevedudenhoeffer.com/steve/executus/run"
exectool "gitea.stevedudenhoeffer.com/steve/executus/tool"
)
const (
// defaultCompactRatio is the fraction of the model's context window at which
// compaction fires. It is deliberately LOWER than executus's own 0.7 default:
// on the large-PR burn the per-step transcript is the embedded diff (~17K) plus
// accumulating read_file results, which rarely reaches 0.7×262K≈183K — so a
// 0.7 threshold never bites. ~0.45×262K≈118K folds the runaway middle while a
// transcript is still well under the cap. Override with GADFLY_COMPACT_RATIO.
defaultCompactRatio = 0.45
// defaultCompactKeepRecent / defaultCompactSummaryWords mirror executus's own
// compactor defaults; surfaced as gadfly env knobs for tuning.
defaultCompactKeepRecent = 8
defaultCompactSummaryWords = 200
// contextTokenLookupTimeout bounds the one-shot /api/show call that resolves a
// cloud model's context window at executor-build time. Kept short so a slow or
// unreachable endpoint adds at most this to startup before degrading to
// no-compaction (rather than the provider cache's default 15s).
contextTokenLookupTimeout = 5 * time.Second
)
// runSeq mints a unique-per-process RunID suffix for each executor run so audit
// and the run kernel can tell one pass from another within a binary process.
var runSeq atomic.Uint64
// wrappedTool adapts an already-built majordomo llm.Tool (gadfly's sandboxed
// read_file/grep/get_diff/… closures over the repoFS) to executus's tool.Tool
// interface so the run kernel can build a toolbox from them by name. gadfly's
// tools need no caller/channel identity, so BuildLLM ignores the Invocation and
// returns the pre-built tool; Permission is the zero value (private, ungated).
type wrappedTool struct{ t llm.Tool }
func (w wrappedTool) Name() string { return w.t.Name }
func (w wrappedTool) Description() string { return w.t.Description }
func (w wrappedTool) Permission() exectool.Permission { return exectool.Permission{} }
func (w wrappedTool) BuildLLM(_ exectool.Invocation) llm.Tool { return w.t }
// gadflyToolRegistry registers the repo's read-only tools (plus the optional
// delegate_investigation worker tool) in a fresh executus tool.Registry and
// returns it along with the tool names for RunnableAgent.LowLevelTools.
func gadflyToolRegistry(fs *repoFS) (exectool.Registry, []string, error) {
reg := exectool.NewRegistry()
tools := fs.allTools()
names := make([]string, 0, len(tools))
for _, t := range tools {
if err := reg.Register(wrappedTool{t: t}); err != nil {
return nil, nil, fmt.Errorf("register tool %q: %w", t.Name, err)
}
names = append(names, t.Name)
}
return reg, names, nil
}
// gadflyBudget is gadfly's per-PR spend ceiling, satisfying run.Ports.Budget.
// It gates a run BEFORE it makes any model call (Check) once the process has
// spent its token or wall-clock allowance on this PR. Tokens are fed in
// out-of-band via addUsage (the Budget interface's Commit only carries seconds);
// the engine calls addUsage after each pass with run.Result.Usage. A nil
// *gadflyBudget is never installed — caps of 0 mean "unlimited", so the port is
// only wired when at least one cap is set.
//
// The guard is PASS-granular: Check runs before each pass, so it stops the NEXT
// pass once the budget is spent but cannot abort a single runaway pass mid-flight.
// The swarm-wide GADFLY_PR_BUDGET_SECS wall-clock backstop (entrypoint.sh) is what
// bounds a mid-pass runaway.
type gadflyBudget struct {
mu sync.Mutex
maxTokens int64
maxSeconds float64
tokens int64
seconds float64
}
// newPRBudget builds the per-PR budget from env, or nil when neither cap is set
// (the default — the swarm-wide ceiling lives in entrypoint.sh; this is the
// per-process belt to its suspenders).
func newPRBudget() *gadflyBudget {
toks := envInt("GADFLY_PR_TOKEN_BUDGET", 0)
secs := envInt("GADFLY_PR_TIME_BUDGET_SECS", 0)
if toks <= 0 && secs <= 0 {
return nil
}
return &gadflyBudget{maxTokens: int64(toks), maxSeconds: float64(secs)}
}
func (b *gadflyBudget) Check(_ context.Context, _ string) error {
if b == nil {
return nil
}
b.mu.Lock()
defer b.mu.Unlock()
if b.maxTokens > 0 && b.tokens >= b.maxTokens {
return fmt.Errorf("gadfly: per-PR token budget exhausted (%d/%d)", b.tokens, b.maxTokens)
}
if b.maxSeconds > 0 && b.seconds >= b.maxSeconds {
return fmt.Errorf("gadfly: per-PR time budget exhausted (%.0f/%.0fs)", b.seconds, b.maxSeconds)
}
return nil
}
func (b *gadflyBudget) Commit(_ context.Context, _ string, runtimeSeconds float64) {
if b == nil {
return
}
b.mu.Lock()
defer b.mu.Unlock()
b.seconds += runtimeSeconds
}
// addUsage records a finished pass's token spend toward the budget. Safe on nil.
func (b *gadflyBudget) addUsage(u llm.Usage) {
if b == nil {
return
}
b.mu.Lock()
defer b.mu.Unlock()
b.tokens += int64(u.InputTokens) + int64(u.OutputTokens)
}
// wrapUpCritic re-expresses gadfly's wrap-up nudge as an executus run.Critic:
// once a run comes within wrapUpReserve steps of its cap, Steer() injects the
// "stop calling tools and write your final answer" message so a thorough model
// spends its last steps finalizing instead of hard-failing empty. It sets no
// hard deadline (Deadline()==zero) and never raises the step ceiling
// (MaxSteps()==0, defer to the run's MaxIterations) — it is purely the nudge.
type wrapUpCritic struct{ reserve int }
func (c *wrapUpCritic) Monitor(_ context.Context, info exrun.RunInfo, _ time.Duration) exrun.CriticHandle {
return &wrapUpHandle{maxSteps: info.MaxIterations, reserve: c.reserve}
}
type wrapUpHandle struct {
mu sync.Mutex
maxSteps int
reserve int
done int // steps completed so far
nudged bool
}
func (h *wrapUpHandle) RecordStep(iter int, _ *llm.Response) {
h.mu.Lock()
h.done = iter + 1
h.mu.Unlock()
}
func (h *wrapUpHandle) RecordToolStart(string, string) {}
func (h *wrapUpHandle) Steer() []llm.Message {
h.mu.Lock()
defer h.mu.Unlock()
at := h.maxSteps - h.reserve
if at < 1 {
at = 1
}
if !h.nudged && h.maxSteps > 0 && h.done >= at {
h.nudged = true
return []llm.Message{llm.UserText(wrapUpInstruction)}
}
return nil
}
func (h *wrapUpHandle) Deadline() time.Time { return time.Time{} }
func (h *wrapUpHandle) MaxSteps() int { return 0 }
func (h *wrapUpHandle) KillCause() error { return nil }
func (h *wrapUpHandle) Stop() {}
// reviewExecutor bundles a run.Executor with the per-run wiring the engine needs
// for each pass (the tool names to expose, the model spec to report as the tier,
// the per-PR caller id, and the budget to feed token usage into).
type reviewExecutor struct {
ex *exrun.Executor
toolNames []string
modelSpec string
callerID string
budget *gadflyBudget
}
// newReviewExecutor builds the run.Executor for the in-process majordomo review
// path. mdl is gadfly's already-resolved review model; summarizer is the cheap
// model the compactor uses (nil disables compaction). Compaction also needs the
// model's context window (resolved once here, not per pass); a 0 window likewise
// disables it. The budget (may be nil) becomes the run.Ports.Budget gate.
func newReviewExecutor(fs *repoFS, mdl, summarizer llm.Model, modelSpec string, budget *gadflyBudget) (*reviewExecutor, error) {
reg, names, err := gadflyToolRegistry(fs)
if err != nil {
return nil, err
}
// gadfly resolves exactly one model per process; the run kernel's resolver
// just hands that model back regardless of the tier string it is asked for.
modelsResolver := func(ctx context.Context, _ string) (context.Context, llm.Model, error) {
return ctx, mdl, nil
}
var compactor compact.CompactorFactory
var ctxTokens func(string) int
if summarizer != nil && compactionEnabled() {
if window := resolveContextTokens(modelSpec); window > 0 {
sumResolver := func(ctx context.Context, _ string) (context.Context, llm.Model, error) {
return ctx, summarizer, nil
}
compactor = compact.NewCompactor(compact.CompactorConfig{
Models: sumResolver,
KeepRecent: envInt("GADFLY_COMPACT_KEEP_RECENT", defaultCompactKeepRecent),
SummaryWordCap: envInt("GADFLY_COMPACT_SUMMARY_WORDS", defaultCompactSummaryWords),
})
ctxTokens = func(string) int { return window } // memoized: one window per process
}
}
var ports exrun.Ports
ports.Critic = &wrapUpCritic{reserve: wrapUpReserve()}
if budget != nil {
ports.Budget = budget
}
cfg := exrun.Config{
Registry: reg,
Models: modelsResolver,
Compactor: compactor,
ContextTokens: ctxTokens,
Defaults: exrun.Defaults{
// MaxIterations/MaxRuntime are intentionally omitted: every pass sets its
// own per-run cap on the RunnableAgent below (the review and recheck caps
// differ), so a Defaults value here would always be overridden — dead. This
// leaves only the cross-pass guards + the compaction ratio.
MaxConsecutiveToolErrors: 4,
MaxSameToolCallRepeats: 4,
CompactionThresholdRatio: compactRatio(),
FallbackTier: modelSpec,
},
Ports: ports,
}
return &reviewExecutor{
ex: exrun.New(cfg),
toolNames: names,
modelSpec: modelSpec,
callerID: prCallerID(),
budget: budget,
}, nil
}
// run executes one agent pass (review or recheck) through the run kernel and
// returns the model's final text. An empty answer with no error is reported as
// an error so the caller (reviewWithSpecialist) renders the advisory "reviewer
// failed to complete" notice rather than a blank section.
func (r *reviewExecutor) run(ctx context.Context, system, task string, maxSteps int) (string, error) {
res := r.ex.Run(ctx, exrun.RunnableAgent{
Name: "gadfly-review",
SystemPrompt: system,
ModelTier: r.modelSpec,
MaxIterations: maxSteps,
MaxRuntime: reviewTimeout(),
LowLevelTools: r.toolNames,
Critic: exrun.CriticConfig{Enabled: true},
}, exectool.Invocation{
RunID: fmt.Sprintf("gadfly-%d", runSeq.Add(1)),
CallerID: r.callerID,
}, task)
// Feed token spend toward the per-PR budget out-of-band (Commit carries only
// seconds; the executor already called it). Safe on a nil budget.
r.budget.addUsage(res.Usage)
if res.Err != nil {
return "", res.Err
}
if out := strings.TrimSpace(res.Output); out != "" {
return out, nil
}
return "", errors.New("agent produced no output")
}
// prCallerID is the budget/audit caller key: the repo + PR, so a budget keyed on
// it is naturally per-PR. Falls back to "local" for an out-of-CI run.
func prCallerID() string {
repo := strings.TrimSpace(os.Getenv("GADFLY_REPO"))
pr := strings.TrimSpace(os.Getenv("GADFLY_PR"))
if repo == "" && pr == "" {
return "local"
}
return repo + "#" + pr
}
// compactionEnabled reports whether context compaction should be wired. On
// unless GADFLY_COMPACT is explicitly falsey.
func compactionEnabled() bool { return envBool("GADFLY_COMPACT", true) }
// compactRatio is the compaction threshold as a fraction of the model context
// window (GADFLY_COMPACT_RATIO), clamped to (0,1]; default defaultCompactRatio.
func compactRatio() float64 {
v := strings.TrimSpace(os.Getenv("GADFLY_COMPACT_RATIO"))
if v == "" {
return defaultCompactRatio
}
f, err := strconv.ParseFloat(v, 64)
if err != nil || f <= 0 || f > 1 {
return defaultCompactRatio
}
return f
}
// resolveContextTokens returns the review model's context window in tokens, used
// to set the compaction threshold. GADFLY_MODEL_CONTEXT_TOKENS overrides it
// (needed for custom/self-hosted endpoints executus can't introspect); otherwise
// it asks executus/model, which knows the static catalog and can fetch an
// Ollama Cloud model's limit via /api/show (one call, at executor-build time).
// Returns 0 — disabling compaction — for an unknown model, mirroring executus's
// "unknown ⇒ don't budget" contract.
func resolveContextTokens(modelSpec string) int {
if v := envInt("GADFLY_MODEL_CONTEXT_TOKENS", 0); v > 0 {
return v
}
key := strings.TrimSpace(os.Getenv("GADFLY_API_KEY"))
if key == "" {
key = strings.TrimSpace(os.Getenv("OLLAMA_API_KEY"))
}
cache := model.NewCloudOllamaLimitCache("", key, nil)
ctx, cancel := context.WithTimeout(context.Background(), contextTokenLookupTimeout)
defer cancel()
if n, ok := model.MaxContextTokensResolving(ctx, modelSpec, cache); ok {
return n
}
// Unknown model or a failed lookup (e.g. no key / unreachable endpoint): don't
// guess — compaction is disabled. Log it so a misconfiguration is debuggable
// rather than silently dropping the protection. Set GADFLY_MODEL_CONTEXT_TOKENS
// to force a window for an endpoint executus can't introspect.
fmt.Fprintf(os.Stderr, "gadfly: no context window resolved for %q; compaction disabled (set GADFLY_MODEL_CONTEXT_TOKENS to enable it)\n", modelSpec)
return 0
}
+140
View File
@@ -0,0 +1,140 @@
package main
import (
"context"
"testing"
llm "gitea.stevedudenhoeffer.com/steve/majordomo/llm"
)
func TestGadflyBudget(t *testing.T) {
ctx := context.Background()
// A nil budget never blocks and never panics.
var nilB *gadflyBudget
if err := nilB.Check(ctx, "pr"); err != nil {
t.Errorf("nil budget Check should be nil, got %v", err)
}
nilB.Commit(ctx, "pr", 10)
nilB.addUsage(llm.Usage{InputTokens: 5})
// Token ceiling: passes until usage crosses it.
b := &gadflyBudget{maxTokens: 100}
if err := b.Check(ctx, "pr"); err != nil {
t.Fatalf("fresh budget should pass, got %v", err)
}
b.addUsage(llm.Usage{InputTokens: 60, OutputTokens: 50}) // 110 >= 100
if err := b.Check(ctx, "pr"); err == nil {
t.Error("budget over the token cap should reject the next run")
}
// Seconds ceiling, accumulated via Commit.
s := &gadflyBudget{maxSeconds: 30}
s.Commit(ctx, "pr", 31)
if err := s.Check(ctx, "pr"); err == nil {
t.Error("budget over the time cap should reject the next run")
}
}
func TestNewPRBudget(t *testing.T) {
t.Setenv("GADFLY_PR_TOKEN_BUDGET", "")
t.Setenv("GADFLY_PR_TIME_BUDGET_SECS", "")
if newPRBudget() != nil {
t.Error("no caps set should yield a nil (disabled) budget")
}
t.Setenv("GADFLY_PR_TOKEN_BUDGET", "1000")
if b := newPRBudget(); b == nil || b.maxTokens != 1000 {
t.Errorf("token cap should build a budget with maxTokens=1000, got %+v", b)
}
}
func TestCompactRatio(t *testing.T) {
t.Setenv("GADFLY_COMPACT_RATIO", "")
if got := compactRatio(); got != defaultCompactRatio {
t.Errorf("default ratio = %v, want %v", got, defaultCompactRatio)
}
t.Setenv("GADFLY_COMPACT_RATIO", "0.6")
if got := compactRatio(); got != 0.6 {
t.Errorf("ratio override = %v, want 0.6", got)
}
for _, bad := range []string{"0", "-1", "2", "nope"} {
t.Setenv("GADFLY_COMPACT_RATIO", bad)
if got := compactRatio(); got != defaultCompactRatio {
t.Errorf("invalid ratio %q should fall back to the default, got %v", bad, got)
}
}
}
func TestResolveContextTokensOverride(t *testing.T) {
// The explicit override short-circuits any model introspection (no network).
t.Setenv("GADFLY_MODEL_CONTEXT_TOKENS", "123456")
if got := resolveContextTokens("anything"); got != 123456 {
t.Errorf("explicit context-token override = %d, want 123456", got)
}
}
func TestPRCallerID(t *testing.T) {
t.Setenv("GADFLY_REPO", "")
t.Setenv("GADFLY_PR", "")
if got := prCallerID(); got != "local" {
t.Errorf("no repo/PR should be %q, got %q", "local", got)
}
t.Setenv("GADFLY_REPO", "steve/mort")
t.Setenv("GADFLY_PR", "1367")
if got := prCallerID(); got != "steve/mort#1367" {
t.Errorf("callerID = %q, want steve/mort#1367", got)
}
}
func TestCompactionEnabled(t *testing.T) {
for _, v := range []string{"", "1", "true", "yes"} {
t.Setenv("GADFLY_COMPACT", v)
if !compactionEnabled() {
t.Errorf("GADFLY_COMPACT=%q should be enabled", v)
}
}
for _, v := range []string{"0", "false", "no", "off"} {
t.Setenv("GADFLY_COMPACT", v)
if compactionEnabled() {
t.Errorf("GADFLY_COMPACT=%q should be disabled", v)
}
}
}
func TestGadflyToolRegistry(t *testing.T) {
fs, err := newRepoFS(t.TempDir(), "diff")
if err != nil {
t.Fatal(err)
}
_, names, err := gadflyToolRegistry(fs)
if err != nil {
t.Fatalf("gadflyToolRegistry: %v", err)
}
want := map[string]bool{"read_file": true, "list_dir": true, "grep": true, "find_files": true, "get_diff": true}
has := func(n string) bool {
for _, g := range names {
if g == n {
return true
}
}
return false
}
for n := range want {
if !has(n) {
t.Errorf("registry missing tool %q (got %v)", n, names)
}
}
if has("delegate_investigation") {
t.Error("delegate_investigation must be absent without a worker model")
}
// With a worker model the delegate tool is registered too.
fs.worker = fakeModel(t, "x")
_, names, err = gadflyToolRegistry(fs)
if err != nil {
t.Fatalf("gadflyToolRegistry with worker: %v", err)
}
if !has("delegate_investigation") {
t.Errorf("delegate_investigation should be registered with a worker model, got %v", names)
}
}
+236
View File
@@ -0,0 +1,236 @@
package main
// Structured findings: the machine-readable contract between a lens's output and
// the telemetry/consolidation pipeline. Each lens is asked (see
// scripts/system-prompt.txt) to append a fenced ```gadfly-findings code block
// holding a JSON array of its findings. Parsing that exact block is far more
// reliable than scraping prose with a path:line regex (emit.go's heuristic),
// and it carries PER-FINDING severity + confidence the prose verdict can't.
//
// Everything here degrades gracefully: a missing, unterminated, or malformed
// block makes extractStructuredFindings return ok=false (and yield no findings),
// so the caller falls back to the heuristic scrape — a weak model that ignores
// the contract still contributes findings, exactly as before.
import (
"encoding/json"
"strconv"
"strings"
)
// structuredFinding mirrors one element of the ```gadfly-findings JSON array.
// Line is json.Number so we tolerate both 123 and "123" from less-precise models.
type structuredFinding struct {
File string `json:"file"`
Line json.Number `json:"line"`
Severity string `json:"severity"`
Confidence string `json:"confidence"`
Title string `json:"title"`
Detail string `json:"detail"` // optional; the prose paragraph is used when absent
}
// findingsFence is the info-string that tags the machine-readable block.
const findingsFence = "gadfly-findings"
// extractStructuredFindings parses the ```gadfly-findings JSON block out of a
// lens's markdown. It returns the findings and ok=true when a TERMINATED block is
// present AND parses as a JSON array (an empty array is valid "nothing found" —
// ok is still true). A missing, unterminated, or unparseable block returns
// ok=false so the caller falls back to the heuristic scrape.
//
// Findings are deduped by file:line (keeping the first, matching parseFindings),
// findings with no usable file are dropped, and each title/detail is backfilled
// from the prose when the JSON omits it (best of both: exact location + the human
// context the model already wrote).
func extractStructuredFindings(out string) ([]finding, bool) {
lines := strings.Split(out, "\n")
start, end, ok := findingsSpan(lines)
if !ok {
return nil, false
}
var raw []structuredFinding
if err := json.Unmarshal([]byte(strings.Join(lines[start+1:end], "\n")), &raw); err != nil {
return nil, false
}
findings := make([]finding, 0, len(raw))
seen := map[string]bool{}
var prose map[string]string // built lazily — only if a finding lacks its own detail
for _, sf := range raw {
file := strings.TrimSpace(sf.File)
if file == "" {
continue
}
ln := 0
if n, err := strconv.Atoi(strings.TrimSpace(sf.Line.String())); err == nil && n > 0 {
ln = n
}
key := file + ":" + strconv.Itoa(ln)
if ln > 0 { // only dedupe concrete locations; unknown-line findings are kept
if seen[key] {
continue
}
seen[key] = true
}
detail := strings.TrimSpace(sf.Detail)
if detail == "" && ln > 0 {
if prose == nil {
prose = proseParagraphs(out)
}
detail = prose[key]
}
title := strings.TrimSpace(sf.Title)
if title == "" { // never empty: fall back to the prose detail, then the location
if detail != "" {
title = truncate(detail, 120)
} else if ln > 0 {
title = key
} else {
title = file
}
}
findings = append(findings, finding{
file: file,
line: ln,
title: title,
detail: truncate(detail, 500),
severity: normalizeSeverity(sf.Severity),
confidence: normalizeConfidence(sf.Confidence),
})
if len(findings) >= maxFindingsPerLens {
break
}
}
return findings, true
}
// extractStructuredFindingsOrScrape returns a lens's findings, preferring the
// structured ```gadfly-findings block and falling back to the heuristic prose
// scrape when the block is absent, unterminated/malformed, OR parsed to zero
// usable findings (e.g. an empty [] emitted alongside real prose findings).
// Factored out of emit() so the fallback rule is unit-testable.
func extractStructuredFindingsOrScrape(r specialistResult) []finding {
if fs, _ := extractStructuredFindings(r.out); len(fs) > 0 {
return fs
}
return parseFindings(r.spec, r.out)
}
// stripFindingsBlock removes every TERMINATED ```gadfly-findings block from out so
// the machine-readable JSON never shows in the rendered comment. An UNTERMINATED
// fence is left in place — treating it as a block would swallow the rest of the
// comment (e.g. when a model's output was truncated mid-block). Trailing
// whitespace is trimmed.
func stripFindingsBlock(out string) string {
lines := strings.Split(out, "\n")
for {
start, end, ok := findingsSpan(lines)
if !ok {
break
}
lines = append(lines[:start:start], lines[end+1:]...)
}
return strings.TrimRight(strings.Join(lines, "\n"), "\n")
}
// findingsSpan returns the [start,end] inclusive line indices of the first
// TERMINATED ```gadfly-findings block in lines (start = the opening fence, end =
// the closing fence), or ok=false when there is none or it is unterminated.
// extract and strip share it so they always agree on what is (and isn't) a block.
func findingsSpan(lines []string) (start, end int, ok bool) {
start = -1
for i, ln := range lines {
if isFindingsOpen(ln) {
start = i
break
}
}
if start < 0 {
return 0, 0, false
}
for j := start + 1; j < len(lines); j++ {
if isFenceClose(lines[j]) {
return start, j, true
}
}
return 0, 0, false // unterminated
}
// fenceInfo returns the info-string (text after the backticks) of a code-fence
// line and whether the line opens/closes a fence at all. A bare ``` yields ("",
// true); ```gadfly-findings yields ("gadfly-findings", true).
func fenceInfo(line string) (string, bool) {
t := strings.TrimSpace(line)
if !strings.HasPrefix(t, "```") {
return "", false
}
return strings.TrimSpace(strings.TrimLeft(t, "`")), true
}
// isFindingsOpen reports whether line opens a ```gadfly-findings block, matching
// the info-string EXACTLY (not as a substring) so a fence like ```not-findings
// can't masquerade as ours.
func isFindingsOpen(line string) bool {
info, ok := fenceInfo(line)
return ok && info == findingsFence
}
// isFenceClose reports whether line is a bare closing fence (``` with no info).
func isFenceClose(line string) bool {
info, ok := fenceInfo(line)
return ok && info == ""
}
// proseParagraphs maps "file:line" -> the prose paragraph that first references
// it, so a structured finding without its own detail can borrow the human
// context the model already wrote. Built from the markdown OUTSIDE the findings
// block (the block is JSON, not prose).
func proseParagraphs(out string) map[string]string {
prose := stripFindingsBlock(out)
lines := strings.Split(prose, "\n")
m := map[string]string{}
for _, loc := range pathLineRe.FindAllStringSubmatchIndex(prose, -1) {
key := prose[loc[2]:loc[3]] + ":" + prose[loc[4]:loc[5]]
if _, dup := m[key]; dup {
continue
}
li := strings.Count(prose[:loc[0]], "\n")
m[key] = paragraphAt(lines, li)
}
return m
}
// normalizeSeverity maps a model's severity word onto the canonical set
// (critical/high/medium/small/trivial), accepting common synonyms. An
// unrecognized value is returned lowercased so the store still sees the raw word.
func normalizeSeverity(s string) string {
switch t := strings.ToLower(strings.TrimSpace(s)); t {
case "critical", "crit", "blocker", "blocking":
return "critical"
case "high", "major", "severe":
return "high"
case "medium", "moderate":
return "medium"
case "small", "low", "minor":
return "small"
case "trivial", "nit", "nitpick", "info", "informational", "style", "cosmetic":
return "trivial"
default:
return t
}
}
// normalizeConfidence maps a model's confidence word onto high/medium/low,
// accepting common synonyms; an unrecognized value is returned lowercased.
func normalizeConfidence(s string) string {
switch t := strings.ToLower(strings.TrimSpace(s)); t {
case "high", "certain", "confirmed", "verified":
return "high"
case "medium", "med", "moderate":
return "medium"
case "low", "unsure", "tentative", "unverified", "speculative":
return "low"
default:
return t
}
}
+146
View File
@@ -0,0 +1,146 @@
package main
import (
"strings"
"testing"
)
const sampleReview = "**Blocking issues found**\n\n" +
"- **Unauthenticated endpoint** — `model.go:184` leaks PR content to a third party.\n" +
"- A nit about naming in `util.go:12`.\n\n" +
"```gadfly-findings\n" +
"[\n" +
" {\"file\": \"model.go\", \"line\": 184, \"severity\": \"high\", \"confidence\": \"high\", \"title\": \"Unauthenticated endpoint\"},\n" +
" {\"file\": \"util.go\", \"line\": 12, \"severity\": \"nit\", \"confidence\": \"medium\", \"title\": \"naming\"}\n" +
"]\n" +
"```\n"
func TestExtractStructuredFindings(t *testing.T) {
fs, ok := extractStructuredFindings(sampleReview)
if !ok {
t.Fatal("expected ok=true for a well-formed block")
}
if len(fs) != 2 {
t.Fatalf("want 2 findings, got %d", len(fs))
}
if fs[0].file != "model.go" || fs[0].line != 184 || fs[0].severity != "high" || fs[0].confidence != "high" {
t.Errorf("finding[0] mismatch: %+v", fs[0])
}
// "nit" must normalize to the canonical "trivial".
if fs[1].severity != "trivial" {
t.Errorf("want severity normalized to trivial, got %q", fs[1].severity)
}
// Detail is borrowed from the prose paragraph referencing the same file:line.
if fs[0].detail == "" {
t.Error("expected detail borrowed from prose, got empty")
}
}
func TestExtractStructuredFindingsEmptyArray(t *testing.T) {
out := "No material issues found\n\n```gadfly-findings\n[]\n```\n"
fs, ok := extractStructuredFindings(out)
if !ok {
t.Fatal("an empty array is a valid block; want ok=true")
}
if len(fs) != 0 {
t.Fatalf("want 0 findings, got %d", len(fs))
}
}
func TestExtractStructuredFindingsFallback(t *testing.T) {
// No block at all -> ok=false so the caller uses the heuristic scrape.
if _, ok := extractStructuredFindings("Minor issues\n\n- something at `x.go:1`\n"); ok {
t.Error("want ok=false when there is no block")
}
// Malformed JSON -> ok=false (graceful fallback).
bad := "Minor issues\n\n```gadfly-findings\n{not json}\n```\n"
if _, ok := extractStructuredFindings(bad); ok {
t.Error("want ok=false for malformed JSON")
}
}
func TestExtractStructuredFindingsStringLine(t *testing.T) {
// Tolerate a quoted line number from a less-precise model.
out := "Minor issues\n\n```gadfly-findings\n[{\"file\":\"a.go\",\"line\":\"42\",\"severity\":\"medium\",\"title\":\"x\"}]\n```\n"
fs, ok := extractStructuredFindings(out)
if !ok || len(fs) != 1 || fs[0].line != 42 {
t.Fatalf("want one finding at line 42, got ok=%v %+v", ok, fs)
}
}
func TestStripFindingsBlock(t *testing.T) {
stripped := stripFindingsBlock(sampleReview)
if strings.Contains(stripped, findingsFence) {
t.Errorf("block not stripped: %q", stripped)
}
// The prose findings must survive.
if !strings.Contains(stripped, "Unauthenticated endpoint") || !strings.Contains(stripped, "util.go:12") {
t.Errorf("prose lost during strip: %q", stripped)
}
}
func TestStripFindingsBlockUnterminated(t *testing.T) {
// A truncated, unterminated block must NOT swallow the prose before it.
out := "Minor issues\n\n- real finding at `x.go:1`\n\n```gadfly-findings\n[{\"file\":\"x.go\""
got := stripFindingsBlock(out)
if !strings.Contains(got, "real finding at `x.go:1`") {
t.Errorf("unterminated block swallowed the prose: %q", got)
}
}
func TestStripFindingsBlockNoBlock(t *testing.T) {
in := "Minor issues\n\n- finding at `x.go:9`"
if out := stripFindingsBlock(in); out != in {
t.Errorf("strip changed block-free text: %q != %q", out, in)
}
}
func TestNormalizeSeverity(t *testing.T) {
cases := map[string]string{
"Critical": "critical", "blocker": "critical",
"major": "high", "HIGH": "high",
"moderate": "medium",
"minor": "small", "low": "small", // "minor" and "low" both map to small (consistently)
"nit": "trivial", "Style": "trivial",
"weird": "weird", // unknown passes through, lowercased
}
for in, want := range cases {
if got := normalizeSeverity(in); got != want {
t.Errorf("normalizeSeverity(%q) = %q, want %q", in, got, want)
}
}
}
func TestNormalizeConfidence(t *testing.T) {
cases := map[string]string{
"High": "high", "confirmed": "high",
"MEDIUM": "medium", "moderate": "medium",
"low": "low", "unverified": "low",
"hunch": "hunch", // unknown passes through, lowercased
}
for in, want := range cases {
if got := normalizeConfidence(in); got != want {
t.Errorf("normalizeConfidence(%q) = %q, want %q", in, got, want)
}
}
}
func TestExtractStructuredFindingsFallbackOnEmpty(t *testing.T) {
// A non-clean lens that emitted an empty [] but listed prose findings must
// fall through to the heuristic scrape, not silently drop everything.
out := "Minor issues\n\n- bug at `pkg/a.go:7`\n\n```gadfly-findings\n[]\n```\n"
r := specialistResult{spec: Specialist{Name: "correctness"}, out: out, verdict: verdictMinor}
fs := extractStructuredFindingsOrScrape(r)
if len(fs) == 0 {
t.Fatal("empty [] must fall back to the heuristic scrape, got no findings")
}
if fs[0].file != "pkg/a.go" || fs[0].line != 7 {
t.Errorf("heuristic fallback wrong: %+v", fs[0])
}
}
func TestVerdictSeverity(t *testing.T) {
if verdictBlocking.severity() != "high" || verdictMinor.severity() != "small" || verdictUnknown.severity() != "trivial" {
t.Error("verdict.severity mapping changed unexpectedly")
}
}
+4 -4
View File
@@ -104,7 +104,7 @@ func TestRunSpecialists_FansOut(t *testing.T) {
}
specs := threeLenses()
results := runSpecialists(&majordomoEngine{mdl: mdl, fsTools: fs}, "sys", specs, "task", "diff")
results := runSpecialists(testEngine(t, mdl, fs), "sys", specs, "task", "diff")
if got := peak(); got != 3 {
t.Errorf("peak concurrent lenses = %d, want 3", got)
@@ -124,7 +124,7 @@ func TestRunSpecialists_SequentialByDefault(t *testing.T) {
}
specs := threeLenses()
results := runSpecialists(&majordomoEngine{mdl: mdl, fsTools: fs}, "sys", specs, "task", "diff")
results := runSpecialists(testEngine(t, mdl, fs), "sys", specs, "task", "diff")
if got := peak(); got != 1 {
t.Errorf("peak concurrent lenses = %d, want 1 (sequential by default)", got)
@@ -146,7 +146,7 @@ func TestRunSpecialists_PerProviderFanOut(t *testing.T) {
}
specs := threeLenses()
results := runSpecialists(&majordomoEngine{mdl: mdl, fsTools: fs}, "sys", specs, "task", "diff")
results := runSpecialists(testEngine(t, mdl, fs), "sys", specs, "task", "diff")
if got := peak(); got != 3 {
t.Errorf("peak concurrent lenses = %d, want 3 (m1 per-provider override)", got)
@@ -156,7 +156,7 @@ func TestRunSpecialists_PerProviderFanOut(t *testing.T) {
// TestLensConcurrency covers the resolution matrix: scalar default, scalar
// override, and per-provider override keyed by the model's resolved lane (same
// lane rule entrypoint.sh uses for GADFLY_PROVIDER_CONCURRENCY).
// lane rule entrypoint.sh uses for GADFLY_PROVIDER_LENS_CONCURRENCY).
func TestLensConcurrency(t *testing.T) {
tests := []struct {
name string
+115
View File
@@ -0,0 +1,115 @@
package main
import (
"context"
"errors"
"fmt"
"os"
"path/filepath"
"syscall"
"time"
)
// lensSem is the PROVIDER-WIDE lens-permit pool. Historically each model's binary
// throttled its own lenses in-process (GADFLY_LENS_CONCURRENCY) while entrypoint.sh
// separately capped how many MODELS ran at once — two multiplicative gates in
// different processes. That let a model hold its whole slot until its last lens
// finished, stalling the next model even with idle lens capacity.
//
// Instead, entrypoint.sh now runs all of a provider's models concurrently and
// seeds ONE directory of N permit files per provider (N = the provider's lens
// budget). Every model process in that lane draws from the same pool: a lens pass
// (review + recheck) acquires a permit before it runs and releases it after, so a
// model winding down immediately yields its freed permits to another model's
// queued lenses. Permits are held with flock, which the kernel drops when the
// holding process exits — so a killed/crashed model frees its permits for free.
type lensSem struct {
dir string
size int
}
// lensSemPollInterval is how often a blocked acquirer re-sweeps the permit files.
// Lens passes run for many seconds to minutes, so a coarse poll adds negligible
// latency while keeping the mechanism a few lines of stdlib (no IPC primitives).
const lensSemPollInterval = 150 * time.Millisecond
// activeLensSem returns the shared semaphore configured by entrypoint.sh, or nil
// when it isn't set (standalone/local runs, tests, or an older entrypoint) — in
// which case runSpecialists falls back to the in-process fanout limit alone, i.e.
// the pre-existing per-model behavior.
func activeLensSem() *lensSem {
dir := os.Getenv("GADFLY_LENS_SEM_DIR")
if dir == "" {
return nil
}
n := envInt("GADFLY_LENS_SEM_SIZE", 0)
if n < 1 {
// A dir was requested but the size is missing/blank/invalid. Rather than
// silently run unthrottled, say so on stderr — it's almost certainly a
// misconfiguration in entrypoint.sh (the two are set together).
fmt.Fprintf(os.Stderr, "gadfly: GADFLY_LENS_SEM_DIR set but GADFLY_LENS_SEM_SIZE=%q invalid; lenses run unthrottled\n", os.Getenv("GADFLY_LENS_SEM_SIZE"))
return nil
}
return &lensSem{dir: dir, size: n}
}
// acquire blocks until a permit is free (or ctx is done) and returns a release
// func. It FAILS OPEN: if the pool directory is structurally unusable (missing,
// unwritable, or on a filesystem without flock) it logs once and returns a no-op
// release with a nil error, so the lens runs UNTHROTTLED rather than hanging the
// whole review forever — this is an advisory reviewer, a slightly oversubscribed
// backend beats a stuck one. Only a full-but-healthy pool actually blocks; on ctx
// cancellation it returns a no-op release plus ctx.Err() so the caller can surface
// the lens as "did not run" instead of leaking a permit.
func (s *lensSem) acquire(ctx context.Context) (func(), error) {
for {
release, ok, err := s.tryAcquire()
if ok {
return release, nil
}
if err != nil {
fmt.Fprintf(os.Stderr, "gadfly: lens permit pool %q unusable (%v); proceeding without a permit\n", s.dir, err)
return func() {}, nil
}
timer := time.NewTimer(lensSemPollInterval)
select {
case <-ctx.Done():
timer.Stop() // don't leak the timer when we bail on cancellation
return func() {}, ctx.Err()
case <-timer.C:
}
}
}
// tryAcquire makes one non-blocking sweep over the permit files. It returns a
// release func for the first one it locks; otherwise it distinguishes a healthy
// FULL pool (some permit was openable but flock-busy → ok=false, err=nil → keep
// polling) from a structurally BROKEN pool (no permit was even acquirable and none
// was merely busy → err set → caller fails open). Permit files are created lazily
// (entrypoint.sh only guarantees the directory exists). Closing the *os.File in
// the returned func drops the flock (the lock lives on the open file description).
func (s *lensSem) tryAcquire() (func(), bool, error) {
sawBusy := false
var structural error
for i := 0; i < s.size; i++ {
path := filepath.Join(s.dir, fmt.Sprintf("permit.%d", i))
f, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0o600)
if err != nil {
structural = err // e.g. the pool dir is gone or unwritable
continue
}
if err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB); err == nil {
return func() { f.Close() }, true, nil
} else if errors.Is(err, syscall.EWOULDBLOCK) {
sawBusy = true // this permit is held by someone else — pool has capacity
f.Close()
} else {
structural = err // flock unsupported / other → not a transient "busy"
f.Close()
}
}
if sawBusy {
return nil, false, nil // full but healthy: another lens will free a permit
}
return nil, false, structural
}
+136
View File
@@ -0,0 +1,136 @@
package main
import (
"context"
"path/filepath"
"sync"
"sync/atomic"
"testing"
"time"
)
// flock permits are per open-file-description, so two separate opens of the same
// permit file conflict even within one process — the exhaustion, blocking, and
// max-in-flight tests below therefore exercise the same semantics a real
// multi-process lane would see.
func TestActiveLensSemUnset(t *testing.T) {
t.Setenv("GADFLY_LENS_SEM_DIR", "")
if s := activeLensSem(); s != nil {
t.Fatalf("expected nil sem when GADFLY_LENS_SEM_DIR unset, got %+v", s)
}
// A dir with a zero/blank size is inert too (must not divide the pool by 0).
t.Setenv("GADFLY_LENS_SEM_DIR", t.TempDir())
t.Setenv("GADFLY_LENS_SEM_SIZE", "0")
if s := activeLensSem(); s != nil {
t.Fatalf("expected nil sem when size < 1, got %+v", s)
}
}
func TestLensSemExhaustionAndRelease(t *testing.T) {
s := &lensSem{dir: t.TempDir(), size: 2}
r1, ok, err := s.tryAcquire()
if !ok || err != nil {
t.Fatalf("first acquire should succeed: ok=%v err=%v", ok, err)
}
r2, ok, err := s.tryAcquire()
if !ok || err != nil {
t.Fatalf("second acquire should succeed: ok=%v err=%v", ok, err)
}
// Pool full but healthy: ok=false with NO error (keep polling), not a
// structural failure.
if _, ok, err := s.tryAcquire(); ok || err != nil {
t.Fatalf("third acquire should be full-but-healthy: ok=%v err=%v", ok, err)
}
r1() // free one permit
r3, ok, err := s.tryAcquire()
if !ok || err != nil {
t.Fatalf("acquire should succeed after a release: ok=%v err=%v", ok, err)
}
r2()
r3()
}
// A structurally broken pool (dir missing/unwritable) must FAIL OPEN — tryAcquire
// surfaces the error and acquire returns promptly with a no-op release and no
// error, so a lens runs unthrottled instead of spinning forever.
func TestLensSemBrokenPoolFailsOpen(t *testing.T) {
s := &lensSem{dir: filepath.Join(t.TempDir(), "does", "not", "exist"), size: 2}
if _, ok, err := s.tryAcquire(); ok || err == nil {
t.Fatalf("tryAcquire on a broken pool should report a structural error: ok=%v err=%v", ok, err)
}
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
defer cancel()
start := time.Now()
release, err := s.acquire(ctx)
if err != nil {
t.Fatalf("acquire should fail open (nil err), got %v", err)
}
if waited := time.Since(start); waited > time.Second {
t.Fatalf("acquire on a broken pool should return promptly, waited %v", waited)
}
release() // must be a safe no-op
}
func TestLensSemAcquireBlocksThenCancels(t *testing.T) {
s := &lensSem{dir: t.TempDir(), size: 1}
// Immediate success while a permit is free.
release, err := s.acquire(context.Background())
if err != nil {
t.Fatalf("acquire on a free pool: %v", err)
}
// With the only permit held, acquire must block until ctx expires.
ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond)
defer cancel()
start := time.Now()
if _, err := s.acquire(ctx); err == nil {
t.Fatal("acquire on an exhausted pool should return ctx error, not a permit")
}
if waited := time.Since(start); waited < 50*time.Millisecond {
t.Fatalf("acquire returned too fast (%v); it should have blocked on the full pool", waited)
}
release()
}
func TestLensSemNeverExceedsSize(t *testing.T) {
const size = 3
s := &lensSem{dir: t.TempDir(), size: size}
var inFlight, peak int64
var wg sync.WaitGroup
for i := 0; i < 12; i++ {
wg.Add(1)
go func() {
defer wg.Done()
release, err := s.acquire(context.Background())
if err != nil {
t.Errorf("acquire: %v", err)
return
}
defer release()
n := atomic.AddInt64(&inFlight, 1)
for {
p := atomic.LoadInt64(&peak)
if n <= p || atomic.CompareAndSwapInt64(&peak, p, n) {
break
}
}
time.Sleep(20 * time.Millisecond)
atomic.AddInt64(&inFlight, -1)
}()
}
wg.Wait()
if peak > size {
t.Fatalf("max in-flight lens permits = %d, exceeds budget %d", peak, size)
}
if peak == 0 {
t.Fatal("no permits were ever acquired")
}
}
+159 -177
View File
@@ -22,7 +22,11 @@
//
// GADFLY_MODEL model id, or a full "provider/model" spec / majordomo
// alias / failover chain (required). A bare id is
// prefixed with GADFLY_PROVIDER.
// prefixed with GADFLY_PROVIDER. Two prefixes select a
// shell-out CLI engine instead of the in-process loop:
// "claude-code/<model>" (Claude Code, see engine.go) and
// "opencode/<model>" (OpenCode over ollama-cloud, see
// opencode.go) — both bring their own repo tools.
// GADFLY_PROVIDER provider for bare model ids (default "ollama-cloud";
// e.g. "ollama" for a local daemon, "openai", …).
// GADFLY_BASE_URL override the backend endpoint (OpenAI/Ollama-compatible
@@ -39,24 +43,26 @@
// GADFLY_TITLE PR title (optional).
// GADFLY_BODY PR description (optional).
// GADFLY_MAX_STEPS review-pass step cap (optional, default 24).
// GADFLY_WRAPUP_RESERVE steps before the cap at which the agent is told to
// stop investigating and write its answer (optional,
// default 4). Plus a tool-free finalization fallback
// guarantees a step-exhausted pass still emits output.
// GADFLY_WRAPUP_RESERVE steps before the cap at which the wrap-up critic nudges
// the agent to stop investigating and write its answer
// (optional, default 4).
// GADFLY_RECHECK set to 0/false to skip the recheck pass (optional, default on).
// GADFLY_RECHECK_MAX_STEPS recheck-pass step cap (optional, default 16).
// GADFLY_TIMEOUT_SECS overall deadline in seconds, shared by both passes (optional, default 300).
// GADFLY_LENS_CONCURRENCY how many specialist lenses run concurrently within this
// model (optional, default 1 = sequential). Total in-flight
// model requests ≈ this × entrypoint.sh's per-provider model
// concurrency, so keep the product within the backend's budget.
// GADFLY_LENS_CONCURRENCY how many specialist lenses run concurrently (optional,
// default 1 = sequential). Under entrypoint.sh this is the
// PROVIDER-WIDE lens budget, shared across all of that
// provider's models via a permit pool (GADFLY_LENS_SEM_DIR),
// so it bounds total lens passes in flight per provider.
// GADFLY_PROVIDER_LENS_CONCURRENCY per-provider override for the above, as a
// "provider=N,provider=N" map keyed by the SAME provider
// lanes as GADFLY_PROVIDER_CONCURRENCY (e.g.
// "ollama-cloud=3,m1=1"). Wins over GADFLY_LENS_CONCURRENCY
// for the model's provider; falls back to it otherwise.
// GADFLY_MAX_DIFF_CHARS diff chars embedded in the prompt (optional, default 60000;
// the full diff is always available via the get_diff tool).
// "provider=N,provider=N" map keyed by the provider lanes
// (e.g. "ollama-cloud=3,m1=1"). Wins over
// GADFLY_LENS_CONCURRENCY for the model's provider.
// GADFLY_LENS_SEM_DIR / GADFLY_LENS_SEM_SIZE set by entrypoint.sh: the shared
// cross-process lens-permit pool (dir of N flock files)
// and its size. Unset => in-process lensConcurrency only.
// GADFLY_MAX_DIFF_CHARS diff chars embedded in the review prompt (optional, default 60000;
// the full diff is reachable via the paginated get_diff tool).
//
// On success it prints the review to stdout and exits 0. On a usage/config or
// model error it prints a diagnostic to stderr and exits non-zero; run.sh then
@@ -70,11 +76,9 @@ import (
"os"
"strconv"
"strings"
"sync"
"time"
"gitea.stevedudenhoeffer.com/steve/majordomo/agent"
llm "gitea.stevedudenhoeffer.com/steve/majordomo/llm"
"gitea.stevedudenhoeffer.com/steve/executus/fanout"
)
const (
@@ -93,9 +97,11 @@ const (
// calls and then hard-failing with "max steps reached without a final
// answer" — it always has a few steps left to wrap up.
defaultWrapUpReserve = 4
// defaultLensConcurrency is how many specialist lenses run at once within a
// single model. 1 keeps the suite sequential (the historical behavior);
// higher values overlap the independent per-lens passes. See runSpecialists.
// defaultLensConcurrency is the fallback lens budget when neither
// GADFLY_PROVIDER_LENS_CONCURRENCY nor GADFLY_LENS_CONCURRENCY is set. 1 keeps
// the suite sequential (the historical behavior); higher values overlap the
// independent per-lens passes. Under entrypoint.sh the resolved value is the
// provider-wide budget shared across the provider's models. See runSpecialists.
defaultLensConcurrency = 1
)
@@ -107,13 +113,6 @@ const wrapUpInstruction = "⚠️ You are almost out of your investigation budge
"Do not begin any new investigation. If a finding could not be confirmed, drop it or mark it explicitly as unverified. " +
"Output the review in the required format right now."
// finalizeInstruction is the user message sent on the tool-free fallback pass
// when the agent exhausted its budget (or tripped a loop guard) without ever
// producing a final answer. It forces the model to synthesize whatever it has.
const finalizeInstruction = "You have run out of investigation steps. Do NOT call any tools. " +
"Based solely on what you have already gathered above, write your final answer now in the required format. " +
"If you could not confirm some findings, omit them or mark them as unverified, but produce the answer."
func main() {
if err := run(); err != nil {
fmt.Fprintln(os.Stderr, "gadfly:", err)
@@ -123,6 +122,14 @@ func main() {
func run() error {
start := time.Now()
// Consolidation mode: not a review at all — read the per-model findings the
// swarm wrote and print the single cross-model consensus comment. entrypoint.sh
// runs this once, after every model has finished.
if strings.TrimSpace(os.Getenv("GADFLY_CONSOLIDATE_DIR")) != "" {
return runConsolidate()
}
repoDir := os.Getenv("GADFLY_REPO_DIR")
diffFile := os.Getenv("GADFLY_DIFF_FILE")
systemFile := os.Getenv("GADFLY_SYSTEM_FILE")
@@ -149,15 +156,18 @@ func run() error {
return err
}
// Resolve the review engine. The claude-code engine shells out to the
// `claude` CLI (its own repo tools); every other spec is a majordomo model.
// auto-selection and the delegate worker are majordomo-only — with
// claude-code they're skipped (Claude Code does its own legwork).
ccSpec := isClaudeCodeSpec(os.Getenv("GADFLY_MODEL"))
// Resolve the review engine. The shell-out engines (claude-code, opencode)
// bring their OWN repo tools; every other spec is an in-process majordomo
// model. auto-selection and the delegate worker are majordomo-only — with a
// shell-out engine they're skipped (the CLI does its own legwork).
spec := os.Getenv("GADFLY_MODEL")
var eng reviewEngine
if ccSpec {
eng = newClaudeCodeEngine(os.Getenv("GADFLY_MODEL"), fsTools.root)
} else {
switch {
case isClaudeCodeSpec(spec):
eng = newClaudeCodeEngine(spec, fsTools.root)
case isOpenCodeSpec(spec):
eng = newOpenCodeEngine(spec, fsTools.root)
default:
mdl, merr := resolveModel()
if merr != nil {
return fmt.Errorf("resolve model: %w", merr)
@@ -169,7 +179,18 @@ func run() error {
} else if worker != nil {
fsTools.worker = worker
}
eng = &majordomoEngine{mdl: mdl, fsTools: fsTools}
// The context compactor needs a cheap summarizer; reuse the worker model
// when present, else the review model. A bad explicit GADFLY_COMPACT_MODEL
// just disables compaction rather than sinking the review.
summarizer, serr := resolveSummarizerModel(mdl, fsTools.worker)
if serr != nil {
fmt.Fprintln(os.Stderr, "gadfly: compaction summarizer disabled:", serr)
}
rex, rerr := newReviewExecutor(fsTools, mdl, summarizer, os.Getenv("GADFLY_MODEL"), newPRBudget())
if rerr != nil {
return fmt.Errorf("build review executor: %w", rerr)
}
eng = &majordomoEngine{rex: rex, mdl: mdl}
}
specialists, registry, auto, serrs := resolveSpecialists(repoDir)
@@ -178,13 +199,15 @@ func run() error {
}
// Dynamic selection: a (cheap) model picks the lenses this diff needs.
// Majordomo-only — the selector is an llm.Model.
// Majordomo-only — the selector is an llm.Model, so a shell-out engine
// (claude-code, opencode) can't provide one; fall back to the default suite.
if auto {
if ccSpec {
fmt.Fprintln(os.Stderr, "gadfly: auto-select is not supported with the claude-code engine; using the default suite")
md, ok := eng.(*majordomoEngine)
if !ok {
fmt.Fprintln(os.Stderr, "gadfly: auto-select requires an in-process model engine; using the default suite")
specialists = suiteFromRegistry(registry, defaultSuite)
} else {
selector, serr := resolveSelectorModel(eng.(*majordomoEngine).mdl)
selector, serr := resolveSelectorModel(md.mdl)
if serr != nil {
return fmt.Errorf("resolve selector model: %w", serr)
}
@@ -214,70 +237,94 @@ func run() error {
// Optional, best-effort telemetry. OFF unless GADFLY_FINDINGS_URL is set;
// any failure is logged to stderr and never affects stdout or the exit code.
emit(results, time.Since(start))
// Optional per-model findings artifact for the cross-model consolidation
// pass. No-op unless GADFLY_FINDINGS_OUT is set (entrypoint sets it for a
// multi-model swarm). Best-effort, never affects stdout or the exit code.
writeFindingsOut(results)
return nil
}
// runSpecialists reviews the diff through each lens and returns the results in
// the SAME order as specialists, regardless of finish order. Up to
// GADFLY_LENS_CONCURRENCY lenses run concurrently; the default of 1 keeps the
// suite sequential, exactly as before. Each lens already runs under its own
// per-lens timeout (reviewWithSpecialist), so concurrency simply overlaps those
// independent passes — and because reviewWithSpecialist builds a fresh toolbox
// per pass and the lenses only read the immutable repoFS, they share no mutable
// state. Results are stored by index so the consolidated comment keeps the
// configured lens order.
// the SAME order as specialists, regardless of finish order. It uses executus's
// fanout primitive to overlap independent lens passes (fanout.Run returns one
// result per lens in input order); each lens runs under its own per-lens timeout
// (reviewWithSpecialist) and the lenses only read the immutable repoFS.
//
// Caution: this fans out WITHIN one model. It multiplies with entrypoint.sh's
// per-provider model concurrency, so total concurrent backend requests ≈
// (models at once) × (lenses at once). To fan lenses out without oversubscribing
// the backend, run models one at a time (provider lane cap 1) and raise this.
// Throttling: when entrypoint.sh runs several of a provider's models at once it
// seeds a shared lens-permit pool (activeLensSem) that every model's lenses draw
// from, so the real cap is total lens passes in flight per PROVIDER — not
// (models at once) × (lenses at once). Absent that pool (local runs, tests) the
// in-process GADFLY_LENS_CONCURRENCY limit applies alone (default 1 = sequential).
func runSpecialists(eng reviewEngine, base string, specialists []Specialist, task, diff string) []specialistResult {
results := make([]specialistResult, len(specialists))
// Optional live status board: publishes this model's per-lens progress to a
// file the entrypoint board renders. Inert (no-op) unless GADFLY_STATUS_FILE
// is set, so plain runs are unaffected.
sw := newStatusWriter(os.Getenv("GADFLY_MODEL"), modelProvider(), specialists)
conc := min(lensConcurrency(), len(specialists))
// The cross-process pool (if any) is the real ceiling; size the in-process
// fanout to it so a lone model in its lane can use the whole provider budget,
// while extra goroutines simply block in sem.acquire until a permit frees.
// Absent a pool, fall back to the in-process lens limit.
sem := activeLensSem()
maxConcurrent := lensConcurrency()
if sem != nil {
maxConcurrent = sem.size // the shared pool is the real ceiling
}
sem := make(chan struct{}, conc)
var wg sync.WaitGroup
for i, sp := range specialists {
wg.Add(1)
sem <- struct{}{} // blocks once `conc` lenses are already in flight
go func(i int, sp Specialist) {
defer wg.Done()
defer func() { <-sem }()
// A panic in one lens must not crash the whole binary (which would
// kill every other lens's output) or leave this lens stuck at
// "running" on the status board. Recover, record it as an errored
// result, and mark the lens finished so the board can complete.
defer func() {
if r := recover(); r != nil {
results[i] = specialistResult{spec: sp, out: fmt.Sprintf("⚠️ This reviewer panicked: %v", r), verdict: verdictUnknown, errored: true}
sw.set(sp.Name, lensFinished, "", true)
}
}()
sw.set(sp.Name, lensRunning, "", false)
out, errored := reviewWithSpecialist(eng, base, sp, task, diff)
v := parseVerdict(out)
results[i] = specialistResult{spec: sp, out: out, verdict: v, errored: errored}
sw.set(sp.Name, lensFinished, v.label(), errored)
}(i, sp)
fanResults := fanout.Run(context.Background(), specialists, fanout.Options[Specialist]{
MaxConcurrent: maxConcurrent,
}, func(ctx context.Context, sp Specialist) (res specialistResult, _ error) {
// A panic in one lens must not crash the whole binary (which would kill
// every other lens's output) or leave this lens stuck at "running" on the
// status board. fanout does not recover fn panics, so we do it here:
// record the panic as an errored result and mark the lens finished.
defer func() {
if r := recover(); r != nil {
res = specialistResult{spec: sp, out: fmt.Sprintf("⚠️ This reviewer panicked: %v", r), verdict: verdictUnknown, errored: true}
sw.set(sp.Name, lensFinished, "", true)
}
}()
// Hold a provider-wide permit for this lens's whole review+recheck pass.
// While waiting the lens stays "queued" on the board; cancellation before
// a permit frees surfaces as a did-not-run lens rather than a leaked slot.
if sem != nil {
release, err := sem.acquire(ctx)
if err != nil {
sw.set(sp.Name, lensFinished, "", true)
return specialistResult{spec: sp, out: fmt.Sprintf("⚠️ This reviewer did not run: %v", err), verdict: verdictUnknown, errored: true}, nil
}
defer release()
}
sw.set(sp.Name, lensRunning, "", false)
out, errored := reviewWithSpecialist(eng, base, sp, task, diff)
v := parseVerdict(out)
sw.set(sp.Name, lensFinished, v.label(), errored)
return specialistResult{spec: sp, out: out, verdict: v, errored: errored}, nil
})
// fanout guarantees input order; its Result.Err is set only when the context
// is cancelled before a lens ran (reviewWithSpecialist embeds its own failures
// in the result), so surface that as an errored lens rather than dropping it.
results := make([]specialistResult, len(specialists))
for i, r := range fanResults {
if r.Err != nil {
results[i] = specialistResult{spec: specialists[i], out: fmt.Sprintf("⚠️ This reviewer did not run: %v", r.Err), verdict: verdictUnknown, errored: true}
sw.set(specialists[i].Name, lensFinished, "", true)
continue
}
results[i] = r.Value
}
wg.Wait()
return results
}
// lensConcurrency resolves how many specialist lenses run at once for THIS run's
// model. It mirrors entrypoint.sh's per-provider MODEL concurrency: a
// per-provider override in GADFLY_PROVIDER_LENS_CONCURRENCY ("provider=N,...")
// wins for the model's provider, otherwise the GADFLY_LENS_CONCURRENCY scalar
// (default 1). The provider is resolved by modelProvider() — the SAME lane rule
// entrypoint uses for GADFLY_PROVIDER_CONCURRENCY — so e.g.
// "ollama-cloud=3,m1=1" fans cloud lenses out while keeping a slow local box
// serial, exactly the way the model map does for whole models.
// lensConcurrency resolves the lens budget for THIS run's provider: a per-provider
// override in GADFLY_PROVIDER_LENS_CONCURRENCY ("provider=N,...") wins for the
// model's provider (resolved by modelProvider()), otherwise the
// GADFLY_LENS_CONCURRENCY scalar (default 1). Under entrypoint.sh the SAME value
// seeds the shared cross-process permit pool (activeLensSem), so it is the
// provider-wide budget rather than a per-model one; standalone it caps the single
// model's in-process fanout.
func lensConcurrency() int {
if n, ok := providerOverride("GADFLY_PROVIDER_LENS_CONCURRENCY", modelProvider()); ok {
return n
@@ -287,7 +334,7 @@ func lensConcurrency() int {
// providerOverride parses a "provider=N,provider=N" env map and returns the
// value for provider when present and valid (>0). Mirrors entrypoint.sh's
// provider_cap lookup so the two concurrency maps share one syntax.
// provider_lens_cap lookup so the two share one syntax.
func providerOverride(envName, provider string) (int, bool) {
for _, item := range strings.Split(os.Getenv(envName), ",") {
k, v, ok := strings.Cut(item, "=")
@@ -307,8 +354,7 @@ func providerOverride(envName, provider string) (int, bool) {
// returned bool is true when the review pass failed (rendered as an inline
// notice — advisory; one lens failing never sinks the others or the job).
func reviewWithSpecialist(eng reviewEngine, base string, sp Specialist, task, diff string) (string, bool) {
timeout := time.Duration(envInt("GADFLY_TIMEOUT_SECS", defaultTimeoutSecs)) * time.Second
ctx, cancel := context.WithTimeout(context.Background(), timeout)
ctx, cancel := context.WithTimeout(context.Background(), reviewTimeout())
defer cancel()
draft, err := eng.runPass(ctx, composeSpecialistPrompt(base, sp), task,
@@ -331,90 +377,6 @@ func reviewWithSpecialist(eng reviewEngine, base string, sp Specialist, task, di
return final, false
}
// runAgent runs one agent pass (its own fresh toolbox over the sandbox) and
// returns the final answer. An empty answer is an error — the caller decides
// whether that is fatal (review pass) or recoverable (recheck pass). A
// non-empty answer that ended on a budget/guard error is still returned: the
// model wrote its output, then ran out of steps.
//
// Two mechanisms keep a step-hungry model from hard-failing with no output:
// 1. A wrap-up steer: once the run comes within wrapUpReserve steps of the
// cap, a forceful "stop calling tools, write your final answer" message is
// injected so the model spends its remaining steps finalizing.
// 2. A finalization fallback: if the loop still ends empty (the model ignored
// the nudge, or a loop guard tripped), one tool-free model call forces a
// final answer out of the transcript already gathered.
func runAgent(ctx context.Context, mdl llm.Model, fsTools *repoFS, system, task string, maxSteps int) (string, error) {
box, err := fsTools.toolbox()
if err != nil {
return "", err
}
loop := agent.New(mdl, system,
agent.WithToolbox(box),
agent.WithMaxSteps(maxSteps),
// Guard rails: stop the model from spinning on failing or identical
// tool calls instead of writing its answer.
agent.WithToolErrorLimits(4, 4),
)
wrapUpAt := maxSteps - wrapUpReserve()
if wrapUpAt < 1 {
wrapUpAt = 1
}
var completed int // steps finished so far (updated after each step)
nudged := false
res, runErr := loop.Run(ctx, task,
agent.OnStep(func(s agent.Step) { completed = s.Index + 1 }),
agent.WithSteer(func() []llm.Message {
if !nudged && completed >= wrapUpAt {
nudged = true
return []llm.Message{llm.UserText(wrapUpInstruction)}
}
return nil
}),
)
out := ""
if res != nil {
out = strings.TrimSpace(res.Output)
}
if out != "" {
return out, nil
}
// No final answer. If we still have budget on the clock and a transcript to
// work from, force a tool-free finalization rather than losing the pass.
if res != nil && len(res.Messages) > 0 && ctx.Err() == nil {
if forced := forceFinalAnswer(ctx, mdl, system, res.Messages); forced != "" {
return forced, nil
}
}
if runErr != nil {
return "", runErr
}
return "", errors.New("agent produced no output")
}
// forceFinalAnswer makes one tool-free model call to squeeze a final answer out
// of an agent that exhausted its step budget without producing one. Tools are
// forbidden (ToolChoice "none") so the model must synthesize from the transcript
// instead of investigating further. Best-effort: any error or empty reply
// returns "" and the caller falls back to its normal empty-output handling.
func forceFinalAnswer(ctx context.Context, mdl llm.Model, system string, transcript []llm.Message) string {
msgs := append(append([]llm.Message(nil), transcript...), llm.UserText(finalizeInstruction))
resp, err := mdl.Generate(ctx, llm.Request{
System: system,
Messages: msgs,
ToolChoice: "none",
})
if err != nil || resp == nil {
return ""
}
return strings.TrimSpace(resp.Text())
}
// wrapUpReserve is how many steps before the cap the wrap-up nudge fires,
// overridable via GADFLY_WRAPUP_RESERVE.
func wrapUpReserve() int {
@@ -431,7 +393,7 @@ func buildTask(diff string) string {
truncNote := ""
if maxDiff > 0 && len(diff) > maxDiff {
diff = diff[:maxDiff]
truncNote = fmt.Sprintf("\n\n[NOTE: diff truncated to %d chars in this message; read the changed files (or call get_diff, if available) for the full text.]", maxDiff)
truncNote = fmt.Sprintf("\n\n[NOTE: diff truncated to %d chars in this message; page the full diff with get_diff (paginated; pass a `path` to scope it to one file) or read the changed files.]", maxDiff)
}
var b strings.Builder
@@ -458,3 +420,23 @@ func envInt(name string, def int) int {
}
return n
}
// envBool reads a boolean-ish env var: def when unset, false for an explicit
// falsey value (0/false/no/off), true otherwise. The shared spelling for
// gadfly's "on unless disabled" opt-out flags (GADFLY_RECHECK, GADFLY_COMPACT).
func envBool(name string, def bool) bool {
switch strings.ToLower(strings.TrimSpace(os.Getenv(name))) {
case "":
return def
case "0", "false", "no", "off":
return false
default:
return true
}
}
// reviewTimeout is the per-specialist-lens deadline (GADFLY_TIMEOUT_SECS), shared
// across a lens's review+recheck passes and applied as each pass's run cap.
func reviewTimeout() time.Duration {
return time.Duration(envInt("GADFLY_TIMEOUT_SECS", defaultTimeoutSecs)) * time.Second
}
+85 -18
View File
@@ -3,6 +3,7 @@ package main
import (
"fmt"
"os"
"slices"
"strings"
"gitea.stevedudenhoeffer.com/steve/majordomo"
@@ -19,6 +20,35 @@ import (
// model list is just ids like "qwen3-coder:480b-cloud" — working unchanged.
const defaultProvider = "ollama-cloud"
// openAICompatProviders are the provider names that resolve to the plain
// openai client at an explicit base URL. openai-compatible is the generic
// spelling; kimi (Moonshot) and qwen (Alibaba Model Studio) are majordomo
// built-ins that ARE that client pointed elsewhere, so an explicit endpoint for
// either belongs on the same branch.
//
// One slice, because three places must agree: resolveModel's endpoint
// override, endpointProvider's GADFLY_ENDPOINT_* parser, and the test that
// pins them. A name accepted by one and rejected by another is a config that
// works when written one way and errors the other, for no reason a user could
// guess.
var openAICompatProviders = []string{"openai", "openai-compatible", "kimi", "qwen"}
func isOpenAICompatProvider(name string) bool {
return slices.Contains(openAICompatProviders, name)
}
// endpointProviderNames is the operator-facing list of providers that accept an
// explicit endpoint. resolveModel and endpointProvider accept the SAME set, so
// one message serves both rather than each carrying a copy that drifts in
// order and spelling.
//
// Every accepted spelling belongs here, aliases included —
// TestEndpointProviderNamesAreAllAccepted asserts that each name listed
// actually resolves, so an omission fails the build rather than misleading an
// operator who is already debugging.
const endpointProviderNames = "openai/openai-compatible/kimi/qwen/ollama/ollama-cloud/" +
"llama-swap/llama-swaps/llamaswap/llamaswaps/foreman/anthropic/google/gemini"
// resolveModel builds the review model from the environment. Gadfly is powered
// by majordomo, so it can target any provider majordomo supports — Ollama
// (local or cloud), OpenAI, Anthropic, Google, or any OpenAI/Ollama-compatible
@@ -33,10 +63,12 @@ const defaultProvider = "ollama-cloud"
// GADFLY_BASE_URL override the backend endpoint (OpenAI/Ollama-compatible
// servers, a remote Ollama, an OpenRouter-style gateway…).
// When set, the provider is constructed directly at that URL.
// GADFLY_API_KEY bearer/API key for the chosen provider. Optional; when
// unset the provider falls back to its standard env var
// (OLLAMA_API_KEY / OPENAI_API_KEY / ANTHROPIC_API_KEY /
// GOOGLE_API_KEY|GEMINI_API_KEY). Local Ollama needs none.
// GADFLY_API_KEY bearer/API key for the chosen provider, used ONLY on the
// GADFLY_BASE_URL override path. With no base URL the
// provider reads its own standard variable and this is never
// consulted: OLLAMA_API_KEY / OPENAI_API_KEY /
// QWEN_API_KEY / KIMI_API_KEY / ANTHROPIC_API_KEY /
// GOOGLE_API_KEY|GEMINI_API_KEY. Local Ollama needs none.
//
// With GADFLY_BASE_URL unset, resolution goes through majordomo's registry, so
// LLM_* env DSNs and registered aliases/tiers work too.
@@ -67,13 +99,21 @@ func resolveModel() (llm.Model, error) {
}
// Endpoint override: construct the provider directly at the given URL.
switch provider {
case "openai", "openai-compatible":
// The openai-compat family is matched by the shared predicate, not a
// repeated case list. The credential on THIS path is GADFLY_API_KEY; the
// built-ins' own KIMI_API_KEY / QWEN_API_KEY are read only on the registry
// path above, where GADFLY_BASE_URL is unset. The two paths never share a
// credential rule — assuming they do produces a config that passes every
// check and then 401s.
if isOpenAICompatProvider(provider) {
opts := []openai.Option{openai.WithBaseURL(baseURL)}
if apiKey != "" {
opts = append(opts, openai.WithAPIKey(apiKey))
}
return openai.New(opts...).Model(model)
}
switch provider {
case "ollama", "ollama-cloud":
opts := []ollama.Option{ollama.WithBaseURL(baseURL)}
if apiKey != "" {
@@ -108,7 +148,7 @@ func resolveModel() (llm.Model, error) {
}
return google.New(opts...).Model(model)
default:
return nil, fmt.Errorf("GADFLY_BASE_URL is set but GADFLY_PROVIDER %q has no endpoint-override support (use openai/openai-compatible/ollama/llama-swap/foreman/anthropic/google, or unset GADFLY_BASE_URL to resolve via majordomo)", provider)
return nil, fmt.Errorf("GADFLY_BASE_URL is set but GADFLY_PROVIDER %q has no endpoint-override support (use %s, or unset GADFLY_BASE_URL to resolve via majordomo)", provider, endpointProviderNames)
}
}
@@ -127,6 +167,27 @@ func resolveWorkerModel() (llm.Model, error) {
return majordomo.Parse(buildSpec(provider, spec))
}
// resolveSummarizerModel picks the model the context compactor uses to compress
// the runaway middle of a transcript. It should be CHEAP, since it fires once per
// compaction: GADFLY_COMPACT_MODEL if set (honoring GADFLY_PROVIDER for a bare
// id), else the delegate worker model when one is configured (already cheap by
// design), else the review model itself. Returns (nil, err) only on an explicit
// bad GADFLY_COMPACT_MODEL spec — the caller logs it and simply runs without
// compaction rather than failing the review.
func resolveSummarizerModel(review, worker llm.Model) (llm.Model, error) {
if spec := strings.TrimSpace(os.Getenv("GADFLY_COMPACT_MODEL")); spec != "" {
provider := strings.TrimSpace(os.Getenv("GADFLY_PROVIDER"))
if provider == "" {
provider = defaultProvider
}
return majordomo.Parse(buildSpec(provider, spec))
}
if worker != nil {
return worker, nil
}
return review, nil
}
// buildSpec turns (provider, model) into a majordomo spec. A model id that
// already carries a "provider/" prefix (or is a multi-element failover chain)
// is passed through verbatim; a bare id is prefixed with the provider.
@@ -141,8 +202,8 @@ func buildSpec(provider, model string) string {
// entrypoint.sh's provider_of: the segment before the first "/" in GADFLY_MODEL,
// else GADFLY_PROVIDER, else the default (ollama-cloud). The binary reviews one
// model per invocation, so this is that model's provider — used to resolve
// per-provider policy (e.g. lens concurrency) against the SAME provider keys
// entrypoint uses for GADFLY_PROVIDER_CONCURRENCY.
// per-provider policy (e.g. the lens budget) against the SAME provider keys
// entrypoint uses for GADFLY_PROVIDER_LENS_CONCURRENCY.
func modelProvider() string {
model := strings.TrimSpace(os.Getenv("GADFLY_MODEL"))
if pfx, _, ok := strings.Cut(model, "/"); ok {
@@ -167,8 +228,10 @@ func modelProvider() string {
// plaintext local Ollama (or foreman queue) works:
// GADFLY_ENDPOINT_BIGBOX="ollama|http://192.168.1.50:11434"
// GADFLY_MODEL=bigbox/qwen2.5-coder:7b
// provider is one of ollama/llama-swap(s)/foreman/openai/anthropic/google; "foreman"
// targets a foreman daemon (native Ollama on the wire):
// provider is ollama/openai/anthropic/google/foreman/llama-swap(s) or an
// openai-compat built-in (kimi, qwen) — endpointProviderNames is the
// authoritative list. "foreman" targets a foreman daemon (native Ollama
// on the wire):
// GADFLY_ENDPOINT_M1="foreman|http://foreman-m1:8080|tok"
//
// GADFLY_ALIAS_<NAME> = "<majordomo spec>"
@@ -219,6 +282,16 @@ func endpointProvider(name, raw string) (llm.Provider, error) {
return nil, fmt.Errorf("missing base URL in %q", raw)
}
// Same shared predicate as resolveModel: the two must accept an identical
// set, and a hand-copied case list cannot guarantee that.
if isOpenAICompatProvider(provider) {
opts := []openai.Option{openai.WithName(name), openai.WithBaseURL(baseURL)}
if key != "" {
opts = append(opts, openai.WithAPIKey(key))
}
return openai.New(opts...), nil
}
switch provider {
case "ollama", "ollama-cloud":
opts := []ollama.Option{ollama.WithName(name), ollama.WithBaseURL(baseURL)}
@@ -237,12 +310,6 @@ func endpointProvider(name, raw string) (llm.Provider, error) {
// its non-streaming degradation. Unlike the HTTPS-only LLM_* foreman://
// DSN, the base URL here is verbatim, so a plaintext http:// foreman works.
return ollama.Foreman(baseURL, key, ollama.WithName(name)), nil
case "openai", "openai-compatible":
opts := []openai.Option{openai.WithName(name), openai.WithBaseURL(baseURL)}
if key != "" {
opts = append(opts, openai.WithAPIKey(key))
}
return openai.New(opts...), nil
case "anthropic":
opts := []anthropic.Option{anthropic.WithName(name), anthropic.WithBaseURL(baseURL)}
if key != "" {
@@ -256,6 +323,6 @@ func endpointProvider(name, raw string) (llm.Provider, error) {
}
return google.New(opts...), nil
default:
return nil, fmt.Errorf("unknown provider %q (use ollama/llama-swap(s)/foreman/openai/openai-compatible/anthropic/google)", provider)
return nil, fmt.Errorf("unknown provider %q (use %s)", provider, endpointProviderNames)
}
}
+68 -1
View File
@@ -1,6 +1,9 @@
package main
import "testing"
import (
"strings"
"testing"
)
func TestEndpointProvider(t *testing.T) {
t.Run("ollama http endpoint registers under its name", func(t *testing.T) {
@@ -68,6 +71,70 @@ func TestEndpointProvider(t *testing.T) {
}
}
// TestOpenAICompatProvidersResolveOnBothPaths pins the two provider switches
// together. kimi and qwen are majordomo built-ins that ARE the openai client at
// a different base URL, and two independent places have to know it:
// resolveModel's GADFLY_BASE_URL override, and endpointProvider's
// GADFLY_ENDPOINT_* parser. A name accepted by one and rejected by the other is
// a provider that works when configured one way and errors the other, for no
// reason a user could guess. Asserting both from one table makes the pair fail
// together.
func TestOpenAICompatProvidersResolveOnBothPaths(t *testing.T) {
// Ranges the SHARED slice: a test that pins a list against drift must not
// be able to drift from it.
for _, provider := range openAICompatProviders {
t.Run(provider+" via GADFLY_ENDPOINT_*", func(t *testing.T) {
p, err := endpointProvider("ep", provider+"|https://host.example/v1|sk-x")
if err != nil {
t.Fatalf("endpointProvider(%q): %v", provider, err)
}
if p.Name() != "ep" {
t.Errorf("Name() = %q, want %q", p.Name(), "ep")
}
})
t.Run(provider+" via GADFLY_BASE_URL", func(t *testing.T) {
t.Setenv("GADFLY_PROVIDER", provider)
t.Setenv("GADFLY_BASE_URL", "https://host.example/v1")
t.Setenv("GADFLY_API_KEY", "sk-x")
t.Setenv("GADFLY_MODEL", "some-model")
if _, err := resolveModel(); err != nil {
t.Fatalf("resolveModel with GADFLY_PROVIDER=%q: %v", provider, err)
}
})
}
}
// TestEndpointProviderNamesAreAllAccepted keeps the operator-facing list
// honest: every name endpointProviderNames advertises must actually resolve.
// The constant is read by somebody whose config just failed, so a name listed
// there and rejected by the code sends them to debug a spelling that was never
// going to work.
func TestEndpointProviderNamesAreAllAccepted(t *testing.T) {
for _, name := range strings.Split(endpointProviderNames, "/") {
name = strings.TrimSpace(name)
if name == "" {
continue
}
// Both switches, not one: this constant is the error text for BOTH
// GADFLY_ENDPOINT_* and GADFLY_BASE_URL, so a name accepted by only
// half of them still misleads whichever operator hits the other path.
t.Run(name+" via GADFLY_ENDPOINT_*", func(t *testing.T) {
if _, err := endpointProvider("ep", name+"|https://host.example/v1|sk-x"); err != nil {
t.Errorf("endpointProviderNames advertises %q but endpointProvider rejects it: %v", name, err)
}
})
t.Run(name+" via GADFLY_BASE_URL", func(t *testing.T) {
t.Setenv("GADFLY_PROVIDER", name)
t.Setenv("GADFLY_BASE_URL", "https://host.example/v1")
t.Setenv("GADFLY_API_KEY", "sk-x")
t.Setenv("GADFLY_MODEL", "some-model")
if _, err := resolveModel(); err != nil {
t.Errorf("endpointProviderNames advertises %q but resolveModel rejects it: %v", name, err)
}
})
}
}
func TestBuildSpec(t *testing.T) {
tests := []struct {
name string
+273
View File
@@ -0,0 +1,273 @@
package main
import (
"bytes"
"context"
"encoding/json"
"fmt"
"os"
"os/exec"
"strings"
)
// openCodeEngine reviews by shelling out to the `opencode` CLI (opencode.ai) in
// non-interactive `run` mode. Like the claude-code engine it is a pure shell-out
// — OpenCode brings its OWN read tools (read/grep/glob/list) and reads the
// checked-out tree, so findings are verified against real code — but it drives an
// ollama-cloud model instead of a Claude subscription. The point is to benchmark
// gadfly's boutique executus harness against a freely-available agentic harness
// on the SAME models (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 the
// OPENCODE_CONFIG_CONTENT env var (see config): a custom "gadfly" agent whose
// prompt is the system prompt with edit/bash denied, plus a "gadfly" provider
// pointing at ollama-cloud. OPENCODE_CONFIG_CONTENT is the highest-precedence
// config source that matters in the container — it outranks any opencode.json a
// reviewed repo might ship — so a repo can't re-enable edits on us.
type openCodeEngine struct {
bin string // CLI binary (GADFLY_OPENCODE_BIN, default "opencode")
providerModel string // ollama model id for the generated "gadfly" provider ("" = none)
modelRef string // --model value ("gadfly/<id>", a pass-through "<prov>/<id>", or "" = CLI default)
baseURL string // ollama-cloud base URL (GADFLY_OPENCODE_BASE_URL)
repoDir string // cwd for the CLI, so its tools read the checked-out tree
extraArgs []string // appended verbatim (GADFLY_OPENCODE_EXTRA_ARGS)
}
// openCodeProviderName / openCodeAgentName are the internal names of the provider
// and agent gadfly generates in the injected config. "gadfly" won't collide with
// OpenCode's models.dev provider registry.
const (
openCodeProviderName = "gadfly"
openCodeAgentName = "gadfly"
)
// defaultOpenCodeBaseURL is ollama-cloud's OpenAI-compatible endpoint.
const defaultOpenCodeBaseURL = "https://ollama.com/v1"
// isOpenCodeSpec reports whether a GADFLY_MODEL spec selects the opencode engine:
// the bare id "opencode"/"open-code" or an "opencode/<model>" form (both
// spellings accepted; "opencode" is canonical).
func isOpenCodeSpec(model string) bool {
m := strings.TrimSpace(model)
for _, p := range []string{"opencode", "open-code"} {
if m == p || strings.HasPrefix(m, p+"/") {
return true
}
}
return false
}
// newOpenCodeEngine builds the engine from the GADFLY_MODEL spec and the optional
// GADFLY_OPENCODE_* overrides. The part after the FIRST slash is the model, taken
// verbatim — no ":"-suffix parsing, because ollama model ids legitimately contain
// colons (e.g. "qwen3-coder:480b-cloud"). Three spec forms:
//
// opencode → bare: no --model, no generated provider (CLI default model)
// opencode/<model> → wrap <model> in the generated ollama-cloud "gadfly" provider
// opencode/<provider>/<model> → pass-through: --model <provider>/<model>, using OpenCode's
// own provider registry/auth (escape hatch, no generated provider)
//
// GADFLY_OPENCODE_MODEL overrides the model taken from the spec (and is itself run
// through the same slash logic). It does not verify the CLI is installed — a
// missing binary surfaces as a normal pass error (advisory, never fatal).
func newOpenCodeEngine(spec, repoDir string) *openCodeEngine {
e := &openCodeEngine{
bin: envOr("GADFLY_OPENCODE_BIN", "opencode"),
baseURL: envOr("GADFLY_OPENCODE_BASE_URL", defaultOpenCodeBaseURL),
repoDir: repoDir,
extraArgs: strings.Fields(os.Getenv("GADFLY_OPENCODE_EXTRA_ARGS")),
}
var after string
if _, a, ok := strings.Cut(strings.TrimSpace(spec), "/"); ok {
after = strings.TrimSpace(a)
}
if env := strings.TrimSpace(os.Getenv("GADFLY_OPENCODE_MODEL")); env != "" {
after = env
}
switch {
case after == "":
// bare spec: let OpenCode's configured default model apply.
case strings.Contains(after, "/"):
// "<provider>/<model>" pass-through to an OpenCode built-in provider.
e.modelRef = after
default:
// A bare model id → serve it via the generated ollama-cloud provider.
e.providerModel = after
e.modelRef = openCodeProviderName + "/" + after
}
return e
}
// args assembles the `opencode` argv for one pass. Factored out (and pure) so it
// can be unit-tested without invoking the CLI. The task is the positional message
// and MUST come last; it never begins with '-' (buildTask output starts with "PR
// title:"/"Review …"), so no "--" terminator is needed. Note: in `opencode run`,
// -p is basic-auth password, NOT the prompt — the message is positional.
func (e *openCodeEngine) args(task string) []string {
a := []string{"run", "--agent", openCodeAgentName}
if e.modelRef != "" {
a = append(a, "--model", e.modelRef)
}
a = append(a, e.extraArgs...)
return append(a, task)
}
// openCode config structs — the minimal shape gadfly generates. Marshaled to JSON
// and handed to the CLI via OPENCODE_CONFIG_CONTENT.
type openCodeConfig struct {
Schema string `json:"$schema"`
Permission openCodePermission `json:"permission"`
Provider map[string]openCodeProvider `json:"provider,omitempty"`
Agent map[string]openCodeAgent `json:"agent"`
}
// openCodePermission is OpenCode's permission map: tool key → "allow"|"ask"|"deny".
type openCodePermission map[string]string
// denyMutations denies every OpenCode permission that could change the repo, run
// commands, or reach the network / the filesystem outside the checked-out tree —
// keeping the reviewer strictly read-only. The read/search tools OpenCode gates
// separately (read/glob/grep/list/lsp) stay at their default so the agent can
// still verify findings against the code. OpenCode's permission keys are
// enumerated at https://opencode.ai/docs/agents; a key it doesn't recognize is
// simply ignored, so listing extras is safe.
func denyMutations() openCodePermission {
return openCodePermission{
"edit": "deny",
"bash": "deny",
"webfetch": "deny",
"websearch": "deny",
"external_directory": "deny",
}
}
type openCodeProvider struct {
NPM string `json:"npm"`
Name string `json:"name"`
Options openCodeProviderOptions `json:"options"`
Models map[string]struct{} `json:"models"`
}
type openCodeProviderOptions struct {
BaseURL string `json:"baseURL"`
APIKey string `json:"apiKey"`
}
type openCodeAgent struct {
Description string `json:"description"`
Mode string `json:"mode"`
Prompt string `json:"prompt"`
Permission openCodePermission `json:"permission"`
}
// config builds the OpenCode config JSON for one pass. The system prompt becomes
// the "gadfly" agent's prompt; the mutating/network tools are denied at BOTH the
// global and agent level (defense in depth — OpenCode's read tools stay
// available). For a 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) {
deny := denyMutations()
cfg := openCodeConfig{
Schema: "https://opencode.ai/config.json",
Permission: deny,
Agent: map[string]openCodeAgent{
openCodeAgentName: {
Description: "Gadfly adversarial code-review lens (read-only).",
Mode: "primary",
Prompt: system,
Permission: deny,
},
},
}
if e.providerModel != "" {
cfg.Provider = map[string]openCodeProvider{
openCodeProviderName: {
NPM: "@ai-sdk/openai-compatible",
Name: openCodeProviderName,
Options: openCodeProviderOptions{
BaseURL: e.baseURL,
APIKey: "{env:OLLAMA_API_KEY}",
},
Models: map[string]struct{}{e.providerModel: {}},
},
}
}
return json.Marshal(cfg)
}
func (e *openCodeEngine) runPass(ctx context.Context, system, task string, _ int) (string, error) {
cfg, err := e.config(system)
if err != nil {
return "", fmt.Errorf("opencode config: %w", err)
}
cmd := exec.CommandContext(ctx, e.bin, e.args(task)...)
cmd.Dir = e.repoDir
// Inject the review config (system prompt as the agent prompt + read-only
// 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")
killGroupOnCancel(cmd) // don't orphan the CLI's Node children on a timed-out lens
var stdout, stderr bytes.Buffer
cmd.Stdout = &stdout
cmd.Stderr = &stderr
runErr := cmd.Run()
// A cancelled/timed-out run must surface as an error, never as whatever partial
// bytes the CLI flushed before it was killed.
if ctx.Err() != nil {
return "", fmt.Errorf("opencode run %v", ctx.Err())
}
if runErr != nil {
detail := truncateForErr(stderr.String())
if detail == "" {
detail = truncateForErr(stdout.String())
}
if detail != "" {
return "", fmt.Errorf("opencode run failed: %v: %s", runErr, detail)
}
return "", fmt.Errorf("opencode run failed: %v", runErr)
}
// OpenCode's default (non-JSON) format prints the assistant's final text; trust
// it as the review. An empty result on a clean exit is an error, never "".
if out := strings.TrimSpace(stdout.String()); out != "" {
return out, nil
}
return "", fmt.Errorf("opencode run returned no output")
}
// openCodeEnv builds a minimal environment for the `opencode` subprocess. It
// forwards what the CLI needs to reach a model provider: OLLAMA_API_KEY for the
// generated ollama-cloud provider (the primary opencode/<model> path), PLUS the
// standard provider keys — ANTHROPIC_*, OPENAI_*, GOOGLE_*, GEMINI_* — so the
// opencode/<provider>/<model> pass-through form can authenticate against
// OpenCode's own built-in providers (those keys are otherwise stripped, which
// broke the documented escape hatch). It still withholds gadfly's OWN secrets —
// GITEA_TOKEN, GADFLY_API_KEY, GADFLY_FINDINGS_TOKEN, and the claude-code
// subscription token (CLAUDE_CODE_OAUTH_TOKEN, which OpenCode can't use anyway) —
// so they never reach the third-party CLI. OPENCODE_CONFIG_CONTENT is never
// inherited: runPass sets it, and a duplicate key would be ambiguous (getenv
// returns the first occurrence).
func openCodeEnv() []string {
return filterEnv(func(k string) bool {
if k == "OPENCODE_CONFIG_CONTENT" {
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":
return true
}
return strings.HasPrefix(k, "LC_") ||
strings.HasPrefix(k, "OPENCODE_") ||
strings.HasPrefix(k, "GADFLY_OPENCODE_") ||
strings.HasPrefix(k, "NODE_") ||
strings.HasPrefix(k, "XDG_") ||
strings.HasPrefix(k, "ANTHROPIC_") ||
strings.HasPrefix(k, "OPENAI_") ||
strings.HasPrefix(k, "GOOGLE_") ||
strings.HasPrefix(k, "GEMINI_")
})
}
+303
View File
@@ -0,0 +1,303 @@
package main
import (
"context"
"encoding/json"
"os"
"slices"
"strings"
"testing"
)
func TestIsOpenCodeSpec(t *testing.T) {
cases := map[string]bool{
"opencode": true,
"opencode/glm-5.2": true,
"open-code/glm-5.2": true, // accepted alias spelling
"opencode/qwen3-coder:480b-cloud": true, // colon-bearing model id
"opencode/anthropic/claude": true, // pass-through form
" opencode ": true, // trimmed
"opencode-extra": false, // not the bare id, not a "/" form
"qwen3-coder:480b-cloud": false,
"claude-code/opus": false,
"": false,
}
for spec, want := range cases {
if got := isOpenCodeSpec(spec); got != want {
t.Errorf("isOpenCodeSpec(%q) = %v, want %v", spec, got, want)
}
}
}
func TestNewOpenCodeEngineModel(t *testing.T) {
t.Setenv("GADFLY_OPENCODE_MODEL", "")
// "opencode/<model>" → wrapped in the generated "gadfly" provider.
if e := newOpenCodeEngine("opencode/glm-5.2", "/repo"); e.providerModel != "glm-5.2" || e.modelRef != "gadfly/glm-5.2" {
t.Errorf("glm-5.2: providerModel=%q modelRef=%q, want glm-5.2 / gadfly/glm-5.2", e.providerModel, e.modelRef)
}
// Colon-bearing ollama id is preserved verbatim — NOT split on ":".
if e := newOpenCodeEngine("opencode/qwen3-coder:480b-cloud", "/repo"); e.providerModel != "qwen3-coder:480b-cloud" {
t.Errorf("colon id: providerModel=%q, want qwen3-coder:480b-cloud (no split)", e.providerModel)
}
// "open-code/" spelling behaves identically.
if e := newOpenCodeEngine("open-code/glm-5.2", "/repo"); e.modelRef != "gadfly/glm-5.2" {
t.Errorf("open-code alias: modelRef=%q, want gadfly/glm-5.2", e.modelRef)
}
// Pass-through "opencode/<provider>/<model>" → no generated provider.
if e := newOpenCodeEngine("opencode/anthropic/claude-sonnet-4-6", "/repo"); e.providerModel != "" || e.modelRef != "anthropic/claude-sonnet-4-6" {
t.Errorf("pass-through: providerModel=%q modelRef=%q, want '' / anthropic/claude-sonnet-4-6", e.providerModel, e.modelRef)
}
// Bare spec → no model, no provider (CLI default applies).
if e := newOpenCodeEngine("opencode", "/repo"); e.providerModel != "" || e.modelRef != "" {
t.Errorf("bare: providerModel=%q modelRef=%q, want both empty", e.providerModel, e.modelRef)
}
// GADFLY_OPENCODE_MODEL overrides the spec suffix.
t.Setenv("GADFLY_OPENCODE_MODEL", "deepseek-v3")
if e := newOpenCodeEngine("opencode/glm-5.2", "/repo"); e.providerModel != "deepseek-v3" || e.modelRef != "gadfly/deepseek-v3" {
t.Errorf("env override: providerModel=%q modelRef=%q, want deepseek-v3 / gadfly/deepseek-v3", e.providerModel, e.modelRef)
}
}
func TestOpenCodeEngineDefaults(t *testing.T) {
t.Setenv("GADFLY_OPENCODE_BIN", "")
t.Setenv("GADFLY_OPENCODE_BASE_URL", "")
t.Setenv("GADFLY_OPENCODE_EXTRA_ARGS", "")
e := newOpenCodeEngine("opencode/glm-5.2", "/repo")
if e.bin != "opencode" {
t.Errorf("bin = %q, want opencode", e.bin)
}
if e.baseURL != defaultOpenCodeBaseURL {
t.Errorf("baseURL = %q, want %q", e.baseURL, defaultOpenCodeBaseURL)
}
if e.repoDir != "/repo" {
t.Errorf("repoDir = %q, want /repo", e.repoDir)
}
}
func TestOpenCodeArgs(t *testing.T) {
t.Setenv("GADFLY_OPENCODE_MODEL", "")
t.Setenv("GADFLY_OPENCODE_EXTRA_ARGS", "--variant reasoning")
e := newOpenCodeEngine("opencode/glm-5.2", "/repo")
args := e.args("TASK-PROMPT")
// "run" is the subcommand and must be first.
if len(args) == 0 || args[0] != "run" {
t.Fatalf("args[0] = %q, want run (args=%v)", args, args)
}
if argAfter(args, "--agent") != openCodeAgentName {
t.Errorf("--agent = %q, want %q", argAfter(args, "--agent"), openCodeAgentName)
}
if argAfter(args, "--model") != "gadfly/glm-5.2" {
t.Errorf("--model = %q, want gadfly/glm-5.2", argAfter(args, "--model"))
}
// extra args appended verbatim (split on whitespace).
if !strings.Contains(strings.Join(args, " "), "--variant reasoning") {
t.Errorf("extra args not appended: %v", args)
}
// task is the positional message and must be LAST.
if args[len(args)-1] != "TASK-PROMPT" {
t.Errorf("last arg = %q, want TASK-PROMPT (args=%v)", args[len(args)-1], args)
}
}
func TestOpenCodeArgsBareModelOmitsFlag(t *testing.T) {
t.Setenv("GADFLY_OPENCODE_MODEL", "")
t.Setenv("GADFLY_OPENCODE_EXTRA_ARGS", "")
e := newOpenCodeEngine("opencode", "/repo")
args := e.args("t")
if slices.Contains(args, "--model") {
t.Errorf("--model should be omitted for a bare opencode spec: %v", args)
}
if args[len(args)-1] != "t" {
t.Errorf("last arg = %q, want t", args[len(args)-1])
}
}
func TestOpenCodeConfig(t *testing.T) {
t.Setenv("GADFLY_OPENCODE_MODEL", "")
t.Setenv("GADFLY_OPENCODE_BASE_URL", "")
// Round-trip a system prompt containing quotes and newlines.
sys := "Line one with \"quotes\".\nLine two."
e := newOpenCodeEngine("opencode/glm-5.2", "/repo")
raw, err := e.config(sys)
if err != nil {
t.Fatalf("config: %v", err)
}
var cfg openCodeConfig
if err := json.Unmarshal(raw, &cfg); err != nil {
t.Fatalf("generated config is not valid JSON: %v\n%s", err, raw)
}
// Agent carries the system prompt verbatim and denies edit+bash.
ag, ok := cfg.Agent[openCodeAgentName]
if !ok {
t.Fatalf("agent %q missing from config", openCodeAgentName)
}
if ag.Prompt != sys {
t.Errorf("agent prompt = %q, want it to round-trip the system prompt", ag.Prompt)
}
// Mutating/network tools are denied at BOTH the agent and global level (defense
// in depth); the read/search tools stay at OpenCode's default.
for _, k := range []string{"edit", "bash", "webfetch", "websearch", "external_directory"} {
if ag.Permission[k] != "deny" {
t.Errorf("agent permission[%q] = %q, want deny", k, ag.Permission[k])
}
if cfg.Permission[k] != "deny" {
t.Errorf("global permission[%q] = %q, want deny", k, cfg.Permission[k])
}
}
// Provider block: correct npm, default baseURL, env-ref apiKey, model in map.
prov, ok := cfg.Provider[openCodeProviderName]
if !ok {
t.Fatalf("provider %q missing from config", openCodeProviderName)
}
if prov.NPM != "@ai-sdk/openai-compatible" {
t.Errorf("provider npm = %q, want @ai-sdk/openai-compatible", prov.NPM)
}
if prov.Options.BaseURL != defaultOpenCodeBaseURL {
t.Errorf("provider baseURL = %q, want %q", prov.Options.BaseURL, defaultOpenCodeBaseURL)
}
if prov.Options.APIKey != "{env:OLLAMA_API_KEY}" {
t.Errorf("provider apiKey = %q, want {env:OLLAMA_API_KEY} (never a literal secret)", prov.Options.APIKey)
}
if _, ok := prov.Models["glm-5.2"]; !ok {
t.Errorf("provider models = %v, want it to contain glm-5.2", prov.Models)
}
// GADFLY_OPENCODE_BASE_URL override reaches the provider.
t.Setenv("GADFLY_OPENCODE_BASE_URL", "http://localhost:11434/v1")
e2 := newOpenCodeEngine("opencode/glm-5.2", "/repo")
raw2, err := e2.config(sys)
if err != nil {
t.Fatalf("config override: %v", err)
}
var cfg2 openCodeConfig
if err := json.Unmarshal(raw2, &cfg2); err != nil {
t.Fatalf("override config invalid JSON: %v", err)
}
if got := cfg2.Provider[openCodeProviderName].Options.BaseURL; got != "http://localhost:11434/v1" {
t.Errorf("override baseURL = %q, want http://localhost:11434/v1", got)
}
}
func TestOpenCodeConfigNoProviderForPassThroughAndBare(t *testing.T) {
t.Setenv("GADFLY_OPENCODE_MODEL", "")
for _, spec := range []string{"opencode", "opencode/anthropic/claude-sonnet-4-6"} {
e := newOpenCodeEngine(spec, "/repo")
raw, err := e.config("sys")
if err != nil {
t.Fatalf("config(%q): %v", spec, err)
}
if strings.Contains(string(raw), "\"provider\"") {
t.Errorf("spec %q: config should omit the provider block, got %s", spec, raw)
}
}
}
func TestOpenCodeEnvFilters(t *testing.T) {
t.Setenv("GITEA_TOKEN", "secret-gitea")
t.Setenv("OLLAMA_API_KEY", "keep-ollama")
t.Setenv("GADFLY_API_KEY", "secret-gadfly")
t.Setenv("GADFLY_FINDINGS_TOKEN", "secret-findings")
t.Setenv("ANTHROPIC_API_KEY", "keep-anthropic") // pass-through provider auth
t.Setenv("OPENAI_API_KEY", "keep-openai") // pass-through provider auth
t.Setenv("CLAUDE_CODE_OAUTH_TOKEN", "secret-claude")
t.Setenv("GADFLY_OPENCODE_MODEL", "keep-knob")
t.Setenv("OPENCODE_CONFIG_CONTENT", "should-not-inherit")
env := openCodeEnv()
has := func(k string) bool {
for _, kv := range env {
if strings.HasPrefix(kv, k+"=") {
return true
}
}
return false
}
// kept: the ollama key + the standard provider keys the opencode/<provider>/<model>
// pass-through form needs + opencode knobs + PATH
for _, k := range []string{"OLLAMA_API_KEY", "ANTHROPIC_API_KEY", "OPENAI_API_KEY", "GADFLY_OPENCODE_MODEL", "PATH"} {
if !has(k) {
t.Errorf("openCodeEnv dropped %s, but it should be kept", k)
}
}
// dropped: gadfly's own secrets + the claude engine's subscription token
// (OpenCode's anthropic provider uses ANTHROPIC_API_KEY, not this OAuth token).
for _, k := range []string{"GITEA_TOKEN", "GADFLY_API_KEY", "GADFLY_FINDINGS_TOKEN", "CLAUDE_CODE_OAUTH_TOKEN"} {
if has(k) {
t.Errorf("openCodeEnv leaked %s into the subprocess env", k)
}
}
// OPENCODE_CONFIG_CONTENT must NOT be inherited — runPass sets it, and a
// duplicate key would be ambiguous.
if has("OPENCODE_CONFIG_CONTENT") {
t.Errorf("openCodeEnv inherited OPENCODE_CONFIG_CONTENT; runPass sets it explicitly")
}
}
// stubOpenCode writes an executable shell stub that prints body and exits code,
// and returns an engine pointed at it.
func stubOpenCode(t *testing.T, body string, code int) *openCodeEngine {
t.Helper()
dir := t.TempDir()
path := dir + "/opencode-stub.sh"
script := "#!/bin/sh\nprintf '%s' " + shSingleQuote(body) + "\nexit " + itoa(code) + "\n"
if err := os.WriteFile(path, []byte(script), 0o755); err != nil {
t.Fatal(err)
}
return &openCodeEngine{bin: path, repoDir: dir}
}
func TestOpenCodeRunPassCleanResult(t *testing.T) {
e := stubOpenCode(t, " REVIEW TEXT ", 0)
out, err := e.runPass(context.Background(), "sys", "task", 0)
if err != nil || out != "REVIEW TEXT" {
t.Fatalf("clean result: got (%q, %v), want (REVIEW TEXT, nil)", out, err)
}
}
func TestOpenCodeRunPassEmptyIsError(t *testing.T) {
e := stubOpenCode(t, " ", 0)
out, err := e.runPass(context.Background(), "sys", "task", 0)
if err == nil {
t.Fatalf("empty output should be an error, got out=%q", out)
}
}
func TestOpenCodeRunPassNonZero(t *testing.T) {
e := stubOpenCode(t, "fatal: provider auth failed", 1)
_, err := e.runPass(context.Background(), "sys", "task", 0)
if err == nil || !strings.Contains(err.Error(), "opencode run failed") {
t.Fatalf("non-zero exit should error with detail, got %v", err)
}
}
// TestOpenCodeRunPassInjectsConfig proves the end-to-end env plumbing: the stub
// echoes OPENCODE_CONFIG_CONTENT back, and the emitted JSON must carry the exact
// system prompt as the gadfly agent's prompt.
func TestOpenCodeRunPassInjectsConfig(t *testing.T) {
dir := t.TempDir()
stub := dir + "/opencode-stub.sh"
script := "#!/bin/sh\nprintf '%s' \"$OPENCODE_CONFIG_CONTENT\"\n"
if err := os.WriteFile(stub, []byte(script), 0o755); err != nil {
t.Fatal(err)
}
e := newOpenCodeEngine("opencode/glm-5.2", dir)
e.bin = stub
sys := "SYSTEM-PROMPT-SENTINEL\nwith a second line"
out, err := e.runPass(context.Background(), sys, "task", 0)
if err != nil {
t.Fatalf("runPass: %v", err)
}
var cfg openCodeConfig
if err := json.Unmarshal([]byte(out), &cfg); err != nil {
t.Fatalf("injected config is not valid JSON: %v\n%s", err, out)
}
if got := cfg.Agent[openCodeAgentName].Prompt; got != sys {
t.Errorf("injected agent prompt = %q, want the system prompt", got)
}
}
+15 -11
View File
@@ -2,7 +2,6 @@ package main
import (
"fmt"
"os"
"strings"
)
@@ -11,6 +10,13 @@ import (
// than discovering them.
const defaultRecheckMaxSteps = 16
// defaultRecheckDiffChars caps the diff embedded in the recheck task. It is much
// smaller than the review task's GADFLY_MAX_DIFF_CHARS: the recheck already has
// the draft findings to verify and can pull the exact hunks it needs via the
// paginated get_diff tool (optionally scoped to a path), so re-embedding the
// whole diff on every recheck step is pure burn. Override: GADFLY_RECHECK_DIFF_CHARS.
const defaultRecheckDiffChars = 20000
// recheckSystemPrompt drives the second, adversarial verification pass. The
// model is given a DRAFT review and must independently confirm each finding
// against the real code before letting it survive — the antidote to a
@@ -49,18 +55,16 @@ Output rules:
- Do NOT invent new findings; this is a verification gate, not a fresh review.
- Do NOT include meta-commentary about the verification process or which
findings you dropped — output only the final, corrected review markdown.
- The draft ends with a fenced ` + "`gadfly-findings`" + ` JSON block. Regenerate it
so it lists ONLY the findings that SURVIVED your verification, in the same schema
({"file","line","severity","confidence","title"}; severity one of
critical/high/medium/small/trivial, confidence one of high/medium/low). If every
finding was dropped, emit an empty array ` + "`[]`" + `. Keep the block last.
- When done investigating, STOP calling tools and reply with the review.`
// recheckEnabled reports whether the verification pass should run. On unless
// GADFLY_RECHECK is explicitly a falsey value.
func recheckEnabled() bool {
switch strings.ToLower(strings.TrimSpace(os.Getenv("GADFLY_RECHECK"))) {
case "0", "false", "no", "off":
return false
default:
return true
}
}
func recheckEnabled() bool { return envBool("GADFLY_RECHECK", true) }
// shouldRecheck decides whether to run the verification pass for a given draft.
// A clean "no material issues" draft has nothing to verify, so it is skipped
@@ -79,11 +83,11 @@ func shouldRecheck(draft string) bool {
// scrutinize, with the full diff available via get_diff (and embedded here,
// truncated, to save a tool call).
func buildRecheckTask(draft, diff string) string {
maxDiff := envInt("GADFLY_MAX_DIFF_CHARS", defaultMaxDiffChars)
maxDiff := envInt("GADFLY_RECHECK_DIFF_CHARS", defaultRecheckDiffChars)
truncNote := ""
if maxDiff > 0 && len(diff) > maxDiff {
diff = diff[:maxDiff]
truncNote = fmt.Sprintf("\n\n[NOTE: diff truncated to %d chars here; read the changed files (or call get_diff, if available) for the full text.]", maxDiff)
truncNote = fmt.Sprintf("\n\n[NOTE: diff truncated to %d chars here; call get_diff (paginated; pass a `path` to scope it to one file) or read the changed files for the rest.]", maxDiff)
}
var b strings.Builder
+29 -8
View File
@@ -49,7 +49,7 @@ func TestRecheckEnabled(t *testing.T) {
}
func TestBuildRecheckTask(t *testing.T) {
t.Setenv("GADFLY_MAX_DIFF_CHARS", "")
t.Setenv("GADFLY_RECHECK_DIFF_CHARS", "")
draft := "VERDICT: Blocking issues found\n- foo.go:1 broken"
out := buildRecheckTask(draft, "diff --git a/x b/x\n+y\n")
if !strings.Contains(out, draft) {
@@ -77,25 +77,46 @@ func fakeModel(t *testing.T, reply string) llm.Model {
return m
}
func TestRunAgent_ReturnsOutput(t *testing.T) {
// newTestReviewExecutor builds a reviewExecutor over a fake model + repo for unit
// tests: no compaction summarizer and no budget, so it exercises the bare agent
// loop through the executus run kernel without any network call.
func newTestReviewExecutor(t *testing.T, mdl llm.Model, fs *repoFS) *reviewExecutor {
t.Helper()
rex, err := newReviewExecutor(fs, mdl, nil, "mock", nil)
if err != nil {
t.Fatal(err)
}
return rex
}
// testEngine wraps newTestReviewExecutor in a majordomoEngine for the
// runSpecialists-level tests.
func testEngine(t *testing.T, mdl llm.Model, fs *repoFS) *majordomoEngine {
t.Helper()
return &majordomoEngine{rex: newTestReviewExecutor(t, mdl, fs), mdl: mdl}
}
func TestReviewExecutor_ReturnsOutput(t *testing.T) {
fs, err := newRepoFS(t.TempDir(), "diff")
if err != nil {
t.Fatal(err)
}
mdl := fakeModel(t, " corrected review: No material issues found. ")
out, err := runAgent(context.Background(), mdl, fs, "sys", "task", 4)
rex := newTestReviewExecutor(t, mdl, fs)
out, err := rex.run(context.Background(), "sys", "task", 4)
if err != nil {
t.Fatalf("runAgent: %v", err)
t.Fatalf("run: %v", err)
}
if out != "corrected review: No material issues found." {
t.Errorf("runAgent should return trimmed model output, got %q", out)
t.Errorf("run should return trimmed model output, got %q", out)
}
}
func TestRunAgent_EmptyIsError(t *testing.T) {
func TestReviewExecutor_EmptyIsError(t *testing.T) {
fs, _ := newRepoFS(t.TempDir(), "diff")
mdl := fakeModel(t, " ")
if _, err := runAgent(context.Background(), mdl, fs, "sys", "task", 4); err == nil {
t.Error("runAgent should error on empty model output")
rex := newTestReviewExecutor(t, mdl, fs)
if _, err := rex.run(context.Background(), "sys", "task", 4); err == nil {
t.Error("run should error on empty model output")
}
}
+296
View File
@@ -0,0 +1,296 @@
package main
// Inline PR review. After the consensus comment is rendered, Gadfly also posts a
// single Gitea pull review (state COMMENT — advisory, NEVER request-changes or
// approve) whose inline comments anchor consensus findings to the exact changed
// lines. The issue comment stays the ranked overview; the review puts each
// finding next to the code it's about — the "reviewer integrated with Gitea" the
// project wanted, without ever blocking a merge.
//
// All of this is best-effort: disabled by GADFLY_INLINE_REVIEW=0 or when the diff
// / API creds aren't available, only anchors findings that land on a line present
// in the diff (Gitea rejects comments off the diff), and any error is logged to
// stderr without touching the consensus comment (already on stdout) or the exit
// code.
import (
"bytes"
"encoding/json"
"fmt"
"io"
"net/http"
"os"
"regexp"
"strconv"
"strings"
"time"
)
const (
// inlineReviewMarker tags our review body so a re-run can delete the previous
// one instead of stacking duplicate inline comments.
inlineReviewMarker = "<!-- gadfly-inline-review -->"
// maxInlineComments caps how many inline comments one review carries, so a
// huge diff can't produce a wall of annotations. Clusters are pre-sorted by
// agreement×severity, so the cap keeps the most important.
maxInlineComments = 25
inlineReviewHTTPTimeout = 20 * time.Second
)
// reviewComment is one inline comment in a Gitea pull review. Field names match
// the Gitea API EXACTLY (new_position = line in the new/head file).
type reviewComment struct {
Path string `json:"path"`
Body string `json:"body"`
NewPosition int `json:"new_position"`
}
// createReview is the POST /pulls/{n}/reviews body. event is ALWAYS "COMMENT".
type createReview struct {
Body string `json:"body"`
Event string `json:"event"`
Comments []reviewComment `json:"comments"`
}
// postInlineReview posts one COMMENT-state pull review with inline comments for
// consensus findings on changed lines. No-op + best-effort (see file comment).
func postInlineReview(clusters []cluster) {
if strings.EqualFold(strings.TrimSpace(os.Getenv("GADFLY_INLINE_REVIEW")), "0") {
return
}
api := strings.TrimRight(strings.TrimSpace(os.Getenv("GITEA_API")), "/")
token := strings.TrimSpace(os.Getenv("GITEA_TOKEN"))
pr := strings.TrimSpace(os.Getenv("GADFLY_PR"))
diffPath := strings.TrimSpace(os.Getenv("GADFLY_DIFF_FILE"))
if api == "" || token == "" || pr == "" || diffPath == "" {
return
}
diff, err := os.ReadFile(diffPath)
if err != nil {
fmt.Fprintln(os.Stderr, "gadfly: inline review: read diff:", err)
return
}
comments := inlineComments(clusters, parseDiffNewLines(string(diff)))
if len(comments) == 0 {
return // nothing anchors to a changed line; the consensus comment covers it
}
client := &http.Client{Timeout: inlineReviewHTTPTimeout}
base := fmt.Sprintf("%s/pulls/%s/reviews", api, pr)
deletePriorReviews(client, base, token) // avoid stacking on re-runs
body := fmt.Sprintf("%s\n🪰 **Gadfly consensus review** — %d inline finding%s on changed lines. See the consensus comment for the full ranked summary.\n\n<sub>Advisory only — does not block merge.</sub>",
inlineReviewMarker, len(comments), plural(len(comments)))
if err := giteaSend(client, http.MethodPost, base, token, createReview{Body: body, Event: "COMMENT", Comments: comments}); err != nil {
fmt.Fprintln(os.Stderr, "gadfly: inline review post:", err)
}
}
// inlineComments builds inline comments for the clusters that anchor to a line
// present in the diff, in priority order (clusters are pre-sorted), capped.
func inlineComments(clusters []cluster, addable map[string]map[int]bool) []reviewComment {
var out []reviewComment
for _, c := range clusters {
path := normPath(c.file)
anchor := anchorLine(addable[path], c.line, c.maxLine)
if anchor == 0 {
continue
}
out = append(out, reviewComment{Path: path, NewPosition: anchor, Body: inlineBody(c)})
if len(out) >= maxInlineComments {
break
}
}
return out
}
// anchorLine returns the first line in [lo,hi] that is an added line in the diff,
// or 0 if none. Scanning the cluster's whole span (not just its representative
// line) anchors a finding whose min line is just outside the diff but whose span
// still overlaps a changed line.
func anchorLine(added map[int]bool, lo, hi int) int {
if added == nil || lo <= 0 {
return 0
}
if hi < lo { // single-line cluster (maxLine unset)
hi = lo
}
for ln := lo; ln <= hi; ln++ {
if added[ln] {
return ln
}
}
return 0
}
// inlineBody renders one inline comment: severity + title, who flagged it, detail.
func inlineBody(c cluster) string {
var b strings.Builder
fmt.Fprintf(&b, "%s **%s**", sevIcon(c.severity), strings.TrimSpace(c.title))
fmt.Fprintf(&b, "\n\n_%s · flagged by %d model%s_", lensList(c.lenses), len(c.models), plural(len(c.models)))
if d := strings.TrimSpace(c.detail); d != "" {
fmt.Fprintf(&b, "\n\n%s", d)
}
b.WriteString("\n\n<sub>🪰 Gadfly · advisory</sub>")
return b.String()
}
// parseDiffNewLines returns, per file, the set of NEW-file line numbers that were
// ADDED in the unified diff — the safest lines for an inline comment to anchor to
// (Gitea reliably accepts comments on added lines). Context lines are walked to
// keep the line counter correct but are NOT recorded: anchoring only to added
// lines avoids the all-or-nothing review POST being rejected for an off-change
// anchor. Hunk lengths from the @@ header bound each hunk, so a content line that
// happens to start with "+++ " or "@@" is still read as content, not a header.
func parseDiffNewLines(diff string) map[string]map[int]bool {
out := map[string]map[int]bool{}
var file string
var newLine, oldRem, newRem int
inHunk := false
for _, line := range strings.Split(diff, "\n") {
if inHunk && (newRem > 0 || oldRem > 0) {
switch {
case strings.HasPrefix(line, "+"):
record(out, file, newLine) // added line — anchorable
newLine++
newRem--
case strings.HasPrefix(line, "-"):
oldRem--
case strings.HasPrefix(line, "\\"): // "\ No newline at end of file"
default: // context line (leading space, or an empty line): advance, don't record
newLine++
newRem--
oldRem--
}
if newRem <= 0 && oldRem <= 0 {
inHunk = false
}
continue
}
switch {
case strings.HasPrefix(line, "+++ "):
file = normPath(strings.TrimPrefix(line, "+++ "))
if file == "/dev/null" {
file = ""
}
case strings.HasPrefix(line, "@@"):
if m := hunkRe.FindStringSubmatch(line); m != nil && file != "" {
newLine, _ = strconv.Atoi(m[3])
oldRem = atoiOr(m[2], 1)
newRem = atoiOr(m[4], 1)
inHunk = newRem > 0 || oldRem > 0
}
}
}
return out
}
// hunkRe captures a unified-diff hunk header's old/new start+length:
// @@ -<oldStart>[,<oldLen>] +<newStart>[,<newLen>] @@
var hunkRe = regexp.MustCompile(`^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@`)
// normPath trims a unified-diff path down to a repo-relative one: strip a single
// leading "a/" or "b/" prefix (and any "./"), and surrounding whitespace. Applied
// to BOTH the diff paths and a finding's file so they match even when a model
// writes "./pkg/x.go" or the diff carries the "b/" prefix.
func normPath(p string) string {
p = strings.TrimSpace(p)
p = strings.TrimPrefix(p, "./")
if strings.HasPrefix(p, "a/") || strings.HasPrefix(p, "b/") {
p = p[2:]
}
return p
}
func record(out map[string]map[int]bool, file string, line int) {
if file == "" {
return
}
if out[file] == nil {
out[file] = map[int]bool{}
}
out[file][line] = true
}
// atoiOr parses s, returning def when s is empty or unparseable. Used for the
// optional hunk-length fields (absent => length 1).
func atoiOr(s string, def int) int {
if s == "" {
return def
}
if n, err := strconv.Atoi(s); err == nil {
return n
}
return def
}
// deletePriorReviews removes our previous inline reviews (matched by the body
// marker) so a re-run replaces rather than stacks. Best-effort and quiet.
func deletePriorReviews(client *http.Client, base, token string) {
const perPage = 50
for page := 1; page <= 10; page++ { // bound the scan, but page past 50 so a stale marked review isn't missed
req, err := http.NewRequest(http.MethodGet, fmt.Sprintf("%s?limit=%d&page=%d", base, perPage, page), nil)
if err != nil {
return
}
req.Header.Set("Authorization", "token "+token)
resp, err := client.Do(req)
if err != nil {
return
}
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
io.Copy(io.Discard, resp.Body)
resp.Body.Close()
return
}
var reviews []struct {
ID int `json:"id"`
Body string `json:"body"`
}
_ = json.NewDecoder(resp.Body).Decode(&reviews)
resp.Body.Close()
for _, r := range reviews {
if !strings.Contains(r.Body, inlineReviewMarker) {
continue
}
dreq, err := http.NewRequest(http.MethodDelete, fmt.Sprintf("%s/%d", base, r.ID), nil)
if err != nil {
continue
}
dreq.Header.Set("Authorization", "token "+token)
if dresp, err := client.Do(dreq); err == nil {
io.Copy(io.Discard, dresp.Body)
dresp.Body.Close()
}
}
if len(reviews) < perPage {
return // last page
}
}
}
// giteaSend marshals payload and sends it with Gitea's "token" auth scheme,
// treating a non-2xx response as an error (with a snippet of the body).
func giteaSend(client *http.Client, method, url, token string, payload any) error {
body, err := json.Marshal(payload)
if err != nil {
return err
}
req, err := http.NewRequest(method, url, bytes.NewReader(body))
if err != nil {
return err
}
req.Header.Set("Content-Type", "application/json")
req.Header.Set("Authorization", "token "+token)
resp, err := client.Do(req)
if err != nil {
return err
}
defer resp.Body.Close()
snippet, _ := io.ReadAll(io.LimitReader(resp.Body, 2048))
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
return fmt.Errorf("%s %s: status %d: %s", method, url, resp.StatusCode, strings.TrimSpace(string(snippet)))
}
return nil
}
+107
View File
@@ -0,0 +1,107 @@
package main
import (
"strings"
"testing"
)
func TestParseDiffNewLines(t *testing.T) {
diff := "diff --git a/a.go b/a.go\n" +
"index 111..222 100644\n" +
"--- a/a.go\n" +
"+++ b/a.go\n" +
"@@ -1,3 +1,4 @@\n" +
" line1\n" +
"-old2\n" +
"+new2\n" +
"+new3\n" +
" line4\n"
got := parseDiffNewLines(diff)
// Only ADDED lines anchor: new2 (line 2) and new3 (line 3). Context lines 1
// and 4 are walked for counting but not recorded.
want := map[int]bool{2: true, 3: true}
if len(got["a.go"]) != len(want) {
t.Fatalf("a.go lines = %v, want %v (added-only)", got["a.go"], want)
}
for ln := range want {
if !got["a.go"][ln] {
t.Errorf("expected a.go line %d anchorable", ln)
}
}
if got["a.go"][1] || got["a.go"][4] {
t.Error("context lines 1/4 should not be anchorable (added-only)")
}
}
func TestParseDiffNewLinesContentLooksLikeHeader(t *testing.T) {
// An added line whose CONTENT is "++ weird" appears as "+++ weird" in the
// diff. Hunk-length tracking must read it as content, not a file header.
diff := "--- a/x\n+++ b/x\n@@ -1,1 +1,2 @@\n ctx\n+++ weird\n"
got := parseDiffNewLines(diff)
if got["x"][1] { // context line, not recorded (added-only)
t.Errorf("line 1 is context, should not anchor: %v", got["x"])
}
if !got["x"][2] { // the "+++ weird" added line
t.Errorf("want added line 2 anchorable, got %v", got["x"])
}
}
func TestAnchorLineScansSpan(t *testing.T) {
// A cluster spanning 10..14 whose min line (10) isn't in the diff but whose
// span includes added line 14 must anchor to 14, not be dropped.
added := map[string]map[int]bool{"a.go": {14: true}}
clusters := []cluster{{file: "a.go", line: 10, maxLine: 14, severity: "high", title: "t", models: set("m1"), lenses: set("x")}}
cs := inlineComments(clusters, added)
if len(cs) != 1 || cs[0].NewPosition != 14 {
t.Fatalf("want anchor at 14 via span scan, got %+v", cs)
}
}
func TestParseDiffNewLinesMultiFile(t *testing.T) {
diff := "diff --git a/one.go b/one.go\n--- a/one.go\n+++ b/one.go\n@@ -0,0 +1,2 @@\n+a\n+b\n" +
"diff --git a/two.go b/two.go\n--- a/two.go\n+++ b/two.go\n@@ -5,0 +6,1 @@\n+c\n"
got := parseDiffNewLines(diff)
if !got["one.go"][1] || !got["one.go"][2] {
t.Errorf("one.go: want 1,2 got %v", got["one.go"])
}
if !got["two.go"][6] {
t.Errorf("two.go: want 6 got %v", got["two.go"])
}
}
func TestInlineCommentsNormalizesPaths(t *testing.T) {
// Diff path "b/pkg/x.go" -> "pkg/x.go"; a finding written as "./pkg/x.go" must
// still anchor (both normalize to the same repo-relative path).
addable := parseDiffNewLines("--- a/pkg/x.go\n+++ b/pkg/x.go\n@@ -0,0 +1,1 @@\n+bug\n")
clusters := []cluster{{file: "./pkg/x.go", line: 1, severity: "high", title: "t", models: set("m1"), lenses: set("x")}}
if cs := inlineComments(clusters, addable); len(cs) != 1 || cs[0].Path != "pkg/x.go" {
t.Errorf("path normalization failed to anchor: %+v", cs)
}
}
func TestInlineCommentsFiltersToDiffLines(t *testing.T) {
addable := map[string]map[int]bool{"a.go": {10: true, 11: true}}
clusters := []cluster{
{file: "a.go", line: 10, severity: "high", title: "anchored", models: set("m1", "m2"), lenses: set("security"), detail: "d"},
{file: "a.go", line: 99, severity: "high", title: "off-diff line", models: set("m1"), lenses: set("security")},
{file: "b.go", line: 1, severity: "high", title: "off-diff file", models: set("m1"), lenses: set("security")},
}
cs := inlineComments(clusters, addable)
if len(cs) != 1 {
t.Fatalf("want 1 anchorable inline comment, got %d", len(cs))
}
if cs[0].Path != "a.go" || cs[0].NewPosition != 10 {
t.Errorf("wrong anchor: %+v", cs[0])
}
if !strings.Contains(cs[0].Body, "anchored") || !strings.Contains(cs[0].Body, "2 model") {
t.Errorf("inline body missing title/agreement: %q", cs[0].Body)
}
}
func set(xs ...string) map[string]bool {
m := map[string]bool{}
for _, x := range xs {
m[x] = true
}
return m
}
+143 -16
View File
@@ -9,6 +9,7 @@ import (
"regexp"
"sort"
"strings"
"sync"
llm "gitea.stevedudenhoeffer.com/steve/majordomo/llm"
)
@@ -17,11 +18,13 @@ import (
// every tool caps how much it can pull in one call — a runaway read_file or
// grep would blow the window and stall the loop.
const (
maxFileBytes = 64 * 1024 // per read_file call
maxReadLines = 800 // per read_file call
maxGrepResults = 200 // per grep call
maxFindResults = 200 // per find_files call
maxLineLen = 400 // truncate any single returned line to this
maxFileBytes = 64 * 1024 // per read_file call
maxReadLines = 800 // per read_file call
maxGrepResults = 200 // per grep call
maxFindResults = 200 // per find_files call
maxLineLen = 400 // truncate any single returned line to this
maxGetDiffLines = 800 // per get_diff call (paginated window)
maxGetDiffBytes = 64 * 1024 // per get_diff call
)
// skipDirs are never descended into by grep / find_files — noise and bulk that
@@ -40,6 +43,12 @@ type repoFS struct {
root string // absolute, symlink-resolved repo root
diff string // the full PR unified diff (served by get_diff)
worker llm.Model // optional cheap model for delegate_investigation; nil = no delegation
// diffLines caches the split diff so paging through get_diff doesn't re-split
// the whole (possibly large) diff on every call. The diff is immutable, so the
// cache is computed once and safe to share across concurrent lenses.
diffOnce sync.Once
diffLines []string
}
// newRepoFS resolves root to an absolute, symlink-free path.
@@ -108,15 +117,24 @@ func (r *repoFS) fsTools() []llm.Tool {
}
}
// toolbox builds the reviewer's toolbox: the read-only repo tools, plus the
// delegate_investigation tool when a worker model is configured.
func (r *repoFS) toolbox() (*llm.Toolbox, error) {
box := llm.NewToolbox("gadfly")
// allTools is the single source of truth for the reviewer's tool set: the
// read-only repo tools, plus delegate_investigation when a worker model is
// configured. Both the executus registry (gadflyToolRegistry, the production
// path) and toolbox() build from this list.
func (r *repoFS) allTools() []llm.Tool {
tools := r.fsTools()
if r.worker != nil {
tools = append(tools, r.delegateTool())
}
for _, t := range tools {
return tools
}
// toolbox builds a majordomo toolbox from allTools(). The production review path
// now goes through executus's tool.Registry (see executus.go); this remains for
// the toolbox-level tests (the `call` helper).
func (r *repoFS) toolbox() (*llm.Toolbox, error) {
box := llm.NewToolbox("gadfly")
for _, t := range r.allTools() {
if err := box.Add(t); err != nil {
return nil, fmt.Errorf("add tool %q: %w", t.Name, err)
}
@@ -396,15 +414,124 @@ func (r *repoFS) findFilesTool() llm.Tool {
)
}
type getDiffArgs struct {
Path string `json:"path,omitempty" description:"Optional changed-file path (e.g. pkg/foo/bar.go); returns ONLY that file's diff hunks. Omit for the whole diff. Use this on a large PR to pull just the file a finding is about."`
StartLine int `json:"start_line,omitempty" description:"Optional 1-based line to start from within the (whole or path-scoped) diff (default 1)."`
Limit int `json:"limit,omitempty" description:"Optional max number of diff lines to return (default/maximum 800)."`
}
func (r *repoFS) getDiffTool() llm.Tool {
return llm.DefineTool[struct{}](
return llm.DefineTool[getDiffArgs](
"get_diff",
"Return the complete unified diff under review. The diff is also included (possibly truncated) in the task message; call this to get the full, untruncated text.",
func(_ context.Context, _ struct{}) (any, error) {
if strings.TrimSpace(r.diff) == "" {
return "(empty diff)", nil
"Return the unified diff under review as a numbered, PAGINATED window (like read_file) — not the whole diff at once, so a huge PR can't blow the context window. Pass `path` to fetch just one changed file's hunks, or `start_line`/`limit` to page through. A truncated copy of the diff is also embedded in the task message.",
func(_ context.Context, args getDiffArgs) (any, error) {
scope := "the diff"
var lines []string
if p := strings.TrimSpace(args.Path); p != "" {
lines = diffLinesForPath(r.diff, p)
if len(lines) == 0 {
return fmt.Sprintf("(no diff hunks for path %q; check it against the changed-files list — the path must match a file the diff touches)", p), nil
}
scope = "the diff for " + p
} else {
lines = r.diffAllLines()
if len(lines) == 0 {
return "(empty diff)", nil
}
}
return r.diff, nil
return windowDiff(lines, scope, args.StartLine, args.Limit), nil
},
)
}
// diffAllLines returns the whole diff split into lines, cached so paging never
// re-splits the (possibly large) diff.
func (r *repoFS) diffAllLines() []string {
r.diffOnce.Do(func() { r.diffLines = splitDiffLines(r.diff) })
return r.diffLines
}
// splitDiffLines splits a unified diff into lines, dropping the single trailing
// empty element a trailing newline produces — otherwise windowDiff would emit a
// blank final line and over-count the total by one.
func splitDiffLines(diff string) []string {
lines := strings.Split(diff, "\n")
if n := len(lines); n > 0 && lines[n-1] == "" {
lines = lines[:n-1]
}
return lines
}
// windowDiff returns a numbered, paginated slice of pre-split diff lines,
// mirroring read_file's caps (maxGetDiffLines / maxGetDiffBytes / maxLineLen) so
// a single get_diff call can never dump a multi-hundred-KB diff into the
// transcript — the amplifier behind the large-PR token burn. The full diff stays
// reachable by paging with start_line, or scoped per file via the path arg.
func windowDiff(lines []string, scope string, start, limit int) string {
total := len(lines)
if start < 1 {
start = 1
}
if limit <= 0 || limit > maxGetDiffLines {
limit = maxGetDiffLines
}
if start > total {
return fmt.Sprintf("(%s has %d lines; nothing at/after line %d)", scope, total, start)
}
var b strings.Builder
emitted := 0
i := start - 1
for ; i < total; i++ {
if emitted >= limit || b.Len() >= maxGetDiffBytes {
break
}
line := lines[i]
if len(line) > maxLineLen {
line = line[:maxLineLen] + "…"
}
fmt.Fprintf(&b, "%d\t%s\n", i+1, line)
emitted++
}
if i < total {
// i (0-based) is the first line NOT emitted; line i was the last shown.
fmt.Fprintf(&b, "... (%s truncated after line %d of %d; call get_diff again with start_line=%d for the rest, or pass a `path` to scope to one file)\n", scope, i, total, i+1)
}
return b.String()
}
// diffLinesForPath returns the unified-diff lines for one changed file: from the
// `diff --git` header that names path through to the next header (or end). The
// header names two path tokens (a/<old> b/<new>); a match is on a WHOLE token,
// so path "foo.go" does not pull in "barfoo.go" — and a trailing "/" scopes to a
// directory (e.g. "pkg/foo/" matches pkg/foo/bar.go).
func diffLinesForPath(diff, path string) []string {
want := strings.TrimPrefix(strings.TrimPrefix(strings.TrimSpace(path), "a/"), "b/")
var out []string
inSection := false
for _, ln := range splitDiffLines(diff) {
if strings.HasPrefix(ln, "diff --git ") {
inSection = diffHeaderNames(ln, want)
}
if inSection {
out = append(out, ln)
}
}
return out
}
// diffHeaderNames reports whether a `diff --git a/X b/Y` header names want as one
// of its (a/b-stripped) path tokens — exact whole-token match, or a directory
// prefix when want ends with "/".
func diffHeaderNames(header, want string) bool {
fields := strings.Fields(header)
if len(fields) < 3 {
return false
}
for _, f := range fields[2:] {
p := strings.TrimPrefix(strings.TrimPrefix(f, "a/"), "b/")
if p == want || (strings.HasSuffix(want, "/") && strings.HasPrefix(p, want)) {
return true
}
}
return false
}
+81 -3
View File
@@ -197,15 +197,93 @@ func TestFindFilesTool(t *testing.T) {
func TestGetDiffTool(t *testing.T) {
root := buildFixtureRepo(t)
const diff = "diff --git a/x b/x\n+added line\n"
const diff = "diff --git a/x b/x\n--- a/x\n+++ b/x\n+added line\n" +
"diff --git a/y.go b/y.go\n--- a/y.go\n+++ b/y.go\n+y change\n"
fs, _ := newRepoFS(root, diff)
// Default: the whole diff as a NUMBERED window (paginated), not a raw dump —
// so a huge PR can't be poured into the transcript in one call.
out, err := call(t, fs, "get_diff", map[string]any{})
if err != nil {
t.Fatalf("get_diff: %v", err)
}
if !strings.Contains(out, "1\tdiff --git a/x b/x") {
t.Errorf("get_diff should return a numbered window, got:\n%s", out)
}
if !strings.Contains(out, "+added line") || !strings.Contains(out, "+y change") {
t.Errorf("the full (short) diff window should include every hunk, got:\n%s", out)
}
// path filter: only the named file's hunks come back.
out, err = call(t, fs, "get_diff", map[string]any{"path": "y.go"})
if err != nil {
t.Fatalf("get_diff path: %v", err)
}
if !strings.Contains(out, "y change") {
t.Errorf("get_diff path=y.go should include y's hunk, got:\n%s", out)
}
if strings.Contains(out, "added line") {
t.Errorf("get_diff path=y.go must NOT include x's hunk, got:\n%s", out)
}
// unknown path: a clear note, never an error.
out, err = call(t, fs, "get_diff", map[string]any{"path": "nope.txt"})
if err != nil {
t.Fatalf("get_diff unknown path: %v", err)
}
if !strings.Contains(out, "no diff hunks") {
t.Errorf("get_diff for an unknown path should note no hunks, got:\n%s", out)
}
}
// TestGetDiffTool_Paginates: a diff longer than the per-call line cap is returned
// as a truncated window with a paging hint, and start_line pages past it — the
// mechanism that stops get_diff from dumping a multi-hundred-KB diff at once.
func TestGetDiffTool_Paginates(t *testing.T) {
diff := "diff --git a/big b/big\n" + strings.Repeat("+line\n", maxGetDiffLines+50)
fs, _ := newRepoFS(t.TempDir(), diff)
out, err := call(t, fs, "get_diff", map[string]any{})
if err != nil {
t.Fatalf("get_diff: %v", err)
}
if out != diff {
t.Errorf("get_diff returned %q, want %q", out, diff)
if !strings.Contains(out, "truncated after line") {
t.Error("a diff longer than the per-call cap should be truncated with a paging hint")
}
if strings.Contains(out, "801\t") {
t.Error("the first window must stop at the line cap, not reach line 801")
}
out, err = call(t, fs, "get_diff", map[string]any{"start_line": 805})
if err != nil {
t.Fatalf("get_diff page: %v", err)
}
if !strings.Contains(out, "805\t") {
t.Error("paging with start_line=805 should include line 805")
}
}
// TestDiffLinesForPath_Anchored: get_diff path= matches on a WHOLE path token,
// so "foo.go" never pulls in "barfoo.go" (the unanchored-substring weakness the
// swarm flagged), while a trailing "/" still scopes to a directory.
func TestDiffLinesForPath_Anchored(t *testing.T) {
diff := "diff --git a/foo.go b/foo.go\n+foo change\n" +
"diff --git a/barfoo.go b/barfoo.go\n+barfoo change\n" +
"diff --git a/pkg/x.go b/pkg/x.go\n+x change\n"
joined := strings.Join(diffLinesForPath(diff, "foo.go"), "\n")
if !strings.Contains(joined, "foo change") {
t.Errorf("foo.go should match its own hunk:\n%s", joined)
}
if strings.Contains(joined, "barfoo change") {
t.Errorf("foo.go must NOT match barfoo.go (unanchored substring regression):\n%s", joined)
}
if !strings.Contains(strings.Join(diffLinesForPath(diff, "pkg/"), "\n"), "x change") {
t.Error("a trailing-slash path should scope to the directory (pkg/ -> pkg/x.go)")
}
if len(diffLinesForPath(diff, "nope.go")) != 0 {
t.Error("an unknown path should yield no lines")
}
}
+20 -51
View File
@@ -37,10 +37,11 @@ func lastUserText(req llm.Request) string {
return req.Messages[len(req.Messages)-1].Text()
}
// TestRunAgent_WrapUpNudgeProducesAnswer: a model that keeps calling tools until
// it is nudged to wrap up should still finish inside its budget — the steer
// message arrives a few steps before the cap and the model writes its answer.
func TestRunAgent_WrapUpNudgeProducesAnswer(t *testing.T) {
// TestReviewExecutor_WrapUpNudgeProducesAnswer: a model that keeps calling tools
// until it is nudged to wrap up should still finish inside its budget — the steer
// message (delivered by the executus wrap-up critic a few steps before the cap)
// arrives and the model writes its answer.
func TestReviewExecutor_WrapUpNudgeProducesAnswer(t *testing.T) {
t.Setenv("GADFLY_WRAPUP_RESERVE", "4")
final := "VERDICT: No material issues found."
@@ -60,9 +61,10 @@ func TestRunAgent_WrapUpNudgeProducesAnswer(t *testing.T) {
}
fs, _ := newRepoFS(t.TempDir(), "diff --git a/x b/x\n+y\n")
out, err := runAgent(context.Background(), mdl, fs, "sys", "task", 12)
rex := newTestReviewExecutor(t, mdl, fs)
out, err := rex.run(context.Background(), "sys", "task", 12)
if err != nil {
t.Fatalf("runAgent should succeed via wrap-up nudge, got error: %v", err)
t.Fatalf("run should succeed via wrap-up nudge, got error: %v", err)
}
if out != final {
t.Errorf("expected final review %q, got %q", final, out)
@@ -72,24 +74,19 @@ func TestRunAgent_WrapUpNudgeProducesAnswer(t *testing.T) {
}
}
// TestRunAgent_FinalizationFallback: a model that ignores the wrap-up nudge and
// spins on tools until the cap should NOT hard-fail — the tool-free finalization
// pass forces a final answer out of the transcript.
func TestRunAgent_FinalizationFallback(t *testing.T) {
// TestReviewExecutor_ExhaustionWithoutAnswerIsError: a model that ignores the
// wrap-up nudge and spins on tools until the step cap produces no final answer.
// The transcript-based forced-finalization fallback was removed in the executus
// re-platform (run.Result does not expose the loop transcript), so the pass now
// surfaces an error — which reviewWithSpecialist renders as an advisory "reviewer
// failed to complete" notice rather than a phantom success.
func TestReviewExecutor_ExhaustionWithoutAnswerIsError(t *testing.T) {
t.Setenv("GADFLY_WRAPUP_RESERVE", "2")
final := "VERDICT: Minor issues\n- something"
forcedCalled := false
n := 0
p := fake.New("fake", fake.WithDefault(func(_ string, req llm.Request) fake.Step {
// Only the tool-free finalization pass forbids tools — reply there.
if req.ToolChoice == "none" {
forcedCalled = true
return fake.Reply(final)
}
// Otherwise keep spinning, ignoring the wrap-up nudge entirely.
p := fake.New("fake", fake.WithDefault(func(_ string, _ llm.Request) fake.Step {
n++
return spinToolCall(n)
return spinToolCall(n) // spin forever, ignoring the wrap-up nudge
}))
mdl, err := p.Model("mock")
if err != nil {
@@ -97,37 +94,9 @@ func TestRunAgent_FinalizationFallback(t *testing.T) {
}
fs, _ := newRepoFS(t.TempDir(), "diff --git a/x b/x\n+y\n")
out, err := runAgent(context.Background(), mdl, fs, "sys", "task", 6)
if err != nil {
t.Fatalf("runAgent should recover via finalization fallback, got error: %v", err)
}
if !forcedCalled {
t.Error("finalization fallback was never invoked")
}
if out != final {
t.Errorf("expected forced final answer %q, got %q", final, out)
}
}
// TestRunAgent_FallbackStillEmptyIsError: if even the tool-free finalization
// yields nothing, runAgent surfaces an error rather than a phantom success.
func TestRunAgent_FallbackStillEmptyIsError(t *testing.T) {
n := 0
p := fake.New("fake", fake.WithDefault(func(_ string, req llm.Request) fake.Step {
if req.ToolChoice == "none" {
return fake.Reply(" ") // finalization produces only whitespace
}
n++
return spinToolCall(n)
}))
mdl, err := p.Model("mock")
if err != nil {
t.Fatal(err)
}
fs, _ := newRepoFS(t.TempDir(), "diff --git a/x b/x\n+y\n")
if _, err := runAgent(context.Background(), mdl, fs, "sys", "task", 4); err == nil {
t.Error("runAgent should error when the finalization fallback also yields no output")
rex := newTestReviewExecutor(t, mdl, fs)
if _, err := rex.run(context.Background(), "sys", "task", 6); err == nil {
t.Error("run should error when the model exhausts its steps without an answer")
}
}
+243 -29
View File
@@ -33,18 +33,34 @@
# Optional config:
# GADFLY_MODELS comma-separated model ids/specs (alias: OLLAMA_REVIEW_MODELS)
# GADFLY_PROVIDER majordomo provider for bare model ids (default ollama-cloud;
# e.g. "ollama" local, "openai", "anthropic", "google")
# e.g. "ollama" local, "openai", "anthropic", "google",
# "qwen" Alibaba Model Studio, "kimi" Moonshot)
# GADFLY_BASE_URL override backend endpoint (OpenAI/Ollama-compatible servers)
# GADFLY_API_KEY provider key (else provider's standard env: OPENAI_API_KEY, …)
# QWEN_API_KEY Alibaba Model Studio key, for GADFLY_MODELS entries like
# "qwen/qwen3.8-max". Read ONLY by the qwen provider — it
# does not fall back to OPENAI_API_KEY, so a forgotten key
# is a clean skip notice naming this variable, not a 401.
# KIMI_API_KEY Moonshot key, same deal for "kimi/<model>". Distinct from
# the ollama-cloud "kimi-k2.6:cloud" entry, which is keyed
# by OLLAMA_CLOUD_API_KEY.
# CLAUDE_CODE_OAUTH_TOKEN auth for the claude-code engine (GADFLY_MODELS entry
# "claude-code"/"claude-code/<model>"); Pro/Max subscription
# token from `claude setup-token`. Else ANTHROPIC_API_KEY.
# OLLAMA_CLOUD_API_KEY also feeds the opencode engine (GADFLY_MODELS entry
# "opencode/<model>"): the bundled `opencode` CLI drives
# that ollama-cloud model, for benchmarking the two
# harnesses on the same model. Tune via GADFLY_OPENCODE_*.
# GADFLY_TRIGGER_PHRASE comment phrase that triggers a re-review (default "@gadfly review")
# GADFLY_ALLOWED_USERS comma-separated usernames allowed to comment-trigger;
# empty => fall back to "is a repo collaborator"
# GADFLY_FINDINGS_URL optional gadfly-reports store base URL; set to POST the run +
# findings for model-quality tracking (off when empty)
# GADFLY_FINDINGS_TOKEN optional bearer token for the gadfly-reports store
# GADFLY_CONSOLIDATE cross-model consensus comment: "auto" (default; on for >=2
# models), "1" force on, "0" force off (one comment per model)
# GADFLY_INLINE_REVIEW when consolidating, also post a COMMENT-state PR review with
# inline comments on changed lines (default on; "0" disables)
set -uo pipefail
# One model by default: the specialist suite already provides breadth, so a
@@ -64,6 +80,29 @@ die() { log "ERROR: $*"; exit 1; }
API() { curl -fsS --connect-timeout 20 --max-time 30 -H "Authorization: token ${GITEA_TOKEN}" "$@"; }
# upsert_comment_body MARKER BODY — create or update (by leading MARKER) a single
# PR comment. Mirrors run.sh's per-model upsert; used for the consensus comment
# and the per-model fallback when consolidation is on.
upsert_comment_body() {
local marker="$1" body="$2" post_body existing_id="" page=1 cmts
post_body="$(jq -n --arg b "$body" '{body:$b}')"
while [ "$page" -le 10 ]; do
cmts="$(API "${GITEA_API}/issues/${PR}/comments?limit=50&page=${page}" || echo '[]')"
[ "$(echo "$cmts" | jq 'length')" = "0" ] && break
existing_id="$(echo "$cmts" | jq -r --arg m "$marker" \
'.[] | select(.body != null and (.body | startswith($m))) | .id' | head -n1)"
[ -n "$existing_id" ] && break
page=$((page+1))
done
if [ -n "$existing_id" ]; then
curl -sS --connect-timeout 20 --max-time 30 -X PATCH -H "Authorization: token ${GITEA_TOKEN}" \
-H "Content-Type: application/json" "${GITEA_API}/issues/comments/${existing_id}" -d "$post_body" >/dev/null
else
curl -sS --connect-timeout 20 --max-time 30 -X POST -H "Authorization: token ${GITEA_TOKEN}" \
-H "Content-Type: application/json" "${GITEA_API}/issues/${PR}/comments" -d "$post_body" >/dev/null
fi
}
# --- is the commenter allowed to trigger a re-review? ----------------------
actor_allowed() {
local actor="$1"
@@ -83,6 +122,26 @@ actor_allowed() {
}
# --- trigger gating --------------------------------------------------------
# Gitea >= 1.27 (breaking change go-gitea#37478, "improve support for reusable
# workflows") runs a CALLED workflow with github.event_name = 'workflow_call'
# instead of propagating the caller's event, so every consumer stub's trigger
# arrived here as an unhandled event and the review silently self-skipped
# (observed 2026-07-14: mort PRs #1445-#1447 got 1-second "success" runs).
# Reclassify from the forwarded event payload: a comment body can only come
# from issue_comment (its trigger-phrase + actor gates still apply below);
# otherwise a PR number means a pull_request-shaped trigger (this also covers
# dispatch-through-reusable, whose pr_number input populates PR — the draft
# check degrades safely since IS_DRAFT defaults false). Neither → fall through
# to the unhandled-event skip. Pre-1.27 servers still send the caller's event
# name and never enter this branch.
if [ "${EVENT_NAME:-}" = "workflow_call" ]; then
if [ -n "${COMMENT_BODY:-}" ]; then
EVENT_NAME="issue_comment"
elif [ -n "${PR:-}" ]; then
EVENT_NAME="pull_request"
fi
log "caller event arrived as 'workflow_call'; reclassified to '${EVENT_NAME}'"
fi
case "$EVENT_NAME" in
workflow_dispatch)
log "manual dispatch for PR #${PR}" ;;
@@ -147,14 +206,48 @@ export GADFLY_FINDINGS_TOKEN="${GADFLY_FINDINGS_TOKEN:-}"
# provider key envs (OPENAI_API_KEY, …) are inherited by run.sh and the binary.
#
# Concurrency: each PROVIDER is its own lane and lanes run in PARALLEL, so a fast
# cloud provider isn't stuck behind a slow local box. Within a lane, at most
# `cap` models run at once. cap = GADFLY_PROVIDER_CONCURRENCY's "provider=N"
# entry, else GADFLY_CONCURRENCY (default 1). A model's provider is the spec's
# first path segment ("m1pro/qwen3.6:35b-mlx" -> m1pro), or GADFLY_PROVIDER /
# ollama-cloud for a bare id. Default (cap 1) keeps a single-provider pool fully
# sequential, exactly as before.
# cloud provider isn't stuck behind a slow local box. Within a lane ALL of the
# provider's models run at once; the real throttle is a single PROVIDER-WIDE lens
# budget (a shared permit pool, seeded per lane) that every model's lenses draw
# from — so total lens passes in flight per provider is bounded, but a model
# winding down to its last lens immediately yields its freed permits to another
# model's queued lenses instead of holding a whole "model slot" (the old
# GADFLY_PROVIDER_CONCURRENCY model cap, now removed, caused that tail stall). The
# budget = GADFLY_PROVIDER_LENS_CONCURRENCY's "provider=N" entry, else
# GADFLY_LENS_CONCURRENCY (default 1). A model's provider is the spec's first path
# segment ("m1pro/qwen3.6:35b-mlx" -> m1pro), or GADFLY_PROVIDER / ollama-cloud
# for a bare id.
MODELS="${GADFLY_MODELS:-${OLLAMA_REVIEW_MODELS:-$DEFAULT_MODELS}}"
DEFAULT_CONC="${GADFLY_CONCURRENCY:-1}"
# --- huge-PR downshift ------------------------------------------------------
# A very large diff is what burns the model budget: every review step re-sends
# it, multiplied across models × lenses × passes × steps (this is what nuked a
# whole Ollama Cloud block on one giant PR). entrypoint is the only process that
# spans the whole fleet, so the fleet-wide size decision lives here: size the PR
# diff ONCE, and above GADFLY_HUGE_DIFF_BYTES collapse to a single cheap model +
# a focused lens subset, fewer steps, no recheck, and a smaller embedded diff.
# A finished shallow review beats a budget-nuking one. All knobs override; set
# GADFLY_HUGE_DIFF_BYTES=0 to disable. Small PRs are never touched.
HUGE_PR=0
HUGE_DIFF_BYTES="${GADFLY_HUGE_DIFF_BYTES:-600000}"
if [ "$HUGE_DIFF_BYTES" -gt 0 ] 2>/dev/null; then
PR_DIFF_BYTES="$(API "${GITEA_API}/pulls/${PR}.diff" 2>/dev/null | wc -c | tr -d '[:space:]')"
[ -z "$PR_DIFF_BYTES" ] && PR_DIFF_BYTES=0
if [ "$PR_DIFF_BYTES" -gt "$HUGE_DIFF_BYTES" ] 2>/dev/null; then
HUGE_PR=1
log "huge PR: diff ${PR_DIFF_BYTES}B > ${HUGE_DIFF_BYTES}B — downshifting the fleet (advisory)"
MODELS="${GADFLY_HUGE_DIFF_MODELS:-${MODELS%%,*}}" # first model only by default
export GADFLY_SPECIALISTS="${GADFLY_HUGE_DIFF_SPECIALISTS:-security,correctness,error-handling}"
export GADFLY_MAX_STEPS="${GADFLY_HUGE_DIFF_MAX_STEPS:-12}"
export GADFLY_RECHECK_MAX_STEPS="${GADFLY_HUGE_DIFF_RECHECK_MAX_STEPS:-8}"
export GADFLY_RECHECK="${GADFLY_HUGE_DIFF_RECHECK:-0}" # skip recheck on huge PRs
# The Go-visible name directly (run.sh prefers GADFLY_MAX_DIFF_CHARS over its
# own MAX_DIFF_CHARS), so the cap is honored without relying on run.sh's alias.
export GADFLY_MAX_DIFF_CHARS="${GADFLY_HUGE_DIFF_MAX_DIFF_CHARS:-20000}"
# Surfaced on each posted comment so the shallower review is self-explaining.
export GADFLY_NOTICE="⚠️ Large PR (${PR_DIFF_BYTES} bytes): Gadfly downshifted to a focused, single-model review to stay within budget — coverage is intentionally shallower. Consider splitting the PR for a deeper review."
fi
fi
provider_of() { case "$1" in */*) echo "${1%%/*}";; *) echo "${GADFLY_PROVIDER:-ollama-cloud}";; esac; }
@@ -164,23 +257,33 @@ provider_of() { case "$1" in */*) echo "${1%%/*}";; *) echo "${GADFLY_PROVIDER:-
STATUS_DIR="${WORKDIR}/status"
status_file_for() { echo "${STATUS_DIR}/$(echo "$1" | tr -c '[:alnum:]._-' '_').json"; }
provider_cap() { # provider -> concurrency (override map "p=N,...", else default)
# Root of the per-provider lens permit pools (one subdir per lane, seeded by
# run_lane). Cleared up front so a reused WORKDIR can't leak stale permit files.
LENS_SEM_ROOT="${WORKDIR}/lenssem"
rm -rf "$LENS_SEM_ROOT" 2>/dev/null || true
provider_lens_cap() { # provider -> provider-wide lens budget (permit-pool size)
local p="$1" item k v
IFS=',' read -ra _caps <<< "${GADFLY_PROVIDER_CONCURRENCY:-}"
for item in "${_caps[@]}"; do
IFS=',' read -ra _lcaps <<< "${GADFLY_PROVIDER_LENS_CONCURRENCY:-}"
for item in "${_lcaps[@]}"; do
k="$(echo "${item%%=*}" | tr -d '[:space:]')"
v="$(echo "${item#*=}" | tr -d '[:space:]')"
if [ "$k" = "$p" ] && [ -n "$v" ]; then echo "$v"; return; fi
done
echo "$DEFAULT_CONC"
echo "${GADFLY_LENS_CONCURRENCY:-1}"
}
review_one() {
local sf=""
[ "${GADFLY_STATUS_BOARD:-1}" != "0" ] && sf="$(status_file_for "$1")"
PROVIDER=ollama MODEL="$1" GADFLY_BIN="/usr/local/bin/gadfly" GADFLY_REPO_DIR="$REPO_DIR" \
GADFLY_STATUS_FILE="$sf" \
bash "${SCRIPTS_DIR}/run.sh" || log "model $1 failed (continuing)"
local m="$1" sem_dir="${2:-}" sem_size="${3:-}" sf="" ff=""
[ "${GADFLY_STATUS_BOARD:-1}" != "0" ] && sf="$(status_file_for "$m")"
[ "$CONSOLIDATE" = "1" ] && ff="$(findings_file_for "$m")"
# GADFLY_LENS_SEM_DIR/_SIZE point the binary at this provider's shared lens
# permit pool (empty => the binary just uses its in-process lens limit). These
# are inherited by the binary through run.sh's environment.
PROVIDER=ollama MODEL="$m" GADFLY_BIN="/usr/local/bin/gadfly" GADFLY_REPO_DIR="$REPO_DIR" \
GADFLY_STATUS_FILE="$sf" GADFLY_FINDINGS_OUT="$ff" GADFLY_CONSOLIDATE="$CONSOLIDATE" \
GADFLY_LENS_SEM_DIR="$sem_dir" GADFLY_LENS_SEM_SIZE="$sem_size" \
bash "${SCRIPTS_DIR}/run.sh" || log "model $m failed (continuing)"
# If the binary never wrote real status (run.sh skipped it: empty diff, no key,
# binary missing), the pre-seed stays {started:0, done:false} and the board
# would show this model "waiting to start" forever and never reach N/N. Mark
@@ -196,6 +299,34 @@ IFS=',' read -ra _raw <<< "$MODELS" || true
MODEL_LIST=()
for raw in "${_raw[@]}"; do m="$(echo "$raw" | tr -d '[:space:]')"; [ -n "$m" ] && MODEL_LIST+=("$m"); done
# --- cross-model consolidation decision ------------------------------------
# With >=2 models, post ONE consensus comment (findings clustered + ranked by
# cross-model agreement) instead of N per-model walls of prose. Each model writes
# its findings to FINDINGS_DIR; a final pass (the binary in GADFLY_CONSOLIDATE_DIR
# mode) renders the consensus comment. GADFLY_CONSOLIDATE: "auto" (default; on for
# >=2 models), "1" force on, "0" force off (keep per-model comments).
FINDINGS_DIR="${WORKDIR}/findings"
CONSOLIDATE=0
case "${GADFLY_CONSOLIDATE:-auto}" in
1) CONSOLIDATE=1 ;;
0) CONSOLIDATE=0 ;;
*) [ "${#MODEL_LIST[@]}" -ge 2 ] && CONSOLIDATE=1 ;;
esac
# A model spec can contain '/' and ':' (e.g. claude-code/opus, qwen3:14b), so
# sanitize to a flat filename — but append a checksum of the raw spec so two
# specs that sanitize the same (foo:bar vs foo/bar -> foo_bar) don't collide onto
# one file and silently drop a model from the consensus.
findings_file_for() {
local safe sum
safe="$(echo "$1" | tr -c '[:alnum:]._-' '_')"
sum="$(printf '%s' "$1" | cksum | cut -d' ' -f1)"
echo "${FINDINGS_DIR}/${safe}-${sum}.json"
}
if [ "$CONSOLIDATE" = "1" ]; then
rm -rf "$FINDINGS_DIR"; mkdir -p "$FINDINGS_DIR"
log "consolidation ON: ${#MODEL_LIST[@]} models -> one consensus comment"
fi
# Distinct providers, in first-seen order (no associative arrays — portable).
PROVIDERS=""
for m in "${MODEL_LIST[@]}"; do
@@ -203,16 +334,19 @@ for m in "${MODEL_LIST[@]}"; do
case " $PROVIDERS " in *" $p "*) ;; *) PROVIDERS="${PROVIDERS}${PROVIDERS:+ }$p" ;; esac
done
run_lane() { # $1=provider: run its models, at most `cap` at a time
local p="$1" cap inflight=0 m
cap="$(provider_cap "$p")"; [ "$cap" -ge 1 ] 2>/dev/null || cap=1
run_lane() { # $1=provider: run ALL its models at once, throttled only by a shared
# provider-wide lens permit pool (no per-model cap).
local p="$1" budget sem_dir m
budget="$(provider_lens_cap "$p")"; [ "$budget" -ge 1 ] 2>/dev/null || budget=1
local mine=()
for m in "${MODEL_LIST[@]}"; do [ "$(provider_of "$m")" = "$p" ] && mine+=("$m"); done
log "lane ${p}: cap ${cap}; models: ${mine[*]}"
# Seed this provider's lens permit pool: a directory the binary flocks N permit
# files in (created lazily), one shared budget across every model in the lane.
sem_dir="${LENS_SEM_ROOT}/$(echo "$p" | tr -c '[:alnum:]._-' '_')"
mkdir -p "$sem_dir"
log "lane ${p}: lens budget ${budget} shared across ${#mine[@]} model(s): ${mine[*]}"
for m in "${mine[@]}"; do
review_one "$m" &
inflight=$((inflight+1))
if [ "$inflight" -ge "$cap" ]; then wait -n 2>/dev/null || wait; inflight=$((inflight-1)); fi
review_one "$m" "$sem_dir" "$budget" &
done
wait
}
@@ -228,8 +362,8 @@ BOARD_PID=""
if [ "${GADFLY_STATUS_BOARD:-1}" != "0" ]; then
rm -rf "$STATUS_DIR"; mkdir -p "$STATUS_DIR"
# Pre-seed every model as queued so the board shows the full swarm from t=0,
# even models still waiting on their provider lane's concurrency cap. Each
# binary overwrites its own file with real per-lens detail once it starts.
# even models whose lenses are still waiting on their provider's lens budget.
# Each binary overwrites its own file with real per-lens detail once it starts.
for m in "${MODEL_LIST[@]}"; do
jq -n --arg model "$m" --arg provider "$(provider_of "$m")" \
'{model:$model, provider:$provider, started:0, updated:0, done:false, lenses:[]}' \
@@ -241,10 +375,36 @@ if [ "${GADFLY_STATUS_BOARD:-1}" != "0" ]; then
log "status board started (pid ${BOARD_PID})"
fi
# --- swarm-wide hard backstop ----------------------------------------------
# A wall-clock ceiling across the WHOLE fleet, so a pathological PR can never
# drain the usage block however the models behave. entrypoint is the only
# process spanning every model, so a single "never exceed X" guard lives here.
# On expiry it stops the review subtrees (the binary + run.sh); whatever partial
# findings were gathered are still posted and the job never fails (advisory).
# GADFLY_PR_BUDGET_SECS=0 (default) disables it.
KILLER_PID=""
rm -f "${WORKDIR}/.budget_killed" "${WORKDIR}/.disarmed" 2>/dev/null || true
if [ "${GADFLY_PR_BUDGET_SECS:-0}" -gt 0 ] 2>/dev/null; then
(
sleep "${GADFLY_PR_BUDGET_SECS}"
log "PR wall-clock budget (${GADFLY_PR_BUDGET_SECS}s) reached — stopping the review fleet (advisory; partial findings still posted)"
: > "${WORKDIR}/.budget_killed"
pkill -TERM -f '/usr/local/bin/gadfly' 2>/dev/null || true
pkill -TERM -f "${SCRIPTS_DIR}/run.sh" 2>/dev/null || true
sleep 5
# Guard the delayed SIGKILL on the disarm marker: once the lanes finished and
# the watchdog was disarmed, the consolidation gadfly pass runs next, and a
# name-based KILL here must NOT catch it.
[ -f "${WORKDIR}/.disarmed" ] || pkill -KILL -f '/usr/local/bin/gadfly' 2>/dev/null || true
) &
KILLER_PID=$!
log "PR budget watchdog armed (${GADFLY_PR_BUDGET_SECS}s, pid ${KILLER_PID})"
fi
log "providers: ${PROVIDERS:-none}"
# Each provider lane runs in parallel; cap is enforced within each lane. Track
# the lane PIDs so we wait ONLY for the review work — not the status board,
# which intentionally runs until we signal it below.
# Each provider lane runs in parallel; the shared lens budget throttles within
# each lane. Track the lane PIDs so we wait ONLY for the review work — not the
# status board, which intentionally runs until we signal it below.
LANE_PIDS=()
for p in $PROVIDERS; do
run_lane "$p" &
@@ -252,9 +412,63 @@ for p in $PROVIDERS; do
done
[ "${#LANE_PIDS[@]}" -gt 0 ] && wait "${LANE_PIDS[@]}"
# Reviews finished (or the watchdog killed them): disarm the watchdog so its
# delayed SIGKILL can't catch the consolidation pass that runs next. Drop the
# disarm marker FIRST so even a racing watchdog that already reached its KILL line
# skips it (the kill below also tears the watchdog subshell down during its sleep).
if [ -n "$KILLER_PID" ]; then
: > "${WORKDIR}/.disarmed"
kill "$KILLER_PID" 2>/dev/null || true
fi
# If the backstop fired, note it on the consensus comment (per-model comments
# were already posted during the run; a killed model surfaces as a failed lane).
if [ -f "${WORKDIR}/.budget_killed" ]; then
export GADFLY_NOTICE="${GADFLY_NOTICE:+${GADFLY_NOTICE} }⏱️ This review was stopped early by the per-PR time budget (GADFLY_PR_BUDGET_SECS); findings are partial."
fi
# Reviews are done: signal the board to render the final state once and exit.
if [ -n "$BOARD_PID" ]; then
touch "${STATUS_DIR}/.done" 2>/dev/null || true
wait "$BOARD_PID" 2>/dev/null || true
fi
# --- cross-model consensus comment -----------------------------------------
# Render ONE consensus comment from the per-model findings the swarm wrote. This
# is advisory and best-effort: if the consolidation pass produces nothing, fall
# back to posting each model's review as its own comment (the per-model comments
# were suppressed during the run), so a consolidation hiccup never loses output.
if [ "$CONSOLIDATE" = "1" ]; then
n_files="$(ls -1 "${FINDINGS_DIR}"/*.json 2>/dev/null | wc -l | tr -d '[:space:]')"
log "consolidating findings from ${n_files} model(s)"
# Fetch the PR diff so the consolidator can also post an inline PR review,
# anchoring findings to changed lines (GADFLY_DIFF_FILE). GITEA_API/GITEA_TOKEN/
# GADFLY_PR are already in the binary's environment. Best-effort: an empty diff
# just means no inline review.
DIFF_FILE="${WORKDIR}/pr.diff"
API "${GITEA_API}/pulls/${PR}.diff" > "$DIFF_FILE" 2>/dev/null || true
CONSENSUS="$(GADFLY_CONSOLIDATE_DIR="$FINDINGS_DIR" GADFLY_DIFF_FILE="$DIFF_FILE" \
/usr/local/bin/gadfly 2>"${WORKDIR}/consolidate.err" || true)"
if [ -n "$CONSENSUS" ]; then
NOTICE_BLOCK=""
[ -n "${GADFLY_NOTICE:-}" ] && NOTICE_BLOCK="> ${GADFLY_NOTICE}"$'\n\n'
BODY="$(printf '%s%s\n\n<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>' "$NOTICE_BLOCK" "$CONSENSUS")"
upsert_comment_body "<!-- gadfly-consensus -->" "$BODY"
log "consensus comment posted"
else
log "consolidation produced no output; falling back to per-model comments"
log "$(tail -c 500 "${WORKDIR}/consolidate.err" 2>/dev/null)"
for f in "${FINDINGS_DIR}"/*.json; do
[ -f "$f" ] || continue
m="$(jq -r '.model // ""' "$f" 2>/dev/null)"
[ -z "$m" ] && continue
prov="$(jq -r '.provider // ""' "$f" 2>/dev/null)"
md="$(jq -r '.markdown // ""' "$f" 2>/dev/null)"
marker="<!-- gadfly-review:ollama:${m} -->"
body="$(printf '%s\n### 🪰 Gadfly review — `%s` (%s)\n\n%s\n\n<sub>Automated adversarial review by Gadfly. Advisory only — does not block merge.</sub>' \
"$marker" "$m" "$prov" "$md")"
upsert_comment_body "$marker" "$body"
done
fi
fi
log "done"
+1
View File
@@ -12,6 +12,7 @@ set the secrets/vars it references. Gadfly is advisory only — it never blocks
| [`openai-compatible.yml`](openai-compatible.yml) | any **OpenAI-compatible** endpoint (local Ollama `/v1`, gateway, vLLM, OpenRouter…) | `GADFLY_BASE_URL` (+ a key for most gateways) |
| [`endpoint-aliases.yml`](endpoint-aliases.yml) | **several named backends** at once (one comment each) | repo vars `GADFLY_ENDPOINT_<NAME>` |
| [`claude-code.yml`](claude-code.yml) | the bundled **Claude Code CLI** engine (`claude-code/<model>`) | secret `CLAUDE_CODE_OAUTH_TOKEN` (or `ANTHROPIC_API_KEY`) |
| [`opencode.yml`](opencode.yml) | the bundled **OpenCode CLI** engine (`opencode/<model>`) driving an ollama-cloud model — benchmark it against the majordomo loop on the same model | secret `OLLAMA_CLOUD_API_KEY` |
| [`.gadfly.yml`](.gadfly.yml) | **per-repo specialist config** (not a workflow — goes at your repo root) | — |
Common to all:
+7 -7
View File
@@ -55,14 +55,14 @@ jobs:
# csv to choose; "all" for everything; or define custom ones via a repo
# .gadfly.yml / GADFLY_SPECIALIST_<NAME>. See README "Specialists".
GADFLY_SPECIALISTS: ${{ vars.GADFLY_SPECIALISTS }}
# Lens fan-out (optional; default 1 = lenses run sequentially within a
# model). Raise it to run a model's lenses concurrently so each model
# posts its comment sooner. Total in-flight requests = (models at once)
# × (lenses at once), so to fan out without oversubscribing a backend,
# keep its model cap low and raise its lens cap. Per-provider configurable
# via GADFLY_PROVIDER_LENS_CONCURRENCY (same lanes as the model map):
# GADFLY_PROVIDER_CONCURRENCY: "ollama-cloud=1,m1=1"
# Concurrency (optional; default 1 = fully sequential per provider). The
# ONE throttle is a per-provider LENS BUDGET: the max lens passes (a lens =
# one specialist's review+recheck) in flight at once for a provider, shared
# across ALL that provider's models — every model in a lane runs at once and
# its lenses draw from the shared budget. Raise it to overlap lenses; set it
# per provider with GADFLY_PROVIDER_LENS_CONCURRENCY:
# GADFLY_PROVIDER_LENS_CONCURRENCY: "ollama-cloud=3,m1=1"
# (The old GADFLY_PROVIDER_CONCURRENCY model cap was removed and is ignored.)
# GADFLY_LENS_CONCURRENCY: ${{ vars.GADFLY_LENS_CONCURRENCY }}
# GADFLY_PROVIDER_LENS_CONCURRENCY: ${{ vars.GADFLY_PROVIDER_LENS_CONCURRENCY }}
# Live status board (optional; ON by default): one consolidated comment
+76
View File
@@ -0,0 +1,76 @@
# Gadfly reviewing via the OpenCode CLI engine.
# Copy to .gitea/workflows/adversarial-review.yml in your repo.
#
# Instead of gadfly's own majordomo loop, each lens shells out to the bundled
# `opencode` CLI (opencode.ai) inside the checked-out repo — it uses its own read
# tools to verify findings — while driving an ollama-cloud model. Gadfly then runs
# its usual verdict + recheck + consolidate pipeline.
#
# Why: benchmark gadfly's boutique harness against a freely-available one ON THE
# SAME MODEL. List both entries to get one comment section each and compare:
# GADFLY_MODELS: "ollama-cloud/glm-5.2,opencode/glm-5.2"
#
# Auth: reuses the OLLAMA_CLOUD_API_KEY secret (same as the ollama-cloud path) —
# no OpenCode-specific credential is needed for the ollama-cloud provider.
#
# Heads-up: this engine is newly wired and lightly tested — read the README's
# "OpenCode engine" note before relying on it.
name: Adversarial Review (Gadfly)
on:
pull_request:
types: [opened, reopened, ready_for_review]
issue_comment:
types: [created]
workflow_dispatch:
inputs:
pr_number: { description: "PR number to review", required: true }
permissions:
contents: read
issues: write
pull-requests: write
concurrency:
group: gadfly-${{ github.event.issue.number || github.event.pull_request.number || github.event.inputs.pr_number }}
cancel-in-progress: true
jobs:
review:
# Security: only trusted users may trigger a secret-bearing run via a PR
# comment. Replace the username(s) below with your maintainers — keep them in
# sync with GADFLY_ALLOWED_USERS (the in-container belt-and-suspenders check).
if: >-
github.event_name != 'issue_comment'
|| github.actor == 'your-username'
runs-on: ubuntu-latest
timeout-minutes: 30
steps:
- uses: docker://gitea.stevedudenhoeffer.com/steve/gadfly:latest
env:
GITEA_API: ${{ github.server_url }}/api/v1/repos/${{ github.repository }}
GITEA_TOKEN: ${{ secrets.GITEA_TOKEN }}
# --- OpenCode engine ---
# Reuses the ollama-cloud key; mapped to OLLAMA_API_KEY in-container and
# referenced by the generated provider as {env:OLLAMA_API_KEY}.
OLLAMA_CLOUD_API_KEY: ${{ secrets.OLLAMA_CLOUD_API_KEY }}
# "opencode/<model>" serves that model via ollama-cloud through OpenCode.
# Model ids are verbatim (colons preserved). List an "ollama-cloud/<model>"
# entry too to benchmark the two harnesses on the same model.
GADFLY_MODELS: "opencode/glm-5.2"
# Optional CLI tuning:
# GADFLY_OPENCODE_BASE_URL: "https://ollama.com/v1" # or a local Ollama /v1
# GADFLY_OPENCODE_MODEL: "glm-5.2" # overrides the spec suffix
# GADFLY_OPENCODE_EXTRA_ARGS: "--variant reasoning" # whitespace-split
# Escape hatch: "opencode/<provider>/<model>" passes straight to OpenCode's
# own provider registry/auth (e.g. opencode/anthropic/claude-sonnet-4-6).
GADFLY_ALLOWED_USERS: "your-username"
# --- event context (leave as-is) ---
EVENT_NAME: ${{ github.event_name }}
PR: ${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr_number }}
PR_BRANCH: ${{ github.head_ref }}
IS_DRAFT: ${{ github.event.pull_request.draft }}
COMMENT_BODY: ${{ github.event.comment.body }}
COMMENT_ID: ${{ github.event.comment.id }}
ACTOR: ${{ github.actor }}
+7 -6
View File
@@ -14,10 +14,11 @@
# Forward ONLY the secrets the reviewer uses (least privilege) — see the
# `secrets:` block below. GITEA_TOKEN is automatic. `secrets: inherit` also works
# but hands the reusable EVERY secret in your repo (registry/deploy/db creds the
# review never touches), so prefer the explicit form. Pin @<ref>: use the @v1
# release tag (a curated pointer moved on deliberate releases) for auto-updating
# stability, or a full @<sha> for an immutable pin. Avoid @main — it moves on
# every push and would change what runs with your forwarded secrets.
# review never touches), so prefer the explicit form. Pin to an immutable
# @<sha>: long-lived act_runners CACHE the reusable by ref, so a moved tag (@v1)
# or @main is often not re-fetched and silently runs a stale copy. Bump the @<sha>
# to adopt a structural change; routine swarm tuning rides owner variables (see
# the gadfly README "Central config via variables") with no re-pin needed.
#
# For custom named endpoints (GADFLY_ENDPOINT_<NAME>) or a provider the reusable
# doesn't map, use the full stub in adversarial-review.yml instead.
@@ -49,8 +50,8 @@ jobs:
if: >-
github.event_name != 'issue_comment'
|| (github.event.issue.pull_request && github.actor == 'your-username')
# @v1 = curated release tag (auto-updates on releases); swap for a full @<sha>
# if you want an immutable pin. Don't use @main (moves on every push).
# Pin to an immutable @<sha> (runners cache the ref, so @v1/@main can run
# stale). Bump it for structural changes; tune the swarm via owner variables.
uses: steve/gadfly/.gitea/workflows/review-reusable.yml@v1
# Forward ONLY what the reviewer needs. Add provider keys you use
# (ANTHROPIC_API_KEY, OPENAI_API_KEY, GOOGLE_API_KEY, GADFLY_API_KEY) and/or
+2
View File
@@ -3,6 +3,7 @@ module gitea.stevedudenhoeffer.com/steve/gadfly
go 1.26.2
require (
gitea.stevedudenhoeffer.com/steve/executus v0.1.4
gitea.stevedudenhoeffer.com/steve/majordomo v0.0.0-20260627225659-aa25b2c33462
gopkg.in/yaml.v3 v3.0.1
)
@@ -17,6 +18,7 @@ require (
github.com/go-logr/stdr v1.2.2 // indirect
github.com/google/go-cmp v0.7.0 // indirect
github.com/google/s2a-go v0.1.9 // indirect
github.com/google/uuid v1.6.0 // indirect
github.com/googleapis/enterprise-certificate-proxy v0.3.17 // indirect
github.com/googleapis/gax-go/v2 v2.22.0 // indirect
github.com/gorilla/websocket v1.5.3 // indirect
+2
View File
@@ -4,6 +4,8 @@ cloud.google.com/go/auth v0.20.0 h1:kXTssoVb4azsVDoUiF8KvxAqrsQcQtB53DcSgta74CA=
cloud.google.com/go/auth v0.20.0/go.mod h1:942/yi/itH1SsmpyrbnTMDgGfdy2BUqIKyd0cyYLc5Q=
cloud.google.com/go/compute/metadata v0.9.0 h1:pDUj4QMoPejqq20dK0Pg2N4yG9zIkYGdBtwLoEkH9Zs=
cloud.google.com/go/compute/metadata v0.9.0/go.mod h1:E0bWwX5wTnLPedCKqk3pJmVgCBSM6qQI1yTBdEb3C10=
gitea.stevedudenhoeffer.com/steve/executus v0.1.4 h1:4F99uCV3OVaE9ITFp0FjPiYxLUQO+WpE+wU2HCnpXNM=
gitea.stevedudenhoeffer.com/steve/executus v0.1.4/go.mod h1:WQP/lH+meU06OSNF0TQO/wQLcJCrMwpi0EMj5vSpVtk=
gitea.stevedudenhoeffer.com/steve/majordomo v0.0.0-20260627225659-aa25b2c33462 h1:1crjE1YkWHLZ91tUDOxN/Y5cuOnJ56e0U9UADoFfEPY=
gitea.stevedudenhoeffer.com/steve/majordomo v0.0.0-20260627225659-aa25b2c33462/go.mod h1:UZLveG17SmENt4sne2RSLIbioix30RZbRIQUzBAnOyY=
github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs=
+71
View File
@@ -0,0 +1,71 @@
#!/usr/bin/env bash
# Credential pre-flight for the agentic reviewer, in ONE definition.
#
# Sourced by run.sh (production) and by preflight_test.sh (the table test), so
# the tested bytes and the running bytes are the same. Keep it that way: a test
# that reimplements this logic can agree with a stale copy of it.
#
# Why pre-flight at all, when majordomo already fails closed with a 401:
# without it a missing key surfaces as five identical per-lens agent failures
# that name no variable, and the operator reads a stack trace to learn which
# secret they forgot to forward.
# gadfly_preflight_key <provider> -> echoes "" when the run may proceed, or the
# name of the environment variable the operator must set.
#
# Scope: the REGISTRY path only — GADFLY_BASE_URL unset — and deliberately so.
# The two resolution paths have DIFFERENT credential rules: with an explicit
# endpoint the credential is GADFLY_API_KEY (falling back to the client's own
# default, OPENAI_API_KEY for the openai family) and a built-in's own variable
# is never consulted; without one, the reverse. Applying either path's rule to
# the other yields a check that passes a run which then 401s — the precise
# failure this exists to prevent. So it covers the path whose rules it can state
# exactly and stays silent on the other. That is also the useful half: an
# override-path config is hand-written, while the registry path is what somebody
# hits by adding a model id to a var and forgetting the secret.
gadfly_preflight_key() {
local provider="$1" key_env="" key_hint=""
# Only the registry path has knowable credential rules — see above.
if [ -n "${GADFLY_BASE_URL:-}" ]; then
echo ""
return 0
fi
# A provider is absent from this table for one of TWO different reasons — do
# not assume the first one and add an arm:
# 1. It needs no key, or carries it in its endpoint/DSN: local ollama,
# llama-swap, foreman.
# 2. It needs a key but accepts more than one variable, so a single-name
# check would skip a correctly-configured run. **google** is this case:
# GOOGLE_API_KEY *or* GEMINI_API_KEY. Adding
# `google) key_env="GOOGLE_API_KEY"` would silently skip every reviewer
# configured with GEMINI_API_KEY. Pre-flighting google needs an
# either-variable check, not this table's one-name shape.
# ollama-cloud is checked on OLLAMA_API_KEY but hinted as OLLAMA_CLOUD_API_KEY:
# run.sh copies the consumer-facing OLLAMA_CLOUD_API_KEY secret into the
# OLLAMA_API_KEY the provider reads, BEFORE calling this. The hint names the
# variable the operator actually sets; the check reads the one the code uses.
# If that copy ever moves after this call, this arm reports a missing key for
# a configured run.
case "$provider" in
ollama-cloud) key_env="OLLAMA_API_KEY"; key_hint="OLLAMA_CLOUD_API_KEY" ;;
qwen) key_env="QWEN_API_KEY"; key_hint="QWEN_API_KEY" ;;
kimi) key_env="KIMI_API_KEY"; key_hint="KIMI_API_KEY" ;;
openai|openai-compatible) key_env="OPENAI_API_KEY"; key_hint="OPENAI_API_KEY" ;;
anthropic) key_env="ANTHROPIC_API_KEY"; key_hint="ANTHROPIC_API_KEY" ;;
esac
if [ -z "$key_env" ]; then
echo "" # provider needs no pre-flight
return 0
fi
# Indirect expansion (bash). Each majordomo built-in reads ONLY its own
# variable — cross-provider fallback is refused by design — so the named hint
# is always the actual fix.
if [ -n "${!key_env:-}" ]; then
echo ""
return 0
fi
echo "$key_hint"
}
+86
View File
@@ -0,0 +1,86 @@
#!/usr/bin/env bash
# Table test for the credential pre-flight in preflight.sh.
#
# It SOURCES the real implementation rather than copying it, so there is no
# second definition that can pass while production fails.
#
# Run: scripts/preflight_test.sh (exit 0 = all cases pass)
set -u
SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"
# shellcheck source=preflight.sh
. "$SCRIPT_DIR/preflight.sh"
fail=0
check() { # description, want, got
if [ "$2" = "$3" ]; then
echo "ok $1"
else
echo "FAIL $1 — want '$2', got '$3'"
fail=1
fi
}
# probe <provider> [VAR=VAL ...] — run the real function in a clean environment
# under the same shell options production uses (set -u), so an unset-variable
# bug surfaces here instead of in a live review.
probe() {
local provider="$1"; shift
env -i PATH="$PATH" HOME="$HOME" "$@" bash -c "
set -u
. '$SCRIPT_DIR/preflight.sh'
gadfly_preflight_key '$provider'
"
}
echo "== registry path: keyed providers with no key must name their variable =="
check "qwen, no key" "QWEN_API_KEY" "$(probe qwen)"
check "kimi, no key" "KIMI_API_KEY" "$(probe kimi)"
check "ollama-cloud, no key" "OLLAMA_CLOUD_API_KEY" "$(probe ollama-cloud)"
check "openai, no key" "OPENAI_API_KEY" "$(probe openai)"
check "openai-compatible, none" "OPENAI_API_KEY" "$(probe openai-compatible)"
check "anthropic, no key" "ANTHROPIC_API_KEY" "$(probe anthropic)"
echo "== registry path: the provider's own key lets it run =="
check "qwen, keyed" "" "$(probe qwen QWEN_API_KEY=k)"
check "kimi, keyed" "" "$(probe kimi KIMI_API_KEY=k)"
check "ollama-cloud, keyed" "" "$(probe ollama-cloud OLLAMA_API_KEY=k)"
check "openai-compatible, keyed" "" "$(probe openai-compatible OPENAI_API_KEY=k)"
echo "== a wrong-provider key never satisfies a provider (no cross-fallback) =="
check "qwen w/ only OPENAI key" "QWEN_API_KEY" "$(probe qwen OPENAI_API_KEY=k)"
check "kimi w/ only QWEN key" "KIMI_API_KEY" "$(probe kimi QWEN_API_KEY=k)"
echo "== an empty-string key counts as missing, not present =="
check "qwen, empty key" "QWEN_API_KEY" "$(probe qwen QWEN_API_KEY=)"
echo "== GADFLY_API_KEY does NOT substitute on the registry path =="
# resolveModel reads GADFLY_API_KEY only after its `baseURL == ""` early
# return, so on this path the built-in reads its own variable and a set
# GADFLY_API_KEY changes nothing. Treating it as sufficient was a false pass.
check "qwen w/ GADFLY_API_KEY only" "QWEN_API_KEY" "$(probe qwen GADFLY_API_KEY=k)"
echo "== override path (GADFLY_BASE_URL set) is deliberately not pre-flighted =="
# The credential there is GADFLY_API_KEY with a client-specific fallback, and
# the built-ins' own variables are never read. Checking one path's rules
# against the other produced a false pass in BOTH directions, so this path is
# left alone rather than guessed at.
check "qwen + BASE_URL, no keys" "" "$(probe qwen GADFLY_BASE_URL=https://x)"
check "qwen + BASE_URL + own key" "" "$(probe qwen GADFLY_BASE_URL=https://x QWEN_API_KEY=k)"
check "qwen + BASE_URL + GADFLY key" "" "$(probe qwen GADFLY_BASE_URL=https://x GADFLY_API_KEY=k)"
check "openai + BASE_URL, no keys" "" "$(probe openai GADFLY_BASE_URL=https://x)"
echo "== providers needing no key are never blocked, with nothing set =="
for p in ollama llama-swap llama-swaps llamaswap llamaswaps foreman google gemini some-dsn-name; do
check "unkeyed $p" "" "$(probe "$p")"
done
# google is absent from the table on purpose: it accepts GOOGLE_API_KEY *or*
# GEMINI_API_KEY, so a one-name arm would skip a correctly-configured run.
check "google w/ only GEMINI_API_KEY" "" "$(probe google GEMINI_API_KEY=k)"
if [ "$fail" -ne 0 ]; then
echo "RESULT: preflight table FAILED"
exit 1
fi
echo "RESULT: all pre-flight cases pass"
+53 -10
View File
@@ -29,6 +29,13 @@
# tuning are read straight from the inherited environment — same as the other
# provider keys (OPENAI_API_KEY, …) — so no extra wiring is needed here.
#
# opencode engine: when MODEL is "opencode" or "opencode/<model>" the binary
# shells out to the bundled `opencode` CLI, driving an ollama-cloud model (for
# benchmarking against the majordomo path on the same model). Its auth reuses
# OLLAMA_CLOUD_API_KEY (mapped to OLLAMA_API_KEY below, same as the ollama-cloud
# path) and GADFLY_OPENCODE_* tuning is read from the inherited environment — so
# no extra wiring is needed here either.
#
# Optional:
# MAX_DIFF_CHARS diff truncation cap for the prompt (default 60000)
# GADFLY_STATUS_FILE per-model JSON path for the live status board (set by
@@ -41,6 +48,11 @@ set -uo pipefail
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
MAX_DIFF_CHARS="${MAX_DIFF_CHARS:-60000}"
# Credential pre-flight, shared verbatim with scripts/preflight_test.sh so the
# tested logic and the running logic are the same bytes.
# shellcheck source=preflight.sh
. "$SCRIPT_DIR/preflight.sh"
: "${GITEA_API:?GITEA_API required}"
: "${GITEA_TOKEN:?GITEA_TOKEN required}"
: "${PR:?PR required}"
@@ -50,6 +62,13 @@ MAX_DIFF_CHARS="${MAX_DIFF_CHARS:-60000}"
MARKER="<!-- gadfly-review:${PROVIDER}:${MODEL} -->"
say() { echo "[gadfly-review:${PROVIDER}:${MODEL}] $*" >&2; }
# When the swarm is consolidating (GADFLY_CONSOLIDATE=1, set by entrypoint.sh for
# a multi-model run), this model does NOT post its own comment — it writes its
# findings to GADFLY_FINDINGS_OUT and a single cross-model consensus comment is
# posted after the whole swarm finishes. Live progress still shows on the status
# board. Default 0 (post a per-model comment, the standalone behavior).
CONSOLIDATE="${GADFLY_CONSOLIDATE:-0}"
# Display the model's ACTUAL backend: the provider segment of the spec
# ("m1pro/qwen3.6:35b-mlx" -> "m1pro"); a bare id uses GADFLY_PROVIDER (default
# ollama-cloud). This is what the comment header shows, not the run.sh lane.
@@ -126,7 +145,9 @@ USR="$(printf 'PR #%s: %s\n\nDescription:\n%s\n\nUnified diff to review:\n```dif
# --- announce start (placeholder comment) -----------------------------------
START_TS="$(date +%s)"
say "starting review with ${MODEL}"
upsert_comment "$(printf '%s\n### 🪰 Gadfly review — `%s` (%s)\n\n⏳ Reviewing… this comment will update with findings and run time.' \
# Skip the per-model placeholder when consolidating (the consensus comment is
# posted later; live progress is on the status board).
[ "$CONSOLIDATE" = "1" ] || upsert_comment "$(printf '%s\n### 🪰 Gadfly review — `%s` (%s)\n\n⏳ Reviewing… this comment will update with findings and run time.' \
"$MARKER" "$MODEL" "$MODEL_PROVIDER")"
# --- call the model ---------------------------------------------------------
@@ -146,10 +167,10 @@ case "$PROVIDER" in
fi
GADFLY_PROVIDER_EFF="$MODEL_PROVIDER"
# Only the default cloud provider strictly needs a key up front; local Ollama
# and other providers either need none or read their own standard env var.
if [ "$GADFLY_PROVIDER_EFF" = "ollama-cloud" ] && [ -z "${OLLAMA_API_KEY:-}" ] && [ -z "${GADFLY_API_KEY:-}" ]; then
REVIEW="⚠️ No Ollama Cloud key configured (set \`OLLAMA_CLOUD_API_KEY\`) and \`GADFLY_PROVIDER\` is the default \`ollama-cloud\`; this reviewer was skipped."
# Credential pre-flight — one definition, shared with preflight_test.sh.
MISSING_KEY="$(gadfly_preflight_key "$GADFLY_PROVIDER_EFF")"
if [ -n "$MISSING_KEY" ]; then
REVIEW="⚠️ No API key configured for provider \`${GADFLY_PROVIDER_EFF}\` (set \`${MISSING_KEY}\`); this reviewer was skipped."
else
BIN="${GADFLY_BIN:-gadfly}"
if ! command -v "$BIN" >/dev/null 2>&1 && [ ! -x "$BIN" ]; then
@@ -168,8 +189,9 @@ case "$PROVIDER" in
GADFLY_SYSTEM_FILE="${SCRIPT_DIR}/system-prompt.txt" \
GADFLY_TITLE="$TITLE" \
GADFLY_BODY="$BODY" \
GADFLY_MAX_DIFF_CHARS="$MAX_DIFF_CHARS" \
GADFLY_MAX_DIFF_CHARS="${GADFLY_MAX_DIFF_CHARS:-$MAX_DIFF_CHARS}" \
GADFLY_STATUS_FILE="${GADFLY_STATUS_FILE:-}" \
GADFLY_FINDINGS_OUT="${GADFLY_FINDINGS_OUT:-}" \
"$BIN" 2>"$ERR_FILE"
)"
rc=$?
@@ -204,7 +226,28 @@ esac
# --- assemble + post final comment (with run time) --------------------------
ELAPSED="$(( $(date +%s) - START_TS ))"
DUR="$(fmt_duration "$ELAPSED")"
COMMENT="$(printf '%s\n### 🪰 Gadfly review — `%s` (%s)\n\n%s\n\n<sub>Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in %s</sub>' \
"$MARKER" "$MODEL" "$MODEL_PROVIDER" "$REVIEW" "$DUR")"
upsert_comment "$COMMENT"
say "done in ${DUR}"
# Consolidating: the binary writes its findings file on success. If it failed or
# was skipped (no file, or an empty one), write a stub so this model still shows
# up in the consensus (as failed) and an all-models-fail run still posts a
# comment — never silently drop a model or the whole review.
if [ "$CONSOLIDATE" = "1" ] && [ -n "${GADFLY_FINDINGS_OUT:-}" ] && [ ! -s "${GADFLY_FINDINGS_OUT}" ]; then
jq -n --arg model "$MODEL" --arg provider "$MODEL_PROVIDER" --arg md "$REVIEW" \
'{model:$model, provider:$provider, verdict:"reviewer failed", errored:true, markdown:$md, findings:[]}' \
> "${GADFLY_FINDINGS_OUT}" 2>/dev/null || true
fi
# When consolidating, the binary has written this model's findings to
# GADFLY_FINDINGS_OUT; the consensus comment is posted by entrypoint.sh after the
# whole swarm finishes, so this model posts no comment of its own.
if [ "$CONSOLIDATE" = "1" ]; then
say "done in ${DUR} (consolidated; no per-model comment)"
else
# An optional one-line notice (e.g. entrypoint's huge-PR downshift advisory),
# shown under the header so a shallower review is self-explaining.
NOTICE_BLOCK=""
[ -n "${GADFLY_NOTICE:-}" ] && NOTICE_BLOCK="> ${GADFLY_NOTICE}"$'\n\n'
COMMENT="$(printf '%s\n### 🪰 Gadfly review — `%s` (%s)\n\n%s%s\n\n<sub>Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in %s</sub>' \
"$MARKER" "$MODEL" "$MODEL_PROVIDER" "$NOTICE_BLOCK" "$REVIEW" "$DUR")"
upsert_comment "$COMMENT"
say "done in ${DUR}"
fi
+21 -1
View File
@@ -16,7 +16,9 @@ state. USE THEM to verify before you report. Do not review the diff in isolation
- list_dir([path]) — list a directory.
- grep(pattern[, path, max_results]) — RE2 regex search across the repo.
- find_files(name[, max_results]) — locate a file by path substring.
- get_diff() — the full unified diff (the task message may truncate it).
- get_diff([path, start_line, limit]) — the unified diff as a paginated, numbered window;
pass `path` to fetch just one changed file's hunks (do this on a big PR instead of pulling
the whole diff at once).
Mandatory verification discipline — this is the whole point of giving you tools:
- Before claiming a missing/duplicate import, an undefined symbol, a wrong signature,
@@ -43,3 +45,21 @@ Output rules:
- Only report issues you are reasonably confident are real after checking. If the diff
is clean, say so plainly rather than inventing nits.
- When you are done investigating, STOP calling tools and reply with the final review.
Machine-readable findings — AFTER the prose review, append ONE fenced code block,
tagged `gadfly-findings`, holding a JSON array of the SAME findings you described above
(this block is consumed by tooling and hidden from the rendered comment):
```gadfly-findings
[
{"file": "path/to/file.go", "line": 123, "severity": "high", "confidence": "high", "title": "one-line summary of the issue"}
]
```
- One object per real finding, in the same order as your prose. `file`/`line` must be a
concrete location you verified (the line the issue is at). `severity` is one of
`critical`, `high`, `medium`, `small`, `trivial`. `confidence` is your post-verification
confidence the issue is real: one of `high`, `medium`, `low`.
- Include ONLY genuine problems — never verification notes ("confirmed X is safe at f:line"),
and never an "Outside my lens:" aside. If your lens is clean, emit an empty array `[]`.
- This block is in ADDITION to the prose; do not drop the human-readable findings.