Conversation
WalkthroughEndpoint filtering was moved upstream into the translation handler, which pre-filters endpoints for native Anthropic support before calling CanPassthrough. CanPassthrough now only checks passthrough enablement and presence of endpoints; tests updated to reflect pre-filtering and passthrough-first flow. Changes
Sequence Diagram(s)mermaid Client->>Handler: POST /translate (with body) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/app/handlers/handler_translation_passthrough_test.go (1)
977-994:⚠️ Potential issue | 🟡 MinorMock
CanPassthroughdiverges from the real implementation — latent test confusion.The real
CanPassthroughnow ignoresprofileLookupentirely and only checkslen(endpoints) == 0. The mock still iterates endpoints and callsprofileLookup.GetAnthropicSupportper-endpoint (the old behaviour). Current tests are unaffected because the handler always passes a pre-filtered slice, so the mock's per-endpoint check always passes. However, any future test that callsCanPassthroughdirectly with a mixed (unfiltered) list will getfalsefrom the mock buttruefrom the real translator — a silent false-negative divergence.Consider aligning the mock with the simplified contract:
♻️ Proposed alignment
func (m *mockPassthroughTranslator) CanPassthrough(endpoints []*domain.Endpoint, profileLookup translator.ProfileLookup) bool { if !m.passthroughEnabled { return false } - - if len(endpoints) == 0 { - return false - } - - // Check if all endpoints support Anthropic - for _, endpoint := range endpoints { - cfg := profileLookup.GetAnthropicSupport(endpoint.Type) - if cfg == nil || !cfg.Enabled { - return false - } - } - - return true + // Mirrors the real CanPassthrough: caller pre-filters; we only validate + // that passthrough is enabled and at least one endpoint was passed through. + return len(endpoints) > 0 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/app/handlers/handler_translation_passthrough_test.go` around lines 977 - 994, The mock CanPassthrough implementation diverges from the real translator by consulting profileLookup per-endpoint; update mockPassthroughTranslator.CanPassthrough to match the real contract by only returning false when passthroughEnabled is false or len(endpoints) == 0 and otherwise returning true, removing any calls to profileLookup.GetAnthropicSupport and the per-endpoint loop so tests that call CanPassthrough directly behave the same as the production implementation.
🤖 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/app/handlers/handler_translation.go`:
- Around line 244-267: The translationHandler currently nests an if
a.profileLookup != nil inside the passthroughTrans type-check, pushing cognitive
complexity over the limit; extract the anthropic-capable filtering into a new
helper named filterAnthropicCapableEndpoints that accepts (endpoints
[]*domain.Endpoint) and returns []*domain.Endpoint, where if a.profileLookup is
nil it returns an empty slice, otherwise it applies the existing
GetAnthropicSupport/Enabled filter; then replace the nested block so
translationHandler calls passthroughEndpoints :=
a.filterAnthropicCapableEndpoints(endpoints) and passes that to
passthroughTrans.CanPassthrough and executePassthroughRequest, leaving
recordTranslatorMetrics and the original mode/fallback logic unchanged.
---
Outside diff comments:
In `@internal/app/handlers/handler_translation_passthrough_test.go`:
- Around line 977-994: The mock CanPassthrough implementation diverges from the
real translator by consulting profileLookup per-endpoint; update
mockPassthroughTranslator.CanPassthrough to match the real contract by only
returning false when passthroughEnabled is false or len(endpoints) == 0 and
otherwise returning true, removing any calls to
profileLookup.GetAnthropicSupport and the per-endpoint loop so tests that call
CanPassthrough directly behave the same as the production implementation.
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/app/handlers/handler_translation.go`:
- Around line 163-197: The early return in tryPassthrough when a.profileLookup
== nil causes resolveTranslationFallback to misattribute failures as
FallbackReasonCannotPassthrough; update tryPassthrough (function tryPassthrough)
to detect the nil profileLookup and either log a clear message and/or record
translator metrics with a new fallback reason (e.g.,
FallbackReasonProfileLookupMissing) before returning false so callers (and
resolveTranslationFallback) see the true cause; add the new constant (e.g.,
constants.FallbackReasonProfileLookupMissing) and ensure recordTranslatorMetrics
is invoked with that reason (or at minimum call a.processLogger.Warn/Error with
context about profileLookup being nil) prior to the early return to preserve
correct telemetry.
| // tryPassthrough attempts to serve the request via passthrough mode if the translator | ||
| // and at least one backend support it. Returns true if the request was handled. | ||
| func (a *Application) tryPassthrough( | ||
| ctx context.Context, | ||
| w http.ResponseWriter, | ||
| r *http.Request, | ||
| bodyBytes []byte, | ||
| endpoints []*domain.Endpoint, | ||
| pr *proxyRequest, | ||
| trans translator.RequestTranslator, | ||
| ) bool { | ||
| passthroughTrans, ok := trans.(translator.PassthroughCapable) | ||
| if !ok || a.profileLookup == nil { | ||
| return false | ||
| } | ||
|
|
||
| // Only pass endpoints whose backend natively supports the wire format. | ||
| // Mixed deployments (e.g. ollama + vllm) must not block passthrough for | ||
| // the capable subset — the proxy will route within that filtered list. | ||
| passthroughEndpoints := make([]*domain.Endpoint, 0, len(endpoints)) | ||
| for _, ep := range endpoints { | ||
| support := a.profileLookup.GetAnthropicSupport(ep.Type) | ||
| if support != nil && support.Enabled { | ||
| passthroughEndpoints = append(passthroughEndpoints, ep) | ||
| } | ||
| } | ||
|
|
||
| if !passthroughTrans.CanPassthrough(passthroughEndpoints, a.profileLookup) { | ||
| return false | ||
| } | ||
|
|
||
| a.executePassthroughRequest(ctx, w, r, bodyBytes, passthroughEndpoints, pr, trans) | ||
| a.recordTranslatorMetrics(trans, pr, constants.TranslatorModePassthrough, constants.FallbackReasonNone) | ||
| return true | ||
| } |
There was a problem hiding this comment.
Misleading FallbackReasonCannotPassthrough when the real cause is a nil profileLookup.
When a.profileLookup == nil triggers the early return at line 175, translationHandler subsequently calls resolveTranslationFallback, which inspects only whether the translator implements PassthroughCapable. If it does, it emits FallbackReasonCannotPassthrough — but the actual reason is "profile lookup not configured", not endpoint incapability. This misattribution can make production debugging harder (e.g., operators looking for a bad endpoint config when the real issue is a wiring/startup problem).
A small targeted guard log or a dedicated fallback reason would preserve the correct signal:
💡 Option — log the nil-profileLookup case before returning
passthroughTrans, ok := trans.(translator.PassthroughCapable)
if !ok || a.profileLookup == nil {
+ if ok && a.profileLookup == nil {
+ // profileLookup not configured — passthrough cannot be attempted even though
+ // the translator supports it. Check application wiring/startup.
+ pr.requestLogger.Warn("passthrough skipped: profileLookup not configured",
+ "translator", trans.Name())
+ }
return false
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/app/handlers/handler_translation.go` around lines 163 - 197, The
early return in tryPassthrough when a.profileLookup == nil causes
resolveTranslationFallback to misattribute failures as
FallbackReasonCannotPassthrough; update tryPassthrough (function tryPassthrough)
to detect the nil profileLookup and either log a clear message and/or record
translator metrics with a new fallback reason (e.g.,
FallbackReasonProfileLookupMissing) before returning false so callers (and
resolveTranslationFallback) see the true cause; add the new constant (e.g.,
constants.FallbackReasonProfileLookupMissing) and ensure recordTranslatorMetrics
is invoked with that reason (or at minimum call a.processLogger.Warn/Error with
context about profileLookup being nil) prior to the early return to preserve
correct telemetry.
Two bugs in the translation handler prevented passthrough from working in certain scenarios due to:
Summary by CodeRabbit
New Features
Bug Fixes
Tests