From c6f25d47954cfd8dd9169fc0dd2b854ee71891ca Mon Sep 17 00:00:00 2001 From: fullsend-code <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Thu, 16 Jul 2026 14:16:38 +0000 Subject: [PATCH 1/3] fix(#5074): add preflight dependency check for validation_loop scripts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a validation_loop declares a preflight_check command, the runner now executes it during the preflight phase — before sandbox creation. This catches missing host-side dependencies (e.g. python3-jsonschema) immediately instead of after the agent has already completed (~74s of wasted execution). Changes: - Add PreflightCheck field to ValidationLoop struct in the harness schema (preflight_check YAML key) - Execute the check in run.go after ValidateFilesExist but before openshell/sandbox setup - Update all 5 scaffold harness YAML files (triage, review, fix, prioritize, retro) that use validate-output-schema.sh to declare preflight_check for jsonschema - Add unit tests for YAML parsing, preflight execution, and base composition inheritance of the new field Note: pre-commit could not run in this environment (network 403 on hook fetch). The post-script runs an authoritative pre-commit on the runner. Relates to #5074. Scope narrowed to validation_loop only; the pre_script/post_script portion of #5074 is tracked separately at fullsend-ai/fullsend#5568. Scaffold removal is tracked at fullsend-ai/agents#422. --- internal/cli/run.go | 20 +++++++ internal/cli/run_test.go | 54 +++++++++++++++++++ internal/harness/harness.go | 9 ++-- internal/harness/harness_test.go | 41 ++++++++++++++ internal/harness/scaffold_integration_test.go | 1 + .../scaffold/fullsend-repo/harness/fix.yaml | 1 + .../fullsend-repo/harness/prioritize.yaml | 1 + .../scaffold/fullsend-repo/harness/retro.yaml | 1 + .../fullsend-repo/harness/review.yaml | 1 + .../fullsend-repo/harness/triage.yaml | 1 + 10 files changed, 126 insertions(+), 4 deletions(-) diff --git a/internal/cli/run.go b/internal/cli/run.go index e6dd78f7c6..40d5fcbdf0 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -668,6 +668,26 @@ 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") + preflightCmd := exec.Command("sh", "-c", h.ValidationLoop.PreflightCheck) + preflightCmd.Env = os.Environ() + preflightOut, preflightErr := preflightCmd.CombinedOutput() + if preflightErr != nil { + printer.StepFail("Preflight dependency check failed") + detail := strings.TrimSpace(string(preflightOut)) + if detail != "" { + return fmt.Errorf("validation_loop.preflight_check failed: %s\n%s\nInstall the missing dependency before running this agent", h.ValidationLoop.PreflightCheck, detail) + } + return fmt.Errorf("validation_loop.preflight_check failed: %s\nInstall the missing dependency before running this agent", h.ValidationLoop.PreflightCheck) + } + 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..797ab32aa8 100644 --- a/internal/cli/run_test.go +++ b/internal/cli/run_test.go @@ -2739,6 +2739,60 @@ func TestExpandValidationLoopSchema(t *testing.T) { require.NoError(t, err, "expanded schema path should exist") } +func TestPreflightCheck_PassingCommand(t *testing.T) { + // Simulate what run.go does for a passing preflight check. + h := &harness.Harness{ + ValidationLoop: &harness.ValidationLoop{ + Script: "scripts/validate.sh", + PreflightCheck: "true", + MaxIterations: 2, + }, + } + + cmd := exec.Command("sh", "-c", h.ValidationLoop.PreflightCheck) + cmd.Env = os.Environ() + _, err := cmd.CombinedOutput() + require.NoError(t, err) +} + +func TestPreflightCheck_FailingCommand(t *testing.T) { + // Simulate what run.go does for a failing preflight check. + h := &harness.Harness{ + ValidationLoop: &harness.ValidationLoop{ + Script: "scripts/validate.sh", + PreflightCheck: "false", + MaxIterations: 2, + }, + } + + cmd := exec.Command("sh", "-c", h.ValidationLoop.PreflightCheck) + cmd.Env = os.Environ() + _, err := cmd.CombinedOutput() + require.Error(t, err) +} + +func TestPreflightCheck_NoCheckConfigured(t *testing.T) { + // When no preflight_check is set, the check should be skipped. + h := &harness.Harness{ + ValidationLoop: &harness.ValidationLoop{ + Script: "scripts/validate.sh", + MaxIterations: 2, + }, + } + + assert.Empty(t, h.ValidationLoop.PreflightCheck) +} + +func TestPreflightCheck_NilValidationLoop(t *testing.T) { + // When no validation_loop is set, no check should run. + h := &harness.Harness{ + Agent: "agents/test.md", + Role: "test", + } + + assert.Nil(t, h.ValidationLoop) +} + func TestBuildSandboxEnvLines_FromEnvSandbox(t *testing.T) { h := &harness.Harness{ Agent: "agents/test.md", diff --git a/internal/harness/harness.go b/internal/harness/harness.go index e0ca02e0ca..5b7b6978ad 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. diff --git a/internal/harness/harness_test.go b/internal/harness/harness_test.go index a53ec76011..0cbd41e69d 100644 --- a/internal/harness/harness_test.go +++ b/internal/harness/harness_test.go @@ -860,6 +860,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..b7d82694f1 100644 --- a/internal/harness/scaffold_integration_test.go +++ b/internal/harness/scaffold_integration_test.go @@ -87,6 +87,7 @@ slug: test-triage assert.NotNil(t, h.ValidationLoop) assert.Equal(t, "scripts/validate-output-schema.sh", h.ValidationLoop.Script) assert.Equal(t, 2, h.ValidationLoop.MaxIterations) + assert.Equal(t, "python3 -c 'import jsonschema'", h.ValidationLoop.PreflightCheck) } // TestLoadWithBase_WrapperOverridesBaseFields verifies that wrapper-level diff --git a/internal/scaffold/fullsend-repo/harness/fix.yaml b/internal/scaffold/fullsend-repo/harness/fix.yaml index 16b1c88b61..d9fd8eefb5 100644 --- a/internal/scaffold/fullsend-repo/harness/fix.yaml +++ b/internal/scaffold/fullsend-repo/harness/fix.yaml @@ -27,6 +27,7 @@ post_script: scripts/post-fix.sh validation_loop: script: scripts/validate-output-schema.sh + preflight_check: "python3 -c 'import jsonschema'" max_iterations: 2 host_files: diff --git a/internal/scaffold/fullsend-repo/harness/prioritize.yaml b/internal/scaffold/fullsend-repo/harness/prioritize.yaml index 122b22bab2..785102a33b 100644 --- a/internal/scaffold/fullsend-repo/harness/prioritize.yaml +++ b/internal/scaffold/fullsend-repo/harness/prioritize.yaml @@ -26,6 +26,7 @@ post_script: scripts/post-prioritize.sh validation_loop: script: scripts/validate-output-schema.sh + preflight_check: "python3 -c 'import jsonschema'" max_iterations: 2 env: diff --git a/internal/scaffold/fullsend-repo/harness/retro.yaml b/internal/scaffold/fullsend-repo/harness/retro.yaml index beaa31f659..292610070b 100644 --- a/internal/scaffold/fullsend-repo/harness/retro.yaml +++ b/internal/scaffold/fullsend-repo/harness/retro.yaml @@ -33,6 +33,7 @@ post_script: scripts/post-retro.sh validation_loop: script: scripts/validate-output-schema.sh + preflight_check: "python3 -c 'import jsonschema'" max_iterations: 2 env: diff --git a/internal/scaffold/fullsend-repo/harness/review.yaml b/internal/scaffold/fullsend-repo/harness/review.yaml index bba2db3e9b..5968f4d982 100644 --- a/internal/scaffold/fullsend-repo/harness/review.yaml +++ b/internal/scaffold/fullsend-repo/harness/review.yaml @@ -36,6 +36,7 @@ post_script: scripts/post-review.sh validation_loop: script: scripts/validate-output-schema.sh + preflight_check: "python3 -c 'import jsonschema'" max_iterations: 2 env: diff --git a/internal/scaffold/fullsend-repo/harness/triage.yaml b/internal/scaffold/fullsend-repo/harness/triage.yaml index 7a59b2cc73..230e6ecdab 100644 --- a/internal/scaffold/fullsend-repo/harness/triage.yaml +++ b/internal/scaffold/fullsend-repo/harness/triage.yaml @@ -29,6 +29,7 @@ post_script: scripts/post-triage.sh validation_loop: script: scripts/validate-output-schema.sh + preflight_check: "python3 -c 'import jsonschema'" max_iterations: 2 env: From e6428c2225fe79790102960ae9ab3b7de5cf419d Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Thu, 23 Jul 2026 23:57:32 +0000 Subject: [PATCH 2/3] fix(#5074): address review feedback on preflight dependency check - Carry forward PreflightCheck during composition when a child harness overrides validation_loop without setting its own preflight_check, preventing silent loss of inherited preflight checks (compose.go, both mergeBaseIntoChild and mergeForgeConfigInto merge sites) - Use exec.CommandContext with 30s timeout (mirroring preflightGitHubTimeout pattern) so preflight checks respect ctx cancellation and cannot hang indefinitely; surface distinct "timed out" error message (run.go) - Rewrite preflight tests to call runAgent directly via the established useFakeOpenshell fixture, covering StepStart/StepFail/StepDone messaging and error-wrapping logic; add timeout test case (run_test.go) - Add composition tests for PreflightCheck carry-forward and child override semantics (compose_test.go) Addresses review feedback on #5192 --- internal/cli/run.go | 12 ++- internal/cli/run_test.go | 145 ++++++++++++++++++++++--------- internal/harness/compose.go | 13 ++- internal/harness/compose_test.go | 88 +++++++++++++++++++ 4 files changed, 214 insertions(+), 44 deletions(-) diff --git a/internal/cli/run.go b/internal/cli/run.go index 40d5fcbdf0..3b6f1c00ac 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -64,6 +64,11 @@ const ( // harness dynamically. defaultAgentsRepoOwner = "fullsend-ai" 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. + preflightCheckTimeout = 30 * time.Second ) // defaultAgentsRepoURLPrefix is the base URL for fetching agent harnesses @@ -674,11 +679,16 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep // 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") - preflightCmd := exec.Command("sh", "-c", h.ValidationLoop.PreflightCheck) + preflightCtx, preflightCancel := context.WithTimeout(ctx, preflightCheckTimeout) + defer preflightCancel() + preflightCmd := exec.CommandContext(preflightCtx, "sh", "-c", h.ValidationLoop.PreflightCheck) preflightCmd.Env = os.Environ() 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) + } detail := strings.TrimSpace(string(preflightOut)) if detail != "" { return fmt.Errorf("validation_loop.preflight_check failed: %s\n%s\nInstall the missing dependency before running this agent", h.ValidationLoop.PreflightCheck, detail) diff --git a/internal/cli/run_test.go b/internal/cli/run_test.go index 797ab32aa8..d876dc5d15 100644 --- a/internal/cli/run_test.go +++ b/internal/cli/run_test.go @@ -2739,58 +2739,121 @@ func TestExpandValidationLoopSchema(t *testing.T) { require.NoError(t, err, "expanded schema path should exist") } -func TestPreflightCheck_PassingCommand(t *testing.T) { - // Simulate what run.go does for a passing preflight check. - h := &harness.Harness{ - ValidationLoop: &harness.ValidationLoop{ - Script: "scripts/validate.sh", - PreflightCheck: "true", - MaxIterations: 2, - }, - } +// 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)) - cmd := exec.Command("sh", "-c", h.ValidationLoop.PreflightCheck) - cmd.Env = os.Environ() - _, err := cmd.CombinedOutput() - require.NoError(t, err) + 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 TestPreflightCheck_FailingCommand(t *testing.T) { - // Simulate what run.go does for a failing preflight check. - h := &harness.Harness{ - ValidationLoop: &harness.ValidationLoop{ - Script: "scripts/validate.sh", - PreflightCheck: "false", - MaxIterations: 2, - }, - } +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") - cmd := exec.Command("sh", "-c", h.ValidationLoop.PreflightCheck) - cmd.Env = os.Environ() - _, err := cmd.CombinedOutput() + 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 TestPreflightCheck_NoCheckConfigured(t *testing.T) { - // When no preflight_check is set, the check should be skipped. - h := &harness.Harness{ - ValidationLoop: &harness.ValidationLoop{ - Script: "scripts/validate.sh", - MaxIterations: 2, - }, - } +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") - assert.Empty(t, h.ValidationLoop.PreflightCheck) + 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 TestPreflightCheck_NilValidationLoop(t *testing.T) { - // When no validation_loop is set, no check should run. - h := &harness.Harness{ - Agent: "agents/test.md", - Role: "test", - } +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)) - assert.Nil(t, h.ValidationLoop) + 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 60\"\n max_iterations: 2\n") + + // Use an already-cancelled context to trigger immediate timeout. + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + rFlags := resolveFlags{maxDepth: 10, maxResources: 50} + printer := ui.New(io.Discard) + repoDir := t.TempDir() + err := runAgent(ctx, "code", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false) + require.Error(t, err) + assert.Contains(t, err.Error(), "preflight_check") } func TestBuildSandboxEnvLines_FromEnvSandbox(t *testing.T) { 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() From 52c398faf7fdab72b9d66647abd74728389148fb Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Fri, 24 Jul 2026 09:22:23 -0400 Subject: [PATCH 3/3] fix(#5074): address remaining review feedback, drop ineffective scaffold edits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Route preflight_check through the same ${VAR} expansion (os.Expand) and ValidateRunnerEnvWith checks that validation_loop.schema already gets, so ${FULLSEND_DIR}-style references resolve instead of silently expanding to empty (run.go, harness.go) - Merge h.RunnerEnv into the preflight command's environment, matching every other host-side command (pre_script, post_script, validation script), so a harness using runner_env to point at a custom tool location gets consistent behavior between the preflight probe and the real validation run (run.go) - Reuse the existing validationFailMessage helper instead of discarding the underlying exec error when the failing command produces no output (run.go) - Remove preflight_check from the 5 scaffold harness YAML files: they have no effect on fullsend-ai's own agents, which resolve from the pinned fullsend-ai/agents commit before ever falling back to this local scaffold. Tracked at fullsend-ai/agents#422 instead. - Convert preflightCheckTimeout from a const to a var and fix TestRunAgent_PreflightCheck_Timeout to genuinely trigger a deadline expiry (shrunk timeout + a command that outlives it) instead of an already-cancelled parent context, which took the context.Canceled path rather than DeadlineExceeded; tighten the assertion accordingly - Update TestLoadWithBase_WrapperMergesScaffold's expectation now that the scaffold no longer sets preflight_check Narrowed PR scope to validation_loop only (not pre_script/post_script, which issue #5074 also asked for) — tracked at #5568. Assisted-by: Claude Signed-off-by: Wayne Sun --- internal/cli/run.go | 23 +++++++------ internal/cli/run_test.go | 15 ++++---- internal/harness/harness.go | 5 +++ internal/harness/harness_test.go | 34 +++++++++++++++++++ internal/harness/scaffold_integration_test.go | 7 ++-- .../scaffold/fullsend-repo/harness/fix.yaml | 1 - .../fullsend-repo/harness/prioritize.yaml | 1 - .../scaffold/fullsend-repo/harness/retro.yaml | 1 - .../fullsend-repo/harness/review.yaml | 1 - .../fullsend-repo/harness/triage.yaml | 1 - 10 files changed, 65 insertions(+), 24 deletions(-) diff --git a/internal/cli/run.go b/internal/cli/run.go index 3b6f1c00ac..4a43054688 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -64,13 +64,15 @@ const ( // harness dynamically. defaultAgentsRepoOwner = "fullsend-ai" 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. - preflightCheckTimeout = 30 * time.Second ) +// 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/" @@ -544,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") @@ -682,18 +687,14 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep preflightCtx, preflightCancel := context.WithTimeout(ctx, preflightCheckTimeout) defer preflightCancel() preflightCmd := exec.CommandContext(preflightCtx, "sh", "-c", h.ValidationLoop.PreflightCheck) - preflightCmd.Env = os.Environ() + 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) } - detail := strings.TrimSpace(string(preflightOut)) - if detail != "" { - return fmt.Errorf("validation_loop.preflight_check failed: %s\n%s\nInstall the missing dependency before running this agent", h.ValidationLoop.PreflightCheck, detail) - } - return fmt.Errorf("validation_loop.preflight_check failed: %s\nInstall the missing dependency before running this agent", 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") } diff --git a/internal/cli/run_test.go b/internal/cli/run_test.go index d876dc5d15..f833ddfa51 100644 --- a/internal/cli/run_test.go +++ b/internal/cli/run_test.go @@ -2842,18 +2842,21 @@ 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 60\"\n max_iterations: 2\n") + 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") - // Use an already-cancelled context to trigger immediate timeout. - ctx, cancel := context.WithCancel(context.Background()) - cancel() + // 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(ctx, "code", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false) require.Error(t, err) - assert.Contains(t, err.Error(), "preflight_check") + assert.Contains(t, err.Error(), "timed out") } func TestBuildSandboxEnvLines_FromEnvSandbox(t *testing.T) { diff --git a/internal/harness/harness.go b/internal/harness/harness.go index 5b7b6978ad..a85aebf5a3 100644 --- a/internal/harness/harness.go +++ b/internal/harness/harness.go @@ -612,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 0cbd41e69d..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 }) diff --git a/internal/harness/scaffold_integration_test.go b/internal/harness/scaffold_integration_test.go index b7d82694f1..b5aa18d926 100644 --- a/internal/harness/scaffold_integration_test.go +++ b/internal/harness/scaffold_integration_test.go @@ -83,11 +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.Equal(t, "python3 -c 'import jsonschema'", h.ValidationLoop.PreflightCheck) + assert.Empty(t, h.ValidationLoop.PreflightCheck) } // TestLoadWithBase_WrapperOverridesBaseFields verifies that wrapper-level diff --git a/internal/scaffold/fullsend-repo/harness/fix.yaml b/internal/scaffold/fullsend-repo/harness/fix.yaml index d9fd8eefb5..16b1c88b61 100644 --- a/internal/scaffold/fullsend-repo/harness/fix.yaml +++ b/internal/scaffold/fullsend-repo/harness/fix.yaml @@ -27,7 +27,6 @@ post_script: scripts/post-fix.sh validation_loop: script: scripts/validate-output-schema.sh - preflight_check: "python3 -c 'import jsonschema'" max_iterations: 2 host_files: diff --git a/internal/scaffold/fullsend-repo/harness/prioritize.yaml b/internal/scaffold/fullsend-repo/harness/prioritize.yaml index 785102a33b..122b22bab2 100644 --- a/internal/scaffold/fullsend-repo/harness/prioritize.yaml +++ b/internal/scaffold/fullsend-repo/harness/prioritize.yaml @@ -26,7 +26,6 @@ post_script: scripts/post-prioritize.sh validation_loop: script: scripts/validate-output-schema.sh - preflight_check: "python3 -c 'import jsonschema'" max_iterations: 2 env: diff --git a/internal/scaffold/fullsend-repo/harness/retro.yaml b/internal/scaffold/fullsend-repo/harness/retro.yaml index 292610070b..beaa31f659 100644 --- a/internal/scaffold/fullsend-repo/harness/retro.yaml +++ b/internal/scaffold/fullsend-repo/harness/retro.yaml @@ -33,7 +33,6 @@ post_script: scripts/post-retro.sh validation_loop: script: scripts/validate-output-schema.sh - preflight_check: "python3 -c 'import jsonschema'" max_iterations: 2 env: diff --git a/internal/scaffold/fullsend-repo/harness/review.yaml b/internal/scaffold/fullsend-repo/harness/review.yaml index 5968f4d982..bba2db3e9b 100644 --- a/internal/scaffold/fullsend-repo/harness/review.yaml +++ b/internal/scaffold/fullsend-repo/harness/review.yaml @@ -36,7 +36,6 @@ post_script: scripts/post-review.sh validation_loop: script: scripts/validate-output-schema.sh - preflight_check: "python3 -c 'import jsonschema'" max_iterations: 2 env: diff --git a/internal/scaffold/fullsend-repo/harness/triage.yaml b/internal/scaffold/fullsend-repo/harness/triage.yaml index 230e6ecdab..7a59b2cc73 100644 --- a/internal/scaffold/fullsend-repo/harness/triage.yaml +++ b/internal/scaffold/fullsend-repo/harness/triage.yaml @@ -29,7 +29,6 @@ post_script: scripts/post-triage.sh validation_loop: script: scripts/validate-output-schema.sh - preflight_check: "python3 -c 'import jsonschema'" max_iterations: 2 env: