diff --git a/internal/cli/run.go b/internal/cli/run.go index e6dd78f7c6..4a43054688 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -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/" @@ -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") @@ -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 != "" { + printer.StepStart("Preflight: checking validation_loop dependencies") + preflightCtx, preflightCancel := context.WithTimeout(ctx, preflightCheckTimeout) + defer preflightCancel() + preflightCmd := exec.CommandContext(preflightCtx, "sh", "-c", h.ValidationLoop.PreflightCheck) + preflightCmd.Env = append(os.Environ(), envToList(h.RunnerEnv)...) + preflightOut, preflightErr := preflightCmd.CombinedOutput() + 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)) + } + printer.StepDone("Preflight dependency check passed") + } + // 2. Check openshell availability. openshellStart := time.Now() printer.StepStart("Checking openshell availability") diff --git a/internal/cli/run_test.go b/internal/cli/run_test.go index 97218c6648..f833ddfa51 100644 --- a/internal/cli/run_test.go +++ b/internal/cli/run_test.go @@ -2739,6 +2739,126 @@ func TestExpandValidationLoopSchema(t *testing.T) { require.NoError(t, err, "expanded schema path should exist") } +// 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", diff --git a/internal/harness/compose.go b/internal/harness/compose.go index dbcdbad89d..1e3456f929 100644 --- a/internal/harness/compose.go +++ b/internal/harness/compose.go @@ -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 @@ -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 } } diff --git a/internal/harness/compose_test.go b/internal/harness/compose_test.go index 1f182225d6..13c0f8be4a 100644 --- a/internal/harness/compose_test.go +++ b/internal/harness/compose_test.go @@ -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() @@ -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() diff --git a/internal/harness/harness.go b/internal/harness/harness.go index e0ca02e0ca..a85aebf5a3 100644 --- a/internal/harness/harness.go +++ b/internal/harness/harness.go @@ -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"` + PreflightCheck string `yaml:"preflight_check,omitempty"` // shell command to validate host deps before sandbox creation } // EnvConfig holds environment variable maps for runner and sandbox targets. @@ -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 { diff --git a/internal/harness/harness_test.go b/internal/harness/harness_test.go index a53ec76011..e59853a924 100644 --- a/internal/harness/harness_test.go +++ b/internal/harness/harness_test.go @@ -567,6 +567,40 @@ func TestValidateRunnerEnvWith_ValidationLoopSchemaVarSet(t *testing.T) { require.NoError(t, h.ValidateRunnerEnvWith(lookup)) } +func TestValidateRunnerEnvWith_ChecksValidationLoopPreflightCheck(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "test", + ValidationLoop: &ValidationLoop{ + Script: "scripts/validate.sh", + PreflightCheck: "test -d ${MISSING_DIR}", + }, + } + lookup := func(key string) (string, bool) { return "", false } + err := h.ValidateRunnerEnvWith(lookup) + require.Error(t, err) + assert.Contains(t, err.Error(), "MISSING_DIR") + assert.Contains(t, err.Error(), "validation_loop.preflight_check") +} + +func TestValidateRunnerEnvWith_ValidationLoopPreflightCheckVarSet(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "test", + ValidationLoop: &ValidationLoop{ + Script: "scripts/validate.sh", + PreflightCheck: "test -d ${FULLSEND_DIR}", + }, + } + lookup := func(key string) (string, bool) { + if key == "FULLSEND_DIR" { + return "/opt/fullsend", true + } + return "", false + } + require.NoError(t, h.ValidateRunnerEnvWith(lookup)) +} + func TestValidateRunnerEnvWith_NilEnvNoError(t *testing.T) { h := &Harness{Agent: "agents/test.md", Role: "test"} err := h.ValidateRunnerEnvWith(func(string) (string, bool) { return "", false }) @@ -860,6 +894,47 @@ func TestValidateFilesExist_SkipsOptionalPaths(t *testing.T) { require.NoError(t, h.ValidateFilesExist()) } +// --- PreflightCheck tests --- + +func TestLoad_ValidationLoopPreflightCheck(t *testing.T) { + content := ` +agent: agents/test.md +role: test +validation_loop: + script: scripts/validate.sh + preflight_check: "python3 -c 'import jsonschema'" + max_iterations: 2 +` + dir := t.TempDir() + path := filepath.Join(dir, "test.yaml") + require.NoError(t, os.WriteFile(path, []byte(content), 0o644)) + + h, err := Load(path) + require.NoError(t, err) + require.NotNil(t, h.ValidationLoop) + assert.Equal(t, "scripts/validate.sh", h.ValidationLoop.Script) + assert.Equal(t, "python3 -c 'import jsonschema'", h.ValidationLoop.PreflightCheck) + assert.Equal(t, 2, h.ValidationLoop.MaxIterations) +} + +func TestLoad_ValidationLoopWithoutPreflightCheck(t *testing.T) { + content := ` +agent: agents/test.md +role: test +validation_loop: + script: scripts/validate.sh + max_iterations: 1 +` + dir := t.TempDir() + path := filepath.Join(dir, "test.yaml") + require.NoError(t, os.WriteFile(path, []byte(content), 0o644)) + + h, err := Load(path) + require.NoError(t, err) + require.NotNil(t, h.ValidationLoop) + assert.Empty(t, h.ValidationLoop.PreflightCheck) +} + // --- AllowedRemoteResources tests --- func TestHarness_AllowedRemoteResources_Parse(t *testing.T) { diff --git a/internal/harness/scaffold_integration_test.go b/internal/harness/scaffold_integration_test.go index 1a43ada594..b5aa18d926 100644 --- a/internal/harness/scaffold_integration_test.go +++ b/internal/harness/scaffold_integration_test.go @@ -83,10 +83,14 @@ slug: test-triage // Local base -> no URL deps. assert.Nil(t, deps) - // ValidationLoop inherited from base. + // ValidationLoop inherited from base. PreflightCheck is intentionally + // empty here: fullsend's own agents resolve from fullsend-ai/agents + // (not this local scaffold), so preflight_check belongs there instead + // — see fullsend-ai/agents#422. assert.NotNil(t, h.ValidationLoop) assert.Equal(t, "scripts/validate-output-schema.sh", h.ValidationLoop.Script) assert.Equal(t, 2, h.ValidationLoop.MaxIterations) + assert.Empty(t, h.ValidationLoop.PreflightCheck) } // TestLoadWithBase_WrapperOverridesBaseFields verifies that wrapper-level