feat!: profile-driven upstream routing and per-profile image analysis#36
Open
flobernd wants to merge 18 commits into
Open
feat!: profile-driven upstream routing and per-profile image analysis#36flobernd wants to merge 18 commits into
flobernd wants to merge 18 commits into
Conversation
A request model's routing target will be named by a model profile (later tasks), so an `upstreams:` entry no longer needs its own `upstream_model` remap or nested `fallback_upstreams` chain. Those keys are now detected and rejected with the replacement they migrate to, rather than silently ignored, and an entry's name is required and validated unique so a profile can reference it unambiguously. `UpstreamConfig`/`PersistedUpstream` fields are renamed to their short forms (`url`, `api_key`, `chat_kwargs`, `request_log_path`), with serde aliases for the old `upstream_*` names as the only back-compat path. Removing the per-entry `fallback_upstreams` field breaks 3 integration tests that exercised routing-mode fallback dispatch end-to-end (selected-upstream failover to a nested fallback, and an exposed fallback alias being listed/routed); that dispatch has no equivalent in the still-supported global `fallback_upstreams` (non-routing failover) path, so those tests are deleted rather than ported. The GLOBAL top-level `fallback_upstreams` (non-routing failover mode) is untouched.
Adds the persisted and runtime schema for profile-driven routing: a profile's `upstream` (named endpoint), ordered `fallbacks` (upstream + optional model remap, bare-string or table form), and `image_analysis` redirect (model + residual-image policy). `native_vision` becomes removed-key detection only, erroring at startup toward `image_analysis` as its replacement; `Config::profile_native_vision` now always returns None so existing G4 gating falls through to its name-based heuristic. No referential validation of these fields' targets yet -- that lands with resolve_route in a later task. Retiring the profile native_vision knob breaks tests that relied on it to force a native-vision backend. Tests that exercised the knob's own mechanics (request override, candidate lookup, decision-table cells) have no substitute and are deleted (listed in task-2-report.md); tests that only used it incidentally, to observe unrelated behavior on a native backend (multimodal content-part fidelity, tool_result image conversion, request-log redaction on a passthrough path), are migrated onto the surviving Kimi name-sniff fallback instead.
The single-upstream knobs (upstream_base_url/api_key/model), the ad-hoc model_routes map, the global fallback_upstreams chain, and the vision knobs (image_agent_enabled, vision_url/model, unsupported_image_policy) are all replaced by named `upstreams` entries plus `model_profiles`. Leaving these keys parsed-but-ignored would silently mislead an upgraded config into thinking it still routed a certain way, so each is now a hard startup error that names its replacement and points at the README migration notes, rather than a warning that scrolls past unheeded. `PersistedConfig::default()` becomes the minimal profile config (one `default` upstream plus a `"*"` catch-all profile) so a missing config file still proxies to localhost out of the box, and OPENAI_API_KEY now seeds any `upstreams` entry that declares no key of its own. The `--model-route` CLI flag and its plumbing die with the persisted routes; `configure` writes the new upstreams/profiles shape. The runtime routing and vision fields are left inert (always empty) for now; Tasks 6-9 remove that machinery.
Replace the runtime `model_profiles` BTreeMap with an ordered `Vec<CompiledProfile>` and add `Config::resolve_route`, the single entry point for resolving a request model to a profile and served model: an exact key (trimmed, case-insensitive) beats any glob, globs match first in declaration order, and `served_model` defaults to `upstream_model`, then the exact key, then the passed-through request model. Tighten `ModelProfile.upstream` to a required `String` and validate profile references in `from_persisted`: the primary upstream must exist, fallbacks must be known, distinct from the primary, and unique, and an `image_analysis` redirect must target an existing exact-key profile that is not itself, a glob, or a profile that also sets `image_analysis`. Collapse the request-model shaping helpers (system prompt prefix, roles) onto the single profile from `resolve_route`, and key the leaf finalization policies by the effective backend model with an ordered glob fallback, so the upstream leaf resolves per-model kwargs/effort/family the same way routing does. Add `exact_profile_keys` and `upstream_by_name` accessors.
…m cooldown Add ProfileRoutingClient, which resolves each request model to a model profile (Config::resolve_route) and dispatches over that profile's failover chain: the primary named upstream plus its declared fallbacks. Profiles share one provider per named upstream, so cooldown is keyed by upstream name and observed across every profile that routes to it. Generalize the shared failover dispatch (stream/count/proxy *_with_provider_indices) from a bare provider-index list to a Vec<ChainStep> so the model override rides on the chain step rather than the provider. The existing failover/routing callers map their indices to steps with the per-provider static upstream_model rewrite, preserving prior behavior. The existing routing/failover/single-mode clients are left intact; lib.rs wiring is swapped in a later change.
Wire ProfileRoutingClient as the single upstream client and delete the legacy routing machinery it replaces. build_app now always builds one FailoverUpstreamProvider per configured upstreams entry (pure named endpoints) and dispatches every request through the profile router, so the routing_mode split and the standalone/bare-primary/global-failover branches are gone. Remove the now-unreferenced RoutingUpstreamClient and its catalog resolve machinery, RouteUpstreamProvider, ModelRouteSpec, the exposed-alias plumbing, and the bare-primary marker path (into_bare_primary), so a single TTL catalog cache (ProfileRoutingClient's) remains. Delete the inert legacy Config fields (upstream_base_url, upstream_api_key, upstream_model, fallback_upstreams, model_routes) and their consumers; the engine's vestigial route-match and global-upstream_model fallbacks drop with them. Persisted legacy knobs still hard-error. Migrate the test suite to the profile shape (named upstreams plus a catch-all or specific profiles) and drop tests that asserted legacy single-mode, global-fallback, or catalog-union semantics with no profile equivalent.
The engine ran its own pre-dispatch resolution ladder that rewrote the
dispatched request model to a profile's upstream_model, a catalog canonical
id, or the catalog default, and ProfileRoutingClient then re-resolved that
rewritten id. Any non-catch-all profile whose key was not itself a served id
(for example one with upstream_model set) therefore 404d end to end; the suite
stayed green only because the migrated configs used a "*" catch-all.
Resolve the route exactly once, in the router. The backend request keeps the
original (trimmed) request model and each chain step supplies the per-provider
POSTed model. The engine still resolves the route for its own needs (served
label, price/header key, capture, non-routing budgeting fallback) through a
thin resolve_request_model wrapper that returns the route's served_model.
Unknown models now surface a clean 404 before any upstream call
("model \"x\" is not served by any configured model profile") and a blank model
with no glob to resolve it is a 400, both from the engine streaming path and the
HTTP label-resolution sites. The exact-id / canonical-key / catalog-default
ladder, should_warn_model_fallback, and the genuine flag are deleted; the engine
catalog is kept only for context-limit metadata.
BREAKING CHANGE: an unserved model returns 404 (blank returns 400) instead of
collapsing to a catalog default, and the served-model label is now the route's
served model (profile key for exact profiles, request model for globs) rather
than a catalog-normalized id.
…ault Image handling is now driven by the resolved profile's `image_analysis` redirect instead of a global image-agent switch and vision endpoint. When a profile declares `image_analysis`, the latest user turn's images are stripped to placeholders, the `analyzeImage` server tool is injected, and the analysis call is dispatched to the analyzer model through the gateway's own upstreams: the router resolves it to the analyzer profile's upstream and served model like any other request, so no separate endpoint is configured. The residual sweep runs only for such profiles, applying the profile's `residual_images` policy (placeholder degrade with a replay-cache bypass, or a pre-dispatch 4xx reject). BREAKING CHANGE: native passthrough replaces degrade-by-default. A profile without `image_analysis` now receives images verbatim regardless of model name. The name-based native-vision sniff, the runtime `image_agent_enabled`, `vision_url`, `vision_model`, and `unsupported_image_policy` knobs, and the standalone vision client are removed.
`get_models` proxied the first configured upstream's `/v1/models` listing verbatim - a placeholder from before profile routing existed, and one that leaked whichever upstream happened to be first rather than describing what the gateway actually serves. It now synthesizes the OpenAI-shaped list directly from the configured profiles: one entry per exact-key profile (glob profiles carry no fixed id to advertise), in declaration order, with `context_length` filled in from the router's unioned model catalog when it knows the profile's served model. A catalog-load failure just omits `context_length`; it never fails the response. The Anthropic reshaping (`transform_models_response_for_anthropic`) is unchanged and now operates over this synthesized list, so `capabilities` merge, pagination cursors, and the display/created-at fallbacks all still apply - just to profile-derived entries rather than raw upstream fields. `ProfileRoutingClient::list_models` (the trait's now-unreachable proxy placeholder) returns an explicit error pointing at `supported_model_catalog` instead of silently proxying the first upstream. Also opportunistically populates `provider_health`'s `catalog_fetched_ms`/`catalog_size` from the shared catalog cache via a non-blocking `try_lock`, since they'd been stuck at `None` since the routing client rewrite. BREAKING CHANGE: `/v1/models` now advertises the configured model profiles instead of proxying the first upstream's catalog. Ids are profile keys, `owned_by` is always `"llmconduit"`, and upstream-only fields (`display_name`, `created`/`created_at`, `max_output_tokens`, per-model `capabilities`) are gone from the raw OpenAI-shaped body - the Anthropic-flavored dressing still fills sensible defaults for those. The response also no longer carries an `ETag` header.
Extract build_configured_persisted_config out of run_configure_flow so the pure config-construction part of the interactive wizard can be exercised without a TTY. Cover both the blank-default-model case (request model passes through) and the entered-default-model case (profile serves the entered model), routing through Config::from_persisted + resolve_route as an integration check of the emitted upstreams/model_profiles shape.
Rewrite the README Configuration section for named upstreams and glob-capable model_profiles (upstream/upstream_model/fallbacks/ image_analysis), add a Migrating section covering every removed top-level knob, and fix AGENTS.md invariants left over from catalog-union routing, nested fallback_upstreams, and the vision knobs. Add the now-required upstream field plus a commented image_analysis example to the GLM-5.2 reference profile.
Scrub remaining planning-label references from comments, correct stale doc comments that described deleted machinery, and tighten two tests: - Reword the removed-key rejection and shorthand-sweep comments so they describe current behavior without task numbers or stale enumerations. - Fix the redaction, request-log, and error-body comments that still narrated deleted vision-agent gating states and the removed VisionOutcome.text field. - Correct the ProviderHealth.catalog_size doc: it is the client-wide union catalog size stamped onto every provider, not a per-provider snapshot. - Delete the unused latest_user_message_has_images predicate, its re-export, and its tests; it had no production caller. - Make the /v1/models declaration-order test discriminate a sort regression by declaring profile keys out of alphabetical order. - Add a startup-validation case for a blank upstream name.
An upgraded config could declare upstreams but no model_profiles at all, starting up successfully while every request 404s at routing time. Fail at startup instead, naming the catch-all fix and pointing at the README migration section, matching the other removed/misconfigured-key errors. Several existing fixtures relied on a profile-less config resolving cleanly to exercise unrelated behavior (debug_log_dirs, price_table, env-seeded upstreams); updated them to either supply a minimal profile or, where the test genuinely needs zero upstreams, build the resolved Config directly instead of going through PersistedConfig. Also, while touching this file and its neighbors: - Reworded the DEFAULT_UPSTREAM_BASE_URL doc to describe its current role (seeding the default upstream entry and the wizard's URL prompt) instead of a removed field and a stale task reference. - Reworded the catalog_fetched_ms doc to match its already-correct sibling catalog_size: the value is the routing client's client-wide union snapshot stamped identically onto every provider, not a per-provider fetch time. - Replaced stray em-dashes introduced since a96abaf with spaced hyphens across engine.rs, upstream.rs, vision/mod.rs, and a few test files.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe gateway migrates to named upstreams, ordered model profiles, profile-scoped failover, local model listings, and gateway-routed image analysis. Configuration, CLI behavior, runtime wiring, documentation, and tests are updated. ChangesProfile routing and image analysis
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
This was referenced Jul 17, 2026
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/engine.rs (1)
2818-2830: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winReplay lookup should use the served model label, not
request.model.
ReplayRecordis stored withmodel: served_model, butfind_replay_baselinestill hashes&request.model, so any profile that remaps the model name will never hit the replay cache. Use the same model string on both insert and lookup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/engine.rs` around lines 2818 - 2830, Update find_replay_baseline to hash and look up using the served model label, matching the model value stored in ReplayRecord. Reuse the same served-model resolution used by the replay insertion path, while preserving existing lookup behavior for non-remapped models.
🧹 Nitpick comments (3)
tests/port_server.rs (1)
387-396: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the opening comment with the defer-to-provider behavior.
The test now expects HTTP 200 and one upstream POST, while Lines 371-376 still claim HTTP 400 and zero POSTs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/port_server.rs` around lines 387 - 396, Update the opening comment for the chat completions mock and its associated assertions to consistently describe the defer-to-provider behavior: expect HTTP 200 with one upstream POST, not HTTP 400 with zero POSTs. Use the existing test symbols and preserve the current SSE response setup.tests/image_agent.rs (1)
3-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the actual request-wide image-stripping scope.
strip_and_cache_imagesrewritesimage_urlparts in every user message, not only the latest turn.
tests/image_agent.rs#L3-L5: say user-message images across request history are stripped.tests/image_agent.rs#L748-L750: narrow residual examples tofile_idimages and images in non-user roles.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/image_agent.rs` around lines 3 - 5, The documentation at tests/image_agent.rs lines 3-5 must state that image_url parts in user messages across the full request history are stripped and cached, not only images in the latest user turn. Update the residual examples at tests/image_agent.rs lines 748-750 to refer specifically to file_id images and images in non-user roles; no other behavior changes are needed.tests/gateway.rs (1)
8237-8240: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the stale model-name-sniff rationale.
Native passthrough now follows from the profile lacking
image_analysis, not from recognizing Kimi by name.
tests/gateway.rs#L8237-L8240: describe passthrough as profile-driven.tests/gateway.rs#L8990-L8993: remove the Kimi name-sniff claim.tests/gateway.rs#L9084-L9088: attribute raw image forwarding to the absentimage_analysissetting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/gateway.rs` around lines 8237 - 8240, Update the comments in tests/gateway.rs at 8237-8240, 8990-8993, and 9084-9088 to remove the stale Kimi name-sniff rationale and describe native/raw image passthrough as driven by the profile’s absent image_analysis setting; no code behavior changes are needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli.rs`:
- Around line 168-188: Update the existing_default lookup and catch-all model
profile lookup in the surrounding configuration merge logic to normalize names
by trimming whitespace and comparing case-insensitively. Apply the same
normalization to the "*" profile key, while preserving the current fallback
behavior only when no normalized default upstream matches.
In `@src/config.rs`:
- Around line 1044-1050: Update the From<PersistedProfileFallback> for
ProfileFallback conversion to normalize persisted.model before constructing
ProfileFallback: trim surrounding whitespace and convert a whitespace-only value
to the same empty/absent representation used for primary upstream_model values.
Leave persisted.upstream handling unchanged.
- Around line 2286-2323: Remove the implicit OPENAI_API_KEY fallback from the
upstream construction in the upstream parsing flow. In the upstreams loop, set
UpstreamConfig.api_key only from the individual entry.api_key after
trim_nonempty, so local and third-party endpoints receive no ambient credential
unless explicitly configured.
---
Outside diff comments:
In `@src/engine.rs`:
- Around line 2818-2830: Update find_replay_baseline to hash and look up using
the served model label, matching the model value stored in ReplayRecord. Reuse
the same served-model resolution used by the replay insertion path, while
preserving existing lookup behavior for non-remapped models.
---
Nitpick comments:
In `@tests/gateway.rs`:
- Around line 8237-8240: Update the comments in tests/gateway.rs at 8237-8240,
8990-8993, and 9084-9088 to remove the stale Kimi name-sniff rationale and
describe native/raw image passthrough as driven by the profile’s absent
image_analysis setting; no code behavior changes are needed.
In `@tests/image_agent.rs`:
- Around line 3-5: The documentation at tests/image_agent.rs lines 3-5 must
state that image_url parts in user messages across the full request history are
stripped and cached, not only images in the latest user turn. Update the
residual examples at tests/image_agent.rs lines 748-750 to refer specifically to
file_id images and images in non-user roles; no other behavior changes are
needed.
In `@tests/port_server.rs`:
- Around line 387-396: Update the opening comment for the chat completions mock
and its associated assertions to consistently describe the defer-to-provider
behavior: expect HTTP 200 with one upstream POST, not HTTP 400 with zero POSTs.
Use the existing test symbols and preserve the current SSE response setup.
🪄 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: CHILL
Plan: Pro Plus
Run ID: cacc4ec0-2820-4b4e-b1c3-f6acf470ade0
📒 Files selected for processing (30)
AGENTS.mdREADME.mdprofiles/glm52.yamlsrc/adapters/responses_to_anthropic/tests.rssrc/cli.rssrc/config.rssrc/engine.rssrc/http.rssrc/lib.rssrc/main.rssrc/redaction.rssrc/search.rssrc/upstream.rssrc/vision/cache.rssrc/vision/client.rssrc/vision/mod.rssrc/vision/strip.rstests/common/mod.rstests/dashboard_provider_health.rstests/dashboard_serving_token.rstests/gateway.rstests/http_body_limit.rstests/image_agent.rstests/port_config.rstests/port_config_routing.rstests/port_errors.rstests/port_routing.rstests/port_server.rstests/port_streaming_peek.rstests/port_translation.rs
💤 Files with no reviewable changes (2)
- tests/dashboard_serving_token.rs
- src/vision/client.rs
The configure wizard matched the default upstream and the catch-all profile key exactly, while runtime resolution trims names and compares upstream names case-insensitively. A config with 'name: Default' was valid at runtime but made the wizard pre-fill from an unrelated first entry and overwrite the config with that entry's URL and credentials. Extract the two lookups into helpers so they are testable without a TTY, and match the default upstream trimmed and case-insensitively and the '*' profile key trimmed.
Primary upstream_model values are normalized with trim_nonempty, but fallback model overrides were passed through verbatim. A padded value was posted with its whitespace on failover, and a whitespace-only value was posted as-is instead of falling back to the served model.
Seeding every keyless upstreams entry from the environment leaked an ambient OpenAI credential to local and third-party endpoints, and an empty api_key could not opt out because it trims to absent and inherits anyway. An entry now authenticates only with its own api_key.
Removing the implicit OPENAI_API_KEY fallback left no way to keep an upstream key out of the config file. api_key_env names an environment variable to read the key from, resolved once at startup. It is mutually exclusive with api_key, and a declared-but-unset (or blank) variable is a startup error so a missing key fails loud instead of surfacing as upstream 401s. The configure wizard carries the field through unchanged unless a literal key is entered.
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Collapses all routing into one rule: a request model resolves to a model profile, and the profile names its upstream, failover chain, and optional image-analysis redirect. Upstreams become pure named endpoints; the legacy global knobs, catalog-union routing,
model_routes, and the global vision offload are removed, each failing startup with a message naming its replacement (see the new README "Migrating" section).Intention
Routing previously had three overlapping mechanisms (global
upstream_*knobs, catalog-union discovery,model_routes) plus a fourth global image subsystem, and they all interacted. This replaces them with a single explicit mechanism and deletes roughly 4,600 net lines.Testing
Full
cargo test(748 unit + ~320 integration tests, 1069 total, 0 failed),cargo clippy --all-targetswith zero warnings,cargo fmt --checkclean. Wire-level tests cover profile routing, fallback model rewrites, cooldown shared across profiles, the 404/400 taxonomy, the image redirect and native passthrough, and a startup-error test per removed key.Notes: breaking changes
upstream_model./v1/modelsadvertises the configured profiles (declaration order) instead of the upstream catalog union, and no longer forwards the upstream ETag.image_analysis; the global vision offload andunsupported_image_policyare gone.upstream_base_url,upstream_api_key,upstream_model,fallback_upstreams,model_routes,image_agent_enabled,vision_url,vision_model,unsupported_image_policy, profilenative_vision, per-entryupstream_modeland nestedfallback_upstreams(includingexposed_model).LLMCONDUIT_UPSTREAM_BASE_URL,LLMCONDUIT_UPSTREAM_API_KEY,LLMCONDUIT_UPSTREAM_MODEL, and the vision/image env overrides are removed and now ignored.OPENAI_API_KEYis no longer read implicitly; a keylessupstreamsentry sends no credentials. Each entry declares its ownapi_key, or names an environment variable via the newapi_key_env(mutually exclusive withapi_key; an unset or blank variable is a startup error).[DONE]now counts as a failed attempt (failover and cooldown apply).upstream_model(Key the replay cache by served model on lookup #32).llmconduit configurerewrites the config to the minimal shape and collapses multi-upstream configs;--model-routeis removed.Related
Follow-ups tracked in #32, #33, #34, #35.
Summary by CodeRabbit
upstream_modelrewriting, and per-profile fallbacks.image_analysis, including image stripping and residual handling (placeholder vs reject) with routing through upstreams.GET /v1/modelsto list configured profile models locally.upstreams/model_profilessemantics and request logging details.