diff --git a/docs/ADRs/0045-forge-portable-harness-schema.md b/docs/ADRs/0045-forge-portable-harness-schema.md index 9683fb6846..8869271560 100644 --- a/docs/ADRs/0045-forge-portable-harness-schema.md +++ b/docs/ADRs/0045-forge-portable-harness-schema.md @@ -370,6 +370,15 @@ The same inheritance table applies to base→child merging: `forge:` map. For each platform key present in both, the per-platform `ForgeConfig` fields merge using the rules above. + **Exception — inherited forge-level scripts:** When the child explicitly + declares a top-level `pre_script` or `post_script`, any forge-level + equivalents inherited from the base are cleared after merging to prevent + `ResolveForge` from overriding the child's explicit declaration. + Child-authored forge-level scripts are preserved. Without this, a child + harness that declares `pre_script: scripts/custom.sh` would silently + lose it when the base has `forge.github.pre_script` set. See + [#847](https://github.com/fullsend-ai/fullsend/issues/847). + #### URL support `base` can be a URL, reusing ADR 0038's infrastructure: diff --git a/docs/guides/user/bring-your-own-agent.md b/docs/guides/user/bring-your-own-agent.md index 2b9878fe51..834bee50b6 100644 --- a/docs/guides/user/bring-your-own-agent.md +++ b/docs/guides/user/bring-your-own-agent.md @@ -300,7 +300,7 @@ security: | Field type | Behavior | |-----------|----------| -| Scalars (`model`, `pre_script`, `image`, etc.) | Child wins if non-empty | +| Scalars (`model`, `pre_script`, `image`, etc.) | Child wins if non-empty¹ | | `skills` | Merged with deduplication by basename (child overrides base) | | `plugins`, `providers`, `api_servers`, `openshell.profiles` | Concatenated (base + child) | | `host_files` | Concatenated; child overrides by `dest` | @@ -308,6 +308,8 @@ security: | `validation_loop`, `security` | Child replaces entirely | | `allowed_remote_resources`, `allow_runtime_fetch`, `max_runtime_fetches` | NOT inherited (child must declare its own) | +¹ **Exception:** When the child explicitly declares a top-level `pre_script` or `post_script`, any forge-level equivalents inherited from the base are cleared to prevent `ResolveForge` from overriding the child's declaration. Child-authored forge-level scripts are preserved. See [#847](https://github.com/fullsend-ai/fullsend/issues/847). + ### Referencing resources: local vs. remote **Local paths** resolve relative to the harness file's base directory: diff --git a/internal/cli/prescript_run_test.go b/internal/cli/prescript_run_test.go index 767749d98e..49dab645af 100644 --- a/internal/cli/prescript_run_test.go +++ b/internal/cli/prescript_run_test.go @@ -209,23 +209,59 @@ func usePreScriptStub(t *testing.T) { t.Setenv("PATH", stubDir+string(filepath.ListSeparator)+os.Getenv("PATH")) } +// Issue #847: the triage agent must abort on non-zero pre-script exit, +// matching code/fix behavior. Pre-script handling in runAgent is generic +// but was never tested with agent name "triage". +func TestRunAgent_TriagePreScriptExitsNonZero(t *testing.T) { + usePreScriptStub(t) + dir := newSkipHarnessDirForAgent(t, "triage", "exit 1\n") + + rFlags := resolveFlags{maxDepth: 10, maxResources: 50} + err := runAgent(context.Background(), "triage", dir, "", t.TempDir(), "", nil, false, "", "", rFlags, + statusOpts{}, ui.New(io.Discard), false) + require.Error(t, err) + assert.Contains(t, err.Error(), "running pre-script", + "triage agent must fail when pre-script exits non-zero") +} + +// A triage pre-script that exits 0 should allow the run to proceed +// (reaching sandbox creation, which fails in the test stub). +func TestRunAgent_TriagePreScriptExitsZero_ProceedsToSandbox(t *testing.T) { + usePreScriptStub(t) + dir := newSkipHarnessDirForAgent(t, "triage", "true\n") + + rFlags := resolveFlags{maxDepth: 10, maxResources: 50} + err := runAgent(context.Background(), "triage", dir, "", t.TempDir(), "", nil, false, "", "", rFlags, + statusOpts{}, ui.New(io.Discard), false) + require.Error(t, err) + assert.Contains(t, err.Error(), "creating sandbox", + "triage agent should proceed past pre-script when it exits 0") +} + // newSkipHarnessDir builds a minimal fullsend dir whose code harness runs // the given pre-script body. func newSkipHarnessDir(t *testing.T, preScriptBody string) string { + t.Helper() + return newSkipHarnessDirForAgent(t, "code", preScriptBody) +} + +// newSkipHarnessDirForAgent builds a minimal fullsend dir for any agent +// name with an optional pre-script body. +func newSkipHarnessDirForAgent(t *testing.T, agentName, preScriptBody string) string { t.Helper() dir := t.TempDir() require.NoError(t, os.MkdirAll(filepath.Join(dir, "harness"), 0o755)) require.NoError(t, os.MkdirAll(filepath.Join(dir, "agents"), 0o755)) - require.NoError(t, os.WriteFile(filepath.Join(dir, "agents", "code.md"), - []byte("You are a coding agent."), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "agents", agentName+".md"), + []byte("You are a "+agentName+" agent."), 0o644)) require.NoError(t, os.WriteFile(filepath.Join(dir, "config.yaml"), - []byte("agents:\n - harness/code.yaml\n"), 0o644)) + []byte("agents:\n - harness/"+agentName+".yaml\n"), 0o644)) - harnessYAML := "agent: agents/code.md\nrole: test\n" + harnessYAML := "agent: agents/" + agentName + ".md\nrole: test\n" if preScriptBody != "" { harnessYAML += "pre_script: " + writePreScript(t, preScriptBody) + "\n" } - require.NoError(t, os.WriteFile(filepath.Join(dir, "harness", "code.yaml"), + require.NoError(t, os.WriteFile(filepath.Join(dir, "harness", agentName+".yaml"), []byte(harnessYAML), 0o644)) return dir } diff --git a/internal/harness/compose.go b/internal/harness/compose.go index 15aee09d81..8b8c3d5490 100644 --- a/internal/harness/compose.go +++ b/internal/harness/compose.go @@ -467,6 +467,13 @@ func matchingAllowedPrefix(rawURL string, allowlist []string) string { // - forge: key-by-key merge; per-platform uses same rules // - allowed_remote_resources: NOT merged (security; child must declare its own) func mergeBaseIntoChild(base, child *Harness) { + // Record which script fields the child explicitly declared before + // scalar merge. After forge blocks are merged (below), inherited + // forge-level scripts must not shadow these explicit declarations + // during ResolveForge — see clearInheritedForgeScripts. + childHadPreScript := child.PreScript != "" + childHadPostScript := child.PostScript != "" + // Scalars: child overrides if non-zero if child.Agent == "" { child.Agent = base.Agent @@ -588,10 +595,78 @@ func mergeBaseIntoChild(base, child *Harness) { child.Security = base.Security } - // Forge: key-by-key merge + // Forge: key-by-key merge. Snapshot the child's forge-level scripts + // before merging so clearInheritedForgeScripts can distinguish + // child-authored values from base-inherited ones. + childForgeSnap := snapshotChildForgeScripts(child.Forge) if base.Forge != nil { child.Forge = mergeForgeBlocks(base.Forge, child.Forge) } + + // When the child explicitly declared a top-level pre_script or + // post_script, clear any forge-level equivalents that were inherited + // from the base. Without this, ResolveForge would overwrite the + // child's explicit script with the base's forge-level script — + // causing the child's pre_script to be silently ignored. See #847. + clearInheritedForgeScripts(child, childHadPreScript, childHadPostScript, childForgeSnap) +} + +// forgeScriptSnapshot records which script fields a forge config had +// before base composition, so clearInheritedForgeScripts can distinguish +// child-authored values from base-inherited ones. +type forgeScriptSnapshot struct { + hadPreScript bool + hadPostScript bool +} + +// snapshotChildForgeScripts captures the child's forge-level script +// state before mergeForgeBlocks modifies it. +func snapshotChildForgeScripts(forge map[string]*ForgeConfig) map[string]forgeScriptSnapshot { + if len(forge) == 0 { + return nil + } + snap := make(map[string]forgeScriptSnapshot, len(forge)) + for platform, fc := range forge { + if fc == nil { + continue + } + snap[platform] = forgeScriptSnapshot{ + hadPreScript: fc.PreScript != "", + hadPostScript: fc.PostScript != "", + } + } + return snap +} + +// clearInheritedForgeScripts removes forge-level pre_script / post_script +// values inherited from a base when the child explicitly declared the +// corresponding top-level field. Only base-inherited values are cleared; +// child-authored forge-level scripts are preserved. See #847. +func clearInheritedForgeScripts(child *Harness, childHadPreScript, childHadPostScript bool, childForgeSnap map[string]forgeScriptSnapshot) { + if child.Forge == nil || (!childHadPreScript && !childHadPostScript) { + return + } + for platform, fc := range child.Forge { + if fc == nil { + continue + } + snap := childForgeSnap[platform] + needClearPre := childHadPreScript && fc.PreScript != "" && !snap.hadPreScript + needClearPost := childHadPostScript && fc.PostScript != "" && !snap.hadPostScript + if !needClearPre && !needClearPost { + continue + } + // Clone to avoid mutating a base ForgeConfig through a shared + // pointer assigned by mergeForgeBlocks. + clone := *fc + if needClearPre { + clone.PreScript = "" + } + if needClearPost { + clone.PostScript = "" + } + child.Forge[platform] = &clone + } } // isFullsendCachePath reports whether p is an absolute path already inside diff --git a/internal/harness/compose_test.go b/internal/harness/compose_test.go index ac4f5373e7..c946abfb9b 100644 --- a/internal/harness/compose_test.go +++ b/internal/harness/compose_test.go @@ -1126,6 +1126,126 @@ func TestMergeForgeConfigInto_PreflightCheckCarryForward(t *testing.T) { "PreflightCheck should be carried forward from base in forge merge") } +// Issue #847: child's top-level pre_script must survive forge merge when +// the base declares forge..pre_script. +func TestMergeBaseIntoChild_ChildPreScriptSurvivesBaseForge(t *testing.T) { + base := &Harness{ + Agent: "agents/base.md", + Role: "triage", + Forge: map[string]*ForgeConfig{ + "github": { + PreScript: "base-forge-pre.sh", + PostScript: "base-forge-post.sh", + }, + }, + } + child := &Harness{ + PreScript: "child-pre.sh", + } + + mergeBaseIntoChild(base, child) + + // After merge, the forge block is inherited from base but the forge-level + // pre_script should be cleared so ResolveForge does not override the child's + // explicit top-level pre_script. + assert.Equal(t, "child-pre.sh", child.PreScript) + require.NotNil(t, child.Forge) + require.NotNil(t, child.Forge["github"]) + assert.Empty(t, child.Forge["github"].PreScript, + "base forge pre_script must be cleared when child has top-level pre_script") + assert.Equal(t, "base-forge-post.sh", child.Forge["github"].PostScript, + "base forge post_script should be inherited when child has no top-level post_script") +} + +// When a child also declares forge-level scripts, those should be preserved +// even when the child has a top-level pre_script. +func TestMergeBaseIntoChild_ChildForgePreScriptPreserved(t *testing.T) { + base := &Harness{ + Agent: "agents/base.md", + Role: "triage", + Forge: map[string]*ForgeConfig{ + "github": { + PreScript: "base-forge-pre.sh", + }, + }, + } + child := &Harness{ + PreScript: "child-top-pre.sh", + Forge: map[string]*ForgeConfig{ + "github": { + PreScript: "child-forge-pre.sh", + }, + }, + } + + mergeBaseIntoChild(base, child) + + // Child's forge pre_script should win over base forge, and should NOT + // be cleared even though the child has a top-level pre_script. + assert.Equal(t, "child-top-pre.sh", child.PreScript) + require.NotNil(t, child.Forge["github"]) + assert.Equal(t, "child-forge-pre.sh", child.Forge["github"].PreScript, + "child's own forge pre_script should be preserved") +} + +// End-to-end test with LoadWithBase: a child that explicitly declares +// pre_script and inherits from a base with forge.github.pre_script. +// After forge resolution the child's pre_script should survive. +func TestLoadWithBase_ChildPreScriptSurvivesBaseForgeResolution(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 +`) + + 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's top-level pre_script must survive base forge resolution") + assert.Equal(t, "base-forge-post.sh", h.PostScript, + "base forge post_script should be promoted when child has no post_script") +} + +// When the child has no top-level pre_script, it should still inherit +// from the base's forge-level pre_script (existing behavior unchanged). +func TestLoadWithBase_BaseForgePreScriptInheritedWhenChildOmits(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 +`) + + path := writeTestHarness(t, dir, "child.yaml", ` +base: base.yaml +model: opus +`) + + h, _, err := LoadWithBase(context.Background(), path, ComposeOpts{ + ForgePlatform: "github", + }) + require.NoError(t, err) + + assert.Equal(t, "base-forge-pre.sh", h.PreScript, + "when child omits pre_script, base forge pre_script should be inherited") +} + func TestLoadWithBase_InvalidForgeAfterMerge(t *testing.T) { dir := t.TempDir()