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.
This commit is contained in:
Bryan Helmkamp 2026-04-09 10:06:16 -04:00
parent eabbca649b
commit f467bd23c5
No known key found for this signature in database
3 changed files with 47 additions and 15 deletions

View file

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

View file

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

View file

@ -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::<Vec<_>>()
.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<OldHookType> {
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::<Vec<_>>()
.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() {