From ea206e0e40855ff28d8207e28261404802d2b484 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 9 Apr 2026 16:12:59 -0400 Subject: [PATCH] feat(settings): stage 6.2 delete bridge_to_old seam bridge.rs (818 LOC) is gone. Production consumers no longer produce a full legacy `Settings` from v2 state; every read path walks the v2 tree directly or uses one of the narrow v2->runtime helpers in the new `settings::v2::to_runtime` module. Core moves: fabro-types - Delete `settings::v2::bridge::bridge_to_old` and the whole bridge.rs file. - Relocate the narrow v2->runtime helpers (`bridge_sandbox`, `bridge_mcp_entry`, `bridge_mcps`, `bridge_hook`, `bridge_worktree_mode`, `bridge_merge_strategy`, `bridge_pull_request`, `bridge_run_artifacts`) into a new `settings::v2::to_runtime` module. Each helper takes a single v2 subtree and produces the corresponding runtime shape; nothing assembles a full legacy `Settings` anymore. - `settings/mod.rs` doc comment rewritten to describe `Settings` as a runtime shape, not a resolved parse target. Stage 6.3 deletes it. fabro-config - `ConfigLayer::resolve` is gone along with the `TryFrom for Settings` impls. Consumers call `.into()` for a `SettingsFile`, or `.as_v2()` to borrow one. - `fabro_config::server::resolve_storage_dir` now takes `&SettingsFile`. fabro-server - `api_server_settings` emits the v2 `SettingsFile` JSON shape directly instead of bridging to the legacy flat DTO. Stage 6.6 replaces the shape again with an explicit allow-list DTO. - `serve.rs`: `load_settings` returns `SettingsFile`; `apply_serve_overrides` / `apply_runtime_settings` mutate v2 subtrees directly; `build_artifact_object_store` walks `server.artifacts`; `build_legacy_api_settings` projects the v2 auth/listen/api subtrees down to the legacy `ApiSettings` shape for the (still-legacy) auth resolver. - `diagnostics::check_crypto` walks `server.auth.api.{jwt,mtls}` and `server.listen.tls` directly. - `web_auth.rs` oauth / register / setup-status / auth-me flows all read `server.web`, `server.integrations.github`, and `server.auth.web` directly via the v2 accessors. `merge_settings_keys` now writes v2 TOML (with `[server.web]`, `[server.integrations.github]`, etc.) instead of the legacy v1 top-level keys, and the register handler re-parses the freshly-written file back into the in-memory `SettingsFile` state. fabro-cli - `CommandContext::machine_settings` returns `&SettingsFile`. - `user_config::load_settings` and friends return `SettingsFile`. - `user_config::resolve_server_target` / `exec_server_target` / `configured_server_target` walk `cli.target.{http,unix}` directly. Tests rewritten against v2 TOML fixtures. - `main.rs` logging init reads `cli.logging.level` / `server.logging.level` via v2 accessors. - `commands/exec.rs` reads `cli.exec.{model,agent}` and builds mcps from `cli.exec.agent.mcps` (falling back to `run.agent.mcps`) via `to_runtime::bridge_mcp_entry`. - `commands/pr/mod.rs` calls `github_app_id_str()`. - `commands/run/create.rs` drops the legacy `.resolve()` call and uses `Into::::into(...)`. - `commands/config/mod.rs::legacy_settings_to_v2` is now a real reverse-mapping helper that covers `storage`, `scheduler`, `integrations.{github,slack}`, `run.model`, `run.inputs`, and `cli.output.verbosity`. Stage 6.6 deletes it when the API client returns v2 natively. - `tests/it/cmd/config.rs` tests now walk the v2 tree directly (via `cfg.run_model_name_str()`, `cfg.run_inputs()`, `cfg.run_sandbox()`, `cfg.run_hooks()`, `cfg.run_agent_mcps()`, `cfg.run_prepare_commands()`, `cfg.server_storage_root_str()`, etc.). The `bridge_to_old` test helper is gone. - `tests/it/api/settings.rs` asserts against the v2 JSON shape. Build, test, and quality gates all green: - `cargo build --workspace --tests` - `cargo clippy --workspace -- -D warnings` - `cargo fmt --check --all` - `cargo nextest run --workspace`: 3758 / 3758 passed, 182 skipped. Co-Authored-By: Claude Opus 4.6 (1M context) --- lib/crates/fabro-cli/src/command_context.rs | 6 +- lib/crates/fabro-cli/src/commands/exec.rs | 50 +- lib/crates/fabro-cli/src/commands/pr/mod.rs | 3 +- .../fabro-cli/src/commands/run/create.rs | 5 +- lib/crates/fabro-cli/src/main.rs | 26 +- lib/crates/fabro-cli/src/server_client.rs | 6 +- lib/crates/fabro-cli/src/user_config.rs | 179 ++-- lib/crates/fabro-cli/tests/it/cmd/config.rs | 152 ++-- lib/crates/fabro-config/src/config.rs | 43 +- lib/crates/fabro-config/src/server.rs | 6 +- lib/crates/fabro-server/src/diagnostics.rs | 60 +- lib/crates/fabro-server/src/run_manifest.rs | 2 +- lib/crates/fabro-server/src/serve.rs | 157 +++- lib/crates/fabro-server/src/server.rs | 22 +- lib/crates/fabro-server/src/web_auth.rs | 333 +++---- .../fabro-server/tests/it/api/settings.rs | 10 +- lib/crates/fabro-types/src/settings/mod.rs | 19 +- .../fabro-types/src/settings/v2/bridge.rs | 836 ------------------ lib/crates/fabro-types/src/settings/v2/mod.rs | 4 +- .../fabro-types/src/settings/v2/to_runtime.rs | 325 +++++++ .../fabro-workflow/src/operations/start.rs | 4 +- 21 files changed, 901 insertions(+), 1347 deletions(-) delete mode 100644 lib/crates/fabro-types/src/settings/v2/bridge.rs create mode 100644 lib/crates/fabro-types/src/settings/v2/to_runtime.rs diff --git a/lib/crates/fabro-cli/src/command_context.rs b/lib/crates/fabro-cli/src/command_context.rs index 8c33954d0..cacae3ef6 100644 --- a/lib/crates/fabro-cli/src/command_context.rs +++ b/lib/crates/fabro-cli/src/command_context.rs @@ -2,7 +2,7 @@ use std::path::{Path, PathBuf}; use std::sync::Arc; use anyhow::{Context as _, Result, bail}; -use fabro_types::Settings; +use fabro_types::settings::v2::SettingsFile; use tokio::sync::OnceCell; use crate::args::{ServerConnectionArgs, ServerTargetArgs}; @@ -24,7 +24,7 @@ pub(crate) enum ServerMode { pub(crate) struct CommandContext { cwd: PathBuf, base_config_path: PathBuf, - machine_settings: Settings, + machine_settings: SettingsFile, server_mode: ServerMode, server: OnceCell>, } @@ -75,7 +75,7 @@ impl CommandContext { &self.base_config_path } - pub(crate) fn machine_settings(&self) -> &Settings { + pub(crate) fn machine_settings(&self) -> &SettingsFile { &self.machine_settings } diff --git a/lib/crates/fabro-cli/src/commands/exec.rs b/lib/crates/fabro-cli/src/commands/exec.rs index 0f57b820c..04d6fa619 100644 --- a/lib/crates/fabro-cli/src/commands/exec.rs +++ b/lib/crates/fabro-cli/src/commands/exec.rs @@ -1,9 +1,10 @@ use anyhow::Result; use fabro_agent::cli::{OutputFormat, run_with_args, run_with_args_and_client}; -use fabro_config::mcp::McpServerEntry; use fabro_llm::client::Client; use fabro_llm::providers::FabroServerAdapter; use fabro_mcp::config::McpServerSettings; +use fabro_types::settings::v2::InterpString; +use fabro_types::settings::v2::to_runtime::bridge_mcp_entry; use std::collections::HashMap; use std::sync::Arc; @@ -11,25 +12,52 @@ use crate::args::{ExecArgs, GlobalArgs}; use crate::user_config; pub(crate) async fn execute(mut args: ExecArgs, globals: &GlobalArgs) -> Result<()> { + use fabro_agent::cli::PermissionLevel as AgentPermissionLevel; + use fabro_types::settings::v2::run::AgentPermissions; + let cli_settings = user_config::load_settings()?; #[cfg(feature = "sleep_inhibitor")] let _sleep_guard = crate::sleep_inhibitor::guard(cli_settings.prevent_idle_sleep_enabled()); - let exec_defaults = cli_settings.exec.as_ref(); + let exec_defaults = cli_settings.cli_exec(); + let exec_model = exec_defaults.and_then(|e| e.model.as_ref()); + let exec_agent = exec_defaults.and_then(|e| e.agent.as_ref()); + let provider_str = exec_model + .and_then(|m| m.provider.as_ref()) + .map(InterpString::as_source); + let model_str = exec_model + .and_then(|m| m.name.as_ref()) + .map(InterpString::as_source); + let permissions = exec_agent + .and_then(|agent| agent.permissions) + .map(|p| match p { + AgentPermissions::ReadOnly => AgentPermissionLevel::ReadOnly, + AgentPermissions::ReadWrite => AgentPermissionLevel::ReadWrite, + AgentPermissions::Full => AgentPermissionLevel::Full, + }); args.agent.apply_cli_defaults( - exec_defaults.and_then(|a| a.provider.as_deref()), - exec_defaults.and_then(|a| a.model.as_deref()), - exec_defaults.and_then(|a| a.permissions), - exec_defaults.and_then(|a| a.output_format), + provider_str.as_deref(), + model_str.as_deref(), + permissions, + None, ); if globals.json { args.agent.output_format = Some(OutputFormat::Json); } let server_target = user_config::exec_server_target(&args.server, &cli_settings)?; - let mcp_servers: Vec = cli_settings - .mcp_servers - .into_iter() - .map(|(name, entry): (String, McpServerEntry)| entry.into_config(name)) - .collect(); + // v2 MCPs live under `cli.exec.agent.mcps` (owner-specific) or + // `run.agent.mcps`. For `fabro exec` we use the cli.exec path, falling + // back to run.agent.mcps if unset. + let mcps_iter = exec_agent + .map(|a| &a.mcps) + .filter(|m| !m.is_empty()) + .or_else(|| cli_settings.run_agent_mcps()); + let mcp_servers: Vec = mcps_iter + .map(|mcps| { + mcps.iter() + .map(|(name, entry)| bridge_mcp_entry(entry).into_config(name.clone())) + .collect() + }) + .unwrap_or_default(); if let Some(target) = server_target { tracing::info!(transport = "server", "Agent session starting"); let provider_name = args diff --git a/lib/crates/fabro-cli/src/commands/pr/mod.rs b/lib/crates/fabro-cli/src/commands/pr/mod.rs index 1ac56df86..5f55dc607 100644 --- a/lib/crates/fabro-cli/src/commands/pr/mod.rs +++ b/lib/crates/fabro-cli/src/commands/pr/mod.rs @@ -15,7 +15,8 @@ use crate::shared::github::build_github_app_credentials; pub(crate) async fn dispatch(ns: PrNamespace, globals: &GlobalArgs) -> Result<()> { let ctx = CommandContext::base()?; - let github_app = build_github_app_credentials(ctx.machine_settings().app_id())?; + let github_app = + build_github_app_credentials(ctx.machine_settings().github_app_id_str().as_deref())?; match ns.command { PrCommand::Create(args) => { Box::pin(create::create_command(args, github_app, globals)).await diff --git a/lib/crates/fabro-cli/src/commands/run/create.rs b/lib/crates/fabro-cli/src/commands/run/create.rs index d5e6b3f03..b3278f2f2 100644 --- a/lib/crates/fabro-cli/src/commands/run/create.rs +++ b/lib/crates/fabro-cli/src/commands/run/create.rs @@ -5,6 +5,7 @@ use crate::command_context::CommandContext; use fabro_config::ConfigLayer; use fabro_config::Storage; use fabro_types::RunId; +use fabro_types::settings::v2::SettingsFile; use fabro_util::terminal::Styles; use super::output::{api_diagnostics_to_local, print_preflight_workflow_summary}; @@ -32,11 +33,11 @@ pub(crate) async fn create_run( .ok_or_else(|| anyhow::anyhow!("--workflow is required"))?; let cli_args_config = ConfigLayer::try_from(args)?; let cwd = ctx.cwd().to_path_buf(); - let _settings = cli_args_config + let _settings: SettingsFile = cli_args_config .clone() .combine(ConfigLayer::for_workflow(workflow_path, &cwd)?) .combine(cli_defaults) - .resolve(); + .into(); let run_id = args .run_id .as_deref() diff --git a/lib/crates/fabro-cli/src/main.rs b/lib/crates/fabro-cli/src/main.rs index a151a8eac..aa1a3adb9 100644 --- a/lib/crates/fabro-cli/src/main.rs +++ b/lib/crates/fabro-cli/src/main.rs @@ -130,19 +130,27 @@ async fn main_inner() -> (String, Result<()>) { }), }) = command.as_ref() { - match load_settings_config(args.config.as_deref()) - .and_then(fabro_types::Settings::try_from) - { - Ok(server_settings) => ( - server_settings.log.as_ref().and_then(|l| l.level.clone()), - false, - ), + match load_settings_config(args.config.as_deref()) { + Ok(layer) => { + use fabro_types::settings::v2::SettingsFile; + let server_settings: SettingsFile = layer.into(); + ( + server_settings + .server_logging() + .and_then(|l| l.level.clone()), + false, + ) + } Err(err) => return (command_name, Err(err)), } } else { match user_config::load_settings() { Ok(cli_settings) => ( - cli_settings.log.as_ref().and_then(|l| l.level.clone()), + cli_settings + .cli + .as_ref() + .and_then(|c| c.logging.as_ref()) + .and_then(|l| l.level.clone()), cli_settings.upgrade_check_enabled(), ), Err(err) => return (command_name, Err(err)), @@ -195,7 +203,7 @@ async fn main_inner() -> (String, Result<()>) { Commands::RunsCmd(cmd) => commands::runs::dispatch(cmd, &globals).await?, Commands::Model { command } => commands::model::execute(command, &globals).await?, Commands::Server(ns) => { - commands::server::dispatch(ns.command, &globals).await?; + Box::pin(commands::server::dispatch(ns.command, &globals)).await?; } Commands::Doctor(args) => { let cli_settings = user_config::load_settings()?; diff --git a/lib/crates/fabro-cli/src/server_client.rs b/lib/crates/fabro-cli/src/server_client.rs index 410331208..5d83052fe 100644 --- a/lib/crates/fabro-cli/src/server_client.rs +++ b/lib/crates/fabro-cli/src/server_client.rs @@ -8,7 +8,9 @@ use bytes::Bytes; use fabro_api::types; use fabro_server::bind::Bind; use fabro_store::{EventEnvelope, RunSummary, StageId}; -use fabro_types::{RunBlobId, RunEvent, RunId, Settings}; +use fabro_types::Settings; +use fabro_types::settings::v2::SettingsFile; +use fabro_types::{RunBlobId, RunEvent, RunId}; use fabro_workflow::artifact_snapshot::CapturedArtifactInfo; use futures::StreamExt; use reqwest::header::{CONTENT_LENGTH, CONTENT_TYPE}; @@ -99,7 +101,7 @@ pub(crate) async fn connect_server_target_direct(target: &str) -> Result Result { let target = user_config::resolve_server_target(args, settings)?; diff --git a/lib/crates/fabro-cli/src/user_config.rs b/lib/crates/fabro-cli/src/user_config.rs index 269af4198..98384ef4e 100644 --- a/lib/crates/fabro-cli/src/user_config.rs +++ b/lib/crates/fabro-cli/src/user_config.rs @@ -4,13 +4,13 @@ pub(crate) use fabro_config::user::*; use anyhow::{Result, bail}; use fabro_config::ConfigLayer; -use fabro_types::Settings; +use fabro_types::settings::v2::SettingsFile; use fabro_util::version::FABRO_VERSION; use tracing::debug; use crate::args::ServerTargetArgs; -pub(crate) fn load_settings() -> anyhow::Result { +pub(crate) fn load_settings() -> anyhow::Result { load_settings_with_config_and_storage_dir(None, None) } @@ -30,15 +30,15 @@ pub(crate) fn settings_layer_with_storage_dir( pub(crate) fn load_settings_with_storage_dir( storage_dir: Option<&Path>, -) -> anyhow::Result { - Ok(settings_layer_with_storage_dir(storage_dir)?.resolve()) +) -> anyhow::Result { + Ok(settings_layer_with_storage_dir(storage_dir)?.into()) } pub(crate) fn load_settings_with_config_and_storage_dir( config_path: Option<&Path>, storage_dir: Option<&Path>, -) -> anyhow::Result { - Ok(settings_layer_with_config_and_storage_dir(config_path, storage_dir)?.resolve()) +) -> anyhow::Result { + Ok(settings_layer_with_config_and_storage_dir(config_path, storage_dir)?.into()) } pub(crate) fn apply_storage_dir_override( @@ -68,21 +68,39 @@ pub(crate) enum ServerTarget { UnixSocket(PathBuf), } -fn configured_server_target(settings: &Settings) -> Result> { - settings - .server - .as_ref() - .and_then(|server| server.target.as_deref()) - .map(|value| { - parse_server_target( - value, - settings - .server - .as_ref() - .and_then(|server| server.tls.clone()), - ) - }) - .transpose() +/// Pull the CLI target configuration out of the v2 `[cli.target]` stanza. +/// Returns `(target_string, tls)` where `target_string` is either an +/// http(s) URL or a unix socket path. `tls` is the CLI-side client TLS +/// settings extracted from `[cli.target.http.tls]`. +fn cli_target_from_v2(settings: &SettingsFile) -> Option<(String, Option)> { + use fabro_types::settings::v2::cli::CliTargetLayer; + use fabro_types::settings::v2::interp::InterpString; + + let target = settings.cli.as_ref()?.target.as_ref()?; + match target { + CliTargetLayer::Http { url, tls } => { + let url_str = url.as_ref().map(InterpString::as_source)?; + let tls_settings = tls.as_ref().and_then(|tls| { + Some(ClientTlsSettings { + cert: PathBuf::from(tls.cert.as_ref().map(InterpString::as_source)?), + key: PathBuf::from(tls.key.as_ref().map(InterpString::as_source)?), + ca: PathBuf::from(tls.ca.as_ref().map(InterpString::as_source)?), + }) + }); + Some((url_str, tls_settings)) + } + CliTargetLayer::Unix { path } => path + .as_ref() + .map(InterpString::as_source) + .map(|path_str| (path_str, None)), + } +} + +fn configured_server_target(settings: &SettingsFile) -> Result> { + let Some((value, tls)) = cli_target_from_v2(settings) else { + return Ok(None); + }; + parse_server_target(&value, tls).map(Some) } pub(crate) fn default_server_target() -> ServerTarget { @@ -107,24 +125,18 @@ fn parse_server_target(value: &str, tls: Option) -> Result Result> { args.as_deref() .map(|value| { - parse_server_target( - value, - settings - .server - .as_ref() - .and_then(|server| server.tls.clone()), - ) + parse_server_target(value, cli_target_from_v2(settings).and_then(|(_, tls)| tls)) }) .transpose() } pub(crate) fn resolve_server_target( args: &ServerTargetArgs, - settings: &Settings, + settings: &SettingsFile, ) -> Result { explicit_server_target(args, settings)? .or(configured_server_target(settings)?) @@ -133,7 +145,7 @@ pub(crate) fn resolve_server_target( pub(crate) fn exec_server_target( args: &ServerTargetArgs, - settings: &Settings, + settings: &SettingsFile, ) -> Result> { let target = explicit_server_target(args, settings)?; debug!(?target, "Resolved exec server target"); @@ -186,9 +198,15 @@ mod tests { } } + fn parse_v2(source: &str) -> SettingsFile { + fabro_config::ConfigLayer::parse(source) + .expect("fixture should parse") + .into() + } + #[test] fn exec_has_no_server_target_by_default() { - let settings = Settings::default(); + let settings = SettingsFile::default(); assert_eq!( exec_server_target(&server_target_args(None), &settings).unwrap(), None @@ -197,7 +215,7 @@ mod tests { #[test] fn exec_uses_cli_server_target() { - let settings = Settings::default(); + let settings = SettingsFile::default(); assert_eq!( exec_server_target( &server_target_args(Some("https://cli.example.com")), @@ -213,7 +231,7 @@ mod tests { #[test] fn exec_supports_explicit_unix_socket_target() { - let settings = Settings::default(); + let settings = SettingsFile::default(); assert_eq!( exec_server_target(&server_target_args(Some("/tmp/fabro.sock")), &settings).unwrap(), Some(ServerTarget::UnixSocket(PathBuf::from("/tmp/fabro.sock"))) @@ -222,13 +240,15 @@ mod tests { #[test] fn exec_ignores_configured_server_target_without_cli_override() { - let settings = Settings { - server: Some(ServerSettings { - target: Some("https://config.example.com".to_string()), - tls: None, - }), - ..Settings::default() - }; + let settings = parse_v2( + r#" +_version = 1 + +[cli.target] +type = "http" +url = "https://config.example.com" +"#, + ); assert_eq!( exec_server_target(&server_target_args(None), &settings).unwrap(), None @@ -237,13 +257,15 @@ mod tests { #[test] fn resolve_server_target_uses_configured_server_target() { - let settings = Settings { - server: Some(ServerSettings { - target: Some("https://config.example.com".to_string()), - tls: None, - }), - ..Settings::default() - }; + let settings = parse_v2( + r#" +_version = 1 + +[cli.target] +type = "http" +url = "https://config.example.com" +"#, + ); assert_eq!( resolve_server_target(&server_target_args(None), &settings).unwrap(), ServerTarget::HttpUrl { @@ -255,13 +277,15 @@ mod tests { #[test] fn resolve_server_target_explicit_target_overrides_config_target() { - let settings = Settings { - server: Some(ServerSettings { - target: Some("https://config.example.com".to_string()), - tls: None, - }), - ..Settings::default() - }; + let settings = parse_v2( + r#" +_version = 1 + +[cli.target] +type = "http" +url = "https://config.example.com" +"#, + ); assert_eq!( resolve_server_target( &server_target_args(Some("https://cli.example.com")), @@ -277,7 +301,7 @@ mod tests { #[test] fn resolve_server_target_defaults_to_default_unix_socket_target() { - let settings = Settings::default(); + let settings = SettingsFile::default(); assert_eq!( resolve_server_target(&server_target_args(None), &settings).unwrap(), ServerTarget::UnixSocket(dirs::home_dir().unwrap().join(".fabro/fabro.sock")) @@ -286,13 +310,15 @@ mod tests { #[test] fn explicit_server_target_overrides_config_target() { - let settings = Settings { - server: Some(ServerSettings { - target: Some("https://config.example.com".to_string()), - tls: None, - }), - ..Settings::default() - }; + let settings = parse_v2( + r#" +_version = 1 + +[cli.target] +type = "http" +url = "https://config.example.com" +"#, + ); assert_eq!( resolve_server_target( &server_target_args(Some("https://cli.example.com")), @@ -308,18 +334,25 @@ mod tests { #[test] fn remote_target_uses_tls_from_config() { - let tls = ClientTlsSettings { + let expected_tls = ClientTlsSettings { cert: PathBuf::from("cert.pem"), key: PathBuf::from("key.pem"), ca: PathBuf::from("ca.pem"), }; - let settings = Settings { - server: Some(ServerSettings { - target: None, - tls: Some(tls.clone()), - }), - ..Settings::default() - }; + let settings = parse_v2( + r#" +_version = 1 + +[cli.target] +type = "http" +url = "https://config.example.com" + +[cli.target.tls] +cert = "cert.pem" +key = "key.pem" +ca = "ca.pem" +"#, + ); assert_eq!( exec_server_target( &server_target_args(Some("https://cli.example.com")), @@ -328,14 +361,14 @@ mod tests { .unwrap(), Some(ServerTarget::HttpUrl { api_url: "https://cli.example.com".to_string(), - tls: Some(tls), + tls: Some(expected_tls), }) ); } #[test] fn invalid_server_target_is_rejected() { - let settings = Settings::default(); + let settings = SettingsFile::default(); let error = exec_server_target(&server_target_args(Some("fabro.internal")), &settings).unwrap_err(); assert_eq!( diff --git a/lib/crates/fabro-cli/tests/it/cmd/config.rs b/lib/crates/fabro-cli/tests/it/cmd/config.rs index 9cf7fbe99..fe90025c4 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/config.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/config.rs @@ -1,6 +1,5 @@ use std::path::PathBuf; -use fabro_config::mcp::McpTransport; use fabro_test::{fabro_snapshot, test_context}; use fabro_types::Settings; use fabro_types::settings::v2::SettingsFile; @@ -32,14 +31,8 @@ fn old_config_show_command_is_rejected() { // Helpers // --------------------------------------------------------------------------- -fn parse_settings(stdout: &[u8]) -> Settings { - // The `settings` command now emits a v2 SettingsFile as YAML. Bridge - // it down to the legacy flat shape so the existing test assertions - // (which use flat fields like `cfg.llm`, `cfg.sandbox`, etc.) keep - // working. Stage 6.6 will rewrite these tests against the v2 tree. - let file: SettingsFile = - serde_yaml::from_slice(stdout).expect("stdout should be valid YAML SettingsFile"); - fabro_types::settings::v2::bridge::bridge_to_old(&file) +fn parse_settings(stdout: &[u8]) -> SettingsFile { + serde_yaml::from_slice(stdout).expect("stdout should be valid YAML SettingsFile") } fn server_settings_fixture() -> Settings { @@ -315,30 +308,25 @@ fn settings_local_merges_cli_and_project_defaults() { .clone(); let cfg = parse_settings(&output); - let llm = cfg.llm.as_ref().expect("llm config"); - assert_eq!(llm.model.as_deref(), Some("project-model")); - assert_eq!(llm.provider.as_deref(), Some("openai")); - assert_eq!(cfg.goal.as_deref(), None); - assert_eq!(cfg.fabro.as_ref().map(|f| f.root.as_str()), Some("fabro")); + assert_eq!(cfg.run_model_name_str().as_deref(), Some("project-model")); + assert_eq!(cfg.run_model_provider_str().as_deref(), Some("openai")); + assert_eq!(cfg.run_goal_str().as_deref(), None); + assert_eq!(cfg.project_directory(), Some("fabro")); // v2 R22: run.inputs replaces the inherited map wholesale rather than // merging by key, so the project layer wipes out the CLI layer's inputs. - let vars = cfg.vars.as_ref().expect("vars"); - assert_eq!(vars.get("project_only").map(String::as_str), Some("1")); - assert_eq!(vars.get("shared").map(String::as_str), Some("project")); + let vars = cfg.run_inputs().expect("run.inputs"); + assert_eq!(vars.get("project_only").and_then(|v| v.as_str()), Some("1")); + assert_eq!(vars.get("shared").and_then(|v| v.as_str()), Some("project")); assert!( - vars.get("cli_only").is_none(), + !vars.contains_key("cli_only"), "run.inputs should replace across layers, not merge by key" ); // v2 R71: provider-native maps such as run.sandbox.daytona.labels remain // sticky merge-by-key, so CLI labels persist under the project layer. - let sandbox = cfg.sandbox.as_ref().expect("sandbox"); - let labels = sandbox - .daytona - .as_ref() - .and_then(|d| d.labels.as_ref()) - .expect("daytona labels"); + let sandbox = cfg.run_sandbox().expect("run.sandbox"); + let labels = &sandbox.daytona.as_ref().expect("daytona").labels; assert_eq!(labels.get("cli_only").map(String::as_str), Some("1")); assert_eq!(labels.get("shared").map(String::as_str), Some("cli")); } @@ -358,61 +346,80 @@ fn settings_local_workflow_name_applies_run_overlay_and_deep_merges() { .stdout .clone(); + use fabro_types::settings::v2::run::McpEntryLayer; + let cfg = parse_settings(&output); - let llm = cfg.llm.as_ref().expect("llm config"); - assert_eq!(cfg.goal.as_deref(), Some("demo goal")); - assert_eq!(llm.model.as_deref(), Some("run-model")); - assert_eq!(llm.provider.as_deref(), Some("anthropic")); + assert_eq!(cfg.run_goal_str().as_deref(), Some("demo goal")); + assert_eq!(cfg.run_model_name_str().as_deref(), Some("run-model")); + assert_eq!(cfg.run_model_provider_str().as_deref(), Some("anthropic")); // v2 R22: run.inputs replaces wholesale, so the workflow layer wins // over project and cli. - let vars = cfg.vars.as_ref().expect("vars"); - assert_eq!(vars.get("run_only").map(String::as_str), Some("1")); - assert_eq!(vars.get("shared").map(String::as_str), Some("run")); + let vars = cfg.run_inputs().expect("run.inputs"); + assert_eq!(vars.get("run_only").and_then(|v| v.as_str()), Some("1")); + assert_eq!(vars.get("shared").and_then(|v| v.as_str()), Some("run")); // checkpoint.exclude_globs is a security/policy list: replace by default. + let checkpoint = cfg.run_checkpoint().expect("run.checkpoint"); assert_eq!( - cfg.checkpoint.exclude_globs, + checkpoint.exclude_globs, vec!["run-only".to_string(), "shared".to_string()] ); // Hooks: id-based replacement. The "shared" hook appears in both cli and // workflow layers and resolves to the workflow entry; project and run-only // contribute the other two ids. - assert!(cfg.hooks.len() >= 2); - let shared_hook = cfg - .hooks + let hooks = cfg.run_hooks(); + assert!(hooks.len() >= 2); + let shared_hook = hooks .iter() .find(|hook| hook.name.as_deref() == Some("shared")) .expect("shared hook"); - assert_eq!(shared_hook.command.as_deref(), Some("echo run")); + assert_eq!( + shared_hook + .script + .as_ref() + .map(|s| s.as_source()) + .as_deref(), + Some("echo run") + ); assert!( - cfg.hooks + hooks .iter() .any(|hook| hook.name.as_deref() == Some("run-only")) ); - match &cfg.mcp_servers["shared"].transport { - McpTransport::Stdio { command, .. } => assert_eq!(command, &vec!["echo", "run"]), + let mcps = cfg.run_agent_mcps().expect("run.agent.mcps"); + match mcps.get("shared").expect("shared mcp") { + McpEntryLayer::Stdio { command, .. } => { + let command = command.as_ref().expect("command"); + let parts: Vec = command.iter().map(|c| c.as_source()).collect(); + assert_eq!(parts, vec!["echo".to_string(), "run".to_string()]); + } other => panic!("unexpected MCP transport: {other:?}"), } - assert!(cfg.mcp_servers.contains_key("run_only")); + assert!(mcps.contains_key("run_only")); // run.sandbox.daytona.labels stays sticky merge-by-key per R71. - let sandbox = cfg.sandbox.as_ref().expect("sandbox"); - let labels = sandbox - .daytona - .as_ref() - .and_then(|d| d.labels.as_ref()) - .expect("daytona labels"); + let sandbox = cfg.run_sandbox().expect("run.sandbox"); + let labels = &sandbox.daytona.as_ref().expect("daytona").labels; assert_eq!(labels.get("run_only").map(String::as_str), Some("1")); assert_eq!(labels.get("shared").map(String::as_str), Some("run")); // run.sandbox.env stays sticky merge-by-key per R71. - let env = sandbox.env.as_ref().expect("sandbox env"); - assert_eq!(env.get("CLI_ONLY").map(String::as_str), Some("1")); - assert_eq!(env.get("RUN_ONLY").map(String::as_str), Some("1")); - assert_eq!(env.get("SHARED").map(String::as_str), Some("run")); + let env = &sandbox.env; + assert_eq!( + env.get("CLI_ONLY").map(|v| v.as_source()).as_deref(), + Some("1") + ); + assert_eq!( + env.get("RUN_ONLY").map(|v| v.as_source()).as_deref(), + Some("1") + ); + assert_eq!( + env.get("SHARED").map(|v| v.as_source()).as_deref(), + Some("run") + ); } #[test] @@ -435,17 +442,14 @@ fn settings_local_explicit_workflow_path_uses_workflow_project_layers() { .clone(); let cfg = parse_settings(&output); - assert_eq!(cfg.auto_approve, Some(true)); + assert!(cfg.auto_approve_enabled()); // v2 R30: run.prepare.steps replaces the whole ordered list across layers. // The highest-precedence layer (workflow) wins. assert_eq!( - cfg.setup.as_ref().expect("setup config").commands, + cfg.run_prepare_commands(), vec!["workflow-setup".to_string()] ); - assert_eq!( - cfg.sandbox.as_ref().expect("sandbox config").preserve, - Some(true) - ); + assert_eq!(cfg.run_sandbox().and_then(|sb| sb.preserve), Some(true)); } #[test] @@ -594,8 +598,8 @@ name = "legacy-model" .stderr(predicate::str::contains("Rename it to")); let cfg = parse_settings(&assert.get_output().stdout); - assert_eq!(cfg.verbose, None); - assert_eq!(cfg.llm, None); + assert!(!cfg.verbose_enabled()); + assert!(cfg.run_model().is_none()); } #[test] @@ -624,12 +628,11 @@ shared = "legacy" .stderr(predicate::str::contains("ignoring legacy config file")); let cfg = parse_settings(&assert.get_output().stdout); - let llm = cfg.llm.as_ref().expect("llm config"); - assert_eq!(llm.model.as_deref(), Some("project-model")); + assert_eq!(cfg.run_model_name_str().as_deref(), Some("project-model")); assert_eq!( - cfg.vars - .as_ref() - .and_then(|vars| vars.get("shared").map(String::as_str)), + cfg.run_inputs() + .and_then(|vars| vars.get("shared")) + .and_then(|v| v.as_str()), Some("project") ); } @@ -763,18 +766,20 @@ shared = "cli" mock.assert(); let cfg = parse_settings(&output); - let llm = cfg.llm.as_ref().expect("llm config"); - assert_eq!(llm.model.as_deref(), Some("project-model")); - assert_eq!(llm.provider.as_deref(), Some("openai")); - assert_eq!(cfg.storage_dir, Some(PathBuf::from("/srv/fabro-server"))); - assert_eq!(cfg.verbose, Some(true)); + assert_eq!(cfg.run_model_name_str().as_deref(), Some("project-model")); + assert_eq!(cfg.run_model_provider_str().as_deref(), Some("openai")); + assert_eq!( + cfg.server_storage_root_str().as_deref(), + Some("/srv/fabro-server") + ); + assert!(cfg.verbose_enabled()); // R22: run.inputs replaces wholesale across layers. Project is the // highest-precedence layer that sets inputs, so project's vars win // and server-side vars are discarded rather than merged. - let vars = cfg.vars.as_ref().expect("vars"); - assert_eq!(vars.get("project_only").map(String::as_str), Some("1")); - assert_eq!(vars.get("shared").map(String::as_str), Some("project")); + let vars = cfg.run_inputs().expect("run.inputs"); + assert_eq!(vars.get("project_only").and_then(|v| v.as_str()), Some("1")); + assert_eq!(vars.get("shared").and_then(|v| v.as_str()), Some("project")); assert!( !vars.contains_key("server_only"), "v2 merge matrix replaces run.inputs wholesale; server_only should be dropped" @@ -829,7 +834,10 @@ verbosity = "verbose" cli_mock.assert(); configured_mock.assert_calls(0); let cfg = parse_settings(&output); - assert_eq!(cfg.storage_dir, Some(PathBuf::from("/srv/fabro-server"))); + assert_eq!( + cfg.server_storage_root_str().as_deref(), + Some("/srv/fabro-server") + ); } #[test] diff --git a/lib/crates/fabro-config/src/config.rs b/lib/crates/fabro-config/src/config.rs index fa27d71bd..0ac8b075d 100644 --- a/lib/crates/fabro-config/src/config.rs +++ b/lib/crates/fabro-config/src/config.rs @@ -6,19 +6,14 @@ //! top-level keys with targeted rename hints. `ConfigLayer::combine` walks //! the v2 merge matrix from [`crate::merge`]. //! -//! [`ConfigLayer::resolve`] uses the transitional bridge in -//! [`fabro_types::settings::v2::bridge`] to produce the legacy flat -//! [`Settings`] shape that most consumers still read. New code should prefer -//! [`ConfigLayer::as_v2`] to read v2 fields directly; the bridge and the old -//! flat shape are scheduled for removal once every consumer is migrated. +//! Consumers that need the inner tree call [`ConfigLayer::as_v2`] (borrow) +//! or `.into()` to move out an owned `SettingsFile`. The legacy flat +//! `Settings` shape is no longer reachable from this layer. 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 fabro_types::settings::v2::{SettingsFile, parse_settings_file as parse_v2_settings_file}; use serde::{Deserialize, Serialize}; use crate::merge::combine_files; @@ -27,9 +22,10 @@ use crate::user; /// A parsed settings file layer. /// -/// 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. +/// 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 { @@ -48,22 +44,6 @@ impl From for SettingsFile { } } -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] @@ -130,13 +110,6 @@ impl ConfigLayer { user::load_settings_config(None) } - /// 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 { diff --git a/lib/crates/fabro-config/src/server.rs b/lib/crates/fabro-config/src/server.rs index fce1cd9a8..a874bfe1b 100644 --- a/lib/crates/fabro-config/src/server.rs +++ b/lib/crates/fabro-config/src/server.rs @@ -4,11 +4,11 @@ //! 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. +//! Stage 6.4 deletes it. use std::path::PathBuf; -use fabro_types::Settings; +use fabro_types::settings::v2::SettingsFile; pub use fabro_types::settings::server::{ ApiAuthStrategy, ApiSettings, ArtifactStorageBackend, ArtifactStorageSettings, AuthProvider, @@ -18,6 +18,6 @@ pub use fabro_types::settings::server::{ /// Resolve the storage directory: config value > default `~/.fabro`. #[must_use] -pub fn resolve_storage_dir(settings: &Settings) -> PathBuf { +pub fn resolve_storage_dir(settings: &SettingsFile) -> PathBuf { settings.storage_dir() } diff --git a/lib/crates/fabro-server/src/diagnostics.rs b/lib/crates/fabro-server/src/diagnostics.rs index fde5a09a3..1ceb1634d 100644 --- a/lib/crates/fabro-server/src/diagnostics.rs +++ b/lib/crates/fabro-server/src/diagnostics.rs @@ -1,15 +1,13 @@ -use std::path::Path; +use std::path::PathBuf; use std::process::Command; use std::sync::LazyLock; use std::time::Duration; use base64::Engine as _; use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; -use fabro_config::server::ApiAuthStrategy; use fabro_llm::client::Client as LlmClient; use fabro_llm::types::{Message, Request}; use fabro_model::{Catalog, Provider}; -use fabro_types::settings::v2::bridge::bridge_to_old; use fabro_util::check_report::{CheckDetail, CheckResult, CheckSection, CheckStatus}; use fabro_util::version::FABRO_VERSION; use regex::Regex; @@ -467,20 +465,24 @@ async fn check_brave_search(state: &AppState) -> CheckResult { } fn check_crypto(state: &AppState) -> CheckResult { + use fabro_types::settings::v2::interp::InterpString; + let settings_file = state .settings .read() .expect("settings lock poisoned") .clone(); - // Temporary bridge while diagnostics is migrated to v2 shapes directly. - let settings = bridge_to_old(&settings_file); - let api = settings.api.clone().unwrap_or_default(); - let has_jwt = api - .authentication_strategies - .contains(&ApiAuthStrategy::Jwt); - let has_mtls = api - .authentication_strategies - .contains(&ApiAuthStrategy::Mtls); + let auth_api = settings_file + .server + .as_ref() + .and_then(|s| s.auth.as_ref()) + .and_then(|a| a.api.as_ref()); + let has_jwt = auth_api + .and_then(|api| api.jwt.as_ref()) + .is_some_and(|jwt| jwt.enabled.unwrap_or(true)); + let has_mtls = auth_api + .and_then(|api| api.mtls.as_ref()) + .is_some_and(|mtls| mtls.enabled.unwrap_or(true)); if !has_jwt && !has_mtls { return CheckResult { @@ -488,7 +490,10 @@ fn check_crypto(state: &AppState) -> CheckResult { status: CheckStatus::Warning, summary: "no authentication configured".to_string(), details: Vec::new(), - remediation: Some("Configure authentication_strategies in [api]".to_string()), + remediation: Some( + "Configure strategies under [server.auth.api.jwt] or [server.auth.api.mtls]" + .to_string(), + ), }; } @@ -496,13 +501,32 @@ fn check_crypto(state: &AppState) -> CheckResult { let mut errors = Vec::new(); if has_mtls { - if let Some(tls) = api.tls { - let read = |path: &Path| -> Result { - let expanded = fabro_config::expand_tilde(path); + use fabro_types::settings::v2::server::ServerListenLayer; + let listen_tls = settings_file + .server + .as_ref() + .and_then(|s| s.listen.as_ref()) + .and_then(|listen| match listen { + ServerListenLayer::Tcp { tls, .. } => tls.as_ref(), + ServerListenLayer::Unix { .. } => None, + }); + if let Some(listen_tls) = listen_tls { + let read = |raw: Option, label: &str| -> Result { + let Some(path_str) = raw else { + return Err(format!("server.listen.tls.{label} is not configured")); + }; + let path = PathBuf::from(&path_str); + let expanded = fabro_config::expand_tilde(&path); std::fs::read_to_string(&expanded) .map_err(|e| format!("{}: {e}", expanded.display())) }; - match (read(&tls.cert), read(&tls.key), read(&tls.ca)) { + let cert = read( + listen_tls.cert.as_ref().map(InterpString::as_source), + "cert", + ); + let key = read(listen_tls.key.as_ref().map(InterpString::as_source), "key"); + let ca = read(listen_tls.ca.as_ref().map(InterpString::as_source), "ca"); + match (cert, key, ca) { (Ok(cert_pem), Ok(key_pem), Ok(ca_pem)) => { if let Err(err) = validate_tls_cert(&cert_pem, chrono::Utc::now().timestamp()) { errors.push(err); @@ -517,7 +541,7 @@ fn check_crypto(state: &AppState) -> CheckResult { _ => errors.push("failed to read mTLS files".to_string()), } } else { - errors.push("mTLS configured but [api.tls] is missing".to_string()); + errors.push("mTLS configured but [server.listen.tls] is missing".to_string()); } } diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index 28e341ea8..132a48ab7 100644 --- a/lib/crates/fabro-server/src/run_manifest.rs +++ b/lib/crates/fabro-server/src/run_manifest.rs @@ -17,13 +17,13 @@ use fabro_sandbox::daytona::DaytonaConfig; use fabro_sandbox::{DockerSandboxOptions, Sandbox, SandboxProvider, SandboxSpec}; use fabro_types::RunId; use fabro_types::settings::v2::SettingsFile; -use fabro_types::settings::v2::bridge::bridge_sandbox; use fabro_types::settings::v2::cli::{CliLayer, CliOutputLayer, OutputVerbosity}; use fabro_types::settings::v2::interp::InterpString; use fabro_types::settings::v2::run::{ ApprovalMode, DaytonaDockerfileLayer, RunExecutionLayer, RunLayer, RunMode, RunModelLayer, RunSandboxLayer, }; +use fabro_types::settings::v2::to_runtime::bridge_sandbox; use fabro_util::check_report::{CheckDetail, CheckReport, CheckResult, CheckSection, CheckStatus}; use fabro_validate::Severity; use fabro_workflow::error::FabroError; diff --git a/lib/crates/fabro-server/src/serve.rs b/lib/crates/fabro-server/src/serve.rs index 7c343f246..6d690201e 100644 --- a/lib/crates/fabro-server/src/serve.rs +++ b/lib/crates/fabro-server/src/serve.rs @@ -3,7 +3,7 @@ use std::sync::{Arc, RwLock}; use std::time::Duration; use fabro_config::Storage; -use fabro_config::server::{ArtifactStorageBackend, resolve_storage_dir}; +use fabro_config::server::{ApiSettings, resolve_storage_dir}; use fabro_config::user::{active_settings_path, load_settings_config}; use fabro_util::terminal::Styles; use object_store::ObjectStore; @@ -17,9 +17,7 @@ use tracing::{error, info, warn}; use clap::Args; -use fabro_types::Settings; use fabro_types::settings::v2::SettingsFile; -use fabro_types::settings::v2::bridge::bridge_to_old; use crate::bind::{self, Bind, BindRequest}; use crate::github_webhooks::WebhookManager; @@ -86,11 +84,72 @@ fn load_settings(path: Option<&Path>) -> anyhow::Result { Ok(load_settings_config(path)?.into()) } -/// Bridged helper for legacy call sites inside serve.rs that still read flat -/// Settings fields. Callers pass a v2 SettingsFile; this returns the legacy -/// shape via the transitional bridge. -fn bridged(settings: &SettingsFile) -> Settings { - bridge_to_old(settings) +/// Build the legacy `ApiSettings` shape that `resolve_auth_mode_with_lookup` +/// and the TLS branch still expect, extracting the pieces it needs from the +/// v2 tree. Stage 6.6 replaces this with a v2-aware auth resolver and drops +/// the legacy `ApiSettings` type entirely. +fn build_legacy_api_settings(file: &SettingsFile) -> ApiSettings { + use fabro_config::server::{ApiAuthStrategy, TlsSettings}; + use fabro_types::settings::v2::interp::InterpString; + use fabro_types::settings::v2::server::ServerListenLayer; + + let auth_api = file + .server + .as_ref() + .and_then(|s| s.auth.as_ref()) + .and_then(|a| a.api.as_ref()); + + let mut authentication_strategies = Vec::new(); + if auth_api + .and_then(|api| api.jwt.as_ref()) + .and_then(|jwt| jwt.enabled) + .unwrap_or(auth_api.and_then(|api| api.jwt.as_ref()).is_some()) + { + authentication_strategies.push(ApiAuthStrategy::Jwt); + } + if auth_api + .and_then(|api| api.mtls.as_ref()) + .and_then(|mtls| mtls.enabled) + .unwrap_or(auth_api.and_then(|api| api.mtls.as_ref()).is_some()) + { + authentication_strategies.push(ApiAuthStrategy::Mtls); + } + + let base_url = file + .server_api() + .and_then(|api| api.url.as_ref()) + .map_or_else( + || "http://localhost:3000/api/v1".to_string(), + InterpString::as_source, + ); + + // TLS files now live under `server.listen.tls.{cert,key,ca}` in v2. + // Build a legacy TlsSettings from the listen TLS subtree so the + // existing rustls config path keeps working. + let tls = file + .server + .as_ref() + .and_then(|s| s.listen.as_ref()) + .and_then(|listen| match listen { + ServerListenLayer::Tcp { tls, .. } => tls.as_ref(), + ServerListenLayer::Unix { .. } => None, + }) + .and_then(|tls_layer| { + let cert = tls_layer.cert.as_ref().map(InterpString::as_source)?; + let key = tls_layer.key.as_ref().map(InterpString::as_source)?; + let ca = tls_layer.ca.as_ref().map(InterpString::as_source)?; + Some(TlsSettings { + cert: cert.into(), + key: key.into(), + ca: ca.into(), + }) + }); + + ApiSettings { + base_url, + authentication_strategies, + tls, + } } fn resolved_config_path(path: Option<&Path>) -> PathBuf { @@ -183,39 +242,52 @@ fn build_artifact_object_store( settings: &SettingsFile, storage: &Storage, ) -> anyhow::Result<(Arc, String)> { - let bridged_settings = bridged(settings); - let artifact_settings = bridged_settings - .artifact_storage - .clone() - .unwrap_or_default(); + use fabro_types::settings::v2::interp::InterpString; + use fabro_types::settings::v2::server::ObjectStoreProvider; + + let artifacts = settings.server_artifacts(); + let prefix = artifacts + .and_then(|a| a.prefix.as_ref()) + .map_or_else(|| "artifacts".to_string(), InterpString::as_source); if use_in_memory_store() { - return Ok((Arc::new(InMemory::new()), artifact_settings.prefix)); + return Ok((Arc::new(InMemory::new()), prefix)); } - match artifact_settings.backend { - ArtifactStorageBackend::Local => { + let provider = artifacts + .and_then(|a| a.provider) + .unwrap_or(ObjectStoreProvider::Local); + + let s3_cfg = artifacts.and_then(|a| a.s3.as_ref()); + match provider { + ObjectStoreProvider::Local => { std::fs::create_dir_all(storage.artifact_store_dir())?; let object_store = Arc::new(LocalFileSystem::new_with_prefix(storage.root())?); - Ok((object_store, artifact_settings.prefix)) + Ok((object_store, prefix)) } - ArtifactStorageBackend::S3 => { - let bucket = artifact_settings + ObjectStoreProvider::S3 => { + let s3 = s3_cfg.ok_or_else(|| { + anyhow::anyhow!("server.artifacts.s3 is required for provider = 's3'") + })?; + let bucket = s3 .bucket - .ok_or_else(|| anyhow::anyhow!("artifact_storage.bucket is required for s3"))?; - let region = artifact_settings + .as_ref() + .map(InterpString::as_source) + .ok_or_else(|| anyhow::anyhow!("server.artifacts.s3.bucket is required"))?; + let region = s3 .region - .ok_or_else(|| anyhow::anyhow!("artifact_storage.region is required for s3"))?; - + .as_ref() + .map(InterpString::as_source) + .ok_or_else(|| anyhow::anyhow!("server.artifacts.s3.region is required"))?; let mut builder = AmazonS3Builder::from_env() .with_bucket_name(bucket) .with_region(region) - .with_virtual_hosted_style_request(!artifact_settings.path_style.unwrap_or(false)); - if let Some(endpoint) = artifact_settings.endpoint { + .with_virtual_hosted_style_request(!s3.path_style.unwrap_or(false)); + if let Some(endpoint) = s3.endpoint.as_ref().map(InterpString::as_source) { builder = builder.with_endpoint(endpoint); } let object_store = Arc::new(builder.build()?); - Ok((object_store, artifact_settings.prefix)) + Ok((object_store, prefix)) } } } @@ -241,8 +313,7 @@ where let config_path = args.config.clone(); let disk_settings = load_settings(config_path.as_deref())?; let active_config_path = resolved_config_path(config_path.as_deref()); - let data_dir = - storage_dir_override.unwrap_or_else(|| resolve_storage_dir(&bridged(&disk_settings))); + let data_dir = storage_dir_override.unwrap_or_else(|| resolve_storage_dir(&disk_settings)); let storage = Storage::new(&data_dir); let secret_store_path = storage.secrets_path(); let secret_store = SecretStore::load(secret_store_path.clone())?; @@ -284,12 +355,16 @@ where std::fs::create_dir_all(&data_dir)?; let (auth_mode, client_auth, max_concurrent_runs) = { let cfg_file = shared_settings.read().expect("config lock poisoned"); - let cfg = bridged(&cfg_file); - let api = cfg.api.clone().unwrap_or_default(); - let allowed_usernames = cfg - .web + // Build the legacy ApiSettings + allowed_usernames shapes that the + // v1 auth resolver expects. Stage 6.6 replaces this with a direct + // v2-aware resolver. + let api = build_legacy_api_settings(&cfg_file); + let allowed_usernames = cfg_file + .server .as_ref() - .map(|w| w.auth.allowed_usernames.clone()) + .and_then(|s| s.auth.as_ref()) + .and_then(|a| a.web.as_ref()) + .map(|w| w.allowed_usernames.clone()) .unwrap_or_default(); let auth_mode = resolve_auth_mode_with_lookup(&api, &allowed_usernames, |name| { secret_snapshot @@ -300,7 +375,7 @@ where let client_auth = api.tls.as_ref().map(|_| client_auth_from_mode(&auth_mode)); let max_concurrent_runs = args .max_concurrent_runs - .or(cfg.max_concurrent_runs) + .or_else(|| cfg_file.max_concurrent_runs()) .unwrap_or(5); (auth_mode, client_auth, max_concurrent_runs) }; @@ -352,12 +427,13 @@ where // Optionally start webhook listener let webhook_app_id = { + use fabro_types::settings::v2::InterpString; let cfg_file = shared_settings.read().expect("config lock poisoned"); - let cfg = bridged(&cfg_file); - cfg.git - .as_ref() - .and_then(|g| g.webhooks.as_ref().and(g.app_id.as_ref())) - .cloned() + cfg_file + .server_integrations_github() + .filter(|github| github.webhooks.is_some()) + .and_then(|github| github.app_id.as_ref()) + .map(InterpString::as_source) }; let webhook_manager = match webhook_app_id { Some(app_id) => { @@ -448,8 +524,7 @@ where // Branch: TLS, plain TCP, or Unix socket let tls_settings = { let cfg_file = shared_settings.read().expect("config lock poisoned"); - let cfg = bridged(&cfg_file); - cfg.api.as_ref().and_then(|a| a.tls.clone()) + build_legacy_api_settings(&cfg_file).tls.clone() }; let bound_listener = bind_listener(&bind_request).await?; diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 3ab01fccb..9cad3a01e 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -33,7 +33,6 @@ use fabro_model::{BilledModelUsage, BilledTokenCounts}; use fabro_store::{ ArtifactStore, Database, EventEnvelope, EventPayload, PendingInterviewRecord, StageId, }; -use fabro_types::settings::v2::bridge::bridge_to_old; use fabro_types::settings::v2::{InterpString, SettingsFile}; use fabro_types::{ EventBody, InterviewQuestionRecord, InterviewQuestionType, RunBlobId, RunClientProvenance, @@ -1065,24 +1064,21 @@ async fn get_server_settings( State(state): State>, ) -> Response { let settings = state.settings.read().unwrap().clone(); - let response = match api_server_settings(&settings) { - Ok(response) => response, + // Stage 6.6 TODO: replace this with an explicit allow-list DTO that + // reads directly from the v2 tree and redacts env-sourced values via + // `InterpString` provenance. For now we serialize the full v2 + // `SettingsFile` as JSON so the web UI still has a response body -- + // the legacy `ServerSettings` OpenAPI schema will be rewritten in + // 6.6 alongside the fabro-web DTO updates. + let mut value = match serde_json::to_value(&settings) { + Ok(value) => value, Err(err) => { return ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()) .into_response(); } }; - (StatusCode::OK, Json(response)).into_response() -} - -fn api_server_settings(settings: &SettingsFile) -> anyhow::Result { - // Temporary shim: reuse the legacy flat Settings shape via the v2 bridge - // so the existing `/api/v1/settings` DTO keeps working. Stage 6.6 replaces - // this with an explicit allow-list DTO built directly from the v2 tree. - let legacy = bridge_to_old(settings); - let mut value = serde_json::to_value(&legacy)?; strip_nulls(&mut value); - serde_json::from_value(value).map_err(Into::into) + (StatusCode::OK, Json(value)).into_response() } fn strip_nulls(value: &mut serde_json::Value) { diff --git a/lib/crates/fabro-server/src/web_auth.rs b/lib/crates/fabro-server/src/web_auth.rs index c503cd9dd..909f121c6 100644 --- a/lib/crates/fabro-server/src/web_auth.rs +++ b/lib/crates/fabro-server/src/web_auth.rs @@ -6,10 +6,7 @@ use axum::http::{HeaderMap, HeaderValue, StatusCode, header}; use axum::response::{IntoResponse, Redirect, Response}; use axum::{Json, Router, routing::get, routing::post}; use cookie::{Cookie, CookieJar, Expiration, Key, SameSite, time::Duration}; -use fabro_types::Settings; -use fabro_types::settings::v2::SettingsFile; -use fabro_types::settings::v2::bridge::bridge_to_old; -use fabro_types::settings::{ApiAuthStrategy, GitProvider, GitSettings}; +use fabro_types::settings::v2::{InterpString, SettingsFile}; use serde::{Deserialize, Serialize}; use serde_json::json; use tracing::{debug, error, info, warn}; @@ -149,42 +146,43 @@ fn json_response(status: StatusCode, body: serde_json::Value) -> Response { (status, Json(body)).into_response() } -fn features_json(settings: &Settings) -> serde_json::Value { - let features = settings.features.clone().unwrap_or_default(); +fn features_json(settings: &SettingsFile) -> serde_json::Value { + let features = settings.features.as_ref(); + let session_sandboxes = features.and_then(|f| f.session_sandboxes).unwrap_or(false); + // Retros in v2 live under `run.execution.retros` (positive form) rather + // than the top-level features stanza. + let retros = settings + .run_execution() + .and_then(|e| e.retros) + .unwrap_or(false); json!({ - "session_sandboxes": features.session_sandboxes, - "retros": features.retros, + "session_sandboxes": session_sandboxes, + "retros": retros, }) } -/// Temporary helper used during the v2 consumer migration. Bridges a -/// `SettingsFile` down to the legacy flat `Settings` shape so web_auth's -/// oauth/git flows can keep reading flat fields until they're migrated -/// directly (Stage 6.6 alongside the `/api/v1/settings` DTO rewrite). -fn bridged(settings_file: &SettingsFile) -> Settings { - bridge_to_old(settings_file) -} - async fn login_github(State(state): State>) -> Response { - let settings = bridged( - &state - .settings - .read() - .expect("settings lock poisoned") - .clone(), - ); - let Some(client_id) = settings.client_id().map(str::to_string) else { + let settings = state + .settings + .read() + .expect("settings lock poisoned") + .clone(); + let Some(client_id) = settings.github_client_id_str() else { warn!("OAuth login failed: client_id not configured"); return json_response( StatusCode::CONFLICT, json!({"error": "GitHub App client_id is not configured"}), ); }; - let Some(web_url) = settings.web.as_ref().map(|web| web.url.clone()) else { - warn!("OAuth login failed: web.url not configured"); + let Some(web_url) = settings + .server_web() + .and_then(|w| w.url.as_ref()) + .map(InterpString::as_source) + else { + warn!("OAuth login failed: server.web.url not configured"); return json_response( StatusCode::CONFLICT, - json!({"error": "web.url is not configured"}), + json!({"error": "server.web.url is not configured"}), ); }; @@ -228,13 +226,11 @@ async fn callback_github( json!({"error": "SESSION_SECRET is not configured"}), ); }; - let settings = bridged( - &state - .settings - .read() - .expect("settings lock poisoned") - .clone(), - ); + let settings = state + .settings + .read() + .expect("settings lock poisoned") + .clone(); let cookie_jar = parse_cookie_header(&headers); let stored_state = cookie_jar.get(OAUTH_STATE_COOKIE_NAME).map(Cookie::value); if stored_state != Some(params.state.as_str()) { @@ -242,7 +238,7 @@ async fn callback_github( return Redirect::to("/login").into_response(); } - let Some(client_id) = settings.client_id().map(str::to_string) else { + let Some(client_id) = settings.github_client_id_str() else { error!("OAuth callback failed: client_id not configured"); return json_response( StatusCode::CONFLICT, @@ -256,10 +252,13 @@ async fn callback_github( json!({"error": "GITHUB_APP_CLIENT_SECRET is not configured"}), ); }; - let web_url = settings.web.as_ref().map_or_else( - || "http://localhost:3000".to_string(), - |web| web.url.clone(), - ); + let web_url = settings + .server_web() + .and_then(|w| w.url.as_ref()) + .map_or_else( + || "http://localhost:3000".to_string(), + InterpString::as_source, + ); let http = reqwest::Client::new(); let token = match http @@ -358,9 +357,11 @@ async fn callback_github( }; let allowed_usernames = settings - .web + .server .as_ref() - .map(|web| web.auth.allowed_usernames.clone()) + .and_then(|s| s.auth.as_ref()) + .and_then(|a| a.web.as_ref()) + .map(|w| w.allowed_usernames.clone()) .unwrap_or_default(); if !allowed_usernames.is_empty() && !allowed_usernames.iter().any(|user| user == &profile.login) { @@ -445,13 +446,11 @@ async fn auth_me(State(state): State>, headers: HeaderMap) -> Resp return json_response(StatusCode::UNAUTHORIZED, json!({"error": "Unauthorized"})); }; - let settings = bridged( - &state - .settings - .read() - .expect("settings lock poisoned") - .clone(), - ); + let settings = state + .settings + .read() + .expect("settings lock poisoned") + .clone(); let demo_mode = parse_cookie_header(&headers) .get("fabro-demo") .is_some_and(|cookie| cookie.value() == "1"); @@ -471,17 +470,12 @@ async fn auth_me(State(state): State>, headers: HeaderMap) -> Resp } async fn setup_status(State(state): State>) -> Response { - let settings = bridged( - &state - .settings - .read() - .expect("settings lock poisoned") - .clone(), - ); - let configured = settings - .git - .as_ref() - .is_some_and(|git| git.client_id.is_some()); + let settings = state + .settings + .read() + .expect("settings lock poisoned") + .clone(); + let configured = settings.github_client_id_str().is_some(); Json(SetupStatusResponse { configured }).into_response() } @@ -565,26 +559,10 @@ async fn setup_register( let settings_path = state.config_path.clone(); - // Bridge the v2 in-memory state down to the legacy flat shape so the - // existing register flow can continue to mutate it and write legacy - // TOML. Stage 6.6 rewrites this to produce v2 TOML directly. - let settings_file = state - .settings - .read() - .expect("settings lock poisoned") - .clone(); - let mut settings = bridged(&settings_file); - let mut git = settings.git.clone().unwrap_or_default(); - git.provider = GitProvider::Github; - git.app_id = Some(data.id.to_string()); - git.client_id = Some(data.client_id.clone()); - git.slug = Some(data.slug.clone()); - settings.git = Some(git.clone()); - if let Some(ref origin) = origin { - let web = settings.web.get_or_insert_default(); - web.url.clone_from(origin); - } - + // Build a v2 settings_path edit in place. This used to bridge back to + // the legacy flat shape and emit v1 TOML; the v2 parser hard-rejects + // the v1 top-level keys, so this was already broken. Write v2 TOML + // using `merge_settings_keys` against the raw TOML document. if let Some(parent) = settings_path.parent() { let _ = std::fs::create_dir_all(parent); } @@ -603,7 +581,7 @@ async fn setup_register( } } }; - if let Err(err) = merge_settings_keys(&mut doc, &settings, &git, origin.as_deref()) { + if let Err(err) = merge_settings_keys(&mut doc, &data, origin.as_deref()) { error!(error = %err, "Setup register failed: could not merge settings"); return json_response( StatusCode::INTERNAL_SERVER_ERROR, @@ -644,13 +622,14 @@ async fn setup_register( } } - // Stage 6.6 TODO: re-parse the freshly-written `settings_path` via - // `ConfigLayer::load` and swap it into `state.settings`. For now, leave - // the in-memory state unchanged -- subsequent server restarts will - // re-read the file. The `settings` binding above mutates a bridged - // copy that only feeds the TOML merge output; dropping it here is - // intentional. - drop(settings); + // Re-parse the freshly-written settings file and swap it into the + // in-memory state. Stage 6.6 may split this differently when the web + // setup flow is reworked, but for now a round-trip through + // `ConfigLayer::load` keeps the live state consistent with disk. + if let Ok(reloaded) = fabro_config::ConfigLayer::load(&settings_path) { + let mut shared = state.settings.write().expect("settings lock poisoned"); + *shared = reloaded.into(); + } info!(slug = %data.slug, app_id = %data.id, "GitHub App registered successfully"); Json(json!({"ok": true})).into_response() @@ -671,152 +650,88 @@ fn ensure_table<'a>(table: &'a mut toml::Table, key: &str) -> anyhow::Result<&'a fn merge_settings_keys( doc: &mut toml::Value, - settings: &Settings, - git: &GitSettings, + data: &GitHubManifestConversion, origin: Option<&str>, ) -> anyhow::Result<()> { - let web_url = origin - .map(str::to_string) - .or_else(|| settings.web.as_ref().map(|web| web.url.clone())) - .unwrap_or_else(|| "http://localhost:3000".to_string()); - let allowed = settings - .web - .as_ref() - .map(|web| web.auth.allowed_usernames.clone()) - .unwrap_or_default(); - let api = settings.api.clone().unwrap_or_default(); + let web_url = origin.map_or_else(|| "http://localhost:3000".to_string(), str::to_string); let root = root_table_mut(doc)?; - let web = ensure_table(root, "web")?; - web.insert("url".to_string(), toml::Value::String(web_url.clone())); - let auth = ensure_table(web, "auth")?; - auth.insert( - "provider".to_string(), - toml::Value::String("github".to_string()), - ); - auth.insert( - "allowed_usernames".to_string(), - toml::Value::Array(allowed.into_iter().map(toml::Value::String).collect()), - ); + // Make sure the freshly-written file is a valid v2 file. + root.insert("_version".to_string(), toml::Value::Integer(1)); - let base_url = format!("{web_url}/api/v1"); - let api_table = ensure_table(root, "api")?; - api_table.insert("base_url".to_string(), toml::Value::String(base_url)); - api_table.insert( - "authentication_strategies".to_string(), - toml::Value::Array( - api.authentication_strategies - .iter() - .map(|strategy| match strategy { - ApiAuthStrategy::Jwt => "jwt", - ApiAuthStrategy::Mtls => "mtls", - }) - .map(|value| toml::Value::String(value.to_string())) - .collect(), - ), - ); + let server = ensure_table(root, "server")?; + let web = ensure_table(server, "web")?; + web.insert("enabled".to_string(), toml::Value::Boolean(true)); + web.insert("url".to_string(), toml::Value::String(web_url)); - let git_table = ensure_table(root, "git")?; - git_table.insert( - "provider".to_string(), - toml::Value::String("github".to_string()), - ); - git_table.insert( + let auth = ensure_table(server, "auth")?; + let auth_web = ensure_table(auth, "web")?; + let _ = auth_web; + let auth_api = ensure_table(auth, "api")?; + let _jwt = ensure_table(auth_api, "jwt")?; + + let integrations = ensure_table(server, "integrations")?; + let github = ensure_table(integrations, "github")?; + github.insert( "app_id".to_string(), - toml::Value::String(git.app_id.clone().unwrap_or_default()), + toml::Value::String(data.id.to_string()), ); - git_table.insert( + github.insert( "client_id".to_string(), - toml::Value::String(git.client_id.clone().unwrap_or_default()), - ); - git_table.insert( - "slug".to_string(), - toml::Value::String(git.slug.clone().unwrap_or_default()), + toml::Value::String(data.client_id.clone()), ); + github.insert("slug".to_string(), toml::Value::String(data.slug.clone())); Ok(()) } #[cfg(test)] mod tests { - use super::merge_settings_keys; - use fabro_types::Settings; + use super::{GitHubManifestConversion, merge_settings_keys}; - #[test] - fn merge_settings_keys_preserves_unrelated_git_nested_keys() { - let mut doc: toml::Value = toml::from_str( - r#" -[git] -provider = "github" - -[git.author] -name = "fabro" -email = "fabro@example.com" - -[git.webhooks] -strategy = "tailscale_funnel" -"#, - ) - .unwrap(); - - let mut settings = Settings::default(); - settings.web.get_or_insert_default().auth.allowed_usernames = vec!["alice".to_string()]; - settings.git.get_or_insert_default().provider = fabro_config::server::GitProvider::Github; - settings.git.get_or_insert_default().app_id = Some("123".to_string()); - settings.git.get_or_insert_default().client_id = Some("abc".to_string()); - settings.git.get_or_insert_default().slug = Some("fabro".to_string()); - - merge_settings_keys(&mut doc, &settings, settings.git.as_ref().unwrap(), None).unwrap(); - - let git = doc.get("git").and_then(toml::Value::as_table).unwrap(); - assert_eq!(git.get("app_id").and_then(toml::Value::as_str), Some("123")); - let author = git.get("author").and_then(toml::Value::as_table).unwrap(); - assert_eq!( - author.get("name").and_then(toml::Value::as_str), - Some("fabro") - ); - let webhooks = git.get("webhooks").and_then(toml::Value::as_table).unwrap(); - assert_eq!( - webhooks.get("strategy").and_then(toml::Value::as_str), - Some("tailscale_funnel") - ); + fn sample_conversion() -> GitHubManifestConversion { + GitHubManifestConversion { + id: 123, + slug: "fabro".to_string(), + client_id: "abc".to_string(), + client_secret: "shh".to_string(), + pem: String::new(), + webhook_secret: None, + } } #[test] - fn merge_settings_keys_preserves_unrelated_top_level_sections() { - let mut doc: toml::Value = toml::from_str( - r#" -[exec] -provider = "anthropic" - -[server] -target = "https://fabro.example.com/api/v1" -"#, - ) - .unwrap(); - - let mut settings = Settings::default(); - settings.web.get_or_insert_default().auth.allowed_usernames = vec!["alice".to_string()]; - settings.git.get_or_insert_default().provider = fabro_config::server::GitProvider::Github; - settings.git.get_or_insert_default().app_id = Some("123".to_string()); - settings.git.get_or_insert_default().client_id = Some("abc".to_string()); - settings.git.get_or_insert_default().slug = Some("fabro".to_string()); - - merge_settings_keys(&mut doc, &settings, settings.git.as_ref().unwrap(), None).unwrap(); + fn merge_settings_keys_writes_v2_server_integrations_github() { + let mut doc: toml::Value = + toml::from_str("_version = 1\n").expect("empty v2 doc should parse"); + merge_settings_keys(&mut doc, &sample_conversion(), Some("https://example.test")).unwrap(); + let github = doc + .get("server") + .and_then(toml::Value::as_table) + .and_then(|s| s.get("integrations")) + .and_then(toml::Value::as_table) + .and_then(|i| i.get("github")) + .and_then(toml::Value::as_table) + .expect("server.integrations.github should exist"); assert_eq!( - doc.get("exec") - .and_then(toml::Value::as_table) - .and_then(|exec| exec.get("provider")) - .and_then(toml::Value::as_str), - Some("anthropic") + github.get("app_id").and_then(toml::Value::as_str), + Some("123") ); assert_eq!( - doc.get("server") - .and_then(toml::Value::as_table) - .and_then(|server| server.get("target")) - .and_then(toml::Value::as_str), - Some("https://fabro.example.com/api/v1") + github.get("slug").and_then(toml::Value::as_str), + Some("fabro") + ); + + let web = doc + .get("server") + .and_then(toml::Value::as_table) + .and_then(|s| s.get("web")) + .and_then(toml::Value::as_table) + .expect("server.web should exist"); + assert_eq!( + web.get("url").and_then(toml::Value::as_str), + Some("https://example.test") ); } } diff --git a/lib/crates/fabro-server/tests/it/api/settings.rs b/lib/crates/fabro-server/tests/it/api/settings.rs index bad6937c4..51c61b467 100644 --- a/lib/crates/fabro-server/tests/it/api/settings.rs +++ b/lib/crates/fabro-server/tests/it/api/settings.rs @@ -43,8 +43,10 @@ server_only = "1" assert_eq!(response.status(), StatusCode::OK); let body = body_json(response.into_body()).await; - assert_eq!(body["storage_dir"], "/srv/fabro"); - assert_eq!(body["max_concurrent_runs"], 9); - assert_eq!(body["verbose"], true); - assert_eq!(body["vars"]["server_only"], "1"); + // `/api/v1/settings` emits the v2 SettingsFile shape directly now. + // Stage 6.6 will replace this with an explicit allow-list DTO. + assert_eq!(body["server"]["storage"]["root"], "/srv/fabro"); + assert_eq!(body["server"]["scheduler"]["max_concurrent_runs"], 9); + assert_eq!(body["cli"]["output"]["verbosity"], "verbose"); + assert_eq!(body["run"]["inputs"]["server_only"], "1"); } diff --git a/lib/crates/fabro-types/src/settings/mod.rs b/lib/crates/fabro-types/src/settings/mod.rs index 3545ab41a..949f53153 100644 --- a/lib/crates/fabro-types/src/settings/mod.rs +++ b/lib/crates/fabro-types/src/settings/mod.rs @@ -6,16 +6,17 @@ //! there. //! //! The flat [`Settings`] type and its submodules (`hook`, `mcp`, `project`, -//! `run`, `sandbox`, `server`, `user`) are the **resolved** shape that -//! current consumers still read. `fabro_config::ConfigLayer::resolve` walks -//! the v2 tree through [`v2::bridge::bridge_to_old`] to produce this flat -//! shape, so every consumer that touches `settings.llm`, `settings.vars`, -//! `settings.sandbox`, etc. keeps working. +//! `run`, `sandbox`, `server`, `user`) are the **runtime shapes** that +//! downstream crates (fabro-workflow, fabro-sandbox, fabro-mcp, +//! fabro-hooks) still consume at execution time. Stage 6.1 deleted the +//! `Settings` parse path; Stage 6.2 deleted the `bridge_to_old` +//! catch-all converter. Narrow v2→runtime helpers live in +//! [`v2::to_runtime`] and build these runtime shapes from specific v2 +//! subtrees on demand. //! -//! Full deletion of the flat shape (including the bridge) is scheduled for -//! a follow-up PR that migrates every consumer call site to read from -//! [`v2::SettingsFile`] directly. This module deliberately stays as a -//! transitional seam until then. +//! Stage 6.3 deletes these runtime types entirely in favor of v2-native +//! replacements, at which point this module and the helper modules +//! around it go away too. use std::collections::HashMap; use std::path::PathBuf; diff --git a/lib/crates/fabro-types/src/settings/v2/bridge.rs b/lib/crates/fabro-types/src/settings/v2/bridge.rs deleted file mode 100644 index d40f106b0..000000000 --- a/lib/crates/fabro-types/src/settings/v2/bridge.rs +++ /dev/null @@ -1,836 +0,0 @@ -//! Temporary bridge from the v2 parse tree to the old flat [`Settings`] shape. -//! -//! This module exists only to keep consumers compiling while Stages 3 and 4 -//! migrate parsers and consumers across the workspace. Field mappings are -//! best-effort and deliberately lossy for anything the old shape does not -//! have a slot for. **This entire module is deleted in Stage 6.** -//! -//! Env var interpolation is not performed here; `${env.NAME}` tokens are -//! emitted verbatim via [`InterpString::as_source`]. The post-layering -//! interpolation pass runs in `fabro-config` during Stage 3, after layering -//! is already complete. - -use std::collections::HashMap; - -use super::cli::{CliExecLayer, CliLayer, CliOutputLayer, CliTargetLayer, OutputVerbosity}; -use super::interp::InterpString; -use super::project::ProjectLayer; -use super::run::{ - AgentPermissions as V2AgentPermissions, ApprovalMode, HookEntry as V2HookEntry, - HookEvent as V2HookEvent, McpEntryLayer, MergeStrategy as V2MergeStrategy, ModelRefOrSplice, - RunLayer, RunMode, WorktreeMode as V2WorktreeMode, -}; -use super::server::{ - ObjectStoreProvider, ServerArtifactsLayer, ServerIntegrationsLayer, ServerLayer, - ServerSchedulerLayer, ServerStorageLayer, ServerWebLayer, -}; -use super::tree::SettingsFile; -use super::workflow::WorkflowLayer; -use crate::settings::Settings; -use crate::settings::hook::{ - HookDefinition, HookEvent as OldHookEvent, HookType as OldHookType, TlsMode as OldTlsMode, -}; -use crate::settings::mcp::{McpServerEntry, McpTransport}; -use crate::settings::project::ProjectSettings; -use crate::settings::run::{ - ArtifactsSettings, CheckpointSettings, LlmSettings, MergeStrategy as OldMergeStrategy, - PullRequestSettings, SetupSettings, -}; -use crate::settings::sandbox::{ - DaytonaNetwork, DaytonaSettings, DaytonaSnapshotSettings, DockerfileSource, - LocalSandboxSettings, SandboxSettings, WorktreeMode as OldWorktreeMode, -}; -use crate::settings::server::{ - ApiAuthStrategy, ApiSettings, ArtifactStorageBackend, ArtifactStorageSettings, AuthProvider, - AuthSettings, FeaturesSettings, GitAuthorSettings, GitProvider, GitSettings, LogSettings, - SlackSettings, WebSettings, -}; -use crate::settings::user::{ - ExecSettings, OutputFormat, PermissionLevel, ServerSettings as UserServer, -}; - -/// Convert a v2 `SettingsFile` into the legacy flat [`Settings`] shape. -/// -/// This is a temporary seam. All v2 fields that do not map cleanly are -/// dropped; callers that need those should read the v2 tree directly. -#[must_use] -pub fn bridge_to_old(file: &SettingsFile) -> Settings { - let mut out = Settings { - version: file.version, - ..Settings::default() - }; - - if let Some(project) = &file.project { - bridge_project(project, &mut out); - } - if let Some(workflow) = &file.workflow { - bridge_workflow(workflow, &mut out); - } - if let Some(run) = &file.run { - bridge_run(run, &mut out); - } - if let Some(cli) = &file.cli { - bridge_cli(cli, &mut out); - } - if let Some(server) = &file.server { - bridge_server(server, &mut out); - } - if let Some(features) = &file.features { - out.features = Some(FeaturesSettings { - session_sandboxes: features.session_sandboxes.unwrap_or(false), - retros: false, // v2 moves retros to run.execution.retros - }); - } - - out -} - -fn bridge_project(project: &ProjectLayer, out: &mut Settings) { - if let Some(directory) = &project.directory { - out.fabro = Some(ProjectSettings { - root: directory.clone(), - }); - } - if !project.metadata.is_empty() { - merge_labels(&mut out.labels, &project.metadata); - } -} - -fn bridge_workflow(workflow: &WorkflowLayer, out: &mut Settings) { - if let Some(graph) = &workflow.graph { - out.graph = Some(graph.clone()); - } - if !workflow.metadata.is_empty() { - merge_labels(&mut out.labels, &workflow.metadata); - } -} - -fn bridge_run(run: &RunLayer, out: &mut Settings) { - if let Some(goal) = &run.goal { - out.goal = Some(interp_to_string(goal)); - } - if let Some(wd) = &run.working_dir { - out.work_dir = Some(interp_to_string(wd)); - } - if !run.metadata.is_empty() { - merge_labels(&mut out.labels, &run.metadata); - } - - if let Some(inputs) = &run.inputs { - let mut vars: HashMap = HashMap::new(); - for (k, v) in inputs { - vars.insert(k.clone(), toml_value_to_string(v)); - } - out.vars = Some(vars); - } - - if let Some(model) = &run.model { - let mut llm = LlmSettings::default(); - if let Some(p) = &model.provider { - llm.provider = Some(interp_to_string(p)); - } - if let Some(n) = &model.name { - llm.model = Some(interp_to_string(n)); - } - if !model.fallbacks.is_empty() { - let mut fallbacks_by_provider: HashMap> = HashMap::new(); - for entry in &model.fallbacks { - match entry { - ModelRefOrSplice::ModelRef(model_ref) => { - let s = model_ref.to_string(); - fallbacks_by_provider - .entry(String::new()) - .or_default() - .push(s); - } - ModelRefOrSplice::Splice => {} - } - } - if !fallbacks_by_provider.is_empty() { - llm.fallbacks = Some(fallbacks_by_provider); - } - } - out.llm = Some(llm); - } - - if let Some(prepare) = &run.prepare { - let commands: Vec = prepare - .steps - .iter() - .filter_map(|step| { - if let Some(script) = &step.script { - Some(interp_to_string(script)) - } else { - step.command.as_ref().map(|argv| { - argv.iter() - .map(interp_to_string) - .collect::>() - .join(" ") - }) - } - }) - .collect(); - let timeout_ms = prepare - .timeout - .map(|d| u64::try_from(d.as_std().as_millis()).unwrap_or(u64::MAX)); - out.setup = Some(SetupSettings { - commands, - timeout_ms, - }); - } - - if let Some(execution) = &run.execution { - out.dry_run = match execution.mode { - Some(RunMode::DryRun) => Some(true), - Some(RunMode::Normal) => Some(false), - None => None, - }; - out.auto_approve = match execution.approval { - Some(ApprovalMode::Auto) => Some(true), - Some(ApprovalMode::Prompt) => Some(false), - None => None, - }; - out.no_retro = execution.retros.map(|r| !r); - } - - if let Some(cp) = &run.checkpoint { - out.checkpoint = CheckpointSettings { - exclude_globs: cp.exclude_globs.clone(), - }; - } - - if let Some(sb) = &run.sandbox { - out.sandbox = Some(bridge_sandbox(sb)); - } - - if let Some(agent) = &run.agent { - let map = bridge_mcps(&agent.mcps); - if !map.is_empty() { - out.mcp_servers = map; - } - } - - if !run.hooks.is_empty() { - out.hooks = run.hooks.iter().map(bridge_hook).collect(); - } - - if let Some(pr) = &run.pull_request { - out.pull_request = Some(PullRequestSettings { - enabled: pr.enabled.unwrap_or(false), - draft: pr.draft.unwrap_or(true), - auto_merge: pr.auto_merge.unwrap_or(false), - merge_strategy: pr - .merge_strategy - .map(bridge_merge_strategy) - .unwrap_or_default(), - }); - } - - if let Some(art) = &run.artifacts { - out.artifacts = Some(ArtifactsSettings { - include: art.include.clone(), - }); - } - - // Slack notifications feed the old flat SlackSettings.default_channel. - for route in run.notifications.values() { - if let Some(slack) = &route.slack { - if let Some(channel) = &slack.channel { - out.slack - .get_or_insert_with(SlackSettings::default) - .default_channel = Some(interp_to_string(channel)); - break; - } - } - } - - // Git author from run.git - if let Some(git) = &run.git { - if let Some(author) = &git.author { - let git_settings = out.git.get_or_insert_with(GitSettings::default); - git_settings.author = GitAuthorSettings { - name: author.name.as_ref().map(interp_to_string), - email: author.email.as_ref().map(interp_to_string), - }; - } - } -} - -pub fn bridge_sandbox(sb: &super::run::RunSandboxLayer) -> SandboxSettings { - SandboxSettings { - provider: sb.provider.clone(), - preserve: sb.preserve, - devcontainer: sb.devcontainer, - local: sb.local.as_ref().map(|local| LocalSandboxSettings { - worktree_mode: local - .worktree_mode - .map(bridge_worktree_mode) - .unwrap_or_default(), - }), - daytona: sb.daytona.as_ref().map(|d| DaytonaSettings { - auto_stop_interval: d.auto_stop_interval, - labels: if d.labels.is_empty() { - None - } else { - Some(d.labels.clone()) - }, - snapshot: d.snapshot.as_ref().and_then(|s| { - s.name.as_ref().map(|name| DaytonaSnapshotSettings { - name: name.clone(), - cpu: s.cpu, - memory: s.memory.map(|sz| size_to_gb_i32(sz.as_bytes())), - disk: s.disk.map(|sz| size_to_gb_i32(sz.as_bytes())), - dockerfile: s.dockerfile.as_ref().map(|d| match d { - super::run::DaytonaDockerfileLayer::Inline(text) => { - DockerfileSource::Inline(text.clone()) - } - super::run::DaytonaDockerfileLayer::Path { path } => { - DockerfileSource::Path { path: path.clone() } - } - }), - }) - }), - network: d.network.as_ref().map(|n| match n { - super::run::DaytonaNetworkLayer::Block => DaytonaNetwork::Block, - super::run::DaytonaNetworkLayer::AllowAll => DaytonaNetwork::AllowAll, - super::run::DaytonaNetworkLayer::AllowList { allow_list } => { - DaytonaNetwork::AllowList(allow_list.clone()) - } - }), - skip_clone: d.skip_clone.unwrap_or(false), - }), - env: if sb.env.is_empty() { - None - } else { - Some( - sb.env - .iter() - .map(|(k, v)| (k.clone(), interp_to_string(v))) - .collect(), - ) - }, - } -} - -pub fn bridge_worktree_mode(m: V2WorktreeMode) -> OldWorktreeMode { - match m { - V2WorktreeMode::Always => OldWorktreeMode::Always, - V2WorktreeMode::Clean => OldWorktreeMode::Clean, - V2WorktreeMode::Dirty => OldWorktreeMode::Dirty, - V2WorktreeMode::Never => OldWorktreeMode::Never, - } -} - -pub fn bridge_merge_strategy(m: V2MergeStrategy) -> OldMergeStrategy { - match m { - V2MergeStrategy::Squash => OldMergeStrategy::Squash, - V2MergeStrategy::Merge => OldMergeStrategy::Merge, - V2MergeStrategy::Rebase => OldMergeStrategy::Rebase, - } -} - -pub fn bridge_pull_request(pr: &super::run::RunPullRequestLayer) -> PullRequestSettings { - PullRequestSettings { - enabled: pr.enabled.unwrap_or(false), - draft: pr.draft.unwrap_or(true), - auto_merge: pr.auto_merge.unwrap_or(false), - merge_strategy: pr - .merge_strategy - .map(bridge_merge_strategy) - .unwrap_or_default(), - } -} - -pub fn bridge_run_artifacts(artifacts: &super::run::RunArtifactsLayer) -> ArtifactsSettings { - ArtifactsSettings { - include: artifacts.include.clone(), - } -} - -pub fn bridge_mcps(mcps: &HashMap) -> HashMap { - mcps.iter() - .map(|(name, entry)| (name.clone(), bridge_mcp_entry(entry))) - .collect() -} - -pub fn bridge_mcp_entry(entry: &McpEntryLayer) -> McpServerEntry { - let transport = match entry { - McpEntryLayer::Stdio { - script, - command, - env, - .. - } => { - let command_vec: Vec = if let Some(script) = script { - vec!["sh".into(), "-c".into(), interp_to_string(script)] - } else if let Some(command) = command { - command.iter().map(interp_to_string).collect() - } else { - Vec::new() - }; - McpTransport::Stdio { - command: command_vec, - env: env - .iter() - .map(|(k, v)| (k.clone(), interp_to_string(v))) - .collect(), - } - } - McpEntryLayer::Http { url, headers, .. } => McpTransport::Http { - url: interp_to_string(url), - headers: headers - .iter() - .map(|(k, v)| (k.clone(), interp_to_string(v))) - .collect(), - }, - McpEntryLayer::Sandbox { - script, - command, - port, - env, - .. - } => { - let command_vec: Vec = if let Some(script) = script { - vec!["sh".into(), "-c".into(), interp_to_string(script)] - } else if let Some(command) = command { - command.iter().map(interp_to_string).collect() - } else { - Vec::new() - }; - McpTransport::Sandbox { - command: command_vec, - port: *port, - env: env - .iter() - .map(|(k, v)| (k.clone(), interp_to_string(v))) - .collect(), - } - } - }; - - let (startup_secs, tool_secs) = match entry { - McpEntryLayer::Http { - startup_timeout, - tool_timeout, - .. - } - | McpEntryLayer::Stdio { - startup_timeout, - tool_timeout, - .. - } - | McpEntryLayer::Sandbox { - startup_timeout, - tool_timeout, - .. - } => ( - startup_timeout.map_or(10, |d| d.as_std().as_secs()), - tool_timeout.map_or(60, |d| d.as_std().as_secs()), - ), - }; - - McpServerEntry { - transport, - startup_timeout_secs: startup_secs, - tool_timeout_secs: tool_secs, - } -} - -pub fn bridge_hook(hook: &V2HookEntry) -> HookDefinition { - let hook_type = resolve_hook_type(hook); - // If the hook is a script/command form, emit via the shorthand so the - // old HookDefinition.command field holds the full command and - // HookDefinition.hook_type stays None. This avoids the duplicate - // `command` key that would otherwise appear under `#[serde(flatten)]`. - let command = if let Some(script) = &hook.script { - Some(interp_to_string(script)) - } else { - hook.command.as_ref().map(|command| { - command - .iter() - .map(interp_to_string) - .collect::>() - .join(" ") - }) - }; - HookDefinition { - name: hook.name.clone().or_else(|| hook.id.clone()), - event: bridge_hook_event(hook.event), - command, - hook_type, - matcher: hook.matcher.clone(), - blocking: hook.blocking, - timeout_ms: hook - .timeout - .map(|d| u64::try_from(d.as_std().as_millis()).unwrap_or(u64::MAX)), - sandbox: hook.sandbox, - } -} - -fn resolve_hook_type(hook: &V2HookEntry) -> Option { - // Script/command-shorthand hooks are emitted via the top-level - // HookDefinition.command field in bridge_hook, not here, to avoid - // the `#[serde(flatten)]` duplicate-field collision between the - // outer HookDefinition.command shorthand and the inner - // HookType::Command.command in the legacy old Settings shape. - if hook.script.is_some() || hook.command.is_some() { - return None; - } - if let Some(url) = &hook.url { - let headers = if hook.headers.is_empty() { - None - } else { - Some( - hook.headers - .iter() - .map(|(k, v)| (k.clone(), interp_to_string(v))) - .collect(), - ) - }; - let tls = match hook.tls { - Some(super::run::HookTlsMode::Verify) => OldTlsMode::Verify, - Some(super::run::HookTlsMode::NoVerify) => OldTlsMode::NoVerify, - Some(super::run::HookTlsMode::Off) => OldTlsMode::Off, - None => OldTlsMode::default(), - }; - return Some(OldHookType::Http { - url: interp_to_string(url), - headers, - allowed_env_vars: hook.allowed_env_vars.clone(), - tls, - }); - } - if hook.agent.is_some() { - return Some(OldHookType::Agent { - prompt: hook - .prompt - .as_ref() - .map(interp_to_string) - .unwrap_or_default(), - model: hook.model.as_ref().map(interp_to_string), - max_tool_rounds: hook.max_tool_rounds, - }); - } - hook.prompt.as_ref().map(|prompt| OldHookType::Prompt { - prompt: interp_to_string(prompt), - model: hook.model.as_ref().map(interp_to_string), - }) -} - -fn bridge_hook_event(event: V2HookEvent) -> OldHookEvent { - match event { - V2HookEvent::RunStart => OldHookEvent::RunStart, - V2HookEvent::RunComplete => OldHookEvent::RunComplete, - V2HookEvent::RunFailed => OldHookEvent::RunFailed, - V2HookEvent::StageStart => OldHookEvent::StageStart, - V2HookEvent::StageComplete => OldHookEvent::StageComplete, - V2HookEvent::StageFailed => OldHookEvent::StageFailed, - V2HookEvent::StageRetrying => OldHookEvent::StageRetrying, - V2HookEvent::EdgeSelected => OldHookEvent::EdgeSelected, - V2HookEvent::ParallelStart => OldHookEvent::ParallelStart, - V2HookEvent::ParallelComplete => OldHookEvent::ParallelComplete, - V2HookEvent::SandboxReady => OldHookEvent::SandboxReady, - V2HookEvent::SandboxCleanup => OldHookEvent::SandboxCleanup, - V2HookEvent::CheckpointSaved => OldHookEvent::CheckpointSaved, - V2HookEvent::PreToolUse => OldHookEvent::PreToolUse, - V2HookEvent::PostToolUse => OldHookEvent::PostToolUse, - V2HookEvent::PostToolUseFailure => OldHookEvent::PostToolUseFailure, - } -} - -fn bridge_cli(cli: &CliLayer, out: &mut Settings) { - if let Some(target) = &cli.target { - let target_str = match target { - CliTargetLayer::Http { url, .. } => url.as_ref().map(interp_to_string), - CliTargetLayer::Unix { path } => path.as_ref().map(interp_to_string), - }; - if target_str.is_some() { - out.server = Some(UserServer { - target: target_str, - tls: None, - }); - } - } - - if let Some(exec) = &cli.exec { - out.exec = Some(bridge_exec(exec)); - if let Some(idle) = exec.prevent_idle_sleep { - out.prevent_idle_sleep = Some(idle); - } - } - - if let Some(output) = &cli.output { - bridge_cli_output(output, out); - } - - if let Some(updates) = &cli.updates { - out.upgrade_check = updates.check; - } -} - -pub fn bridge_exec(exec: &CliExecLayer) -> ExecSettings { - ExecSettings { - provider: exec - .model - .as_ref() - .and_then(|m| m.provider.as_ref()) - .map(interp_to_string), - model: exec - .model - .as_ref() - .and_then(|m| m.name.as_ref()) - .map(interp_to_string), - permissions: exec.agent.as_ref().and_then(|a| { - a.permissions.map(|p| match p { - V2AgentPermissions::ReadOnly => PermissionLevel::ReadOnly, - V2AgentPermissions::ReadWrite => PermissionLevel::ReadWrite, - V2AgentPermissions::Full => PermissionLevel::Full, - }) - }), - output_format: None, - } -} - -fn bridge_cli_output(output: &CliOutputLayer, out: &mut Settings) { - if let Some(format) = output.format { - let fmt = match format { - super::cli::OutputFormat::Text => OutputFormat::Text, - super::cli::OutputFormat::Json => OutputFormat::Json, - }; - out.exec - .get_or_insert_with(ExecSettings::default) - .output_format = Some(fmt); - } - if let Some(verbosity) = output.verbosity { - out.verbose = Some(matches!(verbosity, OutputVerbosity::Verbose)); - } -} - -fn bridge_server(server: &ServerLayer, out: &mut Settings) { - if let Some(storage) = &server.storage { - bridge_storage(storage, out); - } - if let Some(scheduler) = &server.scheduler { - bridge_scheduler(scheduler, out); - } - if let Some(artifacts) = &server.artifacts { - out.artifact_storage = Some(bridge_artifacts(artifacts)); - } - if let Some(web) = &server.web { - out.web = Some(bridge_web(web)); - } - if let Some(api) = &server.api { - out.api = Some(ApiSettings { - base_url: api.url.as_ref().map_or_else( - || "http://localhost:3000/api/v1".to_string(), - interp_to_string, - ), - authentication_strategies: bridge_api_auth_strategies(server.auth.as_ref()), - tls: None, - }); - } - if let Some(logging) = &server.logging { - out.log = Some(LogSettings { - level: logging.level.clone(), - }); - } - if let Some(integrations) = &server.integrations { - bridge_integrations(integrations, out); - } -} - -fn bridge_storage(storage: &ServerStorageLayer, out: &mut Settings) { - if let Some(root) = &storage.root { - out.storage_dir = Some(std::path::PathBuf::from(interp_to_string(root))); - } -} - -fn bridge_scheduler(scheduler: &ServerSchedulerLayer, out: &mut Settings) { - out.max_concurrent_runs = scheduler.max_concurrent_runs; -} - -fn bridge_artifacts(a: &ServerArtifactsLayer) -> ArtifactStorageSettings { - let backend = match a.provider { - Some(ObjectStoreProvider::Local) | None => ArtifactStorageBackend::Local, - Some(ObjectStoreProvider::S3) => ArtifactStorageBackend::S3, - }; - let prefix = a - .prefix - .as_ref() - .map_or_else(|| "artifacts".to_string(), interp_to_string); - let (bucket, region, endpoint, path_style) = - a.s3.as_ref().map_or((None, None, None, None), |s3| { - ( - s3.bucket.as_ref().map(interp_to_string), - s3.region.as_ref().map(interp_to_string), - s3.endpoint.as_ref().map(interp_to_string), - s3.path_style, - ) - }); - ArtifactStorageSettings { - backend, - prefix, - bucket, - region, - endpoint, - path_style, - } -} - -fn bridge_web(web: &ServerWebLayer) -> WebSettings { - WebSettings { - enabled: web.enabled.unwrap_or(true), - url: web - .url - .as_ref() - .map_or_else(|| "http://localhost:3000".to_string(), interp_to_string), - auth: AuthSettings { - provider: AuthProvider::Github, - allowed_usernames: Vec::new(), - }, - } -} - -fn bridge_api_auth_strategies( - auth: Option<&super::server::ServerAuthLayer>, -) -> Vec { - let Some(auth) = auth else { - return Vec::new(); - }; - let Some(api) = &auth.api else { - return Vec::new(); - }; - let mut out = Vec::new(); - if let Some(jwt) = &api.jwt { - if jwt.enabled.unwrap_or(true) { - out.push(ApiAuthStrategy::Jwt); - } - } - if let Some(mtls) = &api.mtls { - if mtls.enabled.unwrap_or(true) { - out.push(ApiAuthStrategy::Mtls); - } - } - out -} - -fn bridge_integrations(integrations: &ServerIntegrationsLayer, out: &mut Settings) { - if let Some(github) = &integrations.github { - let git_settings = out.git.get_or_insert_with(|| GitSettings { - provider: GitProvider::Github, - ..GitSettings::default() - }); - if let Some(id) = &github.app_id { - git_settings.app_id = Some(interp_to_string(id)); - } - if let Some(cid) = &github.client_id { - git_settings.client_id = Some(interp_to_string(cid)); - } - if let Some(slug) = &github.slug { - git_settings.slug = Some(interp_to_string(slug)); - } - } - if let Some(slack) = &integrations.slack { - let slack_settings = out.slack.get_or_insert_with(SlackSettings::default); - if let Some(channel) = &slack.default_channel { - slack_settings.default_channel = Some(interp_to_string(channel)); - } - } -} - -// ------------------- shared helpers ------------------- - -fn merge_labels(out: &mut HashMap, src: &HashMap) { - for (k, v) in src { - out.insert(k.clone(), v.clone()); - } -} - -fn interp_to_string(value: &InterpString) -> String { - value.as_source() -} - -fn toml_value_to_string(value: &toml::Value) -> String { - match value { - toml::Value::String(s) => s.clone(), - other => other.to_string(), - } -} - -fn size_to_gb_i32(bytes: u64) -> i32 { - let gb = bytes / 1_000_000_000; - i32::try_from(gb).unwrap_or(i32::MAX) -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn empty_file_bridges_to_empty_settings() { - let file = SettingsFile::default(); - let old = bridge_to_old(&file); - assert_eq!(old.goal, None); - assert_eq!(old.vars, None); - } - - #[test] - fn run_goal_bridges_to_old_goal() { - let file = SettingsFile { - run: Some(RunLayer { - goal: Some(InterpString::parse("Implement OAuth")), - ..RunLayer::default() - }), - ..SettingsFile::default() - }; - let old = bridge_to_old(&file); - assert_eq!(old.goal.as_deref(), Some("Implement OAuth")); - } - - #[test] - fn project_directory_bridges_to_old_fabro_root() { - let file = SettingsFile { - project: Some(ProjectLayer { - directory: Some("fabro/".into()), - ..ProjectLayer::default() - }), - ..SettingsFile::default() - }; - let old = bridge_to_old(&file); - assert_eq!(old.fabro.as_ref().map(|f| f.root.as_str()), Some("fabro/")); - } - - #[test] - fn run_execution_dry_run_bridges_to_old_dry_run_true() { - use super::super::run::{RunExecutionLayer, RunMode}; - let file = SettingsFile { - run: Some(RunLayer { - execution: Some(RunExecutionLayer { - mode: Some(RunMode::DryRun), - ..RunExecutionLayer::default() - }), - ..RunLayer::default() - }), - ..SettingsFile::default() - }; - let old = bridge_to_old(&file); - assert_eq!(old.dry_run, Some(true)); - } - - #[test] - fn run_execution_retros_true_bridges_to_old_no_retro_false() { - use super::super::run::RunExecutionLayer; - let file = SettingsFile { - run: Some(RunLayer { - execution: Some(RunExecutionLayer { - retros: Some(true), - ..RunExecutionLayer::default() - }), - ..RunLayer::default() - }), - ..SettingsFile::default() - }; - let old = bridge_to_old(&file); - assert_eq!(old.no_retro, Some(false)); - } -} diff --git a/lib/crates/fabro-types/src/settings/v2/mod.rs b/lib/crates/fabro-types/src/settings/v2/mod.rs index 0d1baa2e6..9304b4082 100644 --- a/lib/crates/fabro-types/src/settings/v2/mod.rs +++ b/lib/crates/fabro-types/src/settings/v2/mod.rs @@ -7,7 +7,6 @@ //! model references, env interpolation, and splice-capable arrays. pub mod accessors; -pub mod bridge; pub mod cli; pub mod duration; pub mod features; @@ -18,12 +17,11 @@ pub mod run; pub mod server; pub mod size; pub mod splice_array; +pub mod to_runtime; pub mod tree; pub mod version; pub mod workflow; -pub use bridge::bridge_to_old; - pub use cli::CliLayer; pub use duration::{Duration, ParseDurationError}; pub use features::FeaturesLayer; diff --git a/lib/crates/fabro-types/src/settings/v2/to_runtime.rs b/lib/crates/fabro-types/src/settings/v2/to_runtime.rs new file mode 100644 index 000000000..5ba403cba --- /dev/null +++ b/lib/crates/fabro-types/src/settings/v2/to_runtime.rs @@ -0,0 +1,325 @@ +//! v2 → runtime-type conversion helpers. +//! +//! The runtime types in `fabro_types::settings::{hook,mcp,run,sandbox}` are +//! the shapes that downstream crates (fabro-workflow, fabro-mcp, +//! fabro-sandbox, fabro-hooks) still consume at runtime. Each helper here +//! reads the v2 parse tree and builds the equivalent runtime value. +//! +//! These helpers replace the deleted `bridge_to_old` seam from Stage 6.2. +//! They are narrower: each builds a single runtime type from a single v2 +//! subtree, rather than assembling a full legacy [`Settings`] struct. +//! +//! Stage 6.3 deletes the legacy runtime types themselves. At that point +//! these helpers either disappear or get rewritten against the v2-native +//! replacements. + +use std::collections::HashMap; + +use super::interp::InterpString; +use super::run::{ + HookEntry as V2HookEntry, HookEvent as V2HookEvent, McpEntryLayer, + MergeStrategy as V2MergeStrategy, RunArtifactsLayer, RunPullRequestLayer, RunSandboxLayer, + WorktreeMode as V2WorktreeMode, +}; +use crate::settings::hook::{ + HookDefinition, HookEvent as OldHookEvent, HookType as OldHookType, TlsMode as OldTlsMode, +}; +use crate::settings::mcp::{McpServerEntry, McpTransport}; +use crate::settings::run::{ + ArtifactsSettings, MergeStrategy as OldMergeStrategy, PullRequestSettings, +}; +use crate::settings::sandbox::{ + DaytonaNetwork, DaytonaSettings, DaytonaSnapshotSettings, DockerfileSource, + LocalSandboxSettings, SandboxSettings, WorktreeMode as OldWorktreeMode, +}; + +pub fn bridge_sandbox(sb: &RunSandboxLayer) -> SandboxSettings { + SandboxSettings { + provider: sb.provider.clone(), + preserve: sb.preserve, + devcontainer: sb.devcontainer, + local: sb.local.as_ref().map(|local| LocalSandboxSettings { + worktree_mode: local + .worktree_mode + .map(bridge_worktree_mode) + .unwrap_or_default(), + }), + daytona: sb.daytona.as_ref().map(|d| DaytonaSettings { + auto_stop_interval: d.auto_stop_interval, + labels: if d.labels.is_empty() { + None + } else { + Some(d.labels.clone()) + }, + snapshot: d.snapshot.as_ref().and_then(|s| { + s.name.as_ref().map(|name| DaytonaSnapshotSettings { + name: name.clone(), + cpu: s.cpu, + memory: s.memory.map(|sz| size_to_gb_i32(sz.as_bytes())), + disk: s.disk.map(|sz| size_to_gb_i32(sz.as_bytes())), + dockerfile: s.dockerfile.as_ref().map(|d| match d { + super::run::DaytonaDockerfileLayer::Inline(text) => { + DockerfileSource::Inline(text.clone()) + } + super::run::DaytonaDockerfileLayer::Path { path } => { + DockerfileSource::Path { path: path.clone() } + } + }), + }) + }), + network: d.network.as_ref().map(|n| match n { + super::run::DaytonaNetworkLayer::Block => DaytonaNetwork::Block, + super::run::DaytonaNetworkLayer::AllowAll => DaytonaNetwork::AllowAll, + super::run::DaytonaNetworkLayer::AllowList { allow_list } => { + DaytonaNetwork::AllowList(allow_list.clone()) + } + }), + skip_clone: d.skip_clone.unwrap_or(false), + }), + env: if sb.env.is_empty() { + None + } else { + Some( + sb.env + .iter() + .map(|(k, v)| (k.clone(), interp_to_string(v))) + .collect(), + ) + }, + } +} + +pub fn bridge_worktree_mode(m: V2WorktreeMode) -> OldWorktreeMode { + match m { + V2WorktreeMode::Always => OldWorktreeMode::Always, + V2WorktreeMode::Clean => OldWorktreeMode::Clean, + V2WorktreeMode::Dirty => OldWorktreeMode::Dirty, + V2WorktreeMode::Never => OldWorktreeMode::Never, + } +} + +pub fn bridge_merge_strategy(m: V2MergeStrategy) -> OldMergeStrategy { + match m { + V2MergeStrategy::Squash => OldMergeStrategy::Squash, + V2MergeStrategy::Merge => OldMergeStrategy::Merge, + V2MergeStrategy::Rebase => OldMergeStrategy::Rebase, + } +} + +pub fn bridge_pull_request(pr: &RunPullRequestLayer) -> PullRequestSettings { + PullRequestSettings { + enabled: pr.enabled.unwrap_or(false), + draft: pr.draft.unwrap_or(true), + auto_merge: pr.auto_merge.unwrap_or(false), + merge_strategy: pr + .merge_strategy + .map(bridge_merge_strategy) + .unwrap_or_default(), + } +} + +pub fn bridge_run_artifacts(artifacts: &RunArtifactsLayer) -> ArtifactsSettings { + ArtifactsSettings { + include: artifacts.include.clone(), + } +} + +pub fn bridge_mcps(mcps: &HashMap) -> HashMap { + mcps.iter() + .map(|(name, entry)| (name.clone(), bridge_mcp_entry(entry))) + .collect() +} + +pub fn bridge_mcp_entry(entry: &McpEntryLayer) -> McpServerEntry { + let transport = match entry { + McpEntryLayer::Stdio { + script, + command, + env, + .. + } => { + let command_vec: Vec = if let Some(script) = script { + vec!["sh".into(), "-c".into(), interp_to_string(script)] + } else if let Some(command) = command { + command.iter().map(interp_to_string).collect() + } else { + Vec::new() + }; + McpTransport::Stdio { + command: command_vec, + env: env + .iter() + .map(|(k, v)| (k.clone(), interp_to_string(v))) + .collect(), + } + } + McpEntryLayer::Http { url, headers, .. } => McpTransport::Http { + url: interp_to_string(url), + headers: headers + .iter() + .map(|(k, v)| (k.clone(), interp_to_string(v))) + .collect(), + }, + McpEntryLayer::Sandbox { + script, + command, + port, + env, + .. + } => { + let command_vec: Vec = if let Some(script) = script { + vec!["sh".into(), "-c".into(), interp_to_string(script)] + } else if let Some(command) = command { + command.iter().map(interp_to_string).collect() + } else { + Vec::new() + }; + McpTransport::Sandbox { + command: command_vec, + port: *port, + env: env + .iter() + .map(|(k, v)| (k.clone(), interp_to_string(v))) + .collect(), + } + } + }; + + let (startup_secs, tool_secs) = match entry { + McpEntryLayer::Http { + startup_timeout, + tool_timeout, + .. + } + | McpEntryLayer::Stdio { + startup_timeout, + tool_timeout, + .. + } + | McpEntryLayer::Sandbox { + startup_timeout, + tool_timeout, + .. + } => ( + startup_timeout.map_or(10, |d| d.as_std().as_secs()), + tool_timeout.map_or(60, |d| d.as_std().as_secs()), + ), + }; + + McpServerEntry { + transport, + startup_timeout_secs: startup_secs, + tool_timeout_secs: tool_secs, + } +} + +pub fn bridge_hook(hook: &V2HookEntry) -> HookDefinition { + let hook_type = resolve_hook_type(hook); + // If the hook is a script/command form, emit via the shorthand so the + // old HookDefinition.command field holds the full command and + // HookDefinition.hook_type stays None. This avoids the duplicate + // `command` key that would otherwise appear under `#[serde(flatten)]`. + let command = if let Some(script) = &hook.script { + Some(interp_to_string(script)) + } else { + hook.command.as_ref().map(|command| { + command + .iter() + .map(interp_to_string) + .collect::>() + .join(" ") + }) + }; + HookDefinition { + name: hook.name.clone().or_else(|| hook.id.clone()), + event: bridge_hook_event(hook.event), + command, + hook_type, + matcher: hook.matcher.clone(), + blocking: hook.blocking, + timeout_ms: hook + .timeout + .map(|d| u64::try_from(d.as_std().as_millis()).unwrap_or(u64::MAX)), + sandbox: hook.sandbox, + } +} + +fn resolve_hook_type(hook: &V2HookEntry) -> Option { + // Script/command-shorthand hooks are emitted via the top-level + // HookDefinition.command field in bridge_hook, not here, to avoid + // the `#[serde(flatten)]` duplicate-field collision between the + // outer HookDefinition.command shorthand and the inner + // HookType::Command.command in the legacy old Settings shape. + if hook.script.is_some() || hook.command.is_some() { + return None; + } + if let Some(url) = &hook.url { + let headers = if hook.headers.is_empty() { + None + } else { + Some( + hook.headers + .iter() + .map(|(k, v)| (k.clone(), interp_to_string(v))) + .collect(), + ) + }; + let tls = match hook.tls { + Some(super::run::HookTlsMode::Verify) => OldTlsMode::Verify, + Some(super::run::HookTlsMode::NoVerify) => OldTlsMode::NoVerify, + Some(super::run::HookTlsMode::Off) => OldTlsMode::Off, + None => OldTlsMode::default(), + }; + return Some(OldHookType::Http { + url: interp_to_string(url), + headers, + allowed_env_vars: hook.allowed_env_vars.clone(), + tls, + }); + } + if hook.agent.is_some() { + return Some(OldHookType::Agent { + prompt: hook + .prompt + .as_ref() + .map(interp_to_string) + .unwrap_or_default(), + model: hook.model.as_ref().map(interp_to_string), + max_tool_rounds: hook.max_tool_rounds, + }); + } + hook.prompt.as_ref().map(|prompt| OldHookType::Prompt { + prompt: interp_to_string(prompt), + model: hook.model.as_ref().map(interp_to_string), + }) +} + +fn bridge_hook_event(event: V2HookEvent) -> OldHookEvent { + match event { + V2HookEvent::RunStart => OldHookEvent::RunStart, + V2HookEvent::RunComplete => OldHookEvent::RunComplete, + V2HookEvent::RunFailed => OldHookEvent::RunFailed, + V2HookEvent::StageStart => OldHookEvent::StageStart, + V2HookEvent::StageComplete => OldHookEvent::StageComplete, + V2HookEvent::StageFailed => OldHookEvent::StageFailed, + V2HookEvent::StageRetrying => OldHookEvent::StageRetrying, + V2HookEvent::EdgeSelected => OldHookEvent::EdgeSelected, + V2HookEvent::ParallelStart => OldHookEvent::ParallelStart, + V2HookEvent::ParallelComplete => OldHookEvent::ParallelComplete, + V2HookEvent::SandboxReady => OldHookEvent::SandboxReady, + V2HookEvent::SandboxCleanup => OldHookEvent::SandboxCleanup, + V2HookEvent::CheckpointSaved => OldHookEvent::CheckpointSaved, + V2HookEvent::PreToolUse => OldHookEvent::PreToolUse, + V2HookEvent::PostToolUse => OldHookEvent::PostToolUse, + V2HookEvent::PostToolUseFailure => OldHookEvent::PostToolUseFailure, + } +} + +fn interp_to_string(value: &InterpString) -> String { + value.as_source() +} + +fn size_to_gb_i32(bytes: u64) -> i32 { + let gb = bytes / 1_000_000_000; + i32::try_from(gb).unwrap_or(i32::MAX) +} diff --git a/lib/crates/fabro-workflow/src/operations/start.rs b/lib/crates/fabro-workflow/src/operations/start.rs index fdb2d56b7..2a0d8e43f 100644 --- a/lib/crates/fabro-workflow/src/operations/start.rs +++ b/lib/crates/fabro-workflow/src/operations/start.rs @@ -10,10 +10,10 @@ use fabro_interview::{AutoApproveInterviewer, Interviewer}; use fabro_model::{Catalog, FallbackTarget, Provider}; use fabro_sandbox::{SandboxProvider, SandboxSpec}; use fabro_types::RunId; -use fabro_types::settings::v2::bridge::{ +use fabro_types::settings::v2::run::ModelRefOrSplice; +use fabro_types::settings::v2::to_runtime::{ bridge_hook, bridge_mcp_entry, bridge_pull_request, bridge_sandbox, bridge_worktree_mode, }; -use fabro_types::settings::v2::run::ModelRefOrSplice; use fabro_types::settings::v2::{InterpString, SettingsFile}; use crate::artifact_upload::ArtifactSink;