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).
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.
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.
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.
lukaszraczylo