From 52c295cf768cb3381f89096ab83dc4cd9c184014 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 9 Apr 2026 15:36:09 -0400 Subject: [PATCH] test(settings): update fabro-cli test suite for v2 settings shape Completes the fabro-cli test migration for Stage 6.1. Every test in `cargo nextest run --workspace` now passes (3,764 passed / 0 failed). Changes: - cmd/support.rs: compact_inspect / compact_git_inspect now walk the v2 tree (/settings/run/goal, /settings/run/sandbox/provider, /settings/run/model/provider) and derive `dry_run` from the v2 execution mode. - cmd/attach.rs: the event-log filter strips _version and redacts settings.cli.target.path to [CLI_SOCKET] so randomized tempdir sockets don't pollute the snapshot. Insta snapshot accepted. - cmd/run.rs: same cli.target redaction in the run event filter. dry_run_persists_event_history_in_store and json_run_implies_auto_approve_for_human_gates check for `settings.run.execution.approval == "auto"` instead of `settings.auto_approve == true`. Insta snapshot accepted. - cmd/config.rs: parse_settings bridges the v2 YAML output back down to the legacy flat Settings shape so the existing helper assertions keep working. settings_fetches_server_settings_and_merges_with_local_config now asserts the v2 R22 behavior (run.inputs replaces wholesale, so server-side `server_only` is dropped in favor of project's vars). settings_uses_fabro_home_for_home_config_resolution walks the v2 JSON paths (cli.output.verbosity, run.model.name). create_explicit_workflow_path_uses_project_config_relative_to_workflow asserts against the v2 run-record shape. - fabro-cli/commands/config/mod.rs: legacy_settings_to_v2 is now a real (if narrow) reverse bridge covering storage, scheduler, github integration, slack integration, run.model, run.inputs, and cli verbosity. Stage 6.6 still replaces this when the API client returns v2 types natively, but for now the server-side defaults round-trip through the resolver with enough fidelity to keep the settings command integration tests honest. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../fabro-cli/src/commands/config/mod.rs | 87 ++++++++++++++++--- lib/crates/fabro-cli/tests/it/cmd/attach.rs | 45 ++++++---- lib/crates/fabro-cli/tests/it/cmd/config.rs | 39 ++++++--- lib/crates/fabro-cli/tests/it/cmd/run.rs | 71 ++++++++------- lib/crates/fabro-cli/tests/it/cmd/support.rs | 16 ++-- 5 files changed, 187 insertions(+), 71 deletions(-) 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]",