scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-09-15 23:32:46 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

test(settings): update fabro-cli test suite for v2 settings shape

Browse source 
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) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-04-09 15:36:09 -04:00
parent dc856d0884
commit 52c295cf76
No known key found for this signature in database
5 changed files with 187 additions and 71 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

87
lib/crates/fabro-cli/src/commands/config/mod.rs
View file

@ -83,17 +83,82 @@ async fn merged_config(args: &SettingsArgs) -> anyhow::Result<SettingsFile> {
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<()> {

45
lib/crates/fabro-cli/tests/it/cmd/attach.rs
View file

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

39
lib/crates/fabro-cli/tests/it/cmd/config.rs
View file

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

71
lib/crates/fabro-cli/tests/it/cmd/run.rs
View file

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

16
lib/crates/fabro-cli/tests/it/cmd/support.rs
View file

@ -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]",