Skip to content
Merged
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
31 changes: 31 additions & 0 deletions internal/cli/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,13 @@ const (
defaultAgentsRepoName = "agents"
)

// preflightCheckTimeout bounds the execution time for a validation_loop
// preflight_check command. Mirrors the preflightGitHubTimeout pattern —
// these are fast host-side dependency checks that should never hang. A var
// (not const) so tests can shrink it to genuinely exercise deadline expiry
// without waiting out the real duration.
var preflightCheckTimeout = 30 * time.Second

// defaultAgentsRepoURLPrefix is the base URL for fetching agent harnesses
// from the agents repository. It is a var (not const) to allow test overrides.
var defaultAgentsRepoURLPrefix = "https://raw.githubusercontent.com/fullsend-ai/agents/"
Expand Down Expand Up @@ -539,6 +546,9 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep
if h.ValidationLoop != nil && strings.Contains(h.ValidationLoop.Schema, "${") {
h.ValidationLoop.Schema = os.Expand(h.ValidationLoop.Schema, expander)
}
if h.ValidationLoop != nil && strings.Contains(h.ValidationLoop.PreflightCheck, "${") {
h.ValidationLoop.PreflightCheck = os.Expand(h.ValidationLoop.PreflightCheck, expander)
}

if err := h.ValidateFilesExist(); err != nil {
printer.StepFail("File validation failed")
Expand Down Expand Up @@ -668,6 +678,27 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep
}
}

// 1d. Preflight dependency check for host-side scripts.
// When a validation_loop declares a preflight_check command, run it now
// to catch missing host dependencies (e.g. python3-jsonschema) before
// sandbox creation — not after the agent has already finished. See #5074.
if h.ValidationLoop != nil && h.ValidationLoop.PreflightCheck != "" {
Comment thread
waynesun09 marked this conversation as resolved.
printer.StepStart("Preflight: checking validation_loop dependencies")
Comment thread
waynesun09 marked this conversation as resolved.
preflightCtx, preflightCancel := context.WithTimeout(ctx, preflightCheckTimeout)
Comment thread
waynesun09 marked this conversation as resolved.
defer preflightCancel()
preflightCmd := exec.CommandContext(preflightCtx, "sh", "-c", h.ValidationLoop.PreflightCheck)
preflightCmd.Env = append(os.Environ(), envToList(h.RunnerEnv)...)
preflightOut, preflightErr := preflightCmd.CombinedOutput()
Comment thread
waynesun09 marked this conversation as resolved.
Comment thread
waynesun09 marked this conversation as resolved.
if preflightErr != nil {
printer.StepFail("Preflight dependency check failed")
if preflightCtx.Err() == context.DeadlineExceeded {
return fmt.Errorf("validation_loop.preflight_check timed out after %s: %s", preflightCheckTimeout, h.ValidationLoop.PreflightCheck)
}
return fmt.Errorf("validation_loop.preflight_check failed: %s\n%s\nInstall the missing dependency before running this agent", h.ValidationLoop.PreflightCheck, validationFailMessage(preflightOut, preflightErr))
}
Comment thread
waynesun09 marked this conversation as resolved.
printer.StepDone("Preflight dependency check passed")
}

// 2. Check openshell availability.
openshellStart := time.Now()
printer.StepStart("Checking openshell availability")
Expand Down
120 changes: 120 additions & 0 deletions internal/cli/run_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2739,6 +2739,126 @@ func TestExpandValidationLoopSchema(t *testing.T) {
require.NoError(t, err, "expanded schema path should exist")
}

Comment thread
waynesun09 marked this conversation as resolved.
// preflightTestSetup creates a temporary fullsend directory with the required
// agent and harness files for preflight check tests. When harnessYAML contains
// a validation_loop.script reference, the script file is created as a no-op
// stub so ValidateFilesExist passes.
func preflightTestSetup(t *testing.T, harnessYAML 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.MkdirAll(filepath.Join(dir, "scripts"), 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, "scripts", "validate.sh"),
[]byte("#!/bin/sh\nexit 0\n"),
0o755,
))
require.NoError(t, os.WriteFile(
filepath.Join(dir, "harness", "code.yaml"),
[]byte(harnessYAML),
0o644,
))
return dir
}

func TestRunAgent_PreflightCheck_Passing(t *testing.T) {
// A passing preflight_check should not block the run — runAgent
// proceeds past the guard and fails later at openshell CheckGateway.
useFakeOpenshell(t)
dir := preflightTestSetup(t, "agent: agents/code.md\nrole: test\nvalidation_loop:\n script: scripts/validate.sh\n preflight_check: \"true\"\n max_iterations: 2\n")

rFlags := resolveFlags{maxDepth: 10, maxResources: 50}
printer := ui.New(io.Discard)
repoDir := t.TempDir()
err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false)
require.Error(t, err)
// Must pass the preflight guard and reach the openshell check.
assert.Contains(t, err.Error(), "openshell")
}

func TestRunAgent_PreflightCheck_Failing(t *testing.T) {
// A failing preflight_check should abort runAgent with a descriptive
// error, exercising the StepFail + error-wrapping logic.
useFakeOpenshell(t)
dir := preflightTestSetup(t, "agent: agents/code.md\nrole: test\nvalidation_loop:\n script: scripts/validate.sh\n preflight_check: \"exit 1\"\n max_iterations: 2\n")

rFlags := resolveFlags{maxDepth: 10, maxResources: 50}
printer := ui.New(io.Discard)
repoDir := t.TempDir()
err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false)
require.Error(t, err)
assert.Contains(t, err.Error(), "preflight_check failed")
}

func TestRunAgent_PreflightCheck_NoCheckConfigured(t *testing.T) {
// When no preflight_check is set, runAgent should skip the guard
// and proceed to the openshell check.
useFakeOpenshell(t)
dir := preflightTestSetup(t, "agent: agents/code.md\nrole: test\nvalidation_loop:\n script: scripts/validate.sh\n max_iterations: 2\n")

rFlags := resolveFlags{maxDepth: 10, maxResources: 50}
printer := ui.New(io.Discard)
repoDir := t.TempDir()
err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false)
require.Error(t, err)
assert.Contains(t, err.Error(), "openshell")
}

func TestRunAgent_PreflightCheck_NilValidationLoop(t *testing.T) {
// When no validation_loop is set at all, runAgent should skip the
// preflight guard and proceed to the openshell check.
useFakeOpenshell(t)
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, "harness", "code.yaml"),
[]byte("agent: agents/code.md\nrole: test\n"),
0o644,
))

rFlags := resolveFlags{maxDepth: 10, maxResources: 50}
printer := ui.New(io.Discard)
repoDir := t.TempDir()
err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false)
require.Error(t, err)
assert.Contains(t, err.Error(), "openshell")
}

func TestRunAgent_PreflightCheck_Timeout(t *testing.T) {
// A preflight_check that exceeds the context deadline should abort
// with a "timed out" error, not hang indefinitely.
useFakeOpenshell(t)
dir := preflightTestSetup(t, "agent: agents/code.md\nrole: test\nvalidation_loop:\n script: scripts/validate.sh\n preflight_check: \"sleep 5\"\n max_iterations: 2\n")

// Shrink the real timeout so the "sleep 5" command genuinely outlives
// it, exercising the actual DeadlineExceeded branch rather than
// short-circuiting via an already-cancelled parent context.
original := preflightCheckTimeout
preflightCheckTimeout = 50 * time.Millisecond
t.Cleanup(func() { preflightCheckTimeout = original })

rFlags := resolveFlags{maxDepth: 10, maxResources: 50}
printer := ui.New(io.Discard)
repoDir := t.TempDir()
err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false)
require.Error(t, err)
assert.Contains(t, err.Error(), "timed out")
}

func TestBuildSandboxEnvLines_FromEnvSandbox(t *testing.T) {
h := &harness.Harness{
Agent: "agents/test.md",
Expand Down
13 changes: 11 additions & 2 deletions internal/harness/compose.go
Original file line number Diff line number Diff line change
Expand Up @@ -572,9 +572,14 @@ func mergeBaseIntoChild(base, child *Harness) {
child.Env.mergeEnvFrom(base.Env, false)
}

// Pointer structs: child replaces if non-nil
// Pointer structs: child replaces if non-nil, but carry forward
// PreflightCheck when the child overrides validation_loop without
// setting its own preflight_check (avoids silently dropping inherited
// preflight checks — see #5074).
if child.ValidationLoop == nil {
child.ValidationLoop = base.ValidationLoop
} else if child.ValidationLoop.PreflightCheck == "" && base.ValidationLoop != nil {
child.ValidationLoop.PreflightCheck = base.ValidationLoop.PreflightCheck
}
// Security: child inherits base's config if nil. Note that a base harness
// (even integrity-pinned) could set fail_mode: open. Child authors must
Expand Down Expand Up @@ -1342,9 +1347,13 @@ func mergeForgeConfigInto(base, child *ForgeConfig) {
child.Env.mergeEnvFrom(base.Env, false)
}

// ValidationLoop: child replaces if non-nil
// ValidationLoop: child replaces if non-nil, but carry forward
// PreflightCheck when the child overrides validation_loop without
// setting its own preflight_check (see #5074).
if child.ValidationLoop == nil {
child.ValidationLoop = base.ValidationLoop
} else if child.ValidationLoop.PreflightCheck == "" && base.ValidationLoop != nil {
child.ValidationLoop.PreflightCheck = base.ValidationLoop.PreflightCheck
}
}

Expand Down
88 changes: 88 additions & 0 deletions internal/harness/compose_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,68 @@ model: opus
assert.Equal(t, 5, h.ValidationLoop.MaxIterations)
}

func TestLoadWithBase_LocalBase_PreflightCheckCarryForward(t *testing.T) {
// When a child overrides validation_loop (e.g. to change max_iterations)
// but does not set preflight_check, the base's preflight_check should be
// carried forward. See #5074.
dir := t.TempDir()

writeTestHarness(t, dir, "base.yaml", `
agent: agents/test.md
role: test
validation_loop:
script: base-script.sh
preflight_check: "python3 -c 'import jsonschema'"
max_iterations: 5
`)

path := writeTestHarness(t, dir, "child.yaml", `
base: base.yaml
validation_loop:
script: child-script.sh
max_iterations: 3
`)

h, _, err := LoadWithBase(context.Background(), path, ComposeOpts{})
require.NoError(t, err)

require.NotNil(t, h.ValidationLoop)
assert.Equal(t, "child-script.sh", h.ValidationLoop.Script)
assert.Equal(t, 3, h.ValidationLoop.MaxIterations)
assert.Equal(t, "python3 -c 'import jsonschema'", h.ValidationLoop.PreflightCheck,
"PreflightCheck should be carried forward from base when child overrides validation_loop without setting preflight_check")
}

func TestLoadWithBase_LocalBase_PreflightCheckChildOverrides(t *testing.T) {
// When a child explicitly sets its own preflight_check, it should take
// precedence over the base's value.
dir := t.TempDir()

writeTestHarness(t, dir, "base.yaml", `
agent: agents/test.md
role: test
validation_loop:
script: base-script.sh
preflight_check: "python3 -c 'import jsonschema'"
max_iterations: 5
`)

path := writeTestHarness(t, dir, "child.yaml", `
base: base.yaml
validation_loop:
script: child-script.sh
preflight_check: "which jq"
max_iterations: 3
`)

h, _, err := LoadWithBase(context.Background(), path, ComposeOpts{})
require.NoError(t, err)

require.NotNil(t, h.ValidationLoop)
assert.Equal(t, "which jq", h.ValidationLoop.PreflightCheck,
"Child's own preflight_check should override base's")
}

func TestLoadWithBase_ChainedBases(t *testing.T) {
dir := t.TempDir()

Expand Down Expand Up @@ -1038,6 +1100,32 @@ func TestMergeForgeConfigInto_ValidationLoop(t *testing.T) {
assert.Equal(t, 5, child.ValidationLoop.MaxIterations)
}

func TestMergeForgeConfigInto_PreflightCheckCarryForward(t *testing.T) {
// When a child ForgeConfig overrides validation_loop without setting
// preflight_check, the base's preflight_check should be carried forward.
base := &ForgeConfig{
ValidationLoop: &ValidationLoop{
Script: "base-validate.sh",
PreflightCheck: "python3 -c 'import jsonschema'",
MaxIterations: 5,
},
}
child := &ForgeConfig{
ValidationLoop: &ValidationLoop{
Script: "child-validate.sh",
MaxIterations: 3,
},
}

mergeForgeConfigInto(base, child)

require.NotNil(t, child.ValidationLoop)
assert.Equal(t, "child-validate.sh", child.ValidationLoop.Script)
assert.Equal(t, 3, child.ValidationLoop.MaxIterations)
assert.Equal(t, "python3 -c 'import jsonschema'", child.ValidationLoop.PreflightCheck,
"PreflightCheck should be carried forward from base in forge merge")
}

func TestLoadWithBase_InvalidForgeAfterMerge(t *testing.T) {
dir := t.TempDir()

Expand Down
14 changes: 10 additions & 4 deletions internal/harness/harness.go
Original file line number Diff line number Diff line change
Expand Up @@ -204,10 +204,11 @@ type APIServer struct {

// ValidationLoop configures a deterministic validation step after the agent exits.
type ValidationLoop struct {
Script string `yaml:"script"`
Schema string `yaml:"schema,omitempty"`
MaxIterations int `yaml:"max_iterations"`
FeedbackMode string `yaml:"feedback_mode,omitempty"`
Script string `yaml:"script"`
Schema string `yaml:"schema,omitempty"`
MaxIterations int `yaml:"max_iterations"`
FeedbackMode string `yaml:"feedback_mode,omitempty"`
Comment thread
waynesun09 marked this conversation as resolved.
PreflightCheck string `yaml:"preflight_check,omitempty"` // shell command to validate host deps before sandbox creation
Comment thread
waynesun09 marked this conversation as resolved.
}

// EnvConfig holds environment variable maps for runner and sandbox targets.
Expand Down Expand Up @@ -611,6 +612,11 @@ func (h *Harness) ValidateRunnerEnvWith(lookup func(string) (string, bool)) erro
return err
}
}
if h.ValidationLoop != nil && h.ValidationLoop.PreflightCheck != "" {
if err := checkVarRefs("validation_loop.preflight_check", h.ValidationLoop.PreflightCheck); err != nil {
return err
}
}
if h.Env != nil {
for k, v := range h.Env.Runner {
if err := checkVarRefs(fmt.Sprintf("env.runner[%s]", k), v); err != nil {
Expand Down
Loading
Loading