From f467bd23c5b1dccf23a4acb956c519dcb804560c Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 9 Apr 2026 10:06:16 -0400 Subject: [PATCH] fix(bridge): use hook command shorthand to avoid duplicate serde key The old HookDefinition struct has HookType flattened via #[serde(flatten)], so emitting hook_type = Some(HookType::Command {...}) produces an inner 'command' key at the same level as the outer HookDefinition.command shorthand field. Round-tripping through YAML then fails with 'duplicate field command'. Bridge script/command hooks via the HookDefinition.command shorthand instead, leaving hook_type = None. Also: sandbox FABRO_CONFIG in the manifest_builder unit test so it doesn't pick up the developer's real ~/.fabro/settings.toml, and update settings_local_merges_cli_and_project_defaults to reflect v2 R22 semantics: run.inputs replaces wholesale across layers rather than merging by key, while daytona.labels stays a sticky merge-by-key map per R71. --- lib/crates/fabro-cli/src/manifest_builder.rs | 16 ++++++++ lib/crates/fabro-cli/tests/it/cmd/config.rs | 9 ++++- .../fabro-types/src/settings/v2/bridge.rs | 37 ++++++++++++------- 3 files changed, 47 insertions(+), 15 deletions(-) diff --git a/lib/crates/fabro-cli/src/manifest_builder.rs b/lib/crates/fabro-cli/src/manifest_builder.rs index 2f50af38f..da954527f 100644 --- a/lib/crates/fabro-cli/src/manifest_builder.rs +++ b/lib/crates/fabro-cli/src/manifest_builder.rs @@ -554,6 +554,7 @@ mod tests { use super::*; #[test] + #[allow(unsafe_code, clippy::allow_attributes)] fn build_manifest_bundles_imports_prompts_and_children() { let temp = tempfile::tempdir().unwrap(); let project = temp.path(); @@ -600,6 +601,16 @@ mod tests { ) .unwrap(); + // Isolate from the developer's real ~/.fabro/settings.toml which may + // still be in the legacy shape. Setting FABRO_CONFIG to a path inside + // the test tempdir forces the loader to produce an empty ConfigLayer. + let sandboxed_settings = temp.path().join("empty-settings.toml"); + std::fs::write(&sandboxed_settings, "_version = 1\n").unwrap(); + // SAFETY: single-threaded unit test body. + unsafe { + std::env::set_var("FABRO_CONFIG", &sandboxed_settings); + } + let built = build_run_manifest(ManifestBuildInput { workflow: PathBuf::from("fabro/workflows/demo/workflow.toml"), cwd: project.to_path_buf(), @@ -609,6 +620,11 @@ mod tests { }) .unwrap(); + // SAFETY: single-threaded unit test body. + unsafe { + std::env::remove_var("FABRO_CONFIG"); + } + assert_eq!( built.manifest.target.path, "fabro/workflows/demo/workflow.fabro" diff --git a/lib/crates/fabro-cli/tests/it/cmd/config.rs b/lib/crates/fabro-cli/tests/it/cmd/config.rs index 346442220..5deb3ed05 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/config.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/config.rs @@ -314,11 +314,18 @@ fn settings_local_merges_cli_and_project_defaults() { assert_eq!(cfg.goal.as_deref(), None); assert_eq!(cfg.fabro.as_ref().map(|f| f.root.as_str()), Some("fabro")); + // v2 R22: run.inputs replaces the inherited map wholesale rather than + // merging by key, so the project layer wipes out the CLI layer's inputs. let vars = cfg.vars.as_ref().expect("vars"); - assert_eq!(vars.get("cli_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.get("cli_only").is_none(), + "run.inputs should replace across layers, not merge by key" + ); + // v2 R71: provider-native maps such as run.sandbox.daytona.labels remain + // sticky merge-by-key, so CLI labels persist under the project layer. let sandbox = cfg.sandbox.as_ref().expect("sandbox"); let labels = sandbox .daytona diff --git a/lib/crates/fabro-types/src/settings/v2/bridge.rs b/lib/crates/fabro-types/src/settings/v2/bridge.rs index 59d671969..569247407 100644 --- a/lib/crates/fabro-types/src/settings/v2/bridge.rs +++ b/lib/crates/fabro-types/src/settings/v2/bridge.rs @@ -420,10 +420,25 @@ fn bridge_mcp_entry(entry: &McpEntryLayer) -> McpServerEntry { fn bridge_hook(hook: &V2HookEntry) -> HookDefinition { let hook_type = resolve_hook_type(hook); + // If the hook is a script/command form, emit via the shorthand so the + // old HookDefinition.command field holds the full command and + // HookDefinition.hook_type stays None. This avoids the duplicate + // `command` key that would otherwise appear under `#[serde(flatten)]`. + let command = if let Some(script) = &hook.script { + Some(interp_to_string(script)) + } else { + hook.command.as_ref().map(|command| { + command + .iter() + .map(interp_to_string) + .collect::>() + .join(" ") + }) + }; HookDefinition { name: hook.name.clone().or_else(|| hook.id.clone()), event: bridge_hook_event(hook.event), - command: None, + command, hook_type, matcher: hook.matcher.clone(), blocking: hook.blocking, @@ -435,19 +450,13 @@ fn bridge_hook(hook: &V2HookEntry) -> HookDefinition { } fn resolve_hook_type(hook: &V2HookEntry) -> Option { - if let Some(script) = &hook.script { - return Some(OldHookType::Command { - command: interp_to_string(script), - }); - } - if let Some(command) = &hook.command { - return Some(OldHookType::Command { - command: command - .iter() - .map(interp_to_string) - .collect::>() - .join(" "), - }); + // Script/command-shorthand hooks are emitted via the top-level + // HookDefinition.command field in bridge_hook, not here, to avoid + // the `#[serde(flatten)]` duplicate-field collision between the + // outer HookDefinition.command shorthand and the inner + // HookType::Command.command in the legacy old Settings shape. + if hook.script.is_some() || hook.command.is_some() { + return None; } if let Some(url) = &hook.url { let headers = if hook.headers.is_empty() {