Files
gadfly/cmd/gadfly/main.go
T
steveandClaude Opus 4.8 6a74b64c7a
Build & push image / build-and-push (pull_request) Successful in 4s
fix(concurrency): address gadfly review — fail-open pool + doc drift
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

443 lines
20 KiB
Go
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
// Command gadfly is the agentic backend for the PR adversarial-review
// workflow (.gitea/workflows/pr-adversarial-review.yml). Unlike the old
// one-shot chat call, it runs a tool-using agent (majordomo + Ollama Cloud)
// over the PR's CHECKED-OUT repository: the model can read_file / list_dir /
// grep / find_files / get_diff to VERIFY a finding before reporting it, which
// kills the "diff-only" false positives (claiming a missing import or a
// non-existent method it simply couldn't see).
//
// It is a pure producer of review text: it reads the diff + the repo and
// prints the review markdown to stdout. All Gitea I/O (fetching the diff,
// upserting the comment) stays in run.sh, so this binary needs no repo write
// access and is straightforward to unit-test.
//
// Two passes (unless the draft is a clean "no material issues" pass): a
// REVIEW pass produces a draft, then an adversarial RECHECK pass independently
// re-verifies every finding against the actual files with the same tools and
// drops the ones it cannot confirm, recomputing the verdict. This catches the
// "confident but wrong" findings that survive a single pass — e.g. claiming an
// env var is unset when a wrapper script sets it (see recheck.go).
//
// Inputs (env):
//
// GADFLY_MODEL model id, or a full "provider/model" spec / majordomo
// alias / failover chain (required). A bare id is
// 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
// servers, remote Ollama, gateways). See model.go.
// GADFLY_API_KEY provider key; optional — falls back to the provider's
// standard env (OLLAMA_API_KEY / OPENAI_API_KEY /
// ANTHROPIC_API_KEY / GOOGLE_API_KEY|GEMINI_API_KEY).
// GADFLY_SPECIALISTS csv of review lenses, "all", or "auto" (see specialists.go).
// GADFLY_SELECTOR_MODEL model that picks lenses in "auto" mode (default: review model).
// GADFLY_WORKER_MODEL cheap model for the delegate_investigation tool (unset = off).
// GADFLY_REPO_DIR path to the checked-out repo (required; the FS sandbox root).
// GADFLY_DIFF_FILE path to a file holding the full unified diff (required).
// GADFLY_SYSTEM_FILE path to the reviewer system prompt (required).
// 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 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 (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 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
// posts a "reviewer failed" notice (advisory — never fails the CI job).
package main
import (
"context"
"errors"
"fmt"
"os"
"strconv"
"strings"
"time"
"gitea.stevedudenhoeffer.com/steve/executus/fanout"
)
const (
defaultMaxSteps = 24
// defaultTimeoutSecs is the deadline for EACH specialist's passes (review +
// recheck). It is per-lens, not shared across the suite, so one slow lens
// (e.g. a big local model) can't starve the others. Slow local models may
// need this raised (and a higher job timeout to match the suite total).
defaultTimeoutSecs = 300
defaultMaxDiffChars = 60000
// autoSelectTimeout bounds the dynamic specialist-selection call.
autoSelectTimeout = 120 * time.Second
// defaultWrapUpReserve is how many steps before the cap the agent is told
// to stop investigating and write its final answer. Reserving a margin is
// what keeps a thorough reviewer from spending its whole budget on tool
// 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 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
)
// wrapUpInstruction is steered into a running agent once it comes within the
// wrap-up reserve of its step cap: a forceful nudge to stop calling tools and
// emit the final answer using only what it has already gathered.
const wrapUpInstruction = "⚠️ You are almost out of your investigation budget — only a few tool steps remain. " +
"STOP calling tools now and write your FINAL answer immediately, using only what you have already verified. " +
"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."
func main() {
if err := run(); err != nil {
fmt.Fprintln(os.Stderr, "gadfly:", err)
os.Exit(1)
}
}
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")
if repoDir == "" || diffFile == "" || systemFile == "" {
return errors.New("GADFLY_REPO_DIR, GADFLY_DIFF_FILE and GADFLY_SYSTEM_FILE are all required")
}
diffBytes, err := os.ReadFile(diffFile)
if err != nil {
return fmt.Errorf("read diff file: %w", err)
}
diff := string(diffBytes)
if strings.TrimSpace(diff) == "" {
return errors.New("empty diff; nothing to review")
}
systemBytes, err := os.ReadFile(systemFile)
if err != nil {
return fmt.Errorf("read system prompt: %w", err)
}
fsTools, err := newRepoFS(repoDir, diff)
if err != nil {
return err
}
// 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
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)
}
// Optional cheap worker for delegate_investigation. Non-fatal: a bad
// worker spec just disables delegation rather than sinking the review.
if worker, werr := resolveWorkerModel(); werr != nil {
fmt.Fprintln(os.Stderr, "gadfly: worker model disabled:", werr)
} else if worker != nil {
fsTools.worker = worker
}
// 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)
for _, e := range serrs {
fmt.Fprintln(os.Stderr, "gadfly:", e)
}
// Dynamic selection: a (cheap) model picks the lenses this diff needs.
// 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 {
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(md.mdl)
if serr != nil {
return fmt.Errorf("resolve selector model: %w", serr)
}
selCtx, cancel := context.WithTimeout(context.Background(), autoSelectTimeout)
picked, aerr := autoSelectSpecialists(selCtx, selector, os.Getenv("GADFLY_TITLE"), os.Getenv("GADFLY_BODY"), diff, registry)
cancel()
if aerr != nil {
fmt.Fprintln(os.Stderr, "gadfly: auto-select failed; falling back to the default suite:", aerr)
specialists = suiteFromRegistry(registry, defaultSuite)
} else {
specialists = picked
fmt.Fprintln(os.Stderr, "gadfly: auto-selected specialists:", specialistNamesOf(specialists))
}
}
}
if len(specialists) == 0 {
return errors.New("no specialists resolved to run")
}
base := string(systemBytes)
task := buildTask(diff)
results := runSpecialists(eng, base, specialists, task, diff)
fmt.Println(renderConsolidated(results))
// 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. 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.
//
// 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 {
// 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)
// 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
}
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
}
return results
}
// 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
}
return envInt("GADFLY_LENS_CONCURRENCY", defaultLensConcurrency)
}
// 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_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, "=")
if !ok || strings.TrimSpace(k) != provider {
continue
}
if n, err := strconv.Atoi(strings.TrimSpace(v)); err == nil && n > 0 {
return n, true
}
}
return 0, false
}
// reviewWithSpecialist runs one lens end-to-end under its OWN timeout, so a slow
// model on one lens can't starve the others: a review pass under the
// specialist's composed prompt, then the shared adversarial recheck pass. The
// 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) {
ctx, cancel := context.WithTimeout(context.Background(), reviewTimeout())
defer cancel()
draft, err := eng.runPass(ctx, composeSpecialistPrompt(base, sp), task,
envInt("GADFLY_MAX_STEPS", defaultMaxSteps))
if err != nil {
fmt.Fprintf(os.Stderr, "gadfly: specialist %q review pass failed: %v\n", sp.Name, err)
return fmt.Sprintf("⚠️ This reviewer failed to complete: %v", err), true
}
final := draft
if shouldRecheck(draft) {
rechecked, rerr := eng.runPass(ctx, recheckSystemPrompt, buildRecheckTask(draft, diff),
envInt("GADFLY_RECHECK_MAX_STEPS", defaultRecheckMaxSteps))
if rerr != nil {
fmt.Fprintf(os.Stderr, "gadfly: specialist %q recheck failed; emitting unverified draft: %v\n", sp.Name, rerr)
} else {
final = rechecked
}
}
return final, false
}
// wrapUpReserve is how many steps before the cap the wrap-up nudge fires,
// overridable via GADFLY_WRAPUP_RESERVE.
func wrapUpReserve() int {
return envInt("GADFLY_WRAPUP_RESERVE", defaultWrapUpReserve)
}
// buildTask assembles the user message: PR metadata plus the unified diff,
// truncated for the prompt (the full diff stays available via get_diff).
func buildTask(diff string) string {
title := os.Getenv("GADFLY_TITLE")
body := os.Getenv("GADFLY_BODY")
maxDiff := envInt("GADFLY_MAX_DIFF_CHARS", defaultMaxDiffChars)
truncNote := ""
if maxDiff > 0 && len(diff) > maxDiff {
diff = diff[: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
if title != "" {
fmt.Fprintf(&b, "PR title: %s\n\n", title)
}
if strings.TrimSpace(body) != "" {
fmt.Fprintf(&b, "PR description:\n%s\n\n", body)
}
b.WriteString("Review the following unified diff. Before reporting any cross-file or compile-correctness issue, use your repository read tools to verify it against the actual checked-out code — do not rely on the diff alone.\n\n")
fmt.Fprintf(&b, "```diff\n%s\n```%s", diff, truncNote)
return b.String()
}
// envInt reads an integer env var, falling back to def when unset or unparseable.
func envInt(name string, def int) int {
v := strings.TrimSpace(os.Getenv(name))
if v == "" {
return def
}
n, err := strconv.Atoi(v)
if err != nil || n <= 0 {
return def
}
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
}