Review fix round (4 Important + 2 folded):
1. Version display: OverviewView.vue was missing the muted version line
the old vanilla-JS page rendered (types/api.ts already declared
overview.version) - added.
2. CSP invariant now guarded twice: webui/generate.mjs's new
assertNoInlineAssets fails the build if dist/index.html contains an
inline <script> (no src= attribute) or any <style> tag - the exact
condition the tightened admin CSP (script-src/style-src 'self', no
'unsafe-inline') depends on. admin_test.go's new
TestAdminIndexHTML_NoInlineAssets re-asserts the identical invariant
server-side, against the decoded adminIndexHTML admin.go actually
serves, so a regression is caught even if admin_assets_gen.go were
regenerated with a modified copy of generate.mjs.
3. webui/components.json's iconLibrary:"lucide"/font:"inter" fields
cannot be fixed in place - checked shadcn-vue's own schema
(PRESET_ICON_LIBRARIES/PRESET_FONTS in the installed package): no
"none"/fontawesome/system choice exists for either, and JSON (unlike
the tsconfig.*.json files touched in the previous commit) has no
comment syntax to warn inline. Documented instead: webui/README.md
gained a loud warning section naming the exact regression (already
hit twice this branch - a re-added Google Fonts import, a re-added
lucide icon import) and the two-step fix to apply after any future
shadcn-vue add/init call.
4. Real-CSP verification: the browser pass earlier in this branch ran
under vite preview, which sends no CSP header at all. New
integration/integration_test.go: TestAdminAssetCSP fetches GET
/admin against the real Traefik+Yaegi pipeline, extracts the asset
URL from its <script src="..."> tag, fetches that asset, and asserts
both responses carry the exact adminCSP string byte-for-byte.
admin.go's serveAdminAsset now also sets Content-Security-Policy
(previously JSON/page routes only) so this holds for a direct
navigation to an asset URL, not just as a same-origin sub-resource of
GET /admin; admin_test.go's TestAdminAsset_Serving updated to match.
5. (folded) UsageChart.vue stacked the "tokens" tab's two series
(tokin/tokout - two separate HTTP requests, not one atomic call) by
positional index. A window rollover landing between the two requests
could leave one series one bucket ahead, mis-stacking that refresh.
Now aligns both series by their bucket key (a sorted union of every
active series' buckets - the fixed-width, zero-padded bucket strings
sort chronologically by construction), with a missing bucket in one
series reading as 0 rather than shifting the whole series.
6. (folded) AuthGate.vue now triggers an immediate dashboard.refresh()
on successful key submit, instead of waiting up to 5s for the next
poll tick - the dashboard no longer sits blank right after auth.
Rebuilt via make admin-ui; admin_assets_gen.go regenerated.
README.md: ## Admin rewritten for the Vue rebuild (webui/, new
GET /admin/assets/{hashedname} route, tightened CSP with no more
'unsafe-inline', usage-charts description) and the stats-exclusion
directive ("Admin traffic is never counted" replaces "Poll counts
against quota"). ## Development gains `make admin-ui` plus a note
that Node is needed only for that one target.
docs/superpowers/specs/2026-08-20-llmgateway-v02-design.md §4: an
amendment block documents the Vue rewrite (new route, tightened CSP,
Charts view) following this doc's established
controller-approved-amendment convention; the Accounting bullet is
amended for the stats-exclusion operator directive, with the original
wording struck through rather than deleted.
Operator directive (progress ledger, 2026-08-21): admin requests must
not touch req/min-req/day statistics. handleAdminAPI no longer calls
checkAndCount for GET /admin/api/overview or GET /admin/api/usage (it
already skipped it for GET /admin/api/usage/history) - admin traffic
must never appear in usage statistics, which measure real LLM traffic
only. identify()'s group return is now discarded (u, _, ok :=), since
buildLimitScopes is no longer built here.
Direct consequence, documented in the new doc comment: an admin's own
requestsPerMinute/requestsPerDay limit, if configured, is no longer
enforced against admin-route traffic either - accepted trade-off
(admin-gated, cheap reads; a self-inflicted, not shared, resource
cost).
admin_test.go: TestAdmin_RateLimitApplies (asserted a 2nd request
429s) inverted to TestAdmin_NeverRateLimitedAndCountersUntouched - 5x
requests to both overview and usage against an admin key pinned at
RequestsPerMinute:1, asserting every one is 200 and the admin's own
req/min counter stays exactly 0.
integration/integration_test.go: TestAdmin_Dashboard's admin-user
assertion inverted from requestsPerDay > 0 ("admin API calls count
too") to requestsPerDay == 0 ("admin routes never call
checkAndCount") - the admin key is used throughout the whole suite
exclusively to hit /admin/api/*, so it must show exactly 0 despite
issuing many requests by the time this test runs.
admin.go: GET /admin now serves the generated adminIndexHTML
(admin_assets_gen.go, webui/generate.mjs's output) instead of the old
adminPageHTML Go-string const (removed, ~340 lines, plus its now-dead
io import). New route GET /admin/assets/{hashedname}
(adminAssetsPathPrefix) serves the built app's content-hashed JS/CSS
from the generated adminAssets map - unauthenticated (same rationale as
GET /admin itself), Content-Type per asset, Cache-Control: public,
max-age=31536000, immutable, X-Content-Type-Options: nosniff; an
unknown name is a 404 in the standard OAI error envelope.
CSP tightens: script-src/style-src drop 'unsafe-inline' for 'self' (the
built app has no inline script or style of any kind - vite.config.ts's
build.modulePreload.polyfill:false sees to that), img-src gains 'self'
data:' for the shell's inlined-SVG favicon.
Makefile: new admin-ui target (npm ci && vite build, then
generate.mjs, then gofmt -w) - the only place Node is invoked; CI and
every other make target still need nothing beyond Go.
admin_test.go: TestAdminPage_CSPHeaderAndFetchURLs rewritten as a shell
test against the generated file (new CSP string, body references
/admin/assets/ and the <title>'s "LLM Gateway", no external
src=/href= reference - the favicon's xmlns attribute is excluded from
that check, it names an XML namespace URI, not a fetched resource);
TestAdmin_JSONSecurityHeaders' expected CSP updated; new
TestAdminAsset_Serving (Content-Type/nosniff/immutable-cache/byte-exact
body), TestAdminAsset_UnknownReturns404, and
TestAdminAssets_GeneratedFilePresent (non-empty index.html/assets, no
leftover {{/}}/generator-marker text leaked into the decoded HTML -
proves the base64 round-trip actually worked).
Vue 3 + TypeScript + Pinia + Tailwind v4 + shadcn-vue + Chart.js admin
panel source under webui/, built with Vite. Views: Overview (providers,
redis/cache/retry status, model aliases), Usage (per-user/per-group/total
tables with tokensIn/tokensOut/cost + limits), Charts (scope selector,
req/tokens(stacked in-out)/cost metric tabs, 24h/30d/12mo window
switcher, fed by GET /admin/api/usage/history). Pinia stores: auth
(sessionStorage key gate, x-api-key header, 401/403 re-prompt),
dashboard (5s poll), history (fetch-on-selection + 30s refresh).
Design: deliberate neutral warm-neutral palette with a signature teal
accent (no default Inter/purple-gradient/glassmorphism), real
typographic hierarchy via the platform UI/mono font stacks (zero
external assets - no Google Fonts), dark+light via prefers-color-scheme
(no class toggle, no theme switcher). Charts: zero-based bars, honest
axes, Okabe-Ito colorblind-safe series, tokens-in/out as a
distinguishable stacked pair.
vite.config.ts: base '/admin/' (matches admin.go's route layout) and
build.modulePreload.polyfill:false (no inline bootstrap script, so the
tightened admin CSP needs no 'unsafe-inline'). Code-split: Chart.js
(ChartsView) loads lazily, not in the initial bundle.
generate.mjs reads dist/ after `vite build` and writes
../admin_assets_gen.go (not committed by this change) - base64-encodes
every asset, decoded once at package-var init; documented rationale in
the script's own header comment.
Hygiene: webui/go.mod walls webui/ off from the repo root's `go build`/
`vet`/`test ./...` (npm's node_modules bundles a stray .go file that
would otherwise be swept in); tools/yaegi-check/main.go excludes webui/
from its GOPATH copy for the same reason. webui/ export-ignored
(.gitattributes) from release source tarballs; webui/node_modules/
gitignored. tsconfig.app.json/tsconfig.node.json's JSONC comments
dropped (TS itself is fine with either; the global pre-commit JSON
validator is not) - no config values changed.
1. README's fallback-charting story corrected: the 48h ceiling
(memoryStoreMaxTTL) only binds hour and day, not month. hour charts
its full default 48-bucket span; day shows only its most recent
~2-3 of the 35 possible buckets (its key still sits under the 48h
ceiling). month is different: enforceTTLFor's ~32-day floor wins
over the 48h ceiling there, so the CURRENT (in-progress) month's
bucket charts in full on the fallback — only past months are
unavailable, once their own key expired ~32 days after its own
creation. Aligns with limits.go's clampTTL/enforceTTLFor and the
+5d TestMemoryStore_IncrMulti_MonthCounterSurvivesPast48h... test
from the prior round.
2. limits.go's "5 call sites, all in admin_test.go" attribution
belonged to incrCounter, not incrBy — incrBy itself (memoryStore.
incrBy and redisStore.incrBy together) has 11 direct test call
sites. Fixed at both doc-comment sites (the counterStore interface
doc and memoryStore.incrBy's own doc); the no-production-caller
claim is unchanged and correct for both.
3. Appended the missing "Fix round 2" section to
.superpowers/sdd/usage-history-report.md, documenting the
enforcement-floor design (counterIncr.enforceTTL, enforceTTLFor,
newCounterIncr, clampTTL's ceiling-vs-floor precedence) that this
report's round-1 section never described. Corrected that same
section's stale "incrMulti's loop inherits the clamp for free,
since it calls incrBy" sentence — round 2 replaced that shape:
incrMulti now clamps its own entries via the shared applyIncr
primitive and no longer calls incrBy at all.
Gate: go test ./... -count=1, gofmt, vet — all green. Docs and
comments only, no behavior change.
Re-review round on 5cc6814 found round 1's 48h fallback clamp
defective: it broke TTL > window-length for the MONTH window on
memoryStore, turning tokensPerMonth/costPerMonthUSD into a rolling
~48h budget (fail-open) whenever Redis is absent or down.
1. counterIncr gains enforceTTL time.Duration — the minimum retention
window's own enforcement correctness requires (enforceTTLFor:
min->2m, hour->2h, day->25h, month->32*24h, matching each window's
pre-history-bump TTL). newCounterIncr builds every checkAndCount/
account entry through one helper so a (window, ttl) pair can never
reach incrMulti without its matching floor. memoryStore's clampTTL
now takes (ttl, enforceTTL) and applies max(min(ttl,
memoryStoreMaxTTL), enforceTTL): the 48h ceiling still bounds
min/hour/day (unaffected/still-capped as before), but the floor
wins for month, landing it back at ~32 days (1-2 live keys per
scope/metric — negligible against the day-key savings that
motivated the ceiling). redisStore ignores enforceTTL; incrBy's own
single-key path passes enforceTTL=0 (ceiling only, unchanged
behavior — it has no window to derive a floor from and has no
production caller since the batching, see (3) below).
2. limits.go's TTL-invariant comment restated precisely: the consts
still satisfy TTL > window-length exactly as applied to Redis; that
did not automatically extend to memoryStore's own effective TTL
after its ceiling, which is what round 1 broke and round 2 fixes.
README: the Limits and accounting "Fallback" bullet now says
enforcement stays correct on the fallback at every window
(including tokensPerMonth/costPerMonthUSD), and that the 48h
ceiling applies to usage-history CHARTING retention only.
LimitsConfig's window table (tokensPerDay/tokensPerMonth rows)
cross-links it. The "resets on a rebuild" bullet's stale
limiter.storeIncrBy reference is corrected to storeIncrMulti and
now distinguishes rebuild-loss from the fallback's own per-window
TTL expiry.
3. counterStore.incrBy, limiter.storeIncrBy, and limiter.incrCounter's
doc comments now state plainly they have no production caller since
the batching (perf review, 2026-08-21) and are kept for direct
counter seeding in tests — 5 call sites, all in admin_test.go's
TestAdminUsage_MathAgainstSeededCounters, correcting the prior fix
report's "dozens of tests" overclaim to the actual count.
Tests: TestClampTTL_ExactMathTable pins the ceiling-vs-floor
precedence for every window's real (ttl, enforceTTL) pair.
TestEnforceTTLFor_MatchesTheOriginalPreHistoryBumpValues pins the four
floor values directly. TestMemoryStore_IncrMulti_
MonthCounterSurvivesPast48hAndStillEnforces drives a real
TokensPerMonth budget through account/checkAndCount on the fallback
5 days in and confirms both survival and enforcement.
TestMemoryStore_IncrMulti_DayCounterStillClampsAt48h confirms day
still resets at the 48h ceiling (its own floor sits below it).
Gate: go test ./... -count=1, -race, gofmt, vet, make yaegi-check —
all green.
Review fix round (2 Important + 5 folded) on 08cfdb4/b3fb705.
1. counterStore gains incrMulti(entries []counterIncr) ([]int64, error).
redisStore.incrMulti sends every entry's INCRBY+EXPIRE pair as ONE
pipeline (ttlToSeconds extracted from incrBy, now shared);
memoryStore.incrMulti loops incrBy for interface conformance.
checkAndCount batches every scope's req:min/req:day/req:hour into one
storeIncrMulti call (down from up to 9 round trips for a 3-scope
request) and reads req:min/req:day back from the reply, no extra
GETs. account batches every scope's nonzero tok-in/tok-out/cost
entries into one call (down from up to 27). Violation math and
fail-open/fail-closed/latched semantics are unchanged — only the
round-trip count moves.
2. memoryStoreMaxTTL (48h) caps every TTL memoryStore actually applies,
clamped inside incrBy so incrMulti's loop inherits it for free. The
in-process fallback is continuity, not history: applying the real
hour/day/month TTLs there was growing live-key count ~30x, multiplying
sweepEvery's own documented "167x incrBy cliff" by the same factor.
Sweep cadence is unchanged (still 30s-gated); the clamp shrinks what
there is to sweep instead.
Folded: historyMaxSpan's margin comment corrected (~4 days, not a
month); checkAndCount's godoc names req:hour; the integration
current-hour assertion sums every point instead of reading only the
last one (an hour-rollover race the suite cannot control); a new
exact-boundary test proves tokin+tokout landing exactly on a
TokensPerDay limit still violates; README documents the history route
as unthrottled (admin-gate is its only trust boundary) and the
fallback's 48h retention ceiling.
Tests: countingIncrStore proves exactly one incrMulti call per
checkAndCount and per account, with correct per-scope/metric/window
values and unchanged violation math; a fake-RESP-server test pins
incrMulti's wire format (one pipeline, entry-ordered replies); TTL
clamp tests cover both the ceiling and the pass-through case at the
boundary.
Gates: go test ./... -count=1, -race, gofmt, vet, golangci-lint,
make yaegi-check all green. make integration: every test passes
except the pre-existing TestUsersFileHotReload flake, which is NOT
caused by this change — verified via an isolated worktree at the
prior commit (b3fb705) run under identical conditions (full suite,
same host), which fails identically. This host's Docker Desktop is
currently slow to propagate the bind-mounted users.json change into
the containers; auth.go/users_file.go are untouched by this diff.
Extends Limits and accounting and Admin for the v0.2 data-layer
change (08cfdb4): the tokin/tokout counter split and how a
TokensPerDay/TokensPerMonth limit still reads their sum, the
synthetic total scope, the stats-only hour window, the retention TTL
bump, GET /admin/api/usage/history's query parameters and error
shapes, and the admin usage API's new tokensInPerDay/tokensOutPerDay
fields and total row. One-sentence migration note: old "tok" keys
expire on their existing TTL, nothing deployed.
Data layer for the upcoming Vue admin charts (v0.2 data-layer task).
Tokens-in/out split (limits.go): metricTok replaced by metricTokIn
("tokin") and metricTokOut ("tokout"). account() records usage.prompt
and usage.completion independently, skipping a direction only when its
own value is zero. A TokensPerDay/TokensPerMonth limit still enforces
one TOTAL budget: tokenBudgetViolation reads both counters in one
storeGetMulti batch and sums them; the violation message still says
"tokens". Estimated usage (the request-body-size fallback) lands
entirely in tokin, documented at its call site.
Hour window (limits.go): windowHour ("hour"), bucket format
"2006010215" UTC, hourWindowTTL (48h). checkAndCount/account both write
it on every metered route; no LimitsConfig field ever names it, so
evaluateScope never evaluates it. dayWindowTTL/monthWindowTTL bumped to
35d/400d so the usage-history API's largest span always finds a live
key.
Total scope (limits.go, routes_unified.go): a synthetic
{kind:"total", id:"all"} scope, limits always nil. withTotalScope
wraps buildLimitScopes' result at every metered route (unified,
media, passthrough, mcp/a2a) so none can forget it; handleAdminAPI's
own accounting stays on buildLimitScopes alone, so admin traffic never
contributes to it.
History API (admin.go): GET /admin/api/usage/history?scope=&metric=&
window=&span= — admin-gated like the other two JSON routes, but
skips checkAndCount ahead of that same change landing for every
/admin/api/* route in a later task. Validates scope kind/metric/window
(400), span bounds per window (400), and unknown user/group id (404).
Computes every bucket key up front and reads them in ONE storeGetMulti
batch; a storeDown read is 503, never a silently-zero series. Works
against memoryStore too.
Admin usage API (admin.go): scopeUsage/adminUsageEntryView now expose
tokensInPerDay/tokensOutPerDay and their per-month counterparts in
place of the combined tokensPerDay/tokensPerMonth pair; buildAdminUsage
adds a "total" row to the response, still one storeGetMulti round trip
overall. usageKeysPerScope names the per-scope key stride (was a bare
6, now 8), closing a deferred review follow-up while the count changed
anyway.
Tests: TDD throughout — bucket/window-key math incl. UTC and hour
boundaries, the month step-back day-31 AddDate trap, split accounting,
token budget summing both directions, total-scope counting-without-
evaluating on every metered route, and the full history API (shape,
oldest-first, span bounds, validation table, 404, 503, one-batch
assertion via a counting stub). Integration: TestAdminDashboard now
fetches an hour history series for user alice and for total and
asserts a non-zero current-hour bucket.
Migration: none — old "tok" counter keys simply expire on their
existing TTL; nothing is deployed yet.
Micro-fixes from re-review, all text/bound-level, no behavior change to
production logic:
1. settleToBaseline's doc comment (config_hot_reload_test.go) claimed to
guard the propagation window unconditionally. Reworded: it is a real
guard on the fired path (two known rebuilds to wait out), best effort
only on the skip path, where no amount of polling can prove a rebuild
will never eventually fire — waiting is still required there because
the earlier disappear-only check was a guaranteed-zero-wait bug, not
because settling can prove safety on that path.
2. Restored the cleanup settle bound from 20s to 30s, matching the
longest propagation delay actually observed during verification.
3. README's Config hot-reload intro dropped "on every run" and now
states the same fired-vs-skip distinction the test's own Skip message
makes: zero non-200s is blip-free-reload evidence only when the watch
fires within the test's bound; otherwise it shows just that the stack
survived an unreloaded rewrite attempt.
4. auth.go's group.cacheTTL field comment still said a positive value
"overrides it for this group's cache entries only" — reworded to the
corrected write-time/shared semantics: it sets the TTL written when
that group's own request populates an entry, entries are shared
across groups, not scoped by group.
Gate: go build/vet/gofmt clean on both modules; go test ./... -count=1
green.
Review response (5 Important + 3 folded):
1. README's cacheTTL row/bullet implied per-group scoping; cacheTTL sets
the TTL written when a group's own request populates a shared cache
entry, it never scopes the entry itself. Reworded only, cacheKey
unchanged.
2. README's fallback-counter claim ("used only when redis is not
configured") was wrong: limits.go also falls back during a Redis
outage whenever failOpen is true (the default). Reworded against
limiter.storeIncrBy/storeGet.
3. Dropped the unevidenced "in-flight requests finish on the old
instance" claim; replaced with what TestConfigHotReload actually
observes (zero non-200s across the rewrite window, every run).
4. Real bug: cleanup's settle poll checked for the injected alias to
disappear, which trivially no-ops on the skip path (the alias was
never live, so "absent" was already true on the first check) —
exactly the unguarded window that caused the earlier 404 cascade.
Replaced with settleToBaseline: captures the baseline /v1/models id
set up front, then polls unconditionally (both paths) for 3
consecutive matching polls, bounded 20s. Verified against a live run
that actually hit the skip path.
5. The ~20rps probe loop shared aliceKey/"eng" (1000/min) with most of
this suite's other tests; a budget-exhaustion 429 would look identical
to a reload blip. Added a dedicated hotreload group
(requestsPerMinute: 100000) and hotreload-probe user to
dynamic.yml.tmpl; every request TestConfigHotReload issues now uses
that key.
6. examples/docker-compose.yml still shipped providers.file.filename,
contradicting the new guidance; switched to providers.file.directory.
Verified by actually running the demo stack.
7. waitForModelAliasBounded and the new settle poll now use
doJSONNonFatal (returns an error instead of calling t.Fatalf), so a
transient transport error resets that poll's retry state instead of
aborting the whole test.
8. Reworded the skip-path Skip message: zero failures there proves only
that the stack survived an unreloaded rewrite attempt, not that a
successful reload is blip-free. Re-added the old single-file path to
.gitignore alongside the new directory entry.
Gates: go build/vet/gofmt clean on integration/; root go test -race,
gofmt, vet, yaegi-check all clean; make integration run cold twice, both
exit 0, plus a verbose run that landed on the skip path (the exact
scenario item 4 fixes) with the full suite, including
TestUsersFileHotReload, passing immediately after.
README Kubernetes section: what a Middleware CR edit actually does
(Traefik builds a new instance, warm-fills discovery, swaps it in; requests
in flight finish on the old one), what survives a rebuild (Redis-backed
counters and cache — windowKey's key never carries instance identity) vs
what resets (in-memory fallback counters, down-latches, the discovery
cache), why new models via discovery need no config edit, why the users
Secret must be a whole-volume mount (subPath never receives a kubelet
update) and how the plugin's own 5s mtime poll picks up the resulting
symlink swap, and why a file-provider dynamic config needs directory:
rather than filename: in Kubernetes (ConfigMap symlink swap) and under
Docker Desktop (VirtioFS fsnotify) alike.
examples/kubernetes.yaml: annotated the existing (already-correct,
no-subPath) users Secret volume mount explaining why subPath would
silently break Secret updates, so a future edit doesn't 'simplify' it
into one.
Single-file file-provider watch (providers.file.filename) never fires
under Docker Desktop's VirtioFS bind mount — empirically confirmed
against this stack. Traefik's directory-based file provider
(providers.file.directory) is the fix, and is also the correct pattern
for a Kubernetes ConfigMap mount (a symlink swap, which a single-file
watch never observes either).
integration/traefik/traefik.yml: providers.file.filename ->
providers.file.directory (watch: true unchanged). docker-compose.yml:
both traefik1 and traefik2 now bind-mount integration/traefik/dynamic/
(a directory) instead of the single rendered dynamic.yml. Makefile's
integration-up now mkdir -p's and renders into
integration/traefik/dynamic/dynamic.yml; .gitignore updated to match.
New TestConfigHotReload (build tag integration): baseline chat succeeds,
then a sustained ~20rps request loop runs against an existing model
while the rendered config is rewritten (temp file + os.Rename, atomic,
mirroring a k8s symlink swap) to add a new model alias. /v1/models is
polled up to 30s for the alias to appear. The request loop's zero-
failures property is asserted unconditionally, whether or not the watch
fired within the bound; only when it fired does the test also chat
through the new alias and expect 200. Empirically, on this host the
directory watch fires reliably but with variable latency (observed
under 1s up to several seconds; one run exceeded the 30s bound and hit
the Skip path with zero request failures throughout).
Found and fixed during verification: the test's cleanup step now writes
the restore atomically and WAITS for it to propagate before returning.
Without that wait, a delayed rebuild triggered by the restore could land
during a later test's own non-atomic config write (TestUsersFileHotReload's
users.json), reading a torn file mid-write and failing plugin
construction outright — a global 404 outage until the next unrelated
config change happened to trigger a successful rebuild. Reproduced and
confirmed fixed: two full `make integration` runs (cold teardown-to-up,
then immediately again) both pass end to end.
GroupConfig gains CacheTTL (json cacheTTL, empty inherits the global
cache.ttl). Parsed and validated at construction (newAuthStore,
groupCacheTTL in cache.go): a malformed duration, a duration <= 0, or
CacheTTL set while the global cache block is not configured is a
constructor error naming the group, mirroring Cache:true's existing
"nothing to inherit from" check.
group carries the resolved time.Duration (0 means inherit).
responseCache.store gains an explicit ttl parameter; routes_unified.go
computes effectiveTTL(g.cache, grp) immediately before each store call,
so setEx's Redis-native EX seconds reflect the group's own override.
Tests: constructor-error table, empty-inherits and valid-override
resolution, effectiveTTL inheritance/override, a scripted-RESP-server
proof that store's SET...EX carries a different value per group, and an
end-to-end Gateway.ServeHTTP test with two groups asserting the actual
wire EX value (30 vs 60) differs per group.
README: GroupConfig.cacheTTL config row and a Caching-section paragraph.
Discovery failures are non-fatal (explicit model keeps resolving
offline), and with discovery on, INTEGRATION_REAL=1 runs serve the
gateway's full live catalog from /v1/models instead of one pinned id.
Review fixes (Task 8, second pass):
- Fold TestRESPClient_AttemptPipelineLocked_WriteError and
TestRESPClient_HandshakeLocked_WriteError into one table,
TestRESPClient_WriteError_BrokenPipe — both exercised the identical
broken-pipe shape on a pre-set fakeConn.
- Take c.mu.Lock()/Unlock() around every direct call to an internal
"*Locked" method (ensureConnLocked, attemptPipelineLocked x2,
handshakeLocked) per each method's own "callers must hold c.mu"
precondition, even though every such test is single-goroutine.
- TestUnifiedCostMicros case 2: replace a want that called costMicros
(the function under indirect test) with the literal 11_250_000, hand-
derived from builtinPricing["gpt-5"] (InputPerM 1.25, OutputPerM 10)
at 1M prompt + 1M completion tokens.
- TestRedisStore_Get_DownServer_ReturnsError: assert err.Error() contains
"redisStore: get", not just that an error occurred.
Report corrected: the earlier claim that all new tests were table-driven
was inaccurate (actual split 8/16 at review time); documented as a
deliberate choice, not an oversight — six single-scenario tests stay flat
since a one-row table adds structure without adding coverage.
Gate: go test ./... -count=1 (pass), -race (pass), gofmt (clean), vet
(clean).
Lifts all 14 functions the operator flagged as <75% (measured at ed2119c)
to >=75%, using table-driven tests (require for fatals, assert for checks):
- limits.go: failPolicyGet (50.0% -> 100.0%)
- provider_gemini.go: httpClient (0.0% -> 100.0%)
- redis_store.go: get (71.4% -> 78.6%)
- resp.go: dialTimeoutFor, respDeadlineExceededErr.{Error,Timeout,Temporary},
ensureConnLocked, getBatch (66.7-30.0% -> 91.3-100.0%)
- routes_media.go: resolveMediaRequest (50.0% -> 100.0%)
- routes_unified.go: unifiedCostMicros (66.7% -> 100.0%)
- translate_anthropic.go: isTruthy, toFloat64 (50.0/33.3% -> 100.0%)
- translate_gemini.go: geminiToolConfigFromOpenAI (66.7% -> 100.0%)
resp_test.go adds a hand-rolled fakeConn (net.Conn with injectable
Write/SetDeadline errors) to reach two respClient error branches a real
TCP connection cannot be coaxed into deterministically.
Total coverage: 90.7% -> 92.4% (no regression; report has the full
per-file table). Zero production-code changes. Existing tests untouched:
every diff is a pure addition of new test functions and their imports.
Gates: go test -count=1 (pass), -race (pass, 5x incl. an isolated
baseline-vs-branch comparison after one flaky run implicated pre-existing
registry_test.go timing tests this task never touched), gofmt/vet (clean),
golangci-lint incl. gosec and fieldalignment (0 issues), make yaegi-check
(OK), make integration (pass, untouched-green).
Add github.com/stretchr/testify (test-only) as a direct dependency and
vendor it, matching the traefikoidc precedent: _test.go files are never
Yaegi-interpreted, so vendoring a test-only dependency does not affect the
plugin's runtime under Traefik or the catalog analyzer.
tools/yaegi-check/main.go: add vendor to the harness's repo-copy exclude
list, since vendor/'s nested module layout (go.yaml.in/yaml/v3 alongside
testify) has nothing to do with the plugin package Yaegi imports.
Verified: make yaegi-check stays green; go build/vet/test unaffected.
listFor's alias dedupe keyed on the raw owners map, so a colliding
discovered model whose provider is group-denied caused BOTH the real
entry and the alias to be skipped - the id vanished from the listing
entirely even though resolve still serves it via the alias.
Track ids actually appended to out in a listed map instead, and skip
the alias only when listed[alias] is true. Extends
TestModelRegistry_ListFor_Alias_DedupedAgainstDiscoveredCollision with
a provider-denied variant: group providers:["openai"], collision on
the denied "acme" provider -> alias listed exactly once, owned_by
openai.
Review fix, task 6 (1 Important + 3 folded):
1. TestModelAliases now polls GET /v1/models (waitForModelAlias) until
"aliased/mock" appears, bounded 30s, before asserting - removes the
stale-dynamic-config propagation window a plain healthy-200 check
cannot rule out. Confirmed config-propagation, not alias-logic, per
reviewer's config+error-text analysis.
2. registry.go resolve(): check the warned-set (aliasShadowWarned) before
paying bareWinner's O(providerNames) scan on every aliased request -
only the first request for a colliding alias pays that cost now.
3. listFor: skip an alias entry whose id already exists in owners (a real
model already owns it) - previously produced duplicate ids when a
discovered model collided with a configured alias. resolve's own
alias-wins precedence is unchanged; only the listing is deduped. New
test: TestModelRegistry_ListFor_Alias_DedupedAgainstDiscoveredCollision.
4. newAliasTestRegistry doc comment: "grp's given aliases" -> "the given
aliases" (no grp parameter exists).
Spec §5 (v0.2): Config.ModelAliases map[string]string, alias-first
resolution precedence ahead of provider-prefix splitting, alias-OR-target
authorization, lazy target resolution with a 404 naming both ids,
/v1/models listing, admin overview alias table (JSON + dashboard), and
construction-time validation (shape, prefix-shadow, explicit-model
collision, chain, empty target).
routes_unified.go/routes_media.go needed no resolution-call-site changes
- both already pass the client's raw requested id through to
registry.resolve, so aliases inherit the __alias echo and multipart
rewrite machinery for free. cache.go needed no changes either - cacheKey
already hashes requestedModel, so two aliases resolving to the same
target already produce distinct cache entries.
Adds registry_test.go validation table + precedence/authz/lazy-404/
listing coverage, a dedicated cache-key split test, chat/embeddings/
images/audio-transcriptions end-to-end tests, an extended admin overview
test, and an integration test covering the openai-type vs anthropic-type
echo asymmetry under real Traefik+Yaegi.
Task 5 re-review fixes: 1 Important + 3 folded.
1. (Important) examples/kubernetes.yaml was construction-broken on
copy-paste: the inline dashboard-admin user resolves env:LLMGW_ADMIN_KEY
at construction time regardless of admin.enabled, but the Helm env
snippet never listed it. Added it there, alongside a comment on the
admin user explaining the env var is required either way. Fixed the
two reversed above/below cross-references (kubernetes.yaml:90/:115)
and reworded the misleading "self-contained" comment on the inline
admin user. Verified with a throwaway round-trip test covering: no env
vars set fails, every documented env var set succeeds (admin.enabled
staying false as committed), and — the actual bug — every var set
EXCEPT LLMGW_ADMIN_KEY still fails even with admin disabled.
2. README: Admin section's "(disabled still 404s)" now points at the
same 404/passthroughUnknown wording the section already uses above it.
Caching section's "block absent entirely is a construction error" now
says "not enabled" — auth.go's actual check is !cfg.Cache.Enabled,
which is identically true whether the block is omitted or present with
enabled: false.
3. integration/mock/main.go: added POST /flaky/reset (handleFlakyReset),
reachable through the "flaky" provider's own native passthrough route,
resetting the atomic hit counter on demand. TestRetryFlakyRecovers now
calls it at the start of the test, removing the make integration-keep
re-run trap the old doc comment warned about instead of fixing.
dynamic.yml.tmpl's stale "test 8" comment now reads "task 5".
4. integration_test.go: added redisKeys (docker compose exec redis-cli
KEYS, mirroring flushRedis's own pattern) and a
TestResponseCache assertion that redis holds at least one
llmgw:cache:* key immediately after the miss request.
go test ./... -count=1, gofmt, vet, make yaegi-check all pass; make
integration exits 0 across two fresh full runs.
README, examples, and a comment update covering v0.2's four features:
- README: new Caching section (key semantics incl. requestedModel +
endpoint, hit/miss header, count-requests-free-tokens, group override,
redis-required) and Admin section (enable flag, admin:true users,
browser key-entry flow, sessionStorage, what's exposed/redacted,
poll-counts-against-quota, unauthenticated-shell). Retry and Image/audio
sections already existed from T3/T4; left as-is. Config reference gains
cache/admin rows at the top level, GroupConfig.cache, UserConfig.admin,
and full CacheConfig/AdminConfig tables. Known limitations gains
images/audio-not-cost-accounted and cache-shares-entries-across-users.
- examples/kubernetes.yaml: adds cache{enabled:true,ttl:5m} (this example
already configures redis) and admin{enabled:false} with a comment on
enabling it, plus an inline admin:true user gated behind that flag.
Round-trips through New() cleanly (verified with a throwaway test).
- examples/docker-compose.yml: adds retry, cache (left disabled — this
minimal demo runs no Redis), and admin (left disabled, with a comment
on enabling it). Round-trips through New() cleanly (verified the same
way).
- admin.go: pluginVersion's doc comment now says to keep it in sync with
the release tag.
Controller-ruled small fixes accumulated from T3/T4 reviews:
1. routes_media.go: multipart Content-Type check is now an exact
mediaType != "multipart/form-data" comparison (HasPrefix admitted
"multipart/form-datax"); a malformed-tail multipart body (fails to
parse at all) now reports "malformed multipart body", distinct from
"model is required" (a body that parses fine but carries no model
part) and "duplicate model field".
2. provider_openai.go: audioTranscription's doc comment no longer claims
the body is always "replayed unchanged"/the boundary always
"verbatim" — it now reflects the rewrite path routes_media.go
already takes for an aliased model id.
3. rewriteMultipartModel: buf.Grow(len(body)+512) up front, avoiding
repeated reallocate-and-copy growth while rebuilding an aliased
multipart body.
4. limits.go currentUsage: guards len(vals) == len(keys) before indexing
storeGetMulti's result, so a non-conforming counterStore's short slice
reports storeDown instead of panicking.
5. admin.go dashboard JS: the 5s poll no longer wipes an existing
auth-error message when no key is stored (only a successful load or a
new submission clears it now); clearStoredKey on a 401/403 now only
fires when the failing request's own key still matches what's
currently stored, guarding against a late response for a key the user
has since replaced.
6. Spec §4's duplicated /admin/api/overview bullet and the
nosniff/no-store sentence's placement were already fixed upstream
(docs/superpowers/specs/2026-08-20-llmgateway-v02-design.md, commit
c02010d) — verified identical, nothing further to commit here.
7. Added TestAdmin_JSONSecurityHeaders (nosniff/no-store/CSP on both
admin JSON APIs) and TestHandleAudioTranscriptions_MalformedTail_400
(covers fix 1's new malformed-body branch).
go test ./... -count=1 -race, gofmt, vet, make yaegi-check, golangci-lint
(incl. gosec) all pass.
Extends the integration suite (task 5) to exercise retry, cache, images/audio, and admin under real Traefik+Yaegi:
- mock/main.go: a /flaky/v1/chat/completions endpoint failing the first
request per process with 503 then succeeding (atomic counter, response
carries mock_attempt for verification); /v1/images/generations returning
a canned OpenAI images response; /v1/audio/speech returning binary bytes
under audio/mpeg; gemini mode's :predict returning canned Imagen
predictions.
- dynamic.yml.tmpl: retry{enabled:true,attempts:1}, cache{enabled:true,
ttl:2m}, admin{enabled:true}; a flaky provider pointed at the new mock
endpoint; an admin:true inline user; alice and admin both given a
generous-but-real per-user limits block so their own request counters
are trackable via the usage API.
- integration_test.go: TestRetryFlakyRecovers (client 200, exactly two
upstream hits proven via mock_attempt), TestResponseCache (miss then
hit, tokensPerDay unchanged across the hit via the admin usage API),
TestImagesGenerations (openai native forward + gemini Imagen
translation), TestAudioSpeech (binary round trip, Content-Type
forwarded), TestAdminDashboard (unauthenticated shell, admin vs
non-admin gate, usage reflecting this suite's own traffic).
make integration exits 0 (verified across multiple fresh runs).
Review fixes (2 Important + 4 folded):
1. GET /admin now serves the HTML shell with no authentication -
a browser navigating directly to the URL cannot attach a custom
header. The page reads an admin key from sessionStorage and sends
it as x-api-key on /admin/api/* fetches, falling back to an inline
key-entry form on a missing key or any 401/403. Both JSON routes
keep their original 401/403/200 gate. Spec ss4 amended in place.
2. GET /admin/api/usage no longer serializes 6 GETs per user/group
scope on the shared Redis connection. respClient gains getBatch
(one pipelined round trip); counterStore gains getMulti; limiter
gains storeGetMulti mirroring storeGet's fail-open/fail-closed
policy. currentUsage now issues one getMulti per scope.
3. Provider baseUrl is stripped of userinfo and query before it is
ever echoed in the overview JSON (sanitizeBaseURL), stronger than
net/url's own Redacted (which leaves a bare username in place).
4. limiter.redisStatus now also returns the last failure's timestamp,
so the dashboard can render "last error (Ns ago)" instead of a
bare message that reads as currently-broken indefinitely.
5. Both JSON admin responses now set X-Content-Type-Options: nosniff,
Cache-Control: no-store, and the same CSP the HTML page uses.
6. Added a test proving a file-sourced (replaceFileUsers) admin user
gets the same access an inline admin user does.
go test ./... -count=1 -race, gofmt, vet, make yaegi-check all pass.
Adds GET /admin (single-page dashboard), GET /admin/api/overview, and
GET /admin/api/usage per spec §4: providers/redis/cache/groups summary
and per-user/group current-window usage+limits. Gated by Config.Admin
(default off, not registered when disabled) and UserConfig.Admin.
Gate order: disabled -> 404 fall-through; unauthenticated -> 401;
authenticated non-admin -> 403; admin -> serve, counted against the
admin's own request limits like any other route.
Adds small, read-only, mutex-guarded hooks: modelRegistry.snapshot(),
limiter.currentUsage()/redisStatus(), authStore.snapshot(). No secret
field (API key, digest, redis password) is ever read into any admin
response type; verified by a canary-literal redaction test.
HTML/CSS/JS ship as a single Go raw-string const (Yaegi forbids
go:embed); confirmed interpretable via make yaegi-check.
Critical: attempts now counts retries performed AFTER the first try
(default 1 -> two tries total), not the total try count. The prior
reading made retry:{enabled:true}'s own documented default perform zero
retries. (*retryPolicy).totalTries() is the single conversion point,
clamping any attempts<1 to 1 so do()'s loop bound can never compute <=0
and skip calling call() entirely (no (nil,nil) return path).
Backoff waits are now ctx-aware: sleepFn func(time.Duration) is replaced
by waitFn func(context.Context, time.Duration) bool, whose production
implementation (defaultWaitFn) selects between time.After(d) and
ctx.Done(). A context canceled mid-wait aborts the remaining retries
immediately instead of blocking out the full backoff.
Also: classify a request-build failure (invalid method/URL) as
non-transient via errRequestBuildFailed, since retrying an identically
malformed request only wastes attempts; drop the drain cap to a
dedicated 64KiB constant instead of the 32MiB response cap; fix a
t.Fatal call from a non-test goroutine in the mid-stream-death test;
drop the misleading omitempty on Config.Retry (a struct value is never
'empty' to encoding/json); document the Retry-After >2s-falls-back-to-
backoff behavior and cover it with a direct table test.
README: add the retry config table, a RetryConfig field reference, and
a Retry section documenting the semantics. examples/kubernetes.yaml
gains a retry block alongside its other optional sections.
Gate: go test -race, gofmt, vet, yaegi-check, golangci-lint (incl a
fieldalignment fix on two new test structs) all clean. 295 subtests
pass, 0 failed. integration/ untouched.
Adds spec §1's opt-in retry engine: RetryConfig (top-level, zero value
disabled) and retryPolicy (retry.go), wired into upstreamJSON — the
single HTTP chokepoint every adapter's chat/embeddings/listModels call
already funneled through. A transient failure (net error, 429, 5xx)
retries up to Attempts times with exponential backoff capped at 2s,
honoring a qualifying Retry-After header on 429, draining and closing
each failed attempt's body before retrying. Never retries a non-429 4xx,
a translate error, or after context cancellation.
Because upstreamJSON returns before any adapter forwards a byte to the
client (forwardStream/forwardJSON run only after it returns), the
'zero bytes reached client' safety rule and the 'streaming retries only
the initial exchange' rule both hold by construction — no adapter
streaming logic needed to change.
buildAdapters(cfg) keeps its v0.1 signature: it derives the retryPolicy
from cfg.Retry internally rather than taking it as a parameter, so
every existing direct call (15+ across provider_openai_test.go) keeps
compiling unmodified. Every v0.1 test passes byte-identically, without
edits.
The upstream gateway's model ids carry a uni/ prefix; a provider named
uni made the plugin strip it as a provider prefix. Renamed to real and
listed the full slash-bearing id, exercising bare-id resolution.
logAuthEvent (logger.go) formatted r.URL.Path with %s in both the
failure and success log lines. An unauthenticated client controls that
path and can embed a raw newline (%0A) in it, letting a single request
forge additional, attacker-chosen log lines. Switched both to %q, which
escapes control characters (newlines included) while still rendering
the path as a quoted string.
No test changes needed: both existing tests (llmgateway_test.go) assert
via strings.Contains on the bare path substring, which %q's quoted
output still contains.
Gates: go test ./... -count=1 green, gofmt -l . empty, go vet clean.
I2 (controller ruling): docs/superpowers/specs/2026-08-19-traefik-llmgateway-design.md
described limits as "user overrides group" / "user limit if set, else
group limit, else unlimited" — stale relative to the actual, already-correct
implementation (buildLimitScopes/checkAndCount evaluate every configured
scope and return the first violation). Amended §2 and §7 to state both
scopes are enforced together and the tighter bound wins.
I4 (manifest hedge): .traefik.yml's groups.default was a bare "{}",
the shape empirically reproduced as a decode failure outside this
repo's own (necessarily simplified) yaegi-check harness. Changed to
"providers: []" — same zero semantics, no longer a bare empty object.
manifest_test.go's JSON mirror and drift-check token updated to match;
make yaegi-check still passes (its YAML-subset parser already handles
an inline "[]" under a nested map key).
M9 (pricing provenance): pricing.go's built-in table comment replaced
"verify before release" with a real source note (each provider's own
published list prices, recorded 2026-08, approximate). README's limits
section gained the matching caveat and points to the "pricing" config
override for billing-grade accuracy.
Doc nits: README's Kubernetes section now says the example manifest
applies cleanly once Traefik's CRDs (Middleware, IngressRoute) are
installed, rather than claiming kubectl apply -f works unconditionally.
README's Security notes section no longer claims "no auth-event logging
at all" (see the paired fix: commit) — it now describes what is logged
and that key material never appears.
This commit and its sibling fix: commit are the final wave closing out
the whole-branch review findings for docs/superpowers/sdd/2026-08-20-
traefik-llmgateway.
Gates: go test ./... -count=1 (-race) green, gofmt -l . empty, go vet
clean, make yaegi-check OK, golangci-lint 0 issues.
C1 (quota bypass): a client sending stream_options.include_usage=false
suppressed upstream usage reporting and escaped every token/cost budget,
since the non-streaming T12 estimate never covers a streaming response.
chatCompletion (provider_openai.go) now always forces include_usage=true
on the outgoing request, merged into a copy of the client's own
stream_options map so the caller's map is never mutated in place.
clientAskedUsage is derived from the client's own value via isTruthy, so
the usage-only chunk is still suppressed for a client that never asked
for it.
I1 (vacuous security tests): the X-Api-Key credential-strip set had zero
effective coverage — existing tests either never sent x-api-key, or
targeted anthropic, whose injectAuth overwrites x-api-key regardless of
whether the strip ran. Added one openai-passthrough and one MCP-proxy
test that authenticate via x-api-key itself and assert it arrives empty
upstream. Mutation-checked: both fail when "X-Api-Key": true is removed
from gatewayCredentialHeaders (verified, then restored).
I3 (auth-event logging): added Gateway.logAuthEvent (logger.go), called
at all six authStore.identify call sites (llmgateway.go's ServeHTTP,
routes_unified.go's handleModels). A failed attempt logs method, path,
and remote address; a success logs the resolved user's name and route.
Neither line ever includes the presented API key.
M2 (usage.estimated): runUnified now reads result.estimated and logs the
estimate explicitly when a non-streaming response's usage falls back to
the body-size heuristic, making the field genuinely read — removes the
stale //nolint:unused on limits.go's usage struct.
M8 (gosec consistency): secrets.go's file: secret path read now carries
the same "#nosec G304 -- operator-supplied file path from middleware
config" annotation as users_file.go's equivalent read.
Doc nit: mcp_a2a.go's validateTargetURLs comment now also names the
dot-only ("."/"..") rule validateConfigName enforces, not just character
set and reserved names.
Gates: go test ./... -count=1 (-race) green, gofmt -l . empty, go vet
clean, make yaegi-check OK, golangci-lint (gosec+staticcheck+unused+
errcheck+govet, global config) 0 issues.
Fixes from review, verified against source before rewriting:
- README.md Security notes: "auth events are logged" was false — grepped
auth.go, zero logging calls exist anywhere in it. A lookup miss is a
plain 401 envelope with no log line at all, not "logged without key
material". Also corrected the adjacent providerHTTPError claim: the
struct retains the upstream response body as a field (surfaced
deliberately to the client under error.upstream); only its Error()
string omits the body, and nothing in this codebase currently logs a
providerHTTPError via a bare %v anyway.
- README.md mermaid diagram: the previous version showed a single global
Auth gate ahead of routing. ServeHTTP (llmgateway.go) actually dispatches
on path+method first and calls identify() separately inside each
recognized-route branch; an unrecognized route (passthroughUnknown/404
fallback) never calls identify() at all. Redrawn Route -> per-branch
Auth, with the unauthenticated fallback branching directly off Route.
Also fixed the 401 node's error type label from the nonexistent
"invalid_api_key" to the actual envelope type, "authentication_error"
(llmgateway.go, routes_unified.go). Re-rendered with mmdc before and
after the fix.
- README.md + examples/kubernetes.yaml: narrowed the bare-{} empty-group
claim. Re-verified this session with an isolated, minimal repro
(Traefik file provider only, no docker-compose configs: mechanism
involved) that a bare `default: {}` still fails plugin construction
with the file provider specifically: "expected a map, got 'string'".
Scoped the claim to what is actually verified (file provider, twice)
and explicitly flagged Docker/ECS label providers and the Kubernetes
CRD provider as untested, rather than asserting the file provider is
unaffected or that the CRD provider shares its decode path — the
latter was an unverified inference in the prior text, now hedged.
- examples/kubernetes.yaml: the IngressRoute referenced an undefined
Service (llmgateway-dummy-backend), so the manifest did not actually
apply as advertised. Added the missing Deployment + Service (traefik/
whoami, the same image already exercised end-to-end in
examples/docker-compose.yml this session). Re-validated: 6-document
YAML parses, Middleware's plugin config still round-trips through the
real Config struct and New() (throwaway test, deleted after).
- README.md Known limitations: dropped the standalone path-traversal
bullet — it duplicated content already stated in the "request rejected
before it reaches a provider" bullet immediately above it.
NOTE for the reviewer: the ruling on the bare-{} finding stated "file
provider observed working" and named Docker/ECS label providers as the
affected ones. I re-tested this directly (twice, including a standalone
repro outside the compose demo) and the file provider fails on a bare
{} exactly as originally reported — I could not reproduce "file provider
unaffected" and have not tested the label providers at all. Narrowed the
doc wording to only what I verified rather than asserting either
provider's behaviour without evidence; flagging the discrepancy in case
the review had independent grounds I don't have visibility into.
Gates: go test ./... -count=1, gofmt -l ., go vet ./..., make
yaegi-check — all clean. Also re-ran go test -race, golangci-lint (incl.
gosec), and fieldalignment, matching the standard this task already set;
all clean.
configNamePattern (^[a-zA-Z0-9._-]+$) permits "." and ".." as valid
provider/mcpServers/agents names, since a bare dot is in the allowed
character class. Both names read as directory-traversal segments once
embedded as a path segment in the gateway's own routes
(passthroughRoute's "/{provider}/*", targetRoute's "/mcp/{name}/*" and
"/a2a/{name}/*"). Nothing in this package's current path handling
mis-resolves them, but disallowing them at construction is cheap and
removes any future dependence on that staying true.
- providers.go: add dotOnlyConfigNames ({".", ".."}), checked in
validateConfigName between the character-class check and the
reserved-name check.
- provider_openai_test.go: extend TestValidateConfigName with "." and
".." cases (rejected) across provider/mcpServers/agents kinds, plus
"..." (accepted — the rule is an exact match on "." and "..", not "any
dots-only string").
Gates: go test ./... -count=1, go test ./... -race -count=1, gofmt -l .,
go vet ./..., make yaegi-check, golangci-lint run ./..., golangci-lint
run --enable=gosec ./..., fieldalignment ./... — all clean.
- README.md: what-it-is, quickstart (localPlugins today, catalog form
once published), full config reference table for every Config/
ProviderConfig/GroupConfig/LimitsConfig/UsersConfig/UserConfig/
RedisConfig/ModelPricing/TargetConfig/AgentConfig field, model routing,
limits and accounting semantics, unified vs passthrough differences,
MCP/A2A, security notes, known limitations (streaming buffering under
Yaegi, quota-on-reject nuances, identity-encoding, path-traversal
rejects), Kubernetes deployment notes, and development/catalog-
submission status. Every claim grounded against the actual source read
this session; two draft claims (user-limits "replace" group limits, an
unqualified "go-redis is documented as unsuitable in resp.go/
providers.go") were caught and corrected against the real
buildLimitScopes semantics and an actual grep before committing.
- examples/kubernetes.yaml: Secret (users.json) + Middleware CR +
IngressRoute + a Helm values snippet. The Middleware's plugin config
round-trips through the real Config struct and New() (verified via a
throwaway, since-deleted test), not just YAML-syntax-checked.
- examples/docker-compose.yml: minimal localPlugins-based demo (traefik +
whoami + inline file-provider config via compose's inline configs.content).
Brought up and exercised end-to-end against real OpenAI this session:
auth 401/200, /v1/models, /v1/chat/completions (upstream 401 envelope
with a fake key, proving passthrough+key-injection+wrapping all work),
and the unknown-route 404 envelope. Found and fixed a real bug in the
process: a bare "{}" empty group value fails Traefik's own dynamic-
config decode with "expected a map, got 'string'"; fixed to an
explicit empty list and documented the gotcha in both example files and
the README.
- LICENSE: MIT, copyright 2026 Lukasz Raczylo.
- providers.go: validate provider names against ^[a-zA-Z0-9._-]+$ and
reject reserved names (v1, mcp, a2a) that would shadow gateway routes;
reject nil ProviderConfig map values at construction instead of
panicking.
- mcp_a2a.go: apply the same name-pattern and reserved-name validation to
mcpServers/agents keys, and reject nil TargetConfig/AgentConfig map
values at construction.
- auth.go: reject nil GroupConfig map values at construction.
- provider_openai_test.go, mcp_a2a_test.go, auth_test.go: table-driven
constructor-error coverage for the above.
- integration/integration_test.go: point the package doc at
`make integration-up`/`integration-keep` instead of a bare
`docker compose up -d --build`, which skips the dynamic.yml.tmpl
render step and can run against a stale or absent dynamic.yml; add
max_tokens to the gemini chat request so translate_gemini.go's
hasMaxTokens=true branch is actually exercised under real Yaegi.
- translate_gemini.go: soften the max_tokens comment's verification claim
to match what the integration suite actually now exercises.
Addresses coordinator review of the Task 15 integration suite: 1 critical
ruling, 5 important, 3 folded. make integration now exits 0.
- integration_test.go: TestStreamingIncrementalityProbe restructured —
content assertion (5 deltas + [DONE], in order) always runs; the raw-TCP
timing assertion (unchanged, >=3 gaps >=100ms) is gated behind
INTEGRATION_STREAMING=1 with t.Skipf naming traefik/traefik#10269,
verified to still genuinely fail when re-armed (not silently disabled).
- Makefile: integration-up/integration-wait moved from prerequisites into
the integration recipe body as go test ./... -count=1
ok github.com/lukaszraczylo/traefik-llmgateway 6.065s sub-invocations with per-stage
status capture, so integration-down always runs even when bring-up or
the health wait fails; verified with a synthetic failure. Added
.NOTPARALLEL (the integration targets are stateful).
- translate_gemini.go: fixed the same yaegi multi-value-assignment panic
shape as the prior sse.go fix, on the live max_tokens path. Added a
gemini chat completion assertion to integration_test.go (renamed
TestUnifiedChatAllProviders) so this path now runs under real Yaegi.
- providers.go, translate_anthropic.go: documented the never-wrap
contract on providerHTTPError/translateError (routes_unified.go asserts
their concrete type directly; errors.As panics under yaegi on these).
- integration_test.go: TestCrossReplicaLimits now FLUSHALLs Redis first,
for idempotence under integration-keep.
- mock/main.go: probe's header-terminator search now accumulates a
rolling buffer across reads instead of only checking the latest one.
- Folded: hot-reload deadline 10s -> 20s; dynamic.yml's real-upstream
baseUrl is now a template (dynamic.yml.tmpl) rendered via
INTEGRATION_REAL_BASEURL (default https://llmgw.example.com), not a
hardcoded-only hostname.
See task-15-report.md's addendum for full verification detail.
Adds integration/ (separate module): docker compose stack with real Traefik
v3.5 + localPlugins, mock openai/anthropic/gemini/tool providers, shared
Redis, and a raw-TCP streaming timing probe. 6/7 tests pass; streaming
incrementality fails on a confirmed external yaegi/Traefik bug (see
task-15-report.md), not a plugin defect.
Fixes 4 real Yaegi-interpretation bugs surfaced by running the plugin under
real Traefik for the first time (none caught by go test or yaegi-check,
which never exercises chat/embeddings/streaming/Redis):
- .traefik.yml: add basePkg (Traefik derives the yaegi binding identifier
from the module path, not the package's own declared name; without this
override the plugin never loads at all).
- registry.go: resolveAgainst's multi-value return panicked yaegi on an
inline string concatenation; assign to a local first.
- routes_unified.go: errors.As panicked yaegi on interpreted pointer error
types; use plain type assertions (both types are always returned bare,
never wrapped).
- sse.go: readSSE's dispatch closure hit the same multi-value-assignment
panic as registry.go; split into separate statements.
- resp.go: respErr as a defined string type was indistinguishable from a
plain string in a yaegi type assertion, misreading every successful
Redis SELECT reply as failed and silently disabling rate limiting;
changed to a struct.
Hoist the Upgrade->501 and traversal->400 checks in handleTargetProxy
above checkAndCount, matching handlePassthrough's own order
(403 -> 501 -> 400 -> limit). A request rejected before the upstream
call must never burn the caller's or their group's rate-limit quota
for a call that was never going to reach the target. Updated the
function's doc comment to state and justify the order.
Add TestHandleTargetProxy_RateLimited_Returns429WithRetryAfter:
group requestsPerMinute:1, two hits on /mcp/{name}/x -> first 200,
second 429 with Retry-After, upstream called exactly once.
Add TestHandleTargetProxy_JSONResponse_OnlyRequestCounterMoves: a
usage-shaped JSON response from the target leaves token/cost
counters at 0 while the request-rate counter moves to 1, proving
the target proxy never tees or accounts a response the way native
passthrough does.
Add tools/yaegi-check, a separate Go module (llmgw-yaegi-check,
never a dependency of the plugin module) that replicates the
Traefik Plugin Catalog analyzer (piceus)'s yaegiMiddlewareCheck:
copy the repo into a temp GOPATH, interp.New + i.Use(stdlib.Symbols),
import the plugin package, eval CreateConfig(), decode the plugin's
real .traefik.yml testData into it via a hand-rolled YAML-subset
parser plus a JSON round-trip, reflect-Call New from outside the
interpreter, then exercise the returned handler with two real HTTP
requests (authenticated GET /v1/models against the testData key, and
the same route unauthenticated). Pins github.com/traefik/yaegi to
v0.16.1, confirmed as Traefik v3.5.0's own vendored version by
downloading its go.mod.
Wire cd tools/yaegi-check && GOWORK=off go run . /Users/nvm/Documents/projects/private/traefik-llmgateway
yaegi-check: OK to run the harness from repo root.
The harness caught a real Yaegi v0.16.1 limitation: calling a struct
FIELD of variadic func type (e.g. modelRegistry.log
func(string, ...any)) with two or more variadic arguments crashes
Yaegi's CFG builder ('index out of range'). The identical call shape
through a method or interface method is unaffected — bisected with a
standalone yaegi repro to confirm the exact trigger before touching
plugin code. Fixed by pre-formatting with fmt.Sprintf at
modelRegistry's three affected call sites (registry.go) so the field
is always called with exactly one variadic argument; documented the
constraint on both modelRegistry.log and limiter.logf so it is not
reintroduced. Verified the gate itself is load-bearing by temporarily
renaming CreateConfig (harness failed non-zero) and restoring it
(harness passed again).
Also fixes two golangci-lint findings in the new harness module
(govet shadow, gosec G301 directory permissions).
Add GET /v1/mcp/servers and GET /v1/agents: group-filtered,
name-sorted registry listings of configured MCP servers and A2A
agents, with their gateway-relative proxy (and, for agents, card)
URLs.
Add handleTargetProxy for /mcp/{name}/{rest...} and
/a2a/{name}/{rest...}: reverse-proxies to the target's configured
URL. Checks run unknown name (404), group deny via allowsMCP/
allowsAgent (403), request-rate limits (429/503), then Upgrade
(501) and path traversal (400) around the proxy call itself. No
upstream credential is injected — MCP servers and A2A agents are
in-cluster targets, unlike native provider passthrough.
Extract proxyUpstream from routes_passthrough.go's handlePassthrough
into a shared reverse-proxy core (header stripping, SSE-safe
flushWriter streaming, dead-upstream 502, canceled-client log-only,
optional JSON usage teeing) used by both native passthrough and the
new target proxy. Behavior-preserving for Task 13: its tests pass
unmodified.
Validate every configured MCP-server and agent target URL at
construction (url.Parse, http/https scheme only) — a malformed or
non-HTTP target is a constructor error, not a per-request 502.
Review fixes for Task 12:
1. (CRITICAL) runUnified now calls g.limiter.account BEFORE handling an
adapter error, not after returning on it. All three adapters'
forwardStream deliberately return captured usage alongside a mid-stream
error, so a client that aborts right after the usage frame arrives was
getting tokens delivered but never billed - repeated aborts bypassed
every token/cost budget. A non-streaming error path always carries zero
usage by contract, so accounting it is a no-op (account skips writes
once both total tokens and cost are zero). The zero-usage-stream logf
and the zero-usage-non-stream estimate now only fire when callErr is
nil, so a genuine error no longer gets a fabricated token estimate.
New test: TestHandleChat_MidStreamDropAfterUsageChunk_AccountsPartialUsage
(hijack+close after the usage chunk; asserts the limiter's counter shows
the captured tokens). Verified this test fails against the pre-fix
ordering and the two new alias tests below fail when their guarded
delete() call is removed, before restoring and re-running clean.
2. Added TestHandleChat_OpenAI_AliasKeyStrippedFromUpstreamBody and
TestHandleEmbeddings_OpenAI_AliasKeyStrippedFromUpstreamBody: the
openai-type adapter marshals req verbatim as the upstream wire body, so
it is the one path where a leftover gatewayAliasKey entry actually
reaches a real provider. The only existing alias-leak assertion covered
anthropic, which rebuilds the body field-by-field and was never at risk.
3. Reworded the stale comment on limiter.evaluateScope (limits.go): it
described a nil-limits scope's req counters as a normal, expected case.
Ruling (e), implemented in routes_unified.go's buildLimitScopes, means
the unified route never builds a nil-limits scope in the first place -
the nil check remains a defensive no-op, not a path any current caller
exercises.
Gates: gofmt, go vet, fieldalignment, golangci-lint (incl. gosec) all
clean; go test ./... -race -count=1 passes (full suite, including all
Task 12 tests).
Implements Task 12: POST /v1/chat/completions and POST /v1/embeddings
through a shared pipeline (routes_unified.go: handleChat, handleEmbeddings,
runUnified) wired into ServeHTTP's dispatch in llmgateway.go.
Pipeline: decode (10MiB cap) -> resolve model -> enforce user/group
limits -> call adapter -> account usage, translating every failure mode
into its HTTP envelope per the task brief's carried rulings:
- ALIAS ECHO (ruling a): the client-requested model id is threaded to the
adapter via the internal gatewayAliasKey convention and echoed back in
anthropic/gemini's translated response model field; every adapter
strips the key before building its upstream wire body so it never
leaks to a real provider.
- Limit violations (ruling b): storeDown -> 503 server_error; otherwise
429 rate_limit_error with Retry-After when the violation names a
window.
- Stream-safe error handling (ruling c): statusTrackingWriter.wroteHeader
gates whether an adapter error still gets an envelope; providerHTTPError
wraps into {error:{message,type,code,upstream}}; translateError maps to
400/501; context.Canceled logs quietly; anything else is a 502.
- Usage/accounting (ruling d): zero non-streaming usage gets a
ceil(len(body)/4) prompt-token estimate; zero streaming usage logs once
and accounts the request only. unifiedCostMicros extends costMicros'
single-id lookup to canonical-then-bare without changing its signature.
- Scopes (ruling e): user scope only when u.limits is set, group scope
only when grp.limits is set, user first.
routes_unified_test.go drives every case end-to-end through
Gateway.ServeHTTP against httptest fake upstreams (openai- and
anthropic-shaped): happy path with counters asserted via the limiter's
own store, incremental stream flushing, 401/403/404/429/503, embeddings,
alias echo through a provider-prefixed anthropic request, wrapped
upstream error envelopes, and a mid-stream drop that must not append a
second envelope to an already-committed response.
Gates: gofmt, go vet, fieldalignment, golangci-lint (incl. gosec) all
clean; go test ./... -race -count=1 passes.
1. refreshProvider (the plugin's only background goroutine) had no
recover(): a panicking adapter's listModels would crash the whole
Traefik process. Restructured around deferred recover + finishRefresh,
so a panic is turned into a logged error, inFlight always releases, and
the next interval retries. Documented as a scoped exception to
limits.go's no-background-goroutine rule.
2. newGateway wrapped ctx in an outer 5s WithTimeout before warmFill,
but warmFill already derives a fresh 5s budget per provider from
whatever ctx it receives. The outer wrap made that a single budget
shared across every discovery-enabled provider instead of a per-provider
one - providers after the first could get a near-expired context and
fail instantly. Dropped the outer wrapper; construction's worst case is
now N * 5s, accepted.
3. group.allowsModel stripped at any first slash, so a glob like
'deepseek-*' could spuriously match a bare model id that merely
contains a slash (e.g. an upstream's own 'uni/deepseek-v4-flash-0731'
naming) even though the leading segment names no configured provider.
allowsModel is now an exact glob with no internal splitting;
resolveAgainst and listFor generate the provider-prefixed and
bare-suffix candidates themselves, only when a prefix legitimately
names a configured provider. Updated the Task-3 TestIdentify assertion
to match.
4. Annotated the discarded json.Encode error in handleModels.
Gate: go test ./... -count=1, -race, gofmt, vet, fieldalignment,
golangci-lint (incl gosec) all clean.
Adds modelRegistry: per-provider explicit+discovered model sets, bare-id
and provider-prefixed resolution with sorted-name collision precedence,
group authorization pairing (allowsModel AND allowsProvider), and a
group-filtered OpenAI-compatible /v1/models catalog.
Discovery is lazy and throttled per provider: newGateway runs a
synchronous 5s-bounded warm fill for discovery-enabled providers
(non-fatal on error), and Gateway.ServeHTTP triggers a throttled,
inFlight-guarded background refresh per provider per interval
(default 1h, per-provider DiscoveryInterval parsed at construction).
A failed refresh keeps the previous discovered set (stale-while-error).
Wires GET /v1/models into ServeHTTP (auth required, 401 envelope
otherwise) and gives Gateway its adapters/registry fields, both built
in newGateway.
newModelRegistry's signature gained an error return beyond the task
brief's literal listing: an invalid DiscoveryInterval must be a
constructor error per the controller ruling, which the brief's bare
*modelRegistry signature could not express.
Review round 1 fixes (1 Critical + 5 Important) on the anthropic
adapter/translator.
- CRITICAL: consecutive role:"tool" messages each became their own
Anthropic user turn. Anthropic requires alternating user/assistant
turns and 400s on two consecutive same-role messages; parallel tool
calls (one assistant message with N tool_calls followed by N
role:"tool" messages) are the standard agentic transcript shape.
anthropicRequestFromOpenAI now accumulates consecutive tool messages
into one Anthropic user turn carrying all of their tool_result
blocks (anthropicToolResultBlock replaces anthropicToolResultMessage).
Added chat_parallel_tool_results fixture.
- Streaming tool_calls[].index passed Anthropic's raw content-block
index through, so a tool_use block after a text block emitted
index:1 instead of the required contiguous 0-based ordinal.
anthropicStreamState now tracks a per-stream tool-call ordinal
counter plus a content-block-index -> ordinal map, populated on
content_block_start and consulted on input_json_delta. Added the
stream_text_then_tool fixture (text block at index 0, tool_use at
index 1) proving the emitted index is 0.
- A system message with array-form content ([{"type":"text",...}])
was silently dropped (only a string content was handled). Added
systemTextFromContentParts: text parts join with "\n"; a non-text
part returns a *translateError instead of being dropped. Added
chat_system_array_content (success) and
chat_system_array_content_error fixtures.
- Malformed JSON on a recognized stream event (message_start,
content_block_start, content_block_delta, message_delta) was
swallowed as (nil, nil), which could silently corrupt a client's
reassembled tool-call arguments. All four now return an error
wrapping errUpstream; forwardStream already propagates that and
withholds the terminal [DONE]. The unrecognized-event-type and
error-event branches are unchanged. Added
TestAnthropicStreamState_MalformedEventReturnsError.
- providerAdapter.injectAuth's doc comment claimed a universal
empty-key no-op; updated to state the per-type divergence
(openai-type: keyless is a no-op; anthropic-type: the constructor
rejects an empty key, so injectAuth is never called with one).
- Added case-insensitive-Content-Type coverage for both adapters'
streaming-vs-JSON gate (previously untested): one subtest each in
provider_openai_test.go / provider_anthropic_test.go using a fake
upstream that sends "Text/Event-Stream; charset=utf-8", asserting
the streaming path (not the JSON fallback) is taken.
Gate: go test ./... -count=1 and -race both green; gofmt, go vet,
fieldalignment, golangci-lint (incl. gosec), govulncheck all clean.
Implements Task 9: the anthropic-type providerAdapter and its
request/response/stream translation to and from OpenAI's wire shape.
- translate_anthropic.go: anthropicRequestFromOpenAI (system messages
joined, content blocks incl. base64 data-URI images, tool_calls ->
tool_use, tool messages -> tool_result, max_tokens defaulting,
stop/stop_sequences, tool_choice incl. the none-drops-tools rule,
n>1/logit_bias/logprobs rejected), openAIResponseFromAnthropic
(content blocks joined/tool_calls re-marshaled, stop_reason mapping,
usage), and anthropicStreamState.translate mapping each SSE event to
zero or more OpenAI chat.completion.chunk payloads. translateError
carries a notSupported flag so the caller (Task 12) can tell a 400
from a 501.
- provider_anthropic.go: anthropicAdapter implementing providerAdapter.
newAnthropicAdapter errors on an empty API key per ruling (c), unlike
openai-type's keyless ruling. Headers: x-api-key, anthropic-version.
embeddings always returns a notSupported translateError without
calling upstream (Anthropic has no embeddings endpoint). listModels
GETs /v1/models and returns any non-2xx (incl. 404) as-is.
- providers.go: wires the anthropic buildAdapters case to
newAnthropicAdapter, propagating its constructor error.
- provider_openai_test.go: replaces the now-stale placeholder
TestBuildAdapters subtest with one asserting a keyed anthropic
provider builds successfully and a keyless one errors.
- translate_anthropic_test.go: golden-file harness walking
testdata/anthropic/{chat,resp}_*_in.json against sibling _want.json
(success) or _wanterr.json (*translateError) fixtures, plus a stream
harness driving each *.sse fixture through anthropicStreamState and
asserting the chunk list and usage().
- provider_anthropic_test.go: httptest-backed adapter wiring tests
(streaming + non-streaming), asserting OpenAI-shaped output on the
wire, per-event flushing, and the required auth headers.
Gate: go test ./... -count=1 and -race both green; gofmt, go vet,
fieldalignment, golangci-lint (incl. gosec) all clean.
Content-Type header values are case-insensitive per RFC 7231, but
chatCompletion's streaming-vs-JSON-fallback gate used a plain
strings.Contains against "text/event-stream", so an upstream that
sent "Text/Event-Stream" or similar would silently fall through to
the non-streaming passthrough path. Carried item (a) from Task 9's
brief.
Reject a nested array reply instead of recursing into it. decodeReply
now takes a depth parameter; a '*' type byte is accepted only at
depth 0, so an element that is itself an array (decodeArray calls
decodeReply at depth+1) is a protocol error rather than a recursive
call. A crafted "*1\r\n*1\r\n..." stream repeated could otherwise
recurse to Go's stack limit — a fatal, unrecoverable error, not a
panic recoverPanic can catch — from only a few megabytes of wire
input. Also lower respMaxArrayLen from 1<<20 to 1024: none of this
client's commands (AUTH, SELECT, INCRBY, EXPIRE, GET) ever return an
array reply at all, so even 1024 is generous headroom, not a limit
tuned to real traffic.
Add a store-down latch to the limiter (limits.go): after any store
operation fails, record the failure time; for storeDownLatchFor (5s)
afterward, every storeIncrBy/storeGet call short-circuits straight to
the fail-open/fail-closed policy without attempting the store at all.
One request can touch several counters (its own and its group's,
request/token/cost, day/month) — each respClient call is already
bounded to ~respCallTimeout on its own, but without the latch every
one of those still paid a fresh bounded wait against a store that had
just proven unreachable. After the window, the next operation probes
the real store again: a natural half-open retry, no separate recovery
state. Guarded by the same logMu mutex lastLogAt already uses for this
kind of small timestamp state.
Clamp respClient's dial timeout to the remaining call budget:
dialTimeoutFor returns min(respDialTimeout, time.Until(deadline)),
with a respDeadlineExceededErr (implements net.Error, Timeout() true)
returned outright if the deadline has already elapsed — e.g. this
call queued behind the client's mutex for its whole budget while an
earlier call was stuck. Without this, a future increase to
respDialTimeout alone, leaving respCallTimeout untouched, could
silently let one dial attempt consume more of a call's total budget
than the call-level deadline is supposed to allow.
New tests: a nested-array reply and an array length one over the new
1024 cap, both asserted as protocol errors; a countingErrorStore stub
proving a second checkAndCount within the latch window makes zero
additional store calls, and that the store is probed again once the
window elapses. Tightened TestLimiter_FailOpenTrue_
HungRedisServer_FallsBackQuickly's bound from <6s to <3s now that the
latch collapses its two store ops into one real network attempt.
Bound respClient's worst-case latency to one respCallTimeout per
do/pipeline call instead of per read/write/handshake step. Compute a
single absolute deadline at call entry, apply it once via
conn.SetDeadline after connecting (covering the AUTH/SELECT handshake
and the call's own command I/O), and skip the reconnect-retry when the
failing attempt was a timeout (errors.As -> net.Error, Timeout()) — a
peer that accepted the connection and went silent must not be asked
the same non-question twice. Previously each read, write, and dial got
its own fresh 2s budget and every caller queued behind the client's
single mutex, so a hung (not merely refused) Redis-compatible server
could stall one checkAndCount call for 16-32s+.
Cap decodeBulk/decodeArray's wire-declared lengths (respMaxBulkLen
64MiB, respMaxArrayLen 1<<20) before allocating: an unguarded
"$9223372036854775807\r\n" overflows n+2 into a negative slice length
and panics make([]byte, n+2); a merely huge-but-valid length forces an
outsized allocation either way. decodeBulk also now verifies its two
trailing bytes are CRLF. Bound readLine to respMaxLineLen (64KiB),
reading byte-by-byte so it can bail before ever buffering an unbounded
line from a peer that never sends '\n'.
Fix TestLimiter_FailClosed_BudgetReadReturnsStoreDownViolation: the
erroringStore it used fails incrBy too, so checkAndCount's own
increment loop already returned a storeDown violation before
evaluateScope (and therefore budgetViolation) ever ran, leaving
budgetViolation's storeDown branch uncovered. Replace it with a stub
that only fails get, so the test's TokensPerDay limit actually drives
evaluateScope into budgetViolation's fail-closed path.
Document redis_store.go's incrBy as deliberately at-least-once: the
reconnect-retry can re-send INCRBY after a lost reply (possible
over-count), and fail-open can additionally count in the fallback —
both fail-safe (more restrictive), never fail-open in the unsafe
direction.
Validate Redis.DB >= 0 in newConfiguredLimiter alongside Address, so a
negative db is a constructor error rather than a SELECT -1 on the wire.
New tests: a hung (accepts, never replies) fake RESP server proving a
respClient call and a failOpen limiter call both stay bounded rather
than stacking timeouts; decodeReply subtests for an oversized bulk
length, a length just over the cap, a huge array length, a bad bulk
trailer, and an oversized line; a negative-DB constructor-rejection
test.
Add resp.go: a minimal stdlib-only RESP2 client (respClient) for a
single Redis-compatible server. It lazily dials one mutex-guarded
connection, applies 2s dial/read/write deadlines, sends AUTH (if a
password is set) then SELECT on every new connection, and reconnects
once on any I/O error before giving up. do and pipeline encode
commands as "*N\r\n$len\r\n..." and decode +/-/:/$/* replies.
Add redis_store.go: redisStore implements counterStore over
respClient. incrBy pipelines INCRBY and EXPIRE in one round trip and
returns INCRBY's value; get treats a missing key (RESP null bulk) as
0.
Wire the limiter's fail-open/fail-closed policy in limits.go, which
Task 5 stored but never read: a configured store's error now routes
through storeIncrBy/storeGet. failOpen=true falls back to the
in-process memoryStore transparently; failOpen=false makes
checkAndCount return a storeDown violation and makes account drop the
sample. Both paths log through an injectable, 30s-rate-limited logf.
Wire newGateway (llmgateway.go) to build a redisStore-backed limiter
when Config.Redis is set (password through resolveSecret, failOpen
defaulting to true unless RedisConfig.FailOpen says otherwise) and a
memory-only limiter otherwise, and to route pricing.go's unknown-model
warnings through the gateway's own logf.
Validate LimitsConfig at construction: a new LimitsConfig.validate
rejects a negative int64 count limit or a negative/NaN/Inf cost limit,
called from newAuthStore for every group and every user (inline and
file-sourced) so a malformed limit is a constructor error instead of
silently behaving as unlimited.
Add RequestsPerDay, TokensPerMonth, and CostPerMonthUSD enforcement
tests alongside the existing per-minute/day/cost-per-day cases, plus
an erroringStore-backed test pair proving the fail-open (fallback
counted) and fail-closed (storeDown violation) branches directly.
Folded the validation work into this single commit rather than a
second one: it interleaves with the fail-open changes across
limits.go, limits_test.go, and llmgateway_test.go closely enough that
splitting by hunk would be artificial.
- users_file.go: maybeReload uses reloadMu.TryLock() instead of Lock().
A full reload (stat+read+parse+replaceFileUsers, incl. per-user
resolveSecret file reads) could otherwise block every concurrent
ServeHTTP request for its whole duration, negating the buildMu/mu
lock split. A request that finds a reload in flight now skips this
cycle; reload stays throttled and best-effort.
- users_file.go: mtime check changed from ModTime().After(lastModTime)
to ModTime().Equal(lastModTime), so a backward mtime move (restore
from backup, cp -p) still triggers a reload instead of being
skipped. Added a regression test with os.Chtimes into the past.
- llmgateway.go: attachUsersFile now stats the file before loading it,
matching maybeReload's ordering, so a write landing between stat and
load is caught on the next check instead of being lost. Documented
why the ignored statErr is safe (falls back to zero lastModTime,
causing one harmless extra reload).
Add usersFile (newUsersFile/load) for the JSON users-file shape
{"users":[{name,group,apiKey,limits?}]}, and authStore.maybeReload
for throttled hot reload: checks mtime at most every reloadEvery
(5s), reloads on change, keeps the last good set and logs via
errorf on any error (stat, parse, or a rejected replace), and logs
via logf when the reload changes the file-sourced user count.
nowFn is injectable for tests.
Wire-up in llmgateway.go: newGateway does a synchronous initial
load when Users.File is set (a bad initial file fails the
constructor); ServeHTTP calls g.auth.maybeReload() at entry
(no-op when no file is configured).
Also fixes replaceFileUsers's merge semantics: a file user now
overrides an inline user of the same name, even when their keys
differ, dropping the inline entry from the rebuilt set. The prior
digest-only keying let a same-named inline/file pair with
different keys coexist instead of the file version winning.
Add buildMu to serialize replaceFileUsers's writer body (including
buildEntry's potentially slow file-backed resolveSecret reads for
"file:"-prefixed API keys). Scope mu.Lock() to the final byDigest
swap only, so a rebuild in progress no longer stalls every identify
call for its whole duration. Behavior-preserving: writers stay fully
serialized, same as before.
Adds a regression test that holds buildMu externally and asserts
identify still succeeds — proving identify's read path does not
depend on the writer-serialization lock.
lukaszraczylo