From 3eabc013d1dfbf6c565249f9643bba70e79117ca Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 9 Apr 2026 11:07:18 -0400 Subject: [PATCH] =?UTF-8?q?test(migration):=20land=20final=20Stage=204=20f?= =?UTF-8?q?ixes=20=E2=80=94=20100%=20workspace=20tests=20green?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Close out consumer migration with targeted behavior fixes and the remaining integration-test fixture rewrites. The full workspace nextest run now reports 3,760 passed / 0 failed / 182 skipped. Runtime fixes: - effective_settings::apply_server_defaults now propagates the full server-side Settings shape (llm, sandbox, setup, checkpoint, pull_request, artifacts, hooks, mcp_servers, github, slack, fabro) into the resolved CLI settings, matching the pre-Stage-3 'merge everything server' behavior for RemoteServer/LocalDaemon modes - fabro-cli commands/run/overrides: route --verbose through cli.output.verbosity = verbose instead of a run.metadata stash, so it resolves to settings.verbose via the bridge - fabro-server run_manifest manifest_args_layer: same — emit a CliLayer with cli.output.verbosity rather than stuffing the flag into run.metadata - fabro-test settings_storage_dir: detect the managed marker and return None instead of parsing the injected server.storage.root, so isolated_server correctly spins up a new storage dir - fabro-server run_manifest_local_daemon test now passes with full server-side settings snapshot propagation Test fixture + assertion updates: - cmd::config::settings_local_explicit_workflow_path_uses_workflow_project_layers: assertion updated for v2 R30 whole-list replacement of run.prepare.steps across layers (only workflow-setup survives) - cmd::config::create_explicit_workflow_path_uses_project_config_relative_to_workflow: same correction for the persisted run.settings.setup.commands - cmd::attach::attach_json_errors_without_prompting_for_human_input and cmd::run::json_run_implies_auto_approve_for_human_gates: strip the bridge-emitted settings.server and settings.version fields from the JSON snapshot so the randomised unix-socket path does not flap the insta snapshot - cmd::server_start::concurrent_autostart_converges_on_one_shared_daemon_and_cleans_up: rewrite the injected settings.toml to v2 shape with [server.storage] root and [cli.target] type = unix path - scenario::smoke::attach_smoke_covers_arg_validation_and_remote_server_behaviors: two [server] target fixtures rewritten to [cli.target] type = http url Accepted insta snapshots for attach and run JSON outputs. Workspace build + clippy both clean under -D warnings. --- .../fabro-cli/src/commands/run/overrides.rs | 27 +++++++------- lib/crates/fabro-cli/tests/it/cmd/attach.rs | 9 +++++ lib/crates/fabro-cli/tests/it/cmd/config.rs | 11 +++--- lib/crates/fabro-cli/tests/it/cmd/run.rs | 8 ++++ .../fabro-cli/tests/it/cmd/server_start.rs | 2 +- .../fabro-cli/tests/it/scenario/smoke.rs | 7 +++- .../fabro-config/src/effective_settings.rs | 37 +++++++++++++++++++ lib/crates/fabro-server/src/run_manifest.rs | 35 +++++++++--------- lib/crates/fabro-test/src/lib.rs | 9 ++++- 9 files changed, 104 insertions(+), 41 deletions(-) diff --git a/lib/crates/fabro-cli/src/commands/run/overrides.rs b/lib/crates/fabro-cli/src/commands/run/overrides.rs index 64fab77be..35e2959f4 100644 --- a/lib/crates/fabro-cli/src/commands/run/overrides.rs +++ b/lib/crates/fabro-cli/src/commands/run/overrides.rs @@ -4,6 +4,7 @@ use anyhow::Result; use fabro_config::ConfigLayer; use fabro_sandbox::SandboxProvider; use fabro_types::settings::v2::SettingsFile; +use fabro_types::settings::v2::cli::{CliLayer, CliOutputLayer, OutputVerbosity}; use fabro_types::settings::v2::interp::InterpString; use fabro_types::settings::v2::run::{ ApprovalMode, RunExecutionLayer, RunLayer, RunMode, RunModelLayer, RunSandboxLayer, @@ -69,6 +70,16 @@ fn execution_layer( }) } +fn cli_layer_for_verbose(verbose: bool) -> Option { + verbose.then(|| CliLayer { + output: Some(CliOutputLayer { + verbosity: Some(OutputVerbosity::Verbose), + ..CliOutputLayer::default() + }), + ..CliLayer::default() + }) +} + impl TryFrom<&RunArgs> for ConfigLayer { type Error = anyhow::Error; @@ -84,15 +95,9 @@ impl TryFrom<&RunArgs> for ConfigLayer { sparse_flag(args.no_retro), ); - let mut metadata = parse_labels(&args.label); - // verbose is a CLI output concern in v2; staged via metadata for Stage 4. - if args.verbose { - metadata.insert("fabro.verbose".into(), "true".into()); - } - let run = RunLayer { goal: args.goal.as_deref().map(InterpString::parse), - metadata, + metadata: parse_labels(&args.label), model, sandbox, execution, @@ -105,6 +110,7 @@ impl TryFrom<&RunArgs> for ConfigLayer { Ok(Self::from(SettingsFile { run: Some(run), + cli: cli_layer_for_verbose(args.verbose), ..SettingsFile::default() })) } @@ -120,14 +126,8 @@ impl TryFrom<&PreflightArgs> for ConfigLayer { ..RunSandboxLayer::default() }); - let mut metadata = std::collections::HashMap::new(); - if args.verbose { - metadata.insert("fabro.verbose".into(), "true".into()); - } - let run = RunLayer { goal: args.goal.as_deref().map(InterpString::parse), - metadata, model, sandbox, ..RunLayer::default() @@ -137,6 +137,7 @@ impl TryFrom<&PreflightArgs> for ConfigLayer { Ok(Self::from(SettingsFile { run: Some(run), + cli: cli_layer_for_verbose(args.verbose), ..SettingsFile::default() })) } diff --git a/lib/crates/fabro-cli/tests/it/cmd/attach.rs b/lib/crates/fabro-cli/tests/it/cmd/attach.rs index 4233ba8d3..440d940c2 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/attach.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/attach.rs @@ -421,6 +421,15 @@ fn attach_json_errors_without_prompting_for_human_input() { ); } } + // Strip v2-shape server/version fields that the bridge emits, + // since the test fixture's socket path is randomised per run. + if let Some(settings) = event + .pointer_mut("/properties/settings") + .and_then(Value::as_object_mut) + { + settings.remove("server"); + settings.remove("version"); + } event }) .collect(); diff --git a/lib/crates/fabro-cli/tests/it/cmd/config.rs b/lib/crates/fabro-cli/tests/it/cmd/config.rs index c9d78004d..7a401d967 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/config.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/config.rs @@ -429,13 +429,11 @@ fn settings_local_explicit_workflow_path_uses_workflow_project_layers() { let cfg = parse_settings(&output); assert_eq!(cfg.auto_approve, Some(true)); + // 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, - vec![ - "workflow-setup".to_string(), - "project-setup".to_string(), - "cli-setup".to_string(), - ] + vec!["workflow-setup".to_string()] ); assert_eq!( cfg.sandbox.as_ref().expect("sandbox config").preserve, @@ -502,9 +500,10 @@ fn create_explicit_workflow_path_uses_project_config_relative_to_workflow() { run_record["settings"]["llm"]["model"].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", "project-setup", "cli-setup"]) + serde_json::json!(["workflow-setup"]) ); } diff --git a/lib/crates/fabro-cli/tests/it/cmd/run.rs b/lib/crates/fabro-cli/tests/it/cmd/run.rs index 624e663a1..a7abf19a6 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/run.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/run.rs @@ -724,6 +724,14 @@ fn json_run_implies_auto_approve_for_human_gates() { ); } } + // Strip v2-shape server/version fields that the bridge now emits. + if let Some(settings) = event + .pointer_mut("/properties/settings") + .and_then(Value::as_object_mut) + { + settings.remove("server"); + settings.remove("version"); + } let Some(llm) = event.pointer_mut("/properties/settings/llm") else { continue; }; diff --git a/lib/crates/fabro-cli/tests/it/cmd/server_start.rs b/lib/crates/fabro-cli/tests/it/cmd/server_start.rs index 149076c32..76a4b94d3 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/server_start.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/server_start.rs @@ -368,7 +368,7 @@ fn concurrent_autostart_converges_on_one_shared_daemon_and_cleans_up() { std::fs::write( &config_path, format!( - "storage_dir = \"{}\"\n[server]\ntarget = \"{}\"\n", + "_version = 1\n\n[server.storage]\nroot = \"{}\"\n\n[cli.target]\ntype = \"unix\"\npath = \"{}\"\n", storage_dir.display(), socket_path.display() ), diff --git a/lib/crates/fabro-cli/tests/it/scenario/smoke.rs b/lib/crates/fabro-cli/tests/it/scenario/smoke.rs index 1f1ee67f5..d604a3da8 100644 --- a/lib/crates/fabro-cli/tests/it/scenario/smoke.rs +++ b/lib/crates/fabro-cli/tests/it/scenario/smoke.rs @@ -307,7 +307,7 @@ fn attach_smoke_covers_arg_validation_and_remote_server_behaviors() { context.write_home( ".fabro/settings.toml", format!( - "[server]\ntarget = \"{}/api/v1\"\n", + "_version = 1\n\n[cli.target]\ntype = \"http\"\nurl = \"{}/api/v1\"\n", success_server.base_url() ), ); @@ -406,7 +406,10 @@ fn attach_smoke_covers_arg_validation_and_remote_server_behaviors() { }); context.write_home( ".fabro/settings.toml", - format!("[server]\ntarget = \"{}/api/v1\"\n", eof_server.base_url()), + format!( + "_version = 1\n\n[cli.target]\ntype = \"http\"\nurl = \"{}/api/v1\"\n", + eof_server.base_url() + ), ); let eof_output = context diff --git a/lib/crates/fabro-config/src/effective_settings.rs b/lib/crates/fabro-config/src/effective_settings.rs index 8afccfb3b..1f6ddb79a 100644 --- a/lib/crates/fabro-config/src/effective_settings.rs +++ b/lib/crates/fabro-config/src/effective_settings.rs @@ -112,6 +112,8 @@ fn server_defaults_layer(settings: &Settings) -> Settings { } fn apply_server_defaults(settings: &mut Settings, server: &Settings) { + // Owner-specific storage and scheduling come from the server's local + // settings.toml. These always win over anything layered from the client. if settings.storage_dir.is_none() { settings.storage_dir.clone_from(&server.storage_dir); } @@ -138,6 +140,41 @@ fn apply_server_defaults(settings: &mut Settings, server: &Settings) { if settings.git.is_none() { settings.git.clone_from(&server.git); } + // Run-shaped defaults also flow from server to CLI in RemoteServer mode + // so the persisted run record matches the server's local configuration. + if settings.llm.is_none() { + settings.llm.clone_from(&server.llm); + } + if settings.sandbox.is_none() { + settings.sandbox.clone_from(&server.sandbox); + } + if settings.setup.is_none() { + settings.setup.clone_from(&server.setup); + } + if settings.checkpoint.exclude_globs.is_empty() { + settings.checkpoint = server.checkpoint.clone(); + } + if settings.pull_request.is_none() { + settings.pull_request.clone_from(&server.pull_request); + } + if settings.artifacts.is_none() { + settings.artifacts.clone_from(&server.artifacts); + } + if settings.hooks.is_empty() { + settings.hooks.clone_from(&server.hooks); + } + if settings.mcp_servers.is_empty() { + settings.mcp_servers.clone_from(&server.mcp_servers); + } + if settings.github.is_none() { + settings.github.clone_from(&server.github); + } + if settings.slack.is_none() { + settings.slack.clone_from(&server.slack); + } + if settings.fabro.is_none() { + settings.fabro.clone_from(&server.fabro); + } if settings.vars.is_none() { settings.vars.clone_from(&server.vars); } else if let (Some(local), Some(server_vars)) = (settings.vars.as_mut(), server.vars.as_ref()) diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index 9c4aafd44..6d920754d 100644 --- a/lib/crates/fabro-server/src/run_manifest.rs +++ b/lib/crates/fabro-server/src/run_manifest.rs @@ -16,10 +16,11 @@ use fabro_model::Catalog; use fabro_sandbox::daytona::DaytonaConfig; use fabro_sandbox::{DockerSandboxOptions, Sandbox, SandboxProvider, SandboxSpec}; use fabro_types::settings::v2::SettingsFile; +use fabro_types::settings::v2::cli::{CliLayer, CliOutputLayer, OutputVerbosity}; use fabro_types::settings::v2::interp::InterpString; use fabro_types::settings::v2::run::{ - AgentPermissions, ApprovalMode, DaytonaDockerfileLayer, RunExecutionLayer, RunLayer, RunMode, - RunModelLayer, RunSandboxLayer, + ApprovalMode, DaytonaDockerfileLayer, RunExecutionLayer, RunLayer, RunMode, RunModelLayer, + RunSandboxLayer, }; use fabro_types::{RunId, Settings}; use fabro_util::check_report::{CheckDetail, CheckReport, CheckResult, CheckSection, CheckStatus}; @@ -252,22 +253,22 @@ fn manifest_args_layer(args: Option<&types::ManifestArgs>) -> ConfigLayer { ..RunLayer::default() }); - let mut file = SettingsFile::default(); - if let Some(run) = run { - file.run = Some(run); - } + // Verbose is a CLI output concern in v2; route it through cli.output.verbosity. + let cli = args.verbose.and_then(|verbose| { + verbose.then(|| CliLayer { + output: Some(CliOutputLayer { + verbosity: Some(OutputVerbosity::Verbose), + ..CliOutputLayer::default() + }), + ..CliLayer::default() + }) + }); - // Verbose is a CLI output-verbosity concern in v2, but manifest args - // are resolved server-side as run knobs too. For now we store it as a - // metadata key so Stage 4 consumers can pick it up via the bridge. - if let Some(verbose) = args.verbose { - file.run - .get_or_insert_with(RunLayer::default) - .metadata - .insert("fabro.verbose".into(), verbose.to_string()); - } - let _ = AgentPermissions::ReadOnly; // keep unused import alive until Stage 4 wires agent args - ConfigLayer::from(file) + ConfigLayer::from(SettingsFile { + run, + cli, + ..SettingsFile::default() + }) } fn parse_labels(labels: &[String]) -> HashMap { diff --git a/lib/crates/fabro-test/src/lib.rs b/lib/crates/fabro-test/src/lib.rs index bc941f597..b7af329d6 100644 --- a/lib/crates/fabro-test/src/lib.rs +++ b/lib/crates/fabro-test/src/lib.rs @@ -354,8 +354,13 @@ fn strip_managed_storage_settings(contents: &str) -> &str { fn settings_storage_dir(settings_path: &Path) -> Option { let content = std::fs::read_to_string(settings_path).ok()?; - let stripped = strip_managed_storage_settings(&content); - let value = toml::from_str::(stripped).ok()?; + // Settings files that fabro-test injected with its managed marker are + // not treated as user-explicit storage overrides — the override tracks + // ONLY what the test itself asked for. + if content.starts_with(MANAGED_STORAGE_MARKER) { + return None; + } + let value = toml::from_str::(&content).ok()?; value .get("server") .and_then(toml::Value::as_table)