fix(passthrough): preserve original provider name in route resolution#298
fix(passthrough): preserve original provider name in route resolution#298vfeitoza wants to merge 1 commit intoENTERPILOT:mainfrom
Conversation
The semantic enrichment middleware was overwriting info.Provider with the resolved type (e.g. 'anthropic') before caching, causing passthroughExecutionTarget to route by type instead of the original configured name (e.g. 'teste'). Added info.ProviderName field to preserve the original route name through the resolution pipeline. The execution target now prefers the cached provider name when available, ensuring requests route to the correct configured instance.
📝 WalkthroughWalkthroughA new ChangesProvider-Instance Name Routing
Sequence DiagramsequenceDiagram
participant Client
participant Server as PassthroughService
participant Enrichment as SemanticEnrichment
participant Resolver as ProviderResolver
participant Router as Router
participant Registry as NameRegistry
participant Provider as PassthroughProvider
Client->>Server: HTTP request
Server->>Enrichment: passthrough request (initial)
Enrichment->>Resolver: resolvePassthroughProvider(providerType)
Resolver->>Resolver: compute providerType (trimmed)
Resolver-->>Enrichment: resolved provider info
Enrichment->>Enrichment: cache info.ProviderName from resolved
Enrichment-->>Server: enriched route info
Server->>Server: passthroughExecutionTarget()
Server->>Server: extract providerName (prefer cached, fallback to resolved)
Server-->>Server: return (providerType, providerName, endpoint, info)
Server->>Server: build PassthroughRequest with ProviderName
Server->>Router: Router.Passthrough(ctx, providerType, req with ProviderName)
alt req.ProviderName is set
Router->>Registry: resolve by provider name
Registry-->>Router: named provider instance
Router->>Router: verify implements PassthroughProvider
Router->>Provider: Passthrough(ctx, req)
else fallback to type-based
Router->>Router: resolve by providerType
Router->>Provider: Passthrough(ctx, req)
end
Provider-->>Router: PassthroughResponse
Router-->>Server: PassthroughResponse
Server->>Client: HTTP response
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Suggested Labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Greptile SummaryThe semantic enrichment middleware was overwriting Confidence Score: 4/5Safe to merge; the fix is logically sound with only minor test coverage and logging consistency gaps. All findings are P2: missing unit test for the primary enrichment-cached path and context-less slog.Debug calls. No logic errors or security issues found. passthrough_execution_helpers_test.go — missing test for the pre-populated info.ProviderName case; passthrough_provider_resolution.go — context-less debug logging. Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant SemanticEnrichment as PassthroughSemanticEnrichment
participant ExecTarget as passthroughExecutionTarget
participant Service as passthroughService
participant Router as Router.Passthrough
Client->>SemanticEnrichment: POST /p/teste/v1/messages
Note over SemanticEnrichment: info.Provider = "teste"
SemanticEnrichment->>SemanticEnrichment: resolvePassthroughProvider("teste")<br/>→ {Type:"anthropic", Name:"teste"}
Note over SemanticEnrichment: info.ProviderName = "teste" ✅<br/>info.Provider = "anthropic"
SemanticEnrichment->>SemanticEnrichment: CachePassthroughRouteInfo(env, info)
SemanticEnrichment->>ExecTarget: next(c)
ExecTarget->>ExecTarget: providerName = info.ProviderName → "teste"
ExecTarget->>ExecTarget: providerType = resolved.ProviderType → "anthropic"
ExecTarget-->>Service: (providerType="anthropic", providerName="teste", ...)
Service->>Router: Passthrough(ctx, "anthropic", req{ProviderName:"teste"})
Router->>Router: providerByNameRegistry("teste") → named instance
Router->>Router: named.(PassthroughProvider).Passthrough(ctx, req)
Router-->>Client: upstream response
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| providerType := strings.TrimSpace(named.GetProviderTypeForName(routeProvider)) | ||
| slog.Debug("passthrough provider resolution", "routeProvider", routeProvider, "resolvedType", providerType, "hasNameTypeResolver", true) | ||
| if providerType != "" { | ||
| return passthroughProviderResolution{ | ||
| RouteProvider: routeProvider, | ||
| ProviderType: providerType, | ||
| ProviderName: routeProvider, | ||
| } | ||
| } | ||
| } else { | ||
| slog.Debug("passthrough provider resolution", "routeProvider", routeProvider, "hasNameTypeResolver", false) | ||
| } | ||
| } else { | ||
| slog.Debug("passthrough provider resolution", "routeProvider", routeProvider, "provider", "nil") | ||
| } | ||
|
|
||
| return passthroughProviderResolution{ | ||
| result := passthroughProviderResolution{ | ||
| RouteProvider: routeProvider, | ||
| ProviderType: routeProvider, | ||
| ProviderName: workflowProviderNameForType(provider, routeProvider), | ||
| } | ||
| slog.Debug("passthrough provider resolution fallback", "routeProvider", routeProvider, "providerType", result.ProviderType, "providerName", result.ProviderName) |
There was a problem hiding this comment.
slog.Debug calls lose request-scoped attributes
The new slog.Debug(...) calls use the global logger without a context, so request-scoped values (trace IDs, request IDs, etc.) are not attached. router.go uses slog.DebugContext(ctx, ...) for the same operation. resolvePassthroughProvider currently has no context.Context parameter; if this logging proves useful operationally, threading a context through would make these entries correlate with the caller's trace.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/router.go`:
- Around line 772-795: Add regression tests under **/*_test.go that exercise
Router.Passthrough’s new name-first routing: (1) name hit — register a provider
implementing core.PassthroughProvider, call Router.Passthrough with
req.ProviderName set and assert the provider’s Passthrough is invoked and
response normalized; (2) name miss — ensure providerByNameRegistry returns nil
and Router falls back to resolvePassthroughProvider, asserting the fallback
provider is used and behavior matches existing tests; (3) name hit but provider
lacks PassthroughProvider — register a provider by name that does NOT implement
core.PassthroughProvider, call Router.Passthrough with that name and assert it
falls back to type resolution (or returns correct error/behavior). Use
Router.Passthrough, providerByNameRegistry, resolvePassthroughProvider,
core.PassthroughProvider, core.PassthroughRequest and core.PassthroughResponse
in assertions and cover request translation, response normalization, and error
handling per guidelines.
In `@internal/server/passthrough_semantic_enrichment.go`:
- Around line 43-45: resolvePassthroughProvider can return an empty ProviderName
in fallback cases, so don't unconditionally overwrite info.ProviderName; update
the assignment in the passthrough logic (where resolvePassthroughProvider(...)
is called and info.ProviderName/info.Provider are set) to only set
info.ProviderName when resolved.ProviderName is non-empty, while still assigning
info.Provider = resolved.ProviderType as before to preserve provider type
routing and avoid wiping the original route name before caching.
🪄 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: e166c22d-4b23-4a1e-9a75-f730050a9c0f
📒 Files selected for processing (8)
internal/core/passthrough.gointernal/core/semantic.gointernal/providers/router.gointernal/server/passthrough_execution_helpers.gointernal/server/passthrough_execution_helpers_test.gointernal/server/passthrough_provider_resolution.gointernal/server/passthrough_semantic_enrichment.gointernal/server/passthrough_service.go
| func (r *Router) Passthrough(ctx context.Context, providerType string, req *core.PassthroughRequest) (*core.PassthroughResponse, error) { | ||
| pp, err := r.resolvePassthroughProvider(providerType) | ||
| if err != nil { | ||
| return nil, err | ||
| var pp core.PassthroughProvider | ||
| if req != nil && strings.TrimSpace(req.ProviderName) != "" { | ||
| slog.DebugContext(ctx, "passthrough routing by name", "providerName", req.ProviderName, "providerType", providerType) | ||
| if p := r.providerByNameRegistry(strings.TrimSpace(req.ProviderName)); p != nil { | ||
| if named, ok := p.(core.PassthroughProvider); ok { | ||
| pp = named | ||
| slog.DebugContext(ctx, "passthrough routed by name", "providerName", req.ProviderName) | ||
| } else { | ||
| slog.DebugContext(ctx, "passthrough provider found by name but does not implement PassthroughProvider", "providerName", req.ProviderName) | ||
| } | ||
| } else { | ||
| slog.DebugContext(ctx, "passthrough provider not found by name, falling back to type", "providerName", req.ProviderName, "providerType", providerType) | ||
| } | ||
| } | ||
| if pp == nil { | ||
| var err error | ||
| pp, err = r.resolvePassthroughProvider(providerType) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
| return pp.Passthrough(ctx, req) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add regression tests for the new name-first passthrough branch.
Router.Passthrough() now changes routing precedence, so please cover:
- name hit,
- name miss, and
- name hit where the provider does not implement
PassthroughProvider.
As per coding guidelines, **/*_test.go: Add or update tests for behavior changes. Tests should cover request translation, response normalization, error handling, default configuration, and provider-specific parameter mapping.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/providers/router.go` around lines 772 - 795, Add regression tests
under **/*_test.go that exercise Router.Passthrough’s new name-first routing:
(1) name hit — register a provider implementing core.PassthroughProvider, call
Router.Passthrough with req.ProviderName set and assert the provider’s
Passthrough is invoked and response normalized; (2) name miss — ensure
providerByNameRegistry returns nil and Router falls back to
resolvePassthroughProvider, asserting the fallback provider is used and behavior
matches existing tests; (3) name hit but provider lacks PassthroughProvider —
register a provider by name that does NOT implement core.PassthroughProvider,
call Router.Passthrough with that name and assert it falls back to type
resolution (or returns correct error/behavior). Use Router.Passthrough,
providerByNameRegistry, resolvePassthroughProvider, core.PassthroughProvider,
core.PassthroughRequest and core.PassthroughResponse in assertions and cover
request translation, response normalization, and error handling per guidelines.
| if resolved := resolvePassthroughProvider(provider, info.Provider); resolved.ProviderType != "" { | ||
| info.ProviderName = resolved.ProviderName | ||
| info.Provider = resolved.ProviderType |
There was a problem hiding this comment.
Preserve ProviderName when the resolver cannot supply one.
resolvePassthroughProvider() can return an empty ProviderName in the fallback path. Assigning it unconditionally here can wipe out the original route name before caching, which defeats the new name-based passthrough routing.
🔧 Suggested fix
if resolved := resolvePassthroughProvider(provider, info.Provider); resolved.ProviderType != "" {
- info.ProviderName = resolved.ProviderName
+ if resolved.ProviderName != "" {
+ info.ProviderName = resolved.ProviderName
+ }
info.Provider = resolved.ProviderType
}📝 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.
| if resolved := resolvePassthroughProvider(provider, info.Provider); resolved.ProviderType != "" { | |
| info.ProviderName = resolved.ProviderName | |
| info.Provider = resolved.ProviderType | |
| if resolved := resolvePassthroughProvider(provider, info.Provider); resolved.ProviderType != "" { | |
| if resolved.ProviderName != "" { | |
| info.ProviderName = resolved.ProviderName | |
| } | |
| info.Provider = resolved.ProviderType |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/passthrough_semantic_enrichment.go` around lines 43 - 45,
resolvePassthroughProvider can return an empty ProviderName in fallback cases,
so don't unconditionally overwrite info.ProviderName; update the assignment in
the passthrough logic (where resolvePassthroughProvider(...) is called and
info.ProviderName/info.Provider are set) to only set info.ProviderName when
resolved.ProviderName is non-empty, while still assigning info.Provider =
resolved.ProviderType as before to preserve provider type routing and avoid
wiping the original route name before caching.
1 participant
The semantic enrichment middleware was overwriting info.Provider with the
resolved type (e.g. 'anthropic') before caching, causing passthroughExecutionTarget
to route by type instead of the original configured name (e.g. 'teste').
Added info.ProviderName field to preserve the original route name through the
resolution pipeline. The execution target now prefers the cached provider name
when available, ensuring requests route to the correct configured instance.
Summary by CodeRabbit
New Features
Improvements