diff --git a/docs/contributing/harness-composition.md b/docs/contributing/harness-composition.md index edf3ddb6d..95d4eb534 100644 --- a/docs/contributing/harness-composition.md +++ b/docs/contributing/harness-composition.md @@ -63,6 +63,14 @@ For example, if `mergeBaseIntoChild` gains handling for a new `foo_script` scalar field, then `mergeForgeConfigInto` must also handle `foo_script` if it appears inside `ForgeConfig`. +> **Note — post-merge clearing.** After `mergeForgeBlocks`, +> `mergeBaseIntoChild` clears inherited forge-level fields that would +> conflict with the child's explicit top-level values during +> `ResolveForge` ([#6798](https://github.com/fullsend-ai/fullsend/issues/6798)). +> This clearing step is specific to base composition — `ResolveForge` +> itself does not perform it. When adding a new field to `ForgeConfig`, +> ensure the clearing loop in `mergeBaseIntoChild` handles it as well. + > **Note — removed counterparts.** Earlier versions of this document > referenced path-rewriting functions in `internal/cli/migrate.go` and > diff functions (`DiffHarness`, `diffForgeConfig`) as counterparts to diff --git a/docs/contributing/harness-fields.md b/docs/contributing/harness-fields.md index 42e47d781..7ce7113de 100644 --- a/docs/contributing/harness-fields.md +++ b/docs/contributing/harness-fields.md @@ -66,7 +66,12 @@ per-overlay: When a forge block or overlay is merged into the harness top level, each field type follows specific merge semantics. The same rules apply during -`base:` composition (base → child merging). +`base:` composition (base → child merging), with one addition: during +base composition, `mergeBaseIntoChild` clears inherited forge-level fields +that would conflict with the child's explicit top-level values before +`ResolveForge` runs (see [#6798](https://github.com/fullsend-ai/fullsend/issues/6798)). +This prevents inherited forge platforms from silently overriding a child's +explicit top-level intent. | Field type | Merge behavior | Nil vs empty | |------------------|------------------------------------------------------|-------------------------------------------------------| diff --git a/internal/harness/compose.go b/internal/harness/compose.go index aa7608546..4bdbff968 100644 --- a/internal/harness/compose.go +++ b/internal/harness/compose.go @@ -541,9 +541,47 @@ func matchingAllowedPrefix(rawURL string, allowlist []string) string { // - Maps (runner_env): base merged with child; child keys win // - Pointer structs (validation_loop, 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 +// - forge: key-by-key merge; per-platform uses same rules; +// inherited platforms have fields cleared where child set explicit top-level values (#6798) // - allowed_remote_resources: NOT merged (security; child must declare its own) func mergeBaseIntoChild(base, child *Harness) { + // Snapshot child's explicit top-level fields BEFORE any merge so we + // can clear inherited forge-level fields that would override them + // during ResolveForge (see #6798). + childHasPreScript := child.PreScript != "" + childHasPostScript := child.PostScript != "" + childHasPolicy := child.Policy != "" + childHasSkills := len(child.Skills) > 0 + childHasProviders := len(child.Providers) > 0 + childHasOpenShellProfiles := child.OpenShell != nil && len(child.OpenShell.Profiles) > 0 + childHasHostFiles := len(child.HostFiles) > 0 + childHasValidationLoop := child.ValidationLoop != nil + + // Snapshot child's explicit runner_env keys so we only remove matching + // inherited forge keys, not all inherited env vars. + childRunnerEnvKeys := make(map[string]bool, len(child.RunnerEnv)) + for k := range child.RunnerEnv { + childRunnerEnvKeys[k] = true + } + + // Snapshot child's explicit env sub-map keys. + childEnvRunnerKeys := make(map[string]bool) + childEnvSandboxKeys := make(map[string]bool) + if child.Env != nil { + for k := range child.Env.Runner { + childEnvRunnerKeys[k] = true + } + for k := range child.Env.Sandbox { + childEnvSandboxKeys[k] = true + } + } + + // Snapshot child's existing forge platform keys before mergeForgeBlocks. + childForgeKeys := make(map[string]bool, len(child.Forge)) + for k := range child.Forge { + childForgeKeys[k] = true + } + // Scalars: child overrides if non-zero if child.Agent == "" { child.Agent = base.Agent @@ -671,6 +709,77 @@ func mergeBaseIntoChild(base, child *Harness) { // Forge: key-by-key merge if base.Forge != nil { child.Forge = mergeForgeBlocks(base.Forge, child.Forge) + + // Clear inherited forge-level fields that would override the child's + // explicit top-level values during ResolveForge (#6798). + // + // mergeForgeBlocks inherits the entire base ForgeConfig wholesale for + // platforms the child doesn't define. mergeForgeConfig (called by + // ResolveForge) then blindly applies those inherited forge-level + // values over the child's top-level values. By clearing inherited + // fields here, we ensure the child's explicit top-level intent is + // preserved. + for key, fc := range child.Forge { + if childForgeKeys[key] || fc == nil { + continue // child explicitly defined this platform + } + // Scalars: clear if child explicitly set the top-level value + if childHasPreScript { + fc.PreScript = "" + } + if childHasPostScript { + fc.PostScript = "" + } + if childHasPolicy { + fc.Policy = "" + } + // Slices: clear if child explicitly defined its own top-level entries + if childHasSkills { + fc.Skills = nil + } + if childHasProviders { + fc.Providers = nil + } + if childHasOpenShellProfiles && fc.OpenShell != nil { + fc.OpenShell.Profiles = nil + } + if childHasHostFiles { + fc.HostFiles = nil + } + if childHasValidationLoop { + fc.ValidationLoop = nil + } + // Maps: remove only keys the child explicitly set at top level + if fc.RunnerEnv != nil { + for k := range childRunnerEnvKeys { + delete(fc.RunnerEnv, k) + } + if len(fc.RunnerEnv) == 0 { + fc.RunnerEnv = nil + } + } + if fc.Env != nil { + if fc.Env.Runner != nil { + for k := range childEnvRunnerKeys { + delete(fc.Env.Runner, k) + } + if len(fc.Env.Runner) == 0 { + fc.Env.Runner = nil + } + } + if fc.Env.Sandbox != nil { + for k := range childEnvSandboxKeys { + delete(fc.Env.Sandbox, k) + } + if len(fc.Env.Sandbox) == 0 { + fc.Env.Sandbox = nil + } + } + if fc.Env.Runner == nil && fc.Env.Sandbox == nil { + fc.Env = nil + } + } + } } // Overlays: concatenated (base first, child appended) — same as plugins, diff --git a/internal/harness/compose_test.go b/internal/harness/compose_test.go index 709244bc4..442bbd10b 100644 --- a/internal/harness/compose_test.go +++ b/internal/harness/compose_test.go @@ -629,6 +629,242 @@ model: opus assert.Equal(t, "gl-pre.sh", h.PreScript) } +func TestLoadWithBase_ChildTopLevelOverridesInheritedForge(t *testing.T) { + t.Run("scalars", func(t *testing.T) { + dir := t.TempDir() + + writeTestHarness(t, dir, "base.yaml", ` +agent: agents/test.md +role: test +pre_script: base-top-pre.sh +post_script: base-top-post.sh +forge: + github: + pre_script: base-forge-pre.sh + post_script: base-forge-post.sh + policy: base-forge-policy.yaml + gitlab: + pre_script: base-forge-gl-pre.sh + post_script: base-forge-gl-post.sh +`) + + // Child sets top-level scripts but has no forge block. + // Child's top-level values must survive ResolveForge. + path := writeTestHarness(t, dir, "child.yaml", ` +base: base.yaml +pre_script: child-pre.sh +post_script: child-post.sh +`) + + h, _, err := LoadWithBase(context.Background(), path, ComposeOpts{ + ForgePlatform: "github", + }) + require.NoError(t, err) + + assert.Equal(t, "child-pre.sh", h.PreScript, "child top-level pre_script must survive inherited forge") + assert.Equal(t, "child-post.sh", h.PostScript, "child top-level post_script must survive inherited forge") + // Policy was not set by child, so inherited forge value should apply. + assert.Equal(t, "base-forge-policy.yaml", h.Policy, "inherited forge policy should apply when child did not set it") + }) + + t.Run("partial scalars", func(t *testing.T) { + dir := t.TempDir() + + writeTestHarness(t, dir, "base.yaml", ` +agent: agents/test.md +role: test +forge: + github: + pre_script: base-forge-pre.sh + post_script: base-forge-post.sh +`) + + // Child sets only pre_script but not post_script. + path := writeTestHarness(t, dir, "child.yaml", ` +base: base.yaml +pre_script: child-pre.sh +`) + + h, _, err := LoadWithBase(context.Background(), path, ComposeOpts{ + ForgePlatform: "github", + }) + require.NoError(t, err) + + assert.Equal(t, "child-pre.sh", h.PreScript, "child top-level pre_script must win") + assert.Equal(t, "base-forge-post.sh", h.PostScript, "inherited forge post_script should apply when child did not set it") + }) + + t.Run("child has forge for one platform", func(t *testing.T) { + dir := t.TempDir() + + writeTestHarness(t, dir, "base.yaml", ` +agent: agents/test.md +role: test +forge: + github: + pre_script: base-forge-gh-pre.sh + gitlab: + pre_script: base-forge-gl-pre.sh +`) + + // Child has a forge block for github but not gitlab. + // Only gitlab should have inherited fields cleared. + path := writeTestHarness(t, dir, "child.yaml", ` +base: base.yaml +pre_script: child-pre.sh +forge: + github: + post_script: child-forge-gh-post.sh +`) + + h, _, err := LoadWithBase(context.Background(), path, ComposeOpts{ + ForgePlatform: "gitlab", + }) + require.NoError(t, err) + + // GitLab was inherited; child's top-level pre_script should win. + assert.Equal(t, "child-pre.sh", h.PreScript, "child top-level pre_script must survive inherited gitlab forge") + }) + + t.Run("skills and providers", func(t *testing.T) { + dir := t.TempDir() + + writeTestHarness(t, dir, "base.yaml", ` +agent: agents/test.md +role: test +forge: + github: + skills: + - base-forge-skill + providers: + - base-forge-provider +`) + + // Child sets top-level skills and providers. + path := writeTestHarness(t, dir, "child.yaml", ` +base: base.yaml +skills: + - child-skill +providers: + - child-provider +`) + + h, _, err := LoadWithBase(context.Background(), path, ComposeOpts{ + ForgePlatform: "github", + }) + require.NoError(t, err) + + // Inherited forge skills/providers should not be concatenated onto child's. + assert.Equal(t, []string{"child-skill"}, SkillSources(h.Skills), + "child top-level skills must not be polluted by inherited forge skills") + assert.Equal(t, []string{"child-provider"}, h.Providers, + "child top-level providers must not be polluted by inherited forge providers") + }) + + t.Run("runner env", func(t *testing.T) { + dir := t.TempDir() + + writeTestHarness(t, dir, "base.yaml", ` +agent: agents/test.md +role: test +forge: + github: + runner_env: + SHARED_KEY: base-forge-value + FORGE_ONLY: forge-only-value +`) + + // Child sets a matching top-level runner_env key. + path := writeTestHarness(t, dir, "child.yaml", ` +base: base.yaml +runner_env: + SHARED_KEY: child-value +`) + + h, _, err := LoadWithBase(context.Background(), path, ComposeOpts{ + ForgePlatform: "github", + }) + require.NoError(t, err) + + assert.Equal(t, "child-value", h.RunnerEnv["SHARED_KEY"], + "child top-level runner_env key must survive inherited forge override") + assert.Equal(t, "forge-only-value", h.RunnerEnv["FORGE_ONLY"], + "inherited forge env key not set by child should still apply") + }) + + t.Run("validation loop", func(t *testing.T) { + dir := t.TempDir() + + writeTestHarness(t, dir, "base.yaml", ` +agent: agents/test.md +role: test +forge: + github: + validation_loop: + script: base-forge-validate.sh + max_iterations: 3 +`) + + // Child sets a top-level validation_loop. + path := writeTestHarness(t, dir, "child.yaml", ` +base: base.yaml +validation_loop: + script: child-validate.sh + max_iterations: 5 +`) + + h, _, err := LoadWithBase(context.Background(), path, ComposeOpts{ + ForgePlatform: "github", + }) + require.NoError(t, err) + + require.NotNil(t, h.ValidationLoop, "validation_loop must not be nil") + assert.Equal(t, "child-validate.sh", h.ValidationLoop.Script, + "child top-level validation_loop must survive inherited forge override") + assert.Equal(t, 5, h.ValidationLoop.MaxIterations, + "child top-level validation_loop max_iterations must survive inherited forge override") + }) + + t.Run("env sub maps", func(t *testing.T) { + dir := t.TempDir() + + writeTestHarness(t, dir, "base.yaml", ` +agent: agents/test.md +role: test +forge: + github: + env: + runner: + SHARED_KEY: base-forge-runner-val + FORGE_ONLY: forge-runner-val + sandbox: + SB_SHARED: base-forge-sb-val +`) + + // Child sets top-level env with a matching key. + path := writeTestHarness(t, dir, "child.yaml", ` +base: base.yaml +env: + runner: + SHARED_KEY: child-runner-val + sandbox: + SB_SHARED: child-sb-val +`) + + h, _, err := LoadWithBase(context.Background(), path, ComposeOpts{ + ForgePlatform: "github", + }) + require.NoError(t, err) + + assert.Equal(t, "child-runner-val", h.Env.Runner["SHARED_KEY"], + "child top-level env.runner key must survive inherited forge override") + assert.Equal(t, "forge-runner-val", h.Env.Runner["FORGE_ONLY"], + "inherited forge env.runner key not set by child should still apply") + assert.Equal(t, "child-sb-val", h.Env.Sandbox["SB_SHARED"], + "child top-level env.sandbox key must survive inherited forge override") + }) +} + func TestLoadWithBase_URLBase(t *testing.T) { baseContent := []byte(` agent: agents/remote.md