From 3cfac20343b1b73a79d31e64b830d532dad604a5 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 24 Jul 2026 07:12:01 -0400 Subject: [PATCH 1/3] test: capture preflight provider routing gap --- lib/apps/fabro-server/src/run_manifest.rs | 149 ++++++++++++++++++++++ 1 file changed, 149 insertions(+) diff --git a/lib/apps/fabro-server/src/run_manifest.rs b/lib/apps/fabro-server/src/run_manifest.rs index 259e3d2b5..375ef57e5 100644 --- a/lib/apps/fabro-server/src/run_manifest.rs +++ b/lib/apps/fabro-server/src/run_manifest.rs @@ -1470,6 +1470,81 @@ mod tests { Arc::new(Catalog::from_builtin().unwrap()) } + fn openai_compatible_completion(model: &str) -> serde_json::Value { + serde_json::json!({ + "id": "chatcmpl_preflight", + "object": "chat.completion", + "created": 1_700_000_000, + "model": model, + "choices": [{ + "index": 0, + "message": {"role": "assistant", "content": "OK"}, + "finish_reason": "stop" + }], + "usage": {"prompt_tokens": 1, "completion_tokens": 1, "total_tokens": 2} + }) + } + + fn ready_kimi_and_openrouter_state( + server: &httpmock::MockServer, + ) -> Arc { + let kimi_url = server.url("/kimi/v1"); + let openrouter_url = server.url("/openrouter/v1"); + let llm_catalog_settings: LlmCatalogSettings = toml::from_str(&format!( + r#" +[providers.kimi] +base_url = "{kimi_url}" + +[providers.openrouter] +base_url = "{openrouter_url}" +enabled = true +"# + )) + .expect("catalog overrides should parse"); + + crate::test_support::TestAppStateBuilder::new() + .llm_catalog_settings(llm_catalog_settings) + .vault_entries([ + (EnvVars::KIMI_API_KEY, "test-kimi-key"), + (EnvVars::OPENROUTER_API_KEY, "test-openrouter-key"), + ]) + .build() + } + + async fn preflight_for_model( + state: &Arc, + model: &str, + ) -> (types::PreflightResponse, bool) { + let mut ready_providers = state.ready_llm_provider_ids().await; + ready_providers.sort(); + assert_eq!(ready_providers, vec![ + ProviderId::new("kimi"), + ProviderId::new("openrouter") + ]); + + let mut manifest = minimal_manifest(); + manifest.workflows.get_mut("workflow.fabro").unwrap().source = format!( + r#" +digraph Demo {{ + start [shape=Mdiamond] + exit [shape=Msquare] + work [prompt="Do work", model="{model}"] + start -> work -> exit +}} +"# + ); + let prepared = prepare_manifest( + &manifest_run_defaults(Some(&default_settings_fixture())), + &manifest, + ) + .unwrap(); + let validated = validate_prepared_manifest(&prepared, state.catalog()).unwrap(); + + run_preflight(state.as_ref(), &prepared, &validated) + .await + .unwrap() + } + fn manifest_workflow() -> types::ManifestWorkflow { types::ManifestWorkflow { config: None, @@ -2390,6 +2465,80 @@ digraph Demo { assert!(response_mock.calls_async().await >= 1); } + #[tokio::test] + async fn preflight_uses_ready_providers_for_known_shared_alias() { + let server = httpmock::MockServer::start_async().await; + let openrouter_probe = server + .mock_async(|when, then| { + when.method(httpmock::Method::POST) + .path("/openrouter/v1/chat/completions") + .header("authorization", "Bearer test-openrouter-key") + .json_body_includes(r#"{"model":"anthropic/claude-fable-5"}"#); + then.status(200) + .header("content-type", "application/json") + .json_body(openai_compatible_completion("anthropic/claude-fable-5")); + }) + .await; + let state = ready_kimi_and_openrouter_state(&server); + + let (response, _ok) = preflight_for_model(&state, "claude-fable").await; + + let llm_check = response.checks.sections[0] + .checks + .iter() + .find(|check| check.name == "LLM" && check.summary == "claude-fable-5") + .expect("preflight should include Claude Fable"); + assert_eq!( + llm_check + .details + .iter() + .map(|detail| detail.text.as_str()) + .find(|detail| detail.starts_with("Provider: ")), + Some("Provider: openrouter") + ); + assert_eq!(llm_check.status, types::PreflightCheckResultStatus::Pass); + openrouter_probe.assert_async().await; + } + + #[tokio::test] + async fn preflight_uses_ready_providers_for_unknown_unqualified_model() { + let server = httpmock::MockServer::start_async().await; + let kimi_probe = server + .mock_async(|when, then| { + when.method(httpmock::Method::POST) + .path("/kimi/v1/chat/completions") + .header("authorization", "Bearer test-kimi-key") + .json_body_includes(r#"{"model":"provider-private-preview"}"#); + then.status(200) + .header("content-type", "application/json") + .json_body(openai_compatible_completion("provider-private-preview")); + }) + .await; + let state = ready_kimi_and_openrouter_state(&server); + + let (response, _ok) = preflight_for_model(&state, "provider-private-preview").await; + + assert!(response.workflow.diagnostics.iter().any(|diagnostic| { + diagnostic.rule == "node_model_known" + && diagnostic.message.contains("provider-private-preview") + })); + let llm_check = response.checks.sections[0] + .checks + .iter() + .find(|check| check.name == "LLM" && check.summary == "provider-private-preview") + .expect("preflight should include the unknown passthrough model"); + assert_eq!( + llm_check + .details + .iter() + .map(|detail| detail.text.as_str()) + .find(|detail| detail.starts_with("Provider: ")), + Some("Provider: kimi") + ); + assert_eq!(llm_check.status, types::PreflightCheckResultStatus::Pass); + kimi_probe.assert_async().await; + } + #[test] fn static_validation_rejects_unknown_llm_provider() { let mut manifest = minimal_manifest(); From 1c1ea53093cced949bf9c49c1d547b9bd60b132d Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 24 Jul 2026 07:28:51 -0400 Subject: [PATCH 2/3] fix: prefer ready providers during preflight --- lib/apps/fabro-server/src/run_manifest.rs | 192 ++++++++++++------ .../fabro-server/src/server/handler/runs.rs | 24 ++- .../fabro-workflow/src/operations/create.rs | 6 + .../fabro-workflow/src/operations/mod.rs | 2 +- .../fabro-workflow/src/operations/validate.rs | 33 ++- .../fabro-workflow/src/pipeline/transform.rs | 5 + .../fabro-workflow/src/pipeline/types.rs | 1 + .../fabro-workflow/src/pipeline/validate.rs | 1 + .../fabro-workflow/src/run_materialization.rs | 47 ++++- .../src/transforms/model_resolution.rs | 74 ++++++- .../fabro-workflow/tests/it/integration.rs | 1 + lib/foundation/fabro-model/src/catalog.rs | 61 ++++++ 12 files changed, 365 insertions(+), 82 deletions(-) diff --git a/lib/apps/fabro-server/src/run_manifest.rs b/lib/apps/fabro-server/src/run_manifest.rs index 375ef57e5..aff59b0c5 100644 --- a/lib/apps/fabro-server/src/run_manifest.rs +++ b/lib/apps/fabro-server/src/run_manifest.rs @@ -34,14 +34,17 @@ use fabro_types::{ use fabro_util::check_report::{CheckDetail, CheckReport, CheckResult, CheckSection, CheckStatus}; use fabro_validate::Severity; use fabro_workflow::Error as WorkflowError; -use fabro_workflow::operations::{CreateRunInput, ValidateInput, WorkflowInput, validate}; +use fabro_workflow::operations::{ + CreateRunInput, ValidateInput, WorkflowInput, validate, validate_with_provider_fallback, +}; use fabro_workflow::pipeline::Validated; +#[cfg(test)] use fabro_workflow::run_materialization::materialize_run; +use fabro_workflow::run_materialization::materialize_run_with_provider_fallback; use fabro_workflow::workflow_bundle::{BundledWorkflow, ParsedWorkflowConfig, WorkflowBundle}; use futures_util::stream::{self, StreamExt}; use tokio::process::Command; use tokio::time; -use tracing::warn; use crate::interp::process_env_var; use crate::server::AppState; @@ -206,6 +209,27 @@ pub(crate) fn validate_prepared_manifest_with_vars( }) } +pub(crate) fn validate_prepared_manifest_for_preflight( + prepared: &PreparedManifest, + catalog: Arc, + vars: HashMap, + ready_providers: &[ProviderId], +) -> Result { + let fallback_providers = catalog.all_provider_ids().into_iter().collect::>(); + validate_with_provider_fallback( + ValidateInput { + workflow: WorkflowInput::Bundled(prepared.workflow_input.clone()), + settings: prepared.settings.clone(), + vars, + cwd: prepared.cwd.clone(), + custom_transforms: Vec::new(), + catalog, + }, + ready_providers, + &fallback_providers, + ) +} + pub(crate) fn create_run_input( prepared: PreparedManifest, configured_providers: Vec, @@ -238,8 +262,11 @@ pub(crate) async fn run_preflight( state: &AppState, prepared: &PreparedManifest, validated: &Validated, + preferred_providers: &[ProviderId], + llm_result: Result, ) -> Result<(types::PreflightResponse, bool)> { - let (report, checks_ok) = build_preflight_report(state, prepared, validated).await?; + let (report, checks_ok) = + build_preflight_report(state, prepared, validated, preferred_providers, llm_result).await?; let preflight_ok = !validated.has_errors() && checks_ok; Ok(( preflight_response( @@ -458,6 +485,8 @@ async fn build_preflight_report( state: &AppState, prepared: &PreparedManifest, validated: &Validated, + preferred_providers: &[ProviderId], + llm_result: Result, ) -> Result<(CheckReport, bool)> { let graph = validated.graph(); let mut checks = base_preflight_checks(prepared, graph); @@ -475,20 +504,13 @@ async fn build_preflight_report( } let catalog = state.catalog(); - let llm_result = state.resolve_llm_client().await; - if let Err(err) = &llm_result { - warn!(error = ?err, "Failed to resolve LLM client while checking ready providers"); - } - // Preflight is credential-independent static validation. Materialize - // against every enabled catalog provider so aliases and defaults can be - // inspected even when the corresponding adapter is not currently ready; - // `run_llm_check` below reports actual credential/registration readiness. - let enabled_providers = catalog.all_provider_ids().into_iter().collect::>(); - let materialized = materialize_run( + let fallback_providers = catalog.all_provider_ids().into_iter().collect::>(); + let materialized = materialize_run_with_provider_fallback( prepared.settings.clone(), graph, catalog.as_ref(), - &enabled_providers, + preferred_providers, + &fallback_providers, )?; let resolved_run = materialized.run; let server_settings = state.server_settings(); @@ -1033,13 +1055,21 @@ async fn run_llm_check( catalog: &Catalog, llm_result: Result, ) -> bool { - let model = settings - .model - .name - .as_deref() - .unwrap_or_else(|| catalog.default_for_configured_ids(&[]).id.as_str()); - let provider = settings.model.provider.as_deref(); - let default_provider = provider.unwrap_or("anthropic"); + let (Some(model), Some(default_provider)) = ( + settings.model.name.as_deref(), + settings.model.provider.as_deref(), + ) else { + checks.push(CheckResult { + name: "LLM".into(), + status: CheckStatus::Error, + summary: "model resolution failed".into(), + details: Vec::new(), + remediation: Some( + "Preflight did not produce a resolved run model and provider".to_string(), + ), + }); + return false; + }; let mut model_providers = std::collections::BTreeSet::new(); let mut has_llm_nodes = false; @@ -1050,24 +1080,7 @@ async fn run_llm_check( has_llm_nodes = true; let node_model = node.model().unwrap_or(model); let node_provider = node.provider().unwrap_or(default_provider); - let resolved = if node.provider().is_some() { - catalog.get_on_provider(&ProviderId::new(node_provider), node_model) - } else { - catalog - .select(node_model, None, &catalog.all_provider_ids()) - .ok() - }; - let (resolved_model, resolved_provider) = if let Some(info) = resolved { - (info.id.to_string(), info.provider.to_string()) - } else { - (node_model.to_string(), node_provider.to_string()) - }; - let final_provider = if node.provider().is_some() { - node_provider.to_string() - } else { - resolved_provider - }; - model_providers.insert((resolved_model, final_provider)); + model_providers.insert((node_model.to_string(), node_provider.to_string())); } if !has_llm_nodes { @@ -1515,7 +1528,11 @@ enabled = true state: &Arc, model: &str, ) -> (types::PreflightResponse, bool) { - let mut ready_providers = state.ready_llm_provider_ids().await; + let llm_result = state.resolve_llm_client().await; + let mut ready_providers = llm_result + .as_ref() + .map(LlmClientResult::provider_ids) + .unwrap_or_default(); ready_providers.sort(); assert_eq!(ready_providers, vec![ ProviderId::new("kimi"), @@ -1538,11 +1555,37 @@ digraph Demo {{ &manifest, ) .unwrap(); - let validated = validate_prepared_manifest(&prepared, state.catalog()).unwrap(); + let validated = validate_prepared_manifest_for_preflight( + &prepared, + state.catalog(), + HashMap::new(), + &ready_providers, + ) + .unwrap(); - run_preflight(state.as_ref(), &prepared, &validated) - .await - .unwrap() + run_preflight( + state.as_ref(), + &prepared, + &validated, + &ready_providers, + llm_result, + ) + .await + .unwrap() + } + + async fn run_preflight_with_catalog_routes( + state: &AppState, + prepared: &PreparedManifest, + validated: &Validated, + ) -> Result<(types::PreflightResponse, bool)> { + let llm_result = state.resolve_llm_client().await; + let preferred_providers = state + .catalog() + .all_provider_ids() + .into_iter() + .collect::>(); + run_preflight(state, prepared, validated, &preferred_providers, llm_result).await } fn manifest_workflow() -> types::ManifestWorkflow { @@ -2174,9 +2217,10 @@ name = "Control Plane" assert!(validated.has_errors()); - let (response, ok) = run_preflight(state.as_ref(), &prepared, &validated) - .await - .unwrap(); + let (response, ok) = + run_preflight_with_catalog_routes(state.as_ref(), &prepared, &validated) + .await + .unwrap(); assert!(!ok); assert_eq!(response.workflow.name, "Invalid"); @@ -2218,9 +2262,10 @@ issues = "read" let validated = validate_prepared_manifest(&prepared, test_catalog()).unwrap(); assert!(!validated.has_errors()); - let (response, _ok) = run_preflight(state.as_ref(), &prepared, &validated) - .await - .unwrap(); + let (response, _ok) = + run_preflight_with_catalog_routes(state.as_ref(), &prepared, &validated) + .await + .unwrap(); assert!( response.checks.sections[0] @@ -2269,9 +2314,10 @@ id = "local" assert!(!validated.has_errors()); - let (response, ok) = run_preflight(state.as_ref(), &prepared, &validated) - .await - .unwrap(); + let (response, ok) = + run_preflight_with_catalog_routes(state.as_ref(), &prepared, &validated) + .await + .unwrap(); assert!(ok); assert!(response.workflow.diagnostics.is_empty()); @@ -2376,9 +2422,10 @@ id = "daytona" .unwrap(); let validated = validate_prepared_manifest(&prepared, test_catalog()).unwrap(); - let (response, _ok) = run_preflight(state.as_ref(), &prepared, &validated) - .await - .unwrap(); + let (response, _ok) = + run_preflight_with_catalog_routes(state.as_ref(), &prepared, &validated) + .await + .unwrap(); assert!(response.workflow.diagnostics.is_empty()); assert!( @@ -2444,9 +2491,10 @@ digraph Demo { .unwrap(); let validated = validate_prepared_manifest(&prepared, test_catalog()).unwrap(); - let (response, ok) = run_preflight(state.as_ref(), &prepared, &validated) - .await - .unwrap(); + let (response, ok) = + run_preflight_with_catalog_routes(state.as_ref(), &prepared, &validated) + .await + .unwrap(); assert!(!ok); let llm_check = response.checks.sections[0] @@ -2615,11 +2663,29 @@ digraph Demo { &manifest, ) .unwrap(); - let validated = validate_prepared_manifest(&prepared, state.catalog()).unwrap(); + let llm_result = state.resolve_llm_client().await; + let ready_providers = llm_result + .as_ref() + .map(LlmClientResult::provider_ids) + .unwrap_or_default(); + assert!(ready_providers.is_empty()); + let validated = validate_prepared_manifest_for_preflight( + &prepared, + state.catalog(), + HashMap::new(), + &ready_providers, + ) + .unwrap(); - let (response, ok) = run_preflight(state.as_ref(), &prepared, &validated) - .await - .unwrap(); + let (response, ok) = run_preflight( + state.as_ref(), + &prepared, + &validated, + &ready_providers, + llm_result, + ) + .await + .unwrap(); assert!(!ok); let llm_check = response.checks.sections[0] diff --git a/lib/apps/fabro-server/src/server/handler/runs.rs b/lib/apps/fabro-server/src/server/handler/runs.rs index 614f2d719..20a4b3f9d 100644 --- a/lib/apps/fabro-server/src/server/handler/runs.rs +++ b/lib/apps/fabro-server/src/server/handler/runs.rs @@ -835,10 +835,22 @@ async fn run_preflight( return ApiError::bad_request(format!("Run config variable interpolation failed: {err}")) .into_response(); } - let mut validated = match run_manifest::validate_prepared_manifest_with_vars( + let llm_result = state.resolve_llm_client().await; + if let Err(error) = &llm_result { + tracing::warn!( + error = ?error, + "Failed to resolve LLM client while checking ready providers" + ); + } + let ready_providers = llm_result + .as_ref() + .map(LlmClientResult::provider_ids) + .unwrap_or_default(); + let mut validated = match run_manifest::validate_prepared_manifest_for_preflight( &prepared, state.catalog(), vars, + &ready_providers, ) { Ok(validated) => validated, Err(WorkflowError::Parse(_)) => { @@ -847,7 +859,15 @@ async fn run_preflight( Err(err) => return ApiError::bad_request(err.to_string()).into_response(), }; validated.promote_template_undefined_variables_to_errors(); - let response = match run_manifest::run_preflight(&state, &prepared, &validated).await { + let response = match run_manifest::run_preflight( + &state, + &prepared, + &validated, + &ready_providers, + llm_result, + ) + .await + { Ok((response, _ok)) => response, Err(err) => { return ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()) diff --git a/lib/components/fabro-workflow/src/operations/create.rs b/lib/components/fabro-workflow/src/operations/create.rs index 144cf5976..39bd097bf 100644 --- a/lib/components/fabro-workflow/src/operations/create.rs +++ b/lib/components/fabro-workflow/src/operations/create.rs @@ -312,6 +312,7 @@ fn create_from_source( .filter(|provider| !provider.is_empty()) .map(ProviderId::new), &options.configured_providers, + None, &options.catalog, )?; @@ -336,6 +337,7 @@ pub(super) fn preprocess_and_validate( render_mode: RenderMode, default_provider: Option, eligible_providers: &[ProviderId], + fallback_providers: Option<&[ProviderId]>, catalog: &Arc, ) -> Result { let mut parsed = pipeline::parse(dot_source)?; @@ -351,6 +353,7 @@ pub(super) fn preprocess_and_validate( catalog: Arc::clone(catalog), default_provider, eligible_providers: eligible_providers.iter().cloned().collect(), + fallback_providers: fallback_providers.map(|providers| providers.iter().cloned().collect()), })?; Ok(pipeline::validate(transformed, catalog.as_ref(), &[])) } @@ -579,6 +582,7 @@ reasoning = false RenderMode::Structural, None, &test_provider_ids(), + None, &test_catalog(), ) .unwrap() @@ -749,6 +753,7 @@ reasoning = false RenderMode::Strict, None, &test_provider_ids(), + None, &test_catalog(), ); let Err(err) = result else { @@ -787,6 +792,7 @@ reasoning = false RenderMode::Strict, None, &test_provider_ids(), + None, &test_catalog(), ); let Err(err) = result else { diff --git a/lib/components/fabro-workflow/src/operations/mod.rs b/lib/components/fabro-workflow/src/operations/mod.rs index 4710d97a9..9e5726515 100644 --- a/lib/components/fabro-workflow/src/operations/mod.rs +++ b/lib/components/fabro-workflow/src/operations/mod.rs @@ -22,7 +22,7 @@ pub use rewind::{RewindInput, RewindOutcome, rewind}; pub use source::WorkflowInput; pub use start::{StartServices, Started, start}; pub use timeline::{ForkTarget, RunTimeline, TimelineEntry, build_timeline, timeline}; -pub use validate::{ValidateInput, validate}; +pub use validate::{ValidateInput, validate, validate_with_provider_fallback}; pub use crate::pipeline::{LlmSpec, SandboxEnvSpec}; pub use crate::transforms::RenderMode; diff --git a/lib/components/fabro-workflow/src/operations/validate.rs b/lib/components/fabro-workflow/src/operations/validate.rs index a95b13920..2fb7235b0 100644 --- a/lib/components/fabro-workflow/src/operations/validate.rs +++ b/lib/components/fabro-workflow/src/operations/validate.rs @@ -2,7 +2,7 @@ use std::collections::HashMap; use std::path::PathBuf; use std::sync::Arc; -use fabro_model::Catalog; +use fabro_model::{Catalog, ProviderId}; use fabro_types::WorkflowSettings; use super::create::{preprocess_and_validate, template_context}; @@ -28,17 +28,35 @@ pub struct ValidateInput { /// Returns `Validated` even when validation produced errors. Call /// `validated.raise_on_errors()` if the caller wants to fail fast. pub fn validate(input: ValidateInput) -> Result { + let eligible_providers = input + .catalog + .all_provider_ids() + .into_iter() + .collect::>(); + validate_with_provider_sets(input, &eligible_providers, None) +} + +/// Parse, transform, and validate while preferring one provider snapshot and +/// falling back to another only for provider-readiness selection failures. +pub fn validate_with_provider_fallback( + input: ValidateInput, + preferred_providers: &[ProviderId], + fallback_providers: &[ProviderId], +) -> Result { + validate_with_provider_sets(input, preferred_providers, Some(fallback_providers)) +} + +fn validate_with_provider_sets( + input: ValidateInput, + eligible_providers: &[ProviderId], + fallback_providers: Option<&[ProviderId]>, +) -> Result { let resolved = resolve_workflow(ResolveWorkflowInput { workflow: input.workflow, settings: input.settings, cwd: input.cwd, }) .map_err(|err| Error::Parse(err.to_string()))?; - let eligible_providers = input - .catalog - .all_provider_ids() - .into_iter() - .collect::>(); preprocess_and_validate( &resolved.raw_source, @@ -60,7 +78,8 @@ pub fn validate(input: ValidateInput) -> Result { .as_deref() .filter(|provider| !provider.is_empty()) .map(fabro_model::ProviderId::new), - &eligible_providers, + eligible_providers, + fallback_providers, &input.catalog, ) } diff --git a/lib/components/fabro-workflow/src/pipeline/transform.rs b/lib/components/fabro-workflow/src/pipeline/transform.rs index 6b76380d0..cc3fe089d 100644 --- a/lib/components/fabro-workflow/src/pipeline/transform.rs +++ b/lib/components/fabro-workflow/src/pipeline/transform.rs @@ -68,6 +68,7 @@ pub fn transform(parsed: Parsed, options: &TransformOptions) -> Result, pub default_provider: Option, pub eligible_providers: HashSet, + pub fallback_providers: Option>, } /// Options for the FINALIZE phase. diff --git a/lib/components/fabro-workflow/src/pipeline/validate.rs b/lib/components/fabro-workflow/src/pipeline/validate.rs index 06120e8ce..8e19bb5a6 100644 --- a/lib/components/fabro-workflow/src/pipeline/validate.rs +++ b/lib/components/fabro-workflow/src/pipeline/validate.rs @@ -51,6 +51,7 @@ mod tests { catalog: std::sync::Arc::clone(&catalog), default_provider: None, eligible_providers: catalog.all_provider_ids(), + fallback_providers: None, }) .unwrap(); validate(transformed, catalog.as_ref(), &[]) diff --git a/lib/components/fabro-workflow/src/run_materialization.rs b/lib/components/fabro-workflow/src/run_materialization.rs index 7e82964ac..27183bcf5 100644 --- a/lib/components/fabro-workflow/src/run_materialization.rs +++ b/lib/components/fabro-workflow/src/run_materialization.rs @@ -9,10 +9,36 @@ use fabro_types::settings::run::RunGoal; use crate::error::Error; pub fn materialize_run( + settings: WorkflowSettings, + graph: &Graph, + catalog: &Catalog, + configured_providers: &[ProviderId], +) -> Result { + materialize_run_with_provider_sets(settings, graph, catalog, configured_providers, None) +} + +pub fn materialize_run_with_provider_fallback( + settings: WorkflowSettings, + graph: &Graph, + catalog: &Catalog, + preferred_providers: &[ProviderId], + fallback_providers: &[ProviderId], +) -> Result { + materialize_run_with_provider_sets( + settings, + graph, + catalog, + preferred_providers, + Some(fallback_providers), + ) +} + +fn materialize_run_with_provider_sets( mut settings: WorkflowSettings, graph: &Graph, catalog: &Catalog, configured_providers: &[ProviderId], + fallback_providers: Option<&[ProviderId]>, ) -> Result { let configured_model = settings.run.model.name.take(); let configured_provider = settings.run.model.provider.take(); @@ -30,11 +56,24 @@ pub fn materialize_run( let provider = configured_provider.or(graph_provider); let model = configured_model.or(graph_model); let eligible = configured_providers.iter().cloned().collect::>(); - let (resolved_model, resolved_provider) = - resolve_run_model(catalog, &eligible, model.as_deref(), provider.as_deref())?; + let fallback = + fallback_providers.map(|providers| providers.iter().cloned().collect::>()); + let provider = provider + .as_deref() + .filter(|provider| !provider.is_empty()) + .map(ProviderId::new); + let selected = match fallback { + Some(fallback) => catalog.resolve_selection_with_fallback( + model.as_deref(), + provider.as_ref(), + &eligible, + &fallback, + ), + None => catalog.resolve_selection(model.as_deref(), provider.as_ref(), &eligible), + }?; - settings.run.model.name = Some(resolved_model); - settings.run.model.provider = Some(resolved_provider.into_inner()); + settings.run.model.name = Some(selected.model); + settings.run.model.provider = Some(selected.provider.into_inner()); let goal = graph.goal().to_string(); settings.run.goal = if goal.is_empty() { diff --git a/lib/components/fabro-workflow/src/transforms/model_resolution.rs b/lib/components/fabro-workflow/src/transforms/model_resolution.rs index 7eecbcfd1..00e71ba74 100644 --- a/lib/components/fabro-workflow/src/transforms/model_resolution.rs +++ b/lib/components/fabro-workflow/src/transforms/model_resolution.rs @@ -13,6 +13,7 @@ pub struct ModelResolutionTransform { catalog: Arc, default_provider: Option, eligible_providers: HashSet, + fallback_providers: Option>, } impl ModelResolutionTransform { @@ -23,6 +24,7 @@ impl ModelResolutionTransform { catalog, default_provider: None, eligible_providers, + fallback_providers: None, } } @@ -32,6 +34,7 @@ impl ModelResolutionTransform { catalog, default_provider: None, eligible_providers, + fallback_providers: None, } } @@ -41,16 +44,33 @@ impl ModelResolutionTransform { self } + #[must_use] + pub fn with_fallback_providers( + mut self, + fallback_providers: Option>, + ) -> Self { + self.fallback_providers = fallback_providers; + self + } + fn resolve_model( &self, model: &str, explicit_provider: Option<&ProviderId>, ) -> Result<(String, ProviderId), Error> { - let selected = self.catalog.resolve_selection( - Some(model), - explicit_provider, - &self.eligible_providers, - )?; + let selected = match &self.fallback_providers { + Some(fallback_providers) => self.catalog.resolve_selection_with_fallback( + Some(model), + explicit_provider, + &self.eligible_providers, + fallback_providers, + ), + None => self.catalog.resolve_selection( + Some(model), + explicit_provider, + &self.eligible_providers, + ), + }?; Ok((selected.model, selected.provider)) } } @@ -326,6 +346,50 @@ reasoning = false ); } + #[test] + fn fallback_resolution_keeps_ready_preference_for_unpinned_nodes() { + let overrides: LlmCatalogSettings = toml::from_str( + r" +[providers.openrouter] +enabled = true +", + ) + .unwrap(); + let catalog = Arc::new(Catalog::from_builtin_with_overrides(&overrides).unwrap()); + let mut graph = Graph::new("test"); + let mut portable = Node::new("portable"); + portable.attrs.insert( + "model".to_string(), + AttrValue::String("claude-fable".to_string()), + ); + graph.nodes.insert("portable".to_string(), portable); + let mut pinned = Node::new("pinned"); + pinned.attrs.insert( + "model".to_string(), + AttrValue::String("claude-fable".to_string()), + ); + pinned.attrs.insert( + "provider".to_string(), + AttrValue::String("anthropic".to_string()), + ); + graph.nodes.insert("pinned".to_string(), pinned); + + let graph = ModelResolutionTransform::for_eligible( + Arc::clone(&catalog), + HashSet::from([ProviderId::new("openrouter")]), + ) + .with_fallback_providers(Some(catalog.all_provider_ids())) + .apply(graph) + .unwrap(); + + assert_eq!( + graph.nodes["portable"].provider(), + Some("openrouter"), + "the unrelated unavailable pin must not force catalog-wide routing" + ); + assert_eq!(graph.nodes["pinned"].provider(), Some("anthropic")); + } + #[test] fn graph_default_alias_materializes_to_canonical_offering() { let mut graph = Graph::new("test"); diff --git a/lib/components/fabro-workflow/tests/it/integration.rs b/lib/components/fabro-workflow/tests/it/integration.rs index 5f3f75a55..44307ca81 100644 --- a/lib/components/fabro-workflow/tests/it/integration.rs +++ b/lib/components/fabro-workflow/tests/it/integration.rs @@ -4908,6 +4908,7 @@ async fn import_e2e_through_engine() { catalog: std::sync::Arc::clone(&catalog), default_provider: None, eligible_providers: catalog.all_provider_ids(), + fallback_providers: None, }) .unwrap(); let validated = validate(transformed, catalog.as_ref(), &[]); diff --git a/lib/foundation/fabro-model/src/catalog.rs b/lib/foundation/fabro-model/src/catalog.rs index cac9f878f..da9ab96fd 100644 --- a/lib/foundation/fabro-model/src/catalog.rs +++ b/lib/foundation/fabro-model/src/catalog.rs @@ -1101,6 +1101,32 @@ impl Catalog { } } + /// Resolve a selection against a preferred provider snapshot, falling back + /// to a broader eligible set only when the preferred set cannot supply the + /// requested provider or model. + /// + /// This is useful for readiness checks: ready providers remain preferred, + /// while a catalog-only offering can still be selected so the caller can + /// report why its provider is unavailable. Semantic failures such as an + /// unknown provider do not fall back. + pub fn resolve_selection_with_fallback( + &self, + selector: Option<&str>, + explicit_provider: Option<&ProviderId>, + preferred_providers: &HashSet, + fallback_providers: &HashSet, + ) -> Result { + match self.resolve_selection(selector, explicit_provider, preferred_providers) { + Ok(selected) => Ok(selected), + Err( + ModelSelectionError::ProviderUnavailable { .. } + | ModelSelectionError::NoEligibleOffering { .. } + | ModelSelectionError::NoDefaultModel { .. }, + ) => self.resolve_selection(selector, explicit_provider, fallback_providers), + Err(error) => Err(error), + } + } + #[must_use] pub fn is_model_selector(&self, selector: &str) -> bool { self.candidate_indices(selector).is_some() @@ -3989,6 +4015,41 @@ adapter = "openai_compatible" )); } + #[test] + fn selection_fallback_preserves_ready_preference_per_request() { + let catalog = portable_model_catalog(); + let openai = ProviderId::openai(); + let openrouter = ProviderId::new("openrouter"); + let ready = HashSet::from([openrouter.clone()]); + let catalog_providers = HashSet::from([openai.clone(), openrouter.clone()]); + + let shared = catalog + .resolve_selection_with_fallback(Some("portable"), None, &ready, &catalog_providers) + .unwrap(); + assert_eq!(shared.provider, openrouter); + + let pinned = catalog + .resolve_selection_with_fallback( + Some("portable"), + Some(&openai), + &ready, + &catalog_providers, + ) + .unwrap(); + assert_eq!(pinned.provider, openai); + + let unknown = catalog + .resolve_selection_with_fallback( + Some("provider-private-preview"), + None, + &ready, + &catalog_providers, + ) + .unwrap(); + assert_eq!(unknown.provider, ProviderId::new("openrouter")); + assert_eq!(unknown.model, "provider-private-preview"); + } + #[test] fn legacy_builtin_selector_uses_readiness_priority_and_explicit_pins() { let catalog = portable_model_catalog(); From c06c60214aa478d89bf53d4472c10ce27ea5596d Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 24 Jul 2026 08:49:01 -0400 Subject: [PATCH 3/3] refactor: simplify readiness-fallback plumbing The fallback provider set was always catalog.all_provider_ids(), computed at every call site and threaded through five layers alongside the catalog itself. Fold it into Catalog::resolve_selection_with_catalog_fallback and carry only a catalog_fallback flag through the transform/validate/ materialize entry points. - materialize_run delegates to resolve_run_model again instead of re-inlining its provider normalization and selection - run_preflight derives ready providers from llm_result instead of taking both, so callers cannot pass inconsistent pairs; the legacy tests now exercise the production ready-first routing path - AppState::resolve_llm_client_with_ready_ids replaces three copies of resolve-then-extract-provider-ids, and ready_llm_provider_ids delegates to it - the unreachable "model resolution failed" preflight check becomes an invariant error where the materialized run is produced - validate_prepared_manifest_with_vars/_for_preflight share the ValidateInput construction Co-Authored-By: Claude Fable 5 --- lib/apps/fabro-server/src/run_manifest.rs | 153 +++++++----------- lib/apps/fabro-server/src/server.rs | 26 ++- .../fabro-server/src/server/handler/runs.rs | 49 ++---- .../fabro-workflow/src/operations/create.rs | 12 +- .../fabro-workflow/src/operations/mod.rs | 2 +- .../fabro-workflow/src/operations/start.rs | 1 + .../fabro-workflow/src/operations/validate.rs | 20 +-- .../fabro-workflow/src/pipeline/transform.rs | 10 +- .../fabro-workflow/src/pipeline/types.rs | 4 +- .../fabro-workflow/src/pipeline/validate.rs | 2 +- .../fabro-workflow/src/run_materialization.rs | 59 +++---- .../src/transforms/model_resolution.rs | 32 ++-- .../fabro-workflow/tests/it/integration.rs | 2 +- lib/foundation/fabro-model/src/catalog.rs | 26 +-- 14 files changed, 166 insertions(+), 232 deletions(-) diff --git a/lib/apps/fabro-server/src/run_manifest.rs b/lib/apps/fabro-server/src/run_manifest.rs index aff59b0c5..65cc6d36e 100644 --- a/lib/apps/fabro-server/src/run_manifest.rs +++ b/lib/apps/fabro-server/src/run_manifest.rs @@ -35,12 +35,10 @@ use fabro_util::check_report::{CheckDetail, CheckReport, CheckResult, CheckSecti use fabro_validate::Severity; use fabro_workflow::Error as WorkflowError; use fabro_workflow::operations::{ - CreateRunInput, ValidateInput, WorkflowInput, validate, validate_with_provider_fallback, + CreateRunInput, ValidateInput, WorkflowInput, validate, validate_with_ready_providers, }; use fabro_workflow::pipeline::Validated; -#[cfg(test)] -use fabro_workflow::run_materialization::materialize_run; -use fabro_workflow::run_materialization::materialize_run_with_provider_fallback; +use fabro_workflow::run_materialization::materialize_run_with_ready_providers; use fabro_workflow::workflow_bundle::{BundledWorkflow, ParsedWorkflowConfig, WorkflowBundle}; use futures_util::stream::{self, StreamExt}; use tokio::process::Command; @@ -199,14 +197,7 @@ pub(crate) fn validate_prepared_manifest_with_vars( catalog: Arc, vars: HashMap, ) -> Result { - validate(ValidateInput { - workflow: WorkflowInput::Bundled(prepared.workflow_input.clone()), - settings: prepared.settings.clone(), - vars, - cwd: prepared.cwd.clone(), - custom_transforms: Vec::new(), - catalog, - }) + validate(manifest_validate_input(prepared, catalog, vars)) } pub(crate) fn validate_prepared_manifest_for_preflight( @@ -215,21 +206,27 @@ pub(crate) fn validate_prepared_manifest_for_preflight( vars: HashMap, ready_providers: &[ProviderId], ) -> Result { - let fallback_providers = catalog.all_provider_ids().into_iter().collect::>(); - validate_with_provider_fallback( - ValidateInput { - workflow: WorkflowInput::Bundled(prepared.workflow_input.clone()), - settings: prepared.settings.clone(), - vars, - cwd: prepared.cwd.clone(), - custom_transforms: Vec::new(), - catalog, - }, + validate_with_ready_providers( + manifest_validate_input(prepared, catalog, vars), ready_providers, - &fallback_providers, ) } +fn manifest_validate_input( + prepared: &PreparedManifest, + catalog: Arc, + vars: HashMap, +) -> ValidateInput { + ValidateInput { + workflow: WorkflowInput::Bundled(prepared.workflow_input.clone()), + settings: prepared.settings.clone(), + vars, + cwd: prepared.cwd.clone(), + custom_transforms: Vec::new(), + catalog, + } +} + pub(crate) fn create_run_input( prepared: PreparedManifest, configured_providers: Vec, @@ -262,11 +259,10 @@ pub(crate) async fn run_preflight( state: &AppState, prepared: &PreparedManifest, validated: &Validated, - preferred_providers: &[ProviderId], llm_result: Result, ) -> Result<(types::PreflightResponse, bool)> { let (report, checks_ok) = - build_preflight_report(state, prepared, validated, preferred_providers, llm_result).await?; + build_preflight_report(state, prepared, validated, llm_result).await?; let preflight_ok = !validated.has_errors() && checks_ok; Ok(( preflight_response( @@ -485,7 +481,6 @@ async fn build_preflight_report( state: &AppState, prepared: &PreparedManifest, validated: &Validated, - preferred_providers: &[ProviderId], llm_result: Result, ) -> Result<(CheckReport, bool)> { let graph = validated.graph(); @@ -504,15 +499,23 @@ async fn build_preflight_report( } let catalog = state.catalog(); - let fallback_providers = catalog.all_provider_ids().into_iter().collect::>(); - let materialized = materialize_run_with_provider_fallback( + let ready_providers = llm_result + .as_ref() + .map(LlmClientResult::provider_ids) + .unwrap_or_default(); + let materialized = materialize_run_with_ready_providers( prepared.settings.clone(), graph, catalog.as_ref(), - preferred_providers, - &fallback_providers, + &ready_providers, )?; let resolved_run = materialized.run; + let (Some(run_model), Some(run_provider)) = ( + resolved_run.model.name.as_deref(), + resolved_run.model.provider.as_deref(), + ) else { + bail!("materialized run is missing a resolved model or provider"); + }; let server_settings = state.server_settings(); let github_integration = &server_settings.server.integrations.github; let sandbox_provider = effective_sandbox_provider(&resolved_run); @@ -575,7 +578,8 @@ async fn build_preflight_report( let llm_ok = run_llm_check( &mut checks, graph, - &resolved_run, + run_model, + run_provider, catalog.as_ref(), llm_result, ) @@ -1051,25 +1055,11 @@ struct PendingModelProbe { async fn run_llm_check( checks: &mut Vec, graph: &Graph, - settings: &RunNamespace, + model: &str, + default_provider: &str, catalog: &Catalog, llm_result: Result, ) -> bool { - let (Some(model), Some(default_provider)) = ( - settings.model.name.as_deref(), - settings.model.provider.as_deref(), - ) else { - checks.push(CheckResult { - name: "LLM".into(), - status: CheckStatus::Error, - summary: "model resolution failed".into(), - details: Vec::new(), - remediation: Some( - "Preflight did not produce a resolved run model and provider".to_string(), - ), - }); - return false; - }; let mut model_providers = std::collections::BTreeSet::new(); let mut has_llm_nodes = false; @@ -1388,6 +1378,7 @@ fn report_to_api(report: &CheckReport) -> types::PreflightCheckReport { mod tests { use fabro_model::ProviderId; use fabro_model::catalog::LlmCatalogSettings; + use fabro_workflow::run_materialization::materialize_run; use super::*; @@ -1563,29 +1554,18 @@ digraph Demo {{ ) .unwrap(); - run_preflight( - state.as_ref(), - &prepared, - &validated, - &ready_providers, - llm_result, - ) - .await - .unwrap() + run_preflight(state.as_ref(), &prepared, &validated, llm_result) + .await + .unwrap() } - async fn run_preflight_with_catalog_routes( + async fn resolve_and_run_preflight( state: &AppState, prepared: &PreparedManifest, validated: &Validated, ) -> Result<(types::PreflightResponse, bool)> { let llm_result = state.resolve_llm_client().await; - let preferred_providers = state - .catalog() - .all_provider_ids() - .into_iter() - .collect::>(); - run_preflight(state, prepared, validated, &preferred_providers, llm_result).await + run_preflight(state, prepared, validated, llm_result).await } fn manifest_workflow() -> types::ManifestWorkflow { @@ -2217,10 +2197,9 @@ name = "Control Plane" assert!(validated.has_errors()); - let (response, ok) = - run_preflight_with_catalog_routes(state.as_ref(), &prepared, &validated) - .await - .unwrap(); + let (response, ok) = resolve_and_run_preflight(state.as_ref(), &prepared, &validated) + .await + .unwrap(); assert!(!ok); assert_eq!(response.workflow.name, "Invalid"); @@ -2262,10 +2241,9 @@ issues = "read" let validated = validate_prepared_manifest(&prepared, test_catalog()).unwrap(); assert!(!validated.has_errors()); - let (response, _ok) = - run_preflight_with_catalog_routes(state.as_ref(), &prepared, &validated) - .await - .unwrap(); + let (response, _ok) = resolve_and_run_preflight(state.as_ref(), &prepared, &validated) + .await + .unwrap(); assert!( response.checks.sections[0] @@ -2314,10 +2292,9 @@ id = "local" assert!(!validated.has_errors()); - let (response, ok) = - run_preflight_with_catalog_routes(state.as_ref(), &prepared, &validated) - .await - .unwrap(); + let (response, ok) = resolve_and_run_preflight(state.as_ref(), &prepared, &validated) + .await + .unwrap(); assert!(ok); assert!(response.workflow.diagnostics.is_empty()); @@ -2422,10 +2399,9 @@ id = "daytona" .unwrap(); let validated = validate_prepared_manifest(&prepared, test_catalog()).unwrap(); - let (response, _ok) = - run_preflight_with_catalog_routes(state.as_ref(), &prepared, &validated) - .await - .unwrap(); + let (response, _ok) = resolve_and_run_preflight(state.as_ref(), &prepared, &validated) + .await + .unwrap(); assert!(response.workflow.diagnostics.is_empty()); assert!( @@ -2491,10 +2467,9 @@ digraph Demo { .unwrap(); let validated = validate_prepared_manifest(&prepared, test_catalog()).unwrap(); - let (response, ok) = - run_preflight_with_catalog_routes(state.as_ref(), &prepared, &validated) - .await - .unwrap(); + let (response, ok) = resolve_and_run_preflight(state.as_ref(), &prepared, &validated) + .await + .unwrap(); assert!(!ok); let llm_check = response.checks.sections[0] @@ -2677,15 +2652,9 @@ digraph Demo { ) .unwrap(); - let (response, ok) = run_preflight( - state.as_ref(), - &prepared, - &validated, - &ready_providers, - llm_result, - ) - .await - .unwrap(); + let (response, ok) = run_preflight(state.as_ref(), &prepared, &validated, llm_result) + .await + .unwrap(); assert!(!ok); let llm_check = response.checks.sections[0] diff --git a/lib/apps/fabro-server/src/server.rs b/lib/apps/fabro-server/src/server.rs index fac2dd51a..47b57a462 100644 --- a/lib/apps/fabro-server/src/server.rs +++ b/lib/apps/fabro-server/src/server.rs @@ -1445,14 +1445,26 @@ impl AppState { self.llm_source.configured_providers(catalog.as_ref()).await } - pub(crate) async fn ready_llm_provider_ids(&self) -> Vec { - match self.resolve_llm_client().await { - Ok(result) => result.provider_ids(), - Err(err) => { - warn!(error = ?err, "Failed to resolve LLM client while checking ready providers"); - Vec::new() - } + /// Resolve the LLM client once and derive the ready provider IDs from it, + /// logging a warning when resolution fails. Callers that need both values + /// must use this instead of `ready_llm_provider_ids` so the client is not + /// resolved twice. + pub(crate) async fn resolve_llm_client_with_ready_ids( + &self, + ) -> (anyhow::Result, Vec) { + let llm_result = self.resolve_llm_client().await; + if let Err(err) = &llm_result { + warn!(error = ?err, "Failed to resolve LLM client while checking ready providers"); } + let ready_provider_ids = llm_result + .as_ref() + .map(LlmClientResult::provider_ids) + .unwrap_or_default(); + (llm_result, ready_provider_ids) + } + + pub(crate) async fn ready_llm_provider_ids(&self) -> Vec { + self.resolve_llm_client_with_ready_ids().await.1 } pub(crate) async fn decorate_run_summary(&self, run: fabro_types::Run) -> fabro_types::Run { diff --git a/lib/apps/fabro-server/src/server/handler/runs.rs b/lib/apps/fabro-server/src/server/handler/runs.rs index 20a4b3f9d..fdb9c1c3e 100644 --- a/lib/apps/fabro-server/src/server/handler/runs.rs +++ b/lib/apps/fabro-server/src/server/handler/runs.rs @@ -51,7 +51,6 @@ use crate::run_files::{list_run_commits, list_run_files}; use crate::run_manifest; use crate::run_selector::{ResolveRunError, resolve_run_by_selector}; use crate::run_title_generation::{self, GenerateTitleInput, TitlePromptInput, WorkflowSummary}; -use crate::server_secrets::LlmClientResult; #[cfg(any(test, feature = "test-support"))] use crate::test_support as server_test_support; @@ -591,17 +590,8 @@ pub(crate) async fn create_run_from_manifest( // and ask-fabro-readiness) and the LLM client itself (for the spawned // title-generation task). `ready_llm_provider_ids` would otherwise call // `resolve_llm_client` a second time and discard the client. - let llm_client_for_title = match state.resolve_llm_client().await { - Ok(result) => Some(result), - Err(err) => { - tracing::warn!(error = ?err, "Failed to resolve LLM client while creating run"); - None - } - }; - let ready_provider_ids = llm_client_for_title - .as_ref() - .map(LlmClientResult::provider_ids) - .unwrap_or_default(); + let (llm_result, ready_provider_ids) = state.resolve_llm_client_with_ready_ids().await; + let llm_client_for_title = llm_result.ok(); let run_materialization_provider_ids = { #[cfg(any(test, feature = "test-support"))] { @@ -835,17 +825,7 @@ async fn run_preflight( return ApiError::bad_request(format!("Run config variable interpolation failed: {err}")) .into_response(); } - let llm_result = state.resolve_llm_client().await; - if let Err(error) = &llm_result { - tracing::warn!( - error = ?error, - "Failed to resolve LLM client while checking ready providers" - ); - } - let ready_providers = llm_result - .as_ref() - .map(LlmClientResult::provider_ids) - .unwrap_or_default(); + let (llm_result, ready_providers) = state.resolve_llm_client_with_ready_ids().await; let mut validated = match run_manifest::validate_prepared_manifest_for_preflight( &prepared, state.catalog(), @@ -859,21 +839,14 @@ async fn run_preflight( Err(err) => return ApiError::bad_request(err.to_string()).into_response(), }; validated.promote_template_undefined_variables_to_errors(); - let response = match run_manifest::run_preflight( - &state, - &prepared, - &validated, - &ready_providers, - llm_result, - ) - .await - { - Ok((response, _ok)) => response, - Err(err) => { - return ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()) - .into_response(); - } - }; + let response = + match run_manifest::run_preflight(&state, &prepared, &validated, llm_result).await { + Ok((response, _ok)) => response, + Err(err) => { + return ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()) + .into_response(); + } + }; (StatusCode::OK, Json(response)).into_response() } diff --git a/lib/components/fabro-workflow/src/operations/create.rs b/lib/components/fabro-workflow/src/operations/create.rs index 39bd097bf..2ec40d443 100644 --- a/lib/components/fabro-workflow/src/operations/create.rs +++ b/lib/components/fabro-workflow/src/operations/create.rs @@ -312,7 +312,7 @@ fn create_from_source( .filter(|provider| !provider.is_empty()) .map(ProviderId::new), &options.configured_providers, - None, + false, &options.catalog, )?; @@ -337,7 +337,7 @@ pub(super) fn preprocess_and_validate( render_mode: RenderMode, default_provider: Option, eligible_providers: &[ProviderId], - fallback_providers: Option<&[ProviderId]>, + catalog_fallback: bool, catalog: &Arc, ) -> Result { let mut parsed = pipeline::parse(dot_source)?; @@ -353,7 +353,7 @@ pub(super) fn preprocess_and_validate( catalog: Arc::clone(catalog), default_provider, eligible_providers: eligible_providers.iter().cloned().collect(), - fallback_providers: fallback_providers.map(|providers| providers.iter().cloned().collect()), + catalog_fallback, })?; Ok(pipeline::validate(transformed, catalog.as_ref(), &[])) } @@ -582,7 +582,7 @@ reasoning = false RenderMode::Structural, None, &test_provider_ids(), - None, + false, &test_catalog(), ) .unwrap() @@ -753,7 +753,7 @@ reasoning = false RenderMode::Strict, None, &test_provider_ids(), - None, + false, &test_catalog(), ); let Err(err) = result else { @@ -792,7 +792,7 @@ reasoning = false RenderMode::Strict, None, &test_provider_ids(), - None, + false, &test_catalog(), ); let Err(err) = result else { diff --git a/lib/components/fabro-workflow/src/operations/mod.rs b/lib/components/fabro-workflow/src/operations/mod.rs index 9e5726515..38b7d9aaa 100644 --- a/lib/components/fabro-workflow/src/operations/mod.rs +++ b/lib/components/fabro-workflow/src/operations/mod.rs @@ -22,7 +22,7 @@ pub use rewind::{RewindInput, RewindOutcome, rewind}; pub use source::WorkflowInput; pub use start::{StartServices, Started, start}; pub use timeline::{ForkTarget, RunTimeline, TimelineEntry, build_timeline, timeline}; -pub use validate::{ValidateInput, validate, validate_with_provider_fallback}; +pub use validate::{ValidateInput, validate, validate_with_ready_providers}; pub use crate::pipeline::{LlmSpec, SandboxEnvSpec}; pub use crate::transforms::RenderMode; diff --git a/lib/components/fabro-workflow/src/operations/start.rs b/lib/components/fabro-workflow/src/operations/start.rs index 9c2e3ed8d..ba720b11f 100644 --- a/lib/components/fabro-workflow/src/operations/start.rs +++ b/lib/components/fabro-workflow/src/operations/start.rs @@ -622,6 +622,7 @@ fn resolve_start_llm( &eligible, settings.model.name.as_deref(), settings.model.provider.as_deref(), + false, )?; let fallback_chain = resolve_fallback_chain(catalog, &provider_id, &model, &settings.model, &eligible)?; diff --git a/lib/components/fabro-workflow/src/operations/validate.rs b/lib/components/fabro-workflow/src/operations/validate.rs index 2fb7235b0..c2b990f5c 100644 --- a/lib/components/fabro-workflow/src/operations/validate.rs +++ b/lib/components/fabro-workflow/src/operations/validate.rs @@ -33,23 +33,23 @@ pub fn validate(input: ValidateInput) -> Result { .all_provider_ids() .into_iter() .collect::>(); - validate_with_provider_sets(input, &eligible_providers, None) + validate_with_eligible_providers(input, &eligible_providers, false) } -/// Parse, transform, and validate while preferring one provider snapshot and -/// falling back to another only for provider-readiness selection failures. -pub fn validate_with_provider_fallback( +/// Parse, transform, and validate, resolving models against the ready +/// providers first and falling back to the full catalog only for +/// provider-readiness selection failures. +pub fn validate_with_ready_providers( input: ValidateInput, - preferred_providers: &[ProviderId], - fallback_providers: &[ProviderId], + ready_providers: &[ProviderId], ) -> Result { - validate_with_provider_sets(input, preferred_providers, Some(fallback_providers)) + validate_with_eligible_providers(input, ready_providers, true) } -fn validate_with_provider_sets( +fn validate_with_eligible_providers( input: ValidateInput, eligible_providers: &[ProviderId], - fallback_providers: Option<&[ProviderId]>, + catalog_fallback: bool, ) -> Result { let resolved = resolve_workflow(ResolveWorkflowInput { workflow: input.workflow, @@ -79,7 +79,7 @@ fn validate_with_provider_sets( .filter(|provider| !provider.is_empty()) .map(fabro_model::ProviderId::new), eligible_providers, - fallback_providers, + catalog_fallback, &input.catalog, ) } diff --git a/lib/components/fabro-workflow/src/pipeline/transform.rs b/lib/components/fabro-workflow/src/pipeline/transform.rs index cc3fe089d..b399d6637 100644 --- a/lib/components/fabro-workflow/src/pipeline/transform.rs +++ b/lib/components/fabro-workflow/src/pipeline/transform.rs @@ -68,7 +68,7 @@ pub fn transform(parsed: Parsed, options: &TransformOptions) -> Result, pub default_provider: Option, pub eligible_providers: HashSet, - pub fallback_providers: Option>, + /// Fall back to the full catalog when the eligible providers cannot + /// supply a requested model, instead of erroring. + pub catalog_fallback: bool, } /// Options for the FINALIZE phase. diff --git a/lib/components/fabro-workflow/src/pipeline/validate.rs b/lib/components/fabro-workflow/src/pipeline/validate.rs index 8e19bb5a6..f0cd51dbd 100644 --- a/lib/components/fabro-workflow/src/pipeline/validate.rs +++ b/lib/components/fabro-workflow/src/pipeline/validate.rs @@ -51,7 +51,7 @@ mod tests { catalog: std::sync::Arc::clone(&catalog), default_provider: None, eligible_providers: catalog.all_provider_ids(), - fallback_providers: None, + catalog_fallback: false, }) .unwrap(); validate(transformed, catalog.as_ref(), &[]) diff --git a/lib/components/fabro-workflow/src/run_materialization.rs b/lib/components/fabro-workflow/src/run_materialization.rs index 27183bcf5..d1118b076 100644 --- a/lib/components/fabro-workflow/src/run_materialization.rs +++ b/lib/components/fabro-workflow/src/run_materialization.rs @@ -14,31 +14,27 @@ pub fn materialize_run( catalog: &Catalog, configured_providers: &[ProviderId], ) -> Result { - materialize_run_with_provider_sets(settings, graph, catalog, configured_providers, None) + materialize_run_with_eligible_providers(settings, graph, catalog, configured_providers, false) } -pub fn materialize_run_with_provider_fallback( +/// Materialize while resolving the run model against the ready providers +/// first, falling back to the full catalog only for provider-readiness +/// selection failures. +pub fn materialize_run_with_ready_providers( settings: WorkflowSettings, graph: &Graph, catalog: &Catalog, - preferred_providers: &[ProviderId], - fallback_providers: &[ProviderId], + ready_providers: &[ProviderId], ) -> Result { - materialize_run_with_provider_sets( - settings, - graph, - catalog, - preferred_providers, - Some(fallback_providers), - ) + materialize_run_with_eligible_providers(settings, graph, catalog, ready_providers, true) } -fn materialize_run_with_provider_sets( +fn materialize_run_with_eligible_providers( mut settings: WorkflowSettings, graph: &Graph, catalog: &Catalog, - configured_providers: &[ProviderId], - fallback_providers: Option<&[ProviderId]>, + eligible_providers: &[ProviderId], + catalog_fallback: bool, ) -> Result { let configured_model = settings.run.model.name.take(); let configured_provider = settings.run.model.provider.take(); @@ -55,25 +51,17 @@ fn materialize_run_with_provider_sets( let provider = configured_provider.or(graph_provider); let model = configured_model.or(graph_model); - let eligible = configured_providers.iter().cloned().collect::>(); - let fallback = - fallback_providers.map(|providers| providers.iter().cloned().collect::>()); - let provider = provider - .as_deref() - .filter(|provider| !provider.is_empty()) - .map(ProviderId::new); - let selected = match fallback { - Some(fallback) => catalog.resolve_selection_with_fallback( - model.as_deref(), - provider.as_ref(), - &eligible, - &fallback, - ), - None => catalog.resolve_selection(model.as_deref(), provider.as_ref(), &eligible), - }?; + let eligible = eligible_providers.iter().cloned().collect::>(); + let (resolved_model, resolved_provider) = resolve_run_model( + catalog, + &eligible, + model.as_deref(), + provider.as_deref(), + catalog_fallback, + )?; - settings.run.model.name = Some(selected.model); - settings.run.model.provider = Some(selected.provider.into_inner()); + settings.run.model.name = Some(resolved_model); + settings.run.model.provider = Some(resolved_provider.into_inner()); let goal = graph.goal().to_string(); settings.run.goal = if goal.is_empty() { @@ -99,10 +87,15 @@ pub(crate) fn resolve_run_model( eligible: &HashSet, model: Option<&str>, provider: Option<&str>, + catalog_fallback: bool, ) -> Result<(String, ProviderId), ModelSelectionError> { let provider = provider .filter(|provider| !provider.is_empty()) .map(ProviderId::new); - let selected = catalog.resolve_selection(model, provider.as_ref(), eligible)?; + let selected = if catalog_fallback { + catalog.resolve_selection_with_catalog_fallback(model, provider.as_ref(), eligible)? + } else { + catalog.resolve_selection(model, provider.as_ref(), eligible)? + }; Ok((selected.model, selected.provider)) } diff --git a/lib/components/fabro-workflow/src/transforms/model_resolution.rs b/lib/components/fabro-workflow/src/transforms/model_resolution.rs index 00e71ba74..12f29ac5a 100644 --- a/lib/components/fabro-workflow/src/transforms/model_resolution.rs +++ b/lib/components/fabro-workflow/src/transforms/model_resolution.rs @@ -13,7 +13,7 @@ pub struct ModelResolutionTransform { catalog: Arc, default_provider: Option, eligible_providers: HashSet, - fallback_providers: Option>, + catalog_fallback: bool, } impl ModelResolutionTransform { @@ -24,7 +24,7 @@ impl ModelResolutionTransform { catalog, default_provider: None, eligible_providers, - fallback_providers: None, + catalog_fallback: false, } } @@ -34,7 +34,7 @@ impl ModelResolutionTransform { catalog, default_provider: None, eligible_providers, - fallback_providers: None, + catalog_fallback: false, } } @@ -44,12 +44,11 @@ impl ModelResolutionTransform { self } + /// When enabled, provider-readiness selection failures fall back to the + /// full catalog instead of erroring. #[must_use] - pub fn with_fallback_providers( - mut self, - fallback_providers: Option>, - ) -> Self { - self.fallback_providers = fallback_providers; + pub fn with_catalog_fallback(mut self, catalog_fallback: bool) -> Self { + self.catalog_fallback = catalog_fallback; self } @@ -58,18 +57,15 @@ impl ModelResolutionTransform { model: &str, explicit_provider: Option<&ProviderId>, ) -> Result<(String, ProviderId), Error> { - let selected = match &self.fallback_providers { - Some(fallback_providers) => self.catalog.resolve_selection_with_fallback( + let selected = if self.catalog_fallback { + self.catalog.resolve_selection_with_catalog_fallback( Some(model), explicit_provider, &self.eligible_providers, - fallback_providers, - ), - None => self.catalog.resolve_selection( - Some(model), - explicit_provider, - &self.eligible_providers, - ), + ) + } else { + self.catalog + .resolve_selection(Some(model), explicit_provider, &self.eligible_providers) }?; Ok((selected.model, selected.provider)) } @@ -378,7 +374,7 @@ enabled = true Arc::clone(&catalog), HashSet::from([ProviderId::new("openrouter")]), ) - .with_fallback_providers(Some(catalog.all_provider_ids())) + .with_catalog_fallback(true) .apply(graph) .unwrap(); diff --git a/lib/components/fabro-workflow/tests/it/integration.rs b/lib/components/fabro-workflow/tests/it/integration.rs index 44307ca81..8e415c81a 100644 --- a/lib/components/fabro-workflow/tests/it/integration.rs +++ b/lib/components/fabro-workflow/tests/it/integration.rs @@ -4908,7 +4908,7 @@ async fn import_e2e_through_engine() { catalog: std::sync::Arc::clone(&catalog), default_provider: None, eligible_providers: catalog.all_provider_ids(), - fallback_providers: None, + catalog_fallback: false, }) .unwrap(); let validated = validate(transformed, catalog.as_ref(), &[]); diff --git a/lib/foundation/fabro-model/src/catalog.rs b/lib/foundation/fabro-model/src/catalog.rs index da9ab96fd..db59dcb98 100644 --- a/lib/foundation/fabro-model/src/catalog.rs +++ b/lib/foundation/fabro-model/src/catalog.rs @@ -1102,19 +1102,18 @@ impl Catalog { } /// Resolve a selection against a preferred provider snapshot, falling back - /// to a broader eligible set only when the preferred set cannot supply the - /// requested provider or model. + /// to every provider in the catalog only when the preferred set cannot + /// supply the requested provider or model. /// /// This is useful for readiness checks: ready providers remain preferred, /// while a catalog-only offering can still be selected so the caller can /// report why its provider is unavailable. Semantic failures such as an /// unknown provider do not fall back. - pub fn resolve_selection_with_fallback( + pub fn resolve_selection_with_catalog_fallback( &self, selector: Option<&str>, explicit_provider: Option<&ProviderId>, preferred_providers: &HashSet, - fallback_providers: &HashSet, ) -> Result { match self.resolve_selection(selector, explicit_provider, preferred_providers) { Ok(selected) => Ok(selected), @@ -1122,7 +1121,7 @@ impl Catalog { ModelSelectionError::ProviderUnavailable { .. } | ModelSelectionError::NoEligibleOffering { .. } | ModelSelectionError::NoDefaultModel { .. }, - ) => self.resolve_selection(selector, explicit_provider, fallback_providers), + ) => self.resolve_selection(selector, explicit_provider, &self.all_provider_ids()), Err(error) => Err(error), } } @@ -4021,30 +4020,19 @@ adapter = "openai_compatible" let openai = ProviderId::openai(); let openrouter = ProviderId::new("openrouter"); let ready = HashSet::from([openrouter.clone()]); - let catalog_providers = HashSet::from([openai.clone(), openrouter.clone()]); let shared = catalog - .resolve_selection_with_fallback(Some("portable"), None, &ready, &catalog_providers) + .resolve_selection_with_catalog_fallback(Some("portable"), None, &ready) .unwrap(); assert_eq!(shared.provider, openrouter); let pinned = catalog - .resolve_selection_with_fallback( - Some("portable"), - Some(&openai), - &ready, - &catalog_providers, - ) + .resolve_selection_with_catalog_fallback(Some("portable"), Some(&openai), &ready) .unwrap(); assert_eq!(pinned.provider, openai); let unknown = catalog - .resolve_selection_with_fallback( - Some("provider-private-preview"), - None, - &ready, - &catalog_providers, - ) + .resolve_selection_with_catalog_fallback(Some("provider-private-preview"), None, &ready) .unwrap(); assert_eq!(unknown.provider, ProviderId::new("openrouter")); assert_eq!(unknown.model, "provider-private-preview");