test(migration): land final Stage 4 fixes — 100% workspace tests green

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.
This commit is contained in:
Bryan Helmkamp 2026-04-09 11:07:18 -04:00
parent 16204acf0d
commit 3eabc013d1
9 changed files with 104 additions and 41 deletions

View file

@ -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<CliLayer> {
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()
}))
}

View file

@ -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();

View file

@ -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"])
);
}

View file

@ -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;
};

View file

@ -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()
),

View file

@ -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

View file

@ -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())

View file

@ -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<String, String> {

View file

@ -354,8 +354,13 @@ fn strip_managed_storage_settings(contents: &str) -> &str {
fn settings_storage_dir(settings_path: &Path) -> Option<PathBuf> {
let content = std::fs::read_to_string(settings_path).ok()?;
let stripped = strip_managed_storage_settings(&content);
let value = toml::from_str::<toml::Value>(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::<toml::Value>(&content).ok()?;
value
.get("server")
.and_then(toml::Value::as_table)