feat: improve passthrough support#248
Closed
giorgi-pro wants to merge 23 commits intomainfrom
Closed
Conversation
# Conflicts: # internal/providers/router.go # internal/server/handlers_test.go # internal/server/passthrough_support.go
Contributor
📝 Walkthrough🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR refactors provider-native passthrough support from a monolithic in-handler design to a composable middleware pipeline, and replaces the coarse-grained Key changes:
Confidence Score: 5/5Safe to merge — authentication is preserved, body handling is correct, and all three comments are P2 style nits with no runtime impact. Middleware decomposition is architecturally sound. Auth is not bypassed. Body double-read via readAndRestoreBody is safe. ResolvePassthroughByName uses the registry so it works before model initialization. All comments are style-only. internal/server/passthrough_middleware.go (ignored ok), internal/providers/router.go (_ = providerType), internal/server/passthrough_provider_resolution.go (RouteProvider field) Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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.go`:
- Around line 848-862: ProviderByType is inconsistent with ProviderByName
because it compares r.providerTypes[provider] without trimming stored
whitespace; update ProviderByType to compare using
strings.TrimSpace(r.providerTypes[provider]) (keeping the existing providerType
= strings.TrimSpace(providerType) at the top) so lookup matches entries with
stray whitespace; alternatively ensure RegisterProviderWithNameAndType trims and
stores providerType trimmed, but the quickest/local fix is to trim
r.providerTypes[provider] in the ProviderByType loop to mirror ProviderByName
and other getters (reference ProviderByType, ProviderByName,
RegisterProviderWithNameAndType, providerTypes).
In `@internal/providers/router.go`:
- Around line 781-788: The PassthroughByName method currently calls
ResolvePassthroughByName, discards providerType ( _ = providerType ), and is
unused; either remove PassthroughByName or change its signature to return the
providerType so callers can store it for audit/observability. Fix options: (1)
delete Router.PassthroughByName entirely and update any references to call
ResolvePassthroughByName directly and pass providerType into context before
calling PassthroughProvider.Passthrough, or (2) modify Router.PassthroughByName
to return (resp *core.PassthroughResponse, providerType string, err error) (or
include providerType in a response struct) so callers receive providerType for
recordPassthroughAudit; ensure ResolvePassthroughByName,
PassthroughProvider.Passthrough usage, and audit recording via
recordPassthroughAudit are updated accordingly.
In `@internal/server/http.go`:
- Around line 141-146: The PassthroughGuardrailPatcher is being passed nil
because cfg.PassthroughGuardrailPatcher isn't set; update the initialization so
that if cfg.PassthroughGuardrailPatcher is nil you assign the existing
cfg.TranslatedRequestPatcher as a fallback (i.e., set
passthroughGuardrailPatcher = cfg.PassthroughGuardrailPatcher; if nil then
passthroughGuardrailPatcher = cfg.TranslatedRequestPatcher) before registering
PassthroughGuardrailsMiddleware, ensuring PassthroughGuardrailsMiddleware
receives a non-nil TranslatedRequestPatcher and passthroughDisabled remains
populated from cfg.PassthroughDisabledInstances.
In `@internal/server/passthrough_audit.go`:
- Around line 38-49: The PassthroughAuditEntry.Endpoint value is dropped when
building auditlog.LogEntry (logEntry), so persist the forwarded/normalized
upstream endpoint: add the PassthroughAuditEntry.Endpoint into the audit entry
(e.g., set an existing Endpoint/ForwardedEndpoint field on auditlog.LogEntry or
add it into a Data/Metadata map on logEntry) when creating logEntry in the
function that constructs auditlog.LogEntry; alternatively, if Endpoint is
intentionally not auditable, remove PassthroughAuditEntry.Endpoint from the
struct to avoid unused data.
In `@internal/server/passthrough_execution_helpers.go`:
- Around line 11-33: Duplicate logic: both extractPassthroughEndpoint and
passthroughExecutionTarget perform normalizePassthroughEndpoint + append raw
query; extract this behavior into a single helper (e.g., appendPassthroughQuery
or buildPassthroughEndpoint) or make passthroughExecutionTarget call
extractPassthroughEndpoint so the v1-alias handling, empty-endpoint check, and
query-suffix logic live in one place; update references in
passthroughExecutionTarget to use the new helper (or call
extractPassthroughEndpoint) and remove the duplicated lines that call
normalizePassthroughEndpoint and append RawQuery, ensuring
normalizePassthroughEndpoint, the empty-endpoint error condition, and query
concatenation are preserved exactly.
In `@internal/server/passthrough_middleware.go`:
- Around line 93-96: If readAndRestoreBody(c.Request()) returns an error, do not
call next(c); instead return a typed core.GatewayError immediately. Update the
error branch in the passthrough middleware so that when readAndRestoreBody
returns err (regardless of body length), you construct and return an appropriate
core.GatewayError (with context and status code) rather than forwarding the
request; ensure you reference readAndRestoreBody and replace the current return
next(c) path with returning the GatewayError synchronously.
- Around line 98-104: The middleware currently unmarshals every JSON body into
core.ChatRequest and always calls patcher.PatchChatRequest, which causes
/p/{provider}/responses traffic to be misrouted or fail chat-only validation;
update the logic to detect the endpoint type (chat vs responses) from the
request path or route context and only unmarshal into core.ChatRequest when the
path is the chat endpoint, calling patcher.PatchChatRequest in that case, and
for the responses endpoint unmarshal into the appropriate responses request type
and call patcher.PatchResponsesRequest instead; ensure you preserve the existing
error handling flow (return next(c) on unmarshal failure and return
handleError(c, err) on patcher errors) and reference the existing symbols
core.ChatRequest, PatchChatRequest, PatchResponsesRequest,
patcher.PatchChatRequest, and patcher.PatchResponsesRequest when making the
change.
In `@internal/server/passthrough_response_writer.go`:
- Around line 30-32: The check that returns core.NewInvalidRequestError for a
nil upstream PassthroughResponse should instead return a provider-side error;
update the error construction in the function handling resp in
passthrough_response_writer.go (the nil resp/resp.Body branch) to call
core.NewProviderError (or the appropriate provider error constructor used in
this codebase) with a descriptive message like "upstream returned empty
passthrough response" and preserve the nil/underlying error parameter as before
so the response is categorized as provider_error (502-class) rather than
invalid_request_error.
In `@internal/server/passthrough_service.go`:
- Around line 47-49: When provider.Passthrough returns an error, record a
minimal passthrough audit entry before returning so failures are included in
audit history: call recordPassthroughAudit (or the same audit helper used for
successful passthroughs) immediately after receiving an error from
provider.Passthrough and before calling handleError; derive the audit status
from the error by type-asserting to core.GatewayError (use its status/Code field
when present, fallback to a generic failure status) and include whatever
request/response identifiers are available (request ID, provider ID, route) so
the entry is lean but useful; then return handleError(c, err) as before.
In `@internal/server/translated_inference_service.go`:
- Around line 352-360: The success branch is missing two safeguards present on
the error path: guard against a nil resp.Body and set the X-GoModel-Request-ID
header. Modify the success branch in translated_inference_service.go to (1)
compute requestID via requestIDFromContextOrHeader(c.Request()) and set it on
the response header (use goModelRequestIDHeader) before returning, and (2) if
resp.Body is nil (or resp is nil or resp.StatusCode >= http.StatusBadRequest),
delegate to newRawPassthroughResponseHandler().Handle(c, requestID, resp)
instead of calling handleStreamingReadCloser; otherwise proceed to
copyPassthroughResponseHeaders and call s.handleStreamingReadCloser as before.
In `@README.md`:
- Line 205: The README examples are inconsistent: change the Anthropic example
path from `/p/anthropic1/messages` to include the version prefix so it matches
the OpenAI example (use `/p/anthropic1/v1/messages`), updating the example
string in the line that currently shows `POST /p/anthropic1/messages` so both
examples consistently include `v1/`.
- Line 245: Update the README sentence about providers and the
"/v1/chat/completions" fast path to remove the stale reference to "semantic
enrichment" and correct the workflow description: state that providers without
native passthrough (Gemini, Groq, xAI, Oracle, Ollama) can use the
"/v1/chat/completions" fast path, which operates on an already-resolved workflow
and skips the translation/adapter layers (rather than bypassing workflow
resolution or semantic enrichment).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 863e3942-bc4d-4342-87f2-fe8b34f961ed
📒 Files selected for processing (28)
CLAUDE.mdREADME.mdconfig/config.goconfig/config_test.gointernal/app/app.gointernal/core/passthrough.gointernal/providers/anthropic/anthropic.gointernal/providers/azure/azure.gointernal/providers/factory.gointernal/providers/factory_test.gointernal/providers/openai/openai.gointernal/providers/openrouter/openrouter.gointernal/providers/registry.gointernal/providers/router.gointernal/server/handlers.gointernal/server/handlers_test.gointernal/server/http.gointernal/server/passthrough_audit.gointernal/server/passthrough_execution_helpers.gointernal/server/passthrough_middleware.gointernal/server/passthrough_provider_resolution.gointernal/server/passthrough_response_writer.gointernal/server/passthrough_route_test.gointernal/server/passthrough_semantic_enrichment.gointernal/server/passthrough_semantic_enrichment_test.gointernal/server/passthrough_service.gointernal/server/passthrough_support.gointernal/server/translated_inference_service.go
💤 Files with no reviewable changes (5)
- CLAUDE.md
- internal/providers/factory_test.go
- internal/server/passthrough_semantic_enrichment_test.go
- internal/server/passthrough_semantic_enrichment.go
- config/config_test.go
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
…ariable' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
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 (7)
internal/server/passthrough_support.go (2)
11-37: 🧹 Nitpick | 🔵 TrivialNit: drop the unused
_ context.Contextparameter.Since request-id derivation now happens outside this function and the ctx is no longer consulted, the blank-named parameter is dead surface that callers still have to pass through. Either remove it or replace with a meaningful use (e.g. honouring
ctx.Err()before doing the copy).♻️ Proposed cleanup
-func buildPassthroughHeaders(_ context.Context, src http.Header, goModelRequestID string) http.Header { +func buildPassthroughHeaders(src http.Header, goModelRequestID string) http.Header {Then update the
contextimport if it is no longer referenced elsewhere in the file, and adjust callers (e.g.internal/server/passthrough_service.go).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/passthrough_support.go` around lines 11 - 37, Remove the unused context parameter from buildPassthroughHeaders: change the signature to drop the leading _ context.Context parameter, update all callers (e.g. places in internal/server/passthrough_service.go) to stop passing a context into buildPassthroughHeaders, and remove the context import from this file if it becomes unused; alternatively, if you prefer to keep context, replace the blank param by actively checking ctx.Err() before copying headers—ensure function name buildPassthroughHeaders and helper passthroughConnectionHeaders are updated consistently across call sites.
51-59: 🧹 Nitpick | 🔵 TrivialMinor: redundant canonicalization work.
canonicalKeyis computed on line 52 but never used — line 58 falls through toskipPassthroughHeader(key), which re-canonicalizes the rawkeyagain. Either passcanonicalKeydown (and haveskipPassthroughHeaderaccept an already-canonical key, or add an unexported variant that skips the work), or drop the local variable here.♻️ Minimal fix
func skipPassthroughRequestHeader(key string) bool { canonicalKey := http.CanonicalHeaderKey(strings.TrimSpace(key)) switch canonicalKey { case http.CanonicalHeaderKey(core.RequestIDHeader), http.CanonicalHeaderKey(core.UserPathHeader): return true } - return skipPassthroughHeader(key) + return skipPassthroughHeader(canonicalKey) }Also note:
http.CanonicalHeaderKey(core.RequestIDHeader)/UserPathHeaderare computed on every call — since these are compile-time constants with already-canonical values (X-GoModel-Request-ID,X-GoModel-User-Path), you could hoist them to package-levelvars or just compare against string literals.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/passthrough_support.go` around lines 51 - 59, The local canonicalKey in skipPassthroughRequestHeader is redundant because skipPassthroughHeader re-canonicalizes the key; either pass canonicalKey into skipPassthroughHeader (change its signature or add an unexported helper like skipPassthroughHeaderCanonical that accepts an already-canonical key) or remove canonicalKey and call skipPassthroughHeader with the original key to avoid wasted work; additionally, if core.RequestIDHeader and core.UserPathHeader are constant canonical strings, hoist their http.CanonicalHeaderKey(...) results to package-level vars or compare directly to the known canonical literals to avoid recomputing them on every call.internal/auditlog/entry_capture_test.go (1)
42-46: 🧹 Nitpick | 🔵 TrivialNit: stale header name in failure messages.
Assertions now read the canonicalized
core.RequestIDHeaderkey, but severalt.Fatalfmessages still hard-code"RequestHeaders[X-Request-ID]"/"ResponseHeaders[X-Request-ID]"(lines 43, 52, 76, 82, 106, 112, 187, 190, 254). Ifcore.RequestIDHeaderis ever renamed, these messages will silently drift out of sync with what's actually being asserted. Consider formatting the header name dynamically via%sandcore.RequestIDHeader, matching the style already used forcore.UserPathHeaderon line 46.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auditlog/entry_capture_test.go` around lines 42 - 46, Update the test failure messages that hard-code "RequestHeaders[X-Request-ID]" / "ResponseHeaders[X-Request-ID]" to dynamically format the header name using core.RequestIDHeader (and http.CanonicalHeaderKey(core.RequestIDHeader) where appropriate) so the message matches the asserted key; locate assertions that reference entry.Data.RequestHeaders and entry.Data.ResponseHeaders (e.g., the checks using http.CanonicalHeaderKey(core.RequestIDHeader)) and change their t.Fatalf format strings to use %s with core.RequestIDHeader instead of the literal "X-Request-ID".tests/integration/auditlog_test.go (1)
5-14:⚠️ Potential issue | 🔴 CriticalAdd the missing
coreimport.These new
core.RequestIDHeaderreferences at lines 32, 86, 129, 160, 194, 227, 262, 293, 339, and 381 cause a compilation error:undefined: core.🔧 Proposed fix
import ( "io" "testing" "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "gomodel/internal/core" "gomodel/tests/integration/dbassert" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/auditlog_test.go` around lines 5 - 14, The test file references core.RequestIDHeader but never imports the core package; add the missing import for the core package (e.g. "gomodel/core") to the import block so symbols like core.RequestIDHeader resolve; update the import list in tests/integration/auditlog_test.go to include the core package alongside existing imports so all references to core.RequestIDHeader compile.tests/integration/usage_test.go (1)
5-14:⚠️ Potential issue | 🔴 CriticalAdd the missing
coreimport.The new
core.RequestIDHeaderusages are unresolved in this file becausegomodel/internal/coreis not imported.🔧 Proposed fix
import ( "io" "testing" "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "gomodel/internal/core" "gomodel/tests/integration/dbassert" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/usage_test.go` around lines 5 - 14, The test file is missing the internal core import so usages of core.RequestIDHeader are unresolved; add the import for gomodel/internal/core to the import block in the file (or import it with an alias if you prefer) so references to core.RequestIDHeader compile, then run `go test` to verify the unresolved identifier is fixed.internal/server/translated_inference_service.go (1)
337-347:⚠️ Potential issue | 🟠 MajorPass the generated request ID into upstream passthrough headers.
buildPassthroughHeadersstrips client/internal request ID headers and only injects the third argument. Passing""means fast-path streaming requests losecore.RequestIDHeaderupstream, unlike regular passthrough.🛠️ Proposed fix
- ctx, _ := requestContextWithRequestID(c.Request()) + ctx, requestID := requestContextWithRequestID(c.Request()) c.SetRequest(c.Request().WithContext(ctx)) const endpoint = "/chat/completions" providerType := strings.TrimSpace(workflow.ProviderType) resp, err := passthroughProvider.Passthrough(ctx, providerType, &core.PassthroughRequest{ Method: c.Request().Method, Endpoint: endpoint, Body: c.Request().Body, - Headers: buildPassthroughHeaders(ctx, c.Request().Header, ""), + Headers: buildPassthroughHeaders(ctx, c.Request().Header, requestID), }) if err != nil { return true, handleError(c, err) } - requestID := requestIDFromContextOrHeader(c.Request()) - if resp == nil || resp.Body == nil || resp.StatusCode >= http.StatusBadRequest {Also applies to: 352-352
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/translated_inference_service.go` around lines 337 - 347, The passthrough call is losing the generated request ID because buildPassthroughHeaders is being called with an empty string; change the call to pass the request ID created by requestContextWithRequestID so upstream receives core.RequestIDHeader. Specifically, extract the request ID from the context returned by requestContextWithRequestID (used when creating ctx in requestContextWithRequestID(c.Request())), and pass that value as the third argument to buildPassthroughHeaders in the calls around translated_inference_service.go (the lines invoking buildPassthroughHeaders before passthroughProvider.Passthrough) so the generated request ID is preserved in upstream headers.internal/server/http.go (1)
190-218:⚠️ Potential issue | 🟡 MinorFix request logger to capture the generated request ID.
The request logger middleware (lines 190–221) runs before the middleware that stamps
X-GoModel-Request-ID(lines 236–243). Echo'sv.RequestIDreads from the request'sX-Request-IDheader, which won't contain the generated ID. The structuredrequest_idfield will be empty in REQUEST logs.Proposed fix
LogValuesFunc: func(c *echo.Context, v middleware.RequestLoggerValues) error { + requestID := c.Request().Header.Get(core.RequestIDHeader) + if requestID == "" { + requestID = v.RequestID + } slog.Info("REQUEST", "method", v.Method, "uri", v.URI, "status", v.Status, "latency", v.Latency.String(), "host", v.Host, "bytes_in", v.ContentLength, "bytes_out", v.ResponseSize, "user_agent", v.UserAgent, "remote_ip", v.RemoteIP, - "request_id", v.RequestID, + "request_id", requestID, ) return nil },Also applies to: 234–240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/http.go` around lines 190 - 218, Update the RequestLogger's LogValuesFunc used in middleware.RequestLoggerWithConfig to capture the generated request ID by reading c.Request().Header.Get("X-GoModel-Request-ID") and falling back to v.RequestID if the header is empty, then log that value as "request_id"; apply the same change to the other RequestLoggerConfig usage as well so the structured logs include the generated X-GoModel-Request-ID even when it is stamped after the middleware.
♻️ Duplicate comments (1)
internal/server/passthrough_middleware.go (1)
89-100:⚠️ Potential issue | 🟠 MajorResolve the two outstanding guardrail correctness gaps.
Line 90 still forwards after
readAndRestoreBodyfails, and Lines 94-99 still decode every passthrough payload ascore.ChatRequest. This can forward an unrestored body and misapply chat guardrails to/responsestraffic.🛠️ Suggested shape
body, err := readAndRestoreBody(c.Request()) - if err != nil || len(body) == 0 { + if err != nil { + return handleError(c, core.NewInvalidRequestError("failed to read request body", err)) + } + if len(body) == 0 { return next(c) } - var chatReq core.ChatRequest - if jsonErr := json.Unmarshal(body, &chatReq); jsonErr != nil { - return next(c) - } - - if _, err := patcher.PatchChatRequest(c.Request().Context(), &chatReq); err != nil { - return handleError(c, err) + _, endpoint, _ := core.ParseProviderPassthroughPath(c.Request().URL.Path) + endpoint = strings.TrimPrefix(strings.Trim(endpoint, "/"), "v1/") + + switch endpoint { + case "chat/completions": + var chatReq core.ChatRequest + if jsonErr := json.Unmarshal(body, &chatReq); jsonErr != nil { + return next(c) + } + if _, err := patcher.PatchChatRequest(c.Request().Context(), &chatReq); err != nil { + return handleError(c, err) + } + case "responses": + var responsesReq core.ResponsesRequest + if jsonErr := json.Unmarshal(body, &responsesReq); jsonErr != nil { + return next(c) + } + if _, err := patcher.PatchResponsesRequest(c.Request().Context(), &responsesReq); err != nil { + return handleError(c, err) + } }As per coding guidelines, “Do not hide work in detached goroutines; respect context synchronously and return typed
core.GatewayErrorvalues”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/passthrough_middleware.go` around lines 89 - 100, If readAndRestoreBody(c.Request()) fails or returns an empty body, do not blindly call next(c) with an unrestored request; instead wrap the underlying error as a core.GatewayError (or convert it) and return handleError(c, wrappedErr) so the middleware fails synchronously rather than forwarding bad state. Also stop decoding every passthrough payload as core.ChatRequest: only attempt json.Unmarshal into core.ChatRequest and call patcher.PatchChatRequest when the incoming request is actually the chat endpoint (check the request path/route via c.Path() or c.Request().URL.Path or inspect a distinguishing field in the JSON), otherwise skip unmarshalling/patching and call next(c). Ensure any error returned to handleError is a typed core.GatewayError (wrap non-typed errors before returning).
🤖 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/auditlog/auditlog.go`:
- Around line 91-92: Add a setter that populates LogData.PassthroughEndpoint for
passthrough requests and call it from the enrichment path: implement a function
(e.g., EnrichEntryWithPassthroughEndpoint) that accepts the audit LogData (or
entry) and the workflow object, extracts the endpoint from workflow.Passthrough
(falling back to request path if necessary) and assigns it to
LogData.PassthroughEndpoint, then invoke this new function from
enrichEntryWithWorkflow (or the passthrough branch where workflow.Passthrough is
available) before the audit entry is written so the field is no longer left
empty.
In `@internal/providers/router.go`:
- Around line 798-814: The direct registry casts and resolver calls should be
replaced with the router fallback helpers so lookups that only implement
ListModelsWithProvider still resolve correctly: call the router helper
ResolvePassthroughByName(instanceName) to obtain the provider (and error)
instead of casting r.lookup to providerNameRegistry, then validate it implements
core.PassthroughProvider (pp) as before; for provider type, use the router
helper that resolves provider type for a name (instead of directly casting to
core.ProviderNameTypeResolver and calling GetProviderTypeForName) so
providerType is populated via the router fallback logic. Ensure you update the
code paths that currently set provider, pp, and providerType to use these
helpers and preserve the same error handling (core.NewInvalidRequestError) when
resolution fails.
In `@internal/server/handlers_test.go`:
- Around line 785-788: The mock ResolvePassthroughByName currently returns the
instanceName instead of the concrete provider type; change
mockProvider.ResolvePassthroughByName to call and return
GetProviderTypeForName(instanceName) (or the equivalent provider type lookup
used in production) as the second return value so it mirrors production behavior
and exposes mismatches between instance name and provider type; update any
related tests if they assert on the returned string.
In `@internal/server/passthrough_middleware.go`:
- Around line 36-38: The audit peek currently ignores errors from
readAndRestoreBody causing a closed/consumed request body to be forwarded;
update the passthroughBestEffortModel/audit peek flow to check the error
returned by readAndRestoreBody and, on any failure, return a typed client error
(an instance of core.GatewayError) instead of discarding it; locate the call to
readAndRestoreBody (used by passthroughBestEffortModel or the audit peek block)
and replace the ignored error path with code that wraps/constructs a
core.GatewayError (using your project's factory/constructor for GatewayError)
and returns it to the caller so processing stops and the client receives a
proper GatewayError.
In `@internal/server/passthrough_provider_resolution.go`:
- Around line 17-45: The typed key passthroughContextKey is ineffective because
you convert it to string for echo's map; replace the custom type with plain
string constants and use those in setPassthroughResolution,
getPassthroughInstanceName, getPassthroughProviderType, and
getPassthroughProvider (e.g., declare const passthroughInstanceNameKey =
"passthrough-instance-name" etc.) so the code consistently uses simple string
keys, or alternatively refactor these functions to store/retrieve values on the
request context via context.WithValue/context.Value using the typed
passthroughContextKey to get real collision protection—apply one of these two
approaches and update the Set/Get calls accordingly.
- Around line 32-35: getPassthroughInstanceName is unused and triggers the
linter; either add a real call site that reads the context value or remove the
unused API surface: delete the getPassthroughInstanceName function, remove the
passthroughInstanceNameKey constant, and remove the c.Set(...,
passthroughInstanceNameKey) write in setPassthroughResolution if nothing else
reads it; if you intend to keep the accessor, instead add a call that uses
getPassthroughInstanceName(c) where the passthrough instance name is needed so
the symbol is referenced.
In `@internal/server/passthrough_response_writer.go`:
- Around line 44-51: After calling c.Response().WriteHeader(resp.StatusCode) in
the passthrough response path, do not propagate stream copy errors up to
handleError; instead catch errors from flushStream(...) and io.Copy(...), log
them (including context: response status and any error) and return nil so Echo
will not attempt to write an error body after headers have been committed;
update the logic around isSSEContentType, flushStream, and io.Copy calls to
perform this logging-and- swallow behavior.
---
Outside diff comments:
In `@internal/auditlog/entry_capture_test.go`:
- Around line 42-46: Update the test failure messages that hard-code
"RequestHeaders[X-Request-ID]" / "ResponseHeaders[X-Request-ID]" to dynamically
format the header name using core.RequestIDHeader (and
http.CanonicalHeaderKey(core.RequestIDHeader) where appropriate) so the message
matches the asserted key; locate assertions that reference
entry.Data.RequestHeaders and entry.Data.ResponseHeaders (e.g., the checks using
http.CanonicalHeaderKey(core.RequestIDHeader)) and change their t.Fatalf format
strings to use %s with core.RequestIDHeader instead of the literal
"X-Request-ID".
In `@internal/server/http.go`:
- Around line 190-218: Update the RequestLogger's LogValuesFunc used in
middleware.RequestLoggerWithConfig to capture the generated request ID by
reading c.Request().Header.Get("X-GoModel-Request-ID") and falling back to
v.RequestID if the header is empty, then log that value as "request_id"; apply
the same change to the other RequestLoggerConfig usage as well so the structured
logs include the generated X-GoModel-Request-ID even when it is stamped after
the middleware.
In `@internal/server/passthrough_support.go`:
- Around line 11-37: Remove the unused context parameter from
buildPassthroughHeaders: change the signature to drop the leading _
context.Context parameter, update all callers (e.g. places in
internal/server/passthrough_service.go) to stop passing a context into
buildPassthroughHeaders, and remove the context import from this file if it
becomes unused; alternatively, if you prefer to keep context, replace the blank
param by actively checking ctx.Err() before copying headers—ensure function name
buildPassthroughHeaders and helper passthroughConnectionHeaders are updated
consistently across call sites.
- Around line 51-59: The local canonicalKey in skipPassthroughRequestHeader is
redundant because skipPassthroughHeader re-canonicalizes the key; either pass
canonicalKey into skipPassthroughHeader (change its signature or add an
unexported helper like skipPassthroughHeaderCanonical that accepts an
already-canonical key) or remove canonicalKey and call skipPassthroughHeader
with the original key to avoid wasted work; additionally, if
core.RequestIDHeader and core.UserPathHeader are constant canonical strings,
hoist their http.CanonicalHeaderKey(...) results to package-level vars or
compare directly to the known canonical literals to avoid recomputing them on
every call.
In `@internal/server/translated_inference_service.go`:
- Around line 337-347: The passthrough call is losing the generated request ID
because buildPassthroughHeaders is being called with an empty string; change the
call to pass the request ID created by requestContextWithRequestID so upstream
receives core.RequestIDHeader. Specifically, extract the request ID from the
context returned by requestContextWithRequestID (used when creating ctx in
requestContextWithRequestID(c.Request())), and pass that value as the third
argument to buildPassthroughHeaders in the calls around
translated_inference_service.go (the lines invoking buildPassthroughHeaders
before passthroughProvider.Passthrough) so the generated request ID is preserved
in upstream headers.
In `@tests/integration/auditlog_test.go`:
- Around line 5-14: The test file references core.RequestIDHeader but never
imports the core package; add the missing import for the core package (e.g.
"gomodel/core") to the import block so symbols like core.RequestIDHeader
resolve; update the import list in tests/integration/auditlog_test.go to include
the core package alongside existing imports so all references to
core.RequestIDHeader compile.
In `@tests/integration/usage_test.go`:
- Around line 5-14: The test file is missing the internal core import so usages
of core.RequestIDHeader are unresolved; add the import for gomodel/internal/core
to the import block in the file (or import it with an alias if you prefer) so
references to core.RequestIDHeader compile, then run `go test` to verify the
unresolved identifier is fixed.
---
Duplicate comments:
In `@internal/server/passthrough_middleware.go`:
- Around line 89-100: If readAndRestoreBody(c.Request()) fails or returns an
empty body, do not blindly call next(c) with an unrestored request; instead wrap
the underlying error as a core.GatewayError (or convert it) and return
handleError(c, wrappedErr) so the middleware fails synchronously rather than
forwarding bad state. Also stop decoding every passthrough payload as
core.ChatRequest: only attempt json.Unmarshal into core.ChatRequest and call
patcher.PatchChatRequest when the incoming request is actually the chat endpoint
(check the request path/route via c.Path() or c.Request().URL.Path or inspect a
distinguishing field in the JSON), otherwise skip unmarshalling/patching and
call next(c). Ensure any error returned to handleError is a typed
core.GatewayError (wrap non-typed errors before returning).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 82039dd4-9894-4527-8adc-fb20970add9e
📒 Files selected for processing (35)
README.mdinternal/admin/handler.gointernal/auditlog/auditlog.gointernal/auditlog/auditlog_test.gointernal/auditlog/entry_capture.gointernal/auditlog/entry_capture_test.gointernal/auditlog/middleware.gointernal/core/user_path.gointernal/providers/registry.gointernal/providers/router.gointernal/responsecache/handle_request_test.gointernal/responsecache/responsecache.gointernal/responsecache/semantic.gointernal/responsecache/simple.gointernal/responsecache/usage_hit.gointernal/server/handlers.gointernal/server/handlers_test.gointernal/server/http.gointernal/server/http_test.gointernal/server/model_validation.gointernal/server/model_validation_test.gointernal/server/passthrough_execution_helpers.gointernal/server/passthrough_middleware.gointernal/server/passthrough_provider_resolution.gointernal/server/passthrough_response_writer.gointernal/server/passthrough_route_test.gointernal/server/passthrough_service.gointernal/server/passthrough_support.gointernal/server/request_snapshot.gointernal/server/request_snapshot_test.gointernal/server/request_support.gointernal/server/translated_inference_service.gotests/integration/auditlog_test.gotests/integration/usage_test.gotests/integration/workflows_guardrails_test.go
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/auditlog/auditlog_test.go (1)
599-665: 🧹 Nitpick | 🔵 TrivialPassthrough endpoint coverage looks good.
TestMiddleware_PassthroughWorkflowUsesPassthroughModelexercises theParseProviderPassthroughPathfallback (no Raw/Normalized endpoint set → derived from/p/openai/v1/chat/completions), andTestEnrichLogEntryWithPassthroughEndpoint_PrefersNormalizedEndpointverifiesNormalizedEndpointprecedence overRawEndpoint. One small gap: neither test covers the finalep == "" && path != ""branch (non-/p/path) inEnrichLogEntryWithPassthroughEndpoint. Consider adding a case for completeness.🤖 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 599 - 665, Add a unit test that exercises the branch in EnrichLogEntryWithPassthroughEndpoint where ep == "" && path != "" by creating a LogEntry whose Path is a non-/p/ path (e.g., "/some/other/endpoint") and a Workflow with Mode = core.ExecutionModePassthrough but with Passthrough.RawEndpoint and Passthrough.NormalizedEndpoint empty; call EnrichLogEntryWithPassthroughEndpoint(entry, wf) and assert that entry.Data.PassthroughEndpoint equals the original entry.Path (e.g., "/some/other/endpoint") and that entry.Data is non-nil. This targets EnrichLogEntryWithPassthroughEndpoint and LogEntry/LogData to close the uncovered branch.internal/server/http_test.go (1)
32-58: 🧹 Nitpick | 🔵 TrivialError messages still reference hardcoded
"X-Request-ID"literal.While the header reads/writes now use
core.RequestIDHeader, the error messages on lines 34 and 58 still hardcode"X-Request-ID". If the constant value ever changes, these messages drift. Minor nit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/http_test.go` around lines 32 - 58, Update the test error messages to use the RequestID header constant instead of the hardcoded string: replace occurrences of the literal "X-Request-ID" in the failure messages in the tests inside internal/server/http_test.go (the first subtest that checks rec.Header().Get(core.RequestIDHeader) and the "preserves existing request ID" subtest that checks respID) to reference core.RequestIDHeader so the messages remain correct if the constant changes; keep the rest of the assertions (using got, respID, req, srv.ServeHTTP) unchanged.
♻️ Duplicate comments (2)
internal/server/passthrough_middleware.go (2)
94-97:⚠️ Potential issue | 🟠 MajorReturn a typed error when body restoration fails.
readAndRestoreBodycloses the original body before returning errors; forwarding withnext(c)can send a consumed body upstream and skip guardrails.🛠️ Proposed fix
body, err := readAndRestoreBody(c.Request()) - if err != nil || len(body) == 0 { + if err != nil { + return handleError(c, core.NewInvalidRequestError("failed to read request body", err)) + } + if len(body) == 0 { return next(c) }As per coding guidelines, “All errors returned to clients must be instances of
core.GatewayError”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/passthrough_middleware.go` around lines 94 - 97, When readAndRestoreBody(c.Request()) returns an error you must not call next(c) because the request body may have been closed; instead, detect err != nil and return a typed core.GatewayError wrapping the underlying error (use the project’s constructor/wrapper for core.GatewayError) from the middleware; keep the existing behavior where only len(body) == 0 triggers next(c) — i.e., change the if-check to separate err handling (return core.GatewayError) from the empty-body path (return next(c)).
99-105:⚠️ Potential issue | 🟠 MajorDispatch guardrails by passthrough endpoint type.
Every JSON passthrough body is decoded as
core.ChatRequest, so/p/{provider}/responsestraffic can hit chat-specific validation instead ofPatchResponsesRequest.🛠️ Proposed fix
- var chatReq core.ChatRequest - if jsonErr := json.Unmarshal(body, &chatReq); jsonErr != nil { - return next(c) - } - - if _, err := patcher.PatchChatRequest(c.Request().Context(), &chatReq); err != nil { - return handleError(c, err) + _, endpoint, _ := core.ParseProviderPassthroughPath(c.Request().URL.Path) + endpoint = strings.TrimPrefix(strings.Trim(endpoint, "/"), "v1/") + + switch endpoint { + case "chat/completions": + var chatReq core.ChatRequest + if jsonErr := json.Unmarshal(body, &chatReq); jsonErr != nil { + return next(c) + } + if _, err := patcher.PatchChatRequest(c.Request().Context(), &chatReq); err != nil { + return handleError(c, err) + } + case "responses": + var responsesReq core.ResponsesRequest + if jsonErr := json.Unmarshal(body, &responsesReq); jsonErr != nil { + return next(c) + } + if _, err := patcher.PatchResponsesRequest(c.Request().Context(), &responsesReq); err != nil { + return handleError(c, err) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/passthrough_middleware.go` around lines 99 - 105, The middleware currently always unmarshals the passthrough JSON into core.ChatRequest and calls patcher.PatchChatRequest, causing /p/{provider}/responses traffic to be validated as chat instead of responses; change the logic to branch by endpoint type (e.g., inspect c.Request().URL.Path or the route parameter) and when the request targets the responses endpoint unmarshal into core.PatchResponsesRequest and call patcher.PatchResponsesRequest, otherwise unmarshal into core.ChatRequest and call patcher.PatchChatRequest; preserve the existing behavior of returning next(c) on unmarshal errors and returning handleError(c, err) on patcher errors.
🤖 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/auditlog/middleware.go`:
- Around line 230-260: The EnrichLogEntryWithPassthroughEndpoint function
currently falls back to storing the entire request path when
providers.PassthroughEndpointPath returns empty and
core.ParseProviderPassthroughPath doesn't yield a usable segment; change this so
we do not store the raw /p/... path: after the existing parsing attempt, if ep
is still empty do not set PassthroughEndpoint (leave unset) or alternatively
strip the leading /p/{provider} prefix before assigning; update the logic around
core.ParseProviderPassthroughPath and the final ep = path branch to either skip
assignment or call a helper to remove the /p/{provider} prefix prior to calling
ensureLogData(entry).PassthroughEndpoint so PassthroughEndpoint always contains
only the intended provider path segment.
In `@internal/server/passthrough_response_writer.go`:
- Around line 32-45: The handler rawPassthroughResponseHandler.Handle must
validate resp.StatusCode before calling c.Response().WriteHeader to avoid
panics; check that resp != nil and resp.StatusCode is between 100 and 599
inclusive, and if not, return a typed core.GatewayError (use the provider from
getPassthroughProviderType(c)) such as core.NewGatewayError/provider-specific
equivalent with http.StatusBadGateway and a clear message about an invalid
upstream status code, leaving the rest of the flow (closing resp.Body, copying
headers, setting RequestID) untouched.
In `@internal/server/passthrough_route_test.go`:
- Around line 113-133: The test
TestPassthroughRoute_ProviderResolutionMiddleware_RejectsDisabledInstance
currently swallows the handler error and only checks the status code; update it
to capture the returned error from invoking the middleware-wrapped handler (call
handler(c) and assign err) and assert the response body uses the
OpenAI-compatible error envelope (fields error.type, error.message, error.param,
error.code) consistent with the sibling test for provider resolution;
specifically, after calling PassthroughProviderResolutionMiddleware(provider,
disabledInstances) verify err is non-nil, unmarshal rec.Body into a struct/map
and assert those envelope fields contain the expected values for a disabled
instance error (matching how other tests assert core.GatewayError-shaped
responses).
- Around line 177-191: The mockPatcher struct has an err field that's only used
by PatchResponsesRequest and is ignored when fn is set in PatchChatRequest;
remove the unused err field to simplify the mock: delete the err field from
mockPatcher, update PatchChatRequest to return req, nil when m.fn is nil, and
update PatchResponsesRequest to return req, nil instead of req, m.err; also
update any tests/construction sites that set mockPatcher.err to stop passing
that value.
---
Outside diff comments:
In `@internal/auditlog/auditlog_test.go`:
- Around line 599-665: Add a unit test that exercises the branch in
EnrichLogEntryWithPassthroughEndpoint where ep == "" && path != "" by creating a
LogEntry whose Path is a non-/p/ path (e.g., "/some/other/endpoint") and a
Workflow with Mode = core.ExecutionModePassthrough but with
Passthrough.RawEndpoint and Passthrough.NormalizedEndpoint empty; call
EnrichLogEntryWithPassthroughEndpoint(entry, wf) and assert that
entry.Data.PassthroughEndpoint equals the original entry.Path (e.g.,
"/some/other/endpoint") and that entry.Data is non-nil. This targets
EnrichLogEntryWithPassthroughEndpoint and LogEntry/LogData to close the
uncovered branch.
In `@internal/server/http_test.go`:
- Around line 32-58: Update the test error messages to use the RequestID header
constant instead of the hardcoded string: replace occurrences of the literal
"X-Request-ID" in the failure messages in the tests inside
internal/server/http_test.go (the first subtest that checks
rec.Header().Get(core.RequestIDHeader) and the "preserves existing request ID"
subtest that checks respID) to reference core.RequestIDHeader so the messages
remain correct if the constant changes; keep the rest of the assertions (using
got, respID, req, srv.ServeHTTP) unchanged.
---
Duplicate comments:
In `@internal/server/passthrough_middleware.go`:
- Around line 94-97: When readAndRestoreBody(c.Request()) returns an error you
must not call next(c) because the request body may have been closed; instead,
detect err != nil and return a typed core.GatewayError wrapping the underlying
error (use the project’s constructor/wrapper for core.GatewayError) from the
middleware; keep the existing behavior where only len(body) == 0 triggers
next(c) — i.e., change the if-check to separate err handling (return
core.GatewayError) from the empty-body path (return next(c)).
- Around line 99-105: The middleware currently always unmarshals the passthrough
JSON into core.ChatRequest and calls patcher.PatchChatRequest, causing
/p/{provider}/responses traffic to be validated as chat instead of responses;
change the logic to branch by endpoint type (e.g., inspect c.Request().URL.Path
or the route parameter) and when the request targets the responses endpoint
unmarshal into core.PatchResponsesRequest and call
patcher.PatchResponsesRequest, otherwise unmarshal into core.ChatRequest and
call patcher.PatchChatRequest; preserve the existing behavior of returning
next(c) on unmarshal errors and returning handleError(c, err) on patcher errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 721b0111-e59d-4be9-acf7-ab3bf87b8cba
📒 Files selected for processing (9)
internal/auditlog/auditlog_test.gointernal/auditlog/middleware.gointernal/providers/router.gointernal/server/handlers_test.gointernal/server/http_test.gointernal/server/passthrough_middleware.gointernal/server/passthrough_provider_resolution.gointernal/server/passthrough_response_writer.gointernal/server/passthrough_route_test.go
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
# Conflicts: # CLAUDE.md # README.md # config/config.go # config/config_test.go # internal/server/handlers_test.go # internal/server/passthrough_support.go
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/config_test.go (1)
623-683: 🧹 Nitpick | 🔵 TrivialNit: misleading variable names.
yamlNormalize/envNormalize/wantNormalizetrackallow_passthrough_v1_alias, which has nothing to do with "normalization". Consider renaming toyamlV1Alias/envV1Alias/wantV1Aliasfor clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/config_test.go` around lines 623 - 683, The test TestLoad_PassthroughFlags_EnvOverridesYAML uses misleading variable names yamlNormalize/envNormalize/wantNormalize for the allow_passthrough_v1_alias value; rename these variables to yamlV1Alias/envV1Alias/wantV1Alias and update all their usages in the test body (string construction of yaml, t.Setenv call, and the final assertion comparing result.Config.Server.AllowPassthroughV1Alias) so the variable names clearly reflect the AllowPassthroughV1Alias field being tested.
♻️ Duplicate comments (2)
internal/server/passthrough_middleware.go (2)
101-108:⚠️ Potential issue | 🟠 MajorGuardrails only patch chat requests —
/p/{provider}/responsesis misrouted.Every JSON body is unmarshaled into
core.ChatRequestand pushed throughpatcher.PatchChatRequest. Requests hitting/p/{provider}/responses(or/p/{provider}/v1/responses) will either silently bypass guardrails (unmarshal succeeds but fields don't map) or be wrongly rejected by chat-only validation inside the patcher.TranslatedRequestPatcheralready exposesPatchResponsesRequestspecifically for this — dispatch by endpoint before decoding.🛠️ Suggested dispatch
- var chatReq core.ChatRequest - if jsonErr := json.Unmarshal(body, &chatReq); jsonErr != nil { - return next(c) - } - - if _, err := patcher.PatchChatRequest(c.Request().Context(), &chatReq); err != nil { - return handleError(c, err) - } + _, endpoint, _ := core.ParseProviderPassthroughPath(c.Request().URL.Path) + endpoint = strings.TrimPrefix(strings.Trim(endpoint, "/"), "v1/") + + switch endpoint { + case "chat/completions": + var chatReq core.ChatRequest + if jsonErr := json.Unmarshal(body, &chatReq); jsonErr != nil { + return next(c) + } + if _, err := patcher.PatchChatRequest(c.Request().Context(), &chatReq); err != nil { + return handleError(c, err) + } + case "responses": + var responsesReq core.ResponsesRequest + if jsonErr := json.Unmarshal(body, &responsesReq); jsonErr != nil { + return next(c) + } + if _, err := patcher.PatchResponsesRequest(c.Request().Context(), &responsesReq); err != nil { + return handleError(c, err) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/passthrough_middleware.go` around lines 101 - 108, The middleware currently unmarshals every JSON body into core.ChatRequest and always calls patcher.PatchChatRequest, which misroutes /p/{provider}/responses (and /p/{provider}/v1/responses); change the dispatch to inspect the request path and call the appropriate patcher method: if the path matches the responses endpoints, unmarshal into the responses request type and call patcher.PatchResponsesRequest, otherwise unmarshal into core.ChatRequest and call patcher.PatchChatRequest; preserve existing error handling (return handleError or next(c)) for each branch.
96-99:⚠️ Potential issue | 🟠 MajorBody-read failure still forwards a closed/unrestored body downstream.
readAndRestoreBodyclosesreq.Bodybefore returning and only reassignsreq.Bodyon the success path (lines 121-127). Whenerr != nil, this middleware doesreturn next(c)with a closed body, so the downstream forwarder sees EOF/zero bytes and silently proxies a broken request to the upstream provider. This should surface as a typedcore.GatewayErrorinstead of being swallowed.🛠️ Proposed fix
- body, err := readAndRestoreBody(c.Request()) - if err != nil || len(body) == 0 { - return next(c) - } + body, err := readAndRestoreBody(c.Request()) + if err != nil { + return handleError(c, core.NewInvalidRequestError("failed to read request body", err)) + } + if len(body) == 0 { + return next(c) + }As per coding guidelines: "All errors returned to clients must be instances of
core.GatewayError".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/passthrough_middleware.go` around lines 96 - 99, The middleware currently calls readAndRestoreBody and, on error, calls next(c) while the request body has been closed; change the error path so it does NOT forward a broken request but returns a typed core.GatewayError. Specifically, in the handler around the readAndRestoreBody call, detect err != nil and convert/wrap the error into a core.GatewayError (use the project's factory/constructor for GatewayError) and return that error to the caller instead of calling next(c); keep the existing successful path that reassigns req.Body and calls next(c). Ensure you reference readAndRestoreBody and the middleware handler where the body is read when implementing this change.
🤖 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/server/handlers_test.go`:
- Line 6337: Remove the stray empty line that was accidentally inserted between
the two adjacent test functions: delete the blank line between their func
Test... declarations so the tests are back-to-back (i.e., remove the extra
newline between the two func Test* definitions).
- Around line 785-789: The mock ResolvePassthroughByName implementation returns
(&mockDirectPassthroughProvider{...}, "", nil) when GetProviderTypeForName(...)
yields an empty string; change it so that when providerType == "" the method
returns a not-found error (e.g., core.NewNotFoundError(...) or equivalent)
instead of nil error. Update mockProvider.ResolvePassthroughByName to check the
result of GetProviderTypeForName(instanceName) and return (nil, "",
core.NewNotFoundError("passthrough provider", instanceName)) so tests that
expect lookup failures get an error; keep the existing successful-path return of
(&mockDirectPassthroughProvider{inner: m}, providerType, nil) when providerType
is non-empty.
- Around line 621-637: The test harness currently swallows errors from
ensureRequestID and readAndRestoreBody inside newPassthroughTestEchoWithLogger,
so if those helpers fail the request still proceeds into h.ProviderPassthrough;
change the anonymous route in newPassthroughTestEchoWithLogger to check the
returned errors from ensureRequestID and readAndRestoreBody and abort on error
(do not call h.ProviderPassthrough) by calling c.Error(err) or returning an
appropriate HTTP error (e.g., echo.NewHTTPError(500, err)) so tests fail loudly
and the context is not half-initialized.
In `@internal/server/passthrough_middleware.go`:
- Around line 36-45: The code calls passthroughBestEffortModel (which
reads/restores the request body) before checking disabledInstances, causing
oversized/unreadable bodies to error before the intended rejection; move the
disabledInstances check so it executes immediately after instanceName is known
and before calling passthroughBestEffortModel, i.e., check if _, disabled :=
disabledInstances[instanceName]; disabled { return handleError(c,
core.NewInvalidRequestError("passthrough is disabled for provider
"+instanceName, nil)) } and only then call passthroughBestEffortModel to avoid
unnecessary body reads and incorrect 400 errors.
In `@internal/server/passthrough_route_test.go`:
- Around line 20-196: Add two tests for PassthroughGuardrailsMiddleware: (1) use
the existing failingReader type as the request Body and assert the middleware
returns a typed core.GatewayError surfaced in an OpenAI-compatible envelope
(check Error.Type/Message/Param/Code) instead of letting a consumed body forward
to the handler; (2) create a request whose path is /p/{provider}/responses, pass
a mockPatcher that records PatchResponsesRequest calls (implement
PatchResponsesRequest on mockPatcher) and assert PatchResponsesRequest (not
PatchChatRequest) was invoked by PassthroughGuardrailsMiddleware; reference
PassthroughGuardrailsMiddleware, failingReader,
mockPatcher.PatchResponsesRequest, and core.GatewayError/OpenAI error envelope
in your test assertions.
In `@README.md`:
- Line 242: The sentence "passthrough is enabled for all configured providers"
overstates support; update that sentence in the README to read "passthrough is
enabled for all configured providers that support it" (or similar) and ensure
the surrounding text still references which integrations expose native
passthrough as noted later; change the exact phrase found in the README to
include "that support it" so readers understand only providers with native
passthrough are eligible.
---
Outside diff comments:
In `@config/config_test.go`:
- Around line 623-683: The test TestLoad_PassthroughFlags_EnvOverridesYAML uses
misleading variable names yamlNormalize/envNormalize/wantNormalize for the
allow_passthrough_v1_alias value; rename these variables to
yamlV1Alias/envV1Alias/wantV1Alias and update all their usages in the test body
(string construction of yaml, t.Setenv call, and the final assertion comparing
result.Config.Server.AllowPassthroughV1Alias) so the variable names clearly
reflect the AllowPassthroughV1Alias field being tested.
---
Duplicate comments:
In `@internal/server/passthrough_middleware.go`:
- Around line 101-108: The middleware currently unmarshals every JSON body into
core.ChatRequest and always calls patcher.PatchChatRequest, which misroutes
/p/{provider}/responses (and /p/{provider}/v1/responses); change the dispatch to
inspect the request path and call the appropriate patcher method: if the path
matches the responses endpoints, unmarshal into the responses request type and
call patcher.PatchResponsesRequest, otherwise unmarshal into core.ChatRequest
and call patcher.PatchChatRequest; preserve existing error handling (return
handleError or next(c)) for each branch.
- Around line 96-99: The middleware currently calls readAndRestoreBody and, on
error, calls next(c) while the request body has been closed; change the error
path so it does NOT forward a broken request but returns a typed
core.GatewayError. Specifically, in the handler around the readAndRestoreBody
call, detect err != nil and convert/wrap the error into a core.GatewayError (use
the project's factory/constructor for GatewayError) and return that error to the
caller instead of calling next(c); keep the existing successful path that
reassigns req.Body and calls next(c). Ensure you reference readAndRestoreBody
and the middleware handler where the body is read when implementing this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fd62cb7f-e3a4-4e68-82ca-451b1d71e052
📒 Files selected for processing (20)
.env.templateCLAUDE.mdREADME.mdconfig/config.example.yamlconfig/config.goconfig/config_test.godocs/features/passthrough-api.mdxdocs/guides/vllm.mdxinternal/admin/handler.gointernal/auditlog/middleware.gointernal/providers/anthropic/anthropic.gointernal/providers/factory.gointernal/providers/registry.gointernal/providers/vllm/vllm.gointernal/server/handlers_test.gointernal/server/passthrough_middleware.gointernal/server/passthrough_response_writer.gointernal/server/passthrough_route_test.gotests/integration/auditlog_test.gotests/integration/usage_test.go
Comment on lines
+621
to
+637
| func newPassthroughTestEchoWithLogger(h *Handler, pp core.PassthroughProvider, instanceName, providerType string, logger auditlog.LoggerInterface) *echo.Echo { | ||
| e := echo.New() | ||
| if logger != nil { | ||
| e.Use(auditlog.Middleware(logger)) | ||
| } | ||
| e.Any("/p/:provider/*", func(c *echo.Context) error { | ||
| httpReq, _ := ensureRequestID(c.Request()) | ||
| c.SetRequest(httpReq) | ||
| body, _ := readAndRestoreBody(c.Request()) | ||
| model := bestEffortModel(body) | ||
| auditlog.EnrichEntry(c, model, providerType) | ||
| auditlog.EnrichEntryWithResolvedRoute(c, model, providerType, instanceName) | ||
| setPassthroughResolution(c, providerType, pp) | ||
| return h.ProviderPassthrough(c) | ||
| }) | ||
| return e | ||
| } |
Contributor
There was a problem hiding this comment.
Swallowed errors in the passthrough test harness may mask real failures.
ensureRequestID (Line 627) and readAndRestoreBody (Line 629) both have their errors discarded with _. If a future test passes an unreadable body or an input that trips these helpers, the test will proceed into h.ProviderPassthrough(c) with a potentially half-initialized context and produce a misleading outcome. Consider logging via t.Logf/t.Fatalf — but since *testing.T isn't threaded through here, at minimum propagate them by returning 500 or calling c.Error(err) so tests fail loudly rather than silently succeed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/handlers_test.go` around lines 621 - 637, The test harness
currently swallows errors from ensureRequestID and readAndRestoreBody inside
newPassthroughTestEchoWithLogger, so if those helpers fail the request still
proceeds into h.ProviderPassthrough; change the anonymous route in
newPassthroughTestEchoWithLogger to check the returned errors from
ensureRequestID and readAndRestoreBody and abort on error (do not call
h.ProviderPassthrough) by calling c.Error(err) or returning an appropriate HTTP
error (e.g., echo.NewHTTPError(500, err)) so tests fail loudly and the context
is not half-initialized.
Comment on lines
+785
to
+789
| func (m *mockProvider) ResolvePassthroughByName(instanceName string) (core.PassthroughProvider, string, error) { | ||
| providerType := strings.TrimSpace(m.GetProviderTypeForName(instanceName)) | ||
| m.lastPassthroughProvider = providerType | ||
| return &mockDirectPassthroughProvider{inner: m}, providerType, nil | ||
| } |
Contributor
There was a problem hiding this comment.
Mock silently returns empty providerType with nil error when resolution fails.
If an instanceName is passed that isn't present in providerNames, GetProviderTypeForName returns "" and this mock returns (wrapper, "", nil) — a successful resolution with an empty type. Production ResolvePassthroughByName likely returns a typed not_found_error in that case, so tests relying on error propagation for unknown instances will silently pass here. Consider returning core.NewNotFoundError(...) when providerType == "".
🛠️ Suggested adjustment
func (m *mockProvider) ResolvePassthroughByName(instanceName string) (core.PassthroughProvider, string, error) {
providerType := strings.TrimSpace(m.GetProviderTypeForName(instanceName))
+ if providerType == "" {
+ return nil, "", core.NewNotFoundError("unknown passthrough provider: " + instanceName)
+ }
m.lastPassthroughProvider = providerType
return &mockDirectPassthroughProvider{inner: m}, providerType, nil
}📝 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 (m *mockProvider) ResolvePassthroughByName(instanceName string) (core.PassthroughProvider, string, error) { | |
| providerType := strings.TrimSpace(m.GetProviderTypeForName(instanceName)) | |
| m.lastPassthroughProvider = providerType | |
| return &mockDirectPassthroughProvider{inner: m}, providerType, nil | |
| } | |
| func (m *mockProvider) ResolvePassthroughByName(instanceName string) (core.PassthroughProvider, string, error) { | |
| providerType := strings.TrimSpace(m.GetProviderTypeForName(instanceName)) | |
| if providerType == "" { | |
| return nil, "", core.NewNotFoundError("unknown passthrough provider: " + instanceName) | |
| } | |
| m.lastPassthroughProvider = providerType | |
| return &mockDirectPassthroughProvider{inner: m}, providerType, nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/handlers_test.go` around lines 785 - 789, The mock
ResolvePassthroughByName implementation returns
(&mockDirectPassthroughProvider{...}, "", nil) when GetProviderTypeForName(...)
yields an empty string; change it so that when providerType == "" the method
returns a not-found error (e.g., core.NewNotFoundError(...) or equivalent)
instead of nil error. Update mockProvider.ResolvePassthroughByName to check the
result of GetProviderTypeForName(instanceName) and return (nil, "",
core.NewNotFoundError("passthrough provider", instanceName)) so tests that
expect lookup failures get an error; keep the existing successful-path return of
(&mockDirectPassthroughProvider{inner: m}, providerType, nil) when providerType
is non-empty.
| } | ||
| } | ||
|
|
||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Nit: stray empty added line.
Line 6337 is an empty added line between two test functions — safe to drop for cleanliness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/handlers_test.go` at line 6337, Remove the stray empty line
that was accidentally inserted between the two adjacent test functions: delete
the blank line between their func Test... declarations so the tests are
back-to-back (i.e., remove the extra newline between the two func Test*
definitions).
Comment on lines
+36
to
+45
| bestModel, peekErr := passthroughBestEffortModel(c) | ||
| if peekErr != nil { | ||
| return handleError(c, peekErr) | ||
| } | ||
|
|
||
| if _, disabled := disabledInstances[instanceName]; disabled { | ||
| return handleError(c, core.NewInvalidRequestError( | ||
| "passthrough is disabled for provider "+instanceName, nil, | ||
| )) | ||
| } |
Contributor
There was a problem hiding this comment.
Check disabledInstances before peeking the body.
passthroughBestEffortModel reads and restores the full request body (up to BodyLimit) for every request, including requests targeting disabled instances that are about to be rejected. Two practical problems:
- A disabled instance with an unreadable or oversized body returns
400 "failed to read request body"instead of the clearer"passthrough is disabled for provider <name>", confusing clients. - Disabled instances still pay the full body-buffer cost — an easy way to burn gateway memory on rejected traffic.
Rejecting disabled instances first is cheap and doesn't affect the audit enrichment path (that path is only reached on success).
🛠️ Proposed reorder
- bestModel, peekErr := passthroughBestEffortModel(c)
- if peekErr != nil {
- return handleError(c, peekErr)
- }
-
if _, disabled := disabledInstances[instanceName]; disabled {
return handleError(c, core.NewInvalidRequestError(
"passthrough is disabled for provider "+instanceName, nil,
))
}
+
+ bestModel, peekErr := passthroughBestEffortModel(c)
+ if peekErr != nil {
+ return handleError(c, peekErr)
+ }📝 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
| bestModel, peekErr := passthroughBestEffortModel(c) | |
| if peekErr != nil { | |
| return handleError(c, peekErr) | |
| } | |
| if _, disabled := disabledInstances[instanceName]; disabled { | |
| return handleError(c, core.NewInvalidRequestError( | |
| "passthrough is disabled for provider "+instanceName, nil, | |
| )) | |
| } | |
| if _, disabled := disabledInstances[instanceName]; disabled { | |
| return handleError(c, core.NewInvalidRequestError( | |
| "passthrough is disabled for provider "+instanceName, nil, | |
| )) | |
| } | |
| bestModel, peekErr := passthroughBestEffortModel(c) | |
| if peekErr != nil { | |
| return handleError(c, peekErr) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/passthrough_middleware.go` around lines 36 - 45, The code
calls passthroughBestEffortModel (which reads/restores the request body) before
checking disabledInstances, causing oversized/unreadable bodies to error before
the intended rejection; move the disabledInstances check so it executes
immediately after instanceName is known and before calling
passthroughBestEffortModel, i.e., check if _, disabled :=
disabledInstances[instanceName]; disabled { return handleError(c,
core.NewInvalidRequestError("passthrough is disabled for provider
"+instanceName, nil)) } and only then call passthroughBestEffortModel to avoid
unnecessary body reads and incorrect 400 errors.
Comment on lines
+20
to
+196
| func TestPassthroughRoute_ProviderPassthrough_ForwardsResponse(t *testing.T) { | ||
| pp := &mockDirectPassthroughProvider{ | ||
| inner: &mockProvider{ | ||
| passthroughResponse: &core.PassthroughResponse{ | ||
| StatusCode: 200, | ||
| Headers: map[string][]string{"Content-Type": {"application/json"}}, | ||
| Body: io.NopCloser(bytes.NewReader([]byte(`{"result":"ok"}`))), | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| e := echo.New() | ||
| req := httptest.NewRequest(http.MethodPost, "/p/openai/chat/completions", bytes.NewReader([]byte(`{"model":"gpt-4"}`))) | ||
| rec := httptest.NewRecorder() | ||
| c := e.NewContext(req, rec) | ||
|
|
||
| req.Header.Set(core.RequestIDHeader, "req-123") | ||
| req, _ = ensureRequestID(req) | ||
| c.SetRequest(req) | ||
| setPassthroughResolution(c, "openai", pp) | ||
| assert.Equal(t, "openai", getPassthroughProviderType(c)) | ||
| assert.Equal(t, pp, getPassthroughProvider(c)) | ||
|
|
||
| svc := &passthroughService{ | ||
| responseHandler: newRawPassthroughResponseHandler(), | ||
| normalizeV1Prefix: false, | ||
| } | ||
| err := svc.ProviderPassthrough(c) | ||
| require.NoError(t, err) | ||
|
|
||
| assert.Equal(t, 200, rec.Code) | ||
| assert.Equal(t, `{"result":"ok"}`, rec.Body.String()) | ||
| assert.Equal(t, "req-123", rec.Header().Get(core.RequestIDHeader)) | ||
| } | ||
|
|
||
| func TestPassthroughRoute_GuardrailsMiddleware_RestoresBody(t *testing.T) { | ||
| e := echo.New() | ||
| bodyContent := `{"model":"gpt-4","messages":[]}` | ||
| req := httptest.NewRequest(http.MethodPost, "/p/openai/v1/chat/completions", bytes.NewReader([]byte(bodyContent))) | ||
| rec := httptest.NewRecorder() | ||
| c := e.NewContext(req, rec) | ||
|
|
||
| // Patcher that reads the body | ||
| readingPatcher := &mockPatcher{fn: func(ctx context.Context, cr *core.ChatRequest) (*core.ChatRequest, error) { | ||
| return cr, nil | ||
| }} | ||
|
|
||
| var bodyReadByHandler []byte | ||
| handler := PassthroughGuardrailsMiddleware(readingPatcher)(func(c *echo.Context) error { | ||
| var err error | ||
| bodyReadByHandler, err = io.ReadAll(c.Request().Body) | ||
| return err | ||
| }) | ||
|
|
||
| err := handler(c) | ||
| require.NoError(t, err) | ||
| // Body should be readable again in the handler | ||
| assert.Equal(t, bodyContent, string(bodyReadByHandler)) | ||
| } | ||
|
|
||
| func TestPassthroughRoute_ProviderResolutionMiddleware_RequestBodyReadError(t *testing.T) { | ||
| provider := &mockProvider{} | ||
| e := echo.New() | ||
| req := httptest.NewRequest(http.MethodPost, "/p/openai/responses", nil) | ||
| req.Body = io.NopCloser(failingReader{err: errors.New("read failed")}) | ||
| rec := httptest.NewRecorder() | ||
| c := e.NewContext(req, rec) | ||
|
|
||
| var reachedHandler bool | ||
| handler := PassthroughProviderResolutionMiddleware(provider, nil)(func(c *echo.Context) error { | ||
| reachedHandler = true | ||
| return c.String(http.StatusOK, "ok") | ||
| }) | ||
|
|
||
| err := handler(c) | ||
| require.NoError(t, err) | ||
| assert.False(t, reachedHandler) | ||
| assert.Equal(t, http.StatusBadRequest, rec.Code) | ||
|
|
||
| var env core.OpenAIErrorEnvelope | ||
| require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &env)) | ||
| assert.Equal(t, core.ErrorTypeInvalidRequest, env.Error.Type) | ||
| assert.Contains(t, env.Error.Message, "failed to read request body") | ||
| } | ||
|
|
||
| type failingReader struct { | ||
| err error | ||
| } | ||
|
|
||
| func (r failingReader) Read(p []byte) (int, error) { | ||
| return 0, r.err | ||
| } | ||
|
|
||
| func TestPassthroughRoute_ProviderResolutionMiddleware_RejectsDisabledInstance(t *testing.T) { | ||
| provider := &mockProvider{} | ||
| disabledInstances := map[string]struct{}{"disabled-provider": {}} | ||
|
|
||
| e := echo.New() | ||
| req := httptest.NewRequest(http.MethodGet, "/p/disabled-provider/models", nil) | ||
| rec := httptest.NewRecorder() | ||
| c := e.NewContext(req, rec) | ||
|
|
||
| var reachedHandler bool | ||
| handler := PassthroughProviderResolutionMiddleware(provider, disabledInstances)(func(c *echo.Context) error { | ||
| reachedHandler = true | ||
| return c.String(200, "should not reach") | ||
| }) | ||
|
|
||
| err := handler(c) | ||
| require.NoError(t, err, "Echo returns nil after successfully writing the JSON error body") | ||
| assert.False(t, reachedHandler) | ||
| assert.Equal(t, http.StatusBadRequest, rec.Code) | ||
|
|
||
| var env core.OpenAIErrorEnvelope | ||
| require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &env)) | ||
| assert.Equal(t, core.ErrorTypeInvalidRequest, env.Error.Type) | ||
| assert.Equal(t, "passthrough is disabled for provider disabled-provider", env.Error.Message) | ||
| assert.Nil(t, env.Error.Param) | ||
| assert.Nil(t, env.Error.Code) | ||
| } | ||
|
|
||
| func TestPassthroughRoute_RequestID_GeneratesUUID(t *testing.T) { | ||
| req := httptest.NewRequest(http.MethodGet, "/p/openai/v1/models", nil) | ||
| req, id := ensureRequestID(req) | ||
| assert.NotEmpty(t, id) | ||
| assert.Equal(t, id, req.Header.Get(core.RequestIDHeader)) | ||
| } | ||
|
|
||
| func TestPassthroughRoute_RequestID_EchoesClientID(t *testing.T) { | ||
| req := httptest.NewRequest(http.MethodGet, "/p/openai/v1/models", nil) | ||
| req.Header.Set(core.RequestIDHeader, "client-123") | ||
| req, id := ensureRequestID(req) | ||
| assert.Equal(t, "client-123", id) | ||
| assert.Equal(t, "client-123", req.Header.Get(core.RequestIDHeader)) | ||
| } | ||
|
|
||
| func TestPassthroughRoute_HeaderFiltering_StripsSensitiveHeaders(t *testing.T) { | ||
| req := &http.Request{ | ||
| Header: http.Header{ | ||
| "Authorization": {"Bearer token"}, | ||
| core.RequestIDHeader: {"internal-id"}, | ||
| "X-Api-Key": {"api-key"}, | ||
| "Content-Type": {"application/json"}, | ||
| "User-Agent": {"test-client"}, | ||
| "X-Forwarded-For": {"1.2.3.4"}, | ||
| }, | ||
| } | ||
|
|
||
| headers := buildPassthroughHeaders(context.Background(), req.Header, "req-123") | ||
|
|
||
| // Sensitive headers should be stripped | ||
| assert.Empty(t, headers.Get("Authorization")) | ||
| assert.Empty(t, headers.Get("X-Api-Key")) | ||
| assert.Empty(t, headers.Get("X-Forwarded-For")) | ||
|
|
||
| // Safe headers should be copied | ||
| assert.Equal(t, "application/json", headers.Get("Content-Type")) | ||
| assert.Equal(t, "test-client", headers.Get("User-Agent")) | ||
|
|
||
| // Client-supplied request ID is replaced by the gateway-generated one | ||
| assert.Equal(t, "req-123", headers.Get(core.RequestIDHeader)) | ||
| } | ||
|
|
||
| type mockPatcher struct { | ||
| fn func(context.Context, *core.ChatRequest) (*core.ChatRequest, error) | ||
| } | ||
|
|
||
| func (m *mockPatcher) PatchChatRequest(ctx context.Context, req *core.ChatRequest) (*core.ChatRequest, error) { | ||
| if m.fn != nil { | ||
| return m.fn(ctx, req) | ||
| } | ||
| return req, nil | ||
| } | ||
|
|
||
| func (m *mockPatcher) PatchResponsesRequest(ctx context.Context, req *core.ResponsesRequest) (*core.ResponsesRequest, error) { | ||
| return req, nil | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM — consider two additional cases.
The suite now asserts the full OpenAI-compatible envelope (Type/Message/Param/Code) on error paths, which is exactly the regression guardrail the earlier feedback asked for. Two gaps worth covering while you're here (both correspond to live concerns in passthrough_middleware.go):
PassthroughGuardrailsMiddlewarewith afailingReaderbody — asserts the middleware surfaces a typedcore.GatewayErrorrather than forwarding a consumed body.PassthroughGuardrailsMiddlewareon a/p/{provider}/responsesrequest — asserts the patcher is invoked viaPatchResponsesRequest(or at minimum not asPatchChatRequest).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/passthrough_route_test.go` around lines 20 - 196, Add two
tests for PassthroughGuardrailsMiddleware: (1) use the existing failingReader
type as the request Body and assert the middleware returns a typed
core.GatewayError surfaced in an OpenAI-compatible envelope (check
Error.Type/Message/Param/Code) instead of letting a consumed body forward to the
handler; (2) create a request whose path is /p/{provider}/responses, pass a
mockPatcher that records PatchResponsesRequest calls (implement
PatchResponsesRequest on mockPatcher) and assert PatchResponsesRequest (not
PatchChatRequest) was invoked by PassthroughGuardrailsMiddleware; reference
PassthroughGuardrailsMiddleware, failingReader,
mockPatcher.PatchResponsesRequest, and core.GatewayError/OpenAI error envelope
in your test assertions.
|
|
||
| ### Per-Instance Control | ||
|
|
||
| By default, passthrough is enabled for all configured providers. Disable it for specific instances: |
Contributor
There was a problem hiding this comment.
Minor: wording overstates coverage.
"passthrough is enabled for all configured providers" implies every provider receives passthrough. In reality, only providers whose integration exposes native passthrough are eligible (as you correctly note at line 254). Consider: "passthrough is enabled for all configured providers that support it".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 242, The sentence "passthrough is enabled for all
configured providers" overstates support; update that sentence in the README to
read "passthrough is enabled for all configured providers that support it" (or
similar) and ensure the surrounding text still references which integrations
expose native passthrough as noted later; change the exact phrase found in the
README to include "that support it" so readers understand only providers with
native passthrough are eligible.
Contributor
SantiagoDePolonia
left a comment
SantiagoDePolonia
left a comment
There was a problem hiding this comment.
I've added some minor comments during review, but this comment is the most important.
Generally I think that we shouldn't merge this and I think that it's my bad that I didn't communicate the requirements clearly.
What's well done:
- We should use provider_name instead of provider_type(named by you as instance) for sure
- This layer is more transparent then the previous solution
- Errors should be returned in the original provider shape
The requirements are that we should be able to use it with guardrails and track the usage and have audit_logs for those requests. Also, we should be able to map some of the requests to make them understandable for our app at WhiteBoxPrompt. It unlocks as with enabling guardrails for most of the cases where GoModel can understand the Semantic of the request (what's the message, what model was used, etc)
Summary generated with AI:
- High: passthrough usage logging regresses in the branch. internal/server/handlers.go:187 now builds a
passthrough service that only has a response handler, and /internal/server/passthrough_service.go:12 only
forwards bytes. In main, the old passthrough path still attached SSE usage/
audit observers at internal/server/passthrough_support.go:283. So this PR
is cleaner, but it misses the explicit goal of supporting passthrough
usage, and it downgrades streaming observability. - High: the default passthrough guardrail wiring is effectively a no-op.
/internal/server/http.go:265 skips
workflow resolution for /p/..., but
internal/app/app.go:305 passes a workflow-based guardrail patcher, and that
patcher resolves its pipeline from request context at internal/guardrails/. workflow_executor.go:35. With
no workflow in context, passthrough requests do not get request-scoped
guardrails or other workflow-based mechanisms in the normal app wiring. - High: even if a custom passthrough patcher is injected, request mutation is
explicitly disabled. /internal/
server/passthrough_middleware.go:86 states that guardrail modifications are
intentionally discarded, and the handler still forwards the original body.
That conflicts with the stated goal that content/header modifications
should be possible. - High: passthrough middleware fully buffers opaque bodies, twice. /internal/server/
passthrough_middleware.go:36 reads the whole body for best-effort model
extraction, and /internal/server/
passthrough_middleware.go:96 reads it again for guardrail precheck. That is
the wrong tradeoff for multipart/files/binary passthrough and works against
the “all providers” target.
| pprof_enabled: false # expose /debug/pprof/* for local profiling only | ||
| enable_passthrough_routes: true # expose /p/{provider}/{endpoint} passthrough routes | ||
| allow_passthrough_v1_alias: true # allow /p/{provider}/v1/... while keeping /p/{provider}/... canonical | ||
| enabled_passthrough_providers: ["openai", "anthropic", "openrouter", "zai", "vllm"] # providers enabled on /p/{provider}/... |
Contributor
There was a problem hiding this comment.
I would add an option like:
disable_passthrough_api: false #yaml
And for every provider:
passthrough_enabled: true #yaml
ENV variables should be like:
DISABLE_PASSTHROUGH_API=false
OPENAI_PASSTHROUGH_ENABLED=true
| OpenAI-compatible API. For example, the Anthropic SDK sends requests to | ||
| `/v1/messages`, so you can point it at GoModel's Anthropic passthrough base URL: | ||
| `/v1/messages`, so you can point it at GoModel's Anthropic passthrough base URL. The | ||
| first path segment must match a **key** under `providers:` in your config (not the |
Contributor
There was a problem hiding this comment.
Change key to provider_name (key). Or something like this
|
|
||
| ```text | ||
| /p/{provider}/{endpoint} | ||
| /p/{instance}/{endpoint} |
Contributor
There was a problem hiding this comment.
Let's stick to provider_name - it's more precise. However, we can discuss it.
| ## How it works | ||
|
|
||
| Passthrough routes use this shape: | ||
| Passthrough routes use this shape (first segment = YAML instance name / `providers` key): |
Contributor
There was a problem hiding this comment.
I would remove the parentheses here.
| default. | ||
| - Passthrough is available for any configured **provider instance** that implements | ||
| it. There is no global allowlist: use `passthrough_disabled: true` on a | ||
| `providers.<name>` block to block `/p/<name>/...` for that instance. |
Contributor
There was a problem hiding this comment.
let's use <provider_name> here, even it it might be redundant here, let's do it to be consequent and to make this string easier to find across the whole codebase
| Passthrough routes are provider-type scoped at `/p/vllm/...`. When you need | ||
| to target one named vLLM instance in a multi-provider setup, use translated | ||
| `/v1/...` requests with provider-qualified model IDs such as | ||
| The first path segment is your **provider instance name** in config (e.g. `/p/vllm/...` |
Contributor
There was a problem hiding this comment.
Let's change instance name -> provider_name
|
|
||
| ## Provider-Native Passthrough | ||
|
|
||
| GoModel exposes provider-native endpoints via `/p/{instance}/...` routes (the first segment is the **instance name**, i.e. the key under `providers:` in config), allowing direct access to provider APIs with minimal gateway involvement. This is useful when you need raw provider responses or specific provider features not abstracted by the OpenAI-compatible API. |
Contributor
There was a problem hiding this comment.
instance -> provider_name
| - `ENABLE_PASSTHROUGH_ROUTES` (true: Enable provider-native passthrough routes under /p/{provider}/...) | ||
| - `ALLOW_PASSTHROUGH_V1_ALIAS` (true: Allow /p/{provider}/v1/... aliases while keeping /p/{provider}/... canonical) | ||
| - `ENABLED_PASSTHROUGH_PROVIDERS` (openai,anthropic,openrouter,zai,vllm: Comma-separated list of enabled passthrough providers) | ||
| - `ENABLE_PASSTHROUGH_ROUTES` (true: enable `/p/{instance}/...`; `instance` is the YAML key under `providers:`, not the provider type) |
Contributor
There was a problem hiding this comment.
instance -> provider_name
| # Comma-separated list of provider types enabled for /p/{provider}/... passthrough (default: openai,anthropic,openrouter,zai,vllm) | ||
| # ENABLED_PASSTHROUGH_PROVIDERS=openai,anthropic,openrouter,zai,vllm | ||
| # Per-instance passthrough opt-out: set passthrough_disabled: true on a provider in config/config.yaml | ||
| # (there is no env allowlist; all configured instances with passthrough support are eligible by default) |
Contributor
There was a problem hiding this comment.
This line - should be removed "# (there is no env allowlist; all configured instances with passthrough support are eligible by default)"
| // RequestIDHeader is the HTTP header GoModel uses to correlate requests | ||
| // across the gateway and its upstream calls. Clients may supply their own | ||
| // value; if absent a UUID is generated and stamped on every response. | ||
| RequestIDHeader = "X-GoModel-Request-ID" |
Contributor
There was a problem hiding this comment.
I guess it should not be at the ./user_path.go file?
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
3 participants
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
Refactor
Documentation