Merge pull request #919 from fabro-sh/codex/cli-model-overrides

Apply CLI model and provider overrides to agent execution

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;

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",
+    );
+}

lib/apps/fabro-server/src/manifest_validation.rs

@@ -104,6 +104,7 @@ pub fn validate_collected_workflow(
         &settings,
         &HashMap::new(),
         petri_check::launch_without_catalog(&settings),
+        run_overrides.and_then(|run| run.model.as_ref()),
         offline_runtime(run_overrides),
         false,
     )

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};
@@ -27,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,
@@ -49,11 +51,12 @@ 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,
+        ..Launch::default()
     }
 }
 
@@ -81,28 +84,39 @@ fn launch_goal(settings: &WorkflowSettings) -> Option<String> {
 /// 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,
+        goal: launch_goal(settings),
+        repository: None,
+        ..Launch::default()
     }
 }
 
 /// 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<String, String>,
-    launch: Launch,
+    mut launch: Launch,
+    model_overrides: Option<&RunModelLayer>,
     runtime: RuntimeSpec,
     unbound_is_warning: bool,
 ) -> Result<CheckRequest, WorkflowError> {
+    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 {
@@ -219,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"));
+    }
+}

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<String>,
+    model_overrides:     Option<RunModelLayer>,
     storage_root:        PathBuf,
     workflow_slug:       Option<String>,
     workflow_version_id: Option<WorkflowVersionId>,
@@ -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<NormalizedR
         .as_ref()
         .and_then(|run| run.environment.as_ref())
         .and_then(|environment| environment.id.clone());
+    let model_overrides = run_overrides.as_ref().and_then(|run| run.model.clone());
 
     Ok(NormalizedRun {
         workflow_bundle,
@@ -332,6 +338,7 @@ pub(crate) fn normalize_source(input: RawRunCompilerInput) -> Result<NormalizedR
         metadata: RunMetadata {
             run_id,
             environment_id,
+            model_overrides,
             storage_root,
             workflow_slug,
             workflow_version_id,
@@ -460,6 +467,7 @@ pub(crate) fn assemble_run(pinned: PinnedRun) -> 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,

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<RunModelLayer>,
     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,
@@ -206,6 +212,7 @@ pub(crate) fn validate_prepared_manifest(
         &prepared.settings,
         vars,
         launch,
+        prepared.model_overrides.as_ref(),
         runtime,
         unbound_is_warning,
     )?;

lib/apps/fabro-server/src/server/petri_runs.rs

@@ -281,6 +281,7 @@ pub(crate) async fn admit(
         settings,
         prepared.vars(),
         launch,
+        prepared.model_overrides(),
         runtime_spec(
             state,
             eligible,

lib/components/fabro-petri/src/check.rs

@@ -11,8 +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.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
@@ -26,8 +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, MapFiles, 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};
@@ -65,25 +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<String>,
-    pub provider:    Option<String>,
+    /// 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<String>,
+    pub provider:         Option<String>,
+    /// 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<String>,
+    pub default_provider: Option<String>,
     /// 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<String>,
+    pub environment:      Option<String>,
     /// 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<String>,
+    pub goal:             Option<String>,
     /// The local repository the root `start` stage checks out into the
     /// workspace; `None` starts the run from an empty workspace.
-    pub repository:  Option<PathBuf>,
+    pub repository:       Option<PathBuf>,
 }
 
 /// One check: the bundle, the run's inputs and variables, the launch and
@@ -219,6 +231,12 @@ fn compile_inputs(
     compile
         .vars
         .insert(LAUNCH_PROVIDER_VAR.into(), text(&launch.provider));
+    compile
+        .vars
+        .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(),

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,
+            default_model: Some("gpt-5.4".to_string()),
+            default_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,
@@ -153,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()

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,