Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the channel endpoint management system, transitioning from static frontend configurations to a backend-driven model that distinguishes between dynamically resolved default endpoints and user-provided overrides. The changes include updates to the GraphQL schema, frontend UI components, and backend outbound transformer selection logic to support multiple API formats per channel. Feedback indicates that the ollama/chat format is missing from the validation map and identifies a logic contradiction where the implementation currently blocks overrides of default endpoints despite the architectural intent to allow them.
| llm.APIFormatJinaEmbedding.String(): {}, | ||
| } |
There was a problem hiding this comment.
The ollama/chat API format is missing from the SupportedAPIFormats map. This will cause validation to fail when configuring endpoints for Ollama channels, even though it is defined as a default format in channel_llm.go.
| llm.APIFormatJinaEmbedding.String(): {}, | |
| } | |
| llm.APIFormatJinaEmbedding.String(): {}, | |
| llm.APIFormatOllamaChat.String(): {}, | |
| } |
| defaults := DefaultEndpointsForChannelType(ch.Type) | ||
| for _, userEP := range input.Endpoints { | ||
| for _, defaultEP := range defaults { | ||
| if userEP.APIFormat == defaultEP.APIFormat { | ||
| return nil, fmt.Errorf("endpoint api_format %q conflicts with default endpoint for channel type %s", userEP.APIFormat, ch.Type) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
There is a logic inconsistency regarding endpoint overrides. The comments in SaveChannelEndpointsInput (line 351) and ResolveEndpoints (line 1250 in channel_llm.go) state that user-configured endpoints are intended to override defaults. However, this check explicitly forbids any overlap between user endpoints and defaults, preventing users from customizing the path of a built-in endpoint (e.g., changing the path for openai/chat_completions). If overrides are intended, this validation should be removed.
| if (defaultApiFormats.has(newApiFormat)) { | ||
| setError(t('channels.endpoints.defaultConflictError', 'Cannot override a default endpoint')); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This check prevents users from overriding default endpoints, which contradicts the stated goal of supporting endpoint overrides. If the backend logic in mergeEndpoints is intended to be used, this restriction should be removed from the UI.
| for _, ep := range userEndpoints { | ||
| if ep.APIFormat == "" { | ||
| continue | ||
| } | ||
|
|
||
| out, err := svc.buildNonDefaultEndpointOutbound(c, ch, accountIdentity, ep) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to build outbound for api_format %q on channel %s: %w", ep.APIFormat, c.Name, err) | ||
| } | ||
|
|
||
| outbounds[ep.APIFormat] = out |
There was a problem hiding this comment.
🔴 Missing ollama/chat case in buildNonDefaultEndpointOutbound breaks existing Ollama channels on upgrade
The buildNonDefaultEndpointOutbound switch statement has no case for ollama/chat (or seedance/video). When buildChannelWithOutbounds iterates over user-configured endpoints (c.Endpoints), any ollama/chat entry hits the default error branch at channel_llm.go:301, causing the entire channel build to fail.
This is a real upgrade regression: in the old code, ollama/chat was listed in SupportedAPIFormats (channel_override.go:233), so users could save it as a user endpoint. Furthermore, the old dialog initialized state from defaults (channel.endpoints.length > 0 ? channel.endpoints : defaultEndpoints), so any Ollama user who opened the endpoints dialog and clicked Save would have persisted ollama/chat as a user endpoint. After this PR, the enabled channels cache refresh calls buildChannelWithOutbounds which will error on these pre-existing user endpoints, silently dropping the channel from the active pool.
Fix suggestion
In the user-endpoints loop of `buildChannelWithOutbounds`, skip user endpoints whose `APIFormat` already exists in `defaultEndpoints` (they're already mapped to `ch.Outbound`), or add a data migration to strip default-format entries from the `endpoints` column.Prompt for agents
In buildChannelWithOutbounds (channel_llm.go around line 198-227), the user-endpoint loop unconditionally calls buildNonDefaultEndpointOutbound for every user endpoint. However, buildNonDefaultEndpointOutbound does not handle ollama/chat (or seedance/video), so pre-existing Ollama channels whose endpoints column contains ollama/chat will fail to load.
The simplest fix is to skip user endpoints whose APIFormat is already covered by the default endpoints. In the user-endpoint loop, before calling buildNonDefaultEndpointOutbound, check if outbounds[ep.APIFormat] already exists (it would have been set by the default-endpoint loop above). If it does AND the user endpoint has no custom Path, skip it. If it has a custom Path, build the override normally but also add ollama/chat and seedance/video cases to buildNonDefaultEndpointOutbound.
Alternatively, add a data migration that removes default-format entries from the user endpoints column for all channels.
Was this helpful? React with 👍 or 👎 to provide feedback.
| <label className='text-sm font-medium'>{t('channels.endpoints.defaultEndpoints', 'Default endpoints')}</label> | ||
| <span className='text-muted-foreground text-xs'>{defaultEndpoints.length > 0 && `${defaultEndpoints.length} resolved`}</span> | ||
| </div> | ||
| {endpoints.length === 0 ? ( | ||
| <div className="rounded-lg border border-dashed p-4"> | ||
| <p className="text-sm text-muted-foreground text-center">{t('channels.endpoints.emptyHint')}</p> | ||
| {defaultEndpoints.length === 0 ? ( | ||
| <div className='rounded-lg border border-dashed p-4'> | ||
| <p className='text-muted-foreground text-center text-sm'> | ||
| {t('channels.endpoints.noDefaultEndpoints', 'No default endpoints resolved for this channel type.')} |
There was a problem hiding this comment.
🔴 New i18n keys used in code but missing from locale files (en.json and zh.json)
Per the mandatory rule in .agent/rules/frontend-i18n.md: "Any new i18n key used in code must be added to both frontend/src/locales/en.json and frontend/src/locales/zh.json."
The dialog component introduces at least 8 new translation keys (channels.endpoints.defaultEndpoints, channels.endpoints.noDefaultEndpoints, channels.endpoints.primaryBadge, channels.endpoints.readOnly, channels.endpoints.noOverridesHint, channels.endpoints.unsupportedBadge, channels.endpoints.invalidApiFormat, channels.endpoints.defaultConflictError) but the PR diff contains no changes to either locale file. While the code uses inline fallback strings (second argument to t()), the Chinese locale will show English fallbacks instead of proper translations.
Prompt for agents
Add all new i18n keys used in channels-endpoints-dialog.tsx to both frontend/src/locales/en.json and frontend/src/locales/zh.json under the channels.endpoints namespace. The keys to add are: defaultEndpoints, noDefaultEndpoints, primaryBadge, readOnly, noOverridesHint, unsupportedBadge, invalidApiFormat, and defaultConflictError. Use the inline fallback strings as the English values and provide appropriate Chinese translations in zh.json.
Was this helpful? React with 👍 or 👎 to provide feedback.
Greptile SummaryThis PR refactors the channel endpoint model to separate default endpoints (built-in, read-only capability surfaces derived from channel type) from user-configured custom endpoints (additive overrides). Default endpoints are now resolved dynamically server-side and exposed via a new
Confidence Score: 5/5Safe to merge; all findings are P2 (design/style/theoretical) with no runtime correctness bugs. The refactor is well-structured with good test coverage (new test files for Gemini, NanoGPT, OpenAI-compatible channels, and a comprehensive mapping test). No P0/P1 bugs were found. The P2 comments cover: a theoretical non-atomic DB check, an implicit behavioral contract on the primary outbound shared across all default formats, a global linter suppression, and a test that exercises a code path now blocked by the API. None of these block merge.
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Channel Request] --> B{Route by API Format}
B --> C[BuildOutboundByAPIFormat]
C --> D{Outbounds map lookup}
D -->|Found| E[Return matched outbound]
D -->|Not found| F[Return ch.Outbound primary]
G[buildChannelWithOutbounds] --> H[DefaultEndpointsForChannelType]
H --> I[Map ALL defaults to ch.Outbound primary]
G --> J[User c.Endpoints]
J --> K[buildNonDefaultEndpointOutbound]
K --> L[New format-specific transformer]
I --> M[outbounds map]
L --> M
M --> C
N[SaveChannelEndpoints] --> O[ValidateEndpoints format/path]
O --> P[Get channel from DB]
P --> Q{Conflict with defaults?}
Q -->|Yes| R[Return error]
Q -->|No| S[UpdateOne endpoints]
T[ResolveEndpoints] --> U[mergeEndpoints]
U --> V[defaults + user extras]
|
| } | ||
|
|
||
| func mergeEndpoints(defaultEndpoints, userEndpoints []objects.ChannelEndpoint) []objects.ChannelEndpoint { | ||
| if len(defaultEndpoints) == 0 && len(userEndpoints) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| merged := make([]objects.ChannelEndpoint, 0, len(defaultEndpoints)+len(userEndpoints)) | ||
|
|
||
| overrides := make(map[string]objects.ChannelEndpoint, len(userEndpoints)) | ||
|
|
||
| for _, ep := range userEndpoints { | ||
| if ep.APIFormat == "" { | ||
| continue | ||
| } | ||
|
|
||
| overrides[ep.APIFormat] = ep | ||
| } | ||
|
|
||
| for _, ep := range defaultEndpoints { | ||
| if ep.APIFormat == "" { | ||
| continue | ||
| } | ||
|
|
||
| if override, ok := overrides[ep.APIFormat]; ok { | ||
| merged = append(merged, override) | ||
|
|
||
| delete(overrides, ep.APIFormat) | ||
|
|
||
| continue |
There was a problem hiding this comment.
Every API format in defaultEndpoints is unconditionally mapped to ch.Outbound (same pointer for every format). This works today because each channel type's primary transformer inspects the incoming request type to decide which provider endpoint to call—not the key it was stored under. However, this is a silent implicit contract. If a future default endpoint is added for a channel type whose primary transformer does not handle that format, the outbound will silently dispatch incorrect requests without any compile- or runtime error. A comment on this invariant, or an assertion in a test, would make the assumption explicit.
Uh oh!
There was an error while loading. Please reload this page.