diff --git a/apps/backend/internal/agent/lifecycle/env_preparer_worktree.go b/apps/backend/internal/agent/lifecycle/env_preparer_worktree.go index bdb66c3732..b9c8cfd4b8 100644 --- a/apps/backend/internal/agent/lifecycle/env_preparer_worktree.go +++ b/apps/backend/internal/agent/lifecycle/env_preparer_worktree.go @@ -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) { diff --git a/apps/backend/internal/agent/lifecycle/env_preparer_worktree_dup_test.go b/apps/backend/internal/agent/lifecycle/env_preparer_worktree_dup_test.go new file mode 100644 index 0000000000..447f7caa39 --- /dev/null +++ b/apps/backend/internal/agent/lifecycle/env_preparer_worktree_dup_test.go @@ -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 +} diff --git a/apps/backend/internal/agent/lifecycle/env_preparer_worktree_multi_test.go b/apps/backend/internal/agent/lifecycle/env_preparer_worktree_multi_test.go index 9ecc2e16df..1863964db9 100644 --- a/apps/backend/internal/agent/lifecycle/env_preparer_worktree_multi_test.go +++ b/apps/backend/internal/agent/lifecycle/env_preparer_worktree_multi_test.go @@ -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 ()" 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) - } } diff --git a/apps/web/e2e/tests/session/setup-script-no-duplicate.spec.ts b/apps/web/e2e/tests/session/setup-script-no-duplicate.spec.ts new file mode 100644 index 0000000000..904c9a339f --- /dev/null +++ b/apps/web/e2e/tests/session/setup-script-no-duplicate.spec.ts @@ -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 ` (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(() => {}); + } + }); +});