From eb98b11309220e7689705304ca13883e410e0d8a Mon Sep 17 00:00:00 2001 From: fullsend-code <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Mon, 20 Jul 2026 22:01:40 +0000 Subject: [PATCH 01/12] fix(#5393): defer validation until all retry iterations complete Two changes to the harness execution pipeline: 1. Make repo extraction (SafeDownload, os.RemoveAll) failures non-fatal when a validation loop is configured. Previously, extraction failures inside the retry loop caused an immediate return, aborting subsequent iterations. Now failures are logged as warnings and the loop continues, allowing later iterations to produce valid output. 2. Add a post-loop validation sweep that checks all completed iteration directories (latest first) if no iteration passed inline validation. This catches the case where extraction failed on the iteration that produced valid output but the output files were already extracted independently. 3. Add errSymlink error type in the sandbox package with a proper Error() method. The sanitizeDownload function now wraps symlink removal failures in errSymlink, which provides context (path, target) and prevents fmt.Sprintf panics when the error reaches a deferred cleanup handler that formats it for logging. Closes #5393 --- internal/cli/run.go | 45 +++++++++++++++++++++++++++++--- internal/sandbox/sandbox.go | 43 +++++++++++++++++++++++++++--- internal/sandbox/sandbox_test.go | 31 ++++++++++++++++++++++ 3 files changed, 112 insertions(+), 7 deletions(-) diff --git a/internal/cli/run.go b/internal/cli/run.go index dbb112140..315e72c63 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -1388,8 +1388,18 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep // 9d. Extract target repo back to host. SafeDownload removes dangerous // symlinks (absolute or repo-escaping) and .git/hooks/ to prevent sandbox escape. + // + // When a validation loop is configured, extraction failures are + // non-fatal: the output files (extracted in 9b) are independent of + // the repo download, so validation can still pass. Making extraction + // fatal here would abort the retry loop and prevent subsequent + // iterations from producing valid output — the root cause of #5393. if clearErr := forceRemoveAll(hostRepositoryDownloadDir); clearErr != nil { - return fmt.Errorf("clearing local repo %s before extraction: %w", hostRepositoryDownloadDir, clearErr) + if h.ValidationLoop != nil { + printer.StepWarn(fmt.Sprintf("Clearing local repo %s: %v", hostRepositoryDownloadDir, clearErr)) + } else { + return fmt.Errorf("clearing local repo %s before extraction: %w", hostRepositoryDownloadDir, clearErr) + } } repoExtractStart := time.Now() @@ -1398,9 +1408,14 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep if es := tx.ParseTranscriptErrors(iterTranscriptDir); len(es) > 0 { tx.EmitTranscriptErrors(os.Stderr, es) } - return fmt.Errorf("extracting target repo (iteration %d): %w", iteration, err) + if h.ValidationLoop != nil { + printer.StepWarn(fmt.Sprintf("Extracting target repo: %v", err)) + } else { + return fmt.Errorf("extracting target repo (iteration %d): %w", iteration, err) + } + } else { + printer.StepDone(fmt.Sprintf("Target repo extracted to %s (%.1fs)", hostRepositoryDownloadDir, time.Since(repoExtractStart).Seconds())) } - printer.StepDone(fmt.Sprintf("Target repo extracted to %s (%.1fs)", hostRepositoryDownloadDir, time.Since(repoExtractStart).Seconds())) // 9e. Run validation. if h.ValidationLoop == nil { @@ -1426,6 +1441,30 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep } } + // Post-loop validation sweep: if no iteration passed validation + // inline (e.g., because extraction failed on the iteration that + // produced valid output), check all completed iterations starting + // from the latest. This ensures a successful retry's output is + // found even when earlier steps in that iteration failed. See #5393. + if h.ValidationLoop != nil && !validationPassed { + for i := runCount; i >= 1; i-- { + iterDir := filepath.Join(runDir, fmt.Sprintf("iteration-%d", i)) + valStart := time.Now() + printer.StepStart(fmt.Sprintf("Post-loop validation (iteration %d): %s", i, h.ValidationLoop.Script)) + valCmd := exec.Command(h.ValidationLoop.Script) + valCmd.Dir = iterDir + valCmd.Env = append(os.Environ(), validationEnv(h, hostRepositoryDownloadDir, runDir)...) + valOut, valErr := valCmd.CombinedOutput() + + if valErr == nil { + printer.StepDone(fmt.Sprintf("Validation passed (iteration %d): %s (%.1fs)", i, strings.TrimSpace(string(valOut)), time.Since(valStart).Seconds())) + validationPassed = true + break + } + printer.StepWarn(fmt.Sprintf("Post-loop validation failed (iteration %d): %s", i, validationFailMessage(valOut, valErr))) + } + } + // Write aggregated behavioral metrics. if err := writeMetricsJSON(runDir, aggMetrics); err != nil { printer.StepWarn("Failed to write metrics.json: " + err.Error()) diff --git a/internal/sandbox/sandbox.go b/internal/sandbox/sandbox.go index 8ff9cbd9c..23ea376ff 100644 --- a/internal/sandbox/sandbox.go +++ b/internal/sandbox/sandbox.go @@ -38,6 +38,29 @@ const ( retryMaxBackoff = 15 * time.Second ) +// errSymlink is a structured error for symlink-related failures during +// sanitizeDownload. It wraps the underlying OS error with the symlink path +// and target so error messages are informative during cleanup reporting. +// The Error() method is required by the error interface — without it, +// fmt.Sprintf("%v", err) panics when the error reaches a deferred cleanup +// handler that formats the error for logging. See #5393. +type errSymlink struct { + Path string // filesystem path of the symlink + Target string // symlink target (empty if unreadable) + Err error // underlying error (e.g., from os.Remove) +} + +func (e *errSymlink) Error() string { + if e.Target != "" { + return fmt.Sprintf("symlink %s -> %s: %v", e.Path, e.Target, e.Err) + } + return fmt.Sprintf("symlink %s: %v", e.Path, e.Err) +} + +func (e *errSymlink) Unwrap() error { + return e.Err +} + func sanitizeDownload(localDir string) error { absLocal, err := filepath.Abs(localDir) if err != nil { @@ -55,11 +78,17 @@ func sanitizeDownload(localDir string) error { if d.Type()&fs.ModeSymlink != 0 { target, readErr := os.Readlink(path) if readErr != nil { - return os.Remove(path) + if rmErr := os.Remove(path); rmErr != nil { + return &errSymlink{Path: path, Err: rmErr} + } + return nil } // Absolute targets always point outside the repo root. if filepath.IsAbs(target) { - return os.Remove(path) + if rmErr := os.Remove(path); rmErr != nil { + return &errSymlink{Path: path, Target: target, Err: rmErr} + } + return nil } // Use EvalSymlinks, not filepath.Clean: Clean is textual and misses // chains where an in-repo dir-symlink is used as a component @@ -69,10 +98,16 @@ func sanitizeDownload(localDir string) error { rawPath := filepath.Dir(path) + string(filepath.Separator) + target resolved, evalErr := filepath.EvalSymlinks(rawPath) if evalErr != nil { - return os.Remove(path) + if rmErr := os.Remove(path); rmErr != nil { + return &errSymlink{Path: path, Target: target, Err: rmErr} + } + return nil } if !strings.HasPrefix(resolved+string(filepath.Separator), absLocal+string(filepath.Separator)) { - return os.Remove(path) + if rmErr := os.Remove(path); rmErr != nil { + return &errSymlink{Path: path, Target: target, Err: rmErr} + } + return nil } return nil } diff --git a/internal/sandbox/sandbox_test.go b/internal/sandbox/sandbox_test.go index 68520017c..f5ca77907 100644 --- a/internal/sandbox/sandbox_test.go +++ b/internal/sandbox/sandbox_test.go @@ -2,6 +2,7 @@ package sandbox import ( "context" + "fmt" "os" "os/exec" "path/filepath" @@ -478,6 +479,36 @@ func TestSanitizeDownload_EmptyDir(t *testing.T) { assert.NoError(t, err) } +func TestErrSymlink_ErrorMethod(t *testing.T) { + // Verify that errSymlink implements error and that fmt.Sprintf does not panic. + underlying := fmt.Errorf("permission denied") + + t.Run("with target", func(t *testing.T) { + e := &errSymlink{Path: "/repo/bad-link", Target: "/etc/passwd", Err: underlying} + msg := e.Error() + assert.Contains(t, msg, "/repo/bad-link") + assert.Contains(t, msg, "/etc/passwd") + assert.Contains(t, msg, "permission denied") + + // fmt.Sprintf must not panic (the original bug from #5393). + formatted := fmt.Sprintf("%v", e) + assert.NotEmpty(t, formatted) + }) + + t.Run("without target", func(t *testing.T) { + e := &errSymlink{Path: "/repo/unreadable", Err: underlying} + msg := e.Error() + assert.Contains(t, msg, "/repo/unreadable") + assert.Contains(t, msg, "permission denied") + assert.NotContains(t, msg, "->") + }) + + t.Run("unwrap", func(t *testing.T) { + e := &errSymlink{Path: "/repo/link", Target: "/tmp", Err: underlying} + assert.ErrorIs(t, e, underlying) + }) +} + func TestEffectiveReadyTimeout_Default(t *testing.T) { t.Setenv("FULLSEND_SANDBOX_READY_TIMEOUT", "") got := effectiveReadyTimeout(0) From 353757c031197d80a7068dbe28f088c01b34297f Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Wed, 22 Jul 2026 19:06:54 +0000 Subject: [PATCH 02/12] fix(#5393): address review feedback on PR #5395 - Clear TARGET_REPO_DIR in post-loop validation sweep env since hostRepositoryDownloadDir is a shared path that only reflects the last iteration's extraction attempt (HIGH finding) - Rewrite errSymlink comment to describe what the type actually improves (structured error context) without claiming an unverified panic mechanism (MEDIUM premature-decision) - Scope extraction non-fatal comment to document the constraint that validation scripts must not depend on TARGET_REPO_DIR (MEDIUM premature-decision) - Add TestSanitizeDownload_RemoveFailureReturnsErrSymlink to exercise the real os.Remove failure path via read-only parent directory and assert *errSymlink via errors.As (MEDIUM test coverage) - Fix StepWarn messages to use established past-tense pattern Addresses review feedback on #5395 --- internal/cli/run.go | 25 +++++++++++++------ internal/sandbox/sandbox.go | 10 +++----- internal/sandbox/sandbox_test.go | 42 +++++++++++++++++++++++++++++++- 3 files changed, 63 insertions(+), 14 deletions(-) diff --git a/internal/cli/run.go b/internal/cli/run.go index 315e72c63..c5ae58f8a 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -1390,13 +1390,16 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep // symlinks (absolute or repo-escaping) and .git/hooks/ to prevent sandbox escape. // // When a validation loop is configured, extraction failures are - // non-fatal: the output files (extracted in 9b) are independent of - // the repo download, so validation can still pass. Making extraction - // fatal here would abort the retry loop and prevent subsequent - // iterations from producing valid output — the root cause of #5393. + // non-fatal so the retry loop can continue to subsequent iterations + // (the root cause of #5393). This is safe only because all shipped + // validation scripts (validate-output-schema.sh) inspect output + // files (extracted in 9b) and never read TARGET_REPO_DIR. Custom + // validation scripts that depend on repo state will get stale data + // on iterations where extraction fails — document this constraint + // if adding new validation scripts. if clearErr := forceRemoveAll(hostRepositoryDownloadDir); clearErr != nil { if h.ValidationLoop != nil { - printer.StepWarn(fmt.Sprintf("Clearing local repo %s: %v", hostRepositoryDownloadDir, clearErr)) + printer.StepWarn(fmt.Sprintf("Failed to clear local repo %s: %v", hostRepositoryDownloadDir, clearErr)) } else { return fmt.Errorf("clearing local repo %s before extraction: %w", hostRepositoryDownloadDir, clearErr) } @@ -1409,7 +1412,7 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep tx.EmitTranscriptErrors(os.Stderr, es) } if h.ValidationLoop != nil { - printer.StepWarn(fmt.Sprintf("Extracting target repo: %v", err)) + printer.StepWarn(fmt.Sprintf("Failed to extract target repo: %v", err)) } else { return fmt.Errorf("extracting target repo (iteration %d): %w", iteration, err) } @@ -1446,6 +1449,14 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep // produced valid output), check all completed iterations starting // from the latest. This ensures a successful retry's output is // found even when earlier steps in that iteration failed. See #5393. + // + // TARGET_REPO_DIR is deliberately cleared in the sweep env because + // hostRepositoryDownloadDir is a shared path overwritten each + // iteration — it reflects only the last iteration's extraction + // attempt (which may have failed). Validation scripts run during + // the sweep must not depend on repo state. The post-script's + // REPO_DIR (line 950) has the same limitation; a future change + // should persist per-iteration repo checkouts to close this gap. if h.ValidationLoop != nil && !validationPassed { for i := runCount; i >= 1; i-- { iterDir := filepath.Join(runDir, fmt.Sprintf("iteration-%d", i)) @@ -1453,7 +1464,7 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep printer.StepStart(fmt.Sprintf("Post-loop validation (iteration %d): %s", i, h.ValidationLoop.Script)) valCmd := exec.Command(h.ValidationLoop.Script) valCmd.Dir = iterDir - valCmd.Env = append(os.Environ(), validationEnv(h, hostRepositoryDownloadDir, runDir)...) + valCmd.Env = append(os.Environ(), validationEnv(h, "", runDir)...) valOut, valErr := valCmd.CombinedOutput() if valErr == nil { diff --git a/internal/sandbox/sandbox.go b/internal/sandbox/sandbox.go index 23ea376ff..19d4bd5c8 100644 --- a/internal/sandbox/sandbox.go +++ b/internal/sandbox/sandbox.go @@ -38,12 +38,10 @@ const ( retryMaxBackoff = 15 * time.Second ) -// errSymlink is a structured error for symlink-related failures during -// sanitizeDownload. It wraps the underlying OS error with the symlink path -// and target so error messages are informative during cleanup reporting. -// The Error() method is required by the error interface — without it, -// fmt.Sprintf("%v", err) panics when the error reaches a deferred cleanup -// handler that formats the error for logging. See #5393. +// errSymlink wraps symlink-related os.Remove failures during +// sanitizeDownload with the symlink path and target, so error messages +// in cleanup reporting carry enough context to diagnose which symlink +// failed and why. type errSymlink struct { Path string // filesystem path of the symlink Target string // symlink target (empty if unreadable) diff --git a/internal/sandbox/sandbox_test.go b/internal/sandbox/sandbox_test.go index f5ca77907..ee80d911c 100644 --- a/internal/sandbox/sandbox_test.go +++ b/internal/sandbox/sandbox_test.go @@ -2,10 +2,12 @@ package sandbox import ( "context" + "errors" "fmt" "os" "os/exec" "path/filepath" + "runtime" "strings" "testing" "time" @@ -490,7 +492,7 @@ func TestErrSymlink_ErrorMethod(t *testing.T) { assert.Contains(t, msg, "/etc/passwd") assert.Contains(t, msg, "permission denied") - // fmt.Sprintf must not panic (the original bug from #5393). + // Verify fmt.Sprintf does not panic on errSymlink. formatted := fmt.Sprintf("%v", e) assert.NotEmpty(t, formatted) }) @@ -509,6 +511,44 @@ func TestErrSymlink_ErrorMethod(t *testing.T) { }) } +func TestSanitizeDownload_RemoveFailureReturnsErrSymlink(t *testing.T) { + if runtime.GOOS != "linux" { + t.Skip("read-only directory trick is Linux-specific") + } + if os.Getuid() == 0 { + t.Skip("root bypasses permission checks") + } + + // Create a directory with a dangerous absolute symlink, then make the + // parent directory read-only so os.Remove fails inside sanitizeDownload. + dir := t.TempDir() + sub := filepath.Join(dir, "sub") + require.NoError(t, os.MkdirAll(sub, 0o755)) + require.NoError(t, os.Symlink("/etc/passwd", filepath.Join(sub, "bad-link"))) + + // A second dangerous symlink later in the walk — it should remain + // unremoved because WalkDir aborts after the first error. + require.NoError(t, os.Symlink("/etc/shadow", filepath.Join(sub, "second-link"))) + + // Make sub read-only so os.Remove fails. + require.NoError(t, os.Chmod(sub, 0o555)) + t.Cleanup(func() { + os.Chmod(sub, 0o755) // restore for TempDir cleanup + }) + + err := sanitizeDownload(dir) + require.Error(t, err) + + var symErr *errSymlink + require.True(t, errors.As(err, &symErr), "expected *errSymlink, got %T: %v", err, err) + assert.Contains(t, symErr.Path, "bad-link") + assert.NotNil(t, symErr.Err) + + // The second symlink should still exist because the walk aborted. + _, statErr := os.Lstat(filepath.Join(sub, "second-link")) + assert.NoError(t, statErr, "second symlink should remain after walk aborted on first error") +} + func TestEffectiveReadyTimeout_Default(t *testing.T) { t.Setenv("FULLSEND_SANDBOX_READY_TIMEOUT", "") got := effectiveReadyTimeout(0) From cb1655702f663e46e00f9a95e19667f5f9e603f0 Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Wed, 22 Jul 2026 20:06:35 +0000 Subject: [PATCH 03/12] fix(#5393): close SafeDownload fail-open security gap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SafeDownload failures were non-fatal when a validation loop was configured, but SafeDownload is a security boundary (Download + sanitizeDownload). If sanitizeDownload aborts mid-walk, dangerous symlinks can survive in hostRepositoryDownloadDir and reach the post-script — a regression from the prior fail-closed behavior. When SafeDownload fails with a validation loop: - Clean up hostRepositoryDownloadDir (remove unsanitized content) - Continue the retry loop (output files from 9b are unaffected) Add repoExtractedOK flag to guard both in-loop validation's TARGET_REPO_DIR and the post-script's REPO_DIR — neither receives the repo path unless the last extraction succeeded. Addresses review feedback on #5395 --- internal/cli/run.go | 79 ++++++++++++++++++++++++++++++++------------- 1 file changed, 57 insertions(+), 22 deletions(-) diff --git a/internal/cli/run.go b/internal/cli/run.go index c5ae58f8a..be930e52f 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -900,6 +900,11 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep // ADR 0022's zero-trust model. var validationPassed bool + // repoExtractedOK tracks whether the last SafeDownload call succeeded. + // When false, hostRepositoryDownloadDir may not exist or may contain + // unsanitized content — callers (validation, post-script) must not use it. + var repoExtractedOK bool + // Download-dir cleanup is registered first so LIFO runs it last — // after the post-script defer has finished using it. hostRepositoryDownloadDir := filepath.Join(os.TempDir(), sandboxName) @@ -947,7 +952,18 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep // location, but sandbox output is now extracted to a temp dir. exec uses // last-value-wins so this append takes precedence. TODO(fullsend-ai/agents#191): // remove REPO_DIR from RunnerEnv entirely once harnesses no longer set it. - postCmd.Env = append(postCmd.Env, fmt.Sprintf("REPO_DIR=%s", hostRepositoryDownloadDir)) + // + // Pass REPO_DIR only when the last repo extraction succeeded. + // If SafeDownload failed, hostRepositoryDownloadDir was cleaned + // up and may not exist — passing it would expose the post-script + // to unsanitized or missing content. Post-scripts must handle + // empty REPO_DIR gracefully (the same as TARGET_REPO_DIR="" in + // the validation sweep). + postRepoDir := "" + if repoExtractedOK { + postRepoDir = hostRepositoryDownloadDir + } + postCmd.Env = append(postCmd.Env, fmt.Sprintf("REPO_DIR=%s", postRepoDir)) postCmd.Stdout = os.Stdout postCmd.Stderr = os.Stderr if err := postCmd.Run(); err != nil { @@ -1389,14 +1405,15 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep // 9d. Extract target repo back to host. SafeDownload removes dangerous // symlinks (absolute or repo-escaping) and .git/hooks/ to prevent sandbox escape. // - // When a validation loop is configured, extraction failures are - // non-fatal so the retry loop can continue to subsequent iterations - // (the root cause of #5393). This is safe only because all shipped - // validation scripts (validate-output-schema.sh) inspect output - // files (extracted in 9b) and never read TARGET_REPO_DIR. Custom - // validation scripts that depend on repo state will get stale data - // on iterations where extraction fails — document this constraint - // if adding new validation scripts. + // SafeDownload is a security boundary: it combines Download with + // sanitizeDownload, which strips dangerous symlinks. If either step + // fails, the on-disk content may contain unsanitized symlinks that + // could reach the post-script. SafeDownload failures are therefore + // always treated as fatal for the current iteration's repo state — + // we clean up the directory and skip to the next iteration. + // + // The forceRemoveAll pre-clear is a narrower staleness concern and + // remains non-fatal with a validation loop. if clearErr := forceRemoveAll(hostRepositoryDownloadDir); clearErr != nil { if h.ValidationLoop != nil { printer.StepWarn(fmt.Sprintf("Failed to clear local repo %s: %v", hostRepositoryDownloadDir, clearErr)) @@ -1412,13 +1429,23 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep tx.EmitTranscriptErrors(os.Stderr, es) } if h.ValidationLoop != nil { - printer.StepWarn(fmt.Sprintf("Failed to extract target repo: %v", err)) - } else { - return fmt.Errorf("extracting target repo (iteration %d): %w", iteration, err) + // SafeDownload failed — the repo directory may contain + // unsanitized content (sanitizeDownload aborts on first + // error, leaving subsequent dangerous symlinks intact). + // Clean up to prevent unsanitized content from reaching + // validation or the post-script, then continue the retry + // loop. Output files (extracted in 9b) are unaffected. + printer.StepWarn(fmt.Sprintf("Failed to extract target repo (cleaning up): %v", err)) + if rmErr := forceRemoveAll(hostRepositoryDownloadDir); rmErr != nil { + printer.StepWarn(fmt.Sprintf("Failed to clean up repo dir after extraction failure: %v", rmErr)) + } + repoExtractedOK = false + continue } - } else { - printer.StepDone(fmt.Sprintf("Target repo extracted to %s (%.1fs)", hostRepositoryDownloadDir, time.Since(repoExtractStart).Seconds())) + return fmt.Errorf("extracting target repo (iteration %d): %w", iteration, err) } + repoExtractedOK = true + printer.StepDone(fmt.Sprintf("Target repo extracted to %s (%.1fs)", hostRepositoryDownloadDir, time.Since(repoExtractStart).Seconds())) // 9e. Run validation. if h.ValidationLoop == nil { @@ -1429,7 +1456,14 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep printer.StepStart("Running validation: " + h.ValidationLoop.Script) valCmd := exec.Command(h.ValidationLoop.Script) valCmd.Dir = iterDir - valCmd.Env = append(os.Environ(), validationEnv(h, hostRepositoryDownloadDir, runDir)...) + // Pass TARGET_REPO_DIR only when repo extraction succeeded; + // otherwise pass empty to prevent validation scripts from using + // unsanitized or stale repo content. + valRepoDir := "" + if repoExtractedOK { + valRepoDir = hostRepositoryDownloadDir + } + valCmd.Env = append(os.Environ(), validationEnv(h, valRepoDir, runDir)...) valOut, valErr := valCmd.CombinedOutput() if valErr == nil { @@ -1450,13 +1484,14 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep // from the latest. This ensures a successful retry's output is // found even when earlier steps in that iteration failed. See #5393. // - // TARGET_REPO_DIR is deliberately cleared in the sweep env because - // hostRepositoryDownloadDir is a shared path overwritten each - // iteration — it reflects only the last iteration's extraction - // attempt (which may have failed). Validation scripts run during - // the sweep must not depend on repo state. The post-script's - // REPO_DIR (line 950) has the same limitation; a future change - // should persist per-iteration repo checkouts to close this gap. + // TARGET_REPO_DIR is cleared because hostRepositoryDownloadDir is + // a shared path reflecting only the last iteration's extraction + // state (which may have failed and been cleaned up). Validation + // scripts must not depend on repo state. The post-script's REPO_DIR + // is similarly guarded by repoExtractedOK — when the last extraction + // failed, REPO_DIR is empty. A future change should persist + // per-iteration repo checkouts to provide repo state for non-last + // iterations. if h.ValidationLoop != nil && !validationPassed { for i := runCount; i >= 1; i-- { iterDir := filepath.Join(runDir, fmt.Sprintf("iteration-%d", i)) From e844ff072ae76c1261eee7e71fb4199a8be174c4 Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Wed, 22 Jul 2026 21:36:25 +0000 Subject: [PATCH 04/12] fix(#5393): address review feedback on PR #5395 - Merge main to pick up forceRemoveAll (PR #5474); switch both os.RemoveAll(hostRepositoryDownloadDir) call sites (pre-clear and cleanup-after-extraction-failure) to forceRemoveAll to handle read-only directories left by sandbox enforcement. - Fix post-loop sweep iteration tracking: when the sweep validates iteration i != runCount, clear repoExtractedOK so the post-script receives empty REPO_DIR rather than a mismatched checkout. Extract postLoopValidationSweep into a testable helper returning sweepResult. - Add 6 unit tests for postLoopValidationSweep covering: latest-iter pass, earlier-iter pass (clears repoOK), none pass, TARGET_REPO_DIR always empty during sweep, and repoExtractedOK preservation logic. Addresses review feedback on #5395 --- internal/cli/run.go | 73 +++++++++------- internal/cli/run_test.go | 174 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 219 insertions(+), 28 deletions(-) diff --git a/internal/cli/run.go b/internal/cli/run.go index be930e52f..8fb3447ff 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -900,9 +900,12 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep // ADR 0022's zero-trust model. var validationPassed bool - // repoExtractedOK tracks whether the last SafeDownload call succeeded. - // When false, hostRepositoryDownloadDir may not exist or may contain - // unsanitized content — callers (validation, post-script) must not use it. + // repoExtractedOK tracks whether hostRepositoryDownloadDir is safe + // and corresponds to the validated iteration. It is false when: + // - the last SafeDownload call failed (dir may be missing/unsanitized), or + // - the post-loop sweep validated an earlier iteration (dir holds a + // different iteration's checkout than what was validated). + // Callers (validation, post-script) must not use the dir when false. var repoExtractedOK bool // Download-dir cleanup is registered first so LIFO runs it last — @@ -1483,32 +1486,10 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep // produced valid output), check all completed iterations starting // from the latest. This ensures a successful retry's output is // found even when earlier steps in that iteration failed. See #5393. - // - // TARGET_REPO_DIR is cleared because hostRepositoryDownloadDir is - // a shared path reflecting only the last iteration's extraction - // state (which may have failed and been cleaned up). Validation - // scripts must not depend on repo state. The post-script's REPO_DIR - // is similarly guarded by repoExtractedOK — when the last extraction - // failed, REPO_DIR is empty. A future change should persist - // per-iteration repo checkouts to provide repo state for non-last - // iterations. if h.ValidationLoop != nil && !validationPassed { - for i := runCount; i >= 1; i-- { - iterDir := filepath.Join(runDir, fmt.Sprintf("iteration-%d", i)) - valStart := time.Now() - printer.StepStart(fmt.Sprintf("Post-loop validation (iteration %d): %s", i, h.ValidationLoop.Script)) - valCmd := exec.Command(h.ValidationLoop.Script) - valCmd.Dir = iterDir - valCmd.Env = append(os.Environ(), validationEnv(h, "", runDir)...) - valOut, valErr := valCmd.CombinedOutput() - - if valErr == nil { - printer.StepDone(fmt.Sprintf("Validation passed (iteration %d): %s (%.1fs)", i, strings.TrimSpace(string(valOut)), time.Since(valStart).Seconds())) - validationPassed = true - break - } - printer.StepWarn(fmt.Sprintf("Post-loop validation failed (iteration %d): %s", i, validationFailMessage(valOut, valErr))) - } + sweep := postLoopValidationSweep(h, runDir, runCount, repoExtractedOK, printer) + validationPassed = sweep.passed + repoExtractedOK = sweep.repoExtractedOK } // Write aggregated behavioral metrics. @@ -1984,6 +1965,42 @@ func validationFailMessage(output []byte, execErr error) string { return execErr.Error() } +// sweepResult holds the outcome of a post-loop validation sweep. +type sweepResult struct { + passed bool // true if any iteration's validation passed + validatedIter int // which iteration passed (0 if none) + repoExtractedOK bool // false when the validated iteration != runCount +} + +// postLoopValidationSweep runs the validation script against each completed +// iteration directory, starting from the latest (runCount) and working +// backwards. It returns the first iteration that passes, or signals that +// none passed. When the passing iteration is not runCount, repoExtractedOK +// is set to false because hostRepositoryDownloadDir holds a different +// iteration's repo checkout — the post-script must not use it. +func postLoopValidationSweep(h *harness.Harness, runDir string, runCount int, currentRepoExtractedOK bool, printer *ui.Printer) sweepResult { + for i := runCount; i >= 1; i-- { + iterDir := filepath.Join(runDir, fmt.Sprintf("iteration-%d", i)) + valStart := time.Now() + printer.StepStart(fmt.Sprintf("Post-loop validation (iteration %d): %s", i, h.ValidationLoop.Script)) + valCmd := exec.Command(h.ValidationLoop.Script) + valCmd.Dir = iterDir + valCmd.Env = append(os.Environ(), validationEnv(h, "", runDir)...) + valOut, valErr := valCmd.CombinedOutput() + + if valErr == nil { + printer.StepDone(fmt.Sprintf("Validation passed (iteration %d): %s (%.1fs)", i, strings.TrimSpace(string(valOut)), time.Since(valStart).Seconds())) + repoOK := currentRepoExtractedOK + if i != runCount { + repoOK = false + } + return sweepResult{passed: true, validatedIter: i, repoExtractedOK: repoOK} + } + printer.StepWarn(fmt.Sprintf("Post-loop validation failed (iteration %d): %s", i, validationFailMessage(valOut, valErr))) + } + return sweepResult{passed: false, repoExtractedOK: currentRepoExtractedOK} +} + // envToList converts a map of env vars to a sorted list of KEY=VALUE strings. // toTelemetryMetrics maps fullsend's aggregate run metrics onto the telemetry // summary metrics — the same numbers already written to metrics.json, no new diff --git a/internal/cli/run_test.go b/internal/cli/run_test.go index 70a9f3891..4a02aa072 100644 --- a/internal/cli/run_test.go +++ b/internal/cli/run_test.go @@ -2176,6 +2176,180 @@ func TestValidationEnv_OmitsSchemaWhenNoValidationLoop(t *testing.T) { } } +// writeValScript creates a validation script at dir/name that exits 0 if a +// marker file named "pass" exists in the script's working directory, and +// exits 1 otherwise. Returns the absolute path to the script. +func writeValScript(t *testing.T, dir, name string) string { + t.Helper() + script := filepath.Join(dir, name) + require.NoError(t, os.WriteFile(script, []byte("#!/bin/sh\n[ -f pass ]\n"), 0o755)) + return script +} + +func TestPostLoopValidationSweep_LatestIterPasses(t *testing.T) { + runDir := t.TempDir() + scriptDir := t.TempDir() + script := writeValScript(t, scriptDir, "validate.sh") + + // Create 3 iteration dirs; mark iteration-3 as passing. + for i := 1; i <= 3; i++ { + iterDir := filepath.Join(runDir, fmt.Sprintf("iteration-%d", i)) + require.NoError(t, os.MkdirAll(iterDir, 0o755)) + } + // Create "pass" marker in iteration-3 so the script exits 0. + require.NoError(t, os.WriteFile(filepath.Join(runDir, "iteration-3", "pass"), nil, 0o644)) + + h := &harness.Harness{ + RunnerEnv: map[string]string{}, + ValidationLoop: &harness.ValidationLoop{Script: script}, + } + printer := ui.New(io.Discard) + + result := postLoopValidationSweep(h, runDir, 3, true, printer) + assert.True(t, result.passed, "sweep should pass when latest iteration passes") + assert.Equal(t, 3, result.validatedIter, "validated iteration should be runCount") + assert.True(t, result.repoExtractedOK, "repoExtractedOK should stay true when i == runCount") +} + +func TestPostLoopValidationSweep_EarlierIterPasses(t *testing.T) { + runDir := t.TempDir() + scriptDir := t.TempDir() + script := writeValScript(t, scriptDir, "validate.sh") + + // Create 3 iteration dirs; only iteration-1 passes. + for i := 1; i <= 3; i++ { + iterDir := filepath.Join(runDir, fmt.Sprintf("iteration-%d", i)) + require.NoError(t, os.MkdirAll(iterDir, 0o755)) + } + require.NoError(t, os.WriteFile(filepath.Join(runDir, "iteration-1", "pass"), nil, 0o644)) + + h := &harness.Harness{ + RunnerEnv: map[string]string{}, + ValidationLoop: &harness.ValidationLoop{Script: script}, + } + printer := ui.New(io.Discard) + + result := postLoopValidationSweep(h, runDir, 3, true, printer) + assert.True(t, result.passed, "sweep should pass when earlier iteration passes") + assert.Equal(t, 1, result.validatedIter, "validated iteration should be 1") + assert.False(t, result.repoExtractedOK, "repoExtractedOK must be false when i != runCount") +} + +func TestPostLoopValidationSweep_NonePass(t *testing.T) { + runDir := t.TempDir() + scriptDir := t.TempDir() + script := writeValScript(t, scriptDir, "validate.sh") + + // Create 2 iteration dirs; none pass. + for i := 1; i <= 2; i++ { + iterDir := filepath.Join(runDir, fmt.Sprintf("iteration-%d", i)) + require.NoError(t, os.MkdirAll(iterDir, 0o755)) + } + + h := &harness.Harness{ + RunnerEnv: map[string]string{}, + ValidationLoop: &harness.ValidationLoop{Script: script}, + } + printer := ui.New(io.Discard) + + result := postLoopValidationSweep(h, runDir, 2, false, printer) + assert.False(t, result.passed, "sweep should not pass when no iteration passes") + assert.Equal(t, 0, result.validatedIter, "validatedIter should be 0 when none pass") + assert.False(t, result.repoExtractedOK, "repoExtractedOK should propagate input value") +} + +func TestPostLoopValidationSweep_EarlierIterClearsRepoOK(t *testing.T) { + // Verifies the key security invariant: when the sweep validates an + // earlier iteration (i != runCount), repoExtractedOK is cleared even + // if the caller passed true (meaning the last SafeDownload succeeded). + // This prevents the post-script from receiving REPO_DIR pointing to + // a different iteration's checkout than what was validated. + runDir := t.TempDir() + scriptDir := t.TempDir() + script := writeValScript(t, scriptDir, "validate.sh") + + for i := 1; i <= 2; i++ { + iterDir := filepath.Join(runDir, fmt.Sprintf("iteration-%d", i)) + require.NoError(t, os.MkdirAll(iterDir, 0o755)) + } + // Only iteration-1 passes; iteration-2 (runCount) fails. + require.NoError(t, os.WriteFile(filepath.Join(runDir, "iteration-1", "pass"), nil, 0o644)) + + h := &harness.Harness{ + RunnerEnv: map[string]string{}, + ValidationLoop: &harness.ValidationLoop{Script: script}, + } + printer := ui.New(io.Discard) + + // currentRepoExtractedOK=true simulates a successful last SafeDownload. + result := postLoopValidationSweep(h, runDir, 2, true, printer) + assert.True(t, result.passed) + assert.Equal(t, 1, result.validatedIter) + assert.False(t, result.repoExtractedOK, + "repoExtractedOK must be false when validated iteration != runCount, even if last SafeDownload succeeded") +} + +func TestPostLoopValidationSweep_PassesEmptyTargetRepoDir(t *testing.T) { + // Verifies that the sweep always passes TARGET_REPO_DIR="" to the + // validation script (hostRepositoryDownloadDir is unreliable during sweep). + runDir := t.TempDir() + scriptDir := t.TempDir() + + // Script that checks TARGET_REPO_DIR is empty and writes it to a file. + script := filepath.Join(scriptDir, "validate.sh") + checkFile := filepath.Join(scriptDir, "target_repo_dir.txt") + require.NoError(t, os.WriteFile(script, []byte(fmt.Sprintf( + "#!/bin/sh\necho \"$TARGET_REPO_DIR\" > %s\n[ -f pass ]\n", checkFile)), 0o755)) + + iterDir := filepath.Join(runDir, "iteration-1") + require.NoError(t, os.MkdirAll(iterDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(iterDir, "pass"), nil, 0o644)) + + h := &harness.Harness{ + RunnerEnv: map[string]string{}, + ValidationLoop: &harness.ValidationLoop{Script: script}, + } + printer := ui.New(io.Discard) + + result := postLoopValidationSweep(h, runDir, 1, true, printer) + require.True(t, result.passed) + + got, err := os.ReadFile(checkFile) + require.NoError(t, err) + assert.Equal(t, "\n", string(got), "TARGET_REPO_DIR should be empty during sweep") +} + +func TestSweepResult_RepoExtractedOK_PreservesWhenRunCount(t *testing.T) { + // When the latest iteration (runCount) passes, the input + // currentRepoExtractedOK should be preserved. + runDir := t.TempDir() + scriptDir := t.TempDir() + script := writeValScript(t, scriptDir, "validate.sh") + + iterDir := filepath.Join(runDir, "iteration-1") + require.NoError(t, os.MkdirAll(iterDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(iterDir, "pass"), nil, 0o644)) + + h := &harness.Harness{ + RunnerEnv: map[string]string{}, + ValidationLoop: &harness.ValidationLoop{Script: script}, + } + printer := ui.New(io.Discard) + + // With currentRepoExtractedOK=false (last SafeDownload failed). + result := postLoopValidationSweep(h, runDir, 1, false, printer) + assert.True(t, result.passed) + assert.Equal(t, 1, result.validatedIter) + assert.False(t, result.repoExtractedOK, + "repoExtractedOK should remain false when input was false, even when i == runCount") + + // With currentRepoExtractedOK=true (last SafeDownload succeeded). + result2 := postLoopValidationSweep(h, runDir, 1, true, printer) + assert.True(t, result2.passed) + assert.True(t, result2.repoExtractedOK, + "repoExtractedOK should remain true when input was true and i == runCount") +} + func TestOpenTeeReader_EmptyPath(t *testing.T) { src := strings.NewReader("hello") printer := ui.New(io.Discard) From 7a0fadcc131052aa294a52fe3cb98c0995340bd7 Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Wed, 22 Jul 2026 23:11:01 +0000 Subject: [PATCH 05/12] docs(cli): document dual-phase validation design and update flow diagram Add a design comment near the validation loop explaining why both inline validation (early exit) and the post-loop sweep (#5393) are necessary. Update the cli-internals.md flow diagram to reflect the dual-phase model, non-fatal SafeDownload under validation_loop, and the TARGET_REPO_DIR="" constraint in the sweep. Also: fix misleading comment about REPO_DIR handling (post-scripts fail closed, not gracefully), and simplify dead-code repoExtractedOK check at step 9e (always true at that point). Addresses review feedback on #5395 --- docs/guides/dev/cli-internals.md | 27 ++++++++++++++++++--- internal/cli/run.go | 41 +++++++++++++++++++++++--------- 2 files changed, 54 insertions(+), 14 deletions(-) diff --git a/docs/guides/dev/cli-internals.md b/docs/guides/dev/cli-internals.md index 0b6e7cdcc..ecc0906e8 100644 --- a/docs/guides/dev/cli-internals.md +++ b/docs/guides/dev/cli-internals.md @@ -460,20 +460,41 @@ Vendoring commit messages use title + body (upload and stale delete). `github st │ │ Extract output │ SafeDownload() with sanitization: │ │ │ │ - Remove dangerous symlinks (sandbox escape) │ │ │ │ - Remove .git/hooks/ (hook injection) │ +│ │ │ │ +│ │ │ With validation_loop: SafeDownload │ +│ │ │ failure is non-fatal — clean up repo dir │ +│ │ │ and continue to next iteration. Output │ +│ │ │ files (extracted separately) are kept. │ │ └──────┬───────────┘ │ │ ▼ │ │ ┌──────────────────────────────────────────┐ │ │ │ Validation loop (if configured) │ │ │ │ │ │ -│ │ for i := 0; i < max_iterations; i++ { │ │ +│ │ Phase 1 — inline validation: │ │ +│ │ for i := 1; i <= max_iterations; i++ { │ │ +│ │ run agent → extract output │ │ +│ │ SafeDownload repo (non-fatal on fail) │ │ │ │ run validation script │ │ -│ │ if pass → break │ │ -│ │ feed feedback → re-run agent │ │ +│ │ if pass → break (early exit) │ │ +│ │ feed feedback → next iteration │ │ │ │ } │ │ +│ │ │ │ +│ │ Phase 2 — post-loop sweep (#5393): │ │ +│ │ if no inline pass: │ │ +│ │ for i := latest..1 { │ │ +│ │ run validation on iteration-i dir │ │ +│ │ TARGET_REPO_DIR="" (repo dir is │ │ +│ │ unreliable across iterations) │ │ +│ │ if pass → use this iteration; break │ │ +│ │ } │ │ │ └──────────┬───────────────────────────────┘ │ │ ▼ │ │ ┌──────────────────┐ │ │ │ Post-script │ Run harness.post_script (host-side) │ +│ │ │ REPO_DIR set only when last SafeDownload │ +│ │ │ succeeded and validated iteration is the │ +│ │ │ latest; empty otherwise (post-scripts │ +│ │ │ must fail closed on empty REPO_DIR) │ │ └──────┬───────────┘ │ │ ▼ │ │ ┌──────────────────┐ │ diff --git a/internal/cli/run.go b/internal/cli/run.go index 8fb3447ff..834838963 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -959,9 +959,10 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep // Pass REPO_DIR only when the last repo extraction succeeded. // If SafeDownload failed, hostRepositoryDownloadDir was cleaned // up and may not exist — passing it would expose the post-script - // to unsanitized or missing content. Post-scripts must handle - // empty REPO_DIR gracefully (the same as TARGET_REPO_DIR="" in - // the validation sweep). + // to unsanitized or missing content. Post-scripts must fail + // closed on empty REPO_DIR (the scaffolded post-code.sh and + // post-fix.sh already do via ${REPO_DIR:-repo} + directory + // existence check). postRepoDir := "" if repoExtractedOK { postRepoDir = hostRepositoryDownloadDir @@ -1250,6 +1251,27 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep maxIterations = h.ValidationLoop.MaxIterations } + // Dual-phase validation design (#5393): + // + // Phase 1 — inline validation (step 9e): runs after each iteration. + // If the iteration passes, we break immediately without waiting for + // remaining iterations. This is an early-exit optimization that + // avoids burning agent compute when a good result is already in hand. + // + // Phase 2 — post-loop sweep (postLoopValidationSweep): runs only when + // no iteration passed inline. This is the #5393 fix: it catches the + // case where an iteration produced valid output but its inline + // validation was skipped because SafeDownload failed on that same + // iteration (extraction failure triggers `continue`, bypassing 9e). + // The sweep re-validates all completed iteration directories, latest + // first, using only the output files (TARGET_REPO_DIR is empty because + // hostRepositoryDownloadDir may not correspond to the validated + // iteration). + // + // Both phases are necessary: removing inline validation would force + // every run to exhaust all maxIterations even when iteration 1 passes, + // while removing the sweep would regress #5393. + oidcCtx, oidcCancel := context.WithCancel(context.Background()) var oidcWg sync.WaitGroup if oidcURL := os.Getenv("FULLSEND_GCP_OIDC_URL"); oidcURL != "" { @@ -1459,14 +1481,11 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep printer.StepStart("Running validation: " + h.ValidationLoop.Script) valCmd := exec.Command(h.ValidationLoop.Script) valCmd.Dir = iterDir - // Pass TARGET_REPO_DIR only when repo extraction succeeded; - // otherwise pass empty to prevent validation scripts from using - // unsanitized or stale repo content. - valRepoDir := "" - if repoExtractedOK { - valRepoDir = hostRepositoryDownloadDir - } - valCmd.Env = append(os.Environ(), validationEnv(h, valRepoDir, runDir)...) + // At this point repoExtractedOK is always true: SafeDownload + // failure sets it to false and continues (skipping step 9e), + // while success sets it to true immediately above. Pass the + // repo dir directly. + valCmd.Env = append(os.Environ(), validationEnv(h, hostRepositoryDownloadDir, runDir)...) valOut, valErr := valCmd.CombinedOutput() if valErr == nil { From 0ee0a67be4e3199163b5da2181034ea65eaa6357 Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Thu, 23 Jul 2026 00:54:43 +0000 Subject: [PATCH 06/12] fix(cli): thread validated iteration to post-scripts via FULLSEND_VALIDATED_ITERATION_DIR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The post-loop validation sweep returns which iteration passed validation (sweep.validatedIter), but the call site discarded this value. Post-scripts independently selected the last iteration's output, which could be schema-invalid content from a failed iteration — defeating the purpose of the validation sweep (#5393). Changes: - Track validatedIterNum in runAgent and set it on both inline validation pass (step 9e break) and sweep pass - Pass FULLSEND_VALIDATED_ITERATION_DIR=/iteration-/output to the post-script environment when a validation loop is configured and an iteration passed validation - Update all 5 validation_loop-enabled post-scripts (review, triage, retro, prioritize, fix) to prefer FULLSEND_VALIDATED_ITERATION_DIR when set, falling back to last-iteration scanning for backward compatibility - Update cli-internals.md flow diagram to document the new env var Addresses review feedback on #5395 --- docs/guides/dev/cli-internals.md | 6 +++++ internal/cli/run.go | 20 +++++++++++++++++ .../fullsend-repo/scripts/post-fix.sh | 20 ++++++++++++----- .../fullsend-repo/scripts/post-prioritize.sh | 21 ++++++++++++------ .../fullsend-repo/scripts/post-retro.sh | 21 ++++++++++++------ .../fullsend-repo/scripts/post-review.sh | 11 ++++++++-- .../fullsend-repo/scripts/post-triage.sh | 22 ++++++++++++------- 7 files changed, 91 insertions(+), 30 deletions(-) diff --git a/docs/guides/dev/cli-internals.md b/docs/guides/dev/cli-internals.md index ecc0906e8..a7c997bd4 100644 --- a/docs/guides/dev/cli-internals.md +++ b/docs/guides/dev/cli-internals.md @@ -495,6 +495,12 @@ Vendoring commit messages use title + body (upload and stale delete). `github st │ │ │ succeeded and validated iteration is the │ │ │ │ latest; empty otherwise (post-scripts │ │ │ │ must fail closed on empty REPO_DIR) │ +│ │ │ │ +│ │ │ FULLSEND_VALIDATED_ITERATION_DIR points │ +│ │ │ to the validated iteration's output dir. │ +│ │ │ Post-scripts must use this (when set) to │ +│ │ │ select the result file instead of blindly │ +│ │ │ taking the last iteration. │ │ └──────┬───────────┘ │ │ ▼ │ │ ┌──────────────────┐ │ diff --git a/internal/cli/run.go b/internal/cli/run.go index 834838963..33c53df4d 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -908,6 +908,13 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep // Callers (validation, post-script) must not use the dir when false. var repoExtractedOK bool + // validatedIterNum records which iteration passed validation (1-based), + // or 0 if none. Set by inline validation (step 9e break) or the + // post-loop sweep. The post-script defer uses it to communicate + // FULLSEND_VALIDATED_ITERATION_DIR to the post-script so it selects + // the correct iteration's output rather than blindly taking the last. + var validatedIterNum int + // Download-dir cleanup is registered first so LIFO runs it last — // after the post-script defer has finished using it. hostRepositoryDownloadDir := filepath.Join(os.TempDir(), sandboxName) @@ -968,6 +975,17 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep postRepoDir = hostRepositoryDownloadDir } postCmd.Env = append(postCmd.Env, fmt.Sprintf("REPO_DIR=%s", postRepoDir)) + // FULLSEND_VALIDATED_ITERATION_DIR tells the post-script which + // iteration's output was validated. Without this, post-scripts + // that scan for the last iteration-*/output would pick up + // unvalidated output when the sweep validated an earlier + // iteration. Empty when no validation loop is configured or + // when no iteration passed validation (the post-script is + // skipped in the latter case, so this is defensive). + if h.ValidationLoop != nil && validatedIterNum > 0 { + postCmd.Env = append(postCmd.Env, fmt.Sprintf("FULLSEND_VALIDATED_ITERATION_DIR=%s", + filepath.Join(runDir, fmt.Sprintf("iteration-%d/output", validatedIterNum)))) + } postCmd.Stdout = os.Stdout postCmd.Stderr = os.Stderr if err := postCmd.Run(); err != nil { @@ -1491,6 +1509,7 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep if valErr == nil { printer.StepDone(fmt.Sprintf("Validation passed: %s (%.1fs)", strings.TrimSpace(string(valOut)), time.Since(valStart).Seconds())) validationPassed = true + validatedIterNum = iteration break } @@ -1509,6 +1528,7 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep sweep := postLoopValidationSweep(h, runDir, runCount, repoExtractedOK, printer) validationPassed = sweep.passed repoExtractedOK = sweep.repoExtractedOK + validatedIterNum = sweep.validatedIter } // Write aggregated behavioral metrics. diff --git a/internal/scaffold/fullsend-repo/scripts/post-fix.sh b/internal/scaffold/fullsend-repo/scripts/post-fix.sh index a537e21b7..f2ca6d9a0 100644 --- a/internal/scaffold/fullsend-repo/scripts/post-fix.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-fix.sh @@ -359,12 +359,20 @@ fi # iteration-/output/fix-result.json within runDir. Uses glob order # (naturally ascending iteration numbers) to find the last iteration, # matching the pattern in post-triage.sh. -RESULT_FILE="" -for dir in "${RUN_DIR}"/iteration-*/output; do - if [ -f "${dir}/fix-result.json" ]; then - RESULT_FILE="${dir}/fix-result.json" - fi -done +# Prefer the validated iteration directory set by the harness +# (FULLSEND_VALIDATED_ITERATION_DIR) — without it, the last iteration's output +# may be schema-invalid content that failed validation. Fall back to scanning +# for the last iteration for backward compatibility. +if [ -n "${FULLSEND_VALIDATED_ITERATION_DIR:-}" ] && [ -f "${FULLSEND_VALIDATED_ITERATION_DIR}/fix-result.json" ]; then + RESULT_FILE="${FULLSEND_VALIDATED_ITERATION_DIR}/fix-result.json" +else + RESULT_FILE="" + for dir in "${RUN_DIR}"/iteration-*/output; do + if [ -f "${dir}/fix-result.json" ]; then + RESULT_FILE="${dir}/fix-result.json" + fi + done +fi if [ -z "${RESULT_FILE}" ] || [ ! -f "${RESULT_FILE}" ]; then echo "::warning::No fix-result.json found — skipping summary comment" diff --git a/internal/scaffold/fullsend-repo/scripts/post-prioritize.sh b/internal/scaffold/fullsend-repo/scripts/post-prioritize.sh index 9e331cd5e..bb55a37b6 100755 --- a/internal/scaffold/fullsend-repo/scripts/post-prioritize.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-prioritize.sh @@ -339,13 +339,20 @@ if [[ ! "${GITHUB_ISSUE_URL}" =~ ^https://github\.com/[a-zA-Z0-9._-]+/[a-zA-Z0-9 exit 1 fi -# Find the result JSON from the last iteration. -RESULT_FILE="" -for dir in iteration-*/output; do - if [[ -f "${dir}/agent-result.json" ]]; then - RESULT_FILE="${dir}/agent-result.json" - fi -done +# Find the result JSON. Prefer the validated iteration directory set by the +# harness (FULLSEND_VALIDATED_ITERATION_DIR) — without it, the last iteration's +# output may be schema-invalid content that failed validation. Fall back to +# scanning for the last iteration for backward compatibility. +if [[ -n "${FULLSEND_VALIDATED_ITERATION_DIR:-}" ]] && [[ -f "${FULLSEND_VALIDATED_ITERATION_DIR}/agent-result.json" ]]; then + RESULT_FILE="${FULLSEND_VALIDATED_ITERATION_DIR}/agent-result.json" +else + RESULT_FILE="" + for dir in iteration-*/output; do + if [[ -f "${dir}/agent-result.json" ]]; then + RESULT_FILE="${dir}/agent-result.json" + fi + done +fi if [[ -z "${RESULT_FILE}" ]]; then echo "ERROR: agent-result.json not found in any iteration output directory" >&2 diff --git a/internal/scaffold/fullsend-repo/scripts/post-retro.sh b/internal/scaffold/fullsend-repo/scripts/post-retro.sh index dba492f3c..43543f91a 100755 --- a/internal/scaffold/fullsend-repo/scripts/post-retro.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-retro.sh @@ -17,13 +17,20 @@ set -euo pipefail : "${GH_TOKEN:?GH_TOKEN is required}" echo "::add-mask::${GH_TOKEN}" -# Find the retro result JSON (same pattern as post-triage.sh). -RESULT_FILE="" -for dir in iteration-*/output; do - if [[ -f "${dir}/agent-result.json" ]]; then - RESULT_FILE="${dir}/agent-result.json" - fi -done +# Find the retro result JSON. Prefer the validated iteration directory set by +# the harness (FULLSEND_VALIDATED_ITERATION_DIR) — without it, the last +# iteration's output may be schema-invalid content that failed validation. +# Fall back to scanning for the last iteration for backward compatibility. +if [[ -n "${FULLSEND_VALIDATED_ITERATION_DIR:-}" ]] && [[ -f "${FULLSEND_VALIDATED_ITERATION_DIR}/agent-result.json" ]]; then + RESULT_FILE="${FULLSEND_VALIDATED_ITERATION_DIR}/agent-result.json" +else + RESULT_FILE="" + for dir in iteration-*/output; do + if [[ -f "${dir}/agent-result.json" ]]; then + RESULT_FILE="${dir}/agent-result.json" + fi + done +fi if [[ -z "${RESULT_FILE}" ]]; then echo "ERROR: agent-result.json not found in any iteration output directory" >&2 diff --git a/internal/scaffold/fullsend-repo/scripts/post-review.sh b/internal/scaffold/fullsend-repo/scripts/post-review.sh index a7b783329..a5eaa976a 100755 --- a/internal/scaffold/fullsend-repo/scripts/post-review.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-review.sh @@ -56,8 +56,15 @@ The \`/fs-review\` command only reviews open pull requests. exit 0 fi -# Find the agent result from the last iteration -RESULT_FILE=$(find . -maxdepth 4 -path '*/iteration-*/output/agent-result.json' | sort -V | tail -1) +# Find the agent result. Prefer the validated iteration directory set by the +# harness (FULLSEND_VALIDATED_ITERATION_DIR) — without it, the last iteration's +# output may be schema-invalid content that failed validation. Fall back to the +# last iteration for backward compatibility with older fullsend versions. +if [ -n "${FULLSEND_VALIDATED_ITERATION_DIR:-}" ] && [ -f "${FULLSEND_VALIDATED_ITERATION_DIR}/agent-result.json" ]; then + RESULT_FILE="${FULLSEND_VALIDATED_ITERATION_DIR}/agent-result.json" +else + RESULT_FILE=$(find . -maxdepth 4 -path '*/iteration-*/output/agent-result.json' | sort -V | tail -1) +fi if [ -z "${RESULT_FILE}" ] || [ ! -f "${RESULT_FILE}" ]; then echo "::error::No agent-result.json found — posting failure notice" diff --git a/internal/scaffold/fullsend-repo/scripts/post-triage.sh b/internal/scaffold/fullsend-repo/scripts/post-triage.sh index 94cedb01b..1d183c175 100755 --- a/internal/scaffold/fullsend-repo/scripts/post-triage.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-triage.sh @@ -19,14 +19,20 @@ set -euo pipefail -# Find the triage result JSON. The run dir contains iteration-N/ subdirectories; -# we want the last one's output. -RESULT_FILE="" -for dir in iteration-*/output; do - if [[ -f "${dir}/agent-result.json" ]]; then - RESULT_FILE="${dir}/agent-result.json" - fi -done +# Find the triage result JSON. Prefer the validated iteration directory set by +# the harness (FULLSEND_VALIDATED_ITERATION_DIR) — without it, the last +# iteration's output may be schema-invalid content that failed validation. +# Fall back to scanning for the last iteration for backward compatibility. +if [[ -n "${FULLSEND_VALIDATED_ITERATION_DIR:-}" ]] && [[ -f "${FULLSEND_VALIDATED_ITERATION_DIR}/agent-result.json" ]]; then + RESULT_FILE="${FULLSEND_VALIDATED_ITERATION_DIR}/agent-result.json" +else + RESULT_FILE="" + for dir in iteration-*/output; do + if [[ -f "${dir}/agent-result.json" ]]; then + RESULT_FILE="${dir}/agent-result.json" + fi + done +fi if [[ -z "${RESULT_FILE}" ]]; then echo "ERROR: agent-result.json not found in any iteration output directory" >&2 From 9a138dab3a049c7e5905a919c1f78995b15ed572 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Thu, 23 Jul 2026 14:08:25 -0400 Subject: [PATCH 07/12] docs(runner): scope REPO_DIR fail-closed claim to post-fix.sh only The code comment and cli-internals.md diagram claimed post-scripts generally fail closed on empty REPO_DIR. In fact only post-fix.sh checks it; post-review.sh, post-triage.sh, post-retro.sh, and post-prioritize.sh don't reference REPO_DIR at all, and post-code.sh is unaffected since code.yaml has no validation_loop. Also documents a known limitation surfaced by review: since there is no per-iteration repo checkout, post-fix.sh cannot recover a sweep-validated non-final iteration's repo state and fails closed with "Extracted repo not found" instead of pushing. Assisted-by: Claude Signed-off-by: Wayne Sun --- docs/guides/dev/cli-internals.md | 23 +++++++++++++++-------- internal/cli/run.go | 26 +++++++++++++++++++------- 2 files changed, 34 insertions(+), 15 deletions(-) diff --git a/docs/guides/dev/cli-internals.md b/docs/guides/dev/cli-internals.md index a7c997bd4..f80b8ac4f 100644 --- a/docs/guides/dev/cli-internals.md +++ b/docs/guides/dev/cli-internals.md @@ -492,15 +492,22 @@ Vendoring commit messages use title + body (upload and stale delete). `github st │ ┌──────────────────┐ │ │ │ Post-script │ Run harness.post_script (host-side) │ │ │ │ REPO_DIR set only when last SafeDownload │ -│ │ │ succeeded and validated iteration is the │ -│ │ │ latest; empty otherwise (post-scripts │ -│ │ │ must fail closed on empty REPO_DIR) │ +│ │ │ succeeded and validated iteration is the │ +│ │ │ latest; empty otherwise. Only post- │ +│ │ │ fix.sh fails closed on empty REPO_DIR; │ +│ │ │ the other validation_loop post-scripts │ +│ │ │ don't reference REPO_DIR at all. There │ +│ │ │ is no per-iteration repo checkout, so │ +│ │ │ post-fix.sh cannot recover a sweep- │ +│ │ │ validated non-final iteration; it fails │ +│ │ │ closed instead of pushing (known │ +│ │ │ limitation, see #5393). │ │ │ │ │ -│ │ │ FULLSEND_VALIDATED_ITERATION_DIR points │ -│ │ │ to the validated iteration's output dir. │ -│ │ │ Post-scripts must use this (when set) to │ -│ │ │ select the result file instead of blindly │ -│ │ │ taking the last iteration. │ +│ │ │ FULLSEND_VALIDATED_ITERATION_DIR points │ +│ │ │ to the validated iteration's output dir. │ +│ │ │ Post-scripts must use this (when set) to │ +│ │ │ select the result file instead of │ +│ │ │ blindly taking the last iteration. │ │ └──────┬───────────┘ │ │ ▼ │ │ ┌──────────────────┐ │ diff --git a/internal/cli/run.go b/internal/cli/run.go index 33c53df4d..31296268a 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -963,13 +963,25 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep // last-value-wins so this append takes precedence. TODO(fullsend-ai/agents#191): // remove REPO_DIR from RunnerEnv entirely once harnesses no longer set it. // - // Pass REPO_DIR only when the last repo extraction succeeded. - // If SafeDownload failed, hostRepositoryDownloadDir was cleaned - // up and may not exist — passing it would expose the post-script - // to unsanitized or missing content. Post-scripts must fail - // closed on empty REPO_DIR (the scaffolded post-code.sh and - // post-fix.sh already do via ${REPO_DIR:-repo} + directory - // existence check). + // Pass REPO_DIR only when repoExtractedOK is true: the last + // SafeDownload succeeded AND corresponds to the validated + // iteration (repoExtractedOK is forced false by the sweep when + // it validates an earlier iteration — see its doc comment). + // Passing a stale or missing dir would expose the post-script + // to unsanitized or wrong-iteration content. + // + // Only post-fix.sh's scaffolded post-script fails closed on an + // empty REPO_DIR (via ${REPO_DIR:-repo} + directory existence + // check) — it needs actual repo content to push. The other + // validation_loop post-scripts (post-review.sh, post-triage.sh, + // post-retro.sh, post-prioritize.sh) don't reference REPO_DIR at + // all; they rely on FULLSEND_VALIDATED_ITERATION_DIR (set below) + // to select the correct iteration's output. post-code.sh is + // unaffected because code.yaml has no validation_loop. Because + // there is no per-iteration repo checkout, post-fix.sh cannot + // recover a sweep-validated non-final iteration's repo state — + // it fails closed with "Extracted repo not found" instead of + // pushing. See #5393 follow-up. postRepoDir := "" if repoExtractedOK { postRepoDir = hostRepositoryDownloadDir From 86097a1883a17919918c63122e3b2d570a377b11 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Thu, 23 Jul 2026 14:23:58 -0400 Subject: [PATCH 08/12] fix(runner): correct REPO_DIR claim, avoid unverified pre-clear assumption MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review squad follow-up on PR #5395 found two issues in the previous commit's own changes: - The claim "only post-fix.sh fails closed on empty REPO_DIR" was itself inaccurate: post-code.sh has the identical fail-closed check in its own script logic. It just can't currently observe an empty REPO_DIR because code.yaml has no validation_loop, so the extraction failure path that clears REPO_DIR is unreachable for it today — but the check is real, not dead code, and would activate the moment code.yaml gained a validation_loop. - The forceRemoveAll pre-clear was downgraded to non-fatal based on an unverified assumption that SafeDownload's Download step (an external "openshell sandbox download" call) fully replaces a stale, non-empty destination directory rather than merging onto it. Nothing in this codebase confirms that behavior. Treat a failed pre-clear the same as a SafeDownload failure instead: skip the iteration's repo state rather than extracting into a directory of unknown provenance. Assisted-by: Claude Signed-off-by: Wayne Sun --- docs/guides/dev/cli-internals.md | 22 ++++++++------- internal/cli/run.go | 46 ++++++++++++++++++++------------ 2 files changed, 42 insertions(+), 26 deletions(-) diff --git a/docs/guides/dev/cli-internals.md b/docs/guides/dev/cli-internals.md index f80b8ac4f..ec294e14b 100644 --- a/docs/guides/dev/cli-internals.md +++ b/docs/guides/dev/cli-internals.md @@ -493,15 +493,19 @@ Vendoring commit messages use title + body (upload and stale delete). `github st │ │ Post-script │ Run harness.post_script (host-side) │ │ │ │ REPO_DIR set only when last SafeDownload │ │ │ │ succeeded and validated iteration is the │ -│ │ │ latest; empty otherwise. Only post- │ -│ │ │ fix.sh fails closed on empty REPO_DIR; │ -│ │ │ the other validation_loop post-scripts │ -│ │ │ don't reference REPO_DIR at all. There │ -│ │ │ is no per-iteration repo checkout, so │ -│ │ │ post-fix.sh cannot recover a sweep- │ -│ │ │ validated non-final iteration; it fails │ -│ │ │ closed instead of pushing (known │ -│ │ │ limitation, see #5393). │ +│ │ │ latest; empty otherwise. post-fix.sh and │ +│ │ │ post-code.sh both fail closed on empty │ +│ │ │ REPO_DIR in their own script logic; the │ +│ │ │ other validation_loop post-scripts don't │ +│ │ │ reference REPO_DIR at all. code.yaml has │ +│ │ │ no validation_loop, so post-code.sh │ +│ │ │ can't currently hit this path, but the │ +│ │ │ check is real, not dead code. There is │ +│ │ │ no per-iteration repo checkout, so post- │ +│ │ │ fix.sh cannot recover a sweep-validated │ +│ │ │ non-final iteration; it fails closed │ +│ │ │ instead of pushing (known limitation, │ +│ │ │ see #5393). │ │ │ │ │ │ │ │ FULLSEND_VALIDATED_ITERATION_DIR points │ │ │ │ to the validated iteration's output dir. │ diff --git a/internal/cli/run.go b/internal/cli/run.go index 31296268a..3b126cdb3 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -970,18 +970,23 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep // Passing a stale or missing dir would expose the post-script // to unsanitized or wrong-iteration content. // - // Only post-fix.sh's scaffolded post-script fails closed on an - // empty REPO_DIR (via ${REPO_DIR:-repo} + directory existence - // check) — it needs actual repo content to push. The other - // validation_loop post-scripts (post-review.sh, post-triage.sh, - // post-retro.sh, post-prioritize.sh) don't reference REPO_DIR at - // all; they rely on FULLSEND_VALIDATED_ITERATION_DIR (set below) - // to select the correct iteration's output. post-code.sh is - // unaffected because code.yaml has no validation_loop. Because - // there is no per-iteration repo checkout, post-fix.sh cannot - // recover a sweep-validated non-final iteration's repo state — - // it fails closed with "Extracted repo not found" instead of - // pushing. See #5393 follow-up. + // post-fix.sh and post-code.sh both fail closed on an empty + // REPO_DIR in their own script logic (via ${REPO_DIR:-repo} + + // directory existence check) — both need actual repo content to + // push. The other validation_loop post-scripts (post-review.sh, + // post-triage.sh, post-retro.sh, post-prioritize.sh) don't + // reference REPO_DIR at all; they rely on + // FULLSEND_VALIDATED_ITERATION_DIR (set below) to select the + // correct iteration's output. code.yaml has no validation_loop, + // so post-code.sh cannot currently observe an empty REPO_DIR in + // practice — a SafeDownload failure is fatal for it and this + // defer never runs — but the check in its script is real, not a + // dead branch, and would activate the moment code.yaml gained a + // validation_loop. Because there is no per-iteration repo + // checkout, post-fix.sh (and post-code.sh, were it to gain a + // validation_loop) cannot recover a sweep-validated non-final + // iteration's repo state — it fails closed with "Extracted repo + // not found" instead of pushing. See #5393 follow-up. postRepoDir := "" if repoExtractedOK { postRepoDir = hostRepositoryDownloadDir @@ -1467,14 +1472,21 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep // always treated as fatal for the current iteration's repo state — // we clean up the directory and skip to the next iteration. // - // The forceRemoveAll pre-clear is a narrower staleness concern and - // remains non-fatal with a validation loop. + // The forceRemoveAll pre-clear guards against a stale destination: + // whether "openshell sandbox download" fully replaces a non-empty + // destination directory or merges onto it is not verified anywhere + // in this codebase (it's an external binary). Rather than assume + // replace semantics, a failed pre-clear is treated the same as a + // SafeDownload failure with a validation loop: skip this + // iteration's repo state instead of extracting into a directory of + // unknown provenance. if clearErr := forceRemoveAll(hostRepositoryDownloadDir); clearErr != nil { if h.ValidationLoop != nil { - printer.StepWarn(fmt.Sprintf("Failed to clear local repo %s: %v", hostRepositoryDownloadDir, clearErr)) - } else { - return fmt.Errorf("clearing local repo %s before extraction: %w", hostRepositoryDownloadDir, clearErr) + printer.StepWarn(fmt.Sprintf("Failed to clear local repo %s (skipping repo extraction this iteration): %v", hostRepositoryDownloadDir, clearErr)) + repoExtractedOK = false + continue } + return fmt.Errorf("clearing local repo %s before extraction: %w", hostRepositoryDownloadDir, clearErr) } repoExtractStart := time.Now() From e3d3d04b950e00facf4f7be6a5955bf38f1b0a4d Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Thu, 23 Jul 2026 14:24:06 -0400 Subject: [PATCH 09/12] fix(scaffold): close fail-open gap in FULLSEND_VALIDATED_ITERATION_DIR lookup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prefer-validated-dir logic added in 600df8d falls through to a full rescan of iteration-*/output whenever the expected filename is missing at FULLSEND_VALIDATED_ITERATION_DIR — not just when the env var itself is unset. validate-output-schema.sh accepts "result.json" as a fallback filename during validation without renaming it to agent-result.json (or fix-result.json for post-fix.sh), so a validated iteration that used that filename would fail the presence check and silently fall back to scanning for the last iteration directory — which could be a later, schema-invalid iteration. This reintroduces exactly the class of bug #5393/#5395 fixed for the shipped scaffolded scripts. Check the same result.json fallback at the validated directory instead of falling through to the rescan, and tighten the downstream emptiness checks in post-triage.sh/post-retro.sh/post-prioritize.sh to also verify file existence so a missing result still fails closed with a clear error rather than reaching jq on a nonexistent path. Also updates the building-custom-agents.md example post-script, which still showed the pre-#5393 "always rescan" pattern. Assisted-by: Claude Signed-off-by: Wayne Sun --- docs/guides/user/building-custom-agents.md | 24 ++++++++++++------- .../fullsend-repo/scripts/post-fix.sh | 11 ++++++++- .../fullsend-repo/scripts/post-prioritize.sh | 13 ++++++++-- .../fullsend-repo/scripts/post-retro.sh | 13 ++++++++-- .../fullsend-repo/scripts/post-review.sh | 11 ++++++++- .../fullsend-repo/scripts/post-triage.sh | 13 ++++++++-- 6 files changed, 69 insertions(+), 16 deletions(-) diff --git a/docs/guides/user/building-custom-agents.md b/docs/guides/user/building-custom-agents.md index 7411dafb8..915969847 100644 --- a/docs/guides/user/building-custom-agents.md +++ b/docs/guides/user/building-custom-agents.md @@ -322,14 +322,22 @@ The pre-script has full credentials on the trusted runner. It fetches data from #!/usr/bin/env bash set -euo pipefail -RESULT_FILE="" -for dir in iteration-*/output; do - if [[ -f "${dir}/agent-result.json" ]]; then - RESULT_FILE="${dir}/agent-result.json" - fi -done - -if [[ -z "${RESULT_FILE}" ]]; then +# Prefer the validated iteration directory set by the harness +# (FULLSEND_VALIDATED_ITERATION_DIR) — without it, scanning for the last +# iteration can pick up output that failed validation. Fall back to +# scanning for the last iteration for harnesses with no validation_loop. +if [[ -n "${FULLSEND_VALIDATED_ITERATION_DIR:-}" ]]; then + RESULT_FILE="${FULLSEND_VALIDATED_ITERATION_DIR}/agent-result.json" +else + RESULT_FILE="" + for dir in iteration-*/output; do + if [[ -f "${dir}/agent-result.json" ]]; then + RESULT_FILE="${dir}/agent-result.json" + fi + done +fi + +if [[ -z "${RESULT_FILE}" ]] || [[ ! -f "${RESULT_FILE}" ]]; then echo "ERROR: agent-result.json not found" exit 1 fi diff --git a/internal/scaffold/fullsend-repo/scripts/post-fix.sh b/internal/scaffold/fullsend-repo/scripts/post-fix.sh index f2ca6d9a0..132929ad9 100644 --- a/internal/scaffold/fullsend-repo/scripts/post-fix.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-fix.sh @@ -363,8 +363,17 @@ fi # (FULLSEND_VALIDATED_ITERATION_DIR) — without it, the last iteration's output # may be schema-invalid content that failed validation. Fall back to scanning # for the last iteration for backward compatibility. -if [ -n "${FULLSEND_VALIDATED_ITERATION_DIR:-}" ] && [ -f "${FULLSEND_VALIDATED_ITERATION_DIR}/fix-result.json" ]; then +if [ -n "${FULLSEND_VALIDATED_ITERATION_DIR:-}" ]; then RESULT_FILE="${FULLSEND_VALIDATED_ITERATION_DIR}/fix-result.json" + if [ ! -f "${RESULT_FILE}" ]; then + # Agents sometimes write "result.json" instead of the harness-configured + # "fix-result.json"; validate-output-schema.sh accepts that filename as + # a fallback without renaming it. Check the same variant here rather + # than falling through to the rescan below, which could silently pick + # up a different (and possibly schema-invalid) iteration's output. + fallback_result="${FULLSEND_VALIDATED_ITERATION_DIR}/result.json" + [ -f "${fallback_result}" ] && RESULT_FILE="${fallback_result}" + fi else RESULT_FILE="" for dir in "${RUN_DIR}"/iteration-*/output; do diff --git a/internal/scaffold/fullsend-repo/scripts/post-prioritize.sh b/internal/scaffold/fullsend-repo/scripts/post-prioritize.sh index bb55a37b6..873c62f46 100755 --- a/internal/scaffold/fullsend-repo/scripts/post-prioritize.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-prioritize.sh @@ -343,8 +343,17 @@ fi # harness (FULLSEND_VALIDATED_ITERATION_DIR) — without it, the last iteration's # output may be schema-invalid content that failed validation. Fall back to # scanning for the last iteration for backward compatibility. -if [[ -n "${FULLSEND_VALIDATED_ITERATION_DIR:-}" ]] && [[ -f "${FULLSEND_VALIDATED_ITERATION_DIR}/agent-result.json" ]]; then +if [[ -n "${FULLSEND_VALIDATED_ITERATION_DIR:-}" ]]; then RESULT_FILE="${FULLSEND_VALIDATED_ITERATION_DIR}/agent-result.json" + if [[ ! -f "${RESULT_FILE}" ]]; then + # Agents sometimes write "result.json" instead of "agent-result.json"; + # validate-output-schema.sh accepts that filename as a fallback without + # renaming it. Check the same variant here rather than falling through + # to the rescan below, which could silently pick up a different (and + # possibly schema-invalid) iteration's output. + fallback_result="${FULLSEND_VALIDATED_ITERATION_DIR}/result.json" + [[ -f "${fallback_result}" ]] && RESULT_FILE="${fallback_result}" + fi else RESULT_FILE="" for dir in iteration-*/output; do @@ -354,7 +363,7 @@ else done fi -if [[ -z "${RESULT_FILE}" ]]; then +if [[ -z "${RESULT_FILE}" ]] || [[ ! -f "${RESULT_FILE}" ]]; then echo "ERROR: agent-result.json not found in any iteration output directory" >&2 exit 1 fi diff --git a/internal/scaffold/fullsend-repo/scripts/post-retro.sh b/internal/scaffold/fullsend-repo/scripts/post-retro.sh index 43543f91a..40bb83134 100755 --- a/internal/scaffold/fullsend-repo/scripts/post-retro.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-retro.sh @@ -21,8 +21,17 @@ echo "::add-mask::${GH_TOKEN}" # the harness (FULLSEND_VALIDATED_ITERATION_DIR) — without it, the last # iteration's output may be schema-invalid content that failed validation. # Fall back to scanning for the last iteration for backward compatibility. -if [[ -n "${FULLSEND_VALIDATED_ITERATION_DIR:-}" ]] && [[ -f "${FULLSEND_VALIDATED_ITERATION_DIR}/agent-result.json" ]]; then +if [[ -n "${FULLSEND_VALIDATED_ITERATION_DIR:-}" ]]; then RESULT_FILE="${FULLSEND_VALIDATED_ITERATION_DIR}/agent-result.json" + if [[ ! -f "${RESULT_FILE}" ]]; then + # Agents sometimes write "result.json" instead of "agent-result.json"; + # validate-output-schema.sh accepts that filename as a fallback without + # renaming it. Check the same variant here rather than falling through + # to the rescan below, which could silently pick up a different (and + # possibly schema-invalid) iteration's output. + fallback_result="${FULLSEND_VALIDATED_ITERATION_DIR}/result.json" + [[ -f "${fallback_result}" ]] && RESULT_FILE="${fallback_result}" + fi else RESULT_FILE="" for dir in iteration-*/output; do @@ -32,7 +41,7 @@ else done fi -if [[ -z "${RESULT_FILE}" ]]; then +if [[ -z "${RESULT_FILE}" ]] || [[ ! -f "${RESULT_FILE}" ]]; then echo "ERROR: agent-result.json not found in any iteration output directory" >&2 exit 1 fi diff --git a/internal/scaffold/fullsend-repo/scripts/post-review.sh b/internal/scaffold/fullsend-repo/scripts/post-review.sh index a5eaa976a..049a9a980 100755 --- a/internal/scaffold/fullsend-repo/scripts/post-review.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-review.sh @@ -60,8 +60,17 @@ fi # harness (FULLSEND_VALIDATED_ITERATION_DIR) — without it, the last iteration's # output may be schema-invalid content that failed validation. Fall back to the # last iteration for backward compatibility with older fullsend versions. -if [ -n "${FULLSEND_VALIDATED_ITERATION_DIR:-}" ] && [ -f "${FULLSEND_VALIDATED_ITERATION_DIR}/agent-result.json" ]; then +if [ -n "${FULLSEND_VALIDATED_ITERATION_DIR:-}" ]; then RESULT_FILE="${FULLSEND_VALIDATED_ITERATION_DIR}/agent-result.json" + if [ ! -f "${RESULT_FILE}" ]; then + # Agents sometimes write "result.json" instead of "agent-result.json"; + # validate-output-schema.sh accepts that filename as a fallback without + # renaming it. Check the same variant here rather than falling through + # to the rescan below, which could silently pick up a different (and + # possibly schema-invalid) iteration's output. + fallback_result="${FULLSEND_VALIDATED_ITERATION_DIR}/result.json" + [ -f "${fallback_result}" ] && RESULT_FILE="${fallback_result}" + fi else RESULT_FILE=$(find . -maxdepth 4 -path '*/iteration-*/output/agent-result.json' | sort -V | tail -1) fi diff --git a/internal/scaffold/fullsend-repo/scripts/post-triage.sh b/internal/scaffold/fullsend-repo/scripts/post-triage.sh index 1d183c175..bfa5933cb 100755 --- a/internal/scaffold/fullsend-repo/scripts/post-triage.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-triage.sh @@ -23,8 +23,17 @@ set -euo pipefail # the harness (FULLSEND_VALIDATED_ITERATION_DIR) — without it, the last # iteration's output may be schema-invalid content that failed validation. # Fall back to scanning for the last iteration for backward compatibility. -if [[ -n "${FULLSEND_VALIDATED_ITERATION_DIR:-}" ]] && [[ -f "${FULLSEND_VALIDATED_ITERATION_DIR}/agent-result.json" ]]; then +if [[ -n "${FULLSEND_VALIDATED_ITERATION_DIR:-}" ]]; then RESULT_FILE="${FULLSEND_VALIDATED_ITERATION_DIR}/agent-result.json" + if [[ ! -f "${RESULT_FILE}" ]]; then + # Agents sometimes write "result.json" instead of "agent-result.json"; + # validate-output-schema.sh accepts that filename as a fallback without + # renaming it. Check the same variant here rather than falling through + # to the rescan below, which could silently pick up a different (and + # possibly schema-invalid) iteration's output. + fallback_result="${FULLSEND_VALIDATED_ITERATION_DIR}/result.json" + [[ -f "${fallback_result}" ]] && RESULT_FILE="${fallback_result}" + fi else RESULT_FILE="" for dir in iteration-*/output; do @@ -34,7 +43,7 @@ else done fi -if [[ -z "${RESULT_FILE}" ]]; then +if [[ -z "${RESULT_FILE}" ]] || [[ ! -f "${RESULT_FILE}" ]]; then echo "ERROR: agent-result.json not found in any iteration output directory" >&2 exit 1 fi From c3ae795a4e006352ec54aa33b828383a3dc5f551 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Thu, 23 Jul 2026 14:57:22 -0400 Subject: [PATCH 10/12] test(runner,scaffold): cover FULLSEND_VALIDATED_ITERATION_DIR fallback Adds the test coverage flagged as missing during review-squad follow-up on PR #5395: neither the Go-side REPO_DIR/FULLSEND_VALIDATED_ITERATION_DIR env construction nor the shell-side prefer/fallback logic in the five post-scripts had direct tests, which is how the fail-open regression fixed in 3aa1d711 shipped unnoticed. - Extract the post-script env-var construction out of the defer closure into postScriptRepoEnv (internal/cli/run.go), and add TestPostScriptRepoEnv covering all four repoExtractedOK/validatedIterNum/ ValidationLoop combinations. - Add FULLSEND_VALIDATED_ITERATION_DIR fallback tests to post-review-test.sh, post-triage-test.sh, post-retro-test.sh, and post-prioritize-test.sh, invoking the real scripts with mocked gh/fullsend binaries: validated dir with the expected filename, validated dir with the result.json fallback, and validated dir with neither file (must fail closed, never silently use a later iteration). - post-fix.sh's test harness reimplements logic in isolation rather than invoking the real script (it needs git/gitleaks/python3), so add select_fix_result_file as a matching reimplementation with the same three cases plus a legacy no-env-var rescan sanity check. Assisted-by: Claude Signed-off-by: Wayne Sun --- internal/cli/run.go | 31 ++++-- internal/cli/run_test.go | 57 ++++++++++ .../fullsend-repo/scripts/post-fix-test.sh | 102 ++++++++++++++++++ .../scripts/post-prioritize-test.sh | 71 ++++++++++++ .../fullsend-repo/scripts/post-retro-test.sh | 69 ++++++++++++ .../fullsend-repo/scripts/post-review-test.sh | 74 +++++++++++++ .../fullsend-repo/scripts/post-triage-test.sh | 69 ++++++++++++ 7 files changed, 466 insertions(+), 7 deletions(-) diff --git a/internal/cli/run.go b/internal/cli/run.go index 3b126cdb3..142a1d610 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -987,10 +987,7 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep // validation_loop) cannot recover a sweep-validated non-final // iteration's repo state — it fails closed with "Extracted repo // not found" instead of pushing. See #5393 follow-up. - postRepoDir := "" - if repoExtractedOK { - postRepoDir = hostRepositoryDownloadDir - } + postRepoDir, postValidatedIterDir := postScriptRepoEnv(h, runDir, hostRepositoryDownloadDir, repoExtractedOK, validatedIterNum) postCmd.Env = append(postCmd.Env, fmt.Sprintf("REPO_DIR=%s", postRepoDir)) // FULLSEND_VALIDATED_ITERATION_DIR tells the post-script which // iteration's output was validated. Without this, post-scripts @@ -999,9 +996,8 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep // iteration. Empty when no validation loop is configured or // when no iteration passed validation (the post-script is // skipped in the latter case, so this is defensive). - if h.ValidationLoop != nil && validatedIterNum > 0 { - postCmd.Env = append(postCmd.Env, fmt.Sprintf("FULLSEND_VALIDATED_ITERATION_DIR=%s", - filepath.Join(runDir, fmt.Sprintf("iteration-%d/output", validatedIterNum)))) + if postValidatedIterDir != "" { + postCmd.Env = append(postCmd.Env, fmt.Sprintf("FULLSEND_VALIDATED_ITERATION_DIR=%s", postValidatedIterDir)) } postCmd.Stdout = os.Stdout postCmd.Stderr = os.Stderr @@ -2028,6 +2024,27 @@ func validationFailMessage(output []byte, execErr error) string { return execErr.Error() } +// postScriptRepoEnv computes the REPO_DIR and FULLSEND_VALIDATED_ITERATION_DIR +// values for the post-script's environment. Extracted from the post-script +// defer closure for testability. +// +// repoDir is hostRepositoryDownloadDir when repoExtractedOK is true, empty +// otherwise — see the call site's doc comment for why repoExtractedOK can be +// false. validatedIterDir points at the validated iteration's output +// directory when a validation loop is configured and an iteration passed; +// it is empty when there's no validation loop (the post-script's own +// last-iteration scan is used instead) or when no iteration passed (the +// post-script is skipped entirely in that case, so this is defensive). +func postScriptRepoEnv(h *harness.Harness, runDir, hostRepositoryDownloadDir string, repoExtractedOK bool, validatedIterNum int) (repoDir, validatedIterDir string) { + if repoExtractedOK { + repoDir = hostRepositoryDownloadDir + } + if h.ValidationLoop != nil && validatedIterNum > 0 { + validatedIterDir = filepath.Join(runDir, fmt.Sprintf("iteration-%d/output", validatedIterNum)) + } + return repoDir, validatedIterDir +} + // sweepResult holds the outcome of a post-loop validation sweep. type sweepResult struct { passed bool // true if any iteration's validation passed diff --git a/internal/cli/run_test.go b/internal/cli/run_test.go index 4a02aa072..97218c664 100644 --- a/internal/cli/run_test.go +++ b/internal/cli/run_test.go @@ -2350,6 +2350,63 @@ func TestSweepResult_RepoExtractedOK_PreservesWhenRunCount(t *testing.T) { "repoExtractedOK should remain true when input was true and i == runCount") } +func TestPostScriptRepoEnv(t *testing.T) { + runDir := "/run" + hostRepoDir := "/host/repo" + withLoop := &harness.Harness{ValidationLoop: &harness.ValidationLoop{Script: "validate.sh"}} + noLoop := &harness.Harness{} + + tests := []struct { + name string + h *harness.Harness + repoExtractedOK bool + validatedIterNum int + wantRepoDir string + wantIterDir string + }{ + { + name: "extraction succeeded and validated iteration is latest", + h: withLoop, + repoExtractedOK: true, + validatedIterNum: 2, + wantRepoDir: hostRepoDir, + wantIterDir: filepath.Join(runDir, "iteration-2/output"), + }, + { + name: "extraction failed: REPO_DIR empty regardless of validated iteration", + h: withLoop, + repoExtractedOK: false, + validatedIterNum: 0, + wantRepoDir: "", + wantIterDir: "", + }, + { + name: "sweep validated an earlier iteration: REPO_DIR empty, iteration dir still set", + h: withLoop, + repoExtractedOK: false, + validatedIterNum: 1, + wantRepoDir: "", + wantIterDir: filepath.Join(runDir, "iteration-1/output"), + }, + { + name: "no validation loop: iteration dir never set even if validatedIterNum leaked", + h: noLoop, + repoExtractedOK: true, + validatedIterNum: 3, + wantRepoDir: hostRepoDir, + wantIterDir: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + repoDir, iterDir := postScriptRepoEnv(tt.h, runDir, hostRepoDir, tt.repoExtractedOK, tt.validatedIterNum) + assert.Equal(t, tt.wantRepoDir, repoDir, "REPO_DIR") + assert.Equal(t, tt.wantIterDir, iterDir, "FULLSEND_VALIDATED_ITERATION_DIR") + }) + } +} + func TestOpenTeeReader_EmptyPath(t *testing.T) { src := strings.NewReader("hello") printer := ui.New(io.Discard) diff --git a/internal/scaffold/fullsend-repo/scripts/post-fix-test.sh b/internal/scaffold/fullsend-repo/scripts/post-fix-test.sh index 687a2fa2f..1850b8911 100644 --- a/internal/scaffold/fullsend-repo/scripts/post-fix-test.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-fix-test.sh @@ -157,6 +157,108 @@ run_precommit_retry_test "precommit-passes-with-unstaged" \ run_precommit_retry_test "precommit-retry-passes-but-left-unstaged" \ "1" "yes" "0" "blocked:retry-left-unstaged" "yes" +# --------------------------------------------------------------------------- +# Test helper — reimplements the RESULT_FILE selection logic from post-fix.sh +# (the FULLSEND_VALIDATED_ITERATION_DIR / fix-result.json / result.json +# fallback / rescan block) so we can test it without git, gitleaks, or a +# real PR. Verifies the fix for a fail-open regression (#5395 follow-up): +# the prefer-validated-dir logic must not silently fall back to rescanning +# RUN_DIR/iteration-*/output for fix-result.json when +# FULLSEND_VALIDATED_ITERATION_DIR is set but the expected filename is +# missing there (e.g. the agent wrote "result.json", which +# validate-output-schema.sh accepts as a fallback without renaming). Falling +# back to a rescan would let a later, unvalidated iteration's output win — +# exactly the bug #5393/#5395 fixed. +# --------------------------------------------------------------------------- +select_fix_result_file() { + local run_dir="$1" + local validated_dir="${2:-}" + + local result_file + if [ -n "${validated_dir}" ]; then + result_file="${validated_dir}/fix-result.json" + if [ ! -f "${result_file}" ]; then + local fallback_result="${validated_dir}/result.json" + [ -f "${fallback_result}" ] && result_file="${fallback_result}" + fi + else + result_file="" + for dir in "${run_dir}"/iteration-*/output; do + if [ -f "${dir}/fix-result.json" ]; then + result_file="${dir}/fix-result.json" + fi + done + fi + echo "${result_file}" +} + +run_select_fix_result_test() { + local test_name="$1" + local validated_filename="$2" # "" to leave the validated dir empty + local expect_wrong_iteration="$3" # "true" if a wrong-iteration fallback is acceptable (no FULLSEND_VALIDATED_ITERATION_DIR) + + local run_dir + run_dir="$(mktemp -d)" + mkdir -p "${run_dir}/iteration-1/output" + if [ -n "${validated_filename}" ]; then + echo '{"marker":"validated"}' > "${run_dir}/iteration-1/output/${validated_filename}" + fi + # A later iteration — must never be selected when FULLSEND_VALIDATED_ITERATION_DIR + # is set, whether or not the validated dir's file is found. + mkdir -p "${run_dir}/iteration-2/output" + echo '{"marker":"wrong-iteration"}' > "${run_dir}/iteration-2/output/fix-result.json" + + local result_file + result_file="$(select_fix_result_file "${run_dir}" "${run_dir}/iteration-1/output")" + + rm -rf "${run_dir}" + + if [ "${expect_wrong_iteration}" = "true" ]; then + if [ "${result_file}" != "${run_dir}/iteration-1/output/${validated_filename}" ]; then + echo "FAIL: ${test_name} — expected '${run_dir}/iteration-1/output/${validated_filename}', got '${result_file}'" + FAILURES=$((FAILURES + 1)) + return + fi + echo "PASS: ${test_name}" + return + fi + + # Missing-file case: must fail closed (empty result), never silently + # resolve to iteration-2's fix-result.json. + if [ "${result_file}" = "${run_dir}/iteration-2/output/fix-result.json" ]; then + echo "FAIL: ${test_name} — silently fell back to iteration-2 instead of failing closed" + FAILURES=$((FAILURES + 1)) + return + fi + echo "PASS: ${test_name} (result_file='${result_file}', did not use wrong iteration)" +} + +run_select_fix_result_test "validated-dir-fix-result-json-used" "fix-result.json" "true" +run_select_fix_result_test "validated-dir-result-json-fallback-used" "result.json" "true" +run_select_fix_result_test "validated-dir-missing-file-fails-closed" "" "false" + +# Sanity check: with no FULLSEND_VALIDATED_ITERATION_DIR (legacy behavior), +# the rescan picks the last iteration by glob order. +run_test_rescan_last_iteration() { + local run_dir + run_dir="$(mktemp -d)" + mkdir -p "${run_dir}/iteration-1/output" "${run_dir}/iteration-2/output" + echo '{"marker":"iter1"}' > "${run_dir}/iteration-1/output/fix-result.json" + echo '{"marker":"iter2"}' > "${run_dir}/iteration-2/output/fix-result.json" + + local result_file + result_file="$(select_fix_result_file "${run_dir}" "")" + rm -rf "${run_dir}" + + if [ "${result_file}" != "${run_dir}/iteration-2/output/fix-result.json" ]; then + echo "FAIL: rescan-picks-last-iteration — expected iteration-2, got '${result_file}'" + FAILURES=$((FAILURES + 1)) + return + fi + echo "PASS: rescan-picks-last-iteration" +} +run_test_rescan_last_iteration + # --- Summary --- echo "" diff --git a/internal/scaffold/fullsend-repo/scripts/post-prioritize-test.sh b/internal/scaffold/fullsend-repo/scripts/post-prioritize-test.sh index 48f980250..268ba70f8 100755 --- a/internal/scaffold/fullsend-repo/scripts/post-prioritize-test.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-prioritize-test.sh @@ -630,6 +630,77 @@ export GITHUB_CSMA_MAX_ATTEMPTS=3 run_test_failure_stderr "exit0-rate-limit-exhausted" "100" "rate limit exceeded" "exit0-ratelimit" unset GITHUB_CSMA_MAX_ATTEMPTS +# --- FULLSEND_VALIDATED_ITERATION_DIR selection tests (#5395 follow-up) --- +# +# These verify the fix for a fail-open regression: the prefer-validated-dir +# logic must not silently fall back to rescanning iteration-*/output for +# agent-result.json when FULLSEND_VALIDATED_ITERATION_DIR is set but the +# expected filename is missing there (e.g. the agent wrote "result.json", +# which validate-output-schema.sh accepts as a fallback without renaming). +# Falling back to a rescan would let a later, unvalidated iteration's output +# win — exactly the bug #5393/#5395 fixed. + +WRONG_ITERATION_JSON='{"reach":99,"impact":99,"confidence":0.99,"effort":99,"reasoning":{"reach":"WRONG_ITERATION_MARKER","impact":"x","confidence":"x","effort":"x"}}' + +run_validated_iter_test() { + local test_name="$1" + local validated_filename="$2" # "" to leave the validated dir empty + local expect_success="$3" + + local run_dir="${TEST_TMPDIR}/run-${test_name}" + mkdir -p "${run_dir}/iteration-1/output" + if [[ -n "${validated_filename}" ]]; then + echo "${FIXTURE_JSON}" > "${run_dir}/iteration-1/output/${validated_filename}" + fi + # A later iteration with different, distinguishable content — must never + # be used when FULLSEND_VALIDATED_ITERATION_DIR is set, whether or not + # the validated dir's file is found. + mkdir -p "${run_dir}/iteration-2/output" + echo "${WRONG_ITERATION_JSON}" > "${run_dir}/iteration-2/output/agent-result.json" + + : > "${GH_LOG}" + rm -f "${GH_FAIL_COUNT}" + unset GH_CSMA_FAIL_UNTIL GH_CSMA_FAIL_MODE + + local exit_code=0 + ( + cd "${run_dir}" + export FULLSEND_VALIDATED_ITERATION_DIR="${run_dir}/iteration-1/output" + bash "${POST_SCRIPT}" + ) > "${TEST_TMPDIR}/stdout-${test_name}.log" 2>&1 || exit_code=$? + + if [[ "${expect_success}" == "true" ]]; then + if [[ ${exit_code} -ne 0 ]]; then + echo "FAIL: ${test_name} — exit code ${exit_code}" + cat "${TEST_TMPDIR}/stdout-${test_name}.log" + FAILURES=$((FAILURES + 1)) + return + fi + if grep -qF "WRONG_ITERATION_MARKER" "${GH_LOG}"; then + echo "FAIL: ${test_name} — used iteration-2's output instead of the validated iteration" + cat "${GH_LOG}" + FAILURES=$((FAILURES + 1)) + return + fi + echo "PASS: ${test_name}" + return + fi + + # expect_success == false: must fail closed, and must NOT have silently + # fallen back to iteration-2's output. + if [[ ${exit_code} -eq 0 ]] && grep -qF "WRONG_ITERATION_MARKER" "${GH_LOG}"; then + echo "FAIL: ${test_name} — silently fell back to iteration-2 instead of failing closed" + cat "${GH_LOG}" + FAILURES=$((FAILURES + 1)) + return + fi + echo "PASS: ${test_name} (exit code ${exit_code}, did not use wrong iteration)" +} + +run_validated_iter_test "validated-dir-agent-result-json-used" "agent-result.json" "true" +run_validated_iter_test "validated-dir-result-json-fallback-used" "result.json" "true" +run_validated_iter_test "validated-dir-missing-file-fails-closed" "" "false" + if [[ ${FAILURES} -gt 0 ]]; then echo "" echo "${FAILURES} test(s) failed." diff --git a/internal/scaffold/fullsend-repo/scripts/post-retro-test.sh b/internal/scaffold/fullsend-repo/scripts/post-retro-test.sh index 40f6f94ce..c8cfd4394 100644 --- a/internal/scaffold/fullsend-repo/scripts/post-retro-test.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-retro-test.sh @@ -274,6 +274,75 @@ run_test_stdout "complete-message" \ "${FIXTURE_ONE_PROPOSAL}" \ "Post-retro complete." +# --- FULLSEND_VALIDATED_ITERATION_DIR selection tests (#5395 follow-up) --- +# +# These verify the fix for a fail-open regression: the prefer-validated-dir +# logic must not silently fall back to rescanning iteration-*/output for +# agent-result.json when FULLSEND_VALIDATED_ITERATION_DIR is set but the +# expected filename is missing there (e.g. the agent wrote "result.json", +# which validate-output-schema.sh accepts as a fallback without renaming). +# Falling back to a rescan would let a later, unvalidated iteration's output +# win — exactly the bug #5393/#5395 fixed. + +WRONG_ITERATION_JSON='{"summary":"WRONG_ITERATION_MARKER","proposals":[]}' + +run_validated_iter_test() { + local test_name="$1" + local validated_filename="$2" # "" to leave the validated dir empty + local expect_success="$3" + + local run_dir="${TMPDIR}/run-${test_name}" + mkdir -p "${run_dir}/iteration-1/output" + if [[ -n "${validated_filename}" ]]; then + echo "${FIXTURE_ONE_PROPOSAL}" > "${run_dir}/iteration-1/output/${validated_filename}" + fi + # A later iteration with different, distinguishable content — must never + # be used when FULLSEND_VALIDATED_ITERATION_DIR is set, whether or not + # the validated dir's file is found. + mkdir -p "${run_dir}/iteration-2/output" + echo "${WRONG_ITERATION_JSON}" > "${run_dir}/iteration-2/output/agent-result.json" + : > "${GH_LOG}" + export GH_MOCK_COMMENT_FAIL="" + + local exit_code=0 + ( + cd "${run_dir}" + export FULLSEND_VALIDATED_ITERATION_DIR="${run_dir}/iteration-1/output" + bash "${POST_SCRIPT}" + ) > "${TMPDIR}/stdout.log" 2>&1 || exit_code=$? + + if [[ "${expect_success}" == "true" ]]; then + if [[ ${exit_code} -ne 0 ]]; then + echo "FAIL: ${test_name} — exit code ${exit_code}" + cat "${TMPDIR}/stdout.log" + FAILURES=$((FAILURES + 1)) + return + fi + if grep -qF "WRONG_ITERATION_MARKER" "${GH_LOG}"; then + echo "FAIL: ${test_name} — used iteration-2's output instead of the validated iteration" + cat "${GH_LOG}" + FAILURES=$((FAILURES + 1)) + return + fi + echo "PASS: ${test_name}" + return + fi + + # expect_success == false: must fail closed, and must NOT have silently + # fallen back to iteration-2's output. + if [[ ${exit_code} -eq 0 ]] && grep -qF "WRONG_ITERATION_MARKER" "${GH_LOG}"; then + echo "FAIL: ${test_name} — silently fell back to iteration-2 instead of failing closed" + cat "${GH_LOG}" + FAILURES=$((FAILURES + 1)) + return + fi + echo "PASS: ${test_name} (exit code ${exit_code}, did not use wrong iteration)" +} + +run_validated_iter_test "validated-dir-agent-result-json-used" "agent-result.json" "true" +run_validated_iter_test "validated-dir-result-json-fallback-used" "result.json" "true" +run_validated_iter_test "validated-dir-missing-file-fails-closed" "" "false" + # --- Results --- if [[ ${FAILURES} -gt 0 ]]; then diff --git a/internal/scaffold/fullsend-repo/scripts/post-review-test.sh b/internal/scaffold/fullsend-repo/scripts/post-review-test.sh index bedeff51b..749fa60fb 100644 --- a/internal/scaffold/fullsend-repo/scripts/post-review-test.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-review-test.sh @@ -704,6 +704,80 @@ run_label_test_with_env_stdout "draft-pr-approve-log-message" \ "Draft PR" \ "MOCK_IS_DRAFT" "true" +# --- FULLSEND_VALIDATED_ITERATION_DIR selection tests (#5395 follow-up) --- +# +# These verify the fix for a fail-open regression: the prefer-validated-dir +# logic must not silently fall back to rescanning iteration-*/output for +# agent-result.json when FULLSEND_VALIDATED_ITERATION_DIR is set but the +# expected filename is missing there (e.g. the agent wrote "result.json", +# which validate-output-schema.sh accepts as a fallback without renaming). +# Falling back to a rescan would let a later, unvalidated iteration's output +# win — exactly the bug #5393/#5395 fixed. + +VALIDATED_ITER_JSON='{"action":"approve","pr_number":99,"repo":"test-org/test-repo","head_sha":"abc123","body":"LGTM"}' +WRONG_ITERATION_JSON='{"action":"reject","pr_number":99,"repo":"test-org/test-repo","head_sha":"abc123","body":"WRONG_ITERATION_MARKER"}' + +run_validated_iter_test() { + local test_name="$1" + local validated_filename="$2" # "" to leave the validated dir empty + local expect_success="$3" + + local run_dir="${TMPDIR}/run-${test_name}" + mkdir -p "${run_dir}/iteration-1/output" + if [ -n "${validated_filename}" ]; then + echo "${VALIDATED_ITER_JSON}" > "${run_dir}/iteration-1/output/${validated_filename}" + fi + # A later iteration with different, distinguishable content — must never + # be used when FULLSEND_VALIDATED_ITERATION_DIR is set, whether or not + # the validated dir's file is found. + mkdir -p "${run_dir}/iteration-2/output" + echo "${WRONG_ITERATION_JSON}" > "${run_dir}/iteration-2/output/agent-result.json" + : > "${GH_LOG}" + + local exit_code=0 + # shellcheck disable=SC2030,SC2031 + ( + cd "${run_dir}" + export PATH="${MOCK_BIN}:${PATH}" + export REVIEW_TOKEN="fake-token" + export PR_NUMBER="99" + export REPO_FULL_NAME="test-org/test-repo" + export FULLSEND_VALIDATED_ITERATION_DIR="${run_dir}/iteration-1/output" + bash "${POST_SCRIPT}" + ) > "${TMPDIR}/stdout-${test_name}.log" 2>&1 || exit_code=$? + + if [ "${expect_success}" = "true" ]; then + if [ "${exit_code}" -ne 0 ]; then + echo "FAIL: ${test_name} — exit code ${exit_code}" + cat "${TMPDIR}/stdout-${test_name}.log" + FAILURES=$((FAILURES + 1)) + return + fi + if grep -qF "WRONG_ITERATION_MARKER" "${GH_LOG}"; then + echo "FAIL: ${test_name} — used iteration-2's output instead of the validated iteration" + cat "${GH_LOG}" + FAILURES=$((FAILURES + 1)) + return + fi + echo "PASS: ${test_name}" + return + fi + + # expect_success == false: must fail closed, and must NOT have silently + # fallen back to iteration-2's output. + if [ "${exit_code}" -eq 0 ] && grep -qF "WRONG_ITERATION_MARKER" "${GH_LOG}"; then + echo "FAIL: ${test_name} — silently fell back to iteration-2 instead of failing closed" + cat "${GH_LOG}" + FAILURES=$((FAILURES + 1)) + return + fi + echo "PASS: ${test_name} (exit code ${exit_code}, did not use wrong iteration)" +} + +run_validated_iter_test "validated-dir-agent-result-json-used" "agent-result.json" "true" +run_validated_iter_test "validated-dir-result-json-fallback-used" "result.json" "true" +run_validated_iter_test "validated-dir-missing-file-fails-closed" "" "false" + # --- Summary --- echo "" diff --git a/internal/scaffold/fullsend-repo/scripts/post-triage-test.sh b/internal/scaffold/fullsend-repo/scripts/post-triage-test.sh index fd4f4d8f4..e2fd55153 100755 --- a/internal/scaffold/fullsend-repo/scripts/post-triage-test.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-triage-test.sh @@ -445,6 +445,75 @@ run_test "ready-to-code-applied-without-label-actions" \ '{"action":"sufficient","reasoning":"all clear","clarity_scores":{"symptom":0.9,"cause":0.85,"reproduction":0.9,"impact":0.8,"overall":0.87},"triage_summary":{"title":"Fix crash","severity":"high","category":"bug","problem":"Crash","root_cause_hypothesis":"Buffer overflow","reproduction_steps":["step 1"],"environment":"Linux","impact":"All users","recommended_fix":"Fix buffer","proposed_test_case":"test_crash"},"comment":"## Triage Summary\n\nReady."}' \ "gh api repos/test-org/test-repo/issues/42/labels -f labels[]=ready-to-code --silent" +# --- FULLSEND_VALIDATED_ITERATION_DIR selection tests (#5395 follow-up) --- +# +# These verify the fix for a fail-open regression: the prefer-validated-dir +# logic must not silently fall back to rescanning iteration-*/output for +# agent-result.json when FULLSEND_VALIDATED_ITERATION_DIR is set but the +# expected filename is missing there (e.g. the agent wrote "result.json", +# which validate-output-schema.sh accepts as a fallback without renaming). +# Falling back to a rescan would let a later, unvalidated iteration's output +# win — exactly the bug #5393/#5395 fixed. + +INSUFFICIENT_JSON='{"action":"insufficient","reasoning":"missing repro","clarity_scores":{"symptom":0.6,"cause":0.3,"reproduction":0.1,"impact":0.5,"overall":0.39},"comment":"Could you share the exact steps to reproduce this?"}' +WRONG_ITERATION_JSON='{"action":"insufficient","reasoning":"should not be used","clarity_scores":{"symptom":0,"cause":0,"reproduction":0,"impact":0,"overall":0},"comment":"WRONG_ITERATION_MARKER"}' + +run_validated_iter_test() { + local test_name="$1" + local validated_filename="$2" # "" to leave the validated dir empty + local expect_success="$3" + + local run_dir="${TMPDIR}/run-${test_name}" + mkdir -p "${run_dir}/iteration-1/output" + if [[ -n "${validated_filename}" ]]; then + echo "${INSUFFICIENT_JSON}" > "${run_dir}/iteration-1/output/${validated_filename}" + fi + # A later iteration with different, distinguishable content — must never + # be used when FULLSEND_VALIDATED_ITERATION_DIR is set, whether or not + # the validated dir's file is found. + mkdir -p "${run_dir}/iteration-2/output" + echo "${WRONG_ITERATION_JSON}" > "${run_dir}/iteration-2/output/agent-result.json" + : > "${GH_LOG}" + + local exit_code=0 + ( + cd "${run_dir}" + export FULLSEND_VALIDATED_ITERATION_DIR="${run_dir}/iteration-1/output" + bash "${POST_SCRIPT}" + ) > "${TMPDIR}/stdout.log" 2>&1 || exit_code=$? + + if [[ "${expect_success}" == "true" ]]; then + if [[ ${exit_code} -ne 0 ]]; then + echo "FAIL: ${test_name} — exit code ${exit_code}" + cat "${TMPDIR}/stdout.log" + FAILURES=$((FAILURES + 1)) + return + fi + if grep -qF "WRONG_ITERATION_MARKER" "${GH_LOG}"; then + echo "FAIL: ${test_name} — used iteration-2's output instead of the validated iteration" + cat "${GH_LOG}" + FAILURES=$((FAILURES + 1)) + return + fi + echo "PASS: ${test_name}" + return + fi + + # expect_success == false: must fail closed, and must NOT have silently + # fallen back to iteration-2's output. + if [[ ${exit_code} -eq 0 ]] && grep -qF "WRONG_ITERATION_MARKER" "${GH_LOG}"; then + echo "FAIL: ${test_name} — silently fell back to iteration-2 instead of failing closed" + cat "${GH_LOG}" + FAILURES=$((FAILURES + 1)) + return + fi + echo "PASS: ${test_name} (exit code ${exit_code}, did not use wrong iteration)" +} + +run_validated_iter_test "validated-dir-agent-result-json-used" "agent-result.json" "true" +run_validated_iter_test "validated-dir-result-json-fallback-used" "result.json" "true" +run_validated_iter_test "validated-dir-missing-file-fails-closed" "" "false" + # --- Summary --- echo "" From 4165faf20cd9fd171fa74353d59d9a35c9d7b7a1 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Thu, 23 Jul 2026 15:18:24 -0400 Subject: [PATCH 11/12] chore(scaffold): drop internal/scaffold/fullsend-repo post-script changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Agent scripts (agents/*.md, harness/*.yaml, scripts/pre-*.sh, scripts/post-*.sh, and their tests) have moved to fullsend-ai/agents, the canonical source per ADR 0058's extraction plan. PR #5425 (fullsend-ai/fullsend) confirms the disk-based scaffold fallback for agent resolution is now unreachable for real installations — all customers resolve agents via config or the fullsend-ai/agents runtime fallback, not internal/scaffold/fullsend-repo/. Scaffold file deletion itself is a separate future step (Step 8) not yet done, so the directory isn't gone, but it's no longer where agent-script fixes should land. This reverts the FULLSEND_VALIDATED_ITERATION_DIR selection logic (600df8d) and the fail-open fallback fix + tests (3aa1d711, f4355cd8) from the five scaffolded post-scripts and their test files, restoring them to their pre-#5395 state. The underlying CLI-side fix (run.go's retry/validation loop, postScriptRepoEnv, and the FULLSEND_VALIDATED_ ITERATION_DIR env var the CLI sets for whatever post-script actually runs) is untouched — that's core CLI logic, not an agent script, and remains correct regardless of which repo hosts the script content. Tracked for the agents repo in fullsend-ai/agents#411 with a /fs-triage trigger to port the full feature there. Assisted-by: Claude Signed-off-by: Wayne Sun --- .../fullsend-repo/scripts/post-fix-test.sh | 102 ------------------ .../fullsend-repo/scripts/post-fix.sh | 27 +---- .../scripts/post-prioritize-test.sh | 71 ------------ .../fullsend-repo/scripts/post-prioritize.sh | 30 ++---- .../fullsend-repo/scripts/post-retro-test.sh | 69 ------------ .../fullsend-repo/scripts/post-retro.sh | 30 ++---- .../fullsend-repo/scripts/post-review-test.sh | 74 ------------- .../fullsend-repo/scripts/post-review.sh | 20 +--- .../fullsend-repo/scripts/post-triage-test.sh | 69 ------------ .../fullsend-repo/scripts/post-triage.sh | 31 ++---- 10 files changed, 29 insertions(+), 494 deletions(-) diff --git a/internal/scaffold/fullsend-repo/scripts/post-fix-test.sh b/internal/scaffold/fullsend-repo/scripts/post-fix-test.sh index 1850b8911..687a2fa2f 100644 --- a/internal/scaffold/fullsend-repo/scripts/post-fix-test.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-fix-test.sh @@ -157,108 +157,6 @@ run_precommit_retry_test "precommit-passes-with-unstaged" \ run_precommit_retry_test "precommit-retry-passes-but-left-unstaged" \ "1" "yes" "0" "blocked:retry-left-unstaged" "yes" -# --------------------------------------------------------------------------- -# Test helper — reimplements the RESULT_FILE selection logic from post-fix.sh -# (the FULLSEND_VALIDATED_ITERATION_DIR / fix-result.json / result.json -# fallback / rescan block) so we can test it without git, gitleaks, or a -# real PR. Verifies the fix for a fail-open regression (#5395 follow-up): -# the prefer-validated-dir logic must not silently fall back to rescanning -# RUN_DIR/iteration-*/output for fix-result.json when -# FULLSEND_VALIDATED_ITERATION_DIR is set but the expected filename is -# missing there (e.g. the agent wrote "result.json", which -# validate-output-schema.sh accepts as a fallback without renaming). Falling -# back to a rescan would let a later, unvalidated iteration's output win — -# exactly the bug #5393/#5395 fixed. -# --------------------------------------------------------------------------- -select_fix_result_file() { - local run_dir="$1" - local validated_dir="${2:-}" - - local result_file - if [ -n "${validated_dir}" ]; then - result_file="${validated_dir}/fix-result.json" - if [ ! -f "${result_file}" ]; then - local fallback_result="${validated_dir}/result.json" - [ -f "${fallback_result}" ] && result_file="${fallback_result}" - fi - else - result_file="" - for dir in "${run_dir}"/iteration-*/output; do - if [ -f "${dir}/fix-result.json" ]; then - result_file="${dir}/fix-result.json" - fi - done - fi - echo "${result_file}" -} - -run_select_fix_result_test() { - local test_name="$1" - local validated_filename="$2" # "" to leave the validated dir empty - local expect_wrong_iteration="$3" # "true" if a wrong-iteration fallback is acceptable (no FULLSEND_VALIDATED_ITERATION_DIR) - - local run_dir - run_dir="$(mktemp -d)" - mkdir -p "${run_dir}/iteration-1/output" - if [ -n "${validated_filename}" ]; then - echo '{"marker":"validated"}' > "${run_dir}/iteration-1/output/${validated_filename}" - fi - # A later iteration — must never be selected when FULLSEND_VALIDATED_ITERATION_DIR - # is set, whether or not the validated dir's file is found. - mkdir -p "${run_dir}/iteration-2/output" - echo '{"marker":"wrong-iteration"}' > "${run_dir}/iteration-2/output/fix-result.json" - - local result_file - result_file="$(select_fix_result_file "${run_dir}" "${run_dir}/iteration-1/output")" - - rm -rf "${run_dir}" - - if [ "${expect_wrong_iteration}" = "true" ]; then - if [ "${result_file}" != "${run_dir}/iteration-1/output/${validated_filename}" ]; then - echo "FAIL: ${test_name} — expected '${run_dir}/iteration-1/output/${validated_filename}', got '${result_file}'" - FAILURES=$((FAILURES + 1)) - return - fi - echo "PASS: ${test_name}" - return - fi - - # Missing-file case: must fail closed (empty result), never silently - # resolve to iteration-2's fix-result.json. - if [ "${result_file}" = "${run_dir}/iteration-2/output/fix-result.json" ]; then - echo "FAIL: ${test_name} — silently fell back to iteration-2 instead of failing closed" - FAILURES=$((FAILURES + 1)) - return - fi - echo "PASS: ${test_name} (result_file='${result_file}', did not use wrong iteration)" -} - -run_select_fix_result_test "validated-dir-fix-result-json-used" "fix-result.json" "true" -run_select_fix_result_test "validated-dir-result-json-fallback-used" "result.json" "true" -run_select_fix_result_test "validated-dir-missing-file-fails-closed" "" "false" - -# Sanity check: with no FULLSEND_VALIDATED_ITERATION_DIR (legacy behavior), -# the rescan picks the last iteration by glob order. -run_test_rescan_last_iteration() { - local run_dir - run_dir="$(mktemp -d)" - mkdir -p "${run_dir}/iteration-1/output" "${run_dir}/iteration-2/output" - echo '{"marker":"iter1"}' > "${run_dir}/iteration-1/output/fix-result.json" - echo '{"marker":"iter2"}' > "${run_dir}/iteration-2/output/fix-result.json" - - local result_file - result_file="$(select_fix_result_file "${run_dir}" "")" - rm -rf "${run_dir}" - - if [ "${result_file}" != "${run_dir}/iteration-2/output/fix-result.json" ]; then - echo "FAIL: rescan-picks-last-iteration — expected iteration-2, got '${result_file}'" - FAILURES=$((FAILURES + 1)) - return - fi - echo "PASS: rescan-picks-last-iteration" -} -run_test_rescan_last_iteration - # --- Summary --- echo "" diff --git a/internal/scaffold/fullsend-repo/scripts/post-fix.sh b/internal/scaffold/fullsend-repo/scripts/post-fix.sh index 132929ad9..a537e21b7 100644 --- a/internal/scaffold/fullsend-repo/scripts/post-fix.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-fix.sh @@ -359,29 +359,12 @@ fi # iteration-/output/fix-result.json within runDir. Uses glob order # (naturally ascending iteration numbers) to find the last iteration, # matching the pattern in post-triage.sh. -# Prefer the validated iteration directory set by the harness -# (FULLSEND_VALIDATED_ITERATION_DIR) — without it, the last iteration's output -# may be schema-invalid content that failed validation. Fall back to scanning -# for the last iteration for backward compatibility. -if [ -n "${FULLSEND_VALIDATED_ITERATION_DIR:-}" ]; then - RESULT_FILE="${FULLSEND_VALIDATED_ITERATION_DIR}/fix-result.json" - if [ ! -f "${RESULT_FILE}" ]; then - # Agents sometimes write "result.json" instead of the harness-configured - # "fix-result.json"; validate-output-schema.sh accepts that filename as - # a fallback without renaming it. Check the same variant here rather - # than falling through to the rescan below, which could silently pick - # up a different (and possibly schema-invalid) iteration's output. - fallback_result="${FULLSEND_VALIDATED_ITERATION_DIR}/result.json" - [ -f "${fallback_result}" ] && RESULT_FILE="${fallback_result}" +RESULT_FILE="" +for dir in "${RUN_DIR}"/iteration-*/output; do + if [ -f "${dir}/fix-result.json" ]; then + RESULT_FILE="${dir}/fix-result.json" fi -else - RESULT_FILE="" - for dir in "${RUN_DIR}"/iteration-*/output; do - if [ -f "${dir}/fix-result.json" ]; then - RESULT_FILE="${dir}/fix-result.json" - fi - done -fi +done if [ -z "${RESULT_FILE}" ] || [ ! -f "${RESULT_FILE}" ]; then echo "::warning::No fix-result.json found — skipping summary comment" diff --git a/internal/scaffold/fullsend-repo/scripts/post-prioritize-test.sh b/internal/scaffold/fullsend-repo/scripts/post-prioritize-test.sh index 268ba70f8..48f980250 100755 --- a/internal/scaffold/fullsend-repo/scripts/post-prioritize-test.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-prioritize-test.sh @@ -630,77 +630,6 @@ export GITHUB_CSMA_MAX_ATTEMPTS=3 run_test_failure_stderr "exit0-rate-limit-exhausted" "100" "rate limit exceeded" "exit0-ratelimit" unset GITHUB_CSMA_MAX_ATTEMPTS -# --- FULLSEND_VALIDATED_ITERATION_DIR selection tests (#5395 follow-up) --- -# -# These verify the fix for a fail-open regression: the prefer-validated-dir -# logic must not silently fall back to rescanning iteration-*/output for -# agent-result.json when FULLSEND_VALIDATED_ITERATION_DIR is set but the -# expected filename is missing there (e.g. the agent wrote "result.json", -# which validate-output-schema.sh accepts as a fallback without renaming). -# Falling back to a rescan would let a later, unvalidated iteration's output -# win — exactly the bug #5393/#5395 fixed. - -WRONG_ITERATION_JSON='{"reach":99,"impact":99,"confidence":0.99,"effort":99,"reasoning":{"reach":"WRONG_ITERATION_MARKER","impact":"x","confidence":"x","effort":"x"}}' - -run_validated_iter_test() { - local test_name="$1" - local validated_filename="$2" # "" to leave the validated dir empty - local expect_success="$3" - - local run_dir="${TEST_TMPDIR}/run-${test_name}" - mkdir -p "${run_dir}/iteration-1/output" - if [[ -n "${validated_filename}" ]]; then - echo "${FIXTURE_JSON}" > "${run_dir}/iteration-1/output/${validated_filename}" - fi - # A later iteration with different, distinguishable content — must never - # be used when FULLSEND_VALIDATED_ITERATION_DIR is set, whether or not - # the validated dir's file is found. - mkdir -p "${run_dir}/iteration-2/output" - echo "${WRONG_ITERATION_JSON}" > "${run_dir}/iteration-2/output/agent-result.json" - - : > "${GH_LOG}" - rm -f "${GH_FAIL_COUNT}" - unset GH_CSMA_FAIL_UNTIL GH_CSMA_FAIL_MODE - - local exit_code=0 - ( - cd "${run_dir}" - export FULLSEND_VALIDATED_ITERATION_DIR="${run_dir}/iteration-1/output" - bash "${POST_SCRIPT}" - ) > "${TEST_TMPDIR}/stdout-${test_name}.log" 2>&1 || exit_code=$? - - if [[ "${expect_success}" == "true" ]]; then - if [[ ${exit_code} -ne 0 ]]; then - echo "FAIL: ${test_name} — exit code ${exit_code}" - cat "${TEST_TMPDIR}/stdout-${test_name}.log" - FAILURES=$((FAILURES + 1)) - return - fi - if grep -qF "WRONG_ITERATION_MARKER" "${GH_LOG}"; then - echo "FAIL: ${test_name} — used iteration-2's output instead of the validated iteration" - cat "${GH_LOG}" - FAILURES=$((FAILURES + 1)) - return - fi - echo "PASS: ${test_name}" - return - fi - - # expect_success == false: must fail closed, and must NOT have silently - # fallen back to iteration-2's output. - if [[ ${exit_code} -eq 0 ]] && grep -qF "WRONG_ITERATION_MARKER" "${GH_LOG}"; then - echo "FAIL: ${test_name} — silently fell back to iteration-2 instead of failing closed" - cat "${GH_LOG}" - FAILURES=$((FAILURES + 1)) - return - fi - echo "PASS: ${test_name} (exit code ${exit_code}, did not use wrong iteration)" -} - -run_validated_iter_test "validated-dir-agent-result-json-used" "agent-result.json" "true" -run_validated_iter_test "validated-dir-result-json-fallback-used" "result.json" "true" -run_validated_iter_test "validated-dir-missing-file-fails-closed" "" "false" - if [[ ${FAILURES} -gt 0 ]]; then echo "" echo "${FAILURES} test(s) failed." diff --git a/internal/scaffold/fullsend-repo/scripts/post-prioritize.sh b/internal/scaffold/fullsend-repo/scripts/post-prioritize.sh index 873c62f46..9e331cd5e 100755 --- a/internal/scaffold/fullsend-repo/scripts/post-prioritize.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-prioritize.sh @@ -339,31 +339,15 @@ if [[ ! "${GITHUB_ISSUE_URL}" =~ ^https://github\.com/[a-zA-Z0-9._-]+/[a-zA-Z0-9 exit 1 fi -# Find the result JSON. Prefer the validated iteration directory set by the -# harness (FULLSEND_VALIDATED_ITERATION_DIR) — without it, the last iteration's -# output may be schema-invalid content that failed validation. Fall back to -# scanning for the last iteration for backward compatibility. -if [[ -n "${FULLSEND_VALIDATED_ITERATION_DIR:-}" ]]; then - RESULT_FILE="${FULLSEND_VALIDATED_ITERATION_DIR}/agent-result.json" - if [[ ! -f "${RESULT_FILE}" ]]; then - # Agents sometimes write "result.json" instead of "agent-result.json"; - # validate-output-schema.sh accepts that filename as a fallback without - # renaming it. Check the same variant here rather than falling through - # to the rescan below, which could silently pick up a different (and - # possibly schema-invalid) iteration's output. - fallback_result="${FULLSEND_VALIDATED_ITERATION_DIR}/result.json" - [[ -f "${fallback_result}" ]] && RESULT_FILE="${fallback_result}" +# Find the result JSON from the last iteration. +RESULT_FILE="" +for dir in iteration-*/output; do + if [[ -f "${dir}/agent-result.json" ]]; then + RESULT_FILE="${dir}/agent-result.json" fi -else - RESULT_FILE="" - for dir in iteration-*/output; do - if [[ -f "${dir}/agent-result.json" ]]; then - RESULT_FILE="${dir}/agent-result.json" - fi - done -fi +done -if [[ -z "${RESULT_FILE}" ]] || [[ ! -f "${RESULT_FILE}" ]]; then +if [[ -z "${RESULT_FILE}" ]]; then echo "ERROR: agent-result.json not found in any iteration output directory" >&2 exit 1 fi diff --git a/internal/scaffold/fullsend-repo/scripts/post-retro-test.sh b/internal/scaffold/fullsend-repo/scripts/post-retro-test.sh index c8cfd4394..40f6f94ce 100644 --- a/internal/scaffold/fullsend-repo/scripts/post-retro-test.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-retro-test.sh @@ -274,75 +274,6 @@ run_test_stdout "complete-message" \ "${FIXTURE_ONE_PROPOSAL}" \ "Post-retro complete." -# --- FULLSEND_VALIDATED_ITERATION_DIR selection tests (#5395 follow-up) --- -# -# These verify the fix for a fail-open regression: the prefer-validated-dir -# logic must not silently fall back to rescanning iteration-*/output for -# agent-result.json when FULLSEND_VALIDATED_ITERATION_DIR is set but the -# expected filename is missing there (e.g. the agent wrote "result.json", -# which validate-output-schema.sh accepts as a fallback without renaming). -# Falling back to a rescan would let a later, unvalidated iteration's output -# win — exactly the bug #5393/#5395 fixed. - -WRONG_ITERATION_JSON='{"summary":"WRONG_ITERATION_MARKER","proposals":[]}' - -run_validated_iter_test() { - local test_name="$1" - local validated_filename="$2" # "" to leave the validated dir empty - local expect_success="$3" - - local run_dir="${TMPDIR}/run-${test_name}" - mkdir -p "${run_dir}/iteration-1/output" - if [[ -n "${validated_filename}" ]]; then - echo "${FIXTURE_ONE_PROPOSAL}" > "${run_dir}/iteration-1/output/${validated_filename}" - fi - # A later iteration with different, distinguishable content — must never - # be used when FULLSEND_VALIDATED_ITERATION_DIR is set, whether or not - # the validated dir's file is found. - mkdir -p "${run_dir}/iteration-2/output" - echo "${WRONG_ITERATION_JSON}" > "${run_dir}/iteration-2/output/agent-result.json" - : > "${GH_LOG}" - export GH_MOCK_COMMENT_FAIL="" - - local exit_code=0 - ( - cd "${run_dir}" - export FULLSEND_VALIDATED_ITERATION_DIR="${run_dir}/iteration-1/output" - bash "${POST_SCRIPT}" - ) > "${TMPDIR}/stdout.log" 2>&1 || exit_code=$? - - if [[ "${expect_success}" == "true" ]]; then - if [[ ${exit_code} -ne 0 ]]; then - echo "FAIL: ${test_name} — exit code ${exit_code}" - cat "${TMPDIR}/stdout.log" - FAILURES=$((FAILURES + 1)) - return - fi - if grep -qF "WRONG_ITERATION_MARKER" "${GH_LOG}"; then - echo "FAIL: ${test_name} — used iteration-2's output instead of the validated iteration" - cat "${GH_LOG}" - FAILURES=$((FAILURES + 1)) - return - fi - echo "PASS: ${test_name}" - return - fi - - # expect_success == false: must fail closed, and must NOT have silently - # fallen back to iteration-2's output. - if [[ ${exit_code} -eq 0 ]] && grep -qF "WRONG_ITERATION_MARKER" "${GH_LOG}"; then - echo "FAIL: ${test_name} — silently fell back to iteration-2 instead of failing closed" - cat "${GH_LOG}" - FAILURES=$((FAILURES + 1)) - return - fi - echo "PASS: ${test_name} (exit code ${exit_code}, did not use wrong iteration)" -} - -run_validated_iter_test "validated-dir-agent-result-json-used" "agent-result.json" "true" -run_validated_iter_test "validated-dir-result-json-fallback-used" "result.json" "true" -run_validated_iter_test "validated-dir-missing-file-fails-closed" "" "false" - # --- Results --- if [[ ${FAILURES} -gt 0 ]]; then diff --git a/internal/scaffold/fullsend-repo/scripts/post-retro.sh b/internal/scaffold/fullsend-repo/scripts/post-retro.sh index 40bb83134..dba492f3c 100755 --- a/internal/scaffold/fullsend-repo/scripts/post-retro.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-retro.sh @@ -17,31 +17,15 @@ set -euo pipefail : "${GH_TOKEN:?GH_TOKEN is required}" echo "::add-mask::${GH_TOKEN}" -# Find the retro result JSON. Prefer the validated iteration directory set by -# the harness (FULLSEND_VALIDATED_ITERATION_DIR) — without it, the last -# iteration's output may be schema-invalid content that failed validation. -# Fall back to scanning for the last iteration for backward compatibility. -if [[ -n "${FULLSEND_VALIDATED_ITERATION_DIR:-}" ]]; then - RESULT_FILE="${FULLSEND_VALIDATED_ITERATION_DIR}/agent-result.json" - if [[ ! -f "${RESULT_FILE}" ]]; then - # Agents sometimes write "result.json" instead of "agent-result.json"; - # validate-output-schema.sh accepts that filename as a fallback without - # renaming it. Check the same variant here rather than falling through - # to the rescan below, which could silently pick up a different (and - # possibly schema-invalid) iteration's output. - fallback_result="${FULLSEND_VALIDATED_ITERATION_DIR}/result.json" - [[ -f "${fallback_result}" ]] && RESULT_FILE="${fallback_result}" +# Find the retro result JSON (same pattern as post-triage.sh). +RESULT_FILE="" +for dir in iteration-*/output; do + if [[ -f "${dir}/agent-result.json" ]]; then + RESULT_FILE="${dir}/agent-result.json" fi -else - RESULT_FILE="" - for dir in iteration-*/output; do - if [[ -f "${dir}/agent-result.json" ]]; then - RESULT_FILE="${dir}/agent-result.json" - fi - done -fi +done -if [[ -z "${RESULT_FILE}" ]] || [[ ! -f "${RESULT_FILE}" ]]; then +if [[ -z "${RESULT_FILE}" ]]; then echo "ERROR: agent-result.json not found in any iteration output directory" >&2 exit 1 fi diff --git a/internal/scaffold/fullsend-repo/scripts/post-review-test.sh b/internal/scaffold/fullsend-repo/scripts/post-review-test.sh index 749fa60fb..bedeff51b 100644 --- a/internal/scaffold/fullsend-repo/scripts/post-review-test.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-review-test.sh @@ -704,80 +704,6 @@ run_label_test_with_env_stdout "draft-pr-approve-log-message" \ "Draft PR" \ "MOCK_IS_DRAFT" "true" -# --- FULLSEND_VALIDATED_ITERATION_DIR selection tests (#5395 follow-up) --- -# -# These verify the fix for a fail-open regression: the prefer-validated-dir -# logic must not silently fall back to rescanning iteration-*/output for -# agent-result.json when FULLSEND_VALIDATED_ITERATION_DIR is set but the -# expected filename is missing there (e.g. the agent wrote "result.json", -# which validate-output-schema.sh accepts as a fallback without renaming). -# Falling back to a rescan would let a later, unvalidated iteration's output -# win — exactly the bug #5393/#5395 fixed. - -VALIDATED_ITER_JSON='{"action":"approve","pr_number":99,"repo":"test-org/test-repo","head_sha":"abc123","body":"LGTM"}' -WRONG_ITERATION_JSON='{"action":"reject","pr_number":99,"repo":"test-org/test-repo","head_sha":"abc123","body":"WRONG_ITERATION_MARKER"}' - -run_validated_iter_test() { - local test_name="$1" - local validated_filename="$2" # "" to leave the validated dir empty - local expect_success="$3" - - local run_dir="${TMPDIR}/run-${test_name}" - mkdir -p "${run_dir}/iteration-1/output" - if [ -n "${validated_filename}" ]; then - echo "${VALIDATED_ITER_JSON}" > "${run_dir}/iteration-1/output/${validated_filename}" - fi - # A later iteration with different, distinguishable content — must never - # be used when FULLSEND_VALIDATED_ITERATION_DIR is set, whether or not - # the validated dir's file is found. - mkdir -p "${run_dir}/iteration-2/output" - echo "${WRONG_ITERATION_JSON}" > "${run_dir}/iteration-2/output/agent-result.json" - : > "${GH_LOG}" - - local exit_code=0 - # shellcheck disable=SC2030,SC2031 - ( - cd "${run_dir}" - export PATH="${MOCK_BIN}:${PATH}" - export REVIEW_TOKEN="fake-token" - export PR_NUMBER="99" - export REPO_FULL_NAME="test-org/test-repo" - export FULLSEND_VALIDATED_ITERATION_DIR="${run_dir}/iteration-1/output" - bash "${POST_SCRIPT}" - ) > "${TMPDIR}/stdout-${test_name}.log" 2>&1 || exit_code=$? - - if [ "${expect_success}" = "true" ]; then - if [ "${exit_code}" -ne 0 ]; then - echo "FAIL: ${test_name} — exit code ${exit_code}" - cat "${TMPDIR}/stdout-${test_name}.log" - FAILURES=$((FAILURES + 1)) - return - fi - if grep -qF "WRONG_ITERATION_MARKER" "${GH_LOG}"; then - echo "FAIL: ${test_name} — used iteration-2's output instead of the validated iteration" - cat "${GH_LOG}" - FAILURES=$((FAILURES + 1)) - return - fi - echo "PASS: ${test_name}" - return - fi - - # expect_success == false: must fail closed, and must NOT have silently - # fallen back to iteration-2's output. - if [ "${exit_code}" -eq 0 ] && grep -qF "WRONG_ITERATION_MARKER" "${GH_LOG}"; then - echo "FAIL: ${test_name} — silently fell back to iteration-2 instead of failing closed" - cat "${GH_LOG}" - FAILURES=$((FAILURES + 1)) - return - fi - echo "PASS: ${test_name} (exit code ${exit_code}, did not use wrong iteration)" -} - -run_validated_iter_test "validated-dir-agent-result-json-used" "agent-result.json" "true" -run_validated_iter_test "validated-dir-result-json-fallback-used" "result.json" "true" -run_validated_iter_test "validated-dir-missing-file-fails-closed" "" "false" - # --- Summary --- echo "" diff --git a/internal/scaffold/fullsend-repo/scripts/post-review.sh b/internal/scaffold/fullsend-repo/scripts/post-review.sh index 049a9a980..a7b783329 100755 --- a/internal/scaffold/fullsend-repo/scripts/post-review.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-review.sh @@ -56,24 +56,8 @@ The \`/fs-review\` command only reviews open pull requests. exit 0 fi -# Find the agent result. Prefer the validated iteration directory set by the -# harness (FULLSEND_VALIDATED_ITERATION_DIR) — without it, the last iteration's -# output may be schema-invalid content that failed validation. Fall back to the -# last iteration for backward compatibility with older fullsend versions. -if [ -n "${FULLSEND_VALIDATED_ITERATION_DIR:-}" ]; then - RESULT_FILE="${FULLSEND_VALIDATED_ITERATION_DIR}/agent-result.json" - if [ ! -f "${RESULT_FILE}" ]; then - # Agents sometimes write "result.json" instead of "agent-result.json"; - # validate-output-schema.sh accepts that filename as a fallback without - # renaming it. Check the same variant here rather than falling through - # to the rescan below, which could silently pick up a different (and - # possibly schema-invalid) iteration's output. - fallback_result="${FULLSEND_VALIDATED_ITERATION_DIR}/result.json" - [ -f "${fallback_result}" ] && RESULT_FILE="${fallback_result}" - fi -else - RESULT_FILE=$(find . -maxdepth 4 -path '*/iteration-*/output/agent-result.json' | sort -V | tail -1) -fi +# Find the agent result from the last iteration +RESULT_FILE=$(find . -maxdepth 4 -path '*/iteration-*/output/agent-result.json' | sort -V | tail -1) if [ -z "${RESULT_FILE}" ] || [ ! -f "${RESULT_FILE}" ]; then echo "::error::No agent-result.json found — posting failure notice" diff --git a/internal/scaffold/fullsend-repo/scripts/post-triage-test.sh b/internal/scaffold/fullsend-repo/scripts/post-triage-test.sh index e2fd55153..fd4f4d8f4 100755 --- a/internal/scaffold/fullsend-repo/scripts/post-triage-test.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-triage-test.sh @@ -445,75 +445,6 @@ run_test "ready-to-code-applied-without-label-actions" \ '{"action":"sufficient","reasoning":"all clear","clarity_scores":{"symptom":0.9,"cause":0.85,"reproduction":0.9,"impact":0.8,"overall":0.87},"triage_summary":{"title":"Fix crash","severity":"high","category":"bug","problem":"Crash","root_cause_hypothesis":"Buffer overflow","reproduction_steps":["step 1"],"environment":"Linux","impact":"All users","recommended_fix":"Fix buffer","proposed_test_case":"test_crash"},"comment":"## Triage Summary\n\nReady."}' \ "gh api repos/test-org/test-repo/issues/42/labels -f labels[]=ready-to-code --silent" -# --- FULLSEND_VALIDATED_ITERATION_DIR selection tests (#5395 follow-up) --- -# -# These verify the fix for a fail-open regression: the prefer-validated-dir -# logic must not silently fall back to rescanning iteration-*/output for -# agent-result.json when FULLSEND_VALIDATED_ITERATION_DIR is set but the -# expected filename is missing there (e.g. the agent wrote "result.json", -# which validate-output-schema.sh accepts as a fallback without renaming). -# Falling back to a rescan would let a later, unvalidated iteration's output -# win — exactly the bug #5393/#5395 fixed. - -INSUFFICIENT_JSON='{"action":"insufficient","reasoning":"missing repro","clarity_scores":{"symptom":0.6,"cause":0.3,"reproduction":0.1,"impact":0.5,"overall":0.39},"comment":"Could you share the exact steps to reproduce this?"}' -WRONG_ITERATION_JSON='{"action":"insufficient","reasoning":"should not be used","clarity_scores":{"symptom":0,"cause":0,"reproduction":0,"impact":0,"overall":0},"comment":"WRONG_ITERATION_MARKER"}' - -run_validated_iter_test() { - local test_name="$1" - local validated_filename="$2" # "" to leave the validated dir empty - local expect_success="$3" - - local run_dir="${TMPDIR}/run-${test_name}" - mkdir -p "${run_dir}/iteration-1/output" - if [[ -n "${validated_filename}" ]]; then - echo "${INSUFFICIENT_JSON}" > "${run_dir}/iteration-1/output/${validated_filename}" - fi - # A later iteration with different, distinguishable content — must never - # be used when FULLSEND_VALIDATED_ITERATION_DIR is set, whether or not - # the validated dir's file is found. - mkdir -p "${run_dir}/iteration-2/output" - echo "${WRONG_ITERATION_JSON}" > "${run_dir}/iteration-2/output/agent-result.json" - : > "${GH_LOG}" - - local exit_code=0 - ( - cd "${run_dir}" - export FULLSEND_VALIDATED_ITERATION_DIR="${run_dir}/iteration-1/output" - bash "${POST_SCRIPT}" - ) > "${TMPDIR}/stdout.log" 2>&1 || exit_code=$? - - if [[ "${expect_success}" == "true" ]]; then - if [[ ${exit_code} -ne 0 ]]; then - echo "FAIL: ${test_name} — exit code ${exit_code}" - cat "${TMPDIR}/stdout.log" - FAILURES=$((FAILURES + 1)) - return - fi - if grep -qF "WRONG_ITERATION_MARKER" "${GH_LOG}"; then - echo "FAIL: ${test_name} — used iteration-2's output instead of the validated iteration" - cat "${GH_LOG}" - FAILURES=$((FAILURES + 1)) - return - fi - echo "PASS: ${test_name}" - return - fi - - # expect_success == false: must fail closed, and must NOT have silently - # fallen back to iteration-2's output. - if [[ ${exit_code} -eq 0 ]] && grep -qF "WRONG_ITERATION_MARKER" "${GH_LOG}"; then - echo "FAIL: ${test_name} — silently fell back to iteration-2 instead of failing closed" - cat "${GH_LOG}" - FAILURES=$((FAILURES + 1)) - return - fi - echo "PASS: ${test_name} (exit code ${exit_code}, did not use wrong iteration)" -} - -run_validated_iter_test "validated-dir-agent-result-json-used" "agent-result.json" "true" -run_validated_iter_test "validated-dir-result-json-fallback-used" "result.json" "true" -run_validated_iter_test "validated-dir-missing-file-fails-closed" "" "false" - # --- Summary --- echo "" diff --git a/internal/scaffold/fullsend-repo/scripts/post-triage.sh b/internal/scaffold/fullsend-repo/scripts/post-triage.sh index bfa5933cb..94cedb01b 100755 --- a/internal/scaffold/fullsend-repo/scripts/post-triage.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-triage.sh @@ -19,31 +19,16 @@ set -euo pipefail -# Find the triage result JSON. Prefer the validated iteration directory set by -# the harness (FULLSEND_VALIDATED_ITERATION_DIR) — without it, the last -# iteration's output may be schema-invalid content that failed validation. -# Fall back to scanning for the last iteration for backward compatibility. -if [[ -n "${FULLSEND_VALIDATED_ITERATION_DIR:-}" ]]; then - RESULT_FILE="${FULLSEND_VALIDATED_ITERATION_DIR}/agent-result.json" - if [[ ! -f "${RESULT_FILE}" ]]; then - # Agents sometimes write "result.json" instead of "agent-result.json"; - # validate-output-schema.sh accepts that filename as a fallback without - # renaming it. Check the same variant here rather than falling through - # to the rescan below, which could silently pick up a different (and - # possibly schema-invalid) iteration's output. - fallback_result="${FULLSEND_VALIDATED_ITERATION_DIR}/result.json" - [[ -f "${fallback_result}" ]] && RESULT_FILE="${fallback_result}" +# Find the triage result JSON. The run dir contains iteration-N/ subdirectories; +# we want the last one's output. +RESULT_FILE="" +for dir in iteration-*/output; do + if [[ -f "${dir}/agent-result.json" ]]; then + RESULT_FILE="${dir}/agent-result.json" fi -else - RESULT_FILE="" - for dir in iteration-*/output; do - if [[ -f "${dir}/agent-result.json" ]]; then - RESULT_FILE="${dir}/agent-result.json" - fi - done -fi +done -if [[ -z "${RESULT_FILE}" ]] || [[ ! -f "${RESULT_FILE}" ]]; then +if [[ -z "${RESULT_FILE}" ]]; then echo "ERROR: agent-result.json not found in any iteration output directory" >&2 exit 1 fi From 4367a58924db1bd17caf2310d81d89db9b3d3db9 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Thu, 23 Jul 2026 19:06:48 -0400 Subject: [PATCH 12/12] docs(runner): fix stale FULLSEND_VALIDATED_ITERATION_DIR consumption claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review-squad follow-up found the comment (and cli-internals.md diagram) still claimed post-review.sh/post-triage.sh/post-retro.sh/post-prioritize.sh "rely on FULLSEND_VALIDATED_ITERATION_DIR" — true when written, but the scaffold-script revert (4165faf2) removed that consumption. The CLI still sets the env var correctly; the scaffold-embedded scripts just don't read it yet. Correct both to say so and point at the tracked port (fullsend-ai/agents#411) instead of claiming it's already handled. Assisted-by: Claude Signed-off-by: Wayne Sun --- docs/guides/dev/cli-internals.md | 10 ++++++---- internal/cli/run.go | 12 +++++++++--- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/docs/guides/dev/cli-internals.md b/docs/guides/dev/cli-internals.md index ec294e14b..e30686418 100644 --- a/docs/guides/dev/cli-internals.md +++ b/docs/guides/dev/cli-internals.md @@ -508,10 +508,12 @@ Vendoring commit messages use title + body (upload and stale delete). `github st │ │ │ see #5393). │ │ │ │ │ │ │ │ FULLSEND_VALIDATED_ITERATION_DIR points │ -│ │ │ to the validated iteration's output dir. │ -│ │ │ Post-scripts must use this (when set) to │ -│ │ │ select the result file instead of │ -│ │ │ blindly taking the last iteration. │ +│ │ │ to the validated iteration's output dir, │ +│ │ │ for forward compatibility. The scaffold- │ +│ │ │ embedded post-scripts don't consume it │ +│ │ │ yet (tracked in fullsend-ai/agents#411) │ +│ │ │ — they still scan for the last iteration │ +│ │ │ blindly. │ │ └──────┬───────────┘ │ │ ▼ │ │ ┌──────────────────┐ │ diff --git a/internal/cli/run.go b/internal/cli/run.go index 142a1d610..e6dd78f7c 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -975,9 +975,7 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep // directory existence check) — both need actual repo content to // push. The other validation_loop post-scripts (post-review.sh, // post-triage.sh, post-retro.sh, post-prioritize.sh) don't - // reference REPO_DIR at all; they rely on - // FULLSEND_VALIDATED_ITERATION_DIR (set below) to select the - // correct iteration's output. code.yaml has no validation_loop, + // reference REPO_DIR at all. code.yaml has no validation_loop, // so post-code.sh cannot currently observe an empty REPO_DIR in // practice — a SafeDownload failure is fatal for it and this // defer never runs — but the check in its script is real, not a @@ -987,6 +985,14 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep // validation_loop) cannot recover a sweep-validated non-final // iteration's repo state — it fails closed with "Extracted repo // not found" instead of pushing. See #5393 follow-up. + // + // FULLSEND_VALIDATED_ITERATION_DIR (set below) is set for + // forward compatibility, but the scaffold-embedded post-scripts + // don't consume it yet — that port is tracked separately in + // fullsend-ai/agents#411, since agent scripts now live in that + // repo, not internal/scaffold/fullsend-repo/. Until that lands, + // post-review.sh/post-triage.sh/post-retro.sh/post-prioritize.sh + // still scan for the last iteration-*/output blindly. postRepoDir, postValidatedIterDir := postScriptRepoEnv(h, runDir, hostRepositoryDownloadDir, repoExtractedOK, validatedIterNum) postCmd.Env = append(postCmd.Env, fmt.Sprintf("REPO_DIR=%s", postRepoDir)) // FULLSEND_VALIDATED_ITERATION_DIR tells the post-script which