fix: fixed a bunch of bugs and agents Markdown files updated#134
Merged
SantiagoDePolonia merged 34 commits intomainfrom Mar 12, 2026
Merged
fix: fixed a bunch of bugs and agents Markdown files updated#134SantiagoDePolonia merged 34 commits intomainfrom
SantiagoDePolonia merged 34 commits intomainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds hint-aware native batch routing and a StoredBatch persistence wrapper, extends GatewayError with Param/Code, refines circuit-breaker handling for 429, augments chat/choice types with system_fingerprint/logprobs, and centralizes app Start/Shutdown lifecycle helpers with tests and broad store/handler/provider updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as Handler/Batches
participant Router
participant Provider as Provider\n(Hint-Aware)
participant Store as Batch Store
Client->>Handler: POST /batches (create)
Handler->>Router: CreateBatchWithHints(providerType, req)
Router->>Provider: CreateBatchWithHints(ctx, req)
Provider-->>Router: BatchResponse, hints map
Router-->>Handler: BatchResponse, hints map
Handler->>Store: Create(StoredBatch{Batch: response, RequestEndpointByCustomID: hints})
Handler->>Provider: ClearBatchResultHints(batchID) (optional)
Handler-->>Client: BatchResponse
rect rgba(100,150,200,0.5)
Note over Client,Store: Results retrieval with stored hints
end
Client->>Handler: GET /batches/{id}/results
Handler->>Store: Get(id)
Store-->>Handler: StoredBatch{Batch, RequestEndpointByCustomID}
Handler->>Router: GetBatchResultsWithHints(providerType, id, hints)
Router->>Provider: GetBatchResultsWithHints(ctx, id, hints)
Provider-->>Router: BatchResultsResponse
Router-->>Handler: BatchResultsResponse
Handler-->>Client: Results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/providers/router.go (1)
75-90:⚠️ Potential issue | 🟠 MajorRun
checkReady()before any provider-type routing.When
lookupimplementsinitializedLookupandIsInitialized()is already true, this path skipsModelCount()entirely. That lets batch/file/passthrough routes proceed against an empty registry, which breaks the readiness invariant the router is supposed to enforce.Based on learnings: Check
ModelCount() > 0before routing to ensure the registry is initialized.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/providers/router.go` around lines 75 - 90, In resolveProviderType, ensure readiness is validated even when r.lookup implements initializedLookup and IsInitialized() returns true: call the registry model-count check (e.g., r.lookup.ModelCount() > 0) or otherwise invoke checkReady() before allowing provider-type routing so that an empty registry is treated as not-ready; if ModelCount() == 0 return the same registryUnavailableError(err) path (or wrap an ErrRegistryNotInitialized) to prevent batch/file/passthrough routes from proceeding against an empty registry. Reference: Router.resolveProviderType, initializedLookup, IsInitialized, checkReady, ModelCount, registryUnavailableError.internal/server/handlers.go (1)
1891-1897:⚠️ Potential issue | 🟠 MajorReturn a
GatewayErrorhere too.This fallback still emits an ad-hoc JSON map with
internal_error, so clients get a different schema and an error type outside the allowed set. Route unexpected failures throughcore.GatewayError/ToJSON()instead of constructing the payload manually.As per coding guidelines: All errors returned to clients must use `core.GatewayError` with typed categories (provider_error, rate_limit_error, invalid_request_error, authentication_error, not_found_error).Suggested fix
- // Fallback for unexpected errors - auditlog.EnrichEntryWithError(c, "internal_error", "an unexpected error occurred") - return c.JSON(http.StatusInternalServerError, map[string]interface{}{ - "error": map[string]interface{}{ - "type": "internal_error", - "message": "an unexpected error occurred", - "param": nil, - "code": nil, - }, - }) + // Fallback for unexpected errors + fallbackErr := core.NewProviderError("gateway", http.StatusInternalServerError, "an unexpected error occurred", err) + return handleError(c, fallbackErr)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/handlers.go` around lines 1891 - 1897, Replace the ad-hoc JSON map return with a core.GatewayError constructed for unexpected failures and return its JSON via ToJSON(); specifically, instead of building the inline map in the handler (the block that currently returns c.JSON(... with "internal_error")), create a core.GatewayError instance with the appropriate allowed category (e.g., provider_error) and the same user-facing message ("an unexpected error occurred"), call its ToJSON() and pass that to c.JSON so all error responses follow the core.GatewayError schema.internal/providers/anthropic/anthropic.go (1)
618-643: 🧹 Nitpick | 🔵 TrivialExtract common SSE parsing logic to eliminate duplication.
The pipeline reports duplicate code between this SSE parsing block (lines 618-641) and the equivalent block in
responsesStreamConverter.Read(lines 1493-1516). Both blocks parse SSE data, unmarshal JSON, and handle errors identically.Consider extracting this into a shared helper function that returns the parsed event and any error, which both converters can call.
♻️ Proposed approach
// parseSSEData parses an SSE data line into an anthropicStreamEvent. // Returns nil event for non-data lines. Closes body and returns error on parse failure. func parseSSEData(line []byte, body io.ReadCloser, closed *bool) (*anthropicStreamEvent, error) { if !bytes.HasPrefix(line, []byte("data:")) { return nil, nil } data := bytes.TrimSpace(bytes.TrimPrefix(line, []byte("data:"))) var event anthropicStreamEvent if err := json.Unmarshal(data, &event); err != nil { *closed = true _ = body.Close() return nil, malformedAnthropicStreamError(err) } return &event, nil }Then both
streamConverter.ReadandresponsesStreamConverter.Readcan use this helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/providers/anthropic/anthropic.go` around lines 618 - 643, Extract the duplicated SSE parsing logic into a helper (e.g., parseSSEData) that takes the raw line ([]byte), the stream body (io.ReadCloser) and a pointer to the closed flag, and returns (*anthropicStreamEvent, error); replace the repeated bytes.HasPrefix/TrimPrefix/json.Unmarshal/close-body/malformedAnthropicStreamError sequence in both the streamConverter.Read (the block using sc.body, sc.closed, sc.buffer, sc.convertEvent) and responsesStreamConverter.Read with a call to this helper, and on success pass the returned event into the existing sc.convertEvent and buffer/return logic so behavior remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Line 20: The ordered list item "The Twelve-Factor App" is numbered as "2." but
must follow the repository's MD029 1/1/1 style; update that list marker to "1."
so the list uses consistent "1." numbering (change the list marker for the "The
Twelve-Factor App" line).
- Around line 47-49: Update the "Error Handling" section to document the
concrete client-facing error contract: state that all errors returned to clients
must be instances of core.GatewayError, list the required typed categories
(provider_error, rate_limit_error, invalid_request_error, authentication_error,
not_found_error), and explicitly document the public response shape including
any exposed metadata fields such as param and code; also add a short note that
this doc must be updated whenever behavior, config, providers, commands, or
public APIs change.
In `@cmd/gomodel/main.go`:
- Around line 40-44: The Shutdown call can block forever because provider
cleanup is synchronous; wrap application.Shutdown(shutdownCtx) in a goroutine
and wait with select on either a done channel or shutdownCtx.Done() so the
function returns when the 30s timeout elapses, or alternatively make
application.Shutdown and the underlying provider cleanup (e.g., providers.Close)
context-aware and propagate shutdownCtx into them; locate the call site using
shutdownCtx, cancel and application.Shutdown and update it to abort/return a
timeout error if shutdownCtx.Done() fires, or change providers.Close and related
cleanup functions to accept a context and honor cancellation so Shutdown itself
cannot block past the timeout.
In `@internal/core/batch.go`:
- Around line 89-92: The RequestEndpointByCustomID field on core.BatchResponse
is gateway-only state that should not be JSON-serializable in API DTOs; move
this field out of the public model by creating an internal persistence wrapper
or separate gateway-only struct used for storage/restart hints (e.g., a
BatchPersistence or internalBatch type) and update code paths that read/write
RequestEndpointByCustomID to use that wrapper instead of core.BatchResponse,
keeping core.BatchResponse free of RequestEndpointByCustomID so
json.Marshal(batch) never leaks gateway-only data.
In `@internal/core/errors.go`:
- Around line 182-185: The code currently maps both 401 and 403 to
NewAuthenticationError (which hardcodes StatusCode: 401), so upstream 403
Forbidden responses get rewritten to 401; split the cases so 401 uses
NewAuthenticationError(provider, message) and 403 uses an authorization-specific
gateway error (either call a NewAuthorizationError(provider, message) if
available or construct a core.GatewayError with StatusCode:
http.StatusForbidden, Provider: provider, Message: message) to preserve the
original 403; keep the existing mapping for http.StatusTooManyRequests ->
NewRateLimitError.
In `@internal/llmclient/client.go`:
- Around line 233-245: The current logic in shouldTripCircuitBreaker treats 429
as "do not trip", which causes recordCircuitBreakerCompletion to call
RecordSuccess; change recordCircuitBreakerCompletion (the code that currently
calls c.circuitBreaker.RecordFailure() or RecordSuccess()) to special-case
statusCode == http.StatusTooManyRequests: if the circuit breaker is half-open
(use the circuit breaker's state check, e.g. IsHalfOpen or equivalent), treat
429 as a failure and call c.circuitBreaker.RecordFailure(); otherwise (closed or
open) do not call RecordSuccess or RecordFailure (i.e. return without recording
a success) so 429 won't count as a successful half-open probe; keep
shouldTripCircuitBreaker as-is for closed/trip logic.
In `@internal/providers/router_test.go`:
- Around line 145-149: The hint-aware mock used in these tests must capture the
batch ID forwarded by Router.GetBatchResultsWithHints so the test can assert
correct propagation; modify the mock implementation in router_test.go (the stub
backing GetBatchResultsWithHints) to store the received batchID into an existing
field (e.g., capturedBatchHints map or a new capturedForwardedBatchID field
alongside clearedBatchHintID/hintedBatchResults) when it delegates, then update
the test assertions to check that the expected batch ID equals the captured
value instead of relying on an empty/default ID.
In `@internal/server/handlers_test.go`:
- Around line 170-175: The handler test currently allows the handler to drop
provider type, batch ID and request context because GetBatchResultsWithHints is
stubbed with context.Background(), "", "" and ClearBatchResultHints only records
the batch ID; update the test mocks to capture and assert the forwarded routing
inputs: have the fake GetBatchResultsWithHints record the incoming context,
provider string and batch ID (e.g., into capturedBatchReq / capturedBatchHints
or new variables) instead of ignoring them, and enhance the lifecycle test to
assert those captured values match the original request-scoped context, provider
type and batch ID; also update ClearBatchResultHints to capture both batch ID
and provider/context if applicable so the test fails if the handler stops
propagating these routing inputs.
---
Outside diff comments:
In `@internal/providers/anthropic/anthropic.go`:
- Around line 618-643: Extract the duplicated SSE parsing logic into a helper
(e.g., parseSSEData) that takes the raw line ([]byte), the stream body
(io.ReadCloser) and a pointer to the closed flag, and returns
(*anthropicStreamEvent, error); replace the repeated
bytes.HasPrefix/TrimPrefix/json.Unmarshal/close-body/malformedAnthropicStreamError
sequence in both the streamConverter.Read (the block using sc.body, sc.closed,
sc.buffer, sc.convertEvent) and responsesStreamConverter.Read with a call to
this helper, and on success pass the returned event into the existing
sc.convertEvent and buffer/return logic so behavior remains identical.
In `@internal/providers/router.go`:
- Around line 75-90: In resolveProviderType, ensure readiness is validated even
when r.lookup implements initializedLookup and IsInitialized() returns true:
call the registry model-count check (e.g., r.lookup.ModelCount() > 0) or
otherwise invoke checkReady() before allowing provider-type routing so that an
empty registry is treated as not-ready; if ModelCount() == 0 return the same
registryUnavailableError(err) path (or wrap an ErrRegistryNotInitialized) to
prevent batch/file/passthrough routes from proceeding against an empty registry.
Reference: Router.resolveProviderType, initializedLookup, IsInitialized,
checkReady, ModelCount, registryUnavailableError.
In `@internal/server/handlers.go`:
- Around line 1891-1897: Replace the ad-hoc JSON map return with a
core.GatewayError constructed for unexpected failures and return its JSON via
ToJSON(); specifically, instead of building the inline map in the handler (the
block that currently returns c.JSON(... with "internal_error")), create a
core.GatewayError instance with the appropriate allowed category (e.g.,
provider_error) and the same user-facing message ("an unexpected error
occurred"), call its ToJSON() and pass that to c.JSON so all error responses
follow the core.GatewayError schema.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c8f2753f-ff54-4a81-a65f-fe4c58d2423f
📒 Files selected for processing (24)
AGENTS.mdCLAUDE.mdcmd/gomodel/main.gocmd/gomodel/main_test.gointernal/batch/store_test.gointernal/core/batch.gointernal/core/errors.gointernal/core/errors_test.gointernal/core/interfaces.gointernal/core/types.gointernal/core/types_test.gointernal/llmclient/client.gointernal/llmclient/client_test.gointernal/providers/anthropic/anthropic.gointernal/providers/anthropic/anthropic_test.gointernal/providers/batch_results_file_adapter.gointernal/providers/init.gointernal/providers/init_test.gointernal/providers/router.gointernal/providers/router_test.gointernal/server/handlers.gointernal/server/handlers_test.gointernal/usage/cost.gointernal/usage/cost_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/server/handlers.go (1)
961-968:⚠️ Potential issue | 🟠 MajorMove the typed-nil check into the provider call path.
This guard runs after
fileByIDhas already decided the current provider succeeded. On requests without?provider=, one backend returning(*core.FileContentResponse)(nil), nilnow short-circuits the search and returns 502 instead of trying the next provider, and the constructedGatewayErrorloses the upstream provider by using"".💡 Validate the provider response before `fileByID` treats it as a success
func (h *Handler) GetFileContent(c *echo.Context) error { return h.fileByID(c, func(r core.NativeFileRoutableProvider, provider, id string) (any, error) { - return r.GetFileContent(c.Request().Context(), provider, id) + resp, err := r.GetFileContent(c.Request().Context(), provider, id) + if err != nil { + return nil, err + } + if resp == nil { + return nil, core.NewProviderError(provider, http.StatusBadGateway, "provider returned empty file content response", nil) + } + return resp, nil }, func(c *echo.Context, result any) error { - resp, ok := result.(*core.FileContentResponse) - if !ok || resp == nil { - return handleError(c, core.NewProviderError("", http.StatusBadGateway, "provider returned empty file content response", nil)) - } + resp, ok := result.(*core.FileContentResponse) + if !ok { + return handleError(c, core.NewProviderError("", http.StatusBadGateway, "provider returned invalid file content response", nil)) + } contentType := strings.TrimSpace(resp.ContentType)As per coding guidelines
**/*.go: Use typed gateway errors (core.GatewayError) and map upstream failures correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/handlers.go` around lines 961 - 968, The typed-nil check must be moved into the provider-call path so fileByID only treats truly successful provider responses as success; inside the provider function you pass to fileByID (the lambda that calls r.GetFileContent), after calling r.GetFileContent(c.Request().Context(), provider, id) validate the returned value: if it is a typed nil (resp == nil) return a core.GatewayError (use core.NewGatewayError or the project convention) that includes the provider identifier and http.StatusBadGateway so fileByID can continue to the next provider (or surface the correct upstream); leave the outer result-handling lambda to assume a non-nil *core.FileContentResponse and remove the typed-nil guard there.
♻️ Duplicate comments (7)
cmd/gomodel/main.go (1)
38-45:⚠️ Potential issue | 🟠 MajorThe 30s startup-shutdown timeout is still not enforced end-to-end.
application.Shutdown(shutdownCtx)is still invoked synchronously here. Ininternal/app/app.go:293-379, the context is only honored while waiting for the HTTP server; provider, batch, usage, and audit cleanup continue synchronously afterward. A startup failure can therefore still hang past 30 seconds instead of returning on timeout.One way to bound the call at this layer
func startApplication(application lifecycleApp, addr string) error { if err := application.Start(context.Background(), addr); err != nil { shutdownCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() - if shutdownErr := application.Shutdown(shutdownCtx); shutdownErr != nil { - return fmt.Errorf("server failed to start: %w", errors.Join(err, fmt.Errorf("shutdown after start failure: %w", shutdownErr))) - } + done := make(chan error, 1) + go func() { + done <- application.Shutdown(shutdownCtx) + }() + + select { + case shutdownErr := <-done: + if shutdownErr != nil { + return fmt.Errorf("server failed to start: %w", errors.Join(err, fmt.Errorf("shutdown after start failure: %w", shutdownErr))) + } + case <-shutdownCtx.Done(): + return fmt.Errorf("server failed to start: %w", errors.Join(err, fmt.Errorf("shutdown after start failure: %w", shutdownCtx.Err()))) + } return err } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/gomodel/main.go` around lines 38 - 45, startApplication currently calls application.Shutdown(shutdownCtx) synchronously so the shutdown work that ignores the HTTP wait can block past the 30s timeout; change startApplication to invoke application.Shutdown(shutdownCtx) in a goroutine and wait for either the shutdown to complete or shutdownCtx.Done(), capture any shutdown error from the goroutine via a channel, and return a timeout error if shutdownCtx expires; reference the lifecycleApp interface, the startApplication function, the shutdownCtx/cancel variables and the application.Shutdown call when making this change.CLAUDE.md (2)
20-20:⚠️ Potential issue | 🟡 MinorFix the ordered-list numbering for markdownlint.
Line 20 uses
2.but should use1.to follow the MD0291/1/1list style per markdownlint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` at line 20, Change the numbered list marker for the item "2. [The Twelve-Factor App](https://12factor.net/)." to "1." so the markdown list follows the MD029 1/1/1 style; locate the list entry with that exact link text and replace the leading "2." with "1." to satisfy markdownlint.
47-49:⚠️ Potential issue | 🟡 MinorDocument the concrete error contract.
The error handling section still only states "OpenAI-compatible" without specifying the
core.GatewayErrorcontract and typed categories (provider_error,rate_limit_error,invalid_request_error,authentication_error,not_found_error). Based on learnings: All errors returned to clients must usecore.GatewayErrorwith typed categories.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` around lines 47 - 49, Update the "Error Handling" section to specify the concrete error contract: state that all API error responses must use the core.GatewayError shape and list its fields (e.g., code, message, type, details) and enumerate the required typed categories: provider_error, rate_limit_error, invalid_request_error, authentication_error, not_found_error; include brief examples of each category and note that clients must map provider errors into these categories before returning to callers using the core.GatewayError format.internal/llmclient/client.go (1)
233-245:⚠️ Potential issue | 🟠 Major429 responses still incorrectly count as successes in half-open state.
The
shouldTripCircuitBreakerhelper correctly excludes 429 from tripping, butrecordCircuitBreakerCompletionstill falls through toRecordSuccess()for 429 responses. In half-open state, this increments the probe success counter, potentially closing the breaker while the provider is still rate-limiting.Consider special-casing 429 in
recordCircuitBreakerCompletionto skip bothRecordFailure()andRecordSuccess()when in half-open state, or treat it as a failure specifically for half-open probes.🔧 Suggested approach
func (c *Client) recordCircuitBreakerCompletion(statusCode int, err error) { if c.circuitBreaker == nil { return } if err != nil { c.circuitBreaker.RecordFailure() return } + // 429 should not affect circuit breaker state - provider is healthy but rate-limiting + if statusCode == http.StatusTooManyRequests { + return + } if c.shouldTripCircuitBreaker(statusCode) { c.circuitBreaker.RecordFailure() return } c.circuitBreaker.RecordSuccess() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/llmclient/client.go` around lines 233 - 245, The circuit-breaker completion logic incorrectly counts 429 as a success; update recordCircuitBreakerCompletion to special-case HTTP 429: if statusCode == http.StatusTooManyRequests and c.circuitBreaker.IsHalfOpen() then call c.circuitBreaker.RecordFailure() (or skip calling RecordSuccess()) instead of falling through to RecordSuccess(); keep shouldTripCircuitBreaker unchanged but ensure recordCircuitBreakerCompletion uses c.circuitBreaker.IsHalfOpen(), c.circuitBreaker.RecordFailure(), and c.circuitBreaker.RecordSuccess() to implement the half-open 429 behavior.internal/core/batch.go (1)
89-92:⚠️ Potential issue | 🟠 MajorKeep gateway-only hints out of
core.BatchResponse.
swaggerignore:"true"only hides this field from generated docs; it is still part of the JSON shape for anyjson.Marshal(batch). Sinceinternal/batch/store_postgresql.go:52-86persists the wholeBatchResponse, this internal hint map now lives on a public DTO and is one unsanitized response path away from leaking. Move it to an internal persistence/storage wrapper instead of the API model.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/batch.go` around lines 89 - 92, The field RequestEndpointByCustomID on core.BatchResponse is an internal gateway hint and must be removed from the public API model; create an internal persistence wrapper (e.g., a new struct like persistedBatch or batchStorage in the internal/batch package) that includes the RequestEndpointByCustomID map alongside the public BatchResponse payload, update internal/batch/store_postgresql.go to persist/load that wrapper instead of marshaling core.BatchResponse directly, and remove RequestEndpointByCustomID from core.BatchResponse so json.Marshal no longer includes the gateway-only map.internal/core/errors.go (1)
182-184:⚠️ Potential issue | 🟠 MajorPreserve upstream 403s instead of rewriting them to 401.
This branch still funnels
401and403throughNewAuthenticationError(), which hardcodesStatusCode: 401. Clients will see forbidden responses as unauthorized, and the updated tests end up locking in the wrong contract.Based on learnings: Use typed gateway errors (`core.GatewayError`) and map upstream failures correctly.🔧 Minimal fix
- case statusCode == http.StatusUnauthorized || statusCode == http.StatusForbidden: - gatewayErr = NewAuthenticationError(provider, message) + case statusCode == http.StatusUnauthorized: + gatewayErr = NewAuthenticationError(provider, message) + gatewayErr.Err = originalErr + case statusCode == http.StatusForbidden: + gatewayErr = NewAuthenticationError(provider, message) + gatewayErr.StatusCode = http.StatusForbidden + gatewayErr.Err = originalErr🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/errors.go` around lines 182 - 184, The code currently maps both 401 and 403 to NewAuthenticationError (which hardcodes StatusCode: 401); change the conditional handling in the error conversion routine so that statusCode == http.StatusUnauthorized still maps to NewAuthenticationError(provider, message) but statusCode == http.StatusForbidden is preserved and converted to a gateway/forbidden typed error (e.g., return or construct a core.GatewayError/NewGatewayError with StatusCode 403 and the original message/provider) instead of reusing NewAuthenticationError; update the branch that sets gatewayErr to use the appropriate constructor for 403 so upstream responses keep their 403 status.internal/providers/router_test.go (1)
143-149:⚠️ Potential issue | 🟡 MinorCapture the forwarded batch ID in the hint-aware mock.
This stub still falls back through
GetBatchResults(context.Background(), ""), so the new hinted-path test would pass even ifRouter.GetBatchResultsWithHints()dropped the caller context or routed the wrong batch. Record the incomingid, delegate withctx, id, and assert it in the test.As per coding guidelines `**/*_test.go`: Add or update tests for any behavior change.🧪 Tighten the hinted-path mock and assertion
type mockBatchProvider struct { mockProvider listBatchesResp *core.BatchListResponse hintedBatchResults *core.BatchResultsResponse capturedBatchHints map[string]string + capturedBatchID string clearedBatchHintID string } -func (m *mockBatchProvider) GetBatchResultsWithHints(_ context.Context, _ string, endpointByCustomID map[string]string) (*core.BatchResultsResponse, error) { +func (m *mockBatchProvider) GetBatchResultsWithHints(ctx context.Context, id string, endpointByCustomID map[string]string) (*core.BatchResultsResponse, error) { + m.capturedBatchID = id if len(endpointByCustomID) > 0 { m.capturedBatchHints = make(map[string]string, len(endpointByCustomID)) for customID, endpoint := range endpointByCustomID { m.capturedBatchHints[customID] = endpoint } } if m.hintedBatchResults != nil { return m.hintedBatchResults, nil } - return m.GetBatchResults(context.Background(), "") + return m.GetBatchResults(ctx, id) }if got := provider.capturedBatchHints["resp-1"]; got != "/v1/responses" { t.Fatalf("capturedBatchHints[resp-1] = %q, want /v1/responses", got) } + if provider.capturedBatchID != "provider-batch-1" { + t.Fatalf("capturedBatchID = %q, want provider-batch-1", provider.capturedBatchID) + }Also applies to: 174-185, 699-730
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/providers/router_test.go` around lines 143 - 149, The mockBatchProvider's hinted-path should capture and forward the batch ID instead of calling GetBatchResults with an empty id: update the mock implementation used in tests (mockBatchProvider) so its GetBatchResultsWithHints or equivalent hinted method records the incoming id into capturedBatchHints (or a new capturedBatchID field), delegates to the real provider using ctx, id (not context.Background(), ""), and the same id, and then update the test assertions to check that the capturedBatchHints/capturedBatchID equals the expected batch id; apply the same change pattern to other occurrences noted (lines ~174-185 and ~699-730).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Around line 5-12: Edit the intro paragraph text for grammar: change "a
OpenAI-compatible" to "an OpenAI-compatible", change "provider's response" to
"providers' responses", and change "pass it to the user" to "pass them to the
user" so the sentence reads e.g. "GoModel is a high-performance AI gateway in Go
that routes requests to multiple AI model providers through an OpenAI-compatible
API" and "GOModel accepts providers' responses liberally and passes them to the
user in a conservative OpenAI-compatible shape."
In `@internal/core/types.go`:
- Around line 86-103: The exported types ChatResponse and Choice were extended
with SystemFingerprint and Logprobs but lack Go doc updates; add concise Go
comments clarifying these fields are optional provider metadata included for
diagnostic/compatibility purposes (e.g., update the ChatResponse type comment to
mention SystemFingerprint is optional provider metadata and update the Choice
field comment to explain Logprobs is provider-specific, may be empty/null, and
is returned only when available). Ensure the comments appear immediately above
the type/field declarations (ChatResponse, SystemFingerprint, Choice, Logprobs)
and follow existing Go doc style.
In `@internal/providers/anthropic/anthropic_test.go`:
- Around line 156-185: The test TestGetBatchResultsWithHints only covers the
fully-hinted path; add a row without a custom_id hint to exercise the fallback
branch in GetBatchResultsWithHints and assert it uses the default endpoint
("/v1/chat/completions"). Modify the test to send at least one additional
response row that lacks a mapping in the hints map, then check resp.Data for
that row’s URL and assert it equals "/v1/chat/completions" (while keeping
existing assertions for the hinted row); use the same test setup
(NewWithHTTPClient, provider.SetBaseURL, and the mock server) to locate and
validate the fallback behavior.
In `@internal/providers/anthropic/anthropic.go`:
- Around line 568-603: The payload builders anthropicChatUsagePayload and
anthropicResponsesUsagePayload currently emit provider-specific cache fields
(cache_creation_input_tokens, cache_read_input_tokens) into the normalized usage
map; change them to only return normalized keys ("prompt_tokens"/"input_tokens",
"completion_tokens"/"output_tokens", and "total_tokens") and stop adding any
cache_* fields, and instead marshal the Anthropic-specific counters into the
RawData/RawUsage JSON blob so provider-specific fields
(CacheCreationInputTokens, CacheReadInputTokens) are preserved only in RawData;
update anthropicChatUsagePayload and anthropicResponsesUsagePayload to omit
cache_* keys and ensure callers that persist RawUsage include the
provider-specific fields in RawData.
In `@internal/providers/init.go`:
- Around line 39-46: The teardown in r.closeOnce.Do calls r.stopRefresh() but
does not wait for the background refresher started by StartBackgroundRefresh to
exit, which can race with Refresh/SaveToCache touching r.Cache; change the stop
hook used by StartBackgroundRefresh (stopRefresh) to be blocking by making it
cancel the refresh loop and then wait on a done channel or sync.WaitGroup (e.g.,
have StartBackgroundRefresh return a stop function that signals cancellation
then blocks until the loop returns), and only call r.Cache.Close() (and assign
r.closeErr) after that blocking stop returns so the refresher cannot access
r.Cache after it is closed.
In `@internal/server/handlers.go`:
- Around line 1860-1865: sanitizeBatchResponseForAPI currently mutates the
store-loaded *core.BatchResponse in place and should instead return a sanitized
clone so stored records aren't modified; change sanitizeBatchResponseForAPI to
accept *core.BatchResponse, create and return a shallow/deep copy (preserving
all other fields) with RequestEndpointByCustomID set to nil, and update callers
(GetBatch, ListBatches, CancelBatch) to use the returned clone when writing JSON
/ appending to responses so the underlying stored record remains untouched.
In `@tests/contract/testdata/golden/groq/chat_with_params.golden.json`:
- Line 18: Remove the "system_fingerprint" field from the Groq golden payload(s)
so they match the provider's actual response; since
internal/providers/groq/groq.go unmarshals into core.ChatResponse and
ChatResponse.SystemFingerprint is tagged `omitempty` and Groq does not return
that field, update the golden JSON(s) (remove the "system_fingerprint" key/value
from the groq chat golden(s) such as chat_with_params.golden.json and any other
Groq chat goldens updated in this PR) so serialized test output matches reality.
---
Outside diff comments:
In `@internal/server/handlers.go`:
- Around line 961-968: The typed-nil check must be moved into the provider-call
path so fileByID only treats truly successful provider responses as success;
inside the provider function you pass to fileByID (the lambda that calls
r.GetFileContent), after calling r.GetFileContent(c.Request().Context(),
provider, id) validate the returned value: if it is a typed nil (resp == nil)
return a core.GatewayError (use core.NewGatewayError or the project convention)
that includes the provider identifier and http.StatusBadGateway so fileByID can
continue to the next provider (or surface the correct upstream); leave the outer
result-handling lambda to assume a non-nil *core.FileContentResponse and remove
the typed-nil guard there.
---
Duplicate comments:
In `@CLAUDE.md`:
- Line 20: Change the numbered list marker for the item "2. [The Twelve-Factor
App](https://12factor.net/)." to "1." so the markdown list follows the MD029
1/1/1 style; locate the list entry with that exact link text and replace the
leading "2." with "1." to satisfy markdownlint.
- Around line 47-49: Update the "Error Handling" section to specify the concrete
error contract: state that all API error responses must use the
core.GatewayError shape and list its fields (e.g., code, message, type, details)
and enumerate the required typed categories: provider_error, rate_limit_error,
invalid_request_error, authentication_error, not_found_error; include brief
examples of each category and note that clients must map provider errors into
these categories before returning to callers using the core.GatewayError format.
In `@cmd/gomodel/main.go`:
- Around line 38-45: startApplication currently calls
application.Shutdown(shutdownCtx) synchronously so the shutdown work that
ignores the HTTP wait can block past the 30s timeout; change startApplication to
invoke application.Shutdown(shutdownCtx) in a goroutine and wait for either the
shutdown to complete or shutdownCtx.Done(), capture any shutdown error from the
goroutine via a channel, and return a timeout error if shutdownCtx expires;
reference the lifecycleApp interface, the startApplication function, the
shutdownCtx/cancel variables and the application.Shutdown call when making this
change.
In `@internal/core/batch.go`:
- Around line 89-92: The field RequestEndpointByCustomID on core.BatchResponse
is an internal gateway hint and must be removed from the public API model;
create an internal persistence wrapper (e.g., a new struct like persistedBatch
or batchStorage in the internal/batch package) that includes the
RequestEndpointByCustomID map alongside the public BatchResponse payload, update
internal/batch/store_postgresql.go to persist/load that wrapper instead of
marshaling core.BatchResponse directly, and remove RequestEndpointByCustomID
from core.BatchResponse so json.Marshal no longer includes the gateway-only map.
In `@internal/core/errors.go`:
- Around line 182-184: The code currently maps both 401 and 403 to
NewAuthenticationError (which hardcodes StatusCode: 401); change the conditional
handling in the error conversion routine so that statusCode ==
http.StatusUnauthorized still maps to NewAuthenticationError(provider, message)
but statusCode == http.StatusForbidden is preserved and converted to a
gateway/forbidden typed error (e.g., return or construct a
core.GatewayError/NewGatewayError with StatusCode 403 and the original
message/provider) instead of reusing NewAuthenticationError; update the branch
that sets gatewayErr to use the appropriate constructor for 403 so upstream
responses keep their 403 status.
In `@internal/llmclient/client.go`:
- Around line 233-245: The circuit-breaker completion logic incorrectly counts
429 as a success; update recordCircuitBreakerCompletion to special-case HTTP
429: if statusCode == http.StatusTooManyRequests and
c.circuitBreaker.IsHalfOpen() then call c.circuitBreaker.RecordFailure() (or
skip calling RecordSuccess()) instead of falling through to RecordSuccess();
keep shouldTripCircuitBreaker unchanged but ensure
recordCircuitBreakerCompletion uses c.circuitBreaker.IsHalfOpen(),
c.circuitBreaker.RecordFailure(), and c.circuitBreaker.RecordSuccess() to
implement the half-open 429 behavior.
In `@internal/providers/router_test.go`:
- Around line 143-149: The mockBatchProvider's hinted-path should capture and
forward the batch ID instead of calling GetBatchResults with an empty id: update
the mock implementation used in tests (mockBatchProvider) so its
GetBatchResultsWithHints or equivalent hinted method records the incoming id
into capturedBatchHints (or a new capturedBatchID field), delegates to the real
provider using ctx, id (not context.Background(), ""), and the same id, and then
update the test assertions to check that the capturedBatchHints/capturedBatchID
equals the expected batch id; apply the same change pattern to other occurrences
noted (lines ~174-185 and ~699-730).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c22643b7-79b1-4656-887e-6d280a2cbbbd
📒 Files selected for processing (36)
AGENTS.mdCLAUDE.mdcmd/gomodel/main.gocmd/gomodel/main_test.gointernal/batch/store_test.gointernal/core/batch.gointernal/core/errors.gointernal/core/errors_test.gointernal/core/interfaces.gointernal/core/types.gointernal/core/types_test.gointernal/llmclient/client.gointernal/llmclient/client_test.gointernal/providers/anthropic/anthropic.gointernal/providers/anthropic/anthropic_test.gointernal/providers/batch_results_file_adapter.gointernal/providers/init.gointernal/providers/init_test.gointernal/providers/router.gointernal/providers/router_test.gointernal/server/handlers.gointernal/server/handlers_test.gointernal/usage/cost.gointernal/usage/cost_test.gotests/contract/testdata/golden/groq/chat_completion.golden.jsontests/contract/testdata/golden/groq/chat_with_params.golden.jsontests/contract/testdata/golden/groq/chat_with_tools.golden.jsontests/contract/testdata/golden/openai/chat_completion.golden.jsontests/contract/testdata/golden/openai/chat_completion_reasoning.golden.jsontests/contract/testdata/golden/openai/chat_json_mode.golden.jsontests/contract/testdata/golden/openai/chat_multi_turn.golden.jsontests/contract/testdata/golden/openai/chat_multimodal.golden.jsontests/contract/testdata/golden/openai/chat_with_params.golden.jsontests/contract/testdata/golden/openai/chat_with_tools.golden.jsontests/contract/testdata/golden/xai/chat_completion.golden.jsontests/contract/testdata/golden/xai/chat_with_params.golden.json
Comment on lines
86
to
+103
| // ChatResponse represents the chat completion response | ||
| type ChatResponse struct { | ||
| ID string `json:"id"` | ||
| Object string `json:"object"` | ||
| Model string `json:"model"` | ||
| Provider string `json:"provider"` | ||
| Choices []Choice `json:"choices"` | ||
| Usage Usage `json:"usage"` | ||
| Created int64 `json:"created"` | ||
| ID string `json:"id"` | ||
| Object string `json:"object"` | ||
| Model string `json:"model"` | ||
| Provider string `json:"provider"` | ||
| SystemFingerprint string `json:"system_fingerprint,omitempty"` | ||
| Choices []Choice `json:"choices"` | ||
| Usage Usage `json:"usage"` | ||
| Created int64 `json:"created"` | ||
| } | ||
|
|
||
| // Choice represents a single completion choice | ||
| type Choice struct { | ||
| Message ResponseMessage `json:"message"` | ||
| FinishReason string `json:"finish_reason"` | ||
| Index int `json:"index"` | ||
| Logprobs json.RawMessage `json:"logprobs,omitempty"` |
Contributor
There was a problem hiding this comment.
Update the public API docs for SystemFingerprint and Logprobs.
ChatResponse and Choice are exported response types, but the new fields land without any doc update. Add field comments or expand the type comments so the public contract explains that these are optional provider metadata fields.
Suggested doc update
-// ChatResponse represents the chat completion response
+// ChatResponse represents the chat completion response, including optional
+// provider-specific metadata such as a system fingerprint.
type ChatResponse struct {
ID string `json:"id"`
Object string `json:"object"`
Model string `json:"model"`
Provider string `json:"provider"`
+ // SystemFingerprint identifies the upstream system configuration, when provided.
SystemFingerprint string `json:"system_fingerprint,omitempty"`
Choices []Choice `json:"choices"`
Usage Usage `json:"usage"`
Created int64 `json:"created"`
}
-// Choice represents a single completion choice
+// Choice represents a single completion choice.
type Choice struct {
Message ResponseMessage `json:"message"`
FinishReason string `json:"finish_reason"`
Index int `json:"index"`
+ // Logprobs preserves provider-specific log probability payloads for this choice.
Logprobs json.RawMessage `json:"logprobs,omitempty"`
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/types.go` around lines 86 - 103, The exported types
ChatResponse and Choice were extended with SystemFingerprint and Logprobs but
lack Go doc updates; add concise Go comments clarifying these fields are
optional provider metadata included for diagnostic/compatibility purposes (e.g.,
update the ChatResponse type comment to mention SystemFingerprint is optional
provider metadata and update the Choice field comment to explain Logprobs is
provider-specific, may be empty/null, and is returned only when available).
Ensure the comments appear immediately above the type/field declarations
(ChatResponse, SystemFingerprint, Choice, Logprobs) and follow existing Go doc
style.
Comment on lines
+39
to
+46
| r.closeOnce.Do(func() { | ||
| if r.stopRefresh != nil { | ||
| r.stopRefresh() | ||
| r.stopRefresh = nil | ||
| } | ||
| if r.Cache != nil { | ||
| r.closeErr = r.Cache.Close() | ||
| } |
Contributor
There was a problem hiding this comment.
Wait for the refresh goroutine to stop before tearing down the cache.
stopRefresh() only cancels the background loop; it does not wait for StartBackgroundRefresh to exit. That leaves a race where an in-flight Refresh/SaveToCache can still touch r.Cache after Line 45 closes it. Please make the stop hook blocking (cancel + wait on a done channel / sync.WaitGroup) and only close the cache after the refresher has fully exited.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/providers/init.go` around lines 39 - 46, The teardown in
r.closeOnce.Do calls r.stopRefresh() but does not wait for the background
refresher started by StartBackgroundRefresh to exit, which can race with
Refresh/SaveToCache touching r.Cache; change the stop hook used by
StartBackgroundRefresh (stopRefresh) to be blocking by making it cancel the
refresh loop and then wait on a done channel or sync.WaitGroup (e.g., have
StartBackgroundRefresh return a stop function that signals cancellation then
blocks until the loop returns), and only call r.Cache.Close() (and assign
r.closeErr) after that blocking stop returns so the refresher cannot access
r.Cache after it is closed.
| "model": "llama-3.3-70b-versatile", | ||
| "object": "chat.completion", | ||
| "provider": "", | ||
| "system_fingerprint": "fp_f8b414701e", |
Contributor
There was a problem hiding this comment.
Remove system_fingerprint from the Groq golden payloads.
internal/providers/groq/groq.go:72-84 just unmarshals the upstream response into core.ChatResponse, and the provided context explicitly notes Groq does not return system_fingerprint. With ChatResponse.SystemFingerprint tagged omitempty, this field will be absent in the serialized response, so this golden no longer matches provider reality. The same correction applies to the other Groq chat goldens updated in this PR.
Suggested fixture correction
- "provider": "",
- "system_fingerprint": "fp_f8b414701e",
+ "provider": "",
"usage": {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/contract/testdata/golden/groq/chat_with_params.golden.json` at line 18,
Remove the "system_fingerprint" field from the Groq golden payload(s) so they
match the provider's actual response; since internal/providers/groq/groq.go
unmarshals into core.ChatResponse and ChatResponse.SystemFingerprint is tagged
`omitempty` and Groq does not return that field, update the golden JSON(s)
(remove the "system_fingerprint" key/value from the groq chat golden(s) such as
chat_with_params.golden.json and any other Groq chat goldens updated in this PR)
so serialized test output matches reality.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
AGENTS.md (1)
5-12:⚠️ Potential issue | 🟡 MinorFix grammar in the intro paragraph.
Several grammar issues in the updated intro:
- "a OpenAI-compatible" → "an OpenAI-compatible"
- "provider's response" → "providers' responses"
- "pass it" → "passes them"
✏️ Suggested fix
-GoModel is a high-performance AI gateway in Go that routes requests to multiple AI model providers through a OpenAI-compatible API. +GoModel is a high-performance AI gateway in Go that routes requests to multiple AI model providers through an OpenAI-compatible API. -- GOModel accepts provider's response liberally and pass it to the user in a conservative OpenAI-compatible shape. +- GOModel accepts providers' responses liberally and passes them to the user in a conservative, OpenAI-compatible shape.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 5 - 12, Update the intro paragraph string "GoModel is a high-performance AI gateway in Go that routes requests to multiple AI model providers through a OpenAI-compatible API." to use "an OpenAI-compatible" instead of "a OpenAI-compatible", and adjust the sentence "GOModel accepts provider's response liberally and pass it to the user in a conservative OpenAI-compatible shape." to use plural forms: change "provider's response" → "providers' responses" and "pass it" → "passes them" so subject/verb and singular/plural agree.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/gomodel/main_test.go`:
- Around line 93-119: The test modifies the package-level shutdownTimeout which
can cause flakiness when tests run in parallel; protect the mutation by
acquiring a package-level mutex around the change and restore (e.g., add a
sync.Mutex named shutdownTimeoutMu and wrap the lines that save, set and
defer-restore shutdownTimeout in shutdownTimeoutMu.Lock() / Unlock()), keeping
references to TestStartApplication_StopsWaitingWhenShutdownTimesOut,
shutdownTimeout, and startApplication so the critical section is clear and tests
remain isolated.
In `@cmd/gomodel/main.go`:
- Around line 54-65: Add a short doc comment above the startApplication function
that documents its cleanup-on-failure behavior: describe that startApplication
calls application.Start(ctx, addr) and, if Start returns an error, it will
attempt a graceful shutdown by calling shutdownApplication with a timeout
(shutdownTimeout) and return a combined error (including both the original Start
error and any shutdown error); reference the lifecycleApp interface and Start
method so maintainers know this contract applies to implementations of
lifecycleApp.
In `@internal/auditlog/auditlog_test.go`:
- Around line 586-589: Remove the redundant return statements that follow
t.Fatal calls in the test file; e.g., in the nil-check block that reads if
streamEntry == nil { t.Fatal("expected non-nil stream entry"); return } remove
the trailing return (and similarly remove the extra returns after other t.Fatal
uses referenced in the comment). Ensure any checks that call t.Fatal (which
calls FailNow) do not include unreachable return statements afterward so the
tests are clean.
In `@internal/batch/store.go`:
- Around line 16-21: StoredBatch currently holds endpoint hints but you must
stop persisting gateway-only keys inside core.BatchResponse.Metadata; add
dedicated fields to StoredBatch (e.g., RequestID string and UsageLoggedAt
time.Time or *time.Time) and update internal/server/handlers.go to write those
values into the new StoredBatch fields instead of putting "request_id" and
"usage_logged_at" into BatchResponse.Metadata, strip those keys from the public
Metadata before persisting, and update load/lookup code to read the gateway-only
values from StoredBatch.RequestID and StoredBatch.UsageLoggedAt so API DTOs
never carry gateway-only state.
- Around line 75-89: The legacy fallback in deserializeBatch currently returns a
zero-value core.BatchResponse even when it has no ID; update deserializeBatch to
validate that after unmarshalling into legacy (core.BatchResponse) legacy.ID is
non-empty and return an error (e.g., fmt.Errorf("legacy batch missing ID")) if
it's empty, so only valid batches are accepted; keep the existing
StoredBatch/legacy unmarshalling flow and ensure the returned value is
&StoredBatch{Batch: &legacy} only when legacy.ID != "".
In `@internal/core/errors.go`:
- Around line 182-191: The 401 handling uses NewAuthenticationError which
doesn't preserve originalErr, so errors.Unwrap can't access the upstream error;
change the 401 branch to construct a GatewayError (same shape as the 403 branch)
or update NewAuthenticationError to accept and set the originalErr.
Specifically, in the statusCode == http.StatusUnauthorized branch replace the
NewAuthenticationError call with a GatewayError instance that sets Type:
ErrorTypeAuthentication, Message: message, StatusCode: http.StatusUnauthorized,
Provider: provider, and Err: originalErr (or alter NewAuthenticationError to
accept originalErr and return a GatewayError with Err set).
- Around line 192-193: The 429 branch creates a RateLimitError via
NewRateLimitError(provider, message) but doesn't preserve the underlying error
for errors.Unwrap(); update the 429 handling to attach the original error (the
variable used earlier in this error flow, e.g., err) to the created gatewayErr
by either using a constructor that accepts an error or setting the Err field on
the returned *RateLimitError; specifically modify the case where statusCode ==
http.StatusTooManyRequests to call NewRateLimitError with the original error or
assign gatewayErr.(*RateLimitError).Err = err so errors.Unwrap(gatewayErr)
returns the wrapped error.
In `@internal/server/handlers_test.go`:
- Around line 2413-2504: TestBatchLifecyclePersistsAndUsesInternalEndpointHints
currently doesn't assert that a request ID is present on contexts passed to the
provider; add assertions calling core.GetRequestID(mock.capturedBatchCtx) after
the create phase and core.GetRequestID(mock.capturedBatchHintsCtx) after the
results phase and fail the test if either returns an empty/zero value so the
test verifies the handler injects or preserves request tracing for provider
calls.
In `@internal/server/handlers.go`:
- Around line 1097-1105: The batch provider calls
(CreateBatchWithHints/CreateBatch in the handler and the similar calls in
CancelBatch and BatchResults) must always carry a real request ID: implement a
helper (e.g., requestIDFromContextOrHeader) that reads X-Request-ID from the
context or request header, if empty generates uuid.NewString(), writes that
value back into the request header and into a new context value, and returns the
new context and ID; then ensure every provider invocation
(hintedRouter.CreateBatchWithHints, nativeRouter.CreateBatch, CancelBatch calls,
and BatchResults calls) uses the returned context from this helper so
middleware-generated IDs are preserved, header-only IDs aren't lost, and a UUID
is auto-generated when missing for audit and provider propagation.
---
Duplicate comments:
In `@AGENTS.md`:
- Around line 5-12: Update the intro paragraph string "GoModel is a
high-performance AI gateway in Go that routes requests to multiple AI model
providers through a OpenAI-compatible API." to use "an OpenAI-compatible"
instead of "a OpenAI-compatible", and adjust the sentence "GOModel accepts
provider's response liberally and pass it to the user in a conservative
OpenAI-compatible shape." to use plural forms: change "provider's response" →
"providers' responses" and "pass it" → "passes them" so subject/verb and
singular/plural agree.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b1cb19d9-fa8e-4cfe-8abe-42446602e09a
📒 Files selected for processing (25)
AGENTS.mdCLAUDE.mdcmd/gomodel/main.gocmd/gomodel/main_test.gointernal/auditlog/auditlog_test.gointernal/auditlog/reader_mongodb_test.gointernal/batch/store.gointernal/batch/store_memory.gointernal/batch/store_memory_test.gointernal/batch/store_mongodb.gointernal/batch/store_postgresql.gointernal/batch/store_sqlite.gointernal/batch/store_sqlite_test.gointernal/batch/store_test.gointernal/core/errors.gointernal/core/errors_test.gointernal/core/interfaces.gointernal/core/semantic_test.gointernal/llmclient/client.gointernal/llmclient/client_test.gointernal/providers/anthropic/anthropic.gointernal/providers/router.gointernal/providers/router_test.gointernal/server/handlers.gointernal/server/handlers_test.go
Comment on lines
+93
to
+119
| func TestStartApplication_StopsWaitingWhenShutdownTimesOut(t *testing.T) { | ||
| previousTimeout := shutdownTimeout | ||
| shutdownTimeout = 10 * time.Millisecond | ||
| defer func() { | ||
| shutdownTimeout = previousTimeout | ||
| }() | ||
|
|
||
| startErr := errors.New("listen failed") | ||
| shutdownBlock := make(chan struct{}) | ||
| defer close(shutdownBlock) | ||
|
|
||
| app := &stubLifecycleApp{ | ||
| startErr: startErr, | ||
| shutdownBlock: shutdownBlock, | ||
| } | ||
|
|
||
| err := startApplication(app, ":8080") | ||
| if !errors.Is(err, startErr) { | ||
| t.Fatalf("error = %v, want start error %v", err, startErr) | ||
| } | ||
| if !errors.Is(err, context.DeadlineExceeded) { | ||
| t.Fatalf("error = %v, want context deadline exceeded", err) | ||
| } | ||
| if app.shutdownCalls != 1 { | ||
| t.Fatalf("shutdownCalls = %d, want 1", app.shutdownCalls) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test modifies package-level variable—consider isolation.
The test modifies the package-level shutdownTimeout variable. While the defer restore ensures cleanup, this could cause flaky tests if tests run in parallel. Consider using t.Setenv pattern or documenting that these tests must not use t.Parallel().
📝 Alternative: Add comment to prevent parallel execution
func TestStartApplication_StopsWaitingWhenShutdownTimesOut(t *testing.T) {
+ // This test modifies package-level shutdownTimeout; do not use t.Parallel()
previousTimeout := shutdownTimeout
shutdownTimeout = 10 * time.Millisecond📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestStartApplication_StopsWaitingWhenShutdownTimesOut(t *testing.T) { | |
| previousTimeout := shutdownTimeout | |
| shutdownTimeout = 10 * time.Millisecond | |
| defer func() { | |
| shutdownTimeout = previousTimeout | |
| }() | |
| startErr := errors.New("listen failed") | |
| shutdownBlock := make(chan struct{}) | |
| defer close(shutdownBlock) | |
| app := &stubLifecycleApp{ | |
| startErr: startErr, | |
| shutdownBlock: shutdownBlock, | |
| } | |
| err := startApplication(app, ":8080") | |
| if !errors.Is(err, startErr) { | |
| t.Fatalf("error = %v, want start error %v", err, startErr) | |
| } | |
| if !errors.Is(err, context.DeadlineExceeded) { | |
| t.Fatalf("error = %v, want context deadline exceeded", err) | |
| } | |
| if app.shutdownCalls != 1 { | |
| t.Fatalf("shutdownCalls = %d, want 1", app.shutdownCalls) | |
| } | |
| } | |
| func TestStartApplication_StopsWaitingWhenShutdownTimesOut(t *testing.T) { | |
| // This test modifies package-level shutdownTimeout; do not use t.Parallel() | |
| previousTimeout := shutdownTimeout | |
| shutdownTimeout = 10 * time.Millisecond | |
| defer func() { | |
| shutdownTimeout = previousTimeout | |
| }() | |
| startErr := errors.New("listen failed") | |
| shutdownBlock := make(chan struct{}) | |
| defer close(shutdownBlock) | |
| app := &stubLifecycleApp{ | |
| startErr: startErr, | |
| shutdownBlock: shutdownBlock, | |
| } | |
| err := startApplication(app, ":8080") | |
| if !errors.Is(err, startErr) { | |
| t.Fatalf("error = %v, want start error %v", err, startErr) | |
| } | |
| if !errors.Is(err, context.DeadlineExceeded) { | |
| t.Fatalf("error = %v, want context deadline exceeded", err) | |
| } | |
| if app.shutdownCalls != 1 { | |
| t.Fatalf("shutdownCalls = %d, want 1", app.shutdownCalls) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/gomodel/main_test.go` around lines 93 - 119, The test modifies the
package-level shutdownTimeout which can cause flakiness when tests run in
parallel; protect the mutation by acquiring a package-level mutex around the
change and restore (e.g., add a sync.Mutex named shutdownTimeoutMu and wrap the
lines that save, set and defer-restore shutdownTimeout in
shutdownTimeoutMu.Lock() / Unlock()), keeping references to
TestStartApplication_StopsWaitingWhenShutdownTimesOut, shutdownTimeout, and
startApplication so the critical section is clear and tests remain isolated.
Comment on lines
586
to
589
| if streamEntry == nil { | ||
| t.Fatal("expected non-nil stream entry") | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
Does Go's testing.T.Fatal stop execution of the current test immediately via FailNow, making a following return redundant?
💡 Result:
Yes. t.Fatal(...) is defined as Log(...) followed by FailNow() [1]. FailNow() “stops its execution by calling runtime.Goexit” (running deferred calls in the current goroutine), and execution then continues with the next test/benchmark [1]. So a return immediately after t.Fatal(...) is runtime-redundant in the same test goroutine.
Two caveats:
FailNow/Fatalmust be called from the goroutine running the test; it won’t stop other goroutines you started [1].- A
returnmay still be useful/necessary to satisfy the compiler in functions with return values, sincet.Fataldoesn’t have a “never returns” type signature.
Source: Go testing package docs [1].
Remove the redundant return statements after t.Fatal.
testing.T.Fatal aborts the current test via FailNow, making these return statements unreachable dead code.
Also applies to: lines 615-618, 621-624
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/auditlog/auditlog_test.go` around lines 586 - 589, Remove the
redundant return statements that follow t.Fatal calls in the test file; e.g., in
the nil-check block that reads if streamEntry == nil { t.Fatal("expected non-nil
stream entry"); return } remove the trailing return (and similarly remove the
extra returns after other t.Fatal uses referenced in the comment). Ensure any
checks that call t.Fatal (which calls FailNow) do not include unreachable return
statements afterward so the tests are clean.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/providers/registry.go (1)
878-885:⚠️ Potential issue | 🟡 MinorDon’t warn on expected shutdown cancellation.
cancel()now intentionally aborts in-flight refreshes. IfRefreshormodeldata.Fetchreturnscontext.Canceledduring normal shutdown, these warnings will read like real failures in logs. Please short-circuit or downgrade the canceled case instead of warning.Also applies to: 910-913
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/providers/registry.go` around lines 878 - 885, The current background refresh logging warns on all errors including expected shutdown cancellations; update the refresh flow around refreshCtx/refreshCancel and the calls to r.Refresh(refreshCtx) and r.SaveToCache(refreshCtx) (and similar handling near modeldata.Fetch) to detect context.Canceled and context.DeadlineExceeded and avoid emitting warnings for those cases—either short-circuit returning nil or log at debug/info level instead; keep warning logs for genuine errors only and ensure refreshCancel is still deferred/used correctly.internal/providers/anthropic/anthropic.go (1)
1125-1147:⚠️ Potential issue | 🟠 MajorReject duplicate
custom_idvalues before persisting hints.The new hint map is keyed only by
custom_id, so duplicate IDs silently overwrite earlier endpoints here. A mixed/v1/responses+/v1/chat/completionsbatch with the samecustom_idwill later decode at least one result against the wrong schema.🛡️ Minimal guard
func (p *Provider) createBatch(ctx context.Context, req *core.BatchRequest) (*core.BatchResponse, map[string]string, error) { anthropicReq, endpointByCustomID, err := buildAnthropicBatchCreateRequest(req) if err != nil { return nil, nil, err } + if len(endpointByCustomID) != len(anthropicReq.Requests) { + return nil, nil, core.NewInvalidRequestError("anthropic batch custom_id values must be unique", nil) + } var resp anthropicBatchResponse🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/providers/anthropic/anthropic.go` around lines 1125 - 1147, createBatch currently accepts endpointByCustomID (from buildAnthropicBatchCreateRequest) that uses custom_id as the map key, allowing duplicates to overwrite earlier entries; before calling setBatchResultEndpoints and persisting hints, detect duplicate custom_id values (from the incoming req/batch items or the returned endpointByCustomID) and return a clear error (e.g., a provider error) when any duplicate custom_id is found so batches with mixed schemas cannot collide. Update createBatch to validate uniqueness of custom_id, reference endpointByCustomID and setBatchResultEndpoints when implementing the check, and fail fast with an explanatory error if duplicates exist.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/core/errors_test.go`:
- Around line 541-572: Update the table-driven test
TestParseProviderError_PreservesWrappedErrorsForAuthAndRateLimit to include a
403 case so the forbidden branch in ParseProviderError is exercised; add an
entry with name "403 forbidden", statusCode http.StatusForbidden, and wantType
ErrorTypeForbidden, then assert the returned error.Type equals
ErrorTypeForbidden and that errors.Is(err, originalErr) is true just like the
existing 401/429 cases.
In `@internal/providers/anthropic/anthropic.go`:
- Around line 1263-1267: The code currently treats an empty map the same as nil,
causing an explicit empty persisted hints map to be replaced by transient hints;
change the branch in GetBatchResultsWithHints to only call
p.getBatchResultEndpoints(id) when endpointByCustomID == nil, and otherwise
clone endpointByCustomID via cloneBatchResultEndpoints so an explicitly empty
map remains distinct from nil and won't pick up stale in-memory hints.
In `@internal/providers/registry_test.go`:
- Around line 523-564: Update the CancelWaitsForInFlightRefreshToExit test to
actually verify that StartBackgroundRefresh's cancel waits for an in-flight
ListModels to finish: modify the mock provider (registryMockProvider or
countingRegistryMockProvider) so ListModels blocks on a test-controlled channel
after receiving <-ctx.Done(), start the background refresh and ensure it has
begun, call cancel() in a goroutine and assert that cancel() does not return
until you close/unblock that channel, then release the channel and assert
cancel() returns quickly; additionally add a subcase that exercises the shutdown
path when modelListURL != "" to cover that branch. Ensure you reference the test
name CancelWaitsForInFlightRefreshToExit, StartBackgroundRefresh, and ListModels
when locating where to change the mocks and assertions.
In `@internal/providers/registry.go`:
- Around line 897-901: Update the Go doc for StartBackgroundRefresh to document
that the returned stop function is blocking: explain that calling the returned
function will cancel the refresh goroutine and wait for it to exit (it uses
stopOnce.Do, cancel(), and receives from done), so callers should expect it to
block during shutdown; mention that this is a behavioral change to the exported
API and add the note to the function's top-level comment near
StartBackgroundRefresh.
---
Outside diff comments:
In `@internal/providers/anthropic/anthropic.go`:
- Around line 1125-1147: createBatch currently accepts endpointByCustomID (from
buildAnthropicBatchCreateRequest) that uses custom_id as the map key, allowing
duplicates to overwrite earlier entries; before calling setBatchResultEndpoints
and persisting hints, detect duplicate custom_id values (from the incoming
req/batch items or the returned endpointByCustomID) and return a clear error
(e.g., a provider error) when any duplicate custom_id is found so batches with
mixed schemas cannot collide. Update createBatch to validate uniqueness of
custom_id, reference endpointByCustomID and setBatchResultEndpoints when
implementing the check, and fail fast with an explanatory error if duplicates
exist.
In `@internal/providers/registry.go`:
- Around line 878-885: The current background refresh logging warns on all
errors including expected shutdown cancellations; update the refresh flow around
refreshCtx/refreshCancel and the calls to r.Refresh(refreshCtx) and
r.SaveToCache(refreshCtx) (and similar handling near modeldata.Fetch) to detect
context.Canceled and context.DeadlineExceeded and avoid emitting warnings for
those cases—either short-circuit returning nil or log at debug/info level
instead; keep warning logs for genuine errors only and ensure refreshCancel is
still deferred/used correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f1e98276-fb7a-4151-9bf3-58f00f8b65a6
📒 Files selected for processing (12)
AGENTS.mdcmd/gomodel/main.gointernal/batch/store.gointernal/batch/store_test.gointernal/core/errors.gointernal/core/errors_test.gointernal/providers/anthropic/anthropic.gointernal/providers/anthropic/anthropic_test.gointernal/providers/registry.gointernal/providers/registry_test.gointernal/server/handlers.gointernal/server/handlers_test.go
Comment on lines
+541
to
+572
| func TestParseProviderError_PreservesWrappedErrorsForAuthAndRateLimit(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| statusCode int | ||
| wantType ErrorType | ||
| }{ | ||
| { | ||
| name: "401 unauthorized", | ||
| statusCode: http.StatusUnauthorized, | ||
| wantType: ErrorTypeAuthentication, | ||
| }, | ||
| { | ||
| name: "429 rate limit", | ||
| statusCode: http.StatusTooManyRequests, | ||
| wantType: ErrorTypeRateLimit, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| originalErr := errors.New("upstream transport error") | ||
| err := ParseProviderError("openai", tt.statusCode, []byte(`{"error":{"message":"boom"}}`), originalErr) | ||
|
|
||
| if err.Type != tt.wantType { | ||
| t.Fatalf("Type = %v, want %v", err.Type, tt.wantType) | ||
| } | ||
| if !errors.Is(err, originalErr) { | ||
| t.Fatalf("expected wrapped original error, got %v", err) | ||
| } | ||
| }) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Cover the 403 wrapped-error path too.
ParseProviderError now has a separate forbidden branch, but this table only locks down errors.Is behavior for 401 and 429. Add a 403 case here so a regression doesn't silently drop the wrapped upstream error again.
🧪 Minimal addition
{
name: "401 unauthorized",
statusCode: http.StatusUnauthorized,
wantType: ErrorTypeAuthentication,
},
+ {
+ name: "403 forbidden",
+ statusCode: http.StatusForbidden,
+ wantType: ErrorTypeAuthentication,
+ },
{
name: "429 rate limit",
statusCode: http.StatusTooManyRequests,
wantType: ErrorTypeRateLimit,
},As per coding guidelines **/*_test.go: Add or update tests for any behavior change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/errors_test.go` around lines 541 - 572, Update the table-driven
test TestParseProviderError_PreservesWrappedErrorsForAuthAndRateLimit to include
a 403 case so the forbidden branch in ParseProviderError is exercised; add an
entry with name "403 forbidden", statusCode http.StatusForbidden, and wantType
ErrorTypeForbidden, then assert the returned error.Type equals
ErrorTypeForbidden and that errors.Is(err, originalErr) is true just like the
existing 401/429 cases.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/providers/registry_test.go`:
- Around line 604-653: The test currently calls cancel() synchronously so it
doesn't assert blocking behavior; change the test in the "ModelListFetch"
subtest to invoke cancel() inside a goroutine and then assert that the goroutine
does not return until the fetch completes by waiting for fetchCanceled before
allowing the goroutine to finish (e.g., start a goroutine that calls cancel(),
then wait for the fetchCanceled channel and finally ensure the cancel goroutine
has returned). Update references around StartBackgroundRefresh, cancel,
fetchStarted and fetchCanceled to implement this blocking verification.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: eee94d08-8bb8-4192-8a3b-00d887054062
📒 Files selected for processing (5)
internal/core/errors_test.gointernal/providers/anthropic/anthropic.gointernal/providers/anthropic/anthropic_test.gointernal/providers/registry.gointernal/providers/registry_test.go
Comment on lines
+604
to
+653
| t.Run("ModelListFetch", func(t *testing.T) { | ||
| fetchStarted := make(chan struct{}, 1) | ||
| fetchCanceled := make(chan struct{}, 1) | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| select { | ||
| case fetchStarted <- struct{}{}: | ||
| default: | ||
| } | ||
| <-r.Context().Done() | ||
| select { | ||
| case fetchCanceled <- struct{}{}: | ||
| default: | ||
| } | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| var refreshCount atomic.Int32 | ||
| mock := ®istryMockProvider{ | ||
| name: "test", | ||
| modelsResponse: &core.ModelsResponse{ | ||
| Object: "list", | ||
| Data: []core.Model{ | ||
| {ID: "test-model", Object: "model", OwnedBy: "test"}, | ||
| }, | ||
| }, | ||
| } | ||
| countingMock := &countingRegistryMockProvider{ | ||
| registryMockProvider: mock, | ||
| listCount: &refreshCount, | ||
| } | ||
|
|
||
| registry := NewModelRegistry() | ||
| registry.RegisterProvider(countingMock) | ||
| _ = registry.Initialize(context.Background()) | ||
|
|
||
| cancel := registry.StartBackgroundRefresh(10*time.Millisecond, server.URL) | ||
| select { | ||
| case <-fetchStarted: | ||
| case <-time.After(2 * time.Second): | ||
| t.Fatal("expected StartBackgroundRefresh to begin model list fetch") | ||
| } | ||
|
|
||
| cancel() | ||
|
|
||
| select { | ||
| case <-fetchCanceled: | ||
| case <-time.After(500 * time.Millisecond): | ||
| t.Fatal("expected model list fetch to be canceled during shutdown") | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
This test doesn't verify that cancel() blocks; it would pass with the old non-blocking implementation.
The cancel() on line 646 is called synchronously, and the test only checks that fetchCanceled eventually fires. With the old non-blocking cancel(), the context would still be canceled and the fetch would eventually complete—the test would pass either way.
To properly verify blocking behavior (consistent with the ListModels subtest), call cancel() in a goroutine and assert it doesn't return until the fetch completes:
🧪 Suggested fix to verify blocking behavior
cancel := registry.StartBackgroundRefresh(10*time.Millisecond, server.URL)
select {
case <-fetchStarted:
case <-time.After(2 * time.Second):
t.Fatal("expected StartBackgroundRefresh to begin model list fetch")
}
- cancel()
+ cancelDone := make(chan struct{})
+ go func() {
+ cancel()
+ close(cancelDone)
+ }()
+
+ // Verify cancel() blocks while fetch is in progress
+ select {
+ case <-cancelDone:
+ t.Fatal("cancel() returned before in-flight fetch finished")
+ case <-time.After(50 * time.Millisecond):
+ }
select {
case <-fetchCanceled:
case <-time.After(500 * time.Millisecond):
t.Fatal("expected model list fetch to be canceled during shutdown")
}
+
+ select {
+ case <-cancelDone:
+ case <-time.After(500 * time.Millisecond):
+ t.Fatal("cancel() did not return after fetch completed")
+ }As per coding guidelines: "**/*_test.go: Add or update tests for any behavior change".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/providers/registry_test.go` around lines 604 - 653, The test
currently calls cancel() synchronously so it doesn't assert blocking behavior;
change the test in the "ModelListFetch" subtest to invoke cancel() inside a
goroutine and then assert that the goroutine does not return until the fetch
completes by waiting for fetchCanceled before allowing the goroutine to finish
(e.g., start a goroutine that calls cancel(), then wait for the fetchCanceled
channel and finally ensure the cancel goroutine has returned). Update references
around StartBackgroundRefresh, cancel, fetchStarted and fetchCanceled to
implement this blocking verification.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation