fix(#5448): use field-level merge for validation_loop in base composition

internal/harness/compose.go

@@ -421,12 +421,54 @@ func matchingAllowedPrefix(rawURL string, allowlist []string) string {
 	return MatchingAllowedPrefixInList(rawURL, allowlist)
 }
 
+// mergeValidationLoop merges two ValidationLoop pointers field by field.
+// Child non-zero values win; base fills gaps. Returns the merged result.
+// If both are nil, returns nil. If only one is non-nil, returns it as-is.
+// If child is non-nil but all fields are zero-valued (e.g., from an explicit
+// `validation_loop: {}` in YAML), it is returned as-is so that downstream
+// validation (Validate's "script is required" check) can reject it. This
+// preserves pre-field-merge behavior where an empty block was a load error
+// rather than silent full inheritance.
+func mergeValidationLoop(base, child *ValidationLoop) *ValidationLoop {
+	if base == nil && child == nil {
+		return nil
+	}
+	if base == nil {
+		return child
+	}
+	if child == nil {
+		return base
+	}
+
+	// Non-nil but all-zero child = explicit empty override (e.g., `validation_loop: {}`).
+	// Return as-is to preserve the pre-field-merge rejection behavior.
+	if *child == (ValidationLoop{}) {
+		return child
+	}
+
+	merged := *child // start with child values
+	if merged.Script == "" {
+		merged.Script = base.Script
+	}
+	if merged.Schema == "" {
+		merged.Schema = base.Schema
+	}
+	if merged.MaxIterations == 0 {
+		merged.MaxIterations = base.MaxIterations
+	}
+	if merged.FeedbackMode == "" {
+		merged.FeedbackMode = base.FeedbackMode
+	}
+	return &merged
+}
+
 // mergeBaseIntoChild merges base harness fields into child harness.
 // Child values override base values following ADR-0045 merge rules:
 //   - Scalars: child overrides if non-zero
 //   - Slices (skills, plugins, providers, api_servers): base + child (concatenated)
 //   - Maps (runner_env): base merged with child; child keys win
-//   - Pointer structs (validation_loop, security): child replaces if non-nil
+//   - Pointer structs (validation_loop): field-level merge; child non-zero fields win
+//   - Pointer structs (security): child replaces if non-nil
 //   - host_files: concatenated with last-writer-wins dedup by Dest
 //   - forge: key-by-key merge; per-platform uses same rules
 //   - allowed_remote_resources: NOT merged (security; child must declare its own)
@@ -536,10 +578,8 @@ func mergeBaseIntoChild(base, child *Harness) {
 		child.Env.mergeEnvFrom(base.Env, false)
 	}
 
-	// Pointer structs: child replaces if non-nil
-	if child.ValidationLoop == nil {
-		child.ValidationLoop = base.ValidationLoop
-	}
+	// ValidationLoop: field-level merge; child non-zero fields win
+	child.ValidationLoop = mergeValidationLoop(base.ValidationLoop, child.ValidationLoop)
 	// Security: child inherits base's config if nil. Note that a base harness
 	// (even integrity-pinned) could set fail_mode: open. Child authors must
 	// explicitly set their own security block to prevent inheriting a weaker posture.
@@ -1272,10 +1312,8 @@ func mergeForgeConfigInto(base, child *ForgeConfig) {
 		child.Env.mergeEnvFrom(base.Env, false)
 	}
 
-	// ValidationLoop: child replaces if non-nil
-	if child.ValidationLoop == nil {
-		child.ValidationLoop = base.ValidationLoop
-	}
+	// ValidationLoop: field-level merge; child non-zero fields win
+	child.ValidationLoop = mergeValidationLoop(base.ValidationLoop, child.ValidationLoop)
 }
 
 // FetchAgentHarness fetches a URL-sourced agent harness using the same

internal/harness/compose_test.go

@@ -298,7 +298,7 @@ host_files:
 	assert.Equal(t, "/dest3", h.HostFiles[2].Dest)
 }
 
-func TestLoadWithBase_LocalBase_ValidationLoopReplace(t *testing.T) {
+func TestLoadWithBase_LocalBase_ValidationLoopAllFieldsOverride(t *testing.T) {
 	dir := t.TempDir()
 
 	writeTestHarness(t, dir, "base.yaml", `
@@ -319,12 +319,72 @@ validation_loop:
 	h, _, err := LoadWithBase(context.Background(), path, ComposeOpts{})
 	require.NoError(t, err)
 
-	// ValidationLoop: child replaces entirely
+	// ValidationLoop: child fields win when all set
 	require.NotNil(t, h.ValidationLoop)
 	assert.Equal(t, "child-script.sh", h.ValidationLoop.Script)
 	assert.Equal(t, 3, h.ValidationLoop.MaxIterations)
 }
 
+func TestLoadWithBase_LocalBase_ValidationLoopFieldLevelMerge(t *testing.T) {
+	dir := t.TempDir()
+
+	writeTestHarness(t, dir, "base.yaml", `
+agent: agents/test.md
+role: test
+validation_loop:
+  script: base-script.sh
+  max_iterations: 5
+  feedback_mode: append
+  schema: base-schema.json
+`)
+
+	path := writeTestHarness(t, dir, "child.yaml", `
+base: base.yaml
+validation_loop:
+  schema: child-schema.json
+`)
+
+	h, _, err := LoadWithBase(context.Background(), path, ComposeOpts{})
+	require.NoError(t, err)
+
+	// ValidationLoop: child schema wins, base fills gaps
+	require.NotNil(t, h.ValidationLoop)
+	assert.Equal(t, "base-script.sh", h.ValidationLoop.Script)
+	assert.Equal(t, 5, h.ValidationLoop.MaxIterations)
+	assert.Equal(t, "append", h.ValidationLoop.FeedbackMode)
+	assert.Equal(t, "child-schema.json", h.ValidationLoop.Schema)
+}
+
+func TestLoadWithBase_LocalBase_ValidationLoopPartialScriptOverride(t *testing.T) {
+	dir := t.TempDir()
+
+	writeTestHarness(t, dir, "base.yaml", `
+agent: agents/test.md
+role: test
+validation_loop:
+  script: base-script.sh
+  max_iterations: 5
+  feedback_mode: append
+  schema: base-schema.json
+`)
+
+	path := writeTestHarness(t, dir, "child.yaml", `
+base: base.yaml
+validation_loop:
+  script: child-script.sh
+`)
+
+	h, _, err := LoadWithBase(context.Background(), path, ComposeOpts{})
+	require.NoError(t, err)
+
+	// Child sets only script; base schema/iterations/feedback carry through
+	require.NotNil(t, h.ValidationLoop)
+	assert.Equal(t, "child-script.sh", h.ValidationLoop.Script)
+	assert.Equal(t, 5, h.ValidationLoop.MaxIterations)
+	assert.Equal(t, "append", h.ValidationLoop.FeedbackMode)
+	assert.Equal(t, "base-schema.json", h.ValidationLoop.Schema)
+}
+
 func TestLoadWithBase_LocalBase_ValidationLoopInherit(t *testing.T) {
 	dir := t.TempDir()
 
@@ -350,6 +410,120 @@ model: opus
 	assert.Equal(t, 5, h.ValidationLoop.MaxIterations)
 }
 
+func TestMergeValidationLoop(t *testing.T) {
+	tests := []struct {
+		name     string
+		base     *ValidationLoop
+		child    *ValidationLoop
+		expected *ValidationLoop
+	}{
+		{
+			name:     "both nil",
+			base:     nil,
+			child:    nil,
+			expected: nil,
+		},
+		{
+			name: "base nil",
+			base: nil,
+			child: &ValidationLoop{
+				Script:        "child.sh",
+				MaxIterations: 3,
+			},
+			expected: &ValidationLoop{
+				Script:        "child.sh",
+				MaxIterations: 3,
+			},
+		},
+		{
+			name: "child nil",
+			base: &ValidationLoop{
+				Script:        "base.sh",
+				MaxIterations: 5,
+			},
+			child: nil,
+			expected: &ValidationLoop{
+				Script:        "base.sh",
+				MaxIterations: 5,
+			},
+		},
+		{
+			name: "child partial override - schema only",
+			base: &ValidationLoop{
+				Script:        "base.sh",
+				Schema:        "base-schema.json",
+				MaxIterations: 5,
+				FeedbackMode:  "append",
+			},
+			child: &ValidationLoop{
+				Schema: "child-schema.json",
+			},
+			expected: &ValidationLoop{
+				Script:        "base.sh",
+				Schema:        "child-schema.json",
+				MaxIterations: 5,
+				FeedbackMode:  "append",
+			},
+		},
+		{
+			name: "child full override",
+			base: &ValidationLoop{
+				Script:        "base.sh",
+				Schema:        "base-schema.json",
+				MaxIterations: 5,
+				FeedbackMode:  "append",
+			},
+			child: &ValidationLoop{
+				Script:        "child.sh",
+				Schema:        "child-schema.json",
+				MaxIterations: 3,
+				FeedbackMode:  "replace",
+			},
+			expected: &ValidationLoop{
+				Script:        "child.sh",
+				Schema:        "child-schema.json",
+				MaxIterations: 3,
+				FeedbackMode:  "replace",
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := mergeValidationLoop(tt.base, tt.child)
+			if tt.expected == nil {
+				assert.Nil(t, result)
+			} else {
+				require.NotNil(t, result)
+				assert.Equal(t, tt.expected.Script, result.Script)
+				assert.Equal(t, tt.expected.Schema, result.Schema)
+				assert.Equal(t, tt.expected.MaxIterations, result.MaxIterations)
+				assert.Equal(t, tt.expected.FeedbackMode, result.FeedbackMode)
+			}
+		})
+	}
+}
+
+func TestMergeValidationLoop_AllZeroChild(t *testing.T) {
+	// An explicit `validation_loop: {}` should NOT inherit from base.
+	// The all-zero child is returned as-is so Validate() can reject it
+	// (script is required), matching pre-field-merge behavior.
+	base := &ValidationLoop{
+		Script:        "base.sh",
+		Schema:        "base-schema.json",
+		MaxIterations: 5,
+		FeedbackMode:  "append",
+	}
+	child := &ValidationLoop{} // all zero — explicit empty override
+
+	result := mergeValidationLoop(base, child)
+	require.NotNil(t, result)
+	assert.Equal(t, "", result.Script, "all-zero child should not inherit base script")
+	assert.Equal(t, "", result.Schema, "all-zero child should not inherit base schema")
+	assert.Equal(t, 0, result.MaxIterations, "all-zero child should not inherit base max_iterations")
+	assert.Equal(t, "", result.FeedbackMode, "all-zero child should not inherit base feedback_mode")
+}
+
 func TestLoadWithBase_ChainedBases(t *testing.T) {
 	dir := t.TempDir()
 
@@ -1038,6 +1212,30 @@ func TestMergeForgeConfigInto_ValidationLoop(t *testing.T) {
 	assert.Equal(t, 5, child.ValidationLoop.MaxIterations)
 }
 
+func TestMergeForgeConfigInto_ValidationLoopFieldLevelMerge(t *testing.T) {
+	base := &ForgeConfig{
+		ValidationLoop: &ValidationLoop{
+			Script:        "base-validate.sh",
+			MaxIterations: 5,
+			FeedbackMode:  "append",
+			Schema:        "base-schema.json",
+		},
+	}
+	child := &ForgeConfig{
+		ValidationLoop: &ValidationLoop{
+			Schema: "child-schema.json",
+		},
+	}
+
+	mergeForgeConfigInto(base, child)
+
+	require.NotNil(t, child.ValidationLoop)
+	assert.Equal(t, "base-validate.sh", child.ValidationLoop.Script)
+	assert.Equal(t, 5, child.ValidationLoop.MaxIterations)
+	assert.Equal(t, "append", child.ValidationLoop.FeedbackMode)
+	assert.Equal(t, "child-schema.json", child.ValidationLoop.Schema)
+}
+
 func TestLoadWithBase_InvalidForgeAfterMerge(t *testing.T) {
 	dir := t.TempDir()
 

internal/harness/diff.go

@@ -175,9 +175,9 @@ func DiffHarness(base, child *Harness, customizedFiles map[string]bool) *DiffRes
 		hasAny = true
 	}
 
-	// Pointer structs — keep if non-nil and different; abort if child removes.
-	if child.ValidationLoop != nil && !reflect.DeepEqual(child.ValidationLoop, base.ValidationLoop) {
-		result.Child.ValidationLoop = child.ValidationLoop
+	// ValidationLoop — field-level diff to match mergeValidationLoop semantics.
+	if vlDiff := diffValidationLoop(base.ValidationLoop, child.ValidationLoop); vlDiff != nil {
+		result.Child.ValidationLoop = vlDiff
 		hasAny = true
 	} else if child.ValidationLoop == nil && base.ValidationLoop != nil {
 		result.Warnings = append(result.Warnings, "validation_loop: child removes block from base; cannot express with base: composition")
@@ -467,8 +467,8 @@ func diffForgeConfig(base, child *ForgeConfig, platform string) (*ForgeConfig, [
 		fc.Skills = extras
 		hasAny = true
 	}
-	if child.ValidationLoop != nil && !reflect.DeepEqual(child.ValidationLoop, base.ValidationLoop) {
-		fc.ValidationLoop = child.ValidationLoop
+	if vlDiff := diffValidationLoop(base.ValidationLoop, child.ValidationLoop); vlDiff != nil {
+		fc.ValidationLoop = vlDiff
 		hasAny = true
 	}
 	if d, mapRemoved := diffStringMap(base.RunnerEnv, child.RunnerEnv); len(d) > 0 || mapRemoved {
@@ -491,3 +491,45 @@ func diffForgeConfig(base, child *ForgeConfig, platform string) (*ForgeConfig, [
 	}
 	return fc, warnings
 }
+
+// diffValidationLoop returns a ValidationLoop containing only the fields
+// where child differs from base. Returns nil when both are nil, both are
+// equal, or child is nil (removal is handled by the caller). This mirrors
+// mergeValidationLoop's field set so diff→merge round-trips correctly for
+// non-zero field values.
+func diffValidationLoop(base, child *ValidationLoop) *ValidationLoop {
+	if child == nil {
+		return nil
+	}
+	if base == nil {
+		return child
+	}
+	if reflect.DeepEqual(base, child) {
+		return nil
+	}
+
+	vl := &ValidationLoop{}
+	hasAny := false
+
+	if child.Script != base.Script {
+		vl.Script = child.Script
+		hasAny = true
+	}
+	if child.Schema != base.Schema {
+		vl.Schema = child.Schema
+		hasAny = true
+	}
+	if child.MaxIterations != base.MaxIterations {
+		vl.MaxIterations = child.MaxIterations
+		hasAny = true
+	}
+	if child.FeedbackMode != base.FeedbackMode {
+		vl.FeedbackMode = child.FeedbackMode
+		hasAny = true
+	}
+
+	if !hasAny {
+		return nil
+	}
+	return vl
+}