Compare commits
12
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
14f8533e38 | ||
|
|
ebfaeba07e | ||
|
|
67a73616e1 | ||
|
|
1d6eaa08c5 | ||
|
|
2367e696b5 | ||
|
|
0f40b21d79 | ||
|
|
c9dab69d14 | ||
|
|
b37cd09dc9 | ||
|
|
6a74b64c7a | ||
|
|
74831368ab | ||
|
|
0d51879450 | ||
|
|
bb98fae5f0 |
@@ -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:
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -44,7 +44,7 @@ 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_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.
|
||||
@@ -53,8 +53,8 @@ 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
|
||||
@@ -82,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 }
|
||||
@@ -99,11 +108,12 @@ jobs:
|
||||
runs-on: ubuntu-latest
|
||||
timeout-minutes: ${{ inputs.timeout_minutes }}
|
||||
steps:
|
||||
# sha-3095ebf predated the Gitea >= 1.27 workflow_call reclassification
|
||||
# (entrypoint.sh, added in 9d74cb9) — so the dogfood self-review skipped every
|
||||
# PR with "event 'workflow_call' not handled". Bumped to sha-f468fe6 (current
|
||||
# main HEAD) which contains that fix. Bump per Gadfly release.
|
||||
- uses: docker://gitea.stevedudenhoeffer.com/steve/gadfly:sha-f468fe6
|
||||
# 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 }}
|
||||
@@ -126,6 +136,12 @@ jobs:
|
||||
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
|
||||
@@ -151,7 +167,9 @@ 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 }}
|
||||
|
||||
@@ -57,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>
|
||||
@@ -154,10 +154,19 @@ 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
|
||||
|
||||
@@ -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`)
|
||||
@@ -272,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
|
||||
|
||||
@@ -418,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
|
||||
@@ -441,10 +460,9 @@ The reviewer binary reads these (the stub/entrypoint set sane defaults):
|
||||
| `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 |
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
}
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
+55
-31
@@ -49,15 +49,18 @@
|
||||
// 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.
|
||||
// "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).
|
||||
//
|
||||
@@ -94,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
|
||||
)
|
||||
|
||||
@@ -242,25 +247,34 @@ func run() error {
|
||||
|
||||
// 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: up to GADFLY_LENS_CONCURRENCY lenses run concurrently (the
|
||||
// default of 1 keeps the suite sequential, exactly as before), and fanout.Run
|
||||
// returns one result per lens in input order. Each lens already runs under its
|
||||
// own per-lens timeout (reviewWithSpecialist) and the lenses only read the
|
||||
// immutable repoFS, so concurrency simply overlaps independent passes.
|
||||
// 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 {
|
||||
// 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: lensConcurrency(),
|
||||
}, func(_ context.Context, sp Specialist) (res specialistResult, _ error) {
|
||||
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:
|
||||
@@ -271,6 +285,17 @@ func runSpecialists(eng reviewEngine, base string, specialists []Specialist, tas
|
||||
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)
|
||||
@@ -293,14 +318,13 @@ func runSpecialists(eng reviewEngine, base string, specialists []Specialist, tas
|
||||
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
|
||||
@@ -310,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, "=")
|
||||
|
||||
+64
-18
@@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -162,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 {
|
||||
@@ -188,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>"
|
||||
@@ -240,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)}
|
||||
@@ -258,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 != "" {
|
||||
@@ -277,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)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
+53
-29
@@ -33,9 +33,17 @@
|
||||
# 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.
|
||||
@@ -198,14 +206,18 @@ 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
|
||||
@@ -245,24 +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="" ff=""
|
||||
[ "${GADFLY_STATUS_BOARD:-1}" != "0" ] && sf="$(status_file_for "$1")"
|
||||
[ "$CONSOLIDATE" = "1" ] && ff="$(findings_file_for "$1")"
|
||||
PROVIDER=ollama MODEL="$1" GADFLY_BIN="/usr/local/bin/gadfly" GADFLY_REPO_DIR="$REPO_DIR" \
|
||||
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" \
|
||||
bash "${SCRIPTS_DIR}/run.sh" || log "model $1 failed (continuing)"
|
||||
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
|
||||
@@ -313,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
|
||||
}
|
||||
@@ -338,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:[]}' \
|
||||
@@ -378,9 +402,9 @@ if [ "${GADFLY_PR_BUDGET_SECS:-0}" -gt 0 ] 2>/dev/null; then
|
||||
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" &
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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"
|
||||
}
|
||||
@@ -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"
|
||||
+9
-4
@@ -48,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}"
|
||||
@@ -162,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
|
||||
|
||||
Reference in New Issue
Block a user