diff --git a/lib/crates/fabro-cli/src/commands/config/mod.rs b/lib/crates/fabro-cli/src/commands/config/mod.rs index c7ddde42c..414d43ae5 100644 --- a/lib/crates/fabro-cli/src/commands/config/mod.rs +++ b/lib/crates/fabro-cli/src/commands/config/mod.rs @@ -83,17 +83,82 @@ async fn merged_config(args: &SettingsArgs) -> anyhow::Result { effective_settings::resolve_settings(layers, Some(&server_settings), mode) } -/// Stopgap shim that converts a legacy flat `Settings` back into a -/// `SettingsFile` for consumption by the v2-native resolver. This exists -/// because `retrieve_server_settings` still returns the legacy shape -/// across the wire. When Stage 6.6 rewrites the OpenAPI spec to return v2 -/// types, this conversion goes away and the loaded shape stays v2 end to end. -fn legacy_settings_to_v2(_legacy: &fabro_types::Settings) -> SettingsFile { - // TODO: implement a true reverse bridge. For now, return an empty v2 - // file so `resolve_settings(..., Some(&...), RemoteServer)` has a - // non-None server-settings argument. This loses server-side defaults; - // Stage 6.6 fixes the full round-trip. - SettingsFile::default() +/// Stopgap reverse bridge from the legacy flat `Settings` to a v2 +/// `SettingsFile`. `retrieve_server_settings` still returns the legacy +/// shape across the wire; the v2 resolver needs server-settings in v2 +/// shape. This reverse-maps the fields that matter for server-side +/// defaults (storage, scheduler, integrations, verbose, run model). +/// Stage 6.6 rewrites the API client to return v2 types directly and +/// deletes this helper. +fn legacy_settings_to_v2(legacy: &fabro_types::Settings) -> SettingsFile { + use fabro_types::settings::v2::cli::{CliLayer, CliOutputLayer, OutputVerbosity}; + use fabro_types::settings::v2::interp::InterpString; + use fabro_types::settings::v2::run::{RunLayer, RunModelLayer}; + use fabro_types::settings::v2::server::{ + GithubIntegrationLayer, ServerIntegrationsLayer, ServerLayer, ServerSchedulerLayer, + ServerStorageLayer, SlackIntegrationLayer, + }; + + let mut file = SettingsFile::default(); + + if let Some(storage_dir) = legacy.storage_dir.as_ref() { + let server = file.server.get_or_insert_with(ServerLayer::default); + server.storage = Some(ServerStorageLayer { + root: Some(InterpString::parse(&storage_dir.to_string_lossy())), + }); + } + if let Some(max_concurrent) = legacy.max_concurrent_runs { + let server = file.server.get_or_insert_with(ServerLayer::default); + server.scheduler = Some(ServerSchedulerLayer { + max_concurrent_runs: Some(max_concurrent), + }); + } + if let Some(git) = legacy.git.as_ref() { + let server = file.server.get_or_insert_with(ServerLayer::default); + let integrations = server + .integrations + .get_or_insert_with(ServerIntegrationsLayer::default); + let github = integrations + .github + .get_or_insert_with(GithubIntegrationLayer::default); + github.app_id = git.app_id.as_deref().map(InterpString::parse); + github.client_id = git.client_id.as_deref().map(InterpString::parse); + github.slug = git.slug.as_deref().map(InterpString::parse); + } + if let Some(slack) = legacy.slack.as_ref() { + let server = file.server.get_or_insert_with(ServerLayer::default); + let integrations = server + .integrations + .get_or_insert_with(ServerIntegrationsLayer::default); + integrations.slack = Some(SlackIntegrationLayer { + enabled: None, + default_channel: slack.default_channel.as_deref().map(InterpString::parse), + }); + } + if let Some(llm) = legacy.llm.as_ref() { + let run = file.run.get_or_insert_with(RunLayer::default); + run.model = Some(RunModelLayer { + provider: llm.provider.as_deref().map(InterpString::parse), + name: llm.model.as_deref().map(InterpString::parse), + fallbacks: Vec::new(), + }); + } + if let Some(vars) = legacy.vars.as_ref() { + let run = file.run.get_or_insert_with(RunLayer::default); + run.inputs = Some( + vars.iter() + .map(|(k, v)| (k.clone(), toml::Value::String(v.clone()))) + .collect(), + ); + } + if let Some(true) = legacy.verbose { + let cli = file.cli.get_or_insert_with(CliLayer::default); + cli.output = Some(CliOutputLayer { + verbosity: Some(OutputVerbosity::Verbose), + ..CliOutputLayer::default() + }); + } + file } pub(crate) async fn execute(args: &SettingsArgs, globals: &GlobalArgs) -> anyhow::Result<()> { diff --git a/lib/crates/fabro-cli/tests/it/cmd/attach.rs b/lib/crates/fabro-cli/tests/it/cmd/attach.rs index 440d940c2..8acaa062d 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/attach.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/attach.rs @@ -427,9 +427,21 @@ fn attach_json_errors_without_prompting_for_human_input() { .pointer_mut("/properties/settings") .and_then(Value::as_object_mut) { + settings.remove("_version"); settings.remove("server"); settings.remove("version"); } + if let Some(target) = event + .pointer_mut("/properties/settings/cli/target") + .and_then(Value::as_object_mut) + { + if target.contains_key("path") { + target.insert( + "path".to_string(), + Value::String("[CLI_SOCKET]".to_string()), + ); + } + } event }) .collect(); @@ -556,22 +568,25 @@ fn attach_json_errors_without_prompting_for_human_input() { }, "run_dir": "[RUN_DIR]", "settings": { - "goal": "Wait for approval", - "llm": { - "fallbacks": null, - "model": "gpt-5.4", - "provider": "openai" + "run": { + "execution": { + "retros": false + }, + "goal": "Wait for approval", + "model": { + "name": "gpt-5.4", + "provider": "openai" + }, + "sandbox": { + "provider": "local" + } }, - "no_retro": true, - "sandbox": { - "daytona": null, - "devcontainer": null, - "env": null, - "local": null, - "preserve": null, - "provider": "local" - }, - "storage_dir": "[STORAGE_DIR]" + "cli": { + "target": { + "path": "[CLI_SOCKET]", + "type": "unix" + } + } }, "workflow_slug": "human-gate", "workflow_source": "digraph HumanGate {/n graph [goal=\"Wait for approval\"]/n start [shape=Mdiamond, label=\"Start\"]/n exit [shape=Msquare, label=\"Exit\"]/n approve [shape=hexagon, label=\"Approve?\"]/n ship [shape=parallelogram, script=\"echo shipped\"]/n revise [shape=parallelogram, script=\"echo revised\"]/n start -> approve/n approve -> ship [label=\"[A] Approve\"]/n approve -> revise [label=\"[R] Revise\"]/n ship -> exit/n revise -> exit/n}/n", diff --git a/lib/crates/fabro-cli/tests/it/cmd/config.rs b/lib/crates/fabro-cli/tests/it/cmd/config.rs index 7a401d967..9cf7fbe99 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/config.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/config.rs @@ -3,6 +3,7 @@ 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; use httpmock::MockServer; use predicates::prelude::*; @@ -32,7 +33,13 @@ fn old_config_show_command_is_rejected() { // --------------------------------------------------------------------------- fn parse_settings(stdout: &[u8]) -> Settings { - serde_yaml::from_slice(stdout).expect("stdout should be valid YAML 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 server_settings_fixture() -> Settings { @@ -487,23 +494,26 @@ fn create_explicit_workflow_path_uses_project_config_relative_to_workflow() { let state = run_state(&run_dir); let run_record = serde_json::to_value(state.run.as_ref().expect("run record should exist")).unwrap(); - assert_eq!(run_record["settings"]["auto_approve"].as_bool(), Some(true)); assert_eq!( - run_record["settings"]["storage_dir"].as_str(), + run_record["settings"]["run"]["execution"]["approval"].as_str(), + Some("auto") + ); + assert_eq!( + run_record["settings"]["server"]["storage"]["root"].as_str(), Some(storage_dir.to_str().unwrap()) ); assert_eq!( - run_record["settings"]["sandbox"]["preserve"].as_bool(), + run_record["settings"]["run"]["sandbox"]["preserve"].as_bool(), Some(true) ); assert_eq!( - run_record["settings"]["llm"]["model"].as_str(), + run_record["settings"]["run"]["model"]["name"].as_str(), Some("gpt-5.2") ); // v2 R30: run.prepare.steps replaces the whole ordered list across layers. assert_eq!( - run_record["settings"]["setup"]["commands"], - serde_json::json!(["workflow-setup"]) + run_record["settings"]["run"]["prepare"]["steps"], + serde_json::json!([{"script": "workflow-setup"}]) ); } @@ -659,8 +669,11 @@ name = "from-fabro-home" ); let cfg: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap(); - assert_eq!(cfg["verbose"].as_bool(), Some(true)); - assert_eq!(cfg["llm"]["model"].as_str(), Some("from-fabro-home")); + assert_eq!(cfg["cli"]["output"]["verbosity"].as_str(), Some("verbose")); + assert_eq!( + cfg["run"]["model"]["name"].as_str(), + Some("from-fabro-home") + ); } #[test] @@ -756,10 +769,16 @@ shared = "cli" assert_eq!(cfg.storage_dir, Some(PathBuf::from("/srv/fabro-server"))); assert_eq!(cfg.verbose, Some(true)); + // 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("server_only").map(String::as_str), Some("1")); assert_eq!(vars.get("project_only").map(String::as_str), Some("1")); assert_eq!(vars.get("shared").map(String::as_str), Some("project")); + assert!( + !vars.contains_key("server_only"), + "v2 merge matrix replaces run.inputs wholesale; server_only should be dropped" + ); } #[test] diff --git a/lib/crates/fabro-cli/tests/it/cmd/run.rs b/lib/crates/fabro-cli/tests/it/cmd/run.rs index a7abf19a6..e1a9c8eed 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/run.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/run.rs @@ -592,9 +592,9 @@ fn dry_run_persists_event_history_in_store() { assert_eq!( progress .first() - .and_then(|event| event.pointer("/properties/settings/auto_approve")) - .and_then(Value::as_bool), - Some(true) + .and_then(|event| event.pointer("/properties/settings/run/execution/approval")) + .and_then(Value::as_str), + Some("auto") ); assert!( progress @@ -724,25 +724,35 @@ fn json_run_implies_auto_approve_for_human_gates() { ); } } - // Strip v2-shape server/version fields that the bridge now emits. + // Strip fields that vary between runs (version, server stanzas that + // carry machine-specific values, cli.target sockets). if let Some(settings) = event .pointer_mut("/properties/settings") .and_then(Value::as_object_mut) { + settings.remove("_version"); settings.remove("server"); settings.remove("version"); } - let Some(llm) = event.pointer_mut("/properties/settings/llm") else { + if let Some(target) = event + .pointer_mut("/properties/settings/cli/target") + .and_then(Value::as_object_mut) + { + if target.contains_key("path") { + target.insert( + "path".to_string(), + Value::String("[CLI_SOCKET]".to_string()), + ); + } + } + let Some(model) = event.pointer_mut("/properties/settings/run/model") else { continue; }; - let Some(llm) = llm.as_object_mut() else { + let Some(model) = model.as_object_mut() else { continue; }; - llm.insert( - "model".to_string(), - Value::String("[LLM_MODEL]".to_string()), - ); - llm.insert( + model.insert("name".to_string(), Value::String("[LLM_MODEL]".to_string())); + model.insert( "provider".to_string(), Value::String("[LLM_PROVIDER]".to_string()), ); @@ -870,23 +880,26 @@ fn json_run_implies_auto_approve_for_human_gates() { }, "run_dir": "[RUN_DIR]", "settings": { - "auto_approve": true, - "goal": "Route through the default approval path", - "llm": { - "fallbacks": null, - "model": "[LLM_MODEL]", - "provider": "[LLM_PROVIDER]" + "run": { + "execution": { + "approval": "auto", + "retros": false + }, + "goal": "Route through the default approval path", + "model": { + "name": "[LLM_MODEL]", + "provider": "[LLM_PROVIDER]" + }, + "sandbox": { + "provider": "local" + } }, - "no_retro": true, - "sandbox": { - "daytona": null, - "devcontainer": null, - "env": null, - "local": null, - "preserve": null, - "provider": "local" - }, - "storage_dir": "[STORAGE_DIR]" + "cli": { + "target": { + "path": "[CLI_SOCKET]", + "type": "unix" + } + } }, "workflow_slug": "human-gate", "workflow_source": "digraph HumanGate {/n graph [goal=\"Route through the default approval path\"]/n start [shape=Mdiamond, label=\"Start\"]/n exit [shape=Msquare, label=\"Exit\"]/n approve [shape=hexagon, label=\"Approve?\"]/n ship [shape=parallelogram, script=\"echo shipped\"]/n revise [shape=parallelogram, script=\"echo revised\"]/n start -> approve/n approve -> ship [label=\"[A] Approve\"]/n approve -> revise [label=\"[R] Revise\"]/n ship -> exit/n revise -> exit/n}/n", @@ -1431,8 +1444,8 @@ fn json_run_implies_auto_approve_for_human_gates() { "#); assert_eq!( - progress[0].pointer("/properties/settings/auto_approve"), - Some(&serde_json::json!(true)) + progress[0].pointer("/properties/settings/run/execution/approval"), + Some(&serde_json::json!("auto")) ); } diff --git a/lib/crates/fabro-cli/tests/it/cmd/support.rs b/lib/crates/fabro-cli/tests/it/cmd/support.rs index b36c9fbf2..5262bb590 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/support.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/support.rs @@ -803,15 +803,19 @@ pub(crate) fn compact_inspect(output: &Output) -> Value { let checkpoint = item["checkpoint"].clone(); let conclusion = item["conclusion"].clone(); let sandbox = item["sandbox"].clone(); + let dry_run = run_record + .pointer("/settings/run/execution/mode") + .and_then(Value::as_str) + .map(|mode| Value::Bool(mode == "dry_run")); serde_json::json!({ "run_id": "[ULID]", "status": item["status"], "run_record": { - "goal": run_record.pointer("/settings/goal"), + "goal": run_record.pointer("/settings/run/goal"), "workflow_name": run_record.pointer("/graph/name"), "workflow_slug": run_record.pointer("/workflow_slug"), - "sandbox_provider": run_record.pointer("/settings/sandbox/provider"), - "dry_run": run_record.pointer("/settings/dry_run"), + "sandbox_provider": run_record.pointer("/settings/run/sandbox/provider"), + "dry_run": dry_run, "provenance": run_record.pointer("/provenance").as_ref().map(|_| { serde_json::json!({ "server_version": "[VERSION]", @@ -866,11 +870,11 @@ pub(crate) fn compact_git_inspect(output: &Output) -> Value { "run_id": "[ULID]", "status": item["status"], "run_record": { - "goal": run_record.pointer("/settings/goal"), + "goal": run_record.pointer("/settings/run/goal"), "workflow_name": run_record.pointer("/graph/name"), "workflow_slug": run_record.pointer("/workflow_slug"), - "llm_provider": run_record.pointer("/settings/llm/provider"), - "sandbox_provider": run_record.pointer("/settings/sandbox/provider"), + "llm_provider": run_record.pointer("/settings/run/model/provider"), + "sandbox_provider": run_record.pointer("/settings/run/sandbox/provider"), "provenance": run_record.pointer("/provenance").as_ref().map(|_| { serde_json::json!({ "server_version": "[VERSION]",