From 4e71554773d3807f21ff20bd4524454381a96ded Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Thu, 1 Oct 2026 14:44:01 -0400 Subject: [PATCH 1/3] Apply CLI model and provider overrides to agent execution --- lib/apps/fabro-cli/tests/it/cmd/mod.rs | 1 + lib/apps/fabro-cli/tests/it/cmd/run_model.rs | 192 ++++++++++++++++++ .../fabro-server/src/manifest_validation.rs | 5 +- lib/apps/fabro-server/src/petri_check.rs | 24 ++- lib/apps/fabro-server/src/run_compiler.rs | 10 +- lib/apps/fabro-server/src/run_manifest.rs | 10 +- .../fabro-server/src/server/petri_runs.rs | 1 + lib/components/fabro-petri/src/check.rs | 28 ++- lib/components/fabro-petri/tests/check.rs | 9 +- 9 files changed, 261 insertions(+), 19 deletions(-) create mode 100644 lib/apps/fabro-cli/tests/it/cmd/run_model.rs diff --git a/lib/apps/fabro-cli/tests/it/cmd/mod.rs b/lib/apps/fabro-cli/tests/it/cmd/mod.rs index f3c0b9b4c..b8952cdd2 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/mod.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/mod.rs @@ -44,6 +44,7 @@ mod repo_init; mod resume; mod rm; mod run; +mod run_model; mod runner; mod sandbox_cp; mod sandbox_preview; diff --git a/lib/apps/fabro-cli/tests/it/cmd/run_model.rs b/lib/apps/fabro-cli/tests/it/cmd/run_model.rs new file mode 100644 index 000000000..8c5720021 --- /dev/null +++ b/lib/apps/fabro-cli/tests/it/cmd/run_model.rs @@ -0,0 +1,192 @@ +use std::fmt::Write as _; + +use fabro_test::{TestContext, test_context}; +use httpmock::MockServer; +use serde_json::json; + +/// Exercise admission and the real agent against two local providers. Checking +/// the request path and wire model catches overrides that only reach run state. +fn assert_agent_route( + mut context: TestContext, + flags: &[&str], + graph_defaults: &str, + node_settings: &str, + expected_provider: &str, + expected_model: &str, +) { + let endpoint = MockServer::start(); + let mut settings = "_version = 1\n[server.auth]\nmethods = [\"dev-token\"]\n".to_string(); + for provider in ["primary", "alternate"] { + write!( + settings, + r#" +[llm.providers.{provider}] +display_name = "{provider}" +base_url = "{base}/{provider}/v1" +auth = {{ type = "none" }} +default_model = "base" +[llm.providers.{provider}.metadata.agent] +profile = "openai" +"#, + base = endpoint.base_url(), + ) + .expect("provider settings should format"); + for model in ["base", "override", "node"] { + write!( + settings, + r#" +[llm.providers.{provider}.models.{model}] +display_name = "{model}" +api_model = "{provider}-{model}-wire" +limits = {{ context_tokens = 32000, max_output_tokens = 1000 }} +capabilities = {{ text = true, tools = true }} +"#, + ) + .expect("model settings should format"); + } + } + context.write_home(".fabro/settings.toml", settings); + context.isolated_server(); + context.write_temp( + "workflow.toml", + r#"_version = 1 +[workflow] +graph = "workflow.fabro" +[run.model] +provider = "primary" +name = "base" +[run.pull_request] +enabled = false +"#, + ); + context.write_temp( + "workflow.fabro", + format!( + r#"digraph ModelSelection {{ + {graph_defaults} + start [shape=Mdiamond]; + work [shape=box, prompt="Say hello."]; + {node_settings} + exit [shape=Msquare]; + start -> work -> exit; +}}"#, + ), + ); + + let wire_model = format!("{expected_provider}-{expected_model}-wire"); + let chunk = |delta, finish_reason| { + json!({ + "id": "scripted-response", "object": "chat.completion.chunk", + "created": 1, "model": wire_model, + "choices": [{"index": 0, "delta": delta, "finish_reason": finish_reason}] + }) + }; + let response = format!( + "data: {}\n\ndata: {}\n\ndata: [DONE]\n\n", + chunk( + json!({"role": "assistant", "content": "Hello."}), + json!(null) + ), + chunk(json!({}), json!("stop")), + ); + let expected = endpoint.mock(|when, then| { + when.method("POST") + .path(format!("/{expected_provider}/v1/chat/completions")) + .json_body_includes(json!({"model": wire_model, "stream": true}).to_string()); + then.status(200) + .header("Content-Type", "text/event-stream") + .body(&response); + }); + // Respond immediately to a wrong route too, so the regression fails with + // the mismatched request instead of waiting through model retry backoff. + let unexpected = endpoint.mock(|when, then| { + when.method("POST"); + then.status(200) + .header("Content-Type", "text/event-stream") + .body(&response); + }); + + let output = context + .run_cmd() + .args(["--auto-approve", "--environment", "local"]) + .args(flags) + .arg(context.temp_dir.join("workflow.toml")) + .output() + .expect("run should execute"); + assert!( + output.status.success(), + "run failed:\n{}\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); + unexpected.assert_calls(0); + assert!( + expected.calls() > 0, + "the agent must call the requested route" + ); +} + +#[test] +fn workflow_model_controls_agent_requests() { + assert_agent_route(test_context!(), &[], "", "", "primary", "base"); +} + +#[test] +fn model_flag_overrides_workflow_model_at_execution() { + assert_agent_route( + test_context!(), + &["--model", "override"], + "", + "", + "primary", + "override", + ); +} + +#[test] +fn provider_flag_overrides_workflow_provider_at_execution() { + assert_agent_route( + test_context!(), + &["--provider", "alternate"], + "", + "", + "alternate", + "base", + ); +} + +#[test] +fn model_flags_override_workflow_and_graph_defaults_at_execution() { + assert_agent_route( + test_context!(), + &["--model", "override", "--provider", "alternate"], + r#"graph [default_model="base", default_provider="primary"];"#, + "", + "alternate", + "override", + ); +} + +#[test] +fn explicit_node_model_takes_precedence_over_flags() { + assert_agent_route( + test_context!(), + &["--model", "override", "--provider", "alternate"], + "", + r#"work [model="node", provider="primary"];"#, + "primary", + "node", + ); +} + +#[test] +fn node_stylesheet_model_takes_precedence_over_flags() { + assert_agent_route( + test_context!(), + &["--model", "override", "--provider", "alternate"], + "", + r##"graph [model_stylesheet="#work { model: node; provider: primary; }"];"##, + "primary", + "node", + ); +} diff --git a/lib/apps/fabro-server/src/manifest_validation.rs b/lib/apps/fabro-server/src/manifest_validation.rs index 04e1cd4e8..329049b51 100644 --- a/lib/apps/fabro-server/src/manifest_validation.rs +++ b/lib/apps/fabro-server/src/manifest_validation.rs @@ -103,7 +103,10 @@ pub fn validate_collected_workflow( &lowered.entrypoint, &settings, &HashMap::new(), - petri_check::launch_without_catalog(&settings), + petri_check::with_model_overrides( + petri_check::launch_without_catalog(&settings), + run_overrides.and_then(|run| run.model.as_ref()), + ), offline_runtime(run_overrides), false, ) diff --git a/lib/apps/fabro-server/src/petri_check.rs b/lib/apps/fabro-server/src/petri_check.rs index a720f644e..973b1f171 100644 --- a/lib/apps/fabro-server/src/petri_check.rs +++ b/lib/apps/fabro-server/src/petri_check.rs @@ -12,6 +12,7 @@ use std::collections::{BTreeMap, HashMap, HashSet}; use std::path::PathBuf; +use fabro_config::RunModelLayer; use fabro_llm::lithos_catalog::Catalog; use fabro_llm::selection; use fabro_petri::check::{self, Admitted, Bundle, CheckError, CheckRequest, Diagnostic, Launch}; @@ -54,6 +55,7 @@ pub(crate) fn launch( environment: environment.map(str::to_owned), goal: launch_goal(settings), repository, + ..Launch::default() } } @@ -81,14 +83,28 @@ fn launch_goal(settings: &WorkflowSettings) -> Option { /// name, for a check away from the server. pub(crate) fn launch_without_catalog(settings: &WorkflowSettings) -> Launch { Launch { - model: settings.run.model.name.clone(), - provider: settings.run.model.provider.clone(), + model: settings.run.model.name.clone(), + provider: settings.run.model.provider.clone(), environment: None, - goal: launch_goal(settings), - repository: None, + goal: launch_goal(settings), + repository: None, + ..Launch::default() } } +/// Preserve explicit model flags separately from defaults resolved from the +/// workflow and server, so they can outrank file layers during admission. +pub(crate) fn with_model_overrides( + mut launch: Launch, + overrides: Option<&RunModelLayer>, +) -> Launch { + if let Some(overrides) = overrides { + launch.model_override.clone_from(&overrides.name); + launch.provider_override.clone_from(&overrides.provider); + } + launch +} + /// The check request for `bundle`'s `entrypoint`: every file of every /// workflow in the bundle at its bundle-relative path, the run's inputs and /// variables, the launch and the runtime. `unbound_is_warning` makes a diff --git a/lib/apps/fabro-server/src/run_compiler.rs b/lib/apps/fabro-server/src/run_compiler.rs index 650d7e562..bfd6a9542 100644 --- a/lib/apps/fabro-server/src/run_compiler.rs +++ b/lib/apps/fabro-server/src/run_compiler.rs @@ -30,7 +30,7 @@ use std::path::PathBuf; use fabro_config::parse::{self, ParseError, SettingsSource}; use fabro_config::{ EnvironmentDockerfileLayer, EnvironmentImageLayer, EnvironmentLayer, MergeMap, RunLayer, - SettingsLayer, WorkflowSettingsBuilder, + RunModelLayer, SettingsLayer, WorkflowSettingsBuilder, }; use fabro_types::settings::interp::{InterpString, ResolveError}; use fabro_types::settings::run::{McpServerSettings, RunGoal}; @@ -100,6 +100,7 @@ struct RunMetadata { /// The environment the run overrides selected, for Petri's settings /// layer. environment_id: Option, + model_overrides: Option, storage_root: PathBuf, workflow_slug: Option, workflow_version_id: Option, @@ -174,6 +175,10 @@ impl PreparedRun { self.layered.metadata.environment_id.as_deref() } + pub(crate) fn model_overrides(&self) -> Option<&RunModelLayer> { + self.layered.metadata.model_overrides.as_ref() + } + pub(crate) fn resolve_run_id(mut self) -> (Self, RunId) { let run_id = self.layered.metadata.run_id.unwrap_or_default(); self.layered.metadata.run_id = Some(run_id); @@ -316,6 +321,7 @@ pub(crate) fn normalize_source(input: RawRunCompilerInput) -> Result Result CreateRunPersistenceInput { // Consumed at admission, as the launch's environment; the resolved // settings carry the environment the run persists. environment_id: _, + model_overrides: _, storage_root, workflow_slug, workflow_version_id, diff --git a/lib/apps/fabro-server/src/run_manifest.rs b/lib/apps/fabro-server/src/run_manifest.rs index d1b4ad105..33c3ffa2f 100644 --- a/lib/apps/fabro-server/src/run_manifest.rs +++ b/lib/apps/fabro-server/src/run_manifest.rs @@ -9,7 +9,7 @@ use fabro_api::types; use fabro_config::parse::SettingsSource; use fabro_config::run::resolve_run_goal_from_namespace; use fabro_config::{ - CliLayer, CliOutputLayer, EnvironmentLayer, MergeMap, RunLayer, SettingsLayer, + CliLayer, CliOutputLayer, EnvironmentLayer, MergeMap, RunLayer, RunModelLayer, SettingsLayer, WorkflowSettingsBuilder, parse_input_overrides, parse_labels, project, }; use fabro_dot::WorkflowGraph; @@ -53,6 +53,7 @@ pub(crate) struct PreparedManifest { /// The entrypoint's DOT as written: what the render endpoint draws. pub root_source: String, pub settings: WorkflowSettings, + pub model_overrides: Option, pub target_path: ManifestPath, pub workflow_bundle: WorkflowBundle, pub source_directory: PathBuf, @@ -92,6 +93,10 @@ pub(crate) fn prepare_manifest_with_environment_defaults( let args_overrides = manifest_args_overrides(manifest.args.as_ref()).context("failed to parse manifest args")?; + let model_overrides = args_overrides + .run + .as_ref() + .and_then(|run| run.model.clone()); let mut workflow_settings_builder = WorkflowSettingsBuilder::new() .server_manifest_defaults( manifest_run_defaults.clone(), @@ -166,6 +171,7 @@ pub(crate) fn prepare_manifest_with_environment_defaults( git: manifest.git.clone(), root_source, settings, + model_overrides, target_path, workflow_bundle, source_directory, @@ -205,7 +211,7 @@ pub(crate) fn validate_prepared_manifest( &prepared.target_path, &prepared.settings, vars, - launch, + petri_check::with_model_overrides(launch, prepared.model_overrides.as_ref()), runtime, unbound_is_warning, )?; diff --git a/lib/apps/fabro-server/src/server/petri_runs.rs b/lib/apps/fabro-server/src/server/petri_runs.rs index 99ce39575..1a5b99a0c 100644 --- a/lib/apps/fabro-server/src/server/petri_runs.rs +++ b/lib/apps/fabro-server/src/server/petri_runs.rs @@ -275,6 +275,7 @@ pub(crate) async fn admit( repository, ); let dry_run = settings.run.execution.mode == RunMode::DryRun; + let launch = petri_check::with_model_overrides(launch, prepared.model_overrides()); let request = petri_check::check_request( prepared.workflow_bundle(), prepared.entrypoint(), diff --git a/lib/components/fabro-petri/src/check.rs b/lib/components/fabro-petri/src/check.rs index 62c0523d4..24e9ab63a 100644 --- a/lib/components/fabro-petri/src/check.rs +++ b/lib/components/fabro-petri/src/check.rs @@ -12,7 +12,9 @@ //! //! The launch binds the compile variables the Fabro frontend reads: //! `petri.launch_model` and `petri.launch_provider` as the model default -//! below every file layer, `petri.launch_environment` as the environment +//! below every file layer, `petri.model_override` and `petri.provider_override` +//! as explicit selections above file and graph defaults but below node and +//! stylesheet choices, `petri.launch_environment` as the environment //! the run selected over every file layer, `petri.launch_goal` as the goal //! the run resolved over every file layer and the graph's own, and //! `petri.repository` as the repository the root `start` stage checks out. A @@ -27,7 +29,8 @@ use petri_frontend_attractor::kinds::{AGENT_KIND, PROMPT_KIND}; use petri_runtime::LoadError; use petri_runtime::frontend::{ self, CompileInputs, LAUNCH_ENVIRONMENT_VAR, LAUNCH_GOAL_VAR, LAUNCH_MODEL_VAR, - LAUNCH_PROVIDER_VAR, MapFiles, REPOSITORY_VAR, Severity, + LAUNCH_PROVIDER_VAR, MODEL_OVERRIDE_VAR, MapFiles, PROVIDER_OVERRIDE_VAR, REPOSITORY_VAR, + Severity, }; use petri_runtime::ir::Graph; use serde::{Deserialize, Serialize}; @@ -69,21 +72,25 @@ impl Bundle { /// them, the environment selection above them, and the repository. #[derive(Clone, Debug, Default)] pub struct Launch { - pub model: Option, - pub provider: Option, + pub model: Option, + pub provider: Option, + /// Explicit run overrides, above workflow and graph defaults but below + /// node attributes and stylesheets. Kept separate from catalog defaults. + pub model_override: Option, + pub provider_override: Option, /// The environment the run selected, by its id in the server's /// catalog, over every layer's `[run.environment]`, as the intent's /// selection overrides the bundle in Fabro's own resolution; `None` /// leaves the layers to select. - pub environment: Option, + pub environment: Option, /// The goal the run resolved (the intent's override, else the settings' /// `[run] goal` from any layer), over the bundle's `[run] goal` and the /// graph's own `goal`, so the stages execute with the goal the run /// shows; `None` leaves the bundle's layers and the graph to state it. - pub goal: Option, + pub goal: Option, /// The local repository the root `start` stage checks out into the /// workspace; `None` starts the run from an empty workspace. - pub repository: Option, + pub repository: Option, } /// One check: the bundle, the run's inputs and variables, the launch and @@ -219,6 +226,13 @@ fn compile_inputs( compile .vars .insert(LAUNCH_PROVIDER_VAR.into(), text(&launch.provider)); + compile + .vars + .insert(MODEL_OVERRIDE_VAR.into(), text(&launch.model_override)); + compile.vars.insert( + PROVIDER_OVERRIDE_VAR.into(), + text(&launch.provider_override), + ); if let Some(environment) = &launch.environment { compile.vars.insert( LAUNCH_ENVIRONMENT_VAR.into(), diff --git a/lib/components/fabro-petri/tests/check.rs b/lib/components/fabro-petri/tests/check.rs index 502e4bdc3..bf68adff7 100644 --- a/lib/components/fabro-petri/tests/check.rs +++ b/lib/components/fabro-petri/tests/check.rs @@ -140,11 +140,12 @@ async fn a_launch_binds_the_repository_and_the_model_default() { inputs: BTreeMap::new(), vars: BTreeMap::new(), launch: Launch { - model: Some("gpt-5.4".to_string()), - provider: None, + model: Some("gpt-5.4".to_string()), + provider: None, environment: None, - goal: None, - repository: Some(repository.path().to_path_buf()), + goal: None, + repository: Some(repository.path().to_path_buf()), + ..Launch::default() }, runtime: RuntimeSpec::default(), unbound_is_warning: false, From b401ac0a10bfef903345187a2a06ea9db55bc7b2 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Thu, 1 Oct 2026 16:47:18 -0400 Subject: [PATCH 2/3] Bind CLI model flags as Petri's launch model and settings as its default Petri now treats `petri.launch_model` and `petri.launch_provider` as the model a run's flags ask for, above the file layers and the graph's defaults. A host's last-resort default moved to `petri.default_model` and `petri.default_provider`. Pin Petri at the merge of that change and bind to it: the explicit `--model`/`--provider` flags go to the launch variables, and the model the settings resolved (or the catalog default) goes to the default variables. `Launch` now names the two pairs `model`/`provider` and `default_model`/`default_provider`, matching Petri, in place of the separate override fields. Co-Authored-By: Claude Opus 5.5 --- lib/apps/fabro-server/src/petri_check.rs | 28 +++++++------ lib/components/fabro-petri/src/check.rs | 50 ++++++++++++----------- lib/components/fabro-petri/tests/check.rs | 10 +++-- lib/components/fabro-petri/tests/model.rs | 2 +- 4 files changed, 50 insertions(+), 40 deletions(-) diff --git a/lib/apps/fabro-server/src/petri_check.rs b/lib/apps/fabro-server/src/petri_check.rs index 973b1f171..a08cd516f 100644 --- a/lib/apps/fabro-server/src/petri_check.rs +++ b/lib/apps/fabro-server/src/petri_check.rs @@ -28,11 +28,12 @@ use lithos_llm::catalog::ProviderId; pub(crate) const NO_READY_PROVIDER_RULE: &str = "fabro.model.no_ready_provider"; /// The launch Fabro binds around the settings: the run's model and provider -/// below them, and the environment the run selected and the goal the run -/// resolved above them. When the settings name neither model nor provider, the -/// default offering of the eligible providers is bound as the launch model -/// alone: a node that names no model runs on it, and a node that names a model -/// the catalog lacks stays unqualified, so Petri's admission refuses it. +/// as the default below them, and the environment the run selected and the +/// goal the run resolved above them. When the settings name neither model nor +/// provider, the default offering of the eligible providers is bound as the +/// default model alone: a node that names no model runs on it, and a node that +/// names a model the catalog lacks stays unqualified, so Petri's admission +/// refuses it. pub(crate) fn launch( catalog: &Catalog, settings: &WorkflowSettings, @@ -50,8 +51,8 @@ pub(crate) fn launch( .map(|offering| offering.model.id().to_string()) }); Launch { - model, - provider: settings.run.model.provider.clone(), + default_model: model, + default_provider: settings.run.model.provider.clone(), environment: environment.map(str::to_owned), goal: launch_goal(settings), repository, @@ -83,8 +84,8 @@ fn launch_goal(settings: &WorkflowSettings) -> Option { /// name, for a check away from the server. pub(crate) fn launch_without_catalog(settings: &WorkflowSettings) -> Launch { Launch { - model: settings.run.model.name.clone(), - provider: settings.run.model.provider.clone(), + default_model: settings.run.model.name.clone(), + default_provider: settings.run.model.provider.clone(), environment: None, goal: launch_goal(settings), repository: None, @@ -92,15 +93,16 @@ pub(crate) fn launch_without_catalog(settings: &WorkflowSettings) -> Launch { } } -/// Preserve explicit model flags separately from defaults resolved from the -/// workflow and server, so they can outrank file layers during admission. +/// The run's explicit model flags (`--model`, `--provider`), kept apart from +/// the default the settings resolved, so they outrank the file layers and +/// the graph's defaults during admission. pub(crate) fn with_model_overrides( mut launch: Launch, overrides: Option<&RunModelLayer>, ) -> Launch { if let Some(overrides) = overrides { - launch.model_override.clone_from(&overrides.name); - launch.provider_override.clone_from(&overrides.provider); + launch.model.clone_from(&overrides.name); + launch.provider.clone_from(&overrides.provider); } launch } diff --git a/lib/components/fabro-petri/src/check.rs b/lib/components/fabro-petri/src/check.rs index 24e9ab63a..1d1ed66e7 100644 --- a/lib/components/fabro-petri/src/check.rs +++ b/lib/components/fabro-petri/src/check.rs @@ -11,10 +11,11 @@ //! path the map holds the file under. //! //! The launch binds the compile variables the Fabro frontend reads: -//! `petri.launch_model` and `petri.launch_provider` as the model default -//! below every file layer, `petri.model_override` and `petri.provider_override` -//! as explicit selections above file and graph defaults but below node and -//! stylesheet choices, `petri.launch_environment` as the environment +//! `petri.launch_model` and `petri.launch_provider` as the model the run's +//! flags ask for, over every file layer and the graph's defaults but below a +//! model a node names itself, `petri.default_model` and +//! `petri.default_provider` as the run's default model below every file +//! layer, `petri.launch_environment` as the environment //! the run selected over every file layer, `petri.launch_goal` as the goal //! the run resolved over every file layer and the graph's own, and //! `petri.repository` as the repository the root `start` stage checks out. A @@ -28,9 +29,8 @@ use std::path::PathBuf; use petri_frontend_attractor::kinds::{AGENT_KIND, PROMPT_KIND}; use petri_runtime::LoadError; use petri_runtime::frontend::{ - self, CompileInputs, LAUNCH_ENVIRONMENT_VAR, LAUNCH_GOAL_VAR, LAUNCH_MODEL_VAR, - LAUNCH_PROVIDER_VAR, MODEL_OVERRIDE_VAR, MapFiles, PROVIDER_OVERRIDE_VAR, REPOSITORY_VAR, - Severity, + self, CompileInputs, DEFAULT_MODEL_VAR, DEFAULT_PROVIDER_VAR, LAUNCH_ENVIRONMENT_VAR, + LAUNCH_GOAL_VAR, LAUNCH_MODEL_VAR, LAUNCH_PROVIDER_VAR, MapFiles, REPOSITORY_VAR, Severity, }; use petri_runtime::ir::Graph; use serde::{Deserialize, Serialize}; @@ -68,29 +68,34 @@ impl Bundle { } } -/// What the launch binds around the file layers: the model default below -/// them, the environment selection above them, and the repository. +/// What the launch binds around the file layers: the model the run's flags +/// ask for and the environment selection above them, the run's default model +/// below them, and the repository. #[derive(Clone, Debug, Default)] pub struct Launch { - pub model: Option, - pub provider: Option, - /// Explicit run overrides, above workflow and graph defaults but below - /// node attributes and stylesheets. Kept separate from catalog defaults. - pub model_override: Option, - pub provider_override: Option, + /// The model and provider the run's flags ask for (`--model`, + /// `--provider`), over every file layer and the graph's defaults but + /// below a model a node names itself. + pub model: Option, + pub provider: Option, + /// The run's default model and provider (the settings', else the + /// catalog's default), below every file layer: they fill what nothing + /// else names. + pub default_model: Option, + pub default_provider: Option, /// The environment the run selected, by its id in the server's /// catalog, over every layer's `[run.environment]`, as the intent's /// selection overrides the bundle in Fabro's own resolution; `None` /// leaves the layers to select. - pub environment: Option, + pub environment: Option, /// The goal the run resolved (the intent's override, else the settings' /// `[run] goal` from any layer), over the bundle's `[run] goal` and the /// graph's own `goal`, so the stages execute with the goal the run /// shows; `None` leaves the bundle's layers and the graph to state it. - pub goal: Option, + pub goal: Option, /// The local repository the root `start` stage checks out into the /// workspace; `None` starts the run from an empty workspace. - pub repository: Option, + pub repository: Option, } /// One check: the bundle, the run's inputs and variables, the launch and @@ -228,11 +233,10 @@ fn compile_inputs( .insert(LAUNCH_PROVIDER_VAR.into(), text(&launch.provider)); compile .vars - .insert(MODEL_OVERRIDE_VAR.into(), text(&launch.model_override)); - compile.vars.insert( - PROVIDER_OVERRIDE_VAR.into(), - text(&launch.provider_override), - ); + .insert(DEFAULT_MODEL_VAR.into(), text(&launch.default_model)); + compile + .vars + .insert(DEFAULT_PROVIDER_VAR.into(), text(&launch.default_provider)); if let Some(environment) = &launch.environment { compile.vars.insert( LAUNCH_ENVIRONMENT_VAR.into(), diff --git a/lib/components/fabro-petri/tests/check.rs b/lib/components/fabro-petri/tests/check.rs index bf68adff7..f990bb57a 100644 --- a/lib/components/fabro-petri/tests/check.rs +++ b/lib/components/fabro-petri/tests/check.rs @@ -140,8 +140,8 @@ async fn a_launch_binds_the_repository_and_the_model_default() { inputs: BTreeMap::new(), vars: BTreeMap::new(), launch: Launch { - model: Some("gpt-5.4".to_string()), - provider: None, + default_model: Some("gpt-5.4".to_string()), + default_provider: None, environment: None, goal: None, repository: Some(repository.path().to_path_buf()), @@ -154,7 +154,11 @@ async fn a_launch_binds_the_repository_and_the_model_default() { let admitted = check::check(&request).expect("the command bundle is admitted"); let launch = &admitted.graph.params["fabro.launch"]; - assert_eq!(launch["model"], "gpt-5.4"); + assert_eq!(launch["default_model"], "gpt-5.4"); + assert!( + launch["model"].is_null(), + "the default is not a launch flag: {launch}" + ); assert_eq!( launch["clone"]["repository"], repository.path().to_string_lossy().as_ref() diff --git a/lib/components/fabro-petri/tests/model.rs b/lib/components/fabro-petri/tests/model.rs index 7599826f8..c4dc064f1 100644 --- a/lib/components/fabro-petri/tests/model.rs +++ b/lib/components/fabro-petri/tests/model.rs @@ -67,7 +67,7 @@ async fn a_model_call_authenticates_through_the_vault_and_skills_read_the_home() let graphs = support::admit( &[("workflow.fabro", &workflow), ("workflow.toml", &settings)], Launch { - model: Some(OPENAI_MODEL.to_string()), + default_model: Some(OPENAI_MODEL.to_string()), ..Launch::default() }, &runtime, From fae391c4dcd7202e82c14d94c4341b72c0e26318 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 2 Oct 2026 13:10:52 -0400 Subject: [PATCH 3/3] Bind CLI model flags in one place when building Petri's check The create, validate and preflight paths each wrapped their launch with the run's --model and --provider flags before building the check request, so a new caller could build a check without them. The check request now takes the flags as a required argument and binds them onto the launch itself, and unit tests cover the flags, a provider-only flag, and no flags. Co-Authored-By: Claude Opus 5.5 --- .../fabro-server/src/manifest_validation.rs | 6 +- lib/apps/fabro-server/src/petri_check.rs | 100 ++++++++++++++---- lib/apps/fabro-server/src/run_manifest.rs | 3 +- .../fabro-server/src/server/petri_runs.rs | 2 +- 4 files changed, 87 insertions(+), 24 deletions(-) diff --git a/lib/apps/fabro-server/src/manifest_validation.rs b/lib/apps/fabro-server/src/manifest_validation.rs index 329049b51..acbe81a3d 100644 --- a/lib/apps/fabro-server/src/manifest_validation.rs +++ b/lib/apps/fabro-server/src/manifest_validation.rs @@ -103,10 +103,8 @@ pub fn validate_collected_workflow( &lowered.entrypoint, &settings, &HashMap::new(), - petri_check::with_model_overrides( - petri_check::launch_without_catalog(&settings), - run_overrides.and_then(|run| run.model.as_ref()), - ), + petri_check::launch_without_catalog(&settings), + run_overrides.and_then(|run| run.model.as_ref()), offline_runtime(run_overrides), false, ) diff --git a/lib/apps/fabro-server/src/petri_check.rs b/lib/apps/fabro-server/src/petri_check.rs index a08cd516f..0fee23d46 100644 --- a/lib/apps/fabro-server/src/petri_check.rs +++ b/lib/apps/fabro-server/src/petri_check.rs @@ -93,34 +93,30 @@ pub(crate) fn launch_without_catalog(settings: &WorkflowSettings) -> Launch { } } -/// The run's explicit model flags (`--model`, `--provider`), kept apart from -/// the default the settings resolved, so they outrank the file layers and -/// the graph's defaults during admission. -pub(crate) fn with_model_overrides( - mut launch: Launch, - overrides: Option<&RunModelLayer>, -) -> Launch { - if let Some(overrides) = overrides { - launch.model.clone_from(&overrides.name); - launch.provider.clone_from(&overrides.provider); - } - launch -} - /// The check request for `bundle`'s `entrypoint`: every file of every /// workflow in the bundle at its bundle-relative path, the run's inputs and -/// variables, the launch and the runtime. `unbound_is_warning` makes a -/// template that reads an input nothing binds a warning, for a validation -/// before the run's inputs exist; a run's admission never sets it. +/// variables, the launch and the runtime. `model_overrides` are the run's +/// explicit model flags (`--model`, `--provider`), bound as the launch's +/// model apart from the default the settings resolved, so they outrank the +/// file layers and the graph's defaults during admission. Every check binds +/// them here, so the create, validate and preflight paths judge the same +/// model. `unbound_is_warning` makes a template that reads an input nothing +/// binds a warning, for a validation before the run's inputs exist; a run's +/// admission never sets it. pub(crate) fn check_request( bundle: &WorkflowBundle, entrypoint: &ManifestPath, settings: &WorkflowSettings, vars: &HashMap, - launch: Launch, + mut launch: Launch, + model_overrides: Option<&RunModelLayer>, runtime: RuntimeSpec, unbound_is_warning: bool, ) -> Result { + if let Some(overrides) = model_overrides { + launch.model.clone_from(&overrides.name); + launch.provider.clone_from(&overrides.provider); + } let mut files = BTreeMap::new(); for workflow in bundle.workflows().values() { for (path, text) in &workflow.files { @@ -237,3 +233,71 @@ fn fabro_diagnostic(diagnostic: &Diagnostic) -> FabroDiagnostic { ..FabroDiagnostic::default() } } + +#[cfg(test)] +mod tests { + use super::*; + + fn request(launch: Launch, overrides: Option<&RunModelLayer>) -> CheckRequest { + check_request( + &WorkflowBundle::default(), + &ManifestPath::from_wire("workflow.fabro").expect("the path is valid"), + &WorkflowSettings::default(), + &HashMap::new(), + launch, + overrides, + RuntimeSpec::default(), + false, + ) + .expect("the request builds") + } + + fn settings_default() -> Launch { + Launch { + default_model: Some("settings-model".to_string()), + default_provider: Some("settings-provider".to_string()), + ..Launch::default() + } + } + + #[test] + fn model_flags_bind_as_the_launch_model_beside_the_default() { + let overrides = RunModelLayer { + name: Some("flag-model".to_string()), + provider: Some("flag-provider".to_string()), + ..RunModelLayer::default() + }; + + let launch = request(settings_default(), Some(&overrides)).launch; + + assert_eq!(launch.model.as_deref(), Some("flag-model")); + assert_eq!(launch.provider.as_deref(), Some("flag-provider")); + assert_eq!(launch.default_model.as_deref(), Some("settings-model")); + assert_eq!( + launch.default_provider.as_deref(), + Some("settings-provider") + ); + } + + #[test] + fn a_provider_flag_alone_leaves_the_launch_model_unset() { + let overrides = RunModelLayer { + provider: Some("flag-provider".to_string()), + ..RunModelLayer::default() + }; + + let launch = request(settings_default(), Some(&overrides)).launch; + + assert_eq!(launch.model, None); + assert_eq!(launch.provider.as_deref(), Some("flag-provider")); + } + + #[test] + fn no_model_flags_bind_only_the_default() { + let launch = request(settings_default(), None).launch; + + assert_eq!(launch.model, None); + assert_eq!(launch.provider, None); + assert_eq!(launch.default_model.as_deref(), Some("settings-model")); + } +} diff --git a/lib/apps/fabro-server/src/run_manifest.rs b/lib/apps/fabro-server/src/run_manifest.rs index 33c3ffa2f..665d5e078 100644 --- a/lib/apps/fabro-server/src/run_manifest.rs +++ b/lib/apps/fabro-server/src/run_manifest.rs @@ -211,7 +211,8 @@ pub(crate) fn validate_prepared_manifest( &prepared.target_path, &prepared.settings, vars, - petri_check::with_model_overrides(launch, prepared.model_overrides.as_ref()), + launch, + prepared.model_overrides.as_ref(), runtime, unbound_is_warning, )?; diff --git a/lib/apps/fabro-server/src/server/petri_runs.rs b/lib/apps/fabro-server/src/server/petri_runs.rs index 1a5b99a0c..734a58025 100644 --- a/lib/apps/fabro-server/src/server/petri_runs.rs +++ b/lib/apps/fabro-server/src/server/petri_runs.rs @@ -275,13 +275,13 @@ pub(crate) async fn admit( repository, ); let dry_run = settings.run.execution.mode == RunMode::DryRun; - let launch = petri_check::with_model_overrides(launch, prepared.model_overrides()); let request = petri_check::check_request( prepared.workflow_bundle(), prepared.entrypoint(), settings, prepared.vars(), launch, + prepared.model_overrides(), runtime_spec( state, eligible,