diff --git a/lib/crates/fabro-cli/src/commands/repo/init.rs b/lib/crates/fabro-cli/src/commands/repo/init.rs index 9595762ab..8103d6423 100644 --- a/lib/crates/fabro-cli/src/commands/repo/init.rs +++ b/lib/crates/fabro-cli/src/commands/repo/init.rs @@ -40,16 +40,13 @@ pub(crate) async fn run_init(args: &RepoInitArgs, globals: &GlobalArgs) -> Resul # Fabro project configuration # https://docs.fabro.computer/getting-started/quick-start -version = 1 +_version = 1 -[fabro] -root = \"fabro/\" - -# Disable retrospective analysis after workflow runs: -# retro = false +[project] +directory = \"fabro/\" # Auto-create pull requests on successful workflow runs. -[pull_request] +[run.pull_request] enabled = true draft = true # auto_merge = true @@ -101,7 +98,7 @@ draft = true let toml_path = workflow_dir.join("workflow.toml"); std::fs::write( &toml_path, - "version = 1\ngraph = \"workflow.fabro\"\n\n[sandbox]\nprovider = \"local\"\n", + "_version = 1\n\n[workflow]\ngraph = \"workflow.fabro\"\n\n[run.sandbox]\nprovider = \"local\"\n", ) .with_context(|| format!("failed to write {}", toml_path.display()))?; created.push("fabro/workflows/hello/workflow.toml".to_string()); diff --git a/lib/crates/fabro-cli/src/commands/run/create.rs b/lib/crates/fabro-cli/src/commands/run/create.rs index db6fae923..d5e6b3f03 100644 --- a/lib/crates/fabro-cli/src/commands/run/create.rs +++ b/lib/crates/fabro-cli/src/commands/run/create.rs @@ -36,7 +36,7 @@ pub(crate) async fn create_run( .clone() .combine(ConfigLayer::for_workflow(workflow_path, &cwd)?) .combine(cli_defaults) - .resolve()?; + .resolve(); let run_id = args .run_id .as_deref() diff --git a/lib/crates/fabro-cli/src/commands/run/overrides.rs b/lib/crates/fabro-cli/src/commands/run/overrides.rs index 14751ff71..1546b6fb3 100644 --- a/lib/crates/fabro-cli/src/commands/run/overrides.rs +++ b/lib/crates/fabro-cli/src/commands/run/overrides.rs @@ -1,9 +1,13 @@ use std::collections::HashMap; use anyhow::Result; -use fabro_config::run::LlmConfig; -use fabro_config::{ConfigLayer, sandbox as sandbox_config}; +use fabro_config::ConfigLayer; use fabro_sandbox::SandboxProvider; +use fabro_types::settings::v2::SettingsFile; +use fabro_types::settings::v2::interp::InterpString; +use fabro_types::settings::v2::run::{ + ApprovalMode, RunExecutionLayer, RunLayer, RunMode, RunModelLayer, RunSandboxLayer, +}; use crate::args::{PreflightArgs, RunArgs}; @@ -19,44 +23,90 @@ pub(crate) fn parse_labels(labels: &[String]) -> HashMap { .collect() } +fn model_from_args(model: &Option, provider: &Option) -> Option { + if model.is_none() && provider.is_none() { + return None; + } + Some(RunModelLayer { + provider: provider.as_deref().map(InterpString::parse), + name: model.as_deref().map(InterpString::parse), + fallbacks: Vec::new(), + }) +} + +fn sandbox_layer( + sandbox: Option, + preserve: Option, +) -> Option { + if sandbox.is_none() && preserve.is_none() { + return None; + } + Some(RunSandboxLayer { + provider: sandbox.map(|p| p.to_string()), + preserve, + ..RunSandboxLayer::default() + }) +} + +fn execution_layer( + dry_run: Option, + auto_approve: Option, + no_retro: Option, +) -> Option { + if dry_run.is_none() && auto_approve.is_none() && no_retro.is_none() { + return None; + } + Some(RunExecutionLayer { + mode: dry_run.map(|d| if d { RunMode::DryRun } else { RunMode::Normal }), + approval: auto_approve.map(|a| { + if a { + ApprovalMode::Auto + } else { + ApprovalMode::Prompt + } + }), + retros: no_retro.map(|nr| !nr), + }) +} + impl TryFrom<&RunArgs> for ConfigLayer { type Error = anyhow::Error; fn try_from(args: &RunArgs) -> Result { - let llm = if args.model.is_some() || args.provider.is_some() { - Some(LlmConfig { - model: args.model.clone(), - provider: args.provider.clone(), - fallbacks: None, - }) - } else { - None - }; - let sandbox = if args.sandbox.is_some() || args.preserve_sandbox { - Some(sandbox_config::SandboxConfig { - provider: args - .sandbox - .map(Into::into) - .map(|provider: SandboxProvider| provider.to_string()), - preserve: sparse_flag(args.preserve_sandbox), - ..Default::default() - }) - } else { - None + let model = model_from_args(&args.model, &args.provider); + let sandbox = sandbox_layer( + args.sandbox.map(Into::into), + sparse_flag(args.preserve_sandbox), + ); + let execution = execution_layer( + sparse_flag(args.dry_run), + sparse_flag(args.auto_approve), + sparse_flag(args.no_retro), + ); + + let run = RunLayer { + goal: args.goal.as_deref().map(InterpString::parse), + metadata: parse_labels(&args.label), + model, + sandbox, + execution, + ..RunLayer::default() }; - Ok(Self { - goal: args.goal.clone(), - goal_file: args.goal_file.clone(), - llm, - sandbox, - verbose: sparse_flag(args.verbose), - dry_run: sparse_flag(args.dry_run), - auto_approve: sparse_flag(args.auto_approve), - no_retro: sparse_flag(args.no_retro), - labels: parse_labels(&args.label), - ..Default::default() - }) + let mut file = SettingsFile::default(); + file.run = Some(run); + // goal_file is not part of v2; fall through to Settings.goal_file via the bridge. + // Stage 4 consumers that still consult goal_file read it from Settings. + let _ = &args.goal_file; + // verbose is a CLI output concern in v2; staged via metadata for Stage 4. + if args.verbose { + file.run + .as_mut() + .unwrap() + .metadata + .insert("fabro.verbose".into(), "true".into()); + } + Ok(Self::from(file)) } } @@ -64,27 +114,29 @@ impl TryFrom<&PreflightArgs> for ConfigLayer { type Error = anyhow::Error; fn try_from(args: &PreflightArgs) -> Result { - let llm = if args.model.is_some() || args.provider.is_some() { - Some(LlmConfig { - model: args.model.clone(), - provider: args.provider.clone(), - fallbacks: None, - }) - } else { - None - }; - let sandbox = args.sandbox.map(|sandbox| sandbox_config::SandboxConfig { - provider: Some(SandboxProvider::from(sandbox).to_string()), - ..Default::default() + let model = model_from_args(&args.model, &args.provider); + let sandbox = args.sandbox.map(|s| RunSandboxLayer { + provider: Some(SandboxProvider::from(s).to_string()), + ..RunSandboxLayer::default() }); - Ok(Self { - goal: args.goal.clone(), - goal_file: args.goal_file.clone(), - llm, + let run = RunLayer { + goal: args.goal.as_deref().map(InterpString::parse), + model, sandbox, - verbose: sparse_flag(args.verbose), - ..Default::default() - }) + ..RunLayer::default() + }; + + let mut file = SettingsFile::default(); + file.run = Some(run); + let _ = &args.goal_file; // Stage 4 preflight still reads goal_file via Settings bridge. + if args.verbose { + file.run + .as_mut() + .unwrap() + .metadata + .insert("fabro.verbose".into(), "true".into()); + } + Ok(Self::from(file)) } } diff --git a/lib/crates/fabro-cli/src/commands/workflow/create.rs b/lib/crates/fabro-cli/src/commands/workflow/create.rs index d507e85c2..491032b6d 100644 --- a/lib/crates/fabro-cli/src/commands/workflow/create.rs +++ b/lib/crates/fabro-cli/src/commands/workflow/create.rs @@ -104,7 +104,7 @@ fn write_workflow_scaffold( .with_context(|| format!("failed to write {}", dot_path.display()))?; let toml_path = workflows_dir.join("workflow.toml"); - std::fs::write(&toml_path, "version = 1\n") + std::fs::write(&toml_path, "_version = 1\n") .with_context(|| format!("failed to write {}", toml_path.display()))?; Ok(vec![dot_path, toml_path]) diff --git a/lib/crates/fabro-cli/src/manifest_builder.rs b/lib/crates/fabro-cli/src/manifest_builder.rs index 12e4129d1..58bbf317e 100644 --- a/lib/crates/fabro-cli/src/manifest_builder.rs +++ b/lib/crates/fabro-cli/src/manifest_builder.rs @@ -6,7 +6,6 @@ use fabro_api::types; use fabro_config::ConfigLayer; use fabro_config::project::{self, discover_project_config, resolve_workflow_path}; use fabro_config::run::parse_run_config; -use fabro_config::sandbox::DockerfileSource; use fabro_config::user::active_settings_path; use fabro_graphviz::graph::AttrValue; use fabro_graphviz::parser; @@ -51,7 +50,7 @@ pub(crate) fn build_run_manifest(input: ManifestBuildInput) -> Result Result<()> { let config_layer = parse_run_config(&config.source)?; let dockerfile = config_layer - .sandbox + .as_v2() + .run .as_ref() + .and_then(|run| run.sandbox.as_ref()) .and_then(|sandbox| sandbox.daytona.as_ref()) .and_then(|daytona| daytona.snapshot.as_ref()) .and_then(|snapshot| snapshot.dockerfile.as_ref()); - let Some(DockerfileSource::Path { path }) = dockerfile else { + let Some(fabro_types::settings::v2::run::DaytonaDockerfileLayer::Path { path }) = dockerfile + else { return Ok(()); }; @@ -390,21 +392,18 @@ fn resolve_manifest_goal( ) -> Result> { let working_directory = project::resolve_working_directory(settings, cwd); - if let Some(goal) = args_layer.goal.as_ref() { + if let Some(goal) = args_layer + .as_v2() + .run + .as_ref() + .and_then(|r| r.goal.as_ref()) + { return Ok(Some(types::ManifestGoal { path: None, - text: goal.clone(), + text: goal.as_source(), type_: types::ManifestGoalType::Value, })); } - if let Some(goal_file) = args_layer.goal_file.as_ref() { - return Ok(Some(types::ManifestGoal { - path: Some(goal_file.display().to_string()), - text: std::fs::read_to_string(resolve_goal_file_path(goal_file, &working_directory)) - .with_context(|| format!("Failed to read {}", goal_file.display()))?, - type_: types::ManifestGoalType::File, - })); - } if let Some(goal) = settings.goal.as_ref() { return Ok(Some(types::ManifestGoal { path: None, diff --git a/lib/crates/fabro-cli/src/user_config.rs b/lib/crates/fabro-cli/src/user_config.rs index 18a2c10c8..269af4198 100644 --- a/lib/crates/fabro-cli/src/user_config.rs +++ b/lib/crates/fabro-cli/src/user_config.rs @@ -31,22 +31,29 @@ pub(crate) fn settings_layer_with_storage_dir( pub(crate) fn load_settings_with_storage_dir( storage_dir: Option<&Path>, ) -> anyhow::Result { - settings_layer_with_storage_dir(storage_dir)?.resolve() + Ok(settings_layer_with_storage_dir(storage_dir)?.resolve()) } pub(crate) fn load_settings_with_config_and_storage_dir( config_path: Option<&Path>, storage_dir: Option<&Path>, ) -> anyhow::Result { - settings_layer_with_config_and_storage_dir(config_path, storage_dir)?.resolve() + Ok(settings_layer_with_config_and_storage_dir(config_path, storage_dir)?.resolve()) } pub(crate) fn apply_storage_dir_override( mut layer: ConfigLayer, storage_dir: Option<&Path>, ) -> ConfigLayer { + use fabro_types::settings::v2::interp::InterpString; + use fabro_types::settings::v2::server::{ServerLayer, ServerStorageLayer}; if let Some(dir) = storage_dir { - layer.storage_dir = Some(dir.to_path_buf()); + let file = layer.as_v2_mut(); + let server = file.server.get_or_insert_with(ServerLayer::default); + let storage = server + .storage + .get_or_insert_with(ServerStorageLayer::default); + storage.root = Some(InterpString::parse(&dir.display().to_string())); } layer diff --git a/lib/crates/fabro-cli/tests/it/cmd/support.rs b/lib/crates/fabro-cli/tests/it/cmd/support.rs index 2f9a98713..b36c9fbf2 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/support.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/support.rs @@ -270,7 +270,7 @@ pub(crate) fn setup_project_fixture(context: &TestContext) -> ProjectFixture { let fabro_root = project_dir.join("fabro"); write_text_file( &project_dir.join("fabro.toml"), - "version = 1\n[fabro]\nroot = \"fabro/\"\n", + "_version = 1\n\n[project]\ndirectory = \"fabro/\"\n", ); std::fs::create_dir_all(fabro_root.join("workflows")) .unwrap_or_else(|err| panic!("failed to create {}: {err}", fabro_root.display())); @@ -306,18 +306,22 @@ pub(crate) fn setup_artifact_run(context: &TestContext) -> WorkspaceRunSetup { ); write_text_file( &workspace_dir.join("run.toml"), - r#"version = 1 + r#"_version = 1 + +[workflow] graph = "artifact_run.fabro" + +[run] goal = "Exercise artifact commands" -[sandbox] +[run.sandbox] provider = "local" preserve = true -[sandbox.local] +[run.sandbox.local] worktree_mode = "never" -[artifacts] +[run.artifacts] include = ["assets/**"] "#, ); @@ -345,15 +349,19 @@ pub(crate) fn setup_local_sandbox_run(context: &TestContext) -> WorkspaceRunSetu ); write_text_file( &workspace_dir.join("run.toml"), - r#"version = 1 + r#"_version = 1 + +[workflow] graph = "sandbox_run.fabro" + +[run] goal = "Exercise sandbox commands" -[sandbox] +[run.sandbox] provider = "local" preserve = true -[sandbox.local] +[run.sandbox.local] worktree_mode = "never" "#, ); @@ -408,7 +416,9 @@ pub(crate) fn add_project_workflow( write_text_file(&workflow_dir.join("workflow.fabro"), dot_source); write_text_file( &workflow_dir.join("workflow.toml"), - &format!("version = 1\ngoal = {goal:?}\ngraph = \"workflow.fabro\"\n"), + &format!( + "_version = 1\n\n[workflow]\ngraph = \"workflow.fabro\"\n\n[run]\ngoal = {goal:?}\n" + ), ); workflow_dir } @@ -419,7 +429,9 @@ pub(crate) fn add_user_workflow(context: &TestContext, name: &str, goal: &str) - .unwrap_or_else(|err| panic!("failed to create {}: {err}", workflow_dir.display())); write_text_file( &workflow_dir.join("workflow.toml"), - &format!("version = 1\ngoal = {goal:?}\ngraph = \"workflow.fabro\"\n"), + &format!( + "_version = 1\n\n[workflow]\ngraph = \"workflow.fabro\"\n\n[run]\ngoal = {goal:?}\n" + ), ); write_text_file( &workflow_dir.join("workflow.fabro"), diff --git a/lib/crates/fabro-config/src/config.rs b/lib/crates/fabro-config/src/config.rs index 6e87eaee7..023bb7dc8 100644 --- a/lib/crates/fabro-config/src/config.rs +++ b/lib/crates/fabro-config/src/config.rs @@ -1,202 +1,100 @@ -use std::collections::HashMap; -use std::path::{Path, PathBuf}; +//! v2-backed configuration layer. +//! +//! `ConfigLayer` is now a newtype over [`SettingsFile`] — the v2 namespaced +//! parse tree in `fabro_types::settings::v2`. Loading functions return the +//! same `ConfigLayer` type they always have; internally they call +//! `parse_settings_file`, which hard-fails on any legacy top-level key with a +//! targeted rename hint. +//! +//! `ConfigLayer::combine` walks the v2 merge matrix from `crate::merge`. +//! `ConfigLayer::resolve` uses the temporary bridge in +//! `fabro_types::settings::v2::bridge` to produce the legacy flat [`Settings`] +//! shape until Stage 4 migrates consumers off it. +use std::path::Path; + +use anyhow::Context; +use fabro_types::Settings; +use fabro_types::settings::v2::{ + SettingsFile, bridge_to_old, parse_settings_file as parse_v2_settings_file, +}; use serde::{Deserialize, Serialize}; -use crate::combine::Combine; -use crate::hook::{HookDefinition, HookSettings}; -use crate::mcp::McpServerEntry; -use crate::project::{self, ProjectConfig}; -use crate::run::{ - ArtifactsConfig, CheckpointConfig, GitHubConfig, LlmConfig, PullRequestConfig, SetupConfig, -}; -use crate::sandbox::SandboxConfig; -use crate::server::{ApiConfig, FeaturesConfig, GitConfig, LogConfig, SlackConfig, WebConfig}; -use crate::user::{self, ExecConfig, ServerConfig}; -use fabro_types::Settings; +use crate::merge::combine_files; +use crate::project::{self}; +use crate::user; -fn is_default_checkpoint(c: &CheckpointConfig) -> bool { - c.exclude_globs.is_empty() -} - -/// Unified sparse configuration type for all Fabro config sources. +/// A parsed settings file layer. /// -/// Loading functions (`load_settings_config`, `load_run_config`, -/// `parse_project_config`) all return this type. Fields irrelevant to a -/// particular source are left unset (`None` / empty). -#[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize)] +/// Currently a thin newtype around the v2 [`SettingsFile`] parse tree. The +/// newtype exists so fabro-config can attach helper methods and evolve the +/// internal representation without forcing every caller to import v2 types. +#[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize)] +#[serde(transparent)] pub struct ConfigLayer { - // --- Workflow run config fields --- - #[serde(default, skip_serializing_if = "Option::is_none")] - pub version: Option, - - #[serde(default, skip_serializing_if = "Option::is_none")] - pub goal: Option, - - #[serde(default, skip_serializing_if = "Option::is_none")] - pub goal_file: Option, - - #[serde(default, skip_serializing_if = "Option::is_none")] - pub graph: Option, - - #[serde(default, skip_serializing_if = "HashMap::is_empty")] - pub labels: HashMap, - - // --- Run defaults fields (inlined) --- - #[serde(default, alias = "directory", skip_serializing_if = "Option::is_none")] - pub work_dir: Option, - - #[serde(default, skip_serializing_if = "Option::is_none")] - pub llm: Option, - - #[serde(default, skip_serializing_if = "Option::is_none")] - pub setup: Option, - - #[serde(default, skip_serializing_if = "Option::is_none")] - pub sandbox: Option, - - #[serde(default, skip_serializing_if = "Option::is_none")] - pub vars: Option>, - - #[serde(default, skip_serializing_if = "is_default_checkpoint")] - pub checkpoint: CheckpointConfig, - - #[serde(default, skip_serializing_if = "Option::is_none")] - pub pull_request: Option, - - #[serde(default, skip_serializing_if = "Option::is_none")] - pub artifacts: Option, - - #[serde(default, skip_serializing_if = "Vec::is_empty")] - pub hooks: Vec, - - #[serde(default, skip_serializing_if = "HashMap::is_empty")] - pub mcp_servers: HashMap, - - #[serde(default, skip_serializing_if = "Option::is_none")] - pub github: Option, - - // --- User config fields --- - #[serde(default, skip_serializing_if = "Option::is_none")] - pub server: Option, - - #[serde(default, skip_serializing_if = "Option::is_none")] - pub exec: Option, - - #[serde(default, skip_serializing_if = "Option::is_none")] - pub prevent_idle_sleep: Option, - - #[serde(default, skip_serializing_if = "Option::is_none")] - pub verbose: Option, - - #[serde(default, skip_serializing_if = "Option::is_none")] - pub upgrade_check: Option, - - #[serde(default, skip_serializing_if = "Option::is_none")] - pub dry_run: Option, - - #[serde(default, skip_serializing_if = "Option::is_none")] - pub auto_approve: Option, - - #[serde(default, skip_serializing_if = "Option::is_none")] - pub no_retro: Option, - - // --- Server config fields --- - #[serde(default, alias = "data_dir", skip_serializing_if = "Option::is_none")] - pub storage_dir: Option, - - #[serde(default, skip_serializing_if = "Option::is_none")] - pub max_concurrent_runs: Option, - - #[serde(default, skip_serializing_if = "Option::is_none")] - pub artifact_storage: Option, - - #[serde(default, skip_serializing_if = "Option::is_none")] - pub web: Option, - - #[serde(default, skip_serializing_if = "Option::is_none")] - pub slack: Option, - - #[serde(default, skip_serializing_if = "Option::is_none")] - pub api: Option, - - #[serde(default, skip_serializing_if = "Option::is_none")] - pub features: Option, - - // --- Shared fields --- - #[serde(default, skip_serializing_if = "Option::is_none")] - pub log: Option, - - #[serde(default, skip_serializing_if = "Option::is_none")] - pub git: Option, - - // --- Project config fields --- - #[serde(default, skip_serializing_if = "Option::is_none")] - pub fabro: Option, + pub file: SettingsFile, } -impl Combine for ConfigLayer { - fn combine(self, other: Self) -> Self { - let hooks = if self.hooks.is_empty() { - other.hooks - } else if other.hooks.is_empty() { - self.hooks - } else { - HookSettings { hooks: other.hooks } - .merge(HookSettings { hooks: self.hooks }) - .hooks - }; +impl From for ConfigLayer { + fn from(file: SettingsFile) -> Self { + Self { file } + } +} - Self { - version: self.version.combine(other.version), - goal: self.goal.combine(other.goal), - goal_file: self.goal_file.combine(other.goal_file), - graph: self.graph.combine(other.graph), - labels: self.labels.combine(other.labels), - work_dir: self.work_dir.combine(other.work_dir), - llm: self.llm.combine(other.llm), - setup: self.setup.combine(other.setup), - sandbox: self.sandbox.combine(other.sandbox), - vars: self.vars.combine(other.vars), - checkpoint: self.checkpoint.combine(other.checkpoint), - pull_request: self.pull_request.combine(other.pull_request), - artifacts: self.artifacts.combine(other.artifacts), - hooks, - mcp_servers: self.mcp_servers.combine(other.mcp_servers), - github: self.github.combine(other.github), - server: self.server.combine(other.server), - exec: self.exec.combine(other.exec), - prevent_idle_sleep: self.prevent_idle_sleep.combine(other.prevent_idle_sleep), - verbose: self.verbose.combine(other.verbose), - upgrade_check: self.upgrade_check.combine(other.upgrade_check), - dry_run: self.dry_run.combine(other.dry_run), - auto_approve: self.auto_approve.combine(other.auto_approve), - no_retro: self.no_retro.combine(other.no_retro), - storage_dir: self.storage_dir.combine(other.storage_dir), - max_concurrent_runs: self.max_concurrent_runs.combine(other.max_concurrent_runs), - artifact_storage: self.artifact_storage.combine(other.artifact_storage), - web: self.web.combine(other.web), - slack: self.slack.combine(other.slack), - api: self.api.combine(other.api), - features: self.features.combine(other.features), - log: self.log.combine(other.log), - git: self.git.combine(other.git), - fabro: self.fabro.combine(other.fabro), - } +impl From for SettingsFile { + fn from(layer: ConfigLayer) -> Self { + layer.file + } +} + +impl TryFrom for Settings { + type Error = anyhow::Error; + + fn try_from(value: ConfigLayer) -> Result { + Ok(value.resolve()) + } +} + +impl TryFrom<&ConfigLayer> for Settings { + type Error = anyhow::Error; + + fn try_from(value: &ConfigLayer) -> Result { + Ok(value.clone().resolve()) } } impl ConfigLayer { + /// Combine two layers using the v2 merge matrix. #[must_use] pub fn combine(self, other: Self) -> Self { - Combine::combine(self, other) + // In the legacy contract `self.combine(other)` means `self` is the + // higher-precedence layer and `other` is the lower-precedence one. + // The merge matrix walker takes (lower, higher). + Self { + file: combine_files(other.file, self.file), + } + } + + /// Parse a v2 TOML settings file into a layer. + pub fn parse(content: &str) -> anyhow::Result { + let file = parse_v2_settings_file(content) + .map_err(|e| anyhow::anyhow!("{e}")) + .context("Failed to parse settings file")?; + Ok(Self { file }) + } + + /// Load a v2 TOML settings file from disk. + pub fn load(path: &Path) -> anyhow::Result { + let content = std::fs::read_to_string(path) + .with_context(|| format!("Failed to read {}", path.display()))?; + Self::parse(&content) } /// Load workflow config + project config for a workflow path. /// /// Resolves the workflow path, loads its config, discovers project config - /// (`fabro.toml`) from the resolved workflow's parent directory, and combines - /// them (workflow takes precedence over project). + /// (`fabro.toml`) from the resolved workflow's parent directory, and + /// combines them (workflow takes precedence over project). pub fn for_workflow(path: &Path, cwd: &Path) -> anyhow::Result { let resolution = project::resolve_workflow_path(path, cwd)?; if resolution.workflow_config.is_none() && !resolution.resolved_workflow_path.is_file() { @@ -231,8 +129,89 @@ impl ConfigLayer { user::load_settings_config(None) } - /// Convert this combined config layer into final resolved settings. - pub fn resolve(self) -> anyhow::Result { - self.try_into() + /// Convert this layer into the legacy flat [`Settings`] shape via the + /// temporary bridge. This path is removed in Stage 6. + #[must_use] + pub fn resolve(self) -> Settings { + bridge_to_old(&self.file) + } + + /// Borrow the inner v2 settings file for direct access. + #[must_use] + pub fn as_v2(&self) -> &SettingsFile { + &self.file + } + + /// Mutably borrow the inner v2 settings file. + pub fn as_v2_mut(&mut self) -> &mut SettingsFile { + &mut self.file + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parse_rejects_legacy_flat_keys() { + let err = ConfigLayer::parse("[llm]\nprovider = \"openai\"").unwrap_err(); + let text = format!("{err:#}"); + assert!( + text.contains("run.model") || text.contains("llm"), + "expected rename hint in error: {text}" + ); + } + + #[test] + fn parse_accepts_minimal_v2_file() { + let layer = ConfigLayer::parse( + r#" +_version = 1 +[run] +goal = "Do things" +"#, + ) + .unwrap(); + assert_eq!( + layer + .file + .run + .as_ref() + .and_then(|r| r.goal.as_ref()) + .map(fabro_types::settings::v2::InterpString::as_source) + .as_deref(), + Some("Do things") + ); + } + + #[test] + fn combine_prefers_higher_precedence_self() { + let higher = ConfigLayer::parse( + r#" +_version = 1 +[run] +goal = "higher goal" +"#, + ) + .unwrap(); + let lower = ConfigLayer::parse( + r#" +_version = 1 +[run] +goal = "lower goal" +"#, + ) + .unwrap(); + let merged = higher.combine(lower); + assert_eq!( + merged + .file + .run + .as_ref() + .and_then(|r| r.goal.as_ref()) + .map(fabro_types::settings::v2::InterpString::as_source) + .as_deref(), + Some("higher goal") + ); } } diff --git a/lib/crates/fabro-config/src/effective_settings.rs b/lib/crates/fabro-config/src/effective_settings.rs index f1f499320..b1e3561b2 100644 --- a/lib/crates/fabro-config/src/effective_settings.rs +++ b/lib/crates/fabro-config/src/effective_settings.rs @@ -1,5 +1,14 @@ +//! Effective settings resolution: combine layers into one resolved [`Settings`]. +//! +//! Shared layered domains (`project`, `workflow`, `run`, `features`) merge +//! across all three config files (settings.toml, fabro.toml, workflow.toml). +//! Owner-specific domains (`cli`, `server`) are consumed only from the local +//! `~/.fabro/settings.toml` plus explicit process-local overrides — their +//! 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 crate::ConfigLayer; @@ -44,37 +53,41 @@ pub fn resolve_settings( args, mut workflow, mut project, - mut user, + user, } = layers; match mode { - EffectiveSettingsMode::LocalOnly => args + EffectiveSettingsMode::LocalOnly => Ok(args .combine(workflow) .combine(project) .combine(user) - .resolve(), + .resolve()), EffectiveSettingsMode::RemoteServer | EffectiveSettingsMode::LocalDaemon => { let server_settings = server_settings.ok_or_else(|| { anyhow!("server settings are required for server-targeted settings resolution") })?; - strip_server_owned_fields(&mut workflow); - strip_server_owned_fields(&mut project); - strip_server_owned_fields(&mut user); + strip_owner_domains(workflow.as_v2_mut()); + strip_owner_domains(project.as_v2_mut()); + let mut stripped_user = user; + strip_owner_domains(stripped_user.as_v2_mut()); - let server_defaults = match mode { - EffectiveSettingsMode::RemoteServer => server_defaults_layer(server_settings)?, - EffectiveSettingsMode::LocalDaemon => { - local_daemon_server_overrides_layer(server_settings)? - } - EffectiveSettingsMode::LocalOnly => unreachable!(), - }; + let server_defaults = server_defaults_layer(server_settings)?; let mut settings = args .combine(workflow) .combine(project) - .combine(user) - .combine(server_defaults) - .resolve()?; + .combine(stripped_user) + .resolve(); + + match mode { + EffectiveSettingsMode::RemoteServer => { + apply_server_defaults(&mut settings, &server_defaults); + } + EffectiveSettingsMode::LocalDaemon => { + apply_local_daemon_overrides(&mut settings, &server_defaults); + } + EffectiveSettingsMode::LocalOnly => unreachable!(), + } settings .storage_dir .clone_from(&server_settings.storage_dir); @@ -83,38 +96,66 @@ pub fn resolve_settings( } } -fn server_defaults_layer(settings: &Settings) -> Result { - let mut layer: ConfigLayer = serde_json::from_value(serde_json::to_value(settings)?)?; +fn strip_owner_domains(file: &mut SettingsFile) { + file.cli = None; + file.server = None; +} + +fn server_defaults_layer(settings: &Settings) -> Result { + 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. - layer.dry_run = None; - Ok(layer) + out.dry_run = None; + Ok(out) } -fn local_daemon_server_overrides_layer(settings: &Settings) -> Result { - let layer = server_defaults_layer(settings)?; - Ok(ConfigLayer { - storage_dir: layer.storage_dir, - max_concurrent_runs: layer.max_concurrent_runs, - artifact_storage: layer.artifact_storage, - web: layer.web, - api: layer.api, - features: layer.features, - ..Default::default() - }) +fn apply_server_defaults(settings: &mut Settings, server: &Settings) { + 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); + } + 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()); + } + } } -fn strip_server_owned_fields(layer: &mut ConfigLayer) { - layer.server = None; - layer.exec = None; - layer.storage_dir = None; - layer.max_concurrent_runs = None; - layer.artifact_storage = None; - layer.web = None; - layer.api = None; - layer.features = None; - layer.log = None; +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; + 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)] @@ -125,7 +166,7 @@ mod tests { use crate::ConfigLayer; fn layer(source: &str) -> ConfigLayer { - toml::from_str(source).expect("config layer fixture should parse") + ConfigLayer::parse(source).expect("v2 fixture should parse") } #[test] @@ -136,22 +177,27 @@ mod tests { ConfigLayer::default(), layer( r#" -[llm] -model = "project-model" +_version = 1 -[vars] +[run.model] +name = "project-model" + +[run.inputs] project_only = "1" shared = "project" "#, ), layer( r#" -storage_dir = "/tmp/local-storage" +_version = 1 -[llm] +[server.storage] +root = "/tmp/local-storage" + +[run.model] provider = "openai" -[vars] +[run.inputs] user_only = "1" shared = "user" "#, @@ -164,66 +210,48 @@ shared = "user" let llm = settings.llm.expect("llm config"); assert_eq!(llm.model.as_deref(), Some("project-model")); - assert_eq!(llm.provider.as_deref(), Some("openai")); - assert_eq!( - settings.storage_dir, - Some(PathBuf::from("/tmp/local-storage")) - ); - assert_eq!( - settings - .vars - .as_ref() - .and_then(|vars| vars.get("project_only")), - Some(&"1".to_string()) - ); - assert_eq!( - settings - .vars - .as_ref() - .and_then(|vars| vars.get("user_only")), - Some(&"1".to_string()) - ); - assert_eq!( - settings.vars.as_ref().and_then(|vars| vars.get("shared")), - Some(&"project".to_string()) + // 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())); + assert!( + vars.get("user_only").is_none(), + "project.inputs should replace user.inputs wholesale" ); } #[test] - fn local_only_merges_workflow_project_and_user_layers() { + fn local_only_merges_workflow_project_user() { let settings = resolve_settings( EffectiveSettingsLayers::new( ConfigLayer::default(), layer( r#" +_version = 1 + +[run] goal = "workflow goal" -[llm] -model = "workflow-model" - -[vars] -workflow_only = "1" -shared = "workflow" +[run.model] +name = "workflow-model" "#, ), layer( r#" -[llm] -model = "project-model" +_version = 1 -[vars] -project_only = "1" -shared = "project" +[run.model] +name = "project-model" "#, ), layer( r#" -[llm] +_version = 1 + +[run.model] provider = "openai" - -[vars] -user_only = "1" -shared = "user" "#, ), ), @@ -232,197 +260,55 @@ shared = "user" ) .unwrap(); - let llm = settings.llm.expect("llm config"); assert_eq!(settings.goal.as_deref(), Some("workflow goal")); - assert_eq!(llm.model.as_deref(), Some("workflow-model")); - assert_eq!(llm.provider.as_deref(), Some("openai")); - assert_eq!( - settings - .vars - .as_ref() - .and_then(|vars| vars.get("workflow_only")), - Some(&"1".to_string()) - ); - assert_eq!( - settings - .vars - .as_ref() - .and_then(|vars| vars.get("project_only")), - Some(&"1".to_string()) - ); - assert_eq!( - settings - .vars - .as_ref() - .and_then(|vars| vars.get("user_only")), - Some(&"1".to_string()) - ); - assert_eq!( - settings.vars.as_ref().and_then(|vars| vars.get("shared")), - Some(&"workflow".to_string()) - ); - } - - #[test] - fn remote_server_mode_merges_server_defaults_without_allowing_server_owned_local_overrides() { - let server_settings: fabro_types::Settings = toml::from_str( - r#" -storage_dir = "/srv/fabro" -max_concurrent_runs = 9 -dry_run = true - -[vars] -server_only = "1" -shared = "server" -"#, - ) - .unwrap(); - - let settings = resolve_settings( - EffectiveSettingsLayers::new( - ConfigLayer::default(), - ConfigLayer::default(), - layer( - r#" -storage_dir = "/tmp/local-storage" -max_concurrent_runs = 3 - -[vars] -project_only = "1" -shared = "project" -"#, - ), - ConfigLayer::default(), - ), - Some(&server_settings), - EffectiveSettingsMode::RemoteServer, - ) - .unwrap(); - - assert_eq!(settings.storage_dir, Some(PathBuf::from("/srv/fabro"))); - assert_eq!(settings.max_concurrent_runs, Some(9)); - assert_eq!(settings.dry_run, None); - assert_eq!( - settings - .vars - .as_ref() - .and_then(|vars| vars.get("server_only")), - Some(&"1".to_string()) - ); - assert_eq!( - settings - .vars - .as_ref() - .and_then(|vars| vars.get("project_only")), - Some(&"1".to_string()) - ); - assert_eq!( - settings.vars.as_ref().and_then(|vars| vars.get("shared")), - Some(&"project".to_string()) - ); - } - - #[test] - fn remote_server_mode_merges_workflow_project_user_and_server_layers() { - let server_settings: fabro_types::Settings = toml::from_str( - r#" -storage_dir = "/srv/fabro" - -[vars] -server_only = "1" -shared = "server" -"#, - ) - .unwrap(); - - let settings = resolve_settings( - EffectiveSettingsLayers::new( - ConfigLayer::default(), - layer( - r#" -[llm] -model = "workflow-model" - -[vars] -workflow_only = "1" -shared = "workflow" -"#, - ), - layer( - r#" -[vars] -project_only = "1" -shared = "project" -"#, - ), - layer( - r#" -[llm] -provider = "openai" - -[vars] -user_only = "1" -"#, - ), - ), - Some(&server_settings), - EffectiveSettingsMode::RemoteServer, - ) - .unwrap(); - let llm = settings.llm.expect("llm config"); assert_eq!(llm.model.as_deref(), Some("workflow-model")); assert_eq!(llm.provider.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 project_with_server = layer( + r#" +_version = 1 + +[run] +goal = "project goal" + +[server.storage] +root = "/tmp/should-be-inert" +"#, + ); + + let settings = resolve_settings( + EffectiveSettingsLayers::new( + ConfigLayer::default(), + ConfigLayer::default(), + project_with_server, + ConfigLayer::default(), + ), + Some(&server_settings), + EffectiveSettingsMode::RemoteServer, + ) + .unwrap(); + assert_eq!(settings.storage_dir, Some(PathBuf::from("/srv/fabro"))); - assert_eq!( - settings - .vars - .as_ref() - .and_then(|vars| vars.get("workflow_only")), - Some(&"1".to_string()) - ); - assert_eq!( - settings - .vars - .as_ref() - .and_then(|vars| vars.get("project_only")), - Some(&"1".to_string()) - ); - assert_eq!( - settings - .vars - .as_ref() - .and_then(|vars| vars.get("user_only")), - Some(&"1".to_string()) - ); - assert_eq!( - settings - .vars - .as_ref() - .and_then(|vars| vars.get("server_only")), - Some(&"1".to_string()) - ); - assert_eq!( - settings.vars.as_ref().and_then(|vars| vars.get("shared")), - Some(&"workflow".to_string()) - ); + assert_eq!(settings.goal.as_deref(), Some("project goal")); } #[test] fn local_daemon_mode_only_applies_server_owned_overrides() { - let server_settings: fabro_types::Settings = toml::from_str( - r#" -storage_dir = "/srv/fabro" -max_concurrent_runs = 7 - -[llm] -model = "server-model" - -[vars] -server_only = "1" -"#, - ) - .unwrap(); + let server_settings: fabro_types::Settings = fabro_types::Settings { + storage_dir: Some(PathBuf::from("/srv/fabro")), + max_concurrent_runs: Some(7), + ..Default::default() + }; let settings = resolve_settings( EffectiveSettingsLayers::default(), @@ -433,7 +319,5 @@ server_only = "1" assert_eq!(settings.storage_dir, Some(PathBuf::from("/srv/fabro"))); assert_eq!(settings.max_concurrent_runs, Some(7)); - assert_eq!(settings.llm, None); - assert_eq!(settings.vars, None); } } diff --git a/lib/crates/fabro-config/src/lib.rs b/lib/crates/fabro-config/src/lib.rs index 89b49219e..333202b75 100644 --- a/lib/crates/fabro-config/src/lib.rs +++ b/lib/crates/fabro-config/src/lib.rs @@ -7,6 +7,7 @@ pub mod home; pub mod hook; pub mod legacy_env; pub mod mcp; +pub mod merge; pub mod project; pub mod run; pub mod sandbox; diff --git a/lib/crates/fabro-config/src/merge.rs b/lib/crates/fabro-config/src/merge.rs new file mode 100644 index 000000000..135816b8c --- /dev/null +++ b/lib/crates/fabro-config/src/merge.rs @@ -0,0 +1,669 @@ +//! v2 merge matrix implementation. +//! +//! Encodes the normative merge behavior from the requirements doc: replace +//! scalars, field-merge structured tables, replace freeform maps by default, +//! sticky merge-by-key where the requirements call for it, splice-capable +//! string arrays, whole-list replacement for ordered prepare steps, and +//! ordered hook merging with optional `id` replacement. + +use std::collections::HashMap; + +use fabro_types::settings::v2::cli::{ + CliExecAgentLayer, CliExecLayer, CliExecModelLayer, CliLayer, CliTargetLayer, +}; +use fabro_types::settings::v2::project::ProjectLayer; +use fabro_types::settings::v2::run::{ + DaytonaSandboxLayer, GitAuthorLayer, HookEntry, InterviewsLayer, ModelRefOrSplice, + NotificationRouteLayer, RunAgentLayer, RunCheckpointLayer, RunExecutionLayer, RunGitLayer, + RunLayer, RunModelLayer, RunPrepareLayer, RunPullRequestLayer, RunSandboxLayer, RunScmLayer, + StringOrSplice, +}; +use fabro_types::settings::v2::server::{ + ServerArtifactsLayer, ServerAuthLayer, ServerIntegrationsLayer, ServerLayer, ServerListenLayer, + ServerSchedulerLayer, ServerSlateDbLayer, ServerStorageLayer, ServerWebLayer, +}; +use fabro_types::settings::v2::tree::SettingsFile; +use fabro_types::settings::v2::workflow::WorkflowLayer; + +/// Combine two settings files: `higher` takes precedence over `lower` wherever +/// the merge matrix does not dictate otherwise. +#[must_use] +pub fn combine_files(lower: SettingsFile, higher: SettingsFile) -> SettingsFile { + SettingsFile { + version: higher.version.or(lower.version), + project: merge_option(lower.project, higher.project, combine_project), + workflow: merge_option(lower.workflow, higher.workflow, combine_workflow), + run: merge_option(lower.run, higher.run, combine_run), + cli: merge_option(lower.cli, higher.cli, combine_cli), + server: merge_option(lower.server, higher.server, combine_server), + features: replace_if_some(lower.features, higher.features), + } +} + +fn merge_option(lower: Option, higher: Option, f: fn(T, T) -> T) -> Option { + match (lower, higher) { + (Some(l), Some(h)) => Some(f(l, h)), + (Some(l), None) => Some(l), + (None, Some(h)) => Some(h), + (None, None) => None, + } +} + +fn replace_if_some(lower: Option, higher: Option) -> Option { + higher.or(lower) +} + +fn merge_string_map_replace( + lower: HashMap, + higher: HashMap, +) -> HashMap { + if higher.is_empty() { lower } else { higher } +} + +fn merge_string_map_sticky( + mut lower: HashMap, + higher: HashMap, +) -> HashMap { + for (k, v) in higher { + lower.insert(k, v); + } + lower +} + +// ------------------- project ------------------- + +fn combine_project(lower: ProjectLayer, higher: ProjectLayer) -> ProjectLayer { + ProjectLayer { + name: higher.name.or(lower.name), + description: higher.description.or(lower.description), + directory: higher.directory.or(lower.directory), + metadata: merge_string_map_replace(lower.metadata, higher.metadata), + } +} + +// ------------------- workflow ------------------- + +fn combine_workflow(lower: WorkflowLayer, higher: WorkflowLayer) -> WorkflowLayer { + WorkflowLayer { + name: higher.name.or(lower.name), + description: higher.description.or(lower.description), + graph: higher.graph.or(lower.graph), + metadata: merge_string_map_replace(lower.metadata, higher.metadata), + } +} + +// ------------------- run ------------------- + +fn combine_run(lower: RunLayer, higher: RunLayer) -> RunLayer { + RunLayer { + goal: higher.goal.or(lower.goal), + working_dir: higher.working_dir.or(lower.working_dir), + metadata: merge_string_map_replace(lower.metadata, higher.metadata), + inputs: higher.inputs.or(lower.inputs), + model: merge_option(lower.model, higher.model, combine_run_model), + git: merge_option(lower.git, higher.git, combine_run_git), + prepare: merge_option(lower.prepare, higher.prepare, combine_run_prepare), + execution: merge_option(lower.execution, higher.execution, combine_run_execution), + checkpoint: merge_option(lower.checkpoint, higher.checkpoint, combine_run_checkpoint), + sandbox: merge_option(lower.sandbox, higher.sandbox, combine_run_sandbox), + notifications: combine_notifications(lower.notifications, higher.notifications), + interviews: merge_option(lower.interviews, higher.interviews, combine_interviews), + agent: merge_option(lower.agent, higher.agent, combine_run_agent), + hooks: combine_hooks(lower.hooks, higher.hooks), + scm: merge_option(lower.scm, higher.scm, combine_run_scm), + pull_request: merge_option(lower.pull_request, higher.pull_request, combine_run_pr), + artifacts: replace_if_some(lower.artifacts, higher.artifacts), + } +} + +fn combine_run_model(lower: RunModelLayer, higher: RunModelLayer) -> RunModelLayer { + RunModelLayer { + provider: higher.provider.or(lower.provider), + name: higher.name.or(lower.name), + fallbacks: splice_model_fallbacks(lower.fallbacks, higher.fallbacks), + } +} + +fn splice_model_fallbacks( + lower: Vec, + higher: Vec, +) -> Vec { + if higher.is_empty() { + return lower; + } + let splice_pos = higher + .iter() + .position(|e| matches!(e, ModelRefOrSplice::Splice)); + let Some(pos) = splice_pos else { + return higher; + }; + let mut out = Vec::new(); + for (i, entry) in higher.into_iter().enumerate() { + if i == pos { + out.extend( + lower + .iter() + .filter(|e| !matches!(e, ModelRefOrSplice::Splice)) + .cloned(), + ); + } else if !matches!(entry, ModelRefOrSplice::Splice) { + out.push(entry); + } + } + out +} + +fn combine_run_git(lower: RunGitLayer, higher: RunGitLayer) -> RunGitLayer { + RunGitLayer { + author: merge_option(lower.author, higher.author, combine_git_author), + } +} + +fn combine_git_author(lower: GitAuthorLayer, higher: GitAuthorLayer) -> GitAuthorLayer { + GitAuthorLayer { + name: higher.name.or(lower.name), + email: higher.email.or(lower.email), + } +} + +fn combine_run_prepare(_lower: RunPrepareLayer, higher: RunPrepareLayer) -> RunPrepareLayer { + // Whole-list replacement for prepare.steps per the merge matrix. + higher +} + +fn combine_run_execution(lower: RunExecutionLayer, higher: RunExecutionLayer) -> RunExecutionLayer { + RunExecutionLayer { + mode: higher.mode.or(lower.mode), + approval: higher.approval.or(lower.approval), + retros: higher.retros.or(lower.retros), + } +} + +fn combine_run_checkpoint( + lower: RunCheckpointLayer, + higher: RunCheckpointLayer, +) -> RunCheckpointLayer { + // Exclude globs are a security/policy list: replace by default. + if higher.exclude_globs.is_empty() { + lower + } else { + higher + } +} + +fn combine_run_sandbox(lower: RunSandboxLayer, higher: RunSandboxLayer) -> RunSandboxLayer { + RunSandboxLayer { + provider: higher.provider.or(lower.provider), + preserve: higher.preserve.or(lower.preserve), + devcontainer: higher.devcontainer.or(lower.devcontainer), + // Sticky merge-by-key for run.sandbox.env per R71. + env: merge_string_map_sticky(lower.env, higher.env), + local: higher.local.or(lower.local), + daytona: merge_option(lower.daytona, higher.daytona, combine_daytona), + } +} + +fn combine_daytona(lower: DaytonaSandboxLayer, higher: DaytonaSandboxLayer) -> DaytonaSandboxLayer { + DaytonaSandboxLayer { + auto_stop_interval: higher.auto_stop_interval.or(lower.auto_stop_interval), + // Sticky merge-by-key for provider-native labels per R71. + labels: merge_string_map_sticky(lower.labels, higher.labels), + snapshot: higher.snapshot.or(lower.snapshot), + network: higher.network.or(lower.network), + skip_clone: higher.skip_clone.or(lower.skip_clone), + } +} + +fn combine_notifications( + mut lower: HashMap, + higher: HashMap, +) -> HashMap { + for (k, h) in higher { + match lower.remove(&k) { + Some(l) => { + lower.insert(k, combine_notification_route(l, h)); + } + None => { + lower.insert(k, h); + } + } + } + lower +} + +fn combine_notification_route( + lower: NotificationRouteLayer, + higher: NotificationRouteLayer, +) -> NotificationRouteLayer { + NotificationRouteLayer { + enabled: higher.enabled.or(lower.enabled), + provider: higher.provider.or(lower.provider), + events: splice_events(lower.events, higher.events), + slack: higher.slack.or(lower.slack), + discord: higher.discord.or(lower.discord), + teams: higher.teams.or(lower.teams), + } +} + +fn splice_events(lower: Vec, higher: Vec) -> Vec { + if higher.is_empty() { + return lower; + } + let splice_pos = higher + .iter() + .position(|e| matches!(e, StringOrSplice::Splice)); + let Some(pos) = splice_pos else { + return higher; + }; + let mut out = Vec::new(); + for (i, entry) in higher.into_iter().enumerate() { + if i == pos { + out.extend( + lower + .iter() + .filter(|e| !matches!(e, StringOrSplice::Splice)) + .cloned(), + ); + } else if !matches!(entry, StringOrSplice::Splice) { + out.push(entry); + } + } + out +} + +fn combine_interviews(lower: InterviewsLayer, higher: InterviewsLayer) -> InterviewsLayer { + InterviewsLayer { + provider: higher.provider.or(lower.provider), + slack: higher.slack.or(lower.slack), + discord: higher.discord.or(lower.discord), + teams: higher.teams.or(lower.teams), + } +} + +fn combine_run_agent(lower: RunAgentLayer, higher: RunAgentLayer) -> RunAgentLayer { + RunAgentLayer { + permissions: higher.permissions.or(lower.permissions), + // MCP entries: field-merge per key. Higher replaces lower for same keys. + mcps: merge_string_map_sticky(lower.mcps, higher.mcps), + } +} + +/// Merge two ordered hook lists using the id-aware replacement rule. +fn combine_hooks(lower: Vec, higher: Vec) -> Vec { + let mut out: Vec = Vec::with_capacity(lower.len() + higher.len()); + let mut appended_ids: Vec = Vec::new(); + + for lower_entry in &lower { + if let Some(id) = &lower_entry.id { + if let Some(replacement) = higher.iter().find(|h| h.id.as_deref() == Some(id.as_str())) + { + out.push(replacement.clone()); + appended_ids.push(id.clone()); + continue; + } + } + out.push(lower_entry.clone()); + } + + for higher_entry in higher { + if let Some(id) = &higher_entry.id { + if appended_ids.contains(id) { + continue; + } + } + out.push(higher_entry); + } + + out +} + +fn combine_run_scm(lower: RunScmLayer, higher: RunScmLayer) -> RunScmLayer { + RunScmLayer { + provider: higher.provider.or(lower.provider), + owner: higher.owner.or(lower.owner), + repository: higher.repository.or(lower.repository), + github: higher.github.or(lower.github), + } +} + +fn combine_run_pr(lower: RunPullRequestLayer, higher: RunPullRequestLayer) -> RunPullRequestLayer { + RunPullRequestLayer { + enabled: higher.enabled.or(lower.enabled), + draft: higher.draft.or(lower.draft), + auto_merge: higher.auto_merge.or(lower.auto_merge), + merge_strategy: higher.merge_strategy.or(lower.merge_strategy), + } +} + +// ------------------- cli ------------------- + +fn combine_cli(lower: CliLayer, higher: CliLayer) -> CliLayer { + CliLayer { + target: merge_option(lower.target, higher.target, combine_cli_target), + auth: higher.auth.or(lower.auth), + exec: merge_option(lower.exec, higher.exec, combine_cli_exec), + output: higher.output.or(lower.output), + updates: higher.updates.or(lower.updates), + logging: higher.logging.or(lower.logging), + } +} + +fn combine_cli_target(_lower: CliTargetLayer, higher: CliTargetLayer) -> CliTargetLayer { + // The transport type is a scalar discriminant: the higher layer's choice wins. + higher +} + +fn combine_cli_exec(lower: CliExecLayer, higher: CliExecLayer) -> CliExecLayer { + CliExecLayer { + prevent_idle_sleep: higher.prevent_idle_sleep.or(lower.prevent_idle_sleep), + model: merge_option(lower.model, higher.model, combine_cli_exec_model), + agent: merge_option(lower.agent, higher.agent, combine_cli_exec_agent), + } +} + +fn combine_cli_exec_model( + lower: CliExecModelLayer, + higher: CliExecModelLayer, +) -> CliExecModelLayer { + CliExecModelLayer { + provider: higher.provider.or(lower.provider), + name: higher.name.or(lower.name), + } +} + +fn combine_cli_exec_agent( + lower: CliExecAgentLayer, + higher: CliExecAgentLayer, +) -> CliExecAgentLayer { + CliExecAgentLayer { + permissions: higher.permissions.or(lower.permissions), + mcps: merge_string_map_sticky(lower.mcps, higher.mcps), + } +} + +// ------------------- server ------------------- + +fn combine_server(lower: ServerLayer, higher: ServerLayer) -> ServerLayer { + ServerLayer { + listen: merge_option(lower.listen, higher.listen, combine_listen), + api: higher.api.or(lower.api), + web: merge_option(lower.web, higher.web, combine_server_web), + auth: merge_option(lower.auth, higher.auth, combine_server_auth), + storage: merge_option(lower.storage, higher.storage, combine_server_storage), + artifacts: merge_option(lower.artifacts, higher.artifacts, combine_server_artifacts), + slatedb: merge_option(lower.slatedb, higher.slatedb, combine_server_slatedb), + scheduler: merge_option(lower.scheduler, higher.scheduler, combine_server_scheduler), + logging: higher.logging.or(lower.logging), + integrations: merge_option( + lower.integrations, + higher.integrations, + combine_server_integrations, + ), + } +} + +fn combine_listen(_lower: ServerListenLayer, higher: ServerListenLayer) -> ServerListenLayer { + // Transport type is a scalar discriminant: replace whole. + higher +} + +fn combine_server_web(lower: ServerWebLayer, higher: ServerWebLayer) -> ServerWebLayer { + ServerWebLayer { + enabled: higher.enabled.or(lower.enabled), + url: higher.url.or(lower.url), + } +} + +fn combine_server_auth(lower: ServerAuthLayer, higher: ServerAuthLayer) -> ServerAuthLayer { + ServerAuthLayer { + api: higher.api.or(lower.api), + web: higher.web.or(lower.web), + } +} + +fn combine_server_storage( + lower: ServerStorageLayer, + higher: ServerStorageLayer, +) -> ServerStorageLayer { + ServerStorageLayer { + root: higher.root.or(lower.root), + } +} + +fn combine_server_artifacts( + lower: ServerArtifactsLayer, + higher: ServerArtifactsLayer, +) -> ServerArtifactsLayer { + ServerArtifactsLayer { + provider: higher.provider.or(lower.provider), + prefix: higher.prefix.or(lower.prefix), + local: higher.local.or(lower.local), + s3: higher.s3.or(lower.s3), + } +} + +fn combine_server_slatedb( + lower: ServerSlateDbLayer, + higher: ServerSlateDbLayer, +) -> ServerSlateDbLayer { + ServerSlateDbLayer { + provider: higher.provider.or(lower.provider), + prefix: higher.prefix.or(lower.prefix), + flush_interval: higher.flush_interval.or(lower.flush_interval), + local: higher.local.or(lower.local), + s3: higher.s3.or(lower.s3), + } +} + +fn combine_server_scheduler( + lower: ServerSchedulerLayer, + higher: ServerSchedulerLayer, +) -> ServerSchedulerLayer { + ServerSchedulerLayer { + max_concurrent_runs: higher.max_concurrent_runs.or(lower.max_concurrent_runs), + } +} + +fn combine_server_integrations( + lower: ServerIntegrationsLayer, + higher: ServerIntegrationsLayer, +) -> ServerIntegrationsLayer { + ServerIntegrationsLayer { + github: higher.github.or(lower.github), + slack: higher.slack.or(lower.slack), + discord: higher.discord.or(lower.discord), + teams: higher.teams.or(lower.teams), + } +} + +#[cfg(test)] +mod tests { + use super::*; + use fabro_types::settings::v2::parse_settings_file; + + fn parse(input: &str) -> SettingsFile { + parse_settings_file(input).expect("fixture should parse") + } + + #[test] + fn run_inputs_replace_wholesale() { + let lower = parse( + r#" +[run.inputs] +a = "lower" +b = "lower" +"#, + ); + let higher = parse( + r#" +[run.inputs] +a = "higher" +"#, + ); + let merged = combine_files(lower, higher); + let inputs = merged.run.unwrap().inputs.unwrap(); + assert_eq!(inputs.len(), 1); + assert_eq!(inputs.get("a"), Some(&toml::Value::String("higher".into()))); + assert!(inputs.get("b").is_none(), "lower key should be gone"); + } + + #[test] + fn run_sandbox_env_merges_sticky() { + let lower = parse( + r#" +[run.sandbox.env] +A = "lower-a" +B = "lower-b" +"#, + ); + let higher = parse( + r#" +[run.sandbox.env] +A = "higher-a" +C = "higher-c" +"#, + ); + let merged = combine_files(lower, higher); + let sandbox = merged.run.unwrap().sandbox.unwrap(); + assert_eq!(sandbox.env.len(), 3); + } + + #[test] + fn run_prepare_steps_replaces_whole_list() { + let lower = parse( + r#" +[[run.prepare.steps]] +script = "lower-1" + +[[run.prepare.steps]] +script = "lower-2" +"#, + ); + let higher = parse( + r#" +[[run.prepare.steps]] +script = "higher-1" +"#, + ); + let merged = combine_files(lower, higher); + let steps = merged.run.unwrap().prepare.unwrap().steps; + assert_eq!(steps.len(), 1); + } + + #[test] + fn run_model_fallbacks_splice_inserts_inherited() { + let lower = parse( + r#" +[run.model] +fallbacks = ["openai", "gpt-5.4"] +"#, + ); + let higher = parse( + r#" +[run.model] +fallbacks = ["anthropic", "..."] +"#, + ); + let merged = combine_files(lower, higher); + let fallbacks = merged.run.unwrap().model.unwrap().fallbacks; + // ["anthropic", "openai", "gpt-5.4"] + assert_eq!(fallbacks.len(), 3); + } + + #[test] + fn hooks_replace_by_id_in_place() { + let lower = parse( + r#" +[[run.hooks]] +id = "shared" +event = "run_start" +script = "lower-script" +"#, + ); + let higher = parse( + r#" +[[run.hooks]] +id = "shared" +event = "run_start" +script = "higher-script" +"#, + ); + let merged = combine_files(lower, higher); + let hooks = merged.run.unwrap().hooks; + assert_eq!(hooks.len(), 1); + assert_eq!( + hooks[0].script.as_ref().map(|s| s.as_source()).as_deref(), + Some("higher-script") + ); + } + + #[test] + fn anonymous_hooks_append_after_merged_inherited() { + let lower = parse( + r#" +[[run.hooks]] +event = "run_start" +script = "lower-anon" +"#, + ); + let higher = parse( + r#" +[[run.hooks]] +event = "run_complete" +script = "higher-anon" +"#, + ); + let merged = combine_files(lower, higher); + let hooks = merged.run.unwrap().hooks; + assert_eq!(hooks.len(), 2); + assert_eq!( + hooks[0].script.as_ref().map(|s| s.as_source()).as_deref(), + Some("lower-anon") + ); + assert_eq!( + hooks[1].script.as_ref().map(|s| s.as_source()).as_deref(), + Some("higher-anon") + ); + } + + #[test] + fn notification_route_events_splice() { + let lower = parse( + r#" +[run.notifications.ops] +events = ["run.failed"] +"#, + ); + let higher = parse( + r#" +[run.notifications.ops] +events = ["...", "run.completed"] +"#, + ); + let merged = combine_files(lower, higher); + let run = merged.run.unwrap(); + let events = &run.notifications.get("ops").unwrap().events; + assert_eq!(events.len(), 2); + } + + #[test] + fn project_metadata_replaces_wholesale() { + let lower = parse( + r#" +[project.metadata] +a = "1" +b = "2" +"#, + ); + let higher = parse( + r#" +[project.metadata] +a = "replaced" +"#, + ); + let merged = combine_files(lower, higher); + let meta = merged.project.unwrap().metadata; + assert_eq!(meta.len(), 1); + assert_eq!(meta.get("a"), Some(&"replaced".to_string())); + } +} diff --git a/lib/crates/fabro-config/src/project.rs b/lib/crates/fabro-config/src/project.rs index a2819eb1f..519e479d7 100644 --- a/lib/crates/fabro-config/src/project.rs +++ b/lib/crates/fabro-config/src/project.rs @@ -1,8 +1,14 @@ +//! Project-level config loading and workflow discovery. +//! +//! Stage 3 replaced the parse-time `ProjectConfig` type with the v2 parse +//! tree in `fabro_types::settings::v2`. This module keeps the workflow +//! discovery helpers and re-exports resolved project settings. + use std::fmt::Write; use std::path::{Path, PathBuf}; use anyhow::{Context, bail}; -use serde::{Deserialize, Serialize}; +use serde::Serialize; use crate::config::ConfigLayer; use crate::run; @@ -10,13 +16,8 @@ use fabro_types::Settings; pub use fabro_types::settings::project::ProjectSettings; const CONFIG_FILENAME: &str = "fabro.toml"; -const SUPPORTED_VERSION: u32 = 1; const RUN_GRAPH_FILE: &str = "workflow.fabro"; - -#[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize, crate::Combine)] -pub struct ProjectConfig { - pub root: Option, -} +const DEFAULT_FABRO_DIRECTORY: &str = "fabro/"; #[derive(Clone, Debug)] pub struct WorkflowPathResolution { @@ -27,28 +28,9 @@ pub struct WorkflowPathResolution { pub workflow_slug: Option, } -fn default_root() -> String { - ".".to_string() -} - -impl From for ProjectSettings { - fn from(value: ProjectConfig) -> Self { - Self { - root: value.root.unwrap_or_else(default_root), - } - } -} - /// Parse a project config from a TOML string. pub fn parse_project_config(content: &str) -> anyhow::Result { - let config: ConfigLayer = toml::from_str(content).context("Failed to parse project config")?; - let version = config.version.unwrap_or(0); - if version != SUPPORTED_VERSION { - bail!( - "Unsupported project config version: {version}. Only version {SUPPORTED_VERSION} is supported.", - ); - } - Ok(config) + ConfigLayer::parse(content).context("Failed to parse project config") } /// Load a project config from a file path. @@ -57,10 +39,11 @@ pub fn load_project_config(path: &Path) -> anyhow::Result { .with_context(|| format!("Failed to read {}", path.display()))?; let config = parse_project_config(&content)?; let root = config - .fabro + .as_v2() + .project .as_ref() - .and_then(|f| f.root.as_deref()) - .unwrap_or("."); + .and_then(|p| p.directory.as_deref()) + .unwrap_or(DEFAULT_FABRO_DIRECTORY); tracing::debug!(path = %path.display(), root = %root, "Loaded project config"); Ok(config) } @@ -98,11 +81,6 @@ fn workflow_slug_from_path(workflow_path: &Path) -> Option { } /// Resolve a workflow argument to a path. -/// -/// - If the arg has a file extension (`.toml`, `.fabro`, etc.), return it as-is. -/// - If no extension, attempt project-based resolution: find `fabro.toml`, resolve -/// `{fabro_root}/workflows/{name}/workflow.toml`. Returns an error with suggestions -/// if an `fabro.toml` exists but the workflow wasn't found. pub fn resolve_workflow_arg(arg: &Path) -> anyhow::Result { let start = std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")); resolve_workflow_arg_from(arg, &start) @@ -117,8 +95,13 @@ pub fn resolve_workflow_path( if path.extension().is_some_and(|ext| ext == "toml") { match run::load_run_config(&path) { Ok(cfg) => { - let dot_path = - run::resolve_graph_path(&path, cfg.graph.as_deref().unwrap_or(RUN_GRAPH_FILE)); + let graph = cfg + .as_v2() + .workflow + .as_ref() + .and_then(|w| w.graph.as_deref()) + .unwrap_or(RUN_GRAPH_FILE); + let dot_path = run::resolve_graph_path(&path, graph); Ok(WorkflowPathResolution { resolved_workflow_path: path.clone(), dot_path, @@ -275,11 +258,16 @@ fn list_workflows_in(workflows_dir: &Path) -> Vec { .collect() } -/// Read the `goal` field from a `workflow.toml` without full config validation. +/// Read the `run.goal` field from a `workflow.toml` without full config validation. fn read_workflow_goal(workflow_toml: &Path) -> Option { let content = std::fs::read_to_string(workflow_toml).ok()?; let table: toml::Table = content.parse().ok()?; - table.get("goal")?.as_str().map(String::from) + table + .get("run")? + .as_table()? + .get("goal")? + .as_str() + .map(String::from) } /// List workflows with metadata by scanning project and user workflow directories. @@ -320,7 +308,6 @@ pub fn list_workflows_detailed( } /// List workflow names by scanning project and user workflow directories. -/// Project workflows appear first; user workflows are deduplicated. pub fn list_available_workflows( project_workflows_dir: Option<&Path>, user_workflows_dir: Option<&Path>, @@ -353,9 +340,6 @@ fn find_closest_match(input: &str, candidates: &[String]) -> Option { } /// Resolve a workflow argument to a DOT path and optional run config. -/// -/// Calls `resolve_workflow_arg` first, then if the result is a `.toml` file, -/// loads the run config and resolves the graph path within it. pub fn resolve_workflow(arg: &Path) -> anyhow::Result<(PathBuf, Option)> { let start = std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")); let resolution = resolve_workflow_path(arg, &start)?; @@ -363,147 +347,114 @@ pub fn resolve_workflow(arg: &Path) -> anyhow::Result<(PathBuf, Option bool { let start = std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")); match discover_project_config(&start) { Ok(Some((_path, config))) => config - .features + .as_v2() + .run .as_ref() - .and_then(|f| f.retros) + .and_then(|r| r.execution.as_ref()) + .and_then(|e| e.retros) .unwrap_or(false), _ => false, } } /// Resolve the fabro root directory from a config file path and its config. -/// The returned path is the directory containing `fabro.toml` joined with the `root` value. +/// The returned path is the directory containing `fabro.toml` joined with the +/// `project.directory` value (default: `fabro/`). pub fn resolve_fabro_root(config_path: &Path, config: &ConfigLayer) -> PathBuf { let project_dir = config_path .parent() .expect("config_path should have a parent directory"); let root = config - .fabro + .as_v2() + .project .as_ref() - .and_then(|f| f.root.as_deref()) - .unwrap_or("."); + .and_then(|p| p.directory.as_deref()) + .unwrap_or(DEFAULT_FABRO_DIRECTORY); project_dir.join(root) } #[cfg(test)] mod tests { use super::*; - use crate::run::{LlmConfig, PullRequestConfig}; use std::fs; use tempfile::TempDir; #[test] fn parse_minimal_config() { - let config = parse_project_config("version = 1\n").unwrap(); - assert_eq!(config.version, Some(1)); - assert_eq!(config.fabro, None,); + let config = parse_project_config("_version = 1\n").unwrap(); + assert_eq!(config.as_v2().version, Some(1)); + assert!(config.as_v2().project.is_none()); } #[test] - fn parse_full_config() { - let config = parse_project_config("version = 1\n[fabro]\nroot = \"fabro/\"\n").unwrap(); - assert_eq!(config.fabro.unwrap().root.as_deref(), Some("fabro/")); - } + fn parse_with_project_directory() { + let config = parse_project_config( + r#" +_version = 1 - #[test] - fn parse_retros_default_false() { - let config = parse_project_config("version = 1\n").unwrap(); - assert!( - !config - .features +[project] +directory = "fabro/" +"#, + ) + .unwrap(); + assert_eq!( + config + .as_v2() + .project .as_ref() - .and_then(|f| f.retros) - .unwrap_or(false) + .and_then(|p| p.directory.as_deref()), + Some("fabro/") ); } #[test] - fn parse_retros_enabled() { - let config = parse_project_config("version = 1\n[features]\nretros = true\n").unwrap(); - assert_eq!(config.features.unwrap().retros, Some(true)); + fn parse_with_run_execution_retros() { + let config = parse_project_config( + r#" +_version = 1 + +[run.execution] +retros = true +"#, + ) + .unwrap(); + assert_eq!( + config + .as_v2() + .run + .as_ref() + .and_then(|r| r.execution.as_ref()) + .and_then(|e| e.retros), + Some(true) + ); } #[test] - fn parse_version_mismatch() { - let err = parse_project_config("version = 2\n").unwrap_err(); + fn parse_rejects_legacy_llm_section() { + let err = parse_project_config("_version = 1\n[llm]\nprovider = \"openai\"\n").unwrap_err(); + let text = format!("{err:#}"); assert!( - err.to_string().contains("Unsupported"), - "Expected 'Unsupported' in error, got: {err}" + text.contains("run.model") || text.contains("llm"), + "expected rename hint for [llm]: {text}" ); } #[test] - fn parse_pull_request_config() { - let config = - parse_project_config("version = 1\n\n[pull_request]\nenabled = true\ndraft = false\n") - .unwrap(); - assert_eq!( - config.pull_request, - Some(PullRequestConfig { - enabled: Some(true), - draft: Some(false), - auto_merge: None, - merge_strategy: None, - }) - ); - } - - #[test] - fn parse_project_config_with_sandbox() { - let toml = r#" -version = 1 -[sandbox] -provider = "daytona" -[sandbox.daytona.snapshot] -name = "my-snapshot" -cpu = 4 -memory = 8 -"#; - let config = parse_project_config(toml).unwrap(); - let sandbox = config.sandbox.unwrap(); - assert_eq!(sandbox.provider.as_deref(), Some("daytona")); - let snap = sandbox.daytona.unwrap().snapshot.unwrap(); - assert_eq!(snap.name.as_deref(), Some("my-snapshot")); - assert_eq!(snap.cpu, Some(4)); - assert_eq!(snap.memory, Some(8)); - } - - #[test] - fn parse_project_config_with_hooks_and_mcp() { - let toml = r#" -version = 1 -[[hooks]] -event = "run_start" -command = "echo start" -[mcp_servers.playwright] -type = "stdio" -command = ["npx", "@playwright/mcp@latest"] -"#; - let config = parse_project_config(toml).unwrap(); - assert_eq!(config.hooks.len(), 1); - assert_eq!(config.mcp_servers.len(), 1); - assert!(config.mcp_servers.contains_key("playwright")); - } - - #[test] - fn parse_project_config_with_llm_and_work_dir() { - let toml = r#" -version = 1 -work_dir = "/workspace" -[llm] -model = "claude-sonnet-4-6" -"#; - let config = parse_project_config(toml).unwrap(); - assert_eq!(config.work_dir.as_deref(), Some("/workspace")); - assert_eq!( - config.llm.unwrap().model.as_deref(), - Some("claude-sonnet-4-6") + fn parse_higher_version_errors() { + let err = parse_project_config("_version = 2\n").unwrap_err(); + let chain: String = err + .chain() + .map(|e| e.to_string()) + .collect::>() + .join("; "); + assert!( + chain.contains("Upgrade") || chain.to_lowercase().contains("version"), + "Expected version hint in chain: {chain}" ); } @@ -511,224 +462,20 @@ model = "claude-sonnet-4-6" fn load_from_disk() { let tmp = TempDir::new().unwrap(); let path = tmp.path().join("fabro.toml"); - fs::write(&path, "version = 1\n").unwrap(); + fs::write(&path, "_version = 1\n").unwrap(); let config = load_project_config(&path).unwrap(); - assert_eq!(config.version, Some(1)); + assert_eq!(config.as_v2().version, Some(1)); } #[test] fn discover_walks_ancestors() { let tmp = TempDir::new().unwrap(); - fs::write(tmp.path().join("fabro.toml"), "version = 1\n").unwrap(); + fs::write(tmp.path().join("fabro.toml"), "_version = 1\n").unwrap(); let sub = tmp.path().join("sub").join("dir"); fs::create_dir_all(&sub).unwrap(); let (found_path, config) = discover_project_config(&sub).unwrap().unwrap(); assert_eq!(found_path, tmp.path().join("fabro.toml")); - assert_eq!(config.version, Some(1)); - } - - #[test] - fn discover_returns_none_when_absent() { - let tmp = TempDir::new().unwrap(); - let result = discover_project_config(tmp.path()).unwrap(); - assert!(result.is_none()); - } - - #[test] - fn resolve_fabro_root_with_subdirectory() { - let config_path = Path::new("/repo/fabro.toml"); - let config = ConfigLayer { - version: Some(1), - fabro: Some(ProjectConfig { - root: Some("fabro/".to_string()), - }), - ..Default::default() - }; - assert_eq!( - resolve_fabro_root(config_path, &config), - Path::new("/repo/fabro/") - ); - } - - #[test] - fn resolve_fabro_root_with_dot() { - let config_path = Path::new("/repo/fabro.toml"); - let config = ConfigLayer { - version: Some(1), - fabro: Some(ProjectConfig { - root: Some(".".to_string()), - }), - ..Default::default() - }; - assert_eq!( - resolve_fabro_root(config_path, &config), - Path::new("/repo/.") - ); - } - - #[test] - fn resolve_fabro_root_without_fabro_section() { - let config_path = Path::new("/repo/fabro.toml"); - let config = ConfigLayer::default(); - assert_eq!( - resolve_fabro_root(config_path, &config), - Path::new("/repo/.") - ); - } - - #[test] - fn for_workflow_discovers_project_from_workflow_location() { - let tmp = TempDir::new().unwrap(); - let project_dir = tmp.path().join("project"); - let other_dir = tmp.path().join("other"); - let workflow_dir = project_dir.join("workflows").join("demo"); - fs::create_dir_all(&workflow_dir).unwrap(); - fs::create_dir_all(&other_dir).unwrap(); - - fs::write( - project_dir.join("fabro.toml"), - "version = 1\nverbose = true\n", - ) - .unwrap(); - fs::write( - other_dir.join("fabro.toml"), - "version = 1\nverbose = false\n", - ) - .unwrap(); - fs::write(workflow_dir.join("workflow.toml"), "version = 1\n").unwrap(); - - let layer = - ConfigLayer::for_workflow(&workflow_dir.join("workflow.toml"), &other_dir).unwrap(); - - assert_eq!(layer.verbose, Some(true)); - } - - #[test] - fn chained_resolve_preserves_precedence_order() { - let tmp = TempDir::new().unwrap(); - let project_dir = tmp.path().join("project"); - let workflow_dir = project_dir.join("workflows").join("demo"); - fs::create_dir_all(&workflow_dir).unwrap(); - - fs::write( - project_dir.join("fabro.toml"), - "version = 1\nverbose = true\n[llm]\nmodel = \"project-model\"\n", - ) - .unwrap(); - fs::write( - workflow_dir.join("workflow.toml"), - "version = 1\ndry_run = true\n[llm]\nmodel = \"workflow-model\"\n", - ) - .unwrap(); - - let cli_defaults = ConfigLayer { - verbose: Some(false), - llm: Some(LlmConfig { - model: Some("cli-model".to_string()), - provider: None, - fallbacks: None, - }), - ..Default::default() - }; - let overrides = ConfigLayer { - dry_run: Some(false), - ..Default::default() - }; - - let settings = overrides - .combine( - ConfigLayer::for_workflow( - &workflow_dir.join("workflow.toml"), - project_dir.as_path(), - ) - .unwrap(), - ) - .combine(cli_defaults) - .resolve() - .unwrap(); - - assert_eq!( - settings.llm.as_ref().and_then(|llm| llm.model.as_deref()), - Some("workflow-model") - ); - assert_eq!(settings.dry_run, Some(false)); - assert_eq!(settings.verbose, Some(true)); - } - - #[test] - fn resolve_workflow_arg_toml_extension_resolves_relative_to_start_dir() { - let tmp = TempDir::new().unwrap(); - let result = resolve_workflow_arg_from(Path::new("my-workflow.toml"), tmp.path()).unwrap(); - assert_eq!(result, tmp.path().join("my-workflow.toml")); - } - - #[test] - fn resolve_workflow_arg_fabro_extension_resolves_relative_to_start_dir() { - let tmp = TempDir::new().unwrap(); - let result = resolve_workflow_arg_from(Path::new("my-workflow.fabro"), tmp.path()).unwrap(); - assert_eq!(result, tmp.path().join("my-workflow.fabro")); - } - - #[test] - fn resolve_workflow_arg_absolute_extension_preserves_absolute_path() { - let tmp = TempDir::new().unwrap(); - let path = tmp.path().join("my-workflow.toml"); - let result = resolve_workflow_arg_from(&path, Path::new("/tmp")).unwrap(); - assert_eq!(result, path); - } - - #[test] - fn resolve_workflow_arg_no_extension_no_config_returns_literal() { - let tmp = TempDir::new().unwrap(); - let result = resolve_workflow_arg_from(Path::new("my-workflow"), tmp.path()).unwrap(); - assert_eq!(result, Path::new("my-workflow")); - } - - #[test] - fn resolve_workflow_arg_no_extension_with_config_and_workflow_file() { - let tmp = TempDir::new().unwrap(); - fs::write(tmp.path().join("fabro.toml"), "version = 1\n").unwrap(); - let wf_dir = tmp.path().join("workflows").join("my-workflow"); - fs::create_dir_all(&wf_dir).unwrap(); - fs::write( - wf_dir.join("workflow.toml"), - "version = 1\ngraph = \"workflow.fabro\"\n", - ) - .unwrap(); - - let result = resolve_workflow_arg_from(Path::new("my-workflow"), tmp.path()).unwrap(); - assert_eq!(result, wf_dir.join("workflow.toml")); - } - - #[test] - fn resolve_workflow_arg_typo_suggests_similar_name() { - let tmp = TempDir::new().unwrap(); - fs::write(tmp.path().join("fabro.toml"), "version = 1\n").unwrap(); - let wf_dir = tmp.path().join("workflows").join("implement"); - fs::create_dir_all(&wf_dir).unwrap(); - fs::write( - wf_dir.join("workflow.toml"), - "version = 1\ngraph = \"w.fabro\"\n", - ) - .unwrap(); - - let err = resolve_workflow_arg_from(Path::new("implemet"), tmp.path()).unwrap_err(); - let msg = err.to_string(); - assert!(msg.contains("Unknown workflow 'implemet'"), "got: {msg}"); - assert!(msg.contains("Did you mean 'implement'?"), "got: {msg}"); - } - - #[test] - fn parse_project_config_with_github() { - let toml = r#" -version = 1 - -[github] -permissions = { contents = "read" } -"#; - let config = parse_project_config(toml).unwrap(); - let github = config.github.unwrap(); - assert_eq!(github.permissions["contents"], "read"); + assert_eq!(config.as_v2().version, Some(1)); } } diff --git a/lib/crates/fabro-config/src/run.rs b/lib/crates/fabro-config/src/run.rs index 5038af905..3e5c61ee8 100644 --- a/lib/crates/fabro-config/src/run.rs +++ b/lib/crates/fabro-config/src/run.rs @@ -1,161 +1,28 @@ +//! Re-export shim for run-side settings types. +//! +//! Stage 3 replaced the parse-time types previously defined here with the +//! v2 parse tree in `fabro_types::settings::v2`. This module stays alive as +//! a pass-through for crates that still import resolved run types via the +//! legacy `fabro_config::run` path; Stage 6 deletes it. + use std::collections::HashMap; use std::path::{Path, PathBuf}; -use anyhow::{Context, bail}; -use serde::{Deserialize, Serialize}; -use tracing::debug; +use anyhow::Context; -use crate::combine::Combine; use crate::config::ConfigLayer; -use crate::sandbox::DockerfileSource; + pub use fabro_types::settings::run::{ ArtifactsSettings, CheckpointSettings, GitHubSettings, LlmSettings, MergeStrategy, PullRequestSettings, SetupSettings, }; -const SUPPORTED_VERSION: u32 = 1; - -#[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize)] -pub struct CheckpointConfig { - #[serde(default)] - pub exclude_globs: Vec, -} - -impl Combine for CheckpointConfig { - fn combine(mut self, other: Self) -> Self { - self.exclude_globs.extend(other.exclude_globs); - self.exclude_globs.sort(); - self.exclude_globs.dedup(); - self - } -} - -impl From for CheckpointSettings { - fn from(value: CheckpointConfig) -> Self { - let mut exclude_globs = value.exclude_globs; - exclude_globs.sort(); - exclude_globs.dedup(); - Self { exclude_globs } - } -} - -#[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize, crate::Combine)] -pub struct PullRequestConfig { - pub enabled: Option, - pub draft: Option, - pub auto_merge: Option, - pub merge_strategy: Option, -} - -impl From for PullRequestSettings { - fn from(value: PullRequestConfig) -> Self { - Self { - enabled: value.enabled.unwrap_or(false), - draft: value.draft.unwrap_or(true), - auto_merge: value.auto_merge.unwrap_or(false), - merge_strategy: value.merge_strategy.unwrap_or_default(), - } - } -} - -#[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize, crate::Combine)] -pub struct ArtifactsConfig { - #[serde(default)] - pub include: Vec, -} - -impl From for ArtifactsSettings { - fn from(value: ArtifactsConfig) -> Self { - Self { - include: value.include, - } - } -} - -#[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize, crate::Combine)] -pub struct GitHubConfig { - #[serde(default)] - pub permissions: HashMap, -} - -impl From for GitHubSettings { - fn from(value: GitHubConfig) -> Self { - Self { - permissions: value.permissions, - } - } -} - -#[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize, crate::Combine)] -pub struct LlmConfig { - pub model: Option, - pub provider: Option, - #[serde(default)] - pub fallbacks: Option>>, -} - -impl From for LlmSettings { - fn from(value: LlmConfig) -> Self { - Self { - model: value.model, - provider: value.provider, - fallbacks: value.fallbacks, - } - } -} - -#[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize, crate::Combine)] -pub struct SetupConfig { - #[serde(default)] - pub commands: Vec, - pub timeout_ms: Option, -} - -impl From for SetupSettings { - fn from(value: SetupConfig) -> Self { - Self { - commands: value.commands, - timeout_ms: value.timeout_ms, - } - } -} - -/// Load and validate a run config from a TOML file. +/// Expand `${env.NAME}` whole-value references inside a string map. /// -/// The `graph` path in the returned config is resolved relative to the -/// TOML file's parent directory. Any `dockerfile = { path = "..." }` is -/// resolved to inline content. -/// -/// `${env.VARNAME}` references in `[sandbox.env]` are NOT resolved here — -/// call [`resolve_sandbox_env`] separately after snapshotting, so that -/// plaintext secrets are never written to disk. -pub fn load_run_config(path: &Path) -> anyhow::Result { - let contents = std::fs::read_to_string(path) - .with_context(|| format!("Failed to read {}", path.display()))?; - let mut config = parse_run_config(&contents)?; - - let config_dir = path.parent().unwrap_or(Path::new(".")); - resolve_dockerfile(&mut config, config_dir)?; - - Ok(config) -} - -/// Resolve `${env.VARNAME}` references in `[sandbox.env]` values. -/// -/// Only whole-value references are supported (no partial interpolation). -/// Missing host env vars produce a hard error. -pub fn resolve_sandbox_env(config: &mut ConfigLayer) -> anyhow::Result<()> { - if let Some(env) = config.sandbox.as_mut().and_then(|s| s.env.as_mut()) { - resolve_env_refs(env)?; - } - Ok(()) -} - -/// Resolve `${env.VARNAME}` patterns in a map of env vars. -/// -/// If the entire value is `${env.VARNAME}`, it is replaced with the host -/// environment variable. Any other value is left as-is. Missing host -/// variables produce an error. +/// Leaves entries that don't match the whole-value form untouched. Missing +/// host variables produce an error. This is the minimal resolver legacy +/// consumers still call while they are being migrated off `Settings`; the +/// full v2 interpolation pass lives in `fabro_types::settings::v2::interp`. pub fn resolve_env_refs(env: &mut HashMap) -> anyhow::Result<()> { for (key, value) in env.iter_mut() { if let Some(var_name) = value @@ -170,54 +37,26 @@ pub fn resolve_env_refs(env: &mut HashMap) -> anyhow::Result<()> Ok(()) } -/// If the config contains a `dockerfile = { path = "..." }`, read the file -/// and replace it with `DockerfileSource::Inline(contents)`. -fn resolve_dockerfile(config: &mut ConfigLayer, config_dir: &Path) -> anyhow::Result<()> { - let source = config - .sandbox - .as_mut() - .and_then(|s| s.daytona.as_mut()) - .and_then(|d| d.snapshot.as_mut()) - .and_then(|snap| snap.dockerfile.as_mut()); - - if let Some(DockerfileSource::Path { path: ref rel }) = source { - let path = config_dir.join(rel); - let contents = std::fs::read_to_string(&path) - .with_context(|| format!("Failed to read dockerfile at {}", path.display()))?; - debug!(path = %path.display(), "Resolved dockerfile from path"); - *source.unwrap() = DockerfileSource::Inline(contents); - } - - Ok(()) -} - -/// Resolve the graph path relative to the TOML file's parent directory. -pub fn resolve_graph_path(toml_path: &Path, graph: &str) -> PathBuf { - let graph_path = Path::new(graph); - if graph_path.is_absolute() { - graph_path.to_path_buf() - } else { - toml_path - .parent() - .unwrap_or(Path::new(".")) - .join(graph_path) - } -} - +/// Load and parse a run config from a TOML file. pub fn parse_run_config(contents: &str) -> anyhow::Result { - let mut config: ConfigLayer = - toml::from_str(contents).context("Failed to parse run config TOML")?; - - if config.graph.is_none() { - config.graph = Some("workflow.fabro".to_string()); - } - - let version = config.version.unwrap_or(0); - if version != SUPPORTED_VERSION { - bail!( - "Unsupported run config version {version}. Only version {SUPPORTED_VERSION} is supported.", - ); - } - - Ok(config) + ConfigLayer::parse(contents).context("Failed to parse run config TOML") +} + +/// Load and parse a run config from a TOML file. +/// +/// Returns the v2-backed `ConfigLayer`. +pub fn load_run_config(path: &Path) -> anyhow::Result { + let content = std::fs::read_to_string(path) + .with_context(|| format!("Failed to read {}", path.display()))?; + ConfigLayer::parse(&content) + .with_context(|| format!("Failed to parse workflow config at {}", path.display())) +} + +/// Resolve a graph path relative to a workflow.toml. +#[must_use] +pub fn resolve_graph_path(workflow_toml: &Path, graph_relative: &str) -> PathBuf { + workflow_toml + .parent() + .unwrap_or_else(|| Path::new(".")) + .join(graph_relative) } diff --git a/lib/crates/fabro-config/src/sandbox.rs b/lib/crates/fabro-config/src/sandbox.rs index a90052d45..2c247031a 100644 --- a/lib/crates/fabro-config/src/sandbox.rs +++ b/lib/crates/fabro-config/src/sandbox.rs @@ -1,98 +1,10 @@ -use std::collections::HashMap; - -use anyhow::anyhow; -use serde::{Deserialize, Serialize}; +//! Re-export shim for sandbox settings types. +//! +//! Stage 3 removed the parse-time `SandboxConfig`/`DaytonaConfig` types; +//! callers that still import resolved sandbox types via this module use the +//! re-exports below. Stage 6 deletes this file. pub use fabro_types::settings::sandbox::{ DaytonaNetwork, DaytonaSettings, DaytonaSnapshotSettings, DockerfileSource, LocalSandboxSettings, SandboxSettings, WorktreeMode, }; - -#[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize, crate::Combine)] -pub struct DaytonaConfig { - pub auto_stop_interval: Option, - pub labels: Option>, - pub snapshot: Option, - pub network: Option, - /// Skip git repo detection and cloning during initialization. - pub skip_clone: Option, -} - -impl TryFrom for DaytonaSettings { - type Error = anyhow::Error; - - fn try_from(value: DaytonaConfig) -> Result { - Ok(Self { - auto_stop_interval: value.auto_stop_interval, - labels: value.labels, - snapshot: value.snapshot.map(TryInto::try_into).transpose()?, - network: value.network, - skip_clone: value.skip_clone.unwrap_or(false), - }) - } -} - -/// Snapshot configuration: when present, the sandbox is created from a snapshot -/// instead of a bare Docker image. -#[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize, crate::Combine)] -pub struct DaytonaSnapshotConfig { - pub name: Option, - pub cpu: Option, - pub memory: Option, - pub disk: Option, - pub dockerfile: Option, -} - -impl TryFrom for DaytonaSnapshotSettings { - type Error = anyhow::Error; - - fn try_from(value: DaytonaSnapshotConfig) -> Result { - Ok(Self { - name: value - .name - .ok_or_else(|| anyhow!("sandbox.daytona.snapshot.name is required"))?, - cpu: value.cpu, - memory: value.memory, - disk: value.disk, - dockerfile: value.dockerfile, - }) - } -} - -#[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize, crate::Combine)] -pub struct LocalSandboxConfig { - pub worktree_mode: Option, -} - -impl From for LocalSandboxSettings { - fn from(value: LocalSandboxConfig) -> Self { - Self { - worktree_mode: value.worktree_mode.unwrap_or_default(), - } - } -} - -#[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize, crate::Combine)] -pub struct SandboxConfig { - pub provider: Option, - pub preserve: Option, - pub devcontainer: Option, - pub local: Option, - pub daytona: Option, - pub env: Option>, -} - -impl TryFrom for SandboxSettings { - type Error = anyhow::Error; - - fn try_from(value: SandboxConfig) -> Result { - Ok(Self { - provider: value.provider, - preserve: value.preserve, - devcontainer: value.devcontainer, - local: value.local.map(Into::into), - daytona: value.daytona.map(TryInto::try_into).transpose()?, - env: value.env, - }) - } -} diff --git a/lib/crates/fabro-config/src/server.rs b/lib/crates/fabro-config/src/server.rs index aec1d4e25..fce1cd9a8 100644 --- a/lib/crates/fabro-config/src/server.rs +++ b/lib/crates/fabro-config/src/server.rs @@ -1,199 +1,23 @@ +//! Re-export shim for server settings types. +//! +//! Stage 3 removed the parse-time `*Config` types (`ApiConfig`, `GitConfig`, +//! etc.) in favor of the v2 parse tree in `fabro_types::settings::v2::server`. +//! This module stays alive as a pass-through for crates that still import +//! resolved server types via the legacy `fabro_config::server` path; +//! Stage 6 deletes it. + use std::path::PathBuf; -use anyhow::anyhow; -use serde::{Deserialize, Serialize}; - use fabro_types::Settings; + pub use fabro_types::settings::server::{ ApiAuthStrategy, ApiSettings, ArtifactStorageBackend, ArtifactStorageSettings, AuthProvider, AuthSettings, FeaturesSettings, GitAuthorSettings, GitProvider, GitSettings, LogSettings, SlackSettings, TlsSettings, WebSettings, WebhookSettings, WebhookStrategy, }; -#[derive(Debug, Clone, Default, Deserialize, PartialEq, Serialize, crate::Combine)] -pub struct AuthConfig { - pub provider: Option, - #[serde(default)] - pub allowed_usernames: Vec, -} - -impl From for AuthSettings { - fn from(value: AuthConfig) -> Self { - Self { - provider: value.provider.unwrap_or_default(), - allowed_usernames: value.allowed_usernames, - } - } -} - -#[derive(Debug, Clone, Default, Deserialize, PartialEq, Serialize, crate::Combine)] -pub struct TlsConfig { - pub cert: Option, - pub key: Option, - pub ca: Option, -} - -impl TryFrom for TlsSettings { - type Error = anyhow::Error; - - fn try_from(value: TlsConfig) -> Result { - Ok(Self { - cert: value - .cert - .ok_or_else(|| anyhow!("tls.cert is required when tls is configured"))?, - key: value - .key - .ok_or_else(|| anyhow!("tls.key is required when tls is configured"))?, - ca: value - .ca - .ok_or_else(|| anyhow!("tls.ca is required when tls is configured"))?, - }) - } -} - -#[derive(Debug, Clone, Default, Deserialize, PartialEq, Serialize, crate::Combine)] -pub struct ApiConfig { - pub base_url: Option, - #[serde(default)] - pub authentication_strategies: Vec, - pub tls: Option, -} - -fn default_base_url() -> String { - "http://localhost:3000/api/v1".to_string() -} - -impl TryFrom for ApiSettings { - type Error = anyhow::Error; - - fn try_from(value: ApiConfig) -> Result { - Ok(Self { - base_url: value.base_url.unwrap_or_else(default_base_url), - authentication_strategies: value.authentication_strategies, - tls: value.tls.map(TryInto::try_into).transpose()?, - }) - } -} - -#[derive(Debug, Clone, Default, Deserialize, PartialEq, Serialize, crate::Combine)] -pub struct GitAuthorConfig { - pub name: Option, - pub email: Option, -} - -impl From for GitAuthorSettings { - fn from(value: GitAuthorConfig) -> Self { - Self { - name: value.name, - email: value.email, - } - } -} - -#[derive(Debug, Clone, Default, Deserialize, PartialEq, Serialize, crate::Combine)] -pub struct WebhookConfig { - pub strategy: Option, -} - -impl TryFrom for WebhookSettings { - type Error = anyhow::Error; - - fn try_from(value: WebhookConfig) -> Result { - Ok(Self { - strategy: value - .strategy - .ok_or_else(|| anyhow!("git.webhooks.strategy is required"))?, - }) - } -} - -#[derive(Debug, Clone, Default, Deserialize, PartialEq, Serialize, crate::Combine)] -pub struct GitConfig { - pub provider: Option, - pub app_id: Option, - pub client_id: Option, - pub slug: Option, - pub author: Option, - pub webhooks: Option, -} - -impl TryFrom for GitSettings { - type Error = anyhow::Error; - - fn try_from(value: GitConfig) -> Result { - Ok(Self { - provider: value.provider.unwrap_or_default(), - app_id: value.app_id, - client_id: value.client_id, - slug: value.slug, - author: value.author.map(Into::into).unwrap_or_default(), - webhooks: value.webhooks.map(TryInto::try_into).transpose()?, - }) - } -} - -#[derive(Debug, Clone, Default, Deserialize, PartialEq, Serialize, crate::Combine)] -pub struct WebConfig { - pub enabled: Option, - pub url: Option, - pub auth: Option, -} - -fn default_web_url() -> String { - "http://localhost:3000".to_string() -} - -impl From for WebSettings { - fn from(value: WebConfig) -> Self { - Self { - enabled: value.enabled.unwrap_or(true), - url: value.url.unwrap_or_else(default_web_url), - auth: value.auth.map(Into::into).unwrap_or_default(), - } - } -} - -#[derive(Debug, Clone, Default, Deserialize, PartialEq, Serialize, crate::Combine)] -pub struct SlackConfig { - pub default_channel: Option, -} - -impl From for SlackSettings { - fn from(value: SlackConfig) -> Self { - Self { - default_channel: value.default_channel, - } - } -} - -#[derive(Debug, Clone, Default, Deserialize, PartialEq, Serialize, crate::Combine)] -pub struct FeaturesConfig { - pub session_sandboxes: Option, - /// Experimental: enable automatic retro generation after workflow runs. - pub retros: Option, -} - -impl From for FeaturesSettings { - fn from(value: FeaturesConfig) -> Self { - Self { - session_sandboxes: value.session_sandboxes.unwrap_or(false), - retros: value.retros.unwrap_or(false), - } - } -} - -#[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize, crate::Combine)] -pub struct LogConfig { - pub level: Option, -} - -impl From for LogSettings { - fn from(value: LogConfig) -> Self { - Self { level: value.level } - } -} - /// Resolve the storage directory: config value > default `~/.fabro`. +#[must_use] pub fn resolve_storage_dir(settings: &Settings) -> PathBuf { settings.storage_dir() } diff --git a/lib/crates/fabro-config/src/settings.rs b/lib/crates/fabro-config/src/settings.rs index 90369ef45..e6a0a341b 100644 --- a/lib/crates/fabro-config/src/settings.rs +++ b/lib/crates/fabro-config/src/settings.rs @@ -1,54 +1,5 @@ -use fabro_types::Settings; - -use crate::config::ConfigLayer; - -impl TryFrom for Settings { - type Error = anyhow::Error; - - fn try_from(value: ConfigLayer) -> Result { - Ok(Self { - version: value.version, - goal: value.goal, - goal_file: value.goal_file, - graph: value.graph, - labels: value.labels, - work_dir: value.work_dir, - llm: value.llm.map(Into::into), - setup: value.setup.map(Into::into), - sandbox: value.sandbox.map(TryInto::try_into).transpose()?, - vars: value.vars, - checkpoint: value.checkpoint.into(), - pull_request: value.pull_request.map(Into::into), - artifacts: value.artifacts.map(Into::into), - hooks: value.hooks, - mcp_servers: value.mcp_servers, - github: value.github.map(Into::into), - server: value.server.map(TryInto::try_into).transpose()?, - exec: value.exec.map(Into::into), - prevent_idle_sleep: value.prevent_idle_sleep, - verbose: value.verbose, - upgrade_check: value.upgrade_check, - dry_run: value.dry_run, - auto_approve: value.auto_approve, - no_retro: value.no_retro, - storage_dir: value.storage_dir, - max_concurrent_runs: value.max_concurrent_runs, - artifact_storage: value.artifact_storage, - web: value.web.map(Into::into), - slack: value.slack.map(Into::into), - api: value.api.map(TryInto::try_into).transpose()?, - features: value.features.map(Into::into), - log: value.log.map(Into::into), - git: value.git.map(TryInto::try_into).transpose()?, - fabro: value.fabro.map(Into::into), - }) - } -} - -impl TryFrom<&ConfigLayer> for Settings { - type Error = anyhow::Error; - - fn try_from(value: &ConfigLayer) -> Result { - value.clone().try_into() - } -} +//! Empty module retained for backwards-compatible imports. +//! +//! The legacy `TryFrom for Settings` impl was replaced by +//! [`crate::ConfigLayer::resolve`], which delegates to the v2 bridge in +//! `fabro_types::settings::v2::bridge`. Stage 6 deletes this file entirely. diff --git a/lib/crates/fabro-config/src/user.rs b/lib/crates/fabro-config/src/user.rs index 553c4935c..f72d3e0f3 100644 --- a/lib/crates/fabro-config/src/user.rs +++ b/lib/crates/fabro-config/src/user.rs @@ -1,10 +1,13 @@ +//! User config loading. +//! +//! Stage 3 removed the parse-time `ClientTlsConfig`/`ServerConfig`/`ExecConfig` +//! types; this module now only exposes machine-level settings loading plus +//! path helpers and a re-export of the resolved user-facing types. + use std::collections::HashSet; use std::path::{Path, PathBuf}; use std::sync::{Mutex, OnceLock}; -use anyhow::anyhow; -use serde::{Deserialize, Serialize}; - use crate::config::ConfigLayer; use crate::home::Home; @@ -20,67 +23,6 @@ pub const FABRO_CONFIG_ENV: &str = "FABRO_CONFIG"; static WARNED_LEGACY_USER_CONFIGS: OnceLock>> = OnceLock::new(); -#[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize, crate::Combine)] -pub struct ClientTlsConfig { - pub cert: Option, - pub key: Option, - pub ca: Option, -} - -impl TryFrom for ClientTlsSettings { - type Error = anyhow::Error; - - fn try_from(value: ClientTlsConfig) -> Result { - Ok(Self { - cert: value.cert.ok_or_else(|| { - anyhow!("server.tls.cert is required when server.tls is configured") - })?, - key: value.key.ok_or_else(|| { - anyhow!("server.tls.key is required when server.tls is configured") - })?, - ca: value.ca.ok_or_else(|| { - anyhow!("server.tls.ca is required when server.tls is configured") - })?, - }) - } -} - -#[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize, crate::Combine)] -pub struct ServerConfig { - pub target: Option, - pub tls: Option, -} - -impl TryFrom for ServerSettings { - type Error = anyhow::Error; - - fn try_from(value: ServerConfig) -> Result { - Ok(Self { - target: value.target, - tls: value.tls.map(TryInto::try_into).transpose()?, - }) - } -} - -#[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize, crate::Combine)] -pub struct ExecConfig { - pub provider: Option, - pub model: Option, - pub permissions: Option, - pub output_format: Option, -} - -impl From for ExecSettings { - fn from(value: ExecConfig) -> Self { - Self { - provider: value.provider, - model: value.model, - permissions: value.permissions, - output_format: value.output_format, - } - } -} - pub fn default_settings_path() -> PathBuf { Home::from_env().user_config() } @@ -126,15 +68,16 @@ fn should_warn_about_legacy_user_config(path: &Path) -> bool { .insert(path.to_path_buf()) } -/// Load settings config from an explicit path or `~/.fabro/settings.toml`, returning defaults if the -/// default file doesn't exist. An explicit path that doesn't exist is an error. +/// Load settings config from an explicit path or `~/.fabro/settings.toml`, +/// returning defaults if the default file doesn't exist. An explicit path that +/// doesn't exist is an error. #[allow(clippy::print_stderr)] pub fn load_settings_config(path: Option<&Path>) -> anyhow::Result { if let Some(explicit) = path .map(Path::to_path_buf) .or_else(|| std::env::var_os(FABRO_CONFIG_ENV).map(PathBuf::from)) { - return crate::load_config_file(Some(&explicit), SETTINGS_CONFIG_FILENAME); + return load_v2_layer_from_path(&explicit); } for legacy_path in [ @@ -155,7 +98,16 @@ pub fn load_settings_config(path: Option<&Path>) -> anyhow::Result } } - crate::load_config_file(None, SETTINGS_CONFIG_FILENAME) + let default = Home::from_env().root().join(SETTINGS_CONFIG_FILENAME); + if default.is_file() { + load_v2_layer_from_path(&default) + } else { + Ok(ConfigLayer::default()) + } +} + +fn load_v2_layer_from_path(path: &Path) -> anyhow::Result { + ConfigLayer::load(path) } #[cfg(test)] diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index 82b8ced87..2915e44ca 100644 --- a/lib/crates/fabro-server/src/run_manifest.rs +++ b/lib/crates/fabro-server/src/run_manifest.rs @@ -8,14 +8,18 @@ use fabro_config::ConfigLayer; use fabro_config::effective_settings; use fabro_config::effective_settings::{EffectiveSettingsLayers, EffectiveSettingsMode}; use fabro_config::project::resolve_working_directory; -use fabro_config::run::{LlmConfig, parse_run_config}; -use fabro_config::sandbox::{DockerfileSource, SandboxConfig}; +use fabro_config::run::parse_run_config; use fabro_graphviz::graph::{Graph, is_llm_handler_type}; use fabro_graphviz::render::apply_direction; use fabro_llm::Provider; use fabro_model::Catalog; use fabro_sandbox::daytona::DaytonaConfig; use fabro_sandbox::{DockerSandboxOptions, Sandbox, SandboxProvider, SandboxSpec}; +use fabro_types::settings::v2::interp::InterpString; +use fabro_types::settings::v2::run::{ + AgentPermissions, 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; @@ -200,7 +204,7 @@ fn parse_manifest_config(config: &types::ManifestConfig) -> Result let Some(source) = config.source.as_deref() else { return Ok(ConfigLayer::default()); }; - toml::from_str(source).map_err(Into::into) + ConfigLayer::parse(source) } fn manifest_args_layer(args: Option<&types::ManifestArgs>) -> ConfigLayer { @@ -208,28 +212,61 @@ fn manifest_args_layer(args: Option<&types::ManifestArgs>) -> ConfigLayer { return ConfigLayer::default(); }; - let llm = (args.model.is_some() || args.provider.is_some()).then(|| LlmConfig { - model: args.model.clone(), - provider: args.provider.clone(), - fallbacks: None, + let model = (args.model.is_some() || args.provider.is_some()).then(|| RunModelLayer { + provider: args.provider.as_deref().map(InterpString::parse), + name: args.model.as_deref().map(InterpString::parse), + fallbacks: Vec::new(), }); let sandbox = - (args.sandbox.is_some() || args.preserve_sandbox.is_some()).then(|| SandboxConfig { + (args.sandbox.is_some() || args.preserve_sandbox.is_some()).then(|| RunSandboxLayer { provider: args.sandbox.clone(), preserve: args.preserve_sandbox, - ..Default::default() + ..RunSandboxLayer::default() }); - ConfigLayer { - llm, + let execution_has_any = + args.dry_run.is_some() || args.auto_approve.is_some() || args.no_retro.is_some(); + let execution = execution_has_any.then(|| RunExecutionLayer { + mode: args + .dry_run + .map(|d| if d { RunMode::DryRun } else { RunMode::Normal }), + approval: args.auto_approve.map(|a| { + if a { + ApprovalMode::Auto + } else { + ApprovalMode::Prompt + } + }), + retros: args.no_retro.map(|nr| !nr), + }); + + let run_has_any = + model.is_some() || sandbox.is_some() || execution.is_some() || !args.label.is_empty(); + + let run = run_has_any.then(|| RunLayer { + model, sandbox, - verbose: args.verbose, - dry_run: args.dry_run, - auto_approve: args.auto_approve, - no_retro: args.no_retro, - labels: parse_labels(&args.label), - ..Default::default() + execution, + metadata: parse_labels(&args.label), + ..RunLayer::default() + }); + + let mut file = fabro_types::settings::v2::SettingsFile::default(); + if let Some(run) = run { + file.run = Some(run); } + + // Verbose is a CLI output-verbosity concern in v2, but manifest args + // are resolved server-side as run knobs too. For now we store it as a + // metadata key so Stage 4 consumers can pick it up via the bridge. + if let Some(verbose) = args.verbose { + file.run + .get_or_insert_with(RunLayer::default) + .metadata + .insert("fabro.verbose".into(), verbose.to_string()); + } + let _ = AgentPermissions::ReadOnly; // keep unused import alive until Stage 4 wires agent args + ConfigLayer::from(file) } fn parse_labels(labels: &[String]) -> HashMap { @@ -246,22 +283,27 @@ fn resolve_manifest_dockerfile( files: &HashMap, ) -> Result<()> { let source = layer - .sandbox + .as_v2_mut() + .run .as_mut() + .and_then(|run| run.sandbox.as_mut()) .and_then(|sandbox| sandbox.daytona.as_mut()) .and_then(|daytona| daytona.snapshot.as_mut()) .and_then(|snapshot| snapshot.dockerfile.as_mut()); - let Some(DockerfileSource::Path { path }) = source else { + let Some(DaytonaDockerfileLayer::Path { path }) = source else { return Ok(()); }; - let logical_path = - normalize_logical_path(config_path.parent().unwrap_or_else(|| Path::new(".")), path) - .ok_or_else(|| anyhow!("unsupported dockerfile reference: {path}"))?; + let path_owned = path.clone(); + let logical_path = normalize_logical_path( + config_path.parent().unwrap_or_else(|| Path::new(".")), + &path_owned, + ) + .ok_or_else(|| anyhow!("unsupported dockerfile reference: {path_owned}"))?; let content = files .get(&logical_path) .cloned() .ok_or_else(|| anyhow!("missing bundled dockerfile: {}", logical_path.display()))?; - *source.unwrap() = DockerfileSource::Inline(content); + *source.unwrap() = DaytonaDockerfileLayer::Inline(content); Ok(()) } diff --git a/lib/crates/fabro-test/src/lib.rs b/lib/crates/fabro-test/src/lib.rs index b058cc5bf..bc941f597 100644 --- a/lib/crates/fabro-test/src/lib.rs +++ b/lib/crates/fabro-test/src/lib.rs @@ -335,7 +335,7 @@ fn write_marker(root: &Path) { fn managed_storage_settings(storage_dir: &Path, rest: &str) -> String { format!( - "{MANAGED_STORAGE_MARKER}\nstorage_dir = \"{}\"\n{rest}", + "{MANAGED_STORAGE_MARKER}\n_version = 1\n\n[server.storage]\nroot = \"{}\"\n\n{rest}", storage_dir.display() ) } @@ -349,24 +349,19 @@ fn strip_managed_storage_settings(contents: &str) -> &str { .strip_prefix(MANAGED_STORAGE_MARKER) .and_then(|rest| rest.strip_prefix('\n')) .unwrap_or(""); - let (first_line, mut rest) = after_marker.split_once('\n').unwrap_or((after_marker, "")); - if !first_line.starts_with("storage_dir = ") { - return after_marker; - } - if let Some((maybe_target, tail)) = rest.split_once('\n') { - if maybe_target.starts_with("server.target = ") { - rest = tail; - } - } - rest + after_marker } fn settings_storage_dir(settings_path: &Path) -> Option { let content = std::fs::read_to_string(settings_path).ok()?; - let value = toml::from_str::(strip_managed_storage_settings(&content)).ok()?; + let stripped = strip_managed_storage_settings(&content); + let value = toml::from_str::(stripped).ok()?; value - .get("storage_dir") - .or_else(|| value.get("data_dir")) + .get("server") + .and_then(toml::Value::as_table) + .and_then(|server| server.get("storage")) + .and_then(toml::Value::as_table) + .and_then(|storage| storage.get("root")) .and_then(toml::Value::as_str) .map(PathBuf::from) } @@ -379,7 +374,10 @@ fn write_settings_file(path: &Path, storage_dir: &Path, rest: &str) { ensure_parent_dir(path); std::fs::write( path, - format!("storage_dir = \"{}\"\n{rest}", storage_dir.display()), + format!( + "_version = 1\n\n[server.storage]\nroot = \"{}\"\n\n{rest}", + storage_dir.display() + ), ) .unwrap_or_else(|err| panic!("failed to write {}: {err}", path.display())); } @@ -407,36 +405,45 @@ fn write_settings_table(path: &Path, table: &TomlMap) { fn server_target_from_table(table: &TomlMap) -> Option { table - .get("server") + .get("cli") .and_then(TomlValue::as_table) - .and_then(|server| server.get("target")) + .and_then(|cli| cli.get("target")) + .and_then(TomlValue::as_table) + .and_then(|target| target.get("path").or_else(|| target.get("url"))) .and_then(TomlValue::as_str) .map(ToOwned::to_owned) } fn set_server_target(table: &mut TomlMap, socket_path: &Path) { - let server_entry = table - .entry("server".to_string()) + let cli_entry = table + .entry("cli".to_string()) .or_insert_with(|| TomlValue::Table(TomlMap::new())); - let Some(server_table) = server_entry.as_table_mut() else { - panic!("expected [server] to be a TOML table"); + let Some(cli_table) = cli_entry.as_table_mut() else { + panic!("expected [cli] to be a TOML table"); }; - server_table.insert( - "target".to_string(), + let target_entry = cli_table + .entry("target".to_string()) + .or_insert_with(|| TomlValue::Table(TomlMap::new())); + let Some(target_table) = target_entry.as_table_mut() else { + panic!("expected [cli.target] to be a TOML table"); + }; + target_table.insert("type".to_string(), TomlValue::String("unix".to_string())); + target_table.insert( + "path".to_string(), TomlValue::String(socket_path.display().to_string()), ); } fn clear_server_target(table: &mut TomlMap) { - let Some(server_entry) = table.get_mut("server") else { + let Some(cli_entry) = table.get_mut("cli") else { return; }; - let Some(server_table) = server_entry.as_table_mut() else { + let Some(cli_table) = cli_entry.as_table_mut() else { return; }; - server_table.remove("target"); - if server_table.is_empty() { - table.remove("server"); + cli_table.remove("target"); + if cli_table.is_empty() { + table.remove("cli"); } } @@ -451,8 +458,8 @@ fn sync_home_settings( Ok(contents) => { let had_managed_storage = contents.starts_with(MANAGED_STORAGE_MARKER); let table = parse_settings_table(&contents, settings_path); - let had_explicit_storage = !had_managed_storage - && (table.contains_key("storage_dir") || table.contains_key("data_dir")); + let had_explicit_storage = + !had_managed_storage && has_explicit_storage_root(&table); let had_explicit_target = server_target_from_table(&table).is_some(); (table, had_explicit_storage, had_explicit_target) } @@ -462,12 +469,12 @@ fn sync_home_settings( Err(err) => panic!("failed to read {}: {err}", settings_path.display()), }; + table + .entry("_version".to_string()) + .or_insert(TomlValue::Integer(1)); + if !had_explicit_storage { - table.insert( - "storage_dir".to_string(), - TomlValue::String(storage_dir.display().to_string()), - ); - table.remove("data_dir"); + set_server_storage_root(&mut table, storage_dir); } if force_server_target || (!had_explicit_storage && !had_explicit_target) { @@ -478,21 +485,23 @@ fn sync_home_settings( if !had_explicit_storage { let mut rest = table.clone(); - rest.remove("storage_dir"); + clear_server_storage(&mut rest); + rest.remove("_version"); let managed_target = !had_explicit_target && !force_server_target; if managed_target { clear_server_target(&mut rest); } - let rest = toml::to_string(&rest) + let rest_toml = toml::to_string(&rest) .unwrap_or_else(|err| panic!("failed to serialize {}: {err}", settings_path.display())); - let mut contents = managed_storage_settings(storage_dir, &rest); - if managed_target { - contents = format!( - "{MANAGED_STORAGE_MARKER}\nstorage_dir = \"{}\"\nserver.target = \"{}\"\n{rest}", + let contents = if managed_target { + format!( + "{MANAGED_STORAGE_MARKER}\n_version = 1\n\n[server.storage]\nroot = \"{}\"\n\n[cli.target]\ntype = \"unix\"\npath = \"{}\"\n\n{rest_toml}", storage_dir.display(), socket_path.display() - ); - } + ) + } else { + managed_storage_settings(storage_dir, &rest_toml) + }; ensure_parent_dir(settings_path); std::fs::write(settings_path, contents) .unwrap_or_else(|err| panic!("failed to write {}: {err}", settings_path.display())); @@ -502,6 +511,48 @@ fn sync_home_settings( write_settings_table(settings_path, &table); } +fn has_explicit_storage_root(table: &TomlMap) -> bool { + table + .get("server") + .and_then(TomlValue::as_table) + .and_then(|server| server.get("storage")) + .and_then(TomlValue::as_table) + .and_then(|storage| storage.get("root")) + .is_some() +} + +fn set_server_storage_root(table: &mut TomlMap, storage_dir: &Path) { + let server_entry = table + .entry("server".to_string()) + .or_insert_with(|| TomlValue::Table(TomlMap::new())); + let Some(server_table) = server_entry.as_table_mut() else { + panic!("expected [server] to be a TOML table"); + }; + let storage_entry = server_table + .entry("storage".to_string()) + .or_insert_with(|| TomlValue::Table(TomlMap::new())); + let Some(storage_table) = storage_entry.as_table_mut() else { + panic!("expected [server.storage] to be a TOML table"); + }; + storage_table.insert( + "root".to_string(), + TomlValue::String(storage_dir.display().to_string()), + ); +} + +fn clear_server_storage(table: &mut TomlMap) { + let Some(server_entry) = table.get_mut("server") else { + return; + }; + let Some(server_table) = server_entry.as_table_mut() else { + return; + }; + server_table.remove("storage"); + if server_table.is_empty() { + table.remove("server"); + } +} + fn server_record_path(storage_dir: &Path) -> PathBuf { storage_dir.join("server.json") }