Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
fullsend-ai-coder wants to merge 3 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
797a658
fix(#5448): use field-level merge for validation_loop in base composi…
fullsend-ai-coder[bot] Jul 22, 2026
05b2a68
fix(#5448): correct doc comment on mergeValidationLoop
fullsend-ai-coder[bot] Jul 22, 2026
36dcd1e
fix(#5448): align DiffHarness and validateForge with field-level merge
fullsend-ai-coder[bot] Jul 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 36 additions & 9 deletions internal/harness/compose.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,12 +421,43 @@ 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 a copy of it.
func mergeValidationLoop(base, child *ValidationLoop) *ValidationLoop {

@waynesun09 waynesun09 Jul 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] — DiffHarness was not updated to match the new field-level merge, breaking the base/child round-trip used by migrate-customizations

ADR-0045's Consequences section states the base: merge rules and their inverse (DiffHarness) must change together ("Changes to merge rules must be reflected in both directions"), but this PR only updates the merge side. DiffHarness (internal/harness/diff.go:179-186) and diffForgeConfig (diff.go:470-473) still treat ValidationLoop as whole-struct: if child.ValidationLoop != nil && !reflect.DeepEqual(child.ValidationLoop, base.ValidationLoop) { result.Child.ValidationLoop = child.ValidationLoop }. When old whole-struct-replace merge was in effect, copying the entire child struct into the diff output was correct. With the new field-level mergeValidationLoop, it's not: if the child's ValidationLoop differs from base in just one field (say Script) while another field (say Schema) is genuinely empty/unset for that child, the whole child struct — including that genuine empty Schema — gets copied into the "minimal" diff. Re-composing that diffed-out child against the same base via mergeBaseIntoChild then backfills Schema from base, silently changing effective validation config.

This is reachable in production: internal/cli/migrate.go:479 calls harness.DiffHarness(&upstreamHarness, &customizedHarness, ...) as the core of the migrate-customizations command (ADR-0064), which converts standalone customized harnesses into base:-composed ones — exactly the diff → later-remerge path described above.

Suggestion: Diff ValidationLoop field-by-field in DiffHarness/diffForgeConfig (mirroring mergeValidationLoop's field set), consistent with how RunnerEnv/Env are already diffed field-by-field elsewhere in the same file. Add a round-trip test where base sets schema/feedback_mode and the differing child intentionally leaves them zero.

(Independently identified by 2 of 3 review agents, both verified by tracing the actual code paths.)

if base == nil && child == nil {
return nil
}
if base == nil {
return child
}
if child == nil {
return base
}

merged := *child // start with child values
Comment thread
waynesun09 marked this conversation as resolved.
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

@waynesun09 waynesun09 Jul 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] premature-decision — ADR-0045 still documents whole-struct replacement for validation_loop; not updated alongside this behavior change

This comment (and its counterpart in forge.go's mergeForgeConfig doc) says the merge follows "ADR-0045 merge rules," but docs/ADRs/0045-forge-portable-harness-schema.md wasn't touched by this PR and still documents the old rule in three places: the forge inheritance table ("validation_loop | Forge value replaces top-level value entirely"), the base-composition merge-rules list ("validation_loop: child replaces base entirely (if non-nil)"), and the Open Questions section (still reasons about validation_loop on the premise "non-nil (including zero struct) = override entirely"). The ADR is the authoritative source these code comments explicitly cite; leaving it stale means the documented architecture no longer matches the code, and the ADR's own nil-vs-empty open question for validation_loop is left looking unresolved when this PR actually changes that behavior (see the other comment on this file re: validation_loop: {}).

Suggestion: Amend ADR-0045's inheritance table, base-merge-rules list, and open-questions note for validation_loop to describe the new field-level merge, in this PR or a fast-follow.

(Flagged independently by 2 of 3 review agents; both quoted the exact stale ADR passages.)

// - 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)
Expand Down Expand Up @@ -536,10 +567,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.
Expand Down Expand Up @@ -1272,10 +1301,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
Expand Down
182 changes: 180 additions & 2 deletions internal/harness/compose_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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", `
Expand All @@ -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()

Expand All @@ -350,6 +410,100 @@ 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 TestLoadWithBase_ChainedBases(t *testing.T) {
dir := t.TempDir()

Expand Down Expand Up @@ -1038,6 +1192,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()

Expand Down
7 changes: 3 additions & 4 deletions internal/harness/forge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,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
Expand All @@ -134,9 +134,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 {
Expand Down
26 changes: 26 additions & 0 deletions internal/harness/forge_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,32 @@ func TestResolveForge_ValidationLoopReplace(t *testing.T) {
assert.Equal(t, 1, h.ValidationLoop.MaxIterations)
}

func TestResolveForge_ValidationLoopFieldLevelMerge(t *testing.T) {
Comment thread
waynesun09 marked this conversation as resolved.
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",
Expand Down
Loading