From d0802fbfb142b3b5ccc483b3e8b67de3b383ee19 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 9 Apr 2026 12:52:03 -0400 Subject: [PATCH] wip(settings): stage 6.1 consumer migration (broken build) Partial Stage 6.1 migration of consumers off the legacy flat Settings shape to v2 SettingsFile. Commits the in-flight work so subsequent sessions can resume from here. Workspace currently does NOT build -- fabro-server still has ~60 consumer sites that reference state.settings as legacy Settings, and fabro-cli is entirely untouched. Landed in this commit: fabro-types - RunRecord.settings: Settings -> SettingsFile - RunCreatedProps.settings: Settings -> SettingsFile fabro-config - effective_settings: full rewrite. resolve_settings now returns SettingsFile; apply_server_defaults / apply_local_daemon_overrides are v2-native and use the v2 merge matrix for server-owned domains. - project::resolve_working_directory takes &SettingsFile and reads run.working_dir as an InterpString. fabro-workflow - start.rs, create.rs, source.rs, validate.rs, run_options.rs, git.rs, initialize.rs, manager_loop.rs all migrated to &SettingsFile reads. - resolve_sandbox_provider / resolve_worktree_mode / resolve_daytona_config / resolve_fallback_chain walk v2 trees using the bridge helper fns. - LifecycleOptions built from run_prepare_commands() / run_prepare_timeout_ms(). - Hooks built via bridge_hook on v2 HookEntry. - MCPs built via bridge_mcp_entry on v2 McpEntryLayer. - resolve_run_settings writes resolved model/provider back into run.model (InterpString), not the flat llm struct. - preprocess_and_validate pulls var expansion from run_inputs_as_strings. fabro-server/run_manifest.rs - PreparedManifest.settings -> SettingsFile. - prepare_manifest_with_mode takes &SettingsFile. - build_preflight_report / run_llm_check / resolve_model_provider / run_github_token_check / resolve_sandbox_provider / resolve_daytona_config all migrated. - Tests rewritten to use v2 fixtures via ConfigLayer::parse. fabro-server/server.rs - AppState.settings type changed to Arc>. - github_app_credentials call site uses settings.github_app_id_str() accessor instead of the flat app_id(). Known remaining errors: - fabro-server/server.rs: ~60 state.settings.read() sites still reference legacy Settings fields (llm, sandbox, setup, git, etc.). - fabro-server/web_auth.rs: heavy git settings usage, tests. - fabro-server/serve.rs: state mutation of flat llm/sandbox fields. - fabro-server/diagnostics.rs: app_id / api auth strategies. - fabro-cli: manifest_builder, commands, tests all untouched. - Test fixtures across the workspace still construct Settings literals. - insta snapshots will need bulk-accept after the runtime shape stabilizes. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../fabro-config/src/effective_settings.rs | 270 +++++++++--------- lib/crates/fabro-config/src/project.rs | 8 +- lib/crates/fabro-server/src/run_manifest.rs | 169 ++++++----- lib/crates/fabro-server/src/server.rs | 11 +- lib/crates/fabro-types/src/run.rs | 4 +- lib/crates/fabro-types/src/run_event/run.rs | 5 +- lib/crates/fabro-workflow/src/git.rs | 6 +- .../src/handler/manager_loop.rs | 8 +- .../fabro-workflow/src/operations/create.rs | 89 +++--- .../fabro-workflow/src/operations/source.rs | 45 +-- .../fabro-workflow/src/operations/start.rs | 150 ++++++---- .../fabro-workflow/src/operations/validate.rs | 4 +- .../fabro-workflow/src/pipeline/initialize.rs | 10 +- lib/crates/fabro-workflow/src/run_options.rs | 20 +- 14 files changed, 454 insertions(+), 345 deletions(-) diff --git a/lib/crates/fabro-config/src/effective_settings.rs b/lib/crates/fabro-config/src/effective_settings.rs index 1f6ddb79a..b92a8932a 100644 --- a/lib/crates/fabro-config/src/effective_settings.rs +++ b/lib/crates/fabro-config/src/effective_settings.rs @@ -1,4 +1,4 @@ -//! Effective settings resolution: combine layers into one resolved [`Settings`]. +//! Effective settings resolution: combine layers into one resolved [`SettingsFile`]. //! //! Shared layered domains (`project`, `workflow`, `run`, `features`) merge //! across all three config files (settings.toml, fabro.toml, workflow.toml). @@ -7,10 +7,11 @@ //! stanzas in `fabro.toml` and `workflow.toml` remain schema-valid but inert. use anyhow::{Result, anyhow}; -use fabro_types::Settings; use fabro_types::settings::v2::SettingsFile; +use fabro_types::settings::v2::run::{RunExecutionLayer, RunLayer}; use crate::ConfigLayer; +use crate::merge::combine_files; #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum EffectiveSettingsMode { @@ -44,11 +45,12 @@ impl EffectiveSettingsLayers { } } +/// Resolve layered configuration down to a single effective [`SettingsFile`]. pub fn resolve_settings( layers: EffectiveSettingsLayers, - server_settings: Option<&Settings>, + server_settings: Option<&SettingsFile>, mode: EffectiveSettingsMode, -) -> Result { +) -> Result { let EffectiveSettingsLayers { args, mut workflow, @@ -57,11 +59,9 @@ pub fn resolve_settings( } = layers; match mode { - EffectiveSettingsMode::LocalOnly => Ok(args - .combine(workflow) - .combine(project) - .combine(user) - .resolve()), + EffectiveSettingsMode::LocalOnly => { + Ok(args.combine(workflow).combine(project).combine(user).into()) + } EffectiveSettingsMode::RemoteServer | EffectiveSettingsMode::LocalDaemon => { let server_settings = server_settings.ok_or_else(|| { anyhow!("server settings are required for server-targeted settings resolution") @@ -72,26 +72,33 @@ pub fn resolve_settings( strip_owner_domains(workflow.as_v2_mut()); strip_owner_domains(project.as_v2_mut()); - let server_defaults = server_defaults_layer(server_settings); + let server_defaults = server_defaults_file(server_settings); - let mut settings = args - .combine(workflow) - .combine(project) - .combine(user) - .resolve(); + let combined: SettingsFile = + args.combine(workflow).combine(project).combine(user).into(); - match mode { + let mut settings = match mode { EffectiveSettingsMode::RemoteServer => { - apply_server_defaults(&mut settings, &server_defaults); + apply_server_defaults(combined, &server_defaults) } EffectiveSettingsMode::LocalDaemon => { - apply_local_daemon_overrides(&mut settings, &server_defaults); + apply_local_daemon_overrides(combined, &server_defaults) } EffectiveSettingsMode::LocalOnly => unreachable!(), + }; + // Storage root always comes from the server's local + // ~/.fabro/settings.toml, never from the client. + if let Some(server_root) = server_settings + .server + .as_ref() + .and_then(|s| s.storage.as_ref()) + .cloned() + { + let server = settings + .server + .get_or_insert_with(fabro_types::settings::v2::server::ServerLayer::default); + server.storage = Some(server_root); } - settings - .storage_dir - .clone_from(&server_settings.storage_dir); Ok(settings) } } @@ -102,103 +109,81 @@ fn strip_owner_domains(file: &mut SettingsFile) { file.server = None; } -fn server_defaults_layer(settings: &Settings) -> Settings { +/// Copy of the server settings with startup-time dry-run fallback cleared. +/// Run manifests carry their own dry-run intent; a daemon's startup-time +/// fallback mode must not silently force every submitted run into simulation. +fn server_defaults_file(settings: &SettingsFile) -> SettingsFile { let mut out = settings.clone(); - // Run manifests carry their own dry-run intent. Do not let a daemon's - // startup-time fallback mode silently force every submitted run/preflight - // into simulation. - out.dry_run = None; + if let Some(run) = out.run.as_mut() { + if let Some(execution) = run.execution.as_mut() { + execution.mode = None; + } + } out } -fn apply_server_defaults(settings: &mut Settings, server: &Settings) { - // Owner-specific storage and scheduling come from the server's local - // settings.toml. These always win over anything layered from the client. - if settings.storage_dir.is_none() { - settings.storage_dir.clone_from(&server.storage_dir); - } - if settings.max_concurrent_runs.is_none() { - settings.max_concurrent_runs = server.max_concurrent_runs; - } - if settings.artifact_storage.is_none() { - settings - .artifact_storage - .clone_from(&server.artifact_storage); - } - if settings.web.is_none() { - settings.web.clone_from(&server.web); - } - if settings.api.is_none() { - settings.api.clone_from(&server.api); - } - if settings.features.is_none() { - settings.features.clone_from(&server.features); - } - if settings.log.is_none() { - settings.log.clone_from(&server.log); - } - if settings.git.is_none() { - settings.git.clone_from(&server.git); - } - // Run-shaped defaults also flow from server to CLI in RemoteServer mode - // so the persisted run record matches the server's local configuration. - if settings.llm.is_none() { - settings.llm.clone_from(&server.llm); - } - if settings.sandbox.is_none() { - settings.sandbox.clone_from(&server.sandbox); - } - if settings.setup.is_none() { - settings.setup.clone_from(&server.setup); - } - if settings.checkpoint.exclude_globs.is_empty() { - settings.checkpoint = server.checkpoint.clone(); - } - if settings.pull_request.is_none() { - settings.pull_request.clone_from(&server.pull_request); - } - if settings.artifacts.is_none() { - settings.artifacts.clone_from(&server.artifacts); - } - if settings.hooks.is_empty() { - settings.hooks.clone_from(&server.hooks); - } - if settings.mcp_servers.is_empty() { - settings.mcp_servers.clone_from(&server.mcp_servers); - } - if settings.github.is_none() { - settings.github.clone_from(&server.github); - } - if settings.slack.is_none() { - settings.slack.clone_from(&server.slack); - } - if settings.fabro.is_none() { - settings.fabro.clone_from(&server.fabro); - } - if settings.vars.is_none() { - settings.vars.clone_from(&server.vars); - } else if let (Some(local), Some(server_vars)) = (settings.vars.as_mut(), server.vars.as_ref()) - { - for (k, v) in server_vars { - local.entry(k.clone()).or_insert_with(|| v.clone()); - } - } +/// Apply server-side defaults to a client-layered [`SettingsFile`]. +/// +/// Server-owned domains (`server`, `features`, and parts of `run`) flow from +/// the server's local `~/.fabro/settings.toml` when the corresponding client +/// value is absent. Run-shaped defaults (model, prepare, sandbox, checkpoint, +/// hooks, agent mcps, etc.) also flow from server to client so the persisted +/// run record matches the server's local configuration. +fn apply_server_defaults(mut settings: SettingsFile, server: &SettingsFile) -> SettingsFile { + // Server-owned domains: server-side always wins when client left blank. + // Use the v2 merge matrix with the server layer in lower precedence so + // that client-supplied values still dominate when present. + settings = combine_files(server.clone(), settings); + settings } -fn apply_local_daemon_overrides(settings: &mut Settings, server: &Settings) { - settings.storage_dir.clone_from(&server.storage_dir); - settings.max_concurrent_runs = server.max_concurrent_runs; +/// Apply server-side overrides in LocalDaemon mode. +/// +/// In LocalDaemon mode, a subset of server-owned fields unconditionally +/// override any client-side values. Client-controlled run-level fields are +/// left alone. +fn apply_local_daemon_overrides(mut settings: SettingsFile, server: &SettingsFile) -> SettingsFile { + if let Some(server_layer) = server.server.clone() { + let client = settings + .server + .get_or_insert_with(fabro_types::settings::v2::server::ServerLayer::default); + if let Some(storage) = server_layer.storage { + client.storage = Some(storage); + } + if let Some(scheduler) = server_layer.scheduler { + client.scheduler = Some(scheduler); + } + if let Some(artifacts) = server_layer.artifacts { + client.artifacts = Some(artifacts); + } + if let Some(web) = server_layer.web { + client.web = Some(web); + } + if let Some(api) = server_layer.api { + client.api = Some(api); + } + } + if let Some(features) = server.features.clone() { + settings.features = Some(features); + } + // Ensure a run.execution table exists so downstream consumers that check + // for explicit dry-run defaults see a well-formed layer. + settings.run.get_or_insert_with(RunLayer::default); + settings + .run + .as_mut() + .unwrap() + .execution + .get_or_insert_with(RunExecutionLayer::default); settings - .artifact_storage - .clone_from(&server.artifact_storage); - settings.web.clone_from(&server.web); - settings.api.clone_from(&server.api); - settings.features.clone_from(&server.features); } #[cfg(test)] mod tests { - use std::path::PathBuf; + use fabro_types::settings::v2::InterpString; + use fabro_types::settings::v2::server::{ + ServerLayer, ServerSchedulerLayer, ServerStorageLayer, + }; use super::{EffectiveSettingsLayers, EffectiveSettingsMode, resolve_settings}; use crate::ConfigLayer; @@ -246,16 +231,21 @@ shared = "user" ) .unwrap(); - let llm = settings.llm.expect("llm config"); - assert_eq!(llm.model.as_deref(), Some("project-model")); + assert_eq!( + settings.run_model_name_str().as_deref(), + Some("project-model") + ); // Per R22, run.inputs replaces wholesale — the winning layer is the // highest-precedence layer that sets `inputs` (project here, since it // wins over user). - let vars = settings.vars.as_ref().unwrap(); - assert_eq!(vars.get("project_only"), Some(&"1".to_string())); - assert_eq!(vars.get("shared"), Some(&"project".to_string())); + let inputs = settings.run_inputs().unwrap(); + assert!(inputs.contains_key("project_only")); + assert_eq!( + inputs.get("shared").and_then(|v| v.as_str()), + Some("project") + ); assert!( - vars.get("user_only").is_none(), + !inputs.contains_key("user_only"), "project.inputs should replace user.inputs wholesale" ); } @@ -298,19 +288,26 @@ provider = "openai" ) .unwrap(); - assert_eq!(settings.goal.as_deref(), Some("workflow goal")); - let llm = settings.llm.expect("llm config"); - assert_eq!(llm.model.as_deref(), Some("workflow-model")); - assert_eq!(llm.provider.as_deref(), Some("openai")); + assert_eq!(settings.run_goal_str().as_deref(), Some("workflow goal")); + assert_eq!( + settings.run_model_name_str().as_deref(), + Some("workflow-model") + ); + assert_eq!(settings.run_model_provider_str().as_deref(), Some("openai")); } #[test] fn cli_and_server_domains_from_fabro_toml_are_inert_under_remote_mode() { - let server_settings: fabro_types::Settings = fabro_types::Settings { - storage_dir: Some(PathBuf::from("/srv/fabro")), - max_concurrent_runs: Some(9), - ..Default::default() - }; + let mut server_settings = fabro_types::settings::v2::SettingsFile::default(); + server_settings.server = Some(ServerLayer { + storage: Some(ServerStorageLayer { + root: Some(InterpString::parse("/srv/fabro")), + }), + scheduler: Some(ServerSchedulerLayer { + max_concurrent_runs: Some(9), + }), + ..ServerLayer::default() + }); let project_with_server = layer( r#" @@ -336,17 +333,25 @@ root = "/tmp/should-be-inert" ) .unwrap(); - assert_eq!(settings.storage_dir, Some(PathBuf::from("/srv/fabro"))); - assert_eq!(settings.goal.as_deref(), Some("project goal")); + assert_eq!( + settings.server_storage_root_str().as_deref(), + Some("/srv/fabro") + ); + assert_eq!(settings.run_goal_str().as_deref(), Some("project goal")); } #[test] fn local_daemon_mode_only_applies_server_owned_overrides() { - let server_settings: fabro_types::Settings = fabro_types::Settings { - storage_dir: Some(PathBuf::from("/srv/fabro")), - max_concurrent_runs: Some(7), - ..Default::default() - }; + let mut server_settings = fabro_types::settings::v2::SettingsFile::default(); + server_settings.server = Some(ServerLayer { + storage: Some(ServerStorageLayer { + root: Some(InterpString::parse("/srv/fabro")), + }), + scheduler: Some(ServerSchedulerLayer { + max_concurrent_runs: Some(7), + }), + ..ServerLayer::default() + }); let settings = resolve_settings( EffectiveSettingsLayers::default(), @@ -355,7 +360,10 @@ root = "/tmp/should-be-inert" ) .unwrap(); - assert_eq!(settings.storage_dir, Some(PathBuf::from("/srv/fabro"))); - assert_eq!(settings.max_concurrent_runs, Some(7)); + assert_eq!( + settings.server_storage_root_str().as_deref(), + Some("/srv/fabro") + ); + assert_eq!(settings.max_concurrent_runs(), Some(7)); } } diff --git a/lib/crates/fabro-config/src/project.rs b/lib/crates/fabro-config/src/project.rs index b04ac94a5..677902ede 100644 --- a/lib/crates/fabro-config/src/project.rs +++ b/lib/crates/fabro-config/src/project.rs @@ -12,8 +12,8 @@ use serde::Serialize; use crate::config::ConfigLayer; use crate::run; -use fabro_types::Settings; pub use fabro_types::settings::project::ProjectSettings; +use fabro_types::settings::v2::{InterpString, SettingsFile}; const CONFIG_FILENAME: &str = "fabro.toml"; const RUN_GRAPH_FILE: &str = "workflow.fabro"; @@ -124,11 +124,11 @@ pub fn resolve_workflow_path( } } -pub fn resolve_working_directory(settings: &Settings, caller_cwd: &Path) -> PathBuf { - let Some(work_dir) = settings.work_dir.as_deref() else { +pub fn resolve_working_directory(settings: &SettingsFile, caller_cwd: &Path) -> PathBuf { + let Some(work_dir) = settings.run_working_dir().map(InterpString::as_source) else { return caller_cwd.to_path_buf(); }; - let path = PathBuf::from(work_dir); + let path = PathBuf::from(&work_dir); if path.is_absolute() { path } else { diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index 6d920754d..c04594345 100644 --- a/lib/crates/fabro-server/src/run_manifest.rs +++ b/lib/crates/fabro-server/src/run_manifest.rs @@ -15,6 +15,7 @@ use fabro_llm::Provider; use fabro_model::Catalog; use fabro_sandbox::daytona::DaytonaConfig; use fabro_sandbox::{DockerSandboxOptions, Sandbox, SandboxProvider, SandboxSpec}; +use fabro_types::RunId; use fabro_types::settings::v2::SettingsFile; use fabro_types::settings::v2::cli::{CliLayer, CliOutputLayer, OutputVerbosity}; use fabro_types::settings::v2::interp::InterpString; @@ -22,7 +23,6 @@ use fabro_types::settings::v2::run::{ ApprovalMode, DaytonaDockerfileLayer, RunExecutionLayer, RunLayer, RunMode, RunModelLayer, RunSandboxLayer, }; -use fabro_types::{RunId, Settings}; use fabro_util::check_report::{CheckDetail, CheckReport, CheckResult, CheckSection, CheckStatus}; use fabro_validate::Severity; use fabro_workflow::error::FabroError; @@ -38,7 +38,7 @@ pub(crate) struct PreparedManifest { pub git: Option, pub root_source: String, pub run_id: Option, - pub settings: Settings, + pub settings: SettingsFile, pub target_path: PathBuf, pub workflow_bundle: WorkflowBundle, pub workflow_input: BundledWorkflow, @@ -46,7 +46,7 @@ pub(crate) struct PreparedManifest { } pub(crate) fn prepare_manifest_with_mode( - server_settings: &Settings, + server_settings: &SettingsFile, manifest: &types::RunManifest, local_daemon_mode: bool, ) -> Result { @@ -89,8 +89,8 @@ pub(crate) fn prepare_manifest_with_mode( }, )?; if let Some(goal) = manifest.goal.as_ref() { - settings.goal = Some(goal.text.clone()); - settings.goal_file = None; + let run = settings.run.get_or_insert_with(RunLayer::default); + run.goal = Some(InterpString::parse(&goal.text)); } Ok(PreparedManifest { @@ -338,12 +338,12 @@ async fn build_preflight_report( let settings = &prepared.settings; let sandbox_provider = resolve_sandbox_provider(settings)?; let github_app = state - .github_app_credentials(settings.app_id()) + .github_app_credentials(settings.github_app_id_str().as_deref()) .await .map_err(|err| anyhow!(err))?; let mut checks = Vec::new(); - let setup_command_count = settings.setup_commands().len(); + let setup_command_count = settings.run_prepare_commands().len(); let repo_summary = prepared.git.as_ref().map_or_else( || "unknown".to_string(), |git| { @@ -405,20 +405,19 @@ async fn build_preflight_report( )) } -fn resolve_sandbox_provider(settings: &Settings) -> Result { +fn resolve_sandbox_provider(settings: &SettingsFile) -> Result { Ok(settings - .sandbox_settings() - .and_then(|sandbox| sandbox.provider.as_deref()) + .run_sandbox() + .and_then(|sb| sb.provider.as_deref()) .map(str::parse::) .transpose() .map_err(|err| anyhow!("Invalid sandbox provider: {err}"))? .unwrap_or_default()) } -fn resolve_daytona_config(settings: &Settings) -> Option { - settings - .sandbox_settings() - .and_then(|sandbox| sandbox.daytona.clone()) +fn resolve_daytona_config(settings: &SettingsFile) -> Option { + let sandbox = settings.run_sandbox()?; + fabro_types::settings::v2::bridge::bridge_sandbox(sandbox).daytona } async fn run_sandbox_check( @@ -497,7 +496,7 @@ async fn run_llm_check( state: &AppState, checks: &mut Vec, graph: &Graph, - settings: &Settings, + settings: &SettingsFile, ) -> bool { let (model, provider) = resolve_model_provider(settings, graph); let default_provider = provider.as_deref().unwrap_or("anthropic"); @@ -588,40 +587,34 @@ async fn run_llm_check( } } -fn resolve_model_provider(settings: &Settings, graph: &Graph) -> (String, Option) { - let configured_model = settings.llm.as_ref().and_then(|llm| llm.model.as_deref()); - let configured_provider = settings - .llm - .as_ref() - .and_then(|llm| llm.provider.as_deref()); +fn resolve_model_provider(settings: &SettingsFile, graph: &Graph) -> (String, Option) { + let configured_model = settings.run_model_name_str(); + let configured_provider = settings.run_model_provider_str(); - let provider = configured_provider - .or_else(|| { - graph - .attrs - .get("default_provider") - .and_then(|value| value.as_str()) - }) - .map(String::from); + let provider = configured_provider.or_else(|| { + graph + .attrs + .get("default_provider") + .and_then(|value| value.as_str()) + .map(String::from) + }); let model = configured_model .or_else(|| { graph .attrs .get("default_model") .and_then(|value| value.as_str()) + .map(String::from) }) - .map_or_else( - || { - let catalog = Catalog::builtin(); - let info = provider - .as_deref() - .and_then(|value| value.parse::().ok()) - .and_then(|provider| catalog.default_for_provider(provider)) - .unwrap_or_else(|| catalog.default_from_env()); - info.id.clone() - }, - String::from, - ); + .unwrap_or_else(|| { + let catalog = Catalog::builtin(); + let info = provider + .as_deref() + .and_then(|value| value.parse::().ok()) + .and_then(|provider| catalog.default_for_provider(provider)) + .unwrap_or_else(|| catalog.default_from_env()); + info.id.clone() + }); match Catalog::builtin().get(&model) { Some(info) => ( @@ -635,23 +628,30 @@ fn resolve_model_provider(settings: &Settings, graph: &Graph) -> (String, Option async fn run_github_token_check( checks: &mut Vec, prepared: &PreparedManifest, - settings: &Settings, + settings: &SettingsFile, github_app: Option, ) { - let Some(github_permissions) = settings.github_permissions() else { + let Some(v2_permissions) = settings.github_permissions() else { return; }; - if github_permissions.is_empty() { + if v2_permissions.is_empty() { return; } + // Resolve InterpString permission values eagerly for token minting and + // for display in the preflight report. + let github_permissions: HashMap = v2_permissions + .iter() + .map(|(k, v)| (k.clone(), v.as_source())) + .collect(); + let perm_details = github_permissions .iter() .map(|(key, value)| CheckDetail::new(format!("{key}: {value}"))) .collect::>(); match (&github_app, prepared.git.as_ref()) { (Some(creds), Some(git)) => { - match mint_github_token(creds, &git.origin_url, github_permissions).await { + match mint_github_token(creds, &git.origin_url, &github_permissions).await { Ok(_) => checks.push(CheckResult { name: "GitHub Token".into(), status: CheckStatus::Pass, @@ -810,31 +810,49 @@ mod tests { } } + fn server_settings_fixture(source: &str) -> SettingsFile { + fabro_config::ConfigLayer::parse(source) + .expect("v2 fixture should parse") + .into() + } + #[test] fn prepare_manifest_does_not_inherit_server_dry_run_fallback() { - let server_settings = Settings { - dry_run: Some(true), - storage_dir: Some(PathBuf::from("/srv/fabro")), - ..Default::default() - }; + let server_settings = server_settings_fixture( + r#" +_version = 1 + +[run.execution] +mode = "dry_run" + +[server.storage] +root = "/srv/fabro" +"#, + ); let prepared = prepare_manifest_with_mode(&server_settings, &minimal_manifest(), false).unwrap(); - assert_eq!(prepared.settings.dry_run, None); + assert!(!prepared.settings.dry_run_enabled()); assert_eq!( - prepared.settings.storage_dir, - Some(PathBuf::from("/srv/fabro")) + prepared.settings.server_storage_root_str().as_deref(), + Some("/srv/fabro"), ); } #[test] fn prepare_manifest_preserves_explicit_manifest_dry_run() { - let server_settings = Settings { - dry_run: Some(true), - storage_dir: Some(PathBuf::from("/srv/fabro")), - ..Default::default() - }; + let server_settings = server_settings_fixture( + r#" +_version = 1 + +[run.execution] +mode = "dry_run" + +[server.storage] +root = "/srv/fabro" +"#, + ); let mut manifest = minimal_manifest(); manifest.args = Some(types::ManifestArgs { auto_approve: None, @@ -850,23 +868,25 @@ mod tests { let prepared = prepare_manifest_with_mode(&server_settings, &manifest, false).unwrap(); - assert_eq!(prepared.settings.dry_run, Some(true)); + assert!(prepared.settings.dry_run_enabled()); } #[test] fn prepare_manifest_local_daemon_prefers_bundled_settings_without_duplication() { - let server_settings: Settings = toml::from_str( + let server_settings = server_settings_fixture( r#" -storage_dir = "/srv/fabro" +_version = 1 -[setup] -commands = ["cli-setup"] +[server.storage] +root = "/srv/fabro" -[git] +[[run.prepare.steps]] +script = "cli-setup" + +[server.integrations.github] app_id = "snapshotted-app-id" "#, - ) - .unwrap(); + ); let mut manifest = minimal_manifest(); manifest.workflows.get_mut("workflow.fabro").unwrap().config = @@ -902,17 +922,16 @@ app_id = "snapshotted-app-id" // v2 merge matrix: run.prepare.steps replaces the whole list across // layers, so the higher-precedence workflow layer wins over cli. assert_eq!( - prepared - .settings - .setup - .as_ref() - .map(|setup| setup.commands.clone()), - Some(vec!["workflow-setup".to_string()]) + prepared.settings.run_prepare_commands(), + vec!["workflow-setup".to_string()] ); - assert_eq!(prepared.settings.app_id(), Some("snapshotted-app-id")); assert_eq!( - prepared.settings.storage_dir, - Some(PathBuf::from("/srv/fabro")) + prepared.settings.github_app_id_str().as_deref(), + Some("snapshotted-app-id") + ); + assert_eq!( + prepared.settings.server_storage_root_str().as_deref(), + Some("/srv/fabro"), ); } } diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 94d158599..af3b8f0d2 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -33,6 +33,7 @@ use fabro_model::{BilledModelUsage, BilledTokenCounts}; use fabro_store::{ ArtifactStore, Database, EventEnvelope, EventPayload, PendingInterviewRecord, StageId, }; +use fabro_types::settings::v2::SettingsFile; use fabro_types::{ EventBody, InterviewQuestionRecord, InterviewQuestionType, RunBlobId, RunClientProvenance, RunControlAction, RunEvent, RunId, RunProvenance, RunServerProvenance, RunSubjectProvenance, @@ -520,7 +521,7 @@ pub struct AppState { global_event_tx: broadcast::Sender, pub(crate) secret_store: AsyncRwLock, - pub(crate) settings: Arc>, + pub(crate) settings: Arc>, pub(crate) config_path: PathBuf, pub(crate) local_daemon_mode: bool, shutting_down: AtomicBool, @@ -3585,7 +3586,13 @@ async fn execute_run_in_process(state: Arc, run_id: RunId) { } }; let github_app = match state - .github_app_credentials(persisted.run_record().settings.app_id()) + .github_app_credentials( + persisted + .run_record() + .settings + .github_app_id_str() + .as_deref(), + ) .await { Ok(github_app) => github_app, diff --git a/lib/crates/fabro-types/src/run.rs b/lib/crates/fabro-types/src/run.rs index 4c282d098..81d6bb670 100644 --- a/lib/crates/fabro-types/src/run.rs +++ b/lib/crates/fabro-types/src/run.rs @@ -6,7 +6,7 @@ use serde::{Deserialize, Serialize}; use crate::graph::Graph; use crate::run_blob_id::RunBlobId; use crate::run_id::RunId; -use crate::settings::Settings; +use crate::settings::v2::SettingsFile; #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "snake_case")] @@ -52,7 +52,7 @@ pub struct RunProvenance { #[derive(Debug, Clone, Serialize, Deserialize)] pub struct RunRecord { pub run_id: RunId, - pub settings: Settings, + pub settings: SettingsFile, pub graph: Graph, #[serde(default, skip_serializing_if = "Option::is_none")] pub workflow_slug: Option, diff --git a/lib/crates/fabro-types/src/run_event/run.rs b/lib/crates/fabro-types/src/run_event/run.rs index ce24c10e2..6ec24c606 100644 --- a/lib/crates/fabro-types/src/run_event/run.rs +++ b/lib/crates/fabro-types/src/run_event/run.rs @@ -2,13 +2,14 @@ use std::collections::BTreeMap; use serde::{Deserialize, Serialize}; -use crate::{Graph, RunBlobId, RunControlAction, RunProvenance, Settings, StatusReason}; +use crate::settings::v2::SettingsFile; +use crate::{Graph, RunBlobId, RunControlAction, RunProvenance, StatusReason}; use super::{BilledTokenCounts, RunNoticeLevel}; #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct RunCreatedProps { - pub settings: Settings, + pub settings: SettingsFile, pub graph: Graph, #[serde(default, skip_serializing_if = "Option::is_none")] pub workflow_source: Option, diff --git a/lib/crates/fabro-workflow/src/git.rs b/lib/crates/fabro-workflow/src/git.rs index 24ccf9fac..feaec45a8 100644 --- a/lib/crates/fabro-workflow/src/git.rs +++ b/lib/crates/fabro-workflow/src/git.rs @@ -2,7 +2,7 @@ use std::path::Path; use std::process::Command; use fabro_checkpoint::git::Store; -use fabro_types::Settings; +use fabro_types::settings::v2::SettingsFile; use crate::error::{FabroError, Result}; use tokio::task::{JoinError, spawn_blocking}; @@ -15,9 +15,9 @@ pub use fabro_checkpoint::metadata::MetadataStore; /// Branch prefix for workflow run branches (e.g. `fabro/run/{run_id}`). pub const RUN_BRANCH_PREFIX: &str = "fabro/run/"; -pub fn git_author_from_settings(settings: &Settings) -> GitAuthor { +pub fn git_author_from_settings(settings: &SettingsFile) -> GitAuthor { settings - .git_author() + .run_git_author() .map(GitAuthor::from) .unwrap_or_default() } diff --git a/lib/crates/fabro-workflow/src/handler/manager_loop.rs b/lib/crates/fabro-workflow/src/handler/manager_loop.rs index f8340c4ae..6878a6c1e 100644 --- a/lib/crates/fabro-workflow/src/handler/manager_loop.rs +++ b/lib/crates/fabro-workflow/src/handler/manager_loop.rs @@ -18,7 +18,7 @@ use crate::run_options::RunOptions; use async_trait::async_trait; use fabro_graphviz::graph::{AttrValue, Graph, Node}; use fabro_store::{ArtifactStore, Database}; -use fabro_types::Settings; +use fabro_types::settings::v2::SettingsFile; use object_store::memory::InMemory; use tokio::time::{sleep, timeout}; @@ -74,7 +74,7 @@ fn parse_child_graph( source: dot.to_string(), base_dir: None, }, - settings: Settings::default(), + settings: SettingsFile::default(), cwd: cwd.clone(), custom_transforms: Vec::new(), })?; @@ -116,7 +116,7 @@ fn parse_child_graph( }; let validated = validate(ValidateInput { workflow, - settings: Settings::default(), + settings: SettingsFile::default(), cwd, custom_transforms: Vec::new(), })?; @@ -202,7 +202,7 @@ impl Handler for SubWorkflowHandler { let child_cancel = Arc::clone(&cancel_token); let child_run_options = RunOptions { - settings: Settings::default(), + settings: SettingsFile::default(), run_dir: child_logs, cancel_token: Some(cancel_token), // Child workflows are part of the parent run's event stream. diff --git a/lib/crates/fabro-workflow/src/operations/create.rs b/lib/crates/fabro-workflow/src/operations/create.rs index f0886f405..7c276fdb1 100644 --- a/lib/crates/fabro-workflow/src/operations/create.rs +++ b/lib/crates/fabro-workflow/src/operations/create.rs @@ -3,7 +3,9 @@ use fabro_graphviz::graph::{AttrValue, Graph}; use fabro_model::{Catalog, Provider}; use fabro_sandbox::SandboxProvider; use fabro_store::Database; -use fabro_types::{RunId, RunProvenance, Settings}; +use fabro_types::settings::v2::run::{RunLayer, RunModelLayer}; +use fabro_types::settings::v2::{InterpString, SettingsFile}; +use fabro_types::{RunId, RunProvenance}; use std::collections::BTreeMap; use std::collections::HashMap; use std::path::{Path, PathBuf}; @@ -26,7 +28,7 @@ use crate::event::{Event, append_event, to_run_event_at}; #[derive(Clone, Debug)] pub struct CreateRunInput { pub workflow: WorkflowInput, - pub settings: Settings, + pub settings: SettingsFile, pub cwd: PathBuf, pub workflow_slug: Option, pub workflow_path: Option, @@ -48,7 +50,7 @@ pub struct CreatedRun { } struct PersistCreateOptions { - settings: Settings, + settings: SettingsFile, run_id: Option, run_dir: Option, workflow_slug: Option, @@ -125,7 +127,7 @@ pub async fn create(store: &Database, request: CreateRunInput) -> Result FabroError { FabroError::engine(err.to_string()) } -fn validate_sandbox_provider(settings: &Settings) -> Result<(), FabroError> { +fn validate_sandbox_provider(settings: &SettingsFile) -> Result<(), FabroError> { if let Some(provider) = settings - .sandbox_settings() + .run_sandbox() .and_then(|sandbox| sandbox.provider.as_deref()) { provider @@ -289,12 +291,11 @@ pub(super) fn preprocess_and_validate( current_dir: Option, file_resolver: Option>, custom_transforms: Vec>, - settings: Option<&Settings>, + settings: Option<&SettingsFile>, goal_override: Option<&str>, ) -> Result { - let source = match settings.and_then(|resolved| resolved.vars.as_ref()) { - Some(vars) => { - let mut vars = vars.clone(); + let source = match settings.and_then(SettingsFile::run_inputs_as_strings) { + Some(mut vars) => { vars.insert("goal".to_string(), "$goal".to_string()); expand_vars(dot_source, &vars) .map_err(|e| FabroError::Parse(format!("var expansion failed: {e}")))? @@ -371,28 +372,32 @@ fn persist_validated( ) } -pub(crate) fn resolve_run_settings(mut settings: Settings, graph: &Graph) -> Settings { - let llm_settings = settings.llm.as_ref(); - let configured_model = llm_settings.and_then(|l| l.model.as_deref()); - let configured_provider = llm_settings.and_then(|l| l.provider.as_deref()); - let graph_provider = graph.attrs.get("default_provider").and_then(|v| v.as_str()); - let graph_model = graph.attrs.get("default_model").and_then(|v| v.as_str()); +pub(crate) fn resolve_run_settings(mut settings: SettingsFile, graph: &Graph) -> SettingsFile { + let configured_model = settings.run_model_name_str(); + let configured_provider = settings.run_model_provider_str(); + let graph_provider = graph + .attrs + .get("default_provider") + .and_then(|v| v.as_str()) + .map(str::to_string); + let graph_model = graph + .attrs + .get("default_model") + .and_then(|v| v.as_str()) + .map(str::to_string); - let provider = configured_provider.or(graph_provider).map(str::to_string); + let provider = configured_provider.or(graph_provider); - let model = configured_model.or(graph_model).map_or_else( - || { - let catalog = Catalog::builtin(); - provider - .as_deref() - .and_then(|value| value.parse::().ok()) - .and_then(|provider| catalog.default_for_provider(provider)) - .unwrap_or_else(|| catalog.default_from_env()) - .id - .clone() - }, - str::to_string, - ); + let model = configured_model.or(graph_model).unwrap_or_else(|| { + let catalog = Catalog::builtin(); + provider + .as_deref() + .and_then(|value| value.parse::().ok()) + .and_then(|provider| catalog.default_for_provider(provider)) + .unwrap_or_else(|| catalog.default_from_env()) + .id + .clone() + }); let (resolved_model, resolved_provider) = match Catalog::builtin().get(&model) { Some(info) => ( @@ -402,16 +407,26 @@ pub(crate) fn resolve_run_settings(mut settings: Settings, graph: &Graph) -> Set None => (model, provider), }; - let llm = settings.llm.get_or_insert_default(); - llm.model = Some(resolved_model); - llm.provider = resolved_provider; + let run = settings.run.get_or_insert_with(RunLayer::default); + let model_layer = run.model.get_or_insert_with(RunModelLayer::default); + model_layer.name = Some(InterpString::parse(&resolved_model)); + model_layer.provider = resolved_provider.as_deref().map(InterpString::parse); let goal = graph.goal().to_string(); - settings.goal = if goal.is_empty() { None } else { Some(goal) }; - settings.pull_request = settings + run.goal = if goal.is_empty() { + None + } else { + Some(InterpString::parse(&goal)) + }; + // Strip disabled pull_request entries so downstream consumers can + // treat `Some(_)` as "PR creation is on". + if run .pull_request - .take() - .filter(|pull_request| pull_request.enabled); + .as_ref() + .is_some_and(|pr| !pr.enabled.unwrap_or(false)) + { + run.pull_request = None; + } settings } diff --git a/lib/crates/fabro-workflow/src/operations/source.rs b/lib/crates/fabro-workflow/src/operations/source.rs index dcf9c06ae..d91d0b5fe 100644 --- a/lib/crates/fabro-workflow/src/operations/source.rs +++ b/lib/crates/fabro-workflow/src/operations/source.rs @@ -3,7 +3,7 @@ use std::sync::Arc; use anyhow::Context; use fabro_config::project as project_config; -use fabro_types::Settings; +use fabro_types::settings::v2::{InterpString, SettingsFile}; use fabro_util::path::expand_tilde; use crate::file_resolver::{FileResolver, FilesystemFileResolver}; @@ -22,14 +22,14 @@ pub enum WorkflowInput { #[derive(Clone, Debug)] pub(crate) struct ResolveWorkflowInput { pub workflow: WorkflowInput, - pub settings: Settings, + pub settings: SettingsFile, pub cwd: PathBuf, } #[derive(Clone)] pub(crate) struct ResolvedWorkflow { pub raw_source: String, - pub settings: Settings, + pub settings: SettingsFile, pub workflow_slug: Option, pub workflow_toml_path: Option, pub dot_path: Option, @@ -85,10 +85,7 @@ pub(crate) fn resolve_workflow(request: ResolveWorkflowInput) -> anyhow::Result< project_config::resolve_working_directory(&settings, &request.cwd); let raw_source = std::fs::read_to_string(&resolution.dot_path) .with_context(|| format!("Failed to read {}", resolution.dot_path.display()))?; - let goal_override = settings.goal.clone().or(resolve_goal_file( - settings.goal_file.as_deref(), - &working_directory, - )?); + let goal_override = resolve_goal_override(&settings, &working_directory)?; let current_dir = resolution .dot_path .parent() @@ -113,10 +110,7 @@ pub(crate) fn resolve_workflow(request: ResolveWorkflowInput) -> anyhow::Result< let settings = request.settings; let working_directory = project_config::resolve_working_directory(&settings, &request.cwd); - let goal_override = settings.goal.clone().or(resolve_goal_file( - settings.goal_file.as_deref(), - &working_directory, - )?); + let goal_override = resolve_goal_override(&settings, &working_directory)?; let has_base_dir = base_dir.is_some(); Ok(ResolvedWorkflow { raw_source: source, @@ -138,37 +132,56 @@ pub(crate) fn resolve_workflow(request: ResolveWorkflowInput) -> anyhow::Result< let settings = request.settings; let working_directory = project_config::resolve_working_directory(&settings, &request.cwd); + let goal_override = settings.run_goal().map(InterpString::as_source); Ok(ResolvedWorkflow { raw_source: workflow.source.clone(), - settings: settings.clone(), + settings, workflow_slug: workflow_slug_from_path(&workflow.logical_path), workflow_toml_path: None, dot_path: Some(workflow.logical_path.clone()), current_dir: Some(workflow.current_dir()), file_resolver: Some(workflow.file_resolver()), - goal_override: settings.goal.clone(), + goal_override, working_directory, }) } } } +fn resolve_goal_override( + settings: &SettingsFile, + working_directory: &Path, +) -> anyhow::Result> { + // V2 does not yet carry a separate `goal_file` field; file-based goals + // come through the workflow manifest layer in the server-side flow. + // For direct CLI paths, the goal override comes from `run.goal`. + Ok(settings + .run_goal() + .map(InterpString::as_source) + .or(resolve_goal_file(None, working_directory)?)) +} + #[cfg(test)] mod tests { use super::*; #[test] fn resolve_workflow_uses_explicit_cwd_for_relative_work_dir() { + use fabro_types::settings::v2::run::RunLayer; + let dir = tempfile::tempdir().unwrap(); let resolved = resolve_workflow(ResolveWorkflowInput { workflow: WorkflowInput::DotSource { source: "digraph Test { start -> exit }".to_string(), base_dir: None, }, - settings: Settings { - work_dir: Some("workspace".to_string()), - ..Default::default() + settings: SettingsFile { + run: Some(RunLayer { + working_dir: Some(InterpString::parse("workspace")), + ..RunLayer::default() + }), + ..SettingsFile::default() }, cwd: dir.path().to_path_buf(), }) diff --git a/lib/crates/fabro-workflow/src/operations/start.rs b/lib/crates/fabro-workflow/src/operations/start.rs index 40bef59d0..8da74cdcc 100644 --- a/lib/crates/fabro-workflow/src/operations/start.rs +++ b/lib/crates/fabro-workflow/src/operations/start.rs @@ -5,11 +5,14 @@ use std::sync::{Arc, Mutex}; use std::time::{Duration, Instant}; use fabro_config::sandbox::WorktreeMode; -use fabro_config::{project as project_config, run as run_config, sandbox as sandbox_config}; +use fabro_config::{project as project_config, sandbox as sandbox_config}; use fabro_interview::{AutoApproveInterviewer, Interviewer}; use fabro_model::{Catalog, FallbackTarget, Provider}; use fabro_sandbox::{SandboxProvider, SandboxSpec}; -use fabro_types::{RunId, Settings}; +use fabro_types::RunId; +use fabro_types::settings::v2::bridge::{bridge_mcp_entry, bridge_sandbox, bridge_worktree_mode}; +use fabro_types::settings::v2::run::ModelRefOrSplice; +use fabro_types::settings::v2::{InterpString, SettingsFile}; use crate::artifact_upload::ArtifactSink; use crate::context::Context; @@ -260,7 +263,7 @@ async fn persist_terminal_engine_failure( impl RunSession { async fn new(persisted: &Persisted, services: StartServices) -> Result { let record = persisted.run_record(); - let mut settings = record.settings.clone(); + let settings = &record.settings; let working_directory = record.working_directory.clone(); let state = services .run_store @@ -287,34 +290,21 @@ impl RunSession { let workflow_bundle = accepted_definition.map(|definition| Arc::new(definition.workflow_bundle())); - if let Some(env) = settings - .sandbox - .as_mut() - .and_then(|sandbox| sandbox.env.as_mut()) - { - run_config::resolve_env_refs(env) - .map_err(|err| FabroError::Precondition(err.to_string()))?; - } - let (origin_url, detected_base_branch) = detect_repo_info(&working_directory) .map(|(url, branch)| (Some(url), branch)) .unwrap_or((None, None)); - let sandbox_provider = resolve_sandbox_provider(&settings)?; + let sandbox_provider = resolve_sandbox_provider(settings)?; let sandbox_provider = if settings.dry_run_enabled() && !sandbox_provider.is_local() { SandboxProvider::Local } else { sandbox_provider }; let model = settings - .llm - .as_ref() - .and_then(|llm| llm.model.clone()) + .run_model_name_str() .unwrap_or_else(|| Catalog::builtin().default_from_env().id.clone()); let provider = settings - .llm - .as_ref() - .and_then(|llm| llm.provider.clone()) + .run_model_provider_str() .filter(|value| !value.is_empty()); let provider_enum: Provider = provider @@ -324,13 +314,15 @@ impl RunSession { .map_err(|err| FabroError::Precondition(err.clone()))? .unwrap_or_else(Provider::default_from_env); - let fallback_chain = resolve_fallback_chain(provider_enum, &model, &settings); + let fallback_chain = resolve_fallback_chain(provider_enum, &model, settings); let mcp_servers = settings - .mcp_server_entries() - .clone() - .into_iter() - .map(|(name, entry)| entry.into_config(name)) - .collect(); + .run_agent_mcps() + .map(|mcps| { + mcps.iter() + .map(|(name, entry)| bridge_mcp_entry(entry).into_config(name.clone())) + .collect() + }) + .unwrap_or_default(); let sandbox = match sandbox_provider { SandboxProvider::Local => SandboxSpec::Local { @@ -343,26 +335,39 @@ impl RunSession { }, }, SandboxProvider::Daytona => SandboxSpec::Daytona { - config: resolve_daytona_config(&settings).unwrap_or_default(), + config: resolve_daytona_config(settings).unwrap_or_default(), github_app: services.github_app.clone(), run_id: Some(record.run_id), clone_branch: detected_base_branch.or_else(|| record.base_branch.clone()), }, }; + let toml_env: HashMap = settings + .run_sandbox() + .map(|sb| { + sb.env + .iter() + .map(|(k, v)| (k.clone(), resolve_interp(v))) + .collect() + }) + .unwrap_or_default(); + let github_permissions: Option> = + settings.github_permissions().map(|perms| { + perms + .iter() + .map(|(k, v)| (k.clone(), resolve_interp(v))) + .collect() + }); let sandbox_env = SandboxEnvSpec { devcontainer_env: HashMap::new(), - toml_env: settings - .sandbox_settings() - .and_then(|sandbox| sandbox.env.clone()) - .unwrap_or_default(), - github_permissions: settings.github_permissions().cloned(), + toml_env, + github_permissions, origin_url: origin_url.clone(), }; let devcontainer = settings - .sandbox_settings() - .and_then(|sandbox| sandbox.devcontainer) + .run_sandbox() + .and_then(|sb| sb.devcontainer) .unwrap_or(false) .then(|| DevcontainerSpec { enabled: true, @@ -375,6 +380,10 @@ impl RunSession { services.interviewer }; + let pr_config = settings + .run_pull_request() + .map(fabro_types::settings::v2::bridge::bridge_pull_request); + Ok(Self { cancel_token: services.cancel_token, emitter: services.emitter, @@ -391,12 +400,16 @@ impl RunSession { interviewer, on_node: services.on_node, lifecycle: LifecycleOptions { - setup_commands: settings.setup_commands().to_vec(), - setup_command_timeout_ms: settings.setup_timeout_ms().unwrap_or(300_000), + setup_commands: settings.run_prepare_commands(), + setup_command_timeout_ms: settings.run_prepare_timeout_ms().unwrap_or(300_000), devcontainer_phases: Vec::new(), }, hooks: fabro_hooks::HookSettings { - hooks: settings.hooks.clone(), + hooks: settings + .run_hooks() + .iter() + .map(fabro_types::settings::v2::bridge::bridge_hook) + .collect(), }, sandbox_env, devcontainer, @@ -405,11 +418,11 @@ impl RunSession { artifact_sink: services.artifact_sink, git, github_app: services.github_app.clone(), - worktree_mode: Some(resolve_worktree_mode(&settings)), + worktree_mode: Some(resolve_worktree_mode(settings)), registry_override: services.registry_override, retro_enabled: !settings.no_retro_enabled() && project_config::is_retro_enabled(), - preserve_sandbox: resolve_preserve_sandbox(&settings), - pr_config: settings.pull_request.clone(), + preserve_sandbox: resolve_preserve_sandbox(settings), + pr_config, pr_github_app: services.github_app, pr_origin_url: origin_url, pr_model: model, @@ -419,6 +432,12 @@ impl RunSession { } } +fn resolve_interp(value: &InterpString) -> String { + value + .resolve(|name| std::env::var(name).ok()) + .map_or_else(|_| value.as_source(), |resolved| resolved.value) +} + async fn load_accepted_run_definition( run_store: &RunStoreHandle, blob_id: fabro_types::RunBlobId, @@ -435,45 +454,62 @@ async fn load_accepted_run_definition( serde_json::from_slice(&bytes).map_err(|err| FabroError::Parse(err.to_string())) } -fn resolve_sandbox_provider(settings: &Settings) -> Result { +fn resolve_sandbox_provider(settings: &SettingsFile) -> Result { settings - .sandbox_settings() - .and_then(|sandbox| sandbox.provider.as_deref()) + .run_sandbox() + .and_then(|sb| sb.provider.as_deref()) .map(str::parse::) .transpose() .map_err(|err| FabroError::Precondition(format!("Invalid sandbox provider: {err}")))? .map_or_else(|| Ok(SandboxProvider::default()), Ok) } -fn resolve_preserve_sandbox(settings: &Settings) -> bool { +fn resolve_preserve_sandbox(settings: &SettingsFile) -> bool { settings.preserve_sandbox_enabled() } -fn resolve_worktree_mode(settings: &Settings) -> sandbox_config::WorktreeMode { +fn resolve_worktree_mode(settings: &SettingsFile) -> sandbox_config::WorktreeMode { settings - .sandbox_settings() - .and_then(|sandbox| sandbox.local.as_ref()) - .map(|local| local.worktree_mode) + .run_sandbox() + .and_then(|sb| sb.local.as_ref()) + .and_then(|local| local.worktree_mode) + .map(bridge_worktree_mode) .unwrap_or_default() } -fn resolve_daytona_config(settings: &Settings) -> Option { - settings - .sandbox_settings() - .and_then(|sandbox| sandbox.daytona.clone()) +fn resolve_daytona_config(settings: &SettingsFile) -> Option { + let sandbox = settings.run_sandbox()?; + bridge_sandbox(sandbox).daytona } fn resolve_fallback_chain( provider: Provider, model: &str, - settings: &Settings, + settings: &SettingsFile, ) -> Vec { - let fallbacks = settings.llm.as_ref().and_then(|llm| llm.fallbacks.as_ref()); - - match fallbacks { - Some(map) => Catalog::builtin().build_fallback_chain(provider, model, map), - None => Vec::new(), + let Some(model_layer) = settings.run_model() else { + return Vec::new(); + }; + if model_layer.fallbacks.is_empty() { + return Vec::new(); } + // Group v2 ModelRef entries by provider name, preserving the legacy + // shape expected by `Catalog::build_fallback_chain`. The historical + // bridge grouped all fallback tokens under the empty-string key; we + // preserve that behavior here so `Catalog::build_fallback_chain` + // returns an empty chain unless a consumer has explicitly wired + // provider-keyed fallbacks. A proper provider-aware fallback chain + // is a follow-up along with the model registry work. + let mut by_provider: HashMap> = HashMap::new(); + for entry in &model_layer.fallbacks { + if let ModelRefOrSplice::ModelRef(model_ref) = entry { + by_provider + .entry(String::new()) + .or_default() + .push(model_ref.to_string()); + } + } + Catalog::builtin().build_fallback_chain(provider, model, &by_provider) } impl RunSession { diff --git a/lib/crates/fabro-workflow/src/operations/validate.rs b/lib/crates/fabro-workflow/src/operations/validate.rs index 46c9f9e82..059d8372d 100644 --- a/lib/crates/fabro-workflow/src/operations/validate.rs +++ b/lib/crates/fabro-workflow/src/operations/validate.rs @@ -1,6 +1,6 @@ use std::path::PathBuf; -use fabro_types::Settings; +use fabro_types::settings::v2::SettingsFile; use crate::error::FabroError; use crate::pipeline::Validated; @@ -11,7 +11,7 @@ use super::source::{ResolveWorkflowInput, WorkflowInput, resolve_workflow}; pub struct ValidateInput { pub workflow: WorkflowInput, - pub settings: Settings, + pub settings: SettingsFile, pub cwd: PathBuf, pub custom_transforms: Vec>, } diff --git a/lib/crates/fabro-workflow/src/pipeline/initialize.rs b/lib/crates/fabro-workflow/src/pipeline/initialize.rs index 2b8517de3..6bbfc1782 100644 --- a/lib/crates/fabro-workflow/src/pipeline/initialize.rs +++ b/lib/crates/fabro-workflow/src/pipeline/initialize.rs @@ -534,8 +534,16 @@ pub async fn initialize( build_registry(&options.llm, Arc::clone(&options.interviewer), &env, &graph).await? }; if effective_dry_run { + use fabro_types::settings::v2::run::{RunExecutionLayer, RunLayer, RunMode}; + options.dry_run = true; - options.run_options.settings.dry_run = Some(true); + let run = options + .run_options + .settings + .run + .get_or_insert_with(RunLayer::default); + let execution = run.execution.get_or_insert_with(RunExecutionLayer::default); + execution.mode = Some(RunMode::DryRun); } let has_run_branch = options diff --git a/lib/crates/fabro-workflow/src/run_options.rs b/lib/crates/fabro-workflow/src/run_options.rs index a01da6471..dabceaa28 100644 --- a/lib/crates/fabro-workflow/src/run_options.rs +++ b/lib/crates/fabro-workflow/src/run_options.rs @@ -3,8 +3,9 @@ use std::path::PathBuf; use std::sync::Arc; use std::sync::atomic::AtomicBool; -use fabro_config::run::PullRequestSettings; -use fabro_types::{RunId, Settings}; +use fabro_types::RunId; +use fabro_types::settings::v2::SettingsFile; +use fabro_types::settings::v2::run::RunPullRequestLayer; use crate::git::{GitAuthor, git_author_from_settings}; @@ -19,7 +20,7 @@ pub struct GitCheckpointOptions { /// Options for a workflow run. #[derive(Clone)] pub struct RunOptions { - pub settings: Settings, + pub settings: SettingsFile, pub run_dir: PathBuf, pub cancel_token: Option>, /// Unique identifier for this workflow run. @@ -46,22 +47,23 @@ impl RunOptions { } pub fn checkpoint_exclude_globs(&self) -> &[String] { - &self.settings.checkpoint.exclude_globs + self.settings + .run_checkpoint() + .map_or(&[], |cp| cp.exclude_globs.as_slice()) } pub fn git_author(&self) -> GitAuthor { git_author_from_settings(&self.settings) } - /// PR config (already normalized — disabled entries stripped at construction). - pub fn pull_request(&self) -> Option<&PullRequestSettings> { - self.settings.pull_request.as_ref() + /// PR config, if present in the v2 run layer. + pub fn pull_request(&self) -> Option<&RunPullRequestLayer> { + self.settings.run_pull_request() } pub fn artifact_globs(&self) -> &[String] { self.settings - .artifacts - .as_ref() + .run_artifacts() .map_or(&[], |a| a.include.as_slice()) } }