Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 9 additions & 0 deletions docs/ADRs/0045-forge-portable-harness-schema.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
4 changes: 3 additions & 1 deletion docs/guides/user/bring-your-own-agent.md
Original file line number Diff line number Diff line change
Expand Up @@ -300,14 +300,16 @@ 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` |
| `env`, `runner_env` (deprecated) | Merged; child keys win |
| `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:
Expand Down
46 changes: 41 additions & 5 deletions internal/cli/prescript_run_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
77 changes: 76 additions & 1 deletion internal/harness/compose.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
120 changes: 120 additions & 0 deletions internal/harness/compose_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.<platform>.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()

Expand Down
Loading