fix(agent): prevent duplicate execution of repository setup scripts (#898)

This commit is contained in:
Carlos Florêncio
2026-05-13 19:23:24 +01:00
committed by GitHub
parent d7e183847c
commit e3ea4e1766
4 changed files with 454 additions and 66 deletions
@@ -4,7 +4,6 @@ import (
"context"
"fmt"
"path/filepath"
"strings"
"time"
"go.uber.org/zap"
@@ -85,11 +84,18 @@ func (p *WorktreePreparer) Prepare(ctx context.Context, req *EnvPrepareRequest,
stepIdx++
}
// Step N: Run setup script (if provided)
resolvedScript := resolvePreparerSetupScript(req, workspacePath)
// worktree.Manager.Create already executed the per-repo setup script via
// its ScriptMessageHandler (recorded as a script_execution message). To
// avoid running the same script twice we blank RepoSetupScript on a
// request copy before resolving the prepare script — the placeholder
// substitutes to empty and the resolved script is skipped via
// isScriptEffectivelyEmpty.
scriptReq := *req // shallow copy — Env map is shared (read-only in runSetupScriptStep)
scriptReq.RepoSetupScript = ""
resolvedScript := resolvePreparerSetupScript(&scriptReq, workspacePath)
if resolvedScript != "" {
totalSteps++
steps = runSetupScriptStep(ctx, req, workspacePath, resolvedScript, stepIdx, totalSteps, onProgress, steps, p.logger)
steps = runSetupScriptStep(ctx, &scriptReq, workspacePath, resolvedScript, stepIdx, totalSteps, onProgress, steps, p.logger)
}
return &EnvPrepareResult{
@@ -232,10 +238,13 @@ func finalizeSyncStep(
// failure, all already-created worktrees for this preparation are rolled back
// and the request is reported as failed with the originating error.
//
// Per-repo setup scripts (RepoSetupScript) run as a separate step after each
// repo's worktree is created. The task-level setup script (req.SetupScript)
// is intentionally NOT run from this path — that resolution depends on the
// task root layout and is handled by the caller in a future phase.
// Per-repo setup scripts (RepoSetupScript) are executed by the worktree
// manager during Create() via its script handler, which streams output to a
// "script_execution" chat message. The env preparer does not run them as a
// separate step — doing so caused the script to run twice per repo. The
// task-level setup script (req.SetupScript) is intentionally NOT run from
// this path — that resolution depends on the task root layout and is handled
// by the caller in a future phase.
//
// On success the legacy single-worktree fields on EnvPrepareResult mirror
// Worktrees[0] so existing downstream code (manager_launch.launchApplyPrepareResult,
@@ -263,7 +272,12 @@ func (p *WorktreePreparer) prepareMultiRepo(
}, nil
}
// Step budget: per-repo (validate + create + optional sync + optional checkout + optional setup script).
// Step budget: per-repo (validate + create + optional sync + optional checkout).
// Per-repo setup scripts are executed by the worktree manager during
// Create() (via its script handler, which also creates the chat
// "script_execution" message). The env preparer no longer re-runs them
// here — doing so caused the script to run twice per repo and broke
// non-idempotent scripts (e.g. "mkdir build" on the second run).
totalSteps := 0
for _, spec := range specs {
totalSteps += 2
@@ -273,9 +287,6 @@ func (p *WorktreePreparer) prepareMultiRepo(
if spec.CheckoutBranch != "" {
totalSteps++
}
if strings.TrimSpace(spec.RepoSetupScript) != "" {
totalSteps++
}
}
stepIdx := 0
@@ -329,9 +340,11 @@ func (p *WorktreePreparer) prepareMultiRepo(
return res, nil
}
// prepareOneRepo runs the validate → create-worktree → fetch-PR → setup-script
// sequence for a single repo within a multi-repo preparation. Returns the
// created worktree, updated steps, the next step index, and any error.
// prepareOneRepo runs the validate → create-worktree → fetch-PR sequence for
// a single repo within a multi-repo preparation. The per-repo setup script
// is executed by the worktree manager during Create() and is not re-run
// here. Returns the created worktree, updated steps, the next step index,
// and any error.
func (p *WorktreePreparer) prepareOneRepo(
ctx context.Context,
req *EnvPrepareRequest,
@@ -393,50 +406,14 @@ func (p *WorktreePreparer) prepareOneRepo(
stepIdx++
}
// Per-repo setup script (passed verbatim — no script-engine resolution
// in this phase; that lands with the per-repo placeholder namespace later).
if script := strings.TrimSpace(spec.RepoSetupScript); script != "" {
var runErr error
steps, runErr = runRepoSetupScriptStep(ctx, &subReq, repoLabel, wt.Path, script, stepIdx, totalSteps, onProgress, steps)
if runErr != nil {
return nil, steps, stepIdx + 1, fmt.Errorf("repo %q setup script failed: %w", repoLabel, runErr)
}
stepIdx++
}
// Per-repo setup script execution is owned by the worktree manager
// (createInTaskDir → runWorktreeSetupScript), which streams output to a
// "script_execution" chat message. Running it here as well caused the
// script to execute twice per repo and broke non-idempotent setups.
return wt, steps, stepIdx, nil
}
// runRepoSetupScriptStep executes one per-repo setup script and appends the
// step (with streaming output) to steps. Mirrors runSetupScriptStep but uses a
// repo-scoped step name and returns any execution error to the caller for
// rollback control.
func runRepoSetupScriptStep(
ctx context.Context,
req *EnvPrepareRequest,
repoLabel, workspacePath, script string,
stepIdx, totalSteps int,
onProgress PrepareProgressCallback,
steps []PrepareStep,
) ([]PrepareStep, error) {
step := beginStep(fmt.Sprintf("Run setup script (%s)", repoLabel))
step.Command = setupScriptDisplayCommand(req)
reportProgress(onProgress, step, stepIdx, totalSteps)
output, err := runSetupScript(ctx, script, workspacePath, req.Env, func(current string) {
step.Output = current
reportProgress(onProgress, step, stepIdx, totalSteps)
})
step.Output = output
if err != nil {
completeStepError(&step, err.Error())
} else {
completeStepSuccess(&step)
}
steps = append(steps, step)
reportProgress(onProgress, step, stepIdx, totalSteps)
return steps, err
}
// rollbackWorktrees removes any worktrees created during a failed multi-repo
// preparation. Best effort: failures are logged and do not interrupt teardown.
func (p *WorktreePreparer) rollbackWorktrees(ctx context.Context, ids []string) {
@@ -0,0 +1,322 @@
package lifecycle
import (
"context"
"os"
"os/exec"
"path/filepath"
"strings"
"sync"
"testing"
"github.com/kandev/kandev/internal/agent/executor"
"github.com/kandev/kandev/internal/common/logger"
"github.com/kandev/kandev/internal/worktree"
)
// fakeRepoProvider returns canned Repository values per ID.
type fakeRepoProvider struct {
repos map[string]*worktree.Repository
}
func (p *fakeRepoProvider) GetRepository(_ context.Context, id string) (*worktree.Repository, error) {
if r, ok := p.repos[id]; ok {
return r, nil
}
return &worktree.Repository{ID: id}, nil
}
// recordingScriptHandler records every ExecuteSetupScript invocation and
// runs the script on disk (matching production DefaultScriptMessageHandler
// behaviour). The recorded calls let tests assert how many times the script
// handler is invoked per repo.
type recordingScriptHandler struct {
mu sync.Mutex
setupCalls []worktree.ScriptExecutionRequest
scriptError error
}
func (h *recordingScriptHandler) ExecuteSetupScript(ctx context.Context, req worktree.ScriptExecutionRequest) error {
h.mu.Lock()
h.setupCalls = append(h.setupCalls, req)
h.mu.Unlock()
if h.scriptError != nil {
return h.scriptError
}
cmd := exec.CommandContext(ctx, "sh", "-c", req.Script)
cmd.Dir = req.WorkingDir
return cmd.Run()
}
func (h *recordingScriptHandler) ExecuteCleanupScript(_ context.Context, _ worktree.ScriptExecutionRequest) error {
return nil
}
func (h *recordingScriptHandler) callsForRepo(id string) int {
h.mu.Lock()
defer h.mu.Unlock()
n := 0
for _, c := range h.setupCalls {
if c.RepositoryID == id {
n++
}
}
return n
}
func newPreparerWithScriptHandler(t *testing.T, repos map[string]*worktree.Repository) (*WorktreePreparer, *worktree.Manager, *recordingScriptHandler) {
t.Helper()
tmp := t.TempDir()
cfg := worktree.Config{
Enabled: true,
TasksBasePath: filepath.Join(tmp, "tasks"),
BranchPrefix: "feat/",
}
log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json"})
store := newInMemoryWorktreeStore()
mgr, err := worktree.NewManager(cfg, store, log)
if err != nil {
t.Fatalf("worktree manager: %v", err)
}
handler := &recordingScriptHandler{}
mgr.SetScriptMessageHandler(handler)
mgr.SetRepositoryProvider(&fakeRepoProvider{repos: repos})
return NewWorktreePreparer(mgr, log), mgr, handler
}
// TestWorktreePreparer_MultiRepo_RunsSetupScriptOncePerRepo ensures that when
// the worktree manager is wired with a script handler (production layout),
// each repo's setup script runs exactly once during a multi-repo prepare.
//
// Regression: the worktree manager runs the setup script via the script
// handler from inside Create(), and the env preparer used to also run the
// same per-repo setup script as a separate prepare step, causing the script
// to run twice per repo. For non-idempotent scripts (e.g. anything that
// echoes to a file with > or appends rows to a database) the second run
// produced unexpected state.
func TestWorktreePreparer_MultiRepo_RunsSetupScriptOncePerRepo(t *testing.T) {
repoA := initBareGitRepo(t, "frontend")
repoB := initBareGitRepo(t, "backend")
repos := map[string]*worktree.Repository{
"repo-front": {ID: "repo-front", SetupScript: "echo front >> setup-marker.txt"},
"repo-back": {ID: "repo-back", SetupScript: "echo back >> setup-marker.txt"},
}
preparer, _, handler := newPreparerWithScriptHandler(t, repos)
req := &EnvPrepareRequest{
TaskID: "task-multi-once",
SessionID: "sess-multi-once",
TaskTitle: "Once Task",
ExecutorType: executor.Name("worktree"),
TaskDirName: "once-task_xxx",
Repositories: []RepoPrepareSpec{
{
RepositoryID: "repo-front",
RepositoryPath: repoA,
RepoName: "frontend",
BaseBranch: "main",
RepoSetupScript: "echo front >> setup-marker.txt",
},
{
RepositoryID: "repo-back",
RepositoryPath: repoB,
RepoName: "backend",
BaseBranch: "main",
RepoSetupScript: "echo back >> setup-marker.txt",
},
},
}
res, err := preparer.Prepare(context.Background(), req, nil)
if err != nil {
t.Fatalf("prepare: %v", err)
}
if !res.Success {
t.Fatalf("expected success; err: %s; steps: %+v", res.ErrorMessage, res.Steps)
}
if got := handler.callsForRepo("repo-front"); got != 1 {
t.Errorf("repo-front: setup script invoked %d time(s) via handler, want 1", got)
}
if got := handler.callsForRepo("repo-back"); got != 1 {
t.Errorf("repo-back: setup script invoked %d time(s) via handler, want 1", got)
}
// Cross-check on disk: with proper single-execution semantics the marker
// file should have exactly one line per repo. A second execution from the
// env preparer would append a second line.
for _, w := range res.Worktrees {
marker := filepath.Join(w.WorktreePath, "setup-marker.txt")
data, statErr := os.ReadFile(marker)
if statErr != nil {
t.Errorf("repo %s: missing setup marker: %v", w.RepositoryID, statErr)
continue
}
// Each script writes one line of the form "front\n" or "back\n".
// Two executions would produce two lines.
if lines := splitNonEmpty(string(data)); len(lines) != 1 {
t.Errorf("repo %s: expected 1 marker line, got %d (%q)", w.RepositoryID, len(lines), string(data))
}
}
}
// TestWorktreePreparer_MultiRepo_NonIdempotentSetupScriptSucceeds reproduces
// the user-facing failure: when both repos have non-idempotent setup scripts
// (e.g. mkdir without -p), the duplicate execution makes the second run
// fail and the whole multi-repo prepare reports failure.
func TestWorktreePreparer_MultiRepo_NonIdempotentSetupScriptSucceeds(t *testing.T) {
repoA := initBareGitRepo(t, "frontend")
repoB := initBareGitRepo(t, "backend")
// `mkdir build` fails the second time it runs (directory already exists).
// `set -e` makes any failure abort the script with a non-zero exit code.
script := "set -e; mkdir build"
repos := map[string]*worktree.Repository{
"repo-front": {ID: "repo-front", SetupScript: script},
"repo-back": {ID: "repo-back", SetupScript: script},
}
preparer, _, _ := newPreparerWithScriptHandler(t, repos)
req := &EnvPrepareRequest{
TaskID: "task-multi-nonidempotent",
SessionID: "sess-multi-nonidempotent",
TaskTitle: "Non-Idempotent Task",
ExecutorType: executor.Name("worktree"),
TaskDirName: "nonidempotent_yyy",
Repositories: []RepoPrepareSpec{
{
RepositoryID: "repo-front",
RepositoryPath: repoA,
RepoName: "frontend",
BaseBranch: "main",
RepoSetupScript: script,
},
{
RepositoryID: "repo-back",
RepositoryPath: repoB,
RepoName: "backend",
BaseBranch: "main",
RepoSetupScript: script,
},
},
}
res, err := preparer.Prepare(context.Background(), req, nil)
if err != nil {
t.Fatalf("prepare returned hard error: %v", err)
}
if !res.Success {
t.Fatalf("expected prepare to succeed for both repos; got failure: %s", res.ErrorMessage)
}
if len(res.Worktrees) != 2 {
t.Fatalf("expected 2 worktrees, got %d", len(res.Worktrees))
}
}
// TestWorktreePreparer_SingleRepo_RunsSetupScriptOnce ensures the single-repo
// worktree path runs the per-repo setup script exactly once. The worktree
// manager runs it via its script handler from inside Create(); the env
// preparer must not re-run the same script (directly or via the default
// worktree prepare template that resolves {{repository.setup_script}}).
func TestWorktreePreparer_SingleRepo_RunsSetupScriptOnce(t *testing.T) {
repo := initBareGitRepo(t, "single")
repos := map[string]*worktree.Repository{
"repo-single": {ID: "repo-single", SetupScript: "echo single >> setup-marker.txt"},
}
preparer, _, handler := newPreparerWithScriptHandler(t, repos)
req := &EnvPrepareRequest{
TaskID: "task-single-once",
SessionID: "sess-single-once",
TaskTitle: "Single Once",
ExecutorType: executor.Name("worktree"),
TaskDirName: "single-once_zzz",
UseWorktree: true,
RepositoryID: "repo-single",
RepositoryPath: repo,
RepoName: "single",
BaseBranch: "main",
RepoSetupScript: "echo single >> setup-marker.txt",
}
res, err := preparer.Prepare(context.Background(), req, nil)
if err != nil {
t.Fatalf("prepare: %v", err)
}
if !res.Success {
t.Fatalf("expected success; err: %s; steps: %+v", res.ErrorMessage, res.Steps)
}
if got := handler.callsForRepo("repo-single"); got != 1 {
t.Errorf("repo-single: setup script invoked %d time(s) via handler, want 1", got)
}
// Cross-check on disk: with single-execution semantics the marker file
// should have exactly one line. A second execution from the env preparer
// (directly or via the default template) would append a second line.
marker := filepath.Join(res.WorkspacePath, "setup-marker.txt")
data, statErr := os.ReadFile(marker)
if statErr != nil {
t.Fatalf("missing setup marker at %s: %v", marker, statErr)
}
if lines := splitNonEmpty(string(data)); len(lines) != 1 {
t.Errorf("expected 1 marker line, got %d (%q)", len(lines), string(data))
}
}
// TestWorktreePreparer_SingleRepo_NonIdempotentSetupScriptSucceeds reproduces
// the user-facing failure for the single-repo worktree path: a non-idempotent
// setup script (e.g. "mkdir build") fails on a second run. The env preparer
// records setup failures as a non-fatal step rather than aborting the whole
// prepare, so duplicate execution surfaces as a "failed" step in the chat
// even though the agent eventually launches.
func TestWorktreePreparer_SingleRepo_NonIdempotentSetupScriptSucceeds(t *testing.T) {
repo := initBareGitRepo(t, "single")
script := "set -e; mkdir build"
repos := map[string]*worktree.Repository{
"repo-single": {ID: "repo-single", SetupScript: script},
}
preparer, _, _ := newPreparerWithScriptHandler(t, repos)
req := &EnvPrepareRequest{
TaskID: "task-single-nonidempotent",
SessionID: "sess-single-nonidempotent",
TaskTitle: "Single Non-Idempotent",
ExecutorType: executor.Name("worktree"),
TaskDirName: "single-nonidempotent_qqq",
UseWorktree: true,
RepositoryID: "repo-single",
RepositoryPath: repo,
RepoName: "single",
BaseBranch: "main",
RepoSetupScript: script,
}
res, err := preparer.Prepare(context.Background(), req, nil)
if err != nil {
t.Fatalf("prepare returned hard error: %v", err)
}
if !res.Success {
t.Fatalf("expected single-repo prepare to succeed; got failure: %s", res.ErrorMessage)
}
for _, s := range res.Steps {
if s.Status == PrepareStepFailed {
t.Errorf("unexpected failed step %q: %s (duplicate execution of non-idempotent script)", s.Name, s.Error)
}
}
}
func splitNonEmpty(s string) []string {
var out []string
for _, line := range strings.Split(s, "\n") {
if line != "" {
out = append(out, line)
}
}
return out
}
@@ -272,7 +272,13 @@ func TestWorktreePreparer_MultiRepo_RunsPerRepoSetupScript(t *testing.T) {
repoA := initBareGitRepo(t, "frontend")
repoB := initBareGitRepo(t, "backend")
preparer, _ := newPreparerForTest(t)
// Per-repo setup scripts are executed by the worktree manager during
// Create() through its script handler. Wire one up so the scripts run.
repos := map[string]*worktree.Repository{
"repo-front": {ID: "repo-front", SetupScript: "echo front-script-ran > setup-marker.txt"},
"repo-back": {ID: "repo-back", SetupScript: "echo back-script-ran > setup-marker.txt"},
}
preparer, _, _ := newPreparerWithScriptHandler(t, repos)
req := &EnvPrepareRequest{
TaskID: "task-multi-setup",
@@ -314,14 +320,4 @@ func TestWorktreePreparer_MultiRepo_RunsPerRepoSetupScript(t *testing.T) {
t.Errorf("repo %d setup marker missing: %v", i, err)
}
}
// Each repo should produce a "Run setup script (<name>)" step.
var setupStepsSeen int
for _, s := range res.Steps {
if strings.HasPrefix(s.Name, "Run setup script") {
setupStepsSeen++
}
}
if setupStepsSeen != 2 {
t.Errorf("expected 2 per-repo setup script steps, got %d", setupStepsSeen)
}
}
@@ -0,0 +1,93 @@
import { test, expect } from "../../fixtures/test-base";
import { SessionPage } from "../../pages/session-page";
/**
* Regression test for a duplicate-execution bug where the per-repo setup
* script (Repository.setup_script) ran twice during worktree prepare:
* 1. Once by worktree.Manager.Create via its script handler (correct).
* 2. Once by the env preparer when resolving the prepare script's
* {{repository.setup_script}} placeholder (wrong).
*
* For non-idempotent scripts this surfaces as the second run failing — the
* env preparer treats it as a non-fatal step, so the prepare panel ends in
* `completed_with_error` rather than `completed`. The fix blanks
* RepoSetupScript on the request copy used to resolve the prepare script,
* so the placeholder substitutes to empty and the second run is skipped.
*
* The unit-level coverage lives in
* apps/backend/internal/agent/lifecycle/env_preparer_worktree_dup_test.go;
* this test exercises the full HTTP + worktree manager + env preparer path.
*/
test.describe("Setup script no duplicate execution", () => {
test("non-idempotent repository setup_script runs exactly once on worktree task", async ({
testPage,
apiClient,
seedData,
}) => {
test.setTimeout(60_000);
// `mkdir <dir>` (without -p) fails with "File exists" on a second run.
// Each worktree gets its own working directory, so the directory only
// exists when the script ran twice in the same worktree — which is the
// exact symptom of the duplicate-execution bug.
const nonIdempotentScript = "mkdir build-once-marker";
await apiClient.updateRepository(seedData.repositoryId, {
setup_script: nonIdempotentScript,
});
try {
const task = await apiClient.createTaskWithAgent(
seedData.workspaceId,
"Setup Script No Duplicate",
seedData.agentProfileId,
{
description: "/e2e:simple-message",
workflow_id: seedData.workflowId,
workflow_step_id: seedData.startStepId,
repository_ids: [seedData.repositoryId],
// Default worktree profile uses the {{repository.setup_script}}
// placeholder, which is the path the bug lived on.
executor_profile_id: seedData.worktreeExecutorProfileId,
},
);
await testPage.goto(`/t/${task.id}`);
const session = new SessionPage(testPage);
await session.waitForLoad();
const panel = testPage.getByTestId("prepare-progress-panel");
await expect(panel).toBeVisible({ timeout: 30_000 });
// Wait for prepare to settle into a terminal state.
await expect(panel).toHaveAttribute("data-status", /completed|completed_with_error|failed/, {
timeout: 45_000,
});
// Expand to see step content if auto-collapsed.
if ((await panel.getAttribute("data-expanded")) === "false") {
await panel.getByTestId("prepare-progress-toggle").click();
}
const text = await panel.innerText();
// The setup script must be executed exactly once, by the worktree
// manager (recorded as a `script_execution` message). The env
// preparer must NOT also run it via its own "Run setup script"
// prepare step — that's the duplicate-execution path the fix
// closes. Two distinct assertions because the two runners surface
// through different channels (prepare panel vs. session messages).
expect(text).not.toContain("Run setup script");
expect(task.session_id).toBeTruthy();
const { messages } = await apiClient.listSessionMessages(task.session_id!);
const scriptMsgs = messages.filter(
(m) => (m as { type?: string }).type === "script_execution",
);
expect(scriptMsgs).toHaveLength(1);
} finally {
// Reset the seed repo so worker-scoped state stays clean for other
// tests sharing the same repository.
await apiClient.updateRepository(seedData.repositoryId, { setup_script: "" }).catch(() => {});
}
});
});