From eff3a5a9cb68f138e4b9412d4cd276f67b2869e4 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 12 Jun 2026 05:48:30 -0400 Subject: [PATCH] refactor(llm): resolve request dispatch through the catalog route (#496) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR 8 of the gateway refactor series (after #493) — the optional closer: `Client` dispatch goes through the route machinery #493 introduced, instead of an inline ad-hoc lookup. ## What's here `Client::resolve_provider`'s hand-rolled catalog hop (`catalog.get(model)` → provider id) becomes `adapter_registry::resolve_route`. Fallback order is byte-identical: explicit `request.provider` wins, then the model's catalog route, then the default provider, then the existing configuration error. This puts route resolution on the live request path, so the route-equivalence table from #493 now pins actual dispatch rather than a helper nothing calls: a new live-dispatch sweep asserts every built-in model's request lands on the provider its route names, alongside explicit-provider-wins and unknown-model-default pins. ## Scope notes - **No public API change** — `resolve_provider` is private; all frozen `Client` methods are untouched. - The route's `codec`/`deployment_id` still aren't handed to adapters: `ProviderAdapter::complete(&Request)` is frozen (prod-implemented in fabro-cli), and every allowed pairing equals the adapter's built-in codec until the feature PRs. This PR is deliberately just the dispatch seam, so the OpenRouter redo's Client-side wiring is a no-op. ## Verification - `cargo nextest run --workspace --no-fail-fast` (post-rebase onto #493's merge): green except the same 5 pre-existing environment-dependent fabro-workflow failures, identical on main - clippy `-D warnings` + pinned-nightly fmt clean - Wire snapshots untouched This closes the refactor series. Remaining: the already-open cost PR (#494), then the feature redos — OpenRouter (#438) and Bedrock (#459). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 --- lib/crates/fabro-llm/src/client.rs | 78 +++++++++++++++++++++++++++--- 1 file changed, 70 insertions(+), 8 deletions(-) diff --git a/lib/crates/fabro-llm/src/client.rs b/lib/crates/fabro-llm/src/client.rs index 03f016f6c..84a7a98fa 100644 --- a/lib/crates/fabro-llm/src/client.rs +++ b/lib/crates/fabro-llm/src/client.rs @@ -6,7 +6,7 @@ use fabro_model::{AdapterKind, Catalog, ProviderId}; use tracing::debug; use crate::adapter_registry::{ - AdapterConfig, AdapterKindOptions, OpenAiAdapterOptions, factory_for, + self, AdapterConfig, AdapterKindOptions, OpenAiAdapterOptions, factory_for, }; use crate::cost; use crate::error::{Error, ProviderErrorKind}; @@ -259,18 +259,18 @@ impl Client { ) } - /// Resolve the provider for a request. + /// Resolve the provider for a request: an explicit `request.provider` + /// wins, then the model's catalog route, then the default provider. fn resolve_provider(&self, request: &Request) -> Result, Error> { - let catalog_provider = self.catalog.as_ref().and_then(|catalog| { - catalog - .get(&request.model) - .map(|info| info.provider.to_string()) - }); + let route = self + .catalog + .as_ref() + .and_then(|catalog| adapter_registry::resolve_route(catalog, &request.model)); let provider_name = request .provider .as_deref() - .or(catalog_provider.as_deref()) + .or_else(|| route.as_ref().map(|route| route.provider.as_str())) .or(self.default_provider.as_deref()) .ok_or_else(|| Error::Configuration { message: "No provider specified and no default provider set".into(), @@ -1577,6 +1577,68 @@ reasoning = false assert_eq!(provider.name(), "acme"); } + /// Build a Client with one registered mock per catalog provider, so + /// dispatch tests can observe which provider a request resolves to. + async fn client_with_all_catalog_providers(catalog: &Arc) -> Client { + let mut client = Client::new(HashMap::new(), None, vec![]); + for provider in catalog.providers() { + client + .register_provider(Arc::new(MockProvider::new(provider.id.as_str(), "ok"))) + .await + .unwrap(); + } + client.catalog = Some(Arc::clone(catalog)); + client + } + + /// Live-dispatch counterpart of the adapter_registry route-equivalence + /// table: for every built-in model, `resolve_provider` lands on the same + /// provider the resolved route names. + #[tokio::test] + async fn dispatch_agrees_with_resolve_route_for_every_builtin_model() { + let catalog = catalog_with(""); + let client = client_with_all_catalog_providers(&catalog).await; + + for model in catalog.list(None) { + let route = adapter_registry::resolve_route(&catalog, &model.id) + .expect("built-in model should resolve to a route"); + let mut request = test_request(); + request.model = model.id.clone(); + + let provider = client.resolve_provider(&request).unwrap(); + + assert_eq!(provider.name(), route.provider.as_str(), "{}", model.id); + } + } + + #[tokio::test] + async fn explicit_provider_wins_over_the_model_route() { + let catalog = catalog_with(""); + let client = client_with_all_catalog_providers(&catalog).await; + + let mut request = test_request(); + request.model = "gpt-5.4-mini".to_string(); + request.provider = Some("anthropic".to_string()); + + let provider = client.resolve_provider(&request).unwrap(); + + assert_eq!(provider.name(), "anthropic"); + } + + #[tokio::test] + async fn unknown_model_falls_back_to_default_provider() { + let catalog = catalog_with(""); + let client = client_with_all_catalog_providers(&catalog).await; + let default = client.default_provider().unwrap().to_string(); + + let mut request = test_request(); + request.model = "model-not-in-any-catalog".to_string(); + + let provider = client.resolve_provider(&request).unwrap(); + + assert_eq!(provider.name(), default); + } + #[tokio::test] async fn from_credentials_registers_no_auth_provider_with_extra_headers() { let catalog = catalog_with(