diff --git a/internal/harness/compose.go b/internal/harness/compose.go index 9270f5e06..a5ea2ca95 100644 --- a/internal/harness/compose.go +++ b/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 diff --git a/internal/harness/compose_test.go b/internal/harness/compose_test.go index b3556d66e..519282198 100644 --- a/internal/harness/compose_test.go +++ b/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() diff --git a/internal/harness/diff.go b/internal/harness/diff.go index 5b16432a1..3ea933c13 100644 --- a/internal/harness/diff.go +++ b/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 +} diff --git a/internal/harness/diff_test.go b/internal/harness/diff_test.go index 97378f731..42f008c8a 100644 --- a/internal/harness/diff_test.go +++ b/internal/harness/diff_test.go @@ -222,6 +222,54 @@ func TestDiffHarness_ValidationLoopDifference(t *testing.T) { require.NotNil(t, result.Child) require.NotNil(t, result.Child.ValidationLoop) assert.Equal(t, 5, result.Child.ValidationLoop.MaxIterations) + // Field-level diff: unchanged Script should not appear in diff + assert.Equal(t, "", result.Child.ValidationLoop.Script, + "unchanged Script should not be in diff") +} + +func TestDiffHarness_ValidationLoopFieldLevelRoundTrip(t *testing.T) { + // Verify that diff→mergeBaseIntoChild round-trips correctly when only + // some ValidationLoop fields differ. This is the scenario from the HIGH + // review finding: child overrides Script but keeps base Schema/FeedbackMode. + base := &Harness{ + Agent: "agents/test.md", + ValidationLoop: &ValidationLoop{ + Script: "base-validate.sh", + Schema: "base-schema.json", + MaxIterations: 5, + FeedbackMode: "append", + }, + } + child := &Harness{ + Agent: "agents/test.md", + ValidationLoop: &ValidationLoop{ + Script: "child-validate.sh", + Schema: "base-schema.json", + MaxIterations: 5, + FeedbackMode: "append", + }, + } + + result := DiffHarness(base, child, nil) + require.NotNil(t, result.Child, "diff should be non-nil") + require.Empty(t, result.Warnings) + require.NotNil(t, result.Child.ValidationLoop) + + // Only Script differs — other fields should be zero (not included in diff) + assert.Equal(t, "child-validate.sh", result.Child.ValidationLoop.Script) + assert.Equal(t, "", result.Child.ValidationLoop.Schema, + "matching Schema should not be in diff") + assert.Equal(t, 0, result.Child.ValidationLoop.MaxIterations, + "matching MaxIterations should not be in diff") + assert.Equal(t, "", result.Child.ValidationLoop.FeedbackMode, + "matching FeedbackMode should not be in diff") + + // Round-trip: compose the diff child with base + mergeBaseIntoChild(base, result.Child) + assert.Equal(t, child.ValidationLoop.Script, result.Child.ValidationLoop.Script) + assert.Equal(t, child.ValidationLoop.Schema, result.Child.ValidationLoop.Schema) + assert.Equal(t, child.ValidationLoop.MaxIterations, result.Child.ValidationLoop.MaxIterations) + assert.Equal(t, child.ValidationLoop.FeedbackMode, result.Child.ValidationLoop.FeedbackMode) } func TestDiffHarness_NewForgePlatform(t *testing.T) { @@ -524,7 +572,49 @@ func TestDiffForgeConfig_AllFields(t *testing.T) { assert.Equal(t, []string{"skill/b"}, fc.Skills) assert.Equal(t, map[string]string{"K2": "v2"}, fc.RunnerEnv) assert.Equal(t, map[string]string{"R2": "2"}, fc.Env.Runner) - assert.NotNil(t, fc.ValidationLoop) + // Field-level diff: only differing VL fields are included + require.NotNil(t, fc.ValidationLoop) + assert.Equal(t, "validate-v2.sh", fc.ValidationLoop.Script) + assert.Equal(t, 5, fc.ValidationLoop.MaxIterations) +} + +func TestDiffForgeConfig_ValidationLoopFieldLevel(t *testing.T) { + // Forge-level field diff: only the differing field (MaxIterations) should + // appear; Script should not be in the diff since it matches. + base := &Harness{ + Forge: map[string]*ForgeConfig{ + "github": { + ValidationLoop: &ValidationLoop{ + Script: "validate.sh", + Schema: "schema.json", + MaxIterations: 3, + FeedbackMode: "append", + }, + }, + }, + } + child := &Harness{ + Forge: map[string]*ForgeConfig{ + "github": { + ValidationLoop: &ValidationLoop{ + Script: "validate.sh", + Schema: "schema.json", + MaxIterations: 5, + FeedbackMode: "append", + }, + }, + }, + } + result := DiffHarness(base, child, nil) + require.NotNil(t, result.Child) + fc := result.Child.Forge["github"] + require.NotNil(t, fc) + require.NotNil(t, fc.ValidationLoop) + assert.Equal(t, 5, fc.ValidationLoop.MaxIterations) + assert.Equal(t, "", fc.ValidationLoop.Script, + "unchanged Script should not be in forge diff") + assert.Equal(t, "", fc.ValidationLoop.Schema, + "unchanged Schema should not be in forge diff") } func TestDiffHarness_PluginsAddition(t *testing.T) { diff --git a/internal/harness/forge.go b/internal/harness/forge.go index 8c0d08cbe..b13fa8efd 100644 --- a/internal/harness/forge.go +++ b/internal/harness/forge.go @@ -64,7 +64,10 @@ func (h *Harness) validateForge() error { } } if fc.ValidationLoop != nil { - if fc.ValidationLoop.Script == "" { + // Allow empty forge script when the top-level harness has a script + // that will be inherited via field-level merge in ResolveForge. + topHasScript := h.ValidationLoop != nil && h.ValidationLoop.Script != "" + if fc.ValidationLoop.Script == "" && !topHasScript { return fmt.Errorf("forge.%s.validation_loop.script is required when validation_loop is set", key) } if IsURL(fc.ValidationLoop.Script) { @@ -112,7 +115,7 @@ func (h *Harness) ResolveForge(platform string) error { // - Scalars: forge overrides if non-empty // - Skills: top-level + forge (concatenated) // - RunnerEnv: top-level merged with forge; forge keys win -// - ValidationLoop: forge replaces entirely if non-nil +// - ValidationLoop: field-level merge; forge non-zero fields win func mergeForgeConfig(h *Harness, fc *ForgeConfig) { if fc.PreScript != "" { h.PreScript = fc.PreScript @@ -134,9 +137,8 @@ func mergeForgeConfig(h *Harness, fc *ForgeConfig) { } } - if fc.ValidationLoop != nil { - h.ValidationLoop = fc.ValidationLoop - } + // ValidationLoop: field-level merge; forge non-zero fields win + h.ValidationLoop = mergeValidationLoop(h.ValidationLoop, fc.ValidationLoop) // Env: merge sub-maps independently; forge keys win (ADR 0055) if fc.Env != nil { diff --git a/internal/harness/forge_test.go b/internal/harness/forge_test.go index 10576fb69..fc69c9daf 100644 --- a/internal/harness/forge_test.go +++ b/internal/harness/forge_test.go @@ -180,6 +180,35 @@ func TestResolveForge_ValidationLoopReplace(t *testing.T) { assert.Equal(t, 1, h.ValidationLoop.MaxIterations) } +func TestResolveForge_ValidationLoopFieldLevelMerge(t *testing.T) { + // Tests mergeForgeConfig directly (bypasses validateForge). See + // TestLoadWithOpts_ForgePartialValidationLoopInheritsScript for the + // end-to-end path through validateForge → ResolveForge → Validate. + h := &Harness{ + Agent: "agents/test.md", + ValidationLoop: &ValidationLoop{ + Script: "scripts/validate-common.sh", + MaxIterations: 3, + FeedbackMode: "append", + Schema: "common-schema.json", + }, + Forge: map[string]*ForgeConfig{ + "github": { + ValidationLoop: &ValidationLoop{ + Schema: "gh-schema.json", + }, + }, + }, + } + + require.NoError(t, h.ResolveForge("github")) + require.NotNil(t, h.ValidationLoop) + assert.Equal(t, "scripts/validate-common.sh", h.ValidationLoop.Script) + assert.Equal(t, 3, h.ValidationLoop.MaxIterations) + assert.Equal(t, "append", h.ValidationLoop.FeedbackMode) + assert.Equal(t, "gh-schema.json", h.ValidationLoop.Schema) +} + func TestResolveForge_ValidationLoopNilInherits(t *testing.T) { h := &Harness{ Agent: "agents/test.md", diff --git a/internal/harness/harness_test.go b/internal/harness/harness_test.go index a53ec7601..f731cb5eb 100644 --- a/internal/harness/harness_test.go +++ b/internal/harness/harness_test.go @@ -1507,6 +1507,59 @@ forge: assert.Equal(t, "scripts/validate-gh.sh", h.ValidationLoop.Script) } +func TestLoadWithOpts_ForgePartialValidationLoopInheritsScript(t *testing.T) { + // Forge sets only schema; script is inherited from the top-level + // validation_loop via field-level merge. This proves the real loading + // path (validateForge → ResolveForge → Validate) works end-to-end. + content := ` +agent: agents/test.md +role: test +validation_loop: + script: scripts/validate-common.sh + max_iterations: 3 + feedback_mode: append +forge: + github: + validation_loop: + schema: gh-schema.json +` + dir := t.TempDir() + path := filepath.Join(dir, "test.yaml") + require.NoError(t, os.WriteFile(path, []byte(content), 0o644)) + + h, err := LoadWithOpts(path, LoadOpts{ForgePlatform: "github"}) + require.NoError(t, err) + require.NotNil(t, h.ValidationLoop) + assert.Equal(t, "scripts/validate-common.sh", h.ValidationLoop.Script, + "script should be inherited from top-level") + assert.Equal(t, "gh-schema.json", h.ValidationLoop.Schema, + "forge schema should override top-level") + assert.Equal(t, 3, h.ValidationLoop.MaxIterations, + "max_iterations should be inherited from top-level") + assert.Equal(t, "append", h.ValidationLoop.FeedbackMode, + "feedback_mode should be inherited from top-level") +} + +func TestLoadWithOpts_ForgeEmptyValidationLoopNoTopLevelScript(t *testing.T) { + // Forge validation_loop without script and no top-level script should + // still be rejected by validateForge. + content := ` +agent: agents/test.md +role: test +forge: + github: + validation_loop: + schema: gh-schema.json +` + dir := t.TempDir() + path := filepath.Join(dir, "test.yaml") + require.NoError(t, os.WriteFile(path, []byte(content), 0o644)) + + _, err := LoadWithOpts(path, LoadOpts{ForgePlatform: "github"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "validation_loop.script is required") +} + func TestLoadWithOpts_PlatformNotConfigured(t *testing.T) { content := ` agent: agents/test.md