diff --git a/apps/backend/internal/backendapp/branch_materializer_test.go b/apps/backend/internal/backendapp/branch_materializer_test.go index 94c1a46933..44ad30bde4 100644 --- a/apps/backend/internal/backendapp/branch_materializer_test.go +++ b/apps/backend/internal/backendapp/branch_materializer_test.go @@ -336,6 +336,9 @@ func seedMaterializerTask(t *testing.T, ctx context.Context, repo *sqliterepo.Re WorkspacePath: primaryPath, ExecutorType: "worktree", Status: "ready", CreatedAt: now, UpdatedAt: now, + Repos: []*models.TaskEnvironmentRepo{ + {ID: "env-1-repo-primary", RepositoryID: "repo-1", WorktreePath: primaryPath, CreatedAt: now}, + }, }); err != nil { t.Fatalf("CreateTaskEnvironment: %v", err) } diff --git a/apps/backend/internal/backendapp/e2e_reset.go b/apps/backend/internal/backendapp/e2e_reset.go index c638559c4b..afc84f8cbc 100644 --- a/apps/backend/internal/backendapp/e2e_reset.go +++ b/apps/backend/internal/backendapp/e2e_reset.go @@ -217,6 +217,17 @@ func handleE2EReset( return } + // Automation deletion must run while its bound tasks still exist. It + // stops live turns before removing run rows and owns cleanup of hidden + // automation tasks. Deleting tasks first can strand an open run on a + // missing task and make the next test's reset fail. + deletedAutomations, autoErr := deleteAutomationsForReset(ctx, automationSvc, workspaceID) + if autoErr != nil { + log.Error("e2e reset: failed to delete automations", zap.Error(autoErr)) + c.JSON(http.StatusInternalServerError, gin.H{errKey: autoErr.Error()}) + return + } + // Route through the task service (rather than a raw SQL DELETE) so // each delete spawns the async cleanup goroutine that stops the // agentctl instance and releases its port. Without this, instances @@ -259,13 +270,6 @@ func handleE2EReset( return } - deletedAutomations, autoErr := deleteAutomationsForReset(ctx, automationSvc, workspaceID) - if autoErr != nil { - log.Error("e2e reset: failed to delete automations", zap.Error(autoErr)) - c.JSON(http.StatusInternalServerError, gin.H{errKey: autoErr.Error()}) - return - } - c.JSON(http.StatusOK, gin.H{ "deleted_tasks": deletedTasks, "deleted_workflows": deletedWorkflows, diff --git a/apps/backend/internal/backendapp/helpers_test.go b/apps/backend/internal/backendapp/helpers_test.go index 48fe58ace7..8e4ce792ee 100644 --- a/apps/backend/internal/backendapp/helpers_test.go +++ b/apps/backend/internal/backendapp/helpers_test.go @@ -354,6 +354,17 @@ func TestResolveRepositoryIDForSubpathMatchesSanitizedRepositoryName(t *testing. if err := repo.CreateTaskEnvironment(ctx, &models.TaskEnvironment{ ID: "env-1", TaskID: "task-1", ExecutorType: "worktree", WorkspacePath: "/tmp", Status: models.TaskEnvironmentStatusReady, + Repos: []*models.TaskEnvironmentRepo{ + { + ID: "session-worktree-1", + RepositoryID: "repo-1", + WorktreeID: "worktree-1", + WorktreePath: "/tmp/worktree", + WorktreeBranch: "feature/test", + BranchSlug: "test", + Position: 0, + }, + }, }); err != nil { t.Fatalf("CreateTaskEnvironment: %v", err) } @@ -362,18 +373,6 @@ func TestResolveRepositoryIDForSubpathMatchesSanitizedRepositoryName(t *testing. "env-1", "session-1"); err != nil { t.Fatalf("link session to env: %v", err) } - if err := repo.CreateTaskEnvironmentRepo(ctx, &models.TaskEnvironmentRepo{ - ID: "session-worktree-1", - TaskEnvironmentID: "env-1", - RepositoryID: "repo-1", - WorktreeID: "worktree-1", - WorktreePath: "/tmp/worktree", - WorktreeBranch: "feature/test", - BranchSlug: "test", - Position: 0, - }); err != nil { - t.Fatalf("CreateTaskSessionWorktree: %v", err) - } got := resolveRepositoryIDForSessionSubpath(ctx, repo, "session-1", "kdlbs-kandev", log) if got != "repo-1" { diff --git a/apps/backend/internal/backendapp/workspace_source_materializer.go b/apps/backend/internal/backendapp/workspace_source_materializer.go index 0b54e138b0..88b054d2cd 100644 --- a/apps/backend/internal/backendapp/workspace_source_materializer.go +++ b/apps/backend/internal/backendapp/workspace_source_materializer.go @@ -32,6 +32,8 @@ const legacyLocalPCExecutor = "local_pc" type workspaceSourceMaterializerRepo interface { GetTaskEnvironmentByTaskID(context.Context, string) (*models.TaskEnvironment, error) UpdateTaskEnvironment(context.Context, *models.TaskEnvironment) error + ListTaskEnvironmentRepos(context.Context, string) ([]*models.TaskEnvironmentRepo, error) + CreateTaskEnvironmentRepo(context.Context, *models.TaskEnvironmentRepo) error ListTaskSessions(context.Context, string) ([]*models.TaskSession, error) ListTaskRepositories(context.Context, string) ([]*models.TaskRepository, error) ListTaskWorkspaceFolders(context.Context, string) ([]*models.TaskWorkspaceFolder, error) @@ -143,6 +145,9 @@ func (m *workspaceSourceMaterializer) materializeHostWorkspaceSources(ctx contex if materializeErr != nil { return nil, materializeErr } + if err = m.persistEnvironmentRepositoryInventory(ctx, state.environment.ID, state.environment.ExecutorType, batch, branchMaterializations); err != nil { + return nil, err + } if state.environment.WorkspacePath != materialization.root { state.environment.WorkspacePath, state.environment.UpdatedAt = materialization.root, time.Now().UTC() if err = m.repo.UpdateTaskEnvironment(ctx, state.environment); err != nil { @@ -490,9 +495,120 @@ func (m *workspaceSourceMaterializer) materializeRemoteWorkspaceSources(ctx cont if err != nil { return nil, err } + if err := m.persistEnvironmentRepositoryInventory(ctx, state.environment.ID, state.environment.ExecutorType, batch, nil); err != nil { + return nil, err + } return &taskservice.WorkspaceSourceMaterializationResult{WorkspacePath: state.environment.WorkspacePath, SessionIDs: ids}, nil } +// persistEnvironmentRepositoryInventory records the repository slots created +// by this attachment after their runtime materialization succeeds. The resume +// path validates task_repositories against this canonical inventory before it +// reuses an environment, so a live workspace-source attachment must publish +// its new slot before the next backend restart. +func (m *workspaceSourceMaterializer) persistEnvironmentRepositoryInventory( + ctx context.Context, + environmentID, executorType string, + batch *models.WorkspaceSourceBatch, + branchMaterializations []*branchMaterialization, +) error { + if batch == nil || environmentID == "" { + return nil + } + existing, err := m.loadEnvironmentRepositoryInventory(ctx, environmentID) + if err != nil { + return err + } + for _, row := range workspaceSourceInventoryRows(environmentID, executorType, batch, branchMaterializations, existing) { + if err := m.repo.CreateTaskEnvironmentRepo(ctx, row); err != nil { + return fmt.Errorf("persist task environment repository %q: %w", row.RepositoryID, err) + } + } + return nil +} + +func (m *workspaceSourceMaterializer) loadEnvironmentRepositoryInventory(ctx context.Context, environmentID string) (map[string]struct{}, error) { + rows, err := m.repo.ListTaskEnvironmentRepos(ctx, environmentID) + if err != nil { + return nil, fmt.Errorf("list task environment repository inventory: %w", err) + } + existing := make(map[string]struct{}, len(rows)) + for _, row := range rows { + if row != nil && row.RepositoryID != "" { + existing[environmentRepoInventoryKey(row.RepositoryID, row.BranchSlug)] = struct{}{} + } + } + return existing, nil +} + +func workspaceSourceInventoryRows( + environmentID, executorType string, + batch *models.WorkspaceSourceBatch, + branchMaterializations []*branchMaterialization, + existing map[string]struct{}, +) []*models.TaskEnvironmentRepo { + materialized := make(map[string]*branchMaterialization, len(branchMaterializations)) + for _, branch := range branchMaterializations { + if branch != nil && branch.repositoryID != "" { + materialized[branch.repositoryID] = branch + } + } + rows := make([]*models.TaskEnvironmentRepo, 0, len(batch.Sources)) + for _, source := range batch.Sources { + row, ok := workspaceSourceInventoryRow(environmentID, executorType, source.Repository, materialized) + if !ok { + continue + } + if _, found := existing[environmentRepoInventoryKey(row.RepositoryID, row.BranchSlug)]; found { + continue + } + rows = append(rows, row) + } + return rows +} + +func workspaceSourceInventoryRow( + environmentID, executorType string, + taskRepository *models.TaskRepository, + materialized map[string]*branchMaterialization, +) (*models.TaskEnvironmentRepo, bool) { + if taskRepository == nil || taskRepository.RepositoryID == "" { + return nil, false + } + branchSlug := workspaceSourceBranchSlug(taskRepository) + branch := materialized[taskRepository.RepositoryID] + if executorType == string(models.ExecutorTypeWorktree) && branch == nil { + // A worktree source that was not materialized has no physical checkout + // to publish. The next launch must prepare it first. + return nil, false + } + row := &models.TaskEnvironmentRepo{ + TaskEnvironmentID: environmentID, + RepositoryID: taskRepository.RepositoryID, + BranchSlug: branchSlug, + Position: taskRepository.Position, + } + if branch != nil && branch.worktree != nil { + row.WorktreeID = branch.worktree.ID + row.WorktreePath = branch.worktree.Path + row.WorktreeBranch = branch.worktree.Branch + row.BranchSlug = branch.slug + } + return row, true +} + +func workspaceSourceBranchSlug(taskRepository *models.TaskRepository) string { + branch := taskRepository.CheckoutBranch + if branch == "" { + branch = taskRepository.BaseBranch + } + return worktree.SanitizeBranchSlug(branch) +} + +func environmentRepoInventoryKey(repositoryID, branchSlug string) string { + return repositoryID + "\x00" + branchSlug +} + // buildRemoteWorkspaceRepositoryBatch projects only the sources created by // this attachment operation. Existing durable siblings belong in a fresh // launch/resume projection, but revalidating them in a live execution would diff --git a/apps/backend/internal/backendapp/workspace_source_materializer_test.go b/apps/backend/internal/backendapp/workspace_source_materializer_test.go index ab977aebc3..ce52350a3b 100644 --- a/apps/backend/internal/backendapp/workspace_source_materializer_test.go +++ b/apps/backend/internal/backendapp/workspace_source_materializer_test.go @@ -174,6 +174,20 @@ func TestWorkspaceSourceMaterializer_RemoteMaterializesAdditionalRepositories(t if len(remote.calls) != 1 || len(remote.calls[0]) != 1 || remote.calls[0][0].Destination != "added-main" { t.Fatalf("remote projection=%+v; want only additional repository", remote.calls) } + inventory, err := repo.ListTaskEnvironmentRepos(ctx, env.ID) + if err != nil { + t.Fatalf("ListTaskEnvironmentRepos: %v", err) + } + found := false + for _, row := range inventory { + if row.RepositoryID == "repo-added" && row.BranchSlug == "main" { + found = true + break + } + } + if !found { + t.Fatalf("environment inventory = %+v; want repo-added/main", inventory) + } }) } } diff --git a/apps/backend/internal/orchestrator/event_handlers_git_test.go b/apps/backend/internal/orchestrator/event_handlers_git_test.go index 8997324876..be68259ba3 100644 --- a/apps/backend/internal/orchestrator/event_handlers_git_test.go +++ b/apps/backend/internal/orchestrator/event_handlers_git_test.go @@ -221,6 +221,12 @@ func seedBranchSwitchSession(t *testing.T, startBranch string) (*Service, *mockG if err := testRepo.CreateTaskEnvironment(ctx, &models.TaskEnvironment{ ID: "env-s1", TaskID: "t1", ExecutorType: "worktree", WorkspacePath: "/tmp", Status: models.TaskEnvironmentStatusReady, + Repos: []*models.TaskEnvironmentRepo{ + { + ID: "wt-s1", WorktreeID: "wtree-s1", RepositoryID: "repo1", + WorktreeBranch: startBranch, CreatedAt: now, + }, + }, }); err != nil { t.Fatalf("create environment: %v", err) } @@ -233,14 +239,6 @@ func seedBranchSwitchSession(t *testing.T, startBranch string) (*Service, *mockG if err := testRepo.UpdateTaskSession(ctx, session); err != nil { t.Fatalf("link session to environment: %v", err) } - wt := &models.TaskEnvironmentRepo{ - ID: "wt-s1", TaskEnvironmentID: "env-s1", - WorktreeID: "wtree-s1", RepositoryID: "repo1", - WorktreeBranch: startBranch, CreatedAt: now, - } - if err := testRepo.CreateTaskEnvironmentRepo(ctx, wt); err != nil { - t.Fatalf("create worktree: %v", err) - } svc := createTestService(testRepo, newMockStepGetter(), newMockTaskRepo()) ghSvc := &mockGitHubService{} diff --git a/apps/backend/internal/orchestrator/event_handlers_github_test.go b/apps/backend/internal/orchestrator/event_handlers_github_test.go index da445618eb..ca37d518fa 100644 --- a/apps/backend/internal/orchestrator/event_handlers_github_test.go +++ b/apps/backend/internal/orchestrator/event_handlers_github_test.go @@ -559,6 +559,15 @@ func TestCheckSessionPR(t *testing.T) { if err := repo.CreateTaskEnvironment(ctx, &models.TaskEnvironment{ ID: "env1", TaskID: "t1", ExecutorType: "worktree", WorkspacePath: "/tmp", Status: models.TaskEnvironmentStatusReady, + Repos: []*models.TaskEnvironmentRepo{ + { + ID: "wt1", + WorktreeID: "wtree1", + RepositoryID: "repo1", + WorktreeBranch: branch, + CreatedAt: now, + }, + }, }); err != nil { t.Fatalf("failed to create environment: %v", err) } @@ -570,17 +579,6 @@ func TestCheckSessionPR(t *testing.T) { if err := repo.UpdateTaskSession(ctx, session); err != nil { t.Fatalf("link session to environment: %v", err) } - wt := &models.TaskEnvironmentRepo{ - ID: "wt1", - TaskEnvironmentID: "env1", - WorktreeID: "wtree1", - RepositoryID: "repo1", - WorktreeBranch: branch, - CreatedAt: now, - } - if err := repo.CreateTaskEnvironmentRepo(ctx, wt); err != nil { - t.Fatalf("failed to create worktree: %v", err) - } svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo()) return svc @@ -1205,6 +1203,10 @@ func TestListTasksNeedingPRWatch(t *testing.T) { if err := testRepo.CreateTaskEnvironment(ctx, &models.TaskEnvironment{ ID: "env-s1", TaskID: "t1", ExecutorType: "worktree", WorkspacePath: "/tmp", Status: models.TaskEnvironmentStatusReady, + Repos: []*models.TaskEnvironmentRepo{ + {ID: "wt-1", WorktreeID: "wtree-1", RepositoryID: "repo-front", WorktreeBranch: "feat/frontend", CreatedAt: now}, + {ID: "wt-2", WorktreeID: "wtree-2", RepositoryID: "repo-back", WorktreeBranch: "feat/backend", CreatedAt: now}, + }, }); err != nil { t.Fatalf("create environment: %v", err) } @@ -1216,15 +1218,6 @@ func TestListTasksNeedingPRWatch(t *testing.T) { if err := testRepo.UpdateTaskSession(ctx, session); err != nil { t.Fatalf("link session to environment: %v", err) } - worktrees := []*models.TaskEnvironmentRepo{ - {ID: "wt-1", TaskEnvironmentID: "env-s1", WorktreeID: "wtree-1", RepositoryID: "repo-front", WorktreeBranch: "feat/frontend", CreatedAt: now}, - {ID: "wt-2", TaskEnvironmentID: "env-s1", WorktreeID: "wtree-2", RepositoryID: "repo-back", WorktreeBranch: "feat/backend", CreatedAt: now}, - } - for _, wt := range worktrees { - if err := testRepo.CreateTaskEnvironmentRepo(ctx, wt); err != nil { - t.Fatalf("worktree %s: %v", wt.ID, err) - } - } svc := createTestService(testRepo, newMockStepGetter(), newMockTaskRepo()) // Mock the watch list as if frontend already has a watch (the @@ -1300,6 +1293,10 @@ func TestEnsureSessionPRWatch_MultiRepo(t *testing.T) { if err := repo.CreateTaskEnvironment(ctx, &models.TaskEnvironment{ ID: "env-s1", TaskID: "t1", ExecutorType: "worktree", WorkspacePath: "/tmp", Status: models.TaskEnvironmentStatusReady, + Repos: []*models.TaskEnvironmentRepo{ + {ID: "wt-1", WorktreeID: "wtree-1", RepositoryID: "repo-front", WorktreeBranch: "feat/frontend", CreatedAt: now}, + {ID: "wt-2", WorktreeID: "wtree-2", RepositoryID: "repo-back", WorktreeBranch: "feat/backend", CreatedAt: now}, + }, }); err != nil { t.Fatalf("create environment: %v", err) } @@ -1311,14 +1308,6 @@ func TestEnsureSessionPRWatch_MultiRepo(t *testing.T) { if err := repo.UpdateTaskSession(ctx, session); err != nil { t.Fatalf("link session to environment: %v", err) } - for _, wt := range []*models.TaskEnvironmentRepo{ - {ID: "wt-1", TaskEnvironmentID: "env-s1", WorktreeID: "wtree-1", RepositoryID: "repo-front", WorktreeBranch: "feat/frontend", CreatedAt: now}, - {ID: "wt-2", TaskEnvironmentID: "env-s1", WorktreeID: "wtree-2", RepositoryID: "repo-back", WorktreeBranch: "feat/backend", CreatedAt: now}, - } { - if err := repo.CreateTaskEnvironmentRepo(ctx, wt); err != nil { - t.Fatalf("worktree: %v", err) - } - } svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo()) ghSvc := &mockGitHubService{} diff --git a/apps/backend/internal/orchestrator/executor/executor_environment_reuse.go b/apps/backend/internal/orchestrator/executor/executor_environment_reuse.go index f76f5db01a..5bff0f0017 100644 --- a/apps/backend/internal/orchestrator/executor/executor_environment_reuse.go +++ b/apps/backend/internal/orchestrator/executor/executor_environment_reuse.go @@ -40,9 +40,25 @@ func (e *Executor) validateReuseEnvironmentInventory(ctx context.Context, req *L // Reuse setup below consumes this same inventory, avoiding a second query // and keeping cancellation attached to the caller's context. env.Repos = rows + // Zero recorded rows means the canonical inventory was never captured at + // all (for example: a launch whose prepare step failed before writing + // any repo rows). That is recoverable — letting this launch through lets + // reuseExistingRepositoryWorktrees fall through its own empty-inventory + // check and rebuild fresh worktree/repo specs. A non-empty but wrong + // inventory below is the guard's actual purpose and must still refuse. + if len(rows) == 0 { + req.WorkspaceReuseRequired = workspaceReuseAllowed( + env, + req.ExecutorType, + req.WorkspaceReuseRequired, + e.taskIsRepoBacked(ctx, req.TaskID), + ) + return nil + } for _, spec := range specs { if canonicalInventoryMatches(spec, rows, req.UseWorktree) != 1 { - return fmt.Errorf("%w: canonical workspace repository inventory is incomplete", models.ErrWorkspaceReuseUnsafe) + return fmt.Errorf("%w: canonical workspace repository inventory has no matching entry for repository %q branch %q", + models.ErrWorkspaceReuseUnsafe, spec.RepositoryID, launchRepoBranchIdentitySlug(spec)) } } return nil @@ -97,14 +113,24 @@ func (e *Executor) reuseExistingEnvironment(ctx context.Context, req *LaunchAgen zap.String("req_executor_type", req.ExecutorType)) return } + // A repo-backed environment with no canonical rows is being freshly + // materialized. Do not forward any environment or sibling runtime handle: + // those handles could reconnect to the incomplete workspace, and + // PreviousExecutionID would route the new session through the resume path. + if !req.WorkspaceReuseRequired && e.taskIsRepoBacked(ctx, req.TaskID) && len(env.Repos) == 0 { + return + } if env.TaskDirName != "" && req.UseWorktree { req.TaskDirName = env.TaskDirName } // SSH uses the remote task directory as an environment-scoped attachment // handle. It is distinct from the per-session agentctl directory and must - // therefore be forwarded for a sibling session without adopting its runtime. - if req.ExecutorType == string(models.ExecutorTypeSSH) && env.WorkspacePath != "" { + // therefore be forwarded for a sibling session without adopting its + // runtime. Only forward it when reuse is actually required: a launch that + // fell through to full materialization (see workspaceReuseAllowed) must + // not see the old, possibly incomplete or stale, workspace path. + if req.ExecutorType == string(models.ExecutorTypeSSH) && env.WorkspacePath != "" && req.WorkspaceReuseRequired { ensureLaunchMetadata(req)[lifecycle.MetadataKeySSHRemoteTaskDir] = env.WorkspacePath } diff --git a/apps/backend/internal/orchestrator/executor/executor_environment_reuse_inventory_test.go b/apps/backend/internal/orchestrator/executor/executor_environment_reuse_inventory_test.go new file mode 100644 index 0000000000..91e308b05c --- /dev/null +++ b/apps/backend/internal/orchestrator/executor/executor_environment_reuse_inventory_test.go @@ -0,0 +1,59 @@ +package executor + +import ( + "context" + "errors" + "testing" + + "github.com/kandev/kandev/internal/task/models" +) + +// Regression coverage for the "failed prepare leaves a permanently bricked +// workspace" defect: when a task_environments row was published ready with +// zero task_environment_repos rows (the write-side bug, fixed separately), +// validateReuseEnvironmentInventory used to treat that as a mismatch and +// refuse every subsequent launch forever. Zero rows recorded means the +// canonical inventory was never captured at all, which is recoverable by +// letting the launch rebuild it — distinct from a non-empty but wrong +// inventory, which must still be refused. +func TestValidateReuseEnvironmentInventory_ZeroRowsIsRecoverable(t *testing.T) { + repo := newMockRepository() + repo.taskRepositories["task-repo-1"] = &models.TaskRepository{ID: "task-repo-1", TaskID: "task-1", RepositoryID: "repo-1"} + e := newTestExecutor(t, &mockAgentManager{}, repo) + req := &LaunchAgentRequest{ + TaskID: "task-1", + WorkspaceReuseRequired: true, + RepositoryID: "repo-1", + } + env := &models.TaskEnvironment{ID: "env-1"} + + if err := e.validateReuseEnvironmentInventory(context.Background(), req, env); err != nil { + t.Fatalf("validateReuseEnvironmentInventory() with zero recorded rows = %v, want nil (recoverable)", err) + } + if req.WorkspaceReuseRequired { + t.Fatal("zero inventory kept WorkspaceReuseRequired=true, want fresh materialization") + } +} + +// The read-side fix must not weaken the guard's actual purpose: a non-empty +// but mismatched canonical inventory (wrong repository, wrong branch, or a +// row explicitly marked failed/deleted) is still an unsafe reuse and must be +// refused. +func TestValidateReuseEnvironmentInventory_MismatchedRowsStillRefused(t *testing.T) { + repo := newMockRepository() + e := newTestExecutor(t, &mockAgentManager{}, repo) + req := &LaunchAgentRequest{ + TaskID: "task-1", + WorkspaceReuseRequired: true, + RepositoryID: "repo-1", + } + env := &models.TaskEnvironment{ID: "env-1"} + repo.taskEnvironmentRepos[env.ID] = []*models.TaskEnvironmentRepo{ + {RepositoryID: "repo-other", WorktreeID: "worktree-other"}, + } + + err := e.validateReuseEnvironmentInventory(context.Background(), req, env) + if !errors.Is(err, models.ErrWorkspaceReuseUnsafe) { + t.Fatalf("validateReuseEnvironmentInventory() with mismatched rows = %v, want ErrWorkspaceReuseUnsafe", err) + } +} diff --git a/apps/backend/internal/orchestrator/executor/executor_environment_test.go b/apps/backend/internal/orchestrator/executor/executor_environment_test.go index 5c9d450a9c..a9012f951e 100644 --- a/apps/backend/internal/orchestrator/executor/executor_environment_test.go +++ b/apps/backend/internal/orchestrator/executor/executor_environment_test.go @@ -140,23 +140,42 @@ func TestReuseExistingEnvironment_WorktreeSkippedWhenNotRequested(t *testing.T) } func TestWorkspaceReuseAllowedRequiresMatchingExecutorType(t *testing.T) { - env := &models.TaskEnvironment{ExecutorType: string(models.ExecutorTypeLocal)} - if workspaceReuseAllowed(env, string(models.ExecutorTypeWorktree), true) { + env := &models.TaskEnvironment{ + ExecutorType: string(models.ExecutorTypeLocal), + Repos: []*models.TaskEnvironmentRepo{{RepositoryID: "repo-1"}}, + } + if workspaceReuseAllowed(env, string(models.ExecutorTypeWorktree), true, true) { t.Fatal("workspace reuse should be disabled when the executor type changes") } - if !workspaceReuseAllowed(env, string(models.ExecutorTypeLocal), true) { + if !workspaceReuseAllowed(env, string(models.ExecutorTypeLocal), true, true) { t.Fatal("workspace reuse should remain enabled for the owning executor type") } - if workspaceReuseAllowed(&models.TaskEnvironment{}, string(models.ExecutorTypeWorktree), true) { + if workspaceReuseAllowed(&models.TaskEnvironment{}, string(models.ExecutorTypeWorktree), true, true) { t.Fatal("legacy environments without a physical worktree should not be reused by worktree launches") } if !workspaceReuseAllowed(&models.TaskEnvironment{ Repos: []*models.TaskEnvironmentRepo{{WorktreeID: "legacy-worktree", Status: "active"}}, - }, string(models.ExecutorTypeWorktree), true) { + }, string(models.ExecutorTypeWorktree), true, true) { t.Fatal("legacy environments with a physical worktree should remain reusable") } } +func TestWorkspaceReuseAllowedRequiresInventoryForRepoBackedExecutors(t *testing.T) { + for _, executorType := range []string{ + string(models.ExecutorTypeLocal), + string(models.ExecutorTypeLocalDocker), + string(models.ExecutorTypeSSH), + string(models.ExecutorTypeWorktree), + } { + t.Run(executorType, func(t *testing.T) { + env := &models.TaskEnvironment{ExecutorType: executorType} + if workspaceReuseAllowed(env, executorType, true, true) { + t.Fatalf("workspace reuse allowed for %s with empty repository inventory", executorType) + } + }) + } +} + func TestWaitForTaskEnvironmentReadyWaitsForMaterializer(t *testing.T) { repo := newMockRepository() env := &models.TaskEnvironment{ID: "env-wait", Status: models.TaskEnvironmentStatusCreating} @@ -348,6 +367,156 @@ func TestPersistTaskEnvironment_FinalizesCreatingEnvironmentWithInventory(t *tes } } +func TestPersistTaskEnvironment_RestoresMaterializationClaimOnFinalizeFailure(t *testing.T) { + repo := newMockRepository() + persistErr := errors.New("finalize failed") + repo.finalizeTaskEnvironmentErr = persistErr + env := &models.TaskEnvironment{ + ID: "env-1", + TaskID: "task-1", + ExecutorType: string(models.ExecutorTypeWorktree), + Status: models.TaskEnvironmentStatusCreating, + MaterializationSessionID: "session-1", + } + repo.taskEnvironments[env.ID] = env + e := newTestExecutor(t, &mockAgentManager{}, repo) + session := &models.TaskSession{ID: "session-1", TaskID: "task-1", TaskEnvironmentID: env.ID} + + err := e.persistTaskEnvironment(context.Background(), "task-1", session, env, + &LaunchAgentRequest{TaskID: "task-1", ExecutorType: string(models.ExecutorTypeWorktree)}, + &LaunchAgentResponse{WorktreePath: "/tasks/task-1/repo", Worktrees: []RepoWorktreeResult{{ + RepositoryID: "repo-1", WorktreeID: "wt-1", WorktreePath: "/tasks/task-1/repo", WorktreeBranch: "feature/task-1", + }}}, + executorConfig{ExecutorID: models.ExecutorIDWorktree}) + + if !errors.Is(err, persistErr) { + t.Fatalf("persistTaskEnvironment error = %v, want %v", err, persistErr) + } + if env.Status != models.TaskEnvironmentStatusCreating || env.MaterializationSessionID != session.ID { + t.Fatalf("materialization claim = status %q owner %q, want creating with original owner", env.Status, env.MaterializationSessionID) + } +} + +// TestPersistTaskEnvironment_NonMaterializerSiblingPersistsReposBeforeReady +// pins the fix for the "ready status precedes inventory commit" hazard: a +// non-initial-materializer sibling must write its per-repo rows before +// flipping the environment to ready, so a crash or concurrent read between +// the two writes can never observe a ready environment whose canonical +// inventory is still empty or stale. +func TestPersistTaskEnvironment_NonMaterializerSiblingPersistsReposBeforeReady(t *testing.T) { + repo := newMockRepository() + env := &models.TaskEnvironment{ + ID: "env-1", + TaskID: "task-1", + ExecutorType: string(models.ExecutorTypeWorktree), + Status: models.TaskEnvironmentStatusReady, + } + repo.taskEnvironments[env.ID] = env + repo.taskRepositories["task-repo-1"] = &models.TaskRepository{ID: "task-repo-1", TaskID: "task-1", RepositoryID: "repo-1"} + e := newTestExecutor(t, &mockAgentManager{}, repo) + session := &models.TaskSession{ID: "session-2", TaskID: "task-1", TaskEnvironmentID: env.ID} + + if err := e.persistTaskEnvironment(context.Background(), "task-1", session, env, + &LaunchAgentRequest{TaskID: "task-1", ExecutorType: string(models.ExecutorTypeWorktree)}, + &LaunchAgentResponse{WorktreePath: "/tasks/task-1/repo", Worktrees: []RepoWorktreeResult{{ + RepositoryID: "repo-1", WorktreeID: "wt-2", WorktreePath: "/tasks/task-1/repo", WorktreeBranch: "feature/task-1", + }}}, + executorConfig{ExecutorID: models.ExecutorIDWorktree}); err != nil { + t.Fatalf("persistTaskEnvironment: %v", err) + } + + if len(repo.writeCallLog) < 2 { + t.Fatalf("write call log = %v, want at least a repo write followed by an env update", repo.writeCallLog) + } + repoIdx, envIdx := -1, -1 + for i, call := range repo.writeCallLog { + switch call { + case "create_repo": + if repoIdx == -1 { + repoIdx = i + } + case "update_env": + if envIdx == -1 { + envIdx = i + } + } + } + if repoIdx == -1 || envIdx == -1 || repoIdx > envIdx { + t.Fatalf("write order = %v, want create_repo before update_env", repo.writeCallLog) + } + if got := repo.taskEnvironments[env.ID].Status; got != models.TaskEnvironmentStatusReady { + t.Fatalf("status = %q, want ready once inventory is persisted", got) + } +} + +func TestPersistTaskEnvironment_LeavesEnvironmentUnchangedOnRepoWriteFailure(t *testing.T) { + repo := newMockRepository() + persistErr := errors.New("inventory write failed") + repo.createTaskEnvironmentRepoErr = persistErr + env := &models.TaskEnvironment{ + ID: "env-1", + TaskID: "task-1", + ExecutorType: string(models.ExecutorTypeWorktree), + Status: models.TaskEnvironmentStatusReady, + } + repo.taskEnvironments[env.ID] = env + repo.taskRepositories["task-repo-1"] = &models.TaskRepository{ID: "task-repo-1", TaskID: "task-1", RepositoryID: "repo-1"} + e := newTestExecutor(t, &mockAgentManager{}, repo) + session := &models.TaskSession{ID: "session-2", TaskID: "task-1", TaskEnvironmentID: env.ID} + + err := e.persistTaskEnvironment(context.Background(), "task-1", session, env, + &LaunchAgentRequest{TaskID: "task-1", ExecutorType: string(models.ExecutorTypeWorktree)}, + &LaunchAgentResponse{WorktreePath: "/tasks/task-1/repo", Worktrees: []RepoWorktreeResult{{ + RepositoryID: "repo-1", WorktreeID: "wt-2", WorktreePath: "/tasks/task-1/repo", WorktreeBranch: "feature/task-1", + }}}, + executorConfig{ExecutorID: models.ExecutorIDWorktree}) + + if !errors.Is(err, persistErr) { + t.Fatalf("persistTaskEnvironment error = %v, want %v", err, persistErr) + } + if session.TaskEnvironmentID != env.ID { + t.Fatalf("session TaskEnvironmentID = %q, want unchanged %q", session.TaskEnvironmentID, env.ID) + } + if len(repo.updateTaskEnvironmentCalls) != 0 { + t.Fatalf("UpdateTaskEnvironment calls = %d, want none after inventory failure", len(repo.updateTaskEnvironmentCalls)) + } +} + +// TestPersistTaskEnvironment_NonMaterializerSiblingWithEmptyReposDoesNotSetReady +// covers the companion invariant: when this sibling's own launch produced no +// repos and the environment had none recorded either, a repo-backed task's +// environment must not be forced to ready — that would publish an empty +// canonical inventory that permanently bricks reuse. +func TestPersistTaskEnvironment_NonMaterializerSiblingWithEmptyReposDoesNotSetReady(t *testing.T) { + repo := newMockRepository() + env := &models.TaskEnvironment{ + ID: "env-1", + TaskID: "task-1", + ExecutorType: string(models.ExecutorTypeSSH), + Status: models.TaskEnvironmentStatusCreating, + } + repo.taskEnvironments[env.ID] = env + repo.taskRepositories["task-repo-1"] = &models.TaskRepository{ID: "task-repo-1", TaskID: "task-1", RepositoryID: "repo-1"} + e := newTestExecutor(t, &mockAgentManager{}, repo) + session := &models.TaskSession{ID: "session-2", TaskID: "task-1", TaskEnvironmentID: env.ID} + + if err := e.persistTaskEnvironment(context.Background(), "task-1", session, env, + &LaunchAgentRequest{TaskID: "task-1", ExecutorType: string(models.ExecutorTypeSSH)}, + &LaunchAgentResponse{}, + executorConfig{ExecutorID: "ssh"}); err != nil { + t.Fatalf("persistTaskEnvironment: %v", err) + } + + for _, call := range repo.writeCallLog { + if call == "create_repo" { + t.Fatalf("write call log = %v, want no repo rows created for an empty prepare result", repo.writeCallLog) + } + } + if got := repo.taskEnvironments[env.ID].Status; got != models.TaskEnvironmentStatusCreating { + t.Fatalf("status = %q, want status left untouched instead of forced to ready with empty inventory", got) + } +} + func TestReuseExistingEnvironment_DockerBranchReuse(t *testing.T) { e := newEnvTestExecutor(t) req := &LaunchAgentRequest{TaskID: "task-1", ExecutorType: "local_docker"} @@ -671,6 +840,87 @@ func TestReuseExistingEnvironment_ReuseRequiredSSHUsesCanonicalRemoteTaskDir(t * } } +// TestReuseExistingEnvironment_ReuseNotRequiredSSHSkipsStaleRemoteTaskDir +// pins the fix for the empty-inventory SSH reuse hazard: when +// workspaceReuseAllowed has already forced a fresh materialization (reuse +// not required), reuseExistingEnvironment must not forward the old +// env.WorkspacePath, or the SSH executor's attach-only path would trust a +// possibly incomplete or stale checkout instead of preparing a fresh one. +func TestReuseExistingEnvironment_ReuseNotRequiredSSHSkipsStaleRemoteTaskDir(t *testing.T) { + e := newTestExecutor(t, &mockAgentManager{}, newMockRepository()) + req := &LaunchAgentRequest{ + TaskID: "task-1", + ExecutorType: string(models.ExecutorTypeSSH), + WorkspaceReuseRequired: false, + } + env := &models.TaskEnvironment{ + ID: "environment-1", + ExecutorType: string(models.ExecutorTypeSSH), + WorkspacePath: "/home/kandev/.kandev/tasks/task-1", + } + + e.reuseExistingEnvironment(context.Background(), req, env) + + if got, ok := req.Metadata[lifecycle.MetadataKeySSHRemoteTaskDir]; ok { + t.Fatalf("remote task directory = %v, want unset for a non-reuse launch", got) + } +} + +func TestReuseExistingEnvironment_FreshRepoRecoveryDoesNotAdoptSiblingExecution(t *testing.T) { + repo := newMockRepository() + repo.taskRepositories["task-repo-1"] = &models.TaskRepository{ + ID: "task-repo-1", TaskID: "task-1", RepositoryID: "repo-1", + } + repo.sessions["session-old"] = &models.TaskSession{ + ID: "session-old", TaskID: "task-1", TaskEnvironmentID: "env-1", + } + repo.executorsRunning["session-old"] = &models.ExecutorRunning{ + SessionID: "session-old", + AgentExecutionID: "exec-old", + } + e := newTestExecutor(t, &mockAgentManager{}, repo) + req := &LaunchAgentRequest{ + TaskID: "task-1", + ExecutorType: string(models.ExecutorTypeSSH), + WorkspaceReuseRequired: false, + } + env := &models.TaskEnvironment{ID: "env-1", TaskID: "task-1", ExecutorType: string(models.ExecutorTypeSSH)} + + e.reuseExistingEnvironment(context.Background(), req, env) + + if req.PreviousExecutionID != "" { + t.Fatalf("PreviousExecutionID = %q, want empty during fresh repo recovery", req.PreviousExecutionID) + } +} + +func TestReuseExistingEnvironment_FreshRepoRecoveryDropsContainerHandle(t *testing.T) { + repo := newMockRepository() + repo.taskRepositories["task-repo-1"] = &models.TaskRepository{ + ID: "task-repo-1", TaskID: "task-1", RepositoryID: "repo-1", + } + e := newTestExecutor(t, &mockAgentManager{}, repo) + req := &LaunchAgentRequest{ + TaskID: "task-1", + ExecutorType: string(models.ExecutorTypeLocalDocker), + WorkspaceReuseRequired: false, + } + env := &models.TaskEnvironment{ + ID: "env-1", + TaskID: "task-1", + ExecutorType: string(models.ExecutorTypeLocalDocker), + ContainerID: "container-stale", + } + + e.reuseExistingEnvironment(context.Background(), req, env) + + if req.PreviousExecutionID != "" { + t.Fatalf("PreviousExecutionID = %q, want empty during fresh repo recovery", req.PreviousExecutionID) + } + if req.Metadata != nil { + t.Fatalf("metadata = %#v, want no stale container handle during fresh recovery", req.Metadata) + } +} + // TestApplyExecutorRunningMetadata_SkipsSessionScopedKeys pins the guard // that prevents a SECOND session on the same task from inheriting the FIRST // session's session-scoped runtime resources — agentctl PID/port, remote diff --git a/apps/backend/internal/orchestrator/executor/executor_execute.go b/apps/backend/internal/orchestrator/executor/executor_execute.go index 5e3c608533..0620404900 100644 --- a/apps/backend/internal/orchestrator/executor/executor_execute.go +++ b/apps/backend/internal/orchestrator/executor/executor_execute.go @@ -351,6 +351,23 @@ func (e *Executor) cleanupUnstartedExecutionAfterPersistError( e.stopUnstartedExecution(ctx, sessionID, agentExecutionID) } +func (e *Executor) markTaskEnvironmentMaterializationFailed( + ctx context.Context, + env *models.TaskEnvironment, + sessionID string, +) { + if env == nil || env.Status != models.TaskEnvironmentStatusCreating || env.MaterializationSessionID != sessionID { + return + } + env.Status = models.TaskEnvironmentStatusFailed + env.MaterializationSessionID = "" + if err := e.repo.UpdateTaskEnvironment(ctx, env); err != nil { + e.logger.Warn("failed to mark workspace materialization failed", + zap.String("task_environment_id", env.ID), + zap.Error(err)) + } +} + func (e *Executor) writeTaskReviewStateIfNoWorkingSessions(ctx context.Context, taskID, failedSessionID string) { if e.onTaskReviewStateReconcile != nil { e.onTaskReviewStateReconcile(ctx, taskID, failedSessionID) @@ -1288,7 +1305,12 @@ func (e *Executor) LaunchPreparedSession(ctx context.Context, task *v1.Task, ses } // Create or update the task environment with launch results - e.persistTaskEnvironment(ctx, task.ID, session, existingEnv, req, resp, execCfg) + if err := e.persistTaskEnvironment(ctx, task.ID, session, existingEnv, req, resp, execCfg); err != nil { + e.cleanupUnstartedExecutionAfterPersistError(ctx, sessionID, resp.AgentExecutionID, err) + e.markTaskEnvironmentMaterializationFailed(ctx, existingEnv, session.ID) + repositoryID, taskRepositoryID := failingLaunchRepositoryIdentity(req, err) + return nil, e.handleLaunchFailure(ctx, task.ID, sessionID, repositoryID, taskRepositoryID, err) + } // Capture the current HEAD commit as the base commit for this session asynchronously. // This allows us to filter git log to only show commits made during the session. @@ -1629,7 +1651,8 @@ func (e *Executor) buildLaunchAgentRequest(ctx context.Context, task *v1.Task, s // the new backend instead of attaching to the old environment. Resolve this // before repository configuration so clone URL requirements are applied to // the fresh launch as well. - workspaceReuseRequired = workspaceReuseAllowed(existingEnv, req.ExecutorType, workspaceReuseRequired) + repoBacked := e.taskIsRepoBacked(ctx, task.ID) + workspaceReuseRequired = workspaceReuseAllowed(existingEnv, req.ExecutorType, workspaceReuseRequired, repoBacked) req.WorkspaceReuseRequired = workspaceReuseRequired // For remote executors (containerized *and* SSH), resolve only explicitly @@ -1688,13 +1711,16 @@ func (e *Executor) buildLaunchAgentRequest(ctx context.Context, task *v1.Task, s return req, execConfig, nil } -func workspaceReuseAllowed(existingEnv *models.TaskEnvironment, requestedExecutorType string, required bool) bool { +func workspaceReuseAllowed(existingEnv *models.TaskEnvironment, requestedExecutorType string, required, repoBacked bool) bool { if !required || existingEnv == nil { return required } if existingEnv.ExecutorType != "" && existingEnv.ExecutorType != requestedExecutorType { return false } + if repoBacked && len(existingEnv.Repos) == 0 { + return false + } if requestedExecutorType == string(models.ExecutorTypeWorktree) { return hasLiveWorktreeRepo(existingEnv) } @@ -2188,7 +2214,7 @@ func (e *Executor) persistTaskEnvironment( req *LaunchAgentRequest, resp *LaunchAgentResponse, execCfg executorConfig, -) { +) error { mu := e.taskEnvLock(taskID) mu.Lock() defer mu.Unlock() @@ -2198,7 +2224,11 @@ func (e *Executor) persistTaskEnvironment( // see existingEnv == nil from the original call and try to create a // duplicate. if existingEnv == nil { - if fresh, err := e.repo.GetTaskEnvironmentByTaskID(ctx, taskID); err == nil && fresh != nil { + fresh, err := e.repo.GetTaskEnvironmentByTaskID(ctx, taskID) + if err != nil { + return fmt.Errorf("load task environment before persistence: %w", err) + } + if fresh != nil { existingEnv = fresh } } @@ -2208,12 +2238,19 @@ func (e *Executor) persistTaskEnvironment( if existingEnv != nil { materializationSessionID := existingEnv.MaterializationSessionID isInitialMaterializer := existingEnv.Status == models.TaskEnvironmentStatusCreating && materializationSessionID == session.ID + previousStatus := existingEnv.Status + previousMaterializationSessionID := existingEnv.MaterializationSessionID + repos := environmentReposForLaunch(req, resp) // agent_execution_id is no longer stored on task_environments — the column // is being dropped (executors_running is the single source of truth). // Status, workspace, and container fields are still env-row-owned; the - // physical worktree lives on task_environment_repos. - existingEnv.Status = models.TaskEnvironmentStatusReady - existingEnv.MaterializationSessionID = "" + // physical worktree lives on task_environment_repos. Status is decided + // per branch below rather than forced here: the initial materializer + // always transitions to ready (its inventory publishes atomically in + // the same call), but a non-materializing sibling must not force ready + // onto an environment whose canonical inventory turned out empty for a + // repo-backed task — that permanently bricks reuse (see + // validateReuseEnvironmentInventory). // Refresh workspace + container/sandbox fields. The original update // branch only touched AgentExecutionID/Status, so envs created with // empty paths (e.g. before the worktree resolved) stayed permanently @@ -2244,32 +2281,71 @@ func (e *Executor) persistTaskEnvironment( // The initial materializer must publish its full repository inventory // in the same transaction as the ready transition. Otherwise a sibling // can bind to a ready environment whose canonical rows are incomplete. - existingEnv.Status = models.TaskEnvironmentStatusReady - existingEnv.MaterializationSessionID = "" if finalizer, ok := e.repo.(taskEnvironmentMaterializationFinalizer); ok { - if err := finalizer.FinalizeTaskEnvironmentMaterialization(ctx, existingEnv, environmentReposForLaunch(req, resp), materializationSessionID); err != nil { + existingEnv.Status = models.TaskEnvironmentStatusReady + existingEnv.MaterializationSessionID = "" + if err := finalizer.FinalizeTaskEnvironmentMaterialization(ctx, existingEnv, repos, materializationSessionID); err != nil { + existingEnv.Status = previousStatus + existingEnv.MaterializationSessionID = previousMaterializationSessionID e.logger.Warn("failed to finalize task environment materialization", zap.String("task_id", taskID), zap.String("env_id", existingEnv.ID), zap.Error(err)) - return + return fmt.Errorf("finalize task environment materialization: %w", err) } session.TaskEnvironmentID = existingEnv.ID e.selfHealTaskRepositoryBaseBranches(ctx, taskID, req, resp) - return + return nil } } + // Persist per-repo rows for launches that didn't have them yet *before* + // the environment can be marked ready below. The environment-repository + // rows are the only physical-worktree record, so single-repo launches + // write one row here too. Ordering matters: writing status=ready first + // and these rows second leaves a window where a crash or a concurrent + // reader observes a ready environment with an empty inventory for a + // repo-backed task — exactly the state the guards elsewhere in this PR + // exist to prevent (see validateReuseEnvironmentInventory). + if err := e.persistTaskEnvironmentRepos(ctx, existingEnv.ID, repos); err != nil { + existingEnv.Status = previousStatus + existingEnv.MaterializationSessionID = previousMaterializationSessionID + return err + } + repoBacked := e.taskIsRepoBacked(ctx, taskID) + // A sibling can finish while the initial materializer still owns a + // CREATING environment. In that case the owner remains responsible for + // publishing (or failing) the inventory, so leave the claim untouched + // instead of turning the sibling's empty result into a launch error. Once + // the claim is no longer CREATING, an empty repo-backed inventory is a + // persistence failure and must fail the launch rather than publish an + // unusable READY/STOPPED environment. + if len(repos) == 0 && len(existingEnv.Repos) == 0 && repoBacked && previousStatus != models.TaskEnvironmentStatusCreating { + existingEnv.Status = previousStatus + existingEnv.MaterializationSessionID = previousMaterializationSessionID + return fmt.Errorf("persist task environment: ready status requires repository inventory") + } + // A non-materializing sibling only advances the environment to ready + // when inventory is present: already recorded before this launch, + // about to be written by this launch, or not required because the + // task has no configured repositories at all. A launch whose prepare + // step failed produces an empty repos slice here — leaving the + // environment's existing status untouched (it can only already be + // ready or stopped by the time this branch runs) keeps that launch + // from bricking future reuse instead of silently corrupting the row. + if len(repos) > 0 || len(existingEnv.Repos) > 0 || !repoBacked { + existingEnv.Status = models.TaskEnvironmentStatusReady + } + existingEnv.MaterializationSessionID = "" if err := e.repo.UpdateTaskEnvironment(ctx, existingEnv); err != nil { + existingEnv.Status = previousStatus + existingEnv.MaterializationSessionID = previousMaterializationSessionID e.logger.Warn("failed to update task environment", zap.String("task_id", taskID), zap.String("env_id", existingEnv.ID), zap.Error(err)) + return fmt.Errorf("update task environment: %w", err) } session.TaskEnvironmentID = existingEnv.ID - // Persist per-repo rows for launches that didn't have them yet. The - // environment-repository rows are the only physical-worktree record, - // so single-repo launches write one row here too. - e.persistTaskEnvironmentRepos(ctx, existingEnv.ID, environmentReposForLaunch(req, resp)) e.selfHealTaskRepositoryBaseBranches(ctx, taskID, req, resp) - return + return nil } env := &models.TaskEnvironment{ @@ -2295,10 +2371,28 @@ func (e *Executor) persistTaskEnvironment( e.logger.Warn("failed to create task environment", zap.String("task_id", taskID), zap.Error(err)) - return + return fmt.Errorf("create task environment: %w", err) } session.TaskEnvironmentID = env.ID e.selfHealTaskRepositoryBaseBranches(ctx, taskID, req, resp) + return nil +} + +// taskIsRepoBacked reports whether a task has any configured repositories. +// Fails closed on a read error (treats the task as repo-backed) so a +// transient failure cannot mask the ready-with-empty-inventory bug the +// persistTaskEnvironment status guard exists to prevent. +func (e *Executor) taskIsRepoBacked(ctx context.Context, taskID string) bool { + if e.repo == nil { + return false + } + repos, err := e.repo.ListTaskRepositories(ctx, taskID) + if err != nil { + e.logger.Warn("failed to check task repositories for environment ready-status guard", + zap.String("task_id", taskID), zap.Error(err)) + return true + } + return len(repos) > 0 } func (e *Executor) selfHealTaskRepositoryBaseBranches( @@ -2419,16 +2513,16 @@ func buildTaskEnvironmentRepos(worktrees []RepoWorktreeResult) []*models.TaskEnv // Used when an existing environment is reused (resume / re-launch on the same // task), including cases where stale or legacy rows need the successful launch // result written back for the next handoff. -func (e *Executor) persistTaskEnvironmentRepos(ctx context.Context, envID string, repos []*models.TaskEnvironmentRepo) { +func (e *Executor) persistTaskEnvironmentRepos(ctx context.Context, envID string, repos []*models.TaskEnvironmentRepo) error { if envID == "" || len(repos) == 0 { - return + return nil } existing, err := e.repo.ListTaskEnvironmentRepos(ctx, envID) if err != nil { e.logger.Warn("failed to list existing task_environment_repos before insert", zap.String("env_id", envID), zap.Error(err)) - return + return fmt.Errorf("list task environment repositories: %w", err) } byKey := make(map[string]*models.TaskEnvironmentRepo, len(existing)) legacyFlatByRepo := make(map[string]*models.TaskEnvironmentRepo) @@ -2445,12 +2539,16 @@ func (e *Executor) persistTaskEnvironmentRepos(ctx context.Context, envID string } key := w.RepositoryID + "\x00" + w.BranchSlug if row := byKey[key]; row != nil { - e.refreshTaskEnvironmentRepo(ctx, row, w, i) + if err := e.refreshTaskEnvironmentRepo(ctx, row, w, i); err != nil { + return err + } continue } if w.BranchSlug != "" { if row := legacyFlatByRepo[w.RepositoryID]; row != nil { - e.refreshTaskEnvironmentRepo(ctx, row, w, i) + if err := e.refreshTaskEnvironmentRepo(ctx, row, w, i); err != nil { + return err + } delete(legacyFlatByRepo, w.RepositoryID) byKey[key] = row continue @@ -2471,13 +2569,15 @@ func (e *Executor) persistTaskEnvironmentRepos(ctx context.Context, envID string zap.String("env_id", envID), zap.String("repository_id", w.RepositoryID), zap.Error(createErr)) + return fmt.Errorf("persist task environment repository %q: %w", w.RepositoryID, createErr) } } + return nil } -func (e *Executor) refreshTaskEnvironmentRepo(ctx context.Context, row, w *models.TaskEnvironmentRepo, position int) { +func (e *Executor) refreshTaskEnvironmentRepo(ctx context.Context, row, w *models.TaskEnvironmentRepo, position int) error { if !taskEnvironmentRepoNeedsRefresh(row, w, position) { - return + return nil } row.BranchSlug = w.BranchSlug // Concrete launch results populate the physical tuple together; inventory-only @@ -2495,7 +2595,9 @@ func (e *Executor) refreshTaskEnvironmentRepo(ctx context.Context, row, w *model zap.String("repository_id", row.RepositoryID), zap.String("branch_slug", row.BranchSlug), zap.Error(err)) + return fmt.Errorf("update task environment repository %q: %w", row.RepositoryID, err) } + return nil } func taskEnvironmentRepoNeedsRefresh(row, w *models.TaskEnvironmentRepo, position int) bool { diff --git a/apps/backend/internal/orchestrator/executor/executor_mocks_test.go b/apps/backend/internal/orchestrator/executor/executor_mocks_test.go index a89d50ec35..624506875d 100644 --- a/apps/backend/internal/orchestrator/executor/executor_mocks_test.go +++ b/apps/backend/internal/orchestrator/executor/executor_mocks_test.go @@ -279,6 +279,8 @@ type mockRepository struct { getTaskSessionFunc func(ctx context.Context, id string) (*models.TaskSession, error) getTaskEnvironmentFunc func(ctx context.Context, id string) (*models.TaskEnvironment, error) getTaskEnvironmentByTaskIDFunc func(ctx context.Context, taskID string) (*models.TaskEnvironment, error) + createTaskEnvironmentRepoErr error + finalizeTaskEnvironmentErr error createTaskSessionFunc func(ctx context.Context, session *models.TaskSession) error updateTaskSessionStateFunc func(ctx context.Context, sessionID string, state models.TaskSessionState, errorMessage string) error listActiveTaskSessionsByTaskIDFunc func(ctx context.Context, taskID string) ([]*models.TaskSession, error) @@ -311,6 +313,12 @@ type mockRepository struct { finalizeTaskEnvironmentCalls []*models.TaskEnvironment updateTaskStateIfCurrentInCalls []updateTaskStateIfCurrentInCall updateTaskStateIfNotArchivedCalls []updateTaskStateIfNotArchivedCall + + // writeCallLog records the relative order of environment-row and + // environment-status writes (e.g. "create_repo", "update_env") so tests + // can pin ordering invariants that a call-count assertion alone cannot + // catch — see TestPersistTaskEnvironment_NonMaterializerSiblingPersistsReposBeforeReady. + writeCallLog []string } type sharedWorkspaceBindingCall struct { @@ -1153,23 +1161,31 @@ func (m *mockRepository) UpdateTaskEnvironment(_ context.Context, env *models.Ta if env.ID == "" { return nil } + m.writeCallLog = append(m.writeCallLog, "update_env") m.updateTaskEnvironmentCalls = append(m.updateTaskEnvironmentCalls, env) m.taskEnvironments[env.ID] = env return nil } func (m *mockRepository) FinalizeTaskEnvironmentMaterialization(_ context.Context, env *models.TaskEnvironment, repos []*models.TaskEnvironmentRepo, _ string) error { + if m.finalizeTaskEnvironmentErr != nil { + return m.finalizeTaskEnvironmentErr + } m.finalizeTaskEnvironmentCalls = append(m.finalizeTaskEnvironmentCalls, env) m.taskEnvironments[env.ID] = env m.taskEnvironmentRepos[env.ID] = repos return nil } func (m *mockRepository) CreateTaskEnvironmentRepo(_ context.Context, repo *models.TaskEnvironmentRepo) error { + if m.createTaskEnvironmentRepoErr != nil { + return m.createTaskEnvironmentRepoErr + } if repo.ID == "" { repo.ID = repo.TaskEnvironmentID + "-repo-" + repo.RepositoryID if repo.BranchSlug != "" { repo.ID += "-branch-" + repo.BranchSlug } } + m.writeCallLog = append(m.writeCallLog, "create_repo") m.taskEnvironmentRepos[repo.TaskEnvironmentID] = append(m.taskEnvironmentRepos[repo.TaskEnvironmentID], repo) return nil } diff --git a/apps/backend/internal/orchestrator/executor/executor_multi_repo_test.go b/apps/backend/internal/orchestrator/executor/executor_multi_repo_test.go index 96a4641cb1..3bd3a66f9a 100644 --- a/apps/backend/internal/orchestrator/executor/executor_multi_repo_test.go +++ b/apps/backend/internal/orchestrator/executor/executor_multi_repo_test.go @@ -507,6 +507,44 @@ func TestResumeSession_MultiRepo_PopulatesRequestRepositories(t *testing.T) { } } +func TestBuildResumeRequest_RejectsMismatchedEnvironmentInventory(t *testing.T) { + repo := newMockRepository() + const taskID = "task-resume-inventory-mismatch" + const sessionID = "session-resume-inventory-mismatch" + seedMultiRepoTask(t, repo, taskID) + seedWorktreeExecutor(repo) + repo.tasks[taskID] = &models.Task{ID: taskID, WorkspaceID: "ws-1", Title: "Resume"} + repo.taskEnvironments["env-mismatch"] = &models.TaskEnvironment{ + ID: "env-mismatch", + TaskID: taskID, + ExecutorType: string(models.ExecutorTypeWorktree), + Status: models.TaskEnvironmentStatusReady, + Repos: []*models.TaskEnvironmentRepo{{ + TaskEnvironmentID: "env-mismatch", + RepositoryID: "repo-unrelated", + BranchSlug: "main", + WorktreeID: "wt-unrelated", + }}, + } + repo.taskEnvironmentRepos["env-mismatch"] = repo.taskEnvironments["env-mismatch"].Repos + repo.sessions[sessionID] = &models.TaskSession{ + ID: sessionID, + TaskID: taskID, + RepositoryID: "repo-front", + ExecutorID: models.ExecutorIDWorktree, + AgentProfileID: "profile-123", + State: models.TaskSessionStateCancelled, + StartedAt: time.Now(), + UpdatedAt: time.Now(), + } + + exec := newTestExecutor(t, &mockAgentManager{}, repo) + _, _, _, _, _, err := exec.buildResumeRequest(context.Background(), repo.tasks[taskID].ToAPI(), repo.sessions[sessionID], false) + if !errors.Is(err, models.ErrWorkspaceReuseUnsafe) { + t.Fatalf("buildResumeRequest error = %v, want ErrWorkspaceReuseUnsafe", err) + } +} + func TestReuseExistingRepositoryWorktrees_EnvironmentRowsWinOverSessionRows(t *testing.T) { repo := newMockRepository() taskID := "task-env-wins" diff --git a/apps/backend/internal/orchestrator/executor/executor_resume.go b/apps/backend/internal/orchestrator/executor/executor_resume.go index 06f88e4cf8..ff5edb75c2 100644 --- a/apps/backend/internal/orchestrator/executor/executor_resume.go +++ b/apps/backend/internal/orchestrator/executor/executor_resume.go @@ -748,7 +748,15 @@ func (e *Executor) ResumeSession(ctx context.Context, session *models.TaskSessio return nil, err } } - e.persistTaskEnvironment(ctx, task.ID, session, existingEnv, req, resp, execCfg) + if err := e.persistTaskEnvironment(ctx, task.ID, session, existingEnv, req, resp, execCfg); err != nil { + e.cleanupUnstartedExecutionAfterPersistError(ctx, session.ID, resp.AgentExecutionID, err) + e.markTaskEnvironmentMaterializationFailed(ctx, existingEnv, session.ID) + if startAgent { + e.rollbackResumeStateAfterFailure(ctx, task.ID, session.ID, resumeInitialState, err, + resumeCredentialSnapshotBackupIfPersisted(credentialSnapshotPersisted, previousCredentialSnapshot)) + } + return nil, err + } worktreePath := resp.WorktreePath worktreeBranch := resp.WorktreeBranch @@ -1022,6 +1030,7 @@ func (e *Executor) buildResumeRequestAtCredentialBoundary( existingEnv, req.ExecutorType, existingEnv != nil && existingEnv.MaterializationSessionID != session.ID, + e.taskIsRepoBacked(ctx, task.ID), ) allRepos, err := e.resolveAllRepoInfoForSession(ctx, task.ID, session.ID) @@ -1038,6 +1047,9 @@ func (e *Executor) buildResumeRequestAtCredentialBoundary( req.RemoteSyncHandled = allRepos[0].RemoteSyncHandled req.RefreshRepository = allRepos[0].RefreshRepository } + if err := e.validateReuseEnvironmentInventory(ctx, req, existingEnv); err != nil { + return nil, "", execConfig, existingEnv, nil, err + } e.reuseExistingEnvironment(ctx, req, existingEnv) diff --git a/apps/backend/internal/orchestrator/executor/executor_resume_test.go b/apps/backend/internal/orchestrator/executor/executor_resume_test.go index 1250a4ec5c..a4c2ece3ad 100644 --- a/apps/backend/internal/orchestrator/executor/executor_resume_test.go +++ b/apps/backend/internal/orchestrator/executor/executor_resume_test.go @@ -49,6 +49,39 @@ func TestResumeSession_RejectsArchivedTask(t *testing.T) { } } +func TestResumeSession_PropagatesTaskEnvironmentPersistenceFailure(t *testing.T) { + repo := newMockRepository() + setupLiveResumeTestFixture(repo) + attachManagedGitHubRepositoryForResume(t, repo) + persistErr := errors.New("inventory write failed") + repo.createTaskEnvironmentRepoErr = persistErr + repo.taskEnvironments["env-1"] = &models.TaskEnvironment{ + ID: "env-1", + TaskID: "task-1", + ExecutorType: string(models.ExecutorTypeLocal), + Status: models.TaskEnvironmentStatusReady, + } + repo.sessions["sess-1"].TaskEnvironmentID = "env-1" + + agentManager := &mockAgentManager{ + launchAgentFunc: func(_ context.Context, _ *LaunchAgentRequest) (*LaunchAgentResponse, error) { + return &LaunchAgentResponse{ + AgentExecutionID: "exec-new", + WorktreePath: "/workspace/task-1", + Worktrees: []RepoWorktreeResult{{ + RepositoryID: "repo-1", WorktreeID: "wt-1", WorktreePath: "/workspace/task-1", + }}, + }, nil + }, + } + exec := newTestExecutor(t, agentManager, repo) + + _, err := exec.ResumeSession(context.Background(), repo.sessions["sess-1"], false) + if !errors.Is(err, persistErr) { + t.Fatalf("ResumeSession error = %v, want %v", err, persistErr) + } +} + // setupLiveResumeTestFixture seeds a repo + task + session + executor-running // record suitable for exercising the ResumeSession launch path. func setupLiveResumeTestFixture(repo *mockRepository) { diff --git a/apps/backend/internal/orchestrator/executor/executor_test.go b/apps/backend/internal/orchestrator/executor/executor_test.go index 2e16615b01..88a954451a 100644 --- a/apps/backend/internal/orchestrator/executor/executor_test.go +++ b/apps/backend/internal/orchestrator/executor/executor_test.go @@ -604,6 +604,54 @@ func TestLaunchPreparedSession_Success(t *testing.T) { } } +func TestLaunchPreparedSession_PropagatesTaskEnvironmentPersistenceFailure(t *testing.T) { + repo := newMockRepository() + persistErr := errors.New("inventory write failed") + repo.createTaskEnvironmentRepoErr = persistErr + repo.executors[models.ExecutorIDLocal] = &models.Executor{ + ID: models.ExecutorIDLocal, Type: models.ExecutorTypeLocal, Status: models.ExecutorStatusActive, + } + repo.repositories["repo-1"] = &models.Repository{ + ID: "repo-1", WorkspaceID: "workspace-1", SourceType: sourceTypeLocal, LocalPath: "/repo-1", Name: "repo-1", + } + repo.taskRepositories["task-repo-1"] = &models.TaskRepository{ + ID: "task-repo-1", TaskID: "task-1", RepositoryID: "repo-1", Position: 0, + } + repo.taskEnvironments["env-1"] = &models.TaskEnvironment{ + ID: "env-1", TaskID: "task-1", ExecutorType: string(models.ExecutorTypeLocal), Status: models.TaskEnvironmentStatusReady, + } + repo.sessions["session-1"] = &models.TaskSession{ + ID: "session-1", TaskID: "task-1", AgentProfileID: "profile-1", ExecutorID: models.ExecutorIDLocal, + TaskEnvironmentID: "env-1", State: models.TaskSessionStateCreated, StartedAt: time.Now(), UpdatedAt: time.Now(), + } + stopped := false + agentManager := &mockAgentManager{ + launchAgentFunc: func(_ context.Context, _ *LaunchAgentRequest) (*LaunchAgentResponse, error) { + return &LaunchAgentResponse{ + AgentExecutionID: "exec-new", + Worktrees: []RepoWorktreeResult{{RepositoryID: "repo-1", WorktreeID: "wt-1"}}, + }, nil + }, + stopAgentFunc: func(_ context.Context, executionID string, _ bool) error { + if executionID == "exec-new" { + stopped = true + } + return nil + }, + } + exec := newTestExecutor(t, agentManager, repo) + + _, err := exec.LaunchPreparedSession(context.Background(), &v1.Task{ + ID: "task-1", WorkspaceID: "workspace-1", Title: "Task 1", + }, "session-1", LaunchOptions{AgentProfileID: "profile-1", ExecutorID: models.ExecutorIDLocal}) + if !errors.Is(err, persistErr) { + t.Fatalf("LaunchPreparedSession error = %v, want %v", err, persistErr) + } + if !stopped { + t.Fatal("persistence failure did not stop the unstarted execution") + } +} + func TestLaunchPreparedSession_RejectsNilLaunchResponse(t *testing.T) { repo := newMockRepository() repo.sessions["session-nil-response"] = &models.TaskSession{ diff --git a/apps/backend/internal/orchestrator/executor/executor_workspace_reuse_test.go b/apps/backend/internal/orchestrator/executor/executor_workspace_reuse_test.go index bfafcd19d0..73dde059a5 100644 --- a/apps/backend/internal/orchestrator/executor/executor_workspace_reuse_test.go +++ b/apps/backend/internal/orchestrator/executor/executor_workspace_reuse_test.go @@ -15,6 +15,7 @@ func TestWorkspaceReuseAllowedRequiresLiveWorktreeRepo(t *testing.T) { env *models.TaskEnvironment requestedType string reuseRequired bool + repoBacked bool wantReuseAllowed bool }{ { @@ -119,7 +120,48 @@ func TestWorkspaceReuseAllowedRequiresLiveWorktreeRepo(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := workspaceReuseAllowed(tt.env, tt.requestedType, tt.reuseRequired); got != tt.wantReuseAllowed { + if got := workspaceReuseAllowed(tt.env, tt.requestedType, tt.reuseRequired, tt.repoBacked); got != tt.wantReuseAllowed { + t.Fatalf("workspaceReuseAllowed() = %t, want %t", got, tt.wantReuseAllowed) + } + }) + } +} + +// @covers AC-TASKS-RUNTIME-CLEANUP-001.7 +func TestWorkspaceReuseAllowedRequiresSSHInventory(t *testing.T) { + tests := []struct { + name string + env *models.TaskEnvironment + repoBacked bool + wantReuseAllowed bool + }{ + { + name: "repo-backed with empty inventory forces fresh materialization", + env: &models.TaskEnvironment{ExecutorType: string(models.ExecutorTypeSSH)}, + repoBacked: true, + wantReuseAllowed: false, + }, + { + name: "repo-backed with recorded inventory reuses", + env: &models.TaskEnvironment{ + ExecutorType: string(models.ExecutorTypeSSH), + Repos: []*models.TaskEnvironmentRepo{{RepositoryID: "repo-1"}}, + }, + repoBacked: true, + wantReuseAllowed: true, + }, + { + name: "repo-less task reuses despite empty inventory", + env: &models.TaskEnvironment{ExecutorType: string(models.ExecutorTypeSSH)}, + repoBacked: false, + wantReuseAllowed: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := workspaceReuseAllowed(tt.env, string(models.ExecutorTypeSSH), true, tt.repoBacked) + if got != tt.wantReuseAllowed { t.Fatalf("workspaceReuseAllowed() = %t, want %t", got, tt.wantReuseAllowed) } }) diff --git a/apps/backend/internal/plugins/runtime/process.go b/apps/backend/internal/plugins/runtime/process.go index 796f756e0c..293bae24e1 100644 --- a/apps/backend/internal/plugins/runtime/process.go +++ b/apps/backend/internal/plugins/runtime/process.go @@ -141,20 +141,30 @@ func (p *process) start() error { return nil } -// stop signals the supervision loop to exit, waits for it to drain, and -// kills any still-running process. Idempotent-safe to call at most once per -// process (Manager.Stop removes the process from its map before calling -// this, so a second Stop(id) for the same id is a no-op at the Manager +// stop signals the supervision loop to exit and kills the current process +// before waiting for the loop to drain. The kill must happen first because a +// health ping can be blocked in the plugin protocol; terminating the process +// releases that call and lets the supervisor exit. Idempotent-safe to call at +// most once per process (Manager.Stop removes the process from its map before +// calling this, so a second Stop(id) for the same id is a no-op at the Manager // level). func (p *process) stop() { close(p.stopCh) + p.mu.Lock() + current := p.current + p.current = nil + p.mu.Unlock() + if current != nil { + current.Kill() + } p.wg.Wait() p.mu.Lock() - if p.current != nil { - p.current.Kill() - p.current = nil - } + current = p.current + p.current = nil p.mu.Unlock() + if current != nil { + current.Kill() + } } // remote returns the current live RemotePlugin, if any. diff --git a/apps/backend/internal/plugins/runtime/process_test.go b/apps/backend/internal/plugins/runtime/process_test.go index 009eac9ad9..1fd2d180ec 100644 --- a/apps/backend/internal/plugins/runtime/process_test.go +++ b/apps/backend/internal/plugins/runtime/process_test.go @@ -19,6 +19,33 @@ type fakeSpawnedProcess struct { killed bool } +type blockingPingProcess struct { + pingStarted chan struct{} + releasePing chan struct{} + killOnce sync.Once +} + +func newBlockingPingProcess() *blockingPingProcess { + return &blockingPingProcess{ + pingStarted: make(chan struct{}), + releasePing: make(chan struct{}), + } +} + +func (f *blockingPingProcess) Ping() error { + close(f.pingStarted) + <-f.releasePing + return nil +} + +func (f *blockingPingProcess) Exited() bool { return false } + +func (f *blockingPingProcess) Kill() { + f.killOnce.Do(func() { close(f.releasePing) }) +} + +func (f *blockingPingProcess) Remote() *pluginsdk.RemotePlugin { return nil } + func (f *fakeSpawnedProcess) Ping() error { f.mu.Lock() defer f.mu.Unlock() @@ -258,6 +285,35 @@ func TestProcess_HandleFailureAndRestart_StopDuringBackoffSkipsSpawn(t *testing. } } +func TestProcess_StopKillsProcessBeforeWaitingForBlockedPing(t *testing.T) { + blocked := newBlockingPingProcess() + p := newProcess("test-plugin", testLogger(t), func() (spawnedProcess, error) { + return blocked, nil + }, nil) + p.pingInterval = 0 + + if err := p.start(); err != nil { + t.Fatalf("start: %v", err) + } + <-blocked.pingStarted + + stopped := make(chan struct{}) + go func() { + p.stop() + close(stopped) + }() + + select { + case <-stopped: + case <-time.After(time.Second): + // Release the test double so the old wait-before-kill implementation + // cannot leak its supervisor goroutine after this assertion fails. + blocked.Kill() + <-stopped + t.Fatal("stop waited for a blocked ping before killing the plugin process") + } +} + func equalBoolSlices(a, b []bool) bool { if len(a) != len(b) { return false diff --git a/apps/backend/internal/task/repository/sqlite/task_environment.go b/apps/backend/internal/task/repository/sqlite/task_environment.go index e6174eb367..269a3622ba 100644 --- a/apps/backend/internal/task/repository/sqlite/task_environment.go +++ b/apps/backend/internal/task/repository/sqlite/task_environment.go @@ -43,6 +43,21 @@ func (r *Repository) CreateTaskEnvironment(ctx context.Context, env *models.Task return err } + // A ready environment with zero repo rows is indistinguishable from "the + // canonical inventory was never captured" and permanently bricks reuse + // (validateReuseEnvironmentInventory refuses every later launch). Reject + // at the boundary instead of trusting the caller: only a task with no + // repositories configured at all may legitimately have none. + if env.Status == models.TaskEnvironmentStatusReady && len(env.Repos) == 0 { + hasRepos, err := r.taskHasRepositoriesTx(ctx, tx, env.TaskID) + if err != nil { + return err + } + if hasRepos { + return fmt.Errorf("create task environment: ready status requires repository inventory for repo-backed task (task=%s)", env.TaskID) + } + } + if _, err := tx.ExecContext(ctx, r.db.Rebind(` INSERT INTO task_environments ( id, task_id, executor_type, executor_id, executor_profile_id, @@ -151,9 +166,19 @@ func (r *Repository) UpdateTaskEnvironment(ctx context.Context, env *models.Task if env.ExecutorType == string(models.ExecutorTypeWorktree) && env.WorkspacePath == "" && env.Status != models.TaskEnvironmentStatusCreating { return fmt.Errorf("update task environment: worktree-mode env requires workspace_path (id=%s)", env.ID) } + tx, err := r.db.BeginTxx(ctx, nil) + if err != nil { + return err + } + defer func() { _ = tx.Rollback() }() + if env.Status == models.TaskEnvironmentStatusReady { + if err := r.validateReadyTaskEnvironment(ctx, tx, env.ID); err != nil { + return err + } + } env.UpdatedAt = time.Now().UTC() - result, err := r.db.ExecContext(ctx, r.db.Rebind(` + result, err := tx.ExecContext(ctx, r.db.Rebind(` UPDATE task_environments SET executor_type = ?, executor_id = ?, executor_profile_id = ?, control_port = ?, status = ?, materialization_session_id = ?, @@ -176,9 +201,49 @@ func (r *Repository) UpdateTaskEnvironment(ctx context.Context, env *models.Task if rows == 0 { return fmt.Errorf("%w: %s", ErrTaskEnvironmentNotFound, env.ID) } + return tx.Commit() +} + +func (r *Repository) validateReadyTaskEnvironment(ctx context.Context, tx *sqlx.Tx, environmentID string) error { + taskID, currentStatus, err := r.taskEnvironmentStateTx(ctx, tx, environmentID) + if err != nil { + return err + } + if err := r.taskCleanupBarrierLocked(ctx, tx, taskID); err != nil { + return err + } + // Workspace-source materialization updates an already-ready environment + // after it adds a folder or repository. That update does not publish a new + // ready state, so it must remain compatible with legacy environments whose + // canonical inventory is still empty. Launch persistence separately rejects + // that state before it calls this repository method. + if currentStatus == models.TaskEnvironmentStatusReady { + return nil + } + hasRepos, err := r.taskHasRepositoriesTx(ctx, tx, taskID) + if err != nil || !hasRepos { + return err + } + var inventoryCount int + if err := tx.QueryRowContext(ctx, r.db.Rebind(`SELECT COUNT(1) FROM task_environment_repos WHERE task_environment_id = ?`), environmentID).Scan(&inventoryCount); err != nil { + return err + } + if inventoryCount == 0 { + return fmt.Errorf("update task environment: ready status requires repository inventory for repo-backed task (id=%s)", environmentID) + } return nil } +func (r *Repository) taskEnvironmentStateTx(ctx context.Context, tx *sqlx.Tx, environmentID string) (string, models.TaskEnvironmentStatus, error) { + var taskID string + var status string + err := tx.QueryRowContext(ctx, r.db.Rebind(`SELECT task_id, status FROM task_environments WHERE id = ?`), environmentID).Scan(&taskID, &status) + if err == sql.ErrNoRows { + return "", "", fmt.Errorf("%w: %s", ErrTaskEnvironmentNotFound, environmentID) + } + return taskID, models.TaskEnvironmentStatus(status), err +} + // FinalizeTaskEnvironmentMaterialization writes the complete canonical // repository inventory and transitions the elected creating environment to // ready in one transaction. The materializer ID prevents a sibling session @@ -208,6 +273,19 @@ func (r *Repository) FinalizeTaskEnvironmentMaterialization( if err := r.taskCleanupBarrierLocked(ctx, tx, taskID); err != nil { return err } + // See the matching guard in CreateTaskEnvironment: a materializer that + // publishes ready with an empty inventory (e.g. because its prepare step + // failed and produced no repos) must not be allowed to land, or the + // environment becomes permanently unreusable. + if len(repos) == 0 { + hasRepos, err := r.taskHasRepositoriesTx(ctx, tx, taskID) + if err != nil { + return err + } + if hasRepos { + return fmt.Errorf("finalize task environment: ready status requires repository inventory for repo-backed task (id=%s)", env.ID) + } + } if _, err := tx.ExecContext(ctx, r.db.Rebind(`DELETE FROM task_environment_repos WHERE task_environment_id = ?`), env.ID); err != nil { return err } @@ -243,6 +321,17 @@ func (r *Repository) FinalizeTaskEnvironmentMaterialization( return tx.Commit() } +// taskHasRepositoriesTx reports whether a task has any task_repositories +// rows, i.e. whether it is repo-backed and therefore expected to carry a +// non-empty canonical inventory whenever its environment is ready. +func (r *Repository) taskHasRepositoriesTx(ctx context.Context, tx *sqlx.Tx, taskID string) (bool, error) { + var count int + if err := tx.QueryRowContext(ctx, r.db.Rebind(`SELECT COUNT(1) FROM task_repositories WHERE task_id = ?`), taskID).Scan(&count); err != nil { + return false, err + } + return count > 0, nil +} + func (r *Repository) TransferTaskEnvironmentToTask(ctx context.Context, envID, taskID string) error { result, err := r.db.ExecContext(ctx, r.db.Rebind(` UPDATE task_environments SET diff --git a/apps/backend/internal/task/repository/sqlite/task_environment_test.go b/apps/backend/internal/task/repository/sqlite/task_environment_test.go index 81b3072e24..ecb4c95550 100644 --- a/apps/backend/internal/task/repository/sqlite/task_environment_test.go +++ b/apps/backend/internal/task/repository/sqlite/task_environment_test.go @@ -188,6 +188,42 @@ func TestFinalizeTaskEnvironmentMaterializationPublishesInventoryAtomically(t *t } } +func TestFinalizeTaskEnvironmentMaterializationAllowsEmptyInventoryForRepoLessTask(t *testing.T) { + repo := newRepoForEntityTests(t) + ctx := context.Background() + seedWorkspace(t, repo, "workspace-finalize-repoless") + if err := repo.CreateTask(ctx, &models.Task{ID: "task-finalize-repoless", WorkspaceID: "workspace-finalize-repoless", Title: "Repo-less"}); err != nil { + t.Fatal(err) + } + env := &models.TaskEnvironment{ + ID: "env-finalize-repoless", + TaskID: "task-finalize-repoless", + ExecutorType: string(models.ExecutorTypeLocal), + Status: models.TaskEnvironmentStatusCreating, + MaterializationSessionID: "session-materializer-repoless", + } + if err := repo.CreateTaskEnvironment(ctx, env); err != nil { + t.Fatalf("CreateTaskEnvironment: %v", err) + } + + env.Status = models.TaskEnvironmentStatusReady + env.MaterializationSessionID = "" + if err := repo.FinalizeTaskEnvironmentMaterialization(ctx, env, nil, "session-materializer-repoless"); err != nil { + t.Fatalf("FinalizeTaskEnvironmentMaterialization: %v", err) + } + + persisted, err := repo.GetTaskEnvironment(ctx, env.ID) + if err != nil { + t.Fatalf("GetTaskEnvironment: %v", err) + } + if persisted.Status != models.TaskEnvironmentStatusReady || persisted.MaterializationSessionID != "" { + t.Fatalf("environment = status %q owner %q, want ready with no owner", persisted.Status, persisted.MaterializationSessionID) + } + if len(persisted.Repos) != 0 { + t.Fatalf("canonical inventory = %#v, want empty for repo-less task", persisted.Repos) + } +} + func TestCreateTaskSessionWithWorkspaceBindingElectsAndAttaches(t *testing.T) { repo := newRepoForEntityTests(t) ctx := context.Background() @@ -500,3 +536,150 @@ func TestTransferTaskEnvironmentMissingReturnsSentinel(t *testing.T) { t.Fatalf("TransferTaskEnvironmentToTask error = %v, want ErrTaskEnvironmentNotFound", err) } } + +// seedRepoBackedTask creates a task with one linked task_repositories row, so +// the task reads as repo-backed to the ready-requires-inventory guards below. +func seedRepoBackedTask(t *testing.T, repo *Repository, workspaceID, taskID string) { + t.Helper() + ctx := context.Background() + seedWorkspace(t, repo, workspaceID) + if err := repo.CreateTask(ctx, &models.Task{ID: taskID, WorkspaceID: workspaceID, Title: taskID}); err != nil { + t.Fatalf("CreateTask: %v", err) + } + if err := repo.CreateRepository(ctx, &models.Repository{ID: taskID + "-repo", WorkspaceID: workspaceID, Name: taskID + "-repo"}); err != nil { + t.Fatalf("CreateRepository: %v", err) + } + if err := repo.CreateTaskRepository(ctx, &models.TaskRepository{TaskID: taskID, RepositoryID: taskID + "-repo", BaseBranch: "main"}); err != nil { + t.Fatalf("CreateTaskRepository: %v", err) + } +} + +// Regression test for the "failed prepare leaves a permanently bricked +// workspace" defect: a launch whose prepare step fails produces an empty +// repos slice, and the materializer must not be allowed to publish that as +// ready inventory for a repo-backed task. FinalizeTaskEnvironmentMaterialization +// hardcodes the ready status in its UPDATE, so this must be enforced inside +// the function itself, not by the caller. +func TestFinalizeTaskEnvironmentMaterializationRefusesEmptyInventoryForRepoBackedTask(t *testing.T) { + repo := newRepoForEntityTests(t) + ctx := context.Background() + seedRepoBackedTask(t, repo, "workspace-finalize-empty-inventory", "task-finalize-empty-inventory") + + env := &models.TaskEnvironment{ + ID: "env-finalize-empty-inventory", + TaskID: "task-finalize-empty-inventory", + ExecutorType: string(models.ExecutorTypeSSH), + Status: models.TaskEnvironmentStatusCreating, + MaterializationSessionID: "session-materializer-empty", + } + if err := repo.CreateTaskEnvironment(ctx, env); err != nil { + t.Fatalf("CreateTaskEnvironment: %v", err) + } + + env.Status = models.TaskEnvironmentStatusReady + env.MaterializationSessionID = "" + if err := repo.FinalizeTaskEnvironmentMaterialization(ctx, env, nil, "session-materializer-empty"); err == nil { + t.Fatal("FinalizeTaskEnvironmentMaterialization published ready with empty inventory for a repo-backed task, want error") + } + + persisted, err := repo.GetTaskEnvironment(ctx, env.ID) + if err != nil { + t.Fatalf("GetTaskEnvironment: %v", err) + } + if persisted.Status != models.TaskEnvironmentStatusCreating { + t.Fatalf("environment after refused finalize = status %q, want unchanged creating", persisted.Status) + } + if len(persisted.Repos) != 0 { + t.Fatalf("environment after refused finalize has inventory = %#v, want none", persisted.Repos) + } +} + +func TestUpdateTaskEnvironmentRefusesReadyWithEmptyInventoryForRepoBackedTask(t *testing.T) { + repo := newRepoForEntityTests(t) + ctx := context.Background() + seedRepoBackedTask(t, repo, "workspace-update-empty-inventory", "task-update-empty-inventory") + + env := &models.TaskEnvironment{ + ID: "env-update-empty-inventory", + TaskID: "task-update-empty-inventory", + ExecutorType: string(models.ExecutorTypeSSH), + Status: models.TaskEnvironmentStatusCreating, + } + if err := repo.CreateTaskEnvironment(ctx, env); err != nil { + t.Fatalf("CreateTaskEnvironment: %v", err) + } + + env.Status = models.TaskEnvironmentStatusReady + if err := repo.UpdateTaskEnvironment(ctx, env); err == nil { + t.Fatal("UpdateTaskEnvironment published ready with empty inventory for repo-backed task, want error") + } + + persisted, err := repo.GetTaskEnvironment(ctx, env.ID) + if err != nil { + t.Fatalf("GetTaskEnvironment: %v", err) + } + if persisted.Status != models.TaskEnvironmentStatusCreating { + t.Fatalf("environment after refused update = status %q, want unchanged creating", persisted.Status) + } +} + +func TestUpdateTaskEnvironmentAllowsExistingReadyEnvironmentWithoutInventory(t *testing.T) { + repo := newRepoForEntityTests(t) + ctx := context.Background() + workspaceID := "workspace-update-existing-ready" + taskID := "task-update-existing-ready" + seedWorkspace(t, repo, workspaceID) + if err := repo.CreateTask(ctx, &models.Task{ID: taskID, WorkspaceID: workspaceID, Title: taskID}); err != nil { + t.Fatalf("CreateTask: %v", err) + } + + env := &models.TaskEnvironment{ + ID: "env-update-existing-ready", + TaskID: taskID, + ExecutorType: string(models.ExecutorTypeSSH), + Status: models.TaskEnvironmentStatusReady, + } + if err := repo.CreateTaskEnvironment(ctx, env); err != nil { + t.Fatalf("CreateTaskEnvironment: %v", err) + } + if err := repo.CreateRepository(ctx, &models.Repository{ID: taskID + "-repo", WorkspaceID: workspaceID, Name: taskID + "-repo"}); err != nil { + t.Fatalf("CreateRepository: %v", err) + } + if err := repo.CreateTaskRepository(ctx, &models.TaskRepository{TaskID: taskID, RepositoryID: taskID + "-repo", BaseBranch: "main"}); err != nil { + t.Fatalf("CreateTaskRepository: %v", err) + } + env.WorkspacePath = "/workspace/task-update-existing-ready" + if err := repo.UpdateTaskEnvironment(ctx, env); err != nil { + t.Fatalf("UpdateTaskEnvironment on existing ready environment: %v", err) + } + + persisted, err := repo.GetTaskEnvironment(ctx, env.ID) + if err != nil { + t.Fatalf("GetTaskEnvironment: %v", err) + } + if persisted.Status != models.TaskEnvironmentStatusReady || persisted.WorkspacePath != env.WorkspacePath { + t.Fatalf("persisted environment = %+v, want ready with updated workspace path", persisted) + } +} + +// Same defect, direct-create path: CreateTaskEnvironment must not persist a +// ready environment with zero repo rows for a repo-backed task either. +func TestCreateTaskEnvironmentRefusesReadyWithEmptyInventoryForRepoBackedTask(t *testing.T) { + repo := newRepoForEntityTests(t) + ctx := context.Background() + seedRepoBackedTask(t, repo, "workspace-create-empty-inventory", "task-create-empty-inventory") + + env := &models.TaskEnvironment{ + ID: "env-create-empty-inventory", + TaskID: "task-create-empty-inventory", + ExecutorType: string(models.ExecutorTypeSSH), + Status: models.TaskEnvironmentStatusReady, + } + if err := repo.CreateTaskEnvironment(ctx, env); err == nil { + t.Fatal("CreateTaskEnvironment published ready with empty inventory for a repo-backed task, want error") + } + + if _, err := repo.GetTaskEnvironment(ctx, env.ID); !errors.Is(err, ErrTaskEnvironmentNotFound) { + t.Fatalf("GetTaskEnvironment after refused create = %v, want ErrTaskEnvironmentNotFound", err) + } +} diff --git a/apps/backend/internal/task/service/service_branches_test.go b/apps/backend/internal/task/service/service_branches_test.go index 2addda8b8f..c260ed3ba3 100644 --- a/apps/backend/internal/task/service/service_branches_test.go +++ b/apps/backend/internal/task/service/service_branches_test.go @@ -182,6 +182,9 @@ func TestAddBranchToTask_RejectsNonWorktreeExecutor(t *testing.T) { Status: "ready", CreatedAt: now, UpdatedAt: now, + Repos: []*models.TaskEnvironmentRepo{ + {ID: "env-1-repo-0", RepositoryID: "repo-1", CreatedAt: now}, + }, }); err != nil { t.Fatalf("CreateTaskEnvironment: %v", err) } @@ -231,6 +234,9 @@ func TestAddBranchToTask_AllowsWorktreeExecutor(t *testing.T) { Status: "ready", CreatedAt: now, UpdatedAt: now, + Repos: []*models.TaskEnvironmentRepo{ + {ID: "env-1-repo-0", RepositoryID: "repo-1", CreatedAt: now}, + }, }); err != nil { t.Fatalf("CreateTaskEnvironment: %v", err) } @@ -591,6 +597,9 @@ func TestAddBranchToTask_RejectsNonWorktreeExecutor_NoOrphanRepo(t *testing.T) { Status: "ready", CreatedAt: now, UpdatedAt: now, + Repos: []*models.TaskEnvironmentRepo{ + {ID: "env-1-repo-0", RepositoryID: "repo-primary", CreatedAt: now}, + }, }); err != nil { t.Fatalf("CreateTaskEnvironment: %v", err) } @@ -693,14 +702,32 @@ func (m *stubMaterializer) MaterializeBranch(_ context.Context, _ string, _ stri // seedWorktreeTaskEnv attaches a worktree-executor task_environments row so // requireWorktreeExecutorForBranchAdd permits the call and taskAlreadyLaunched -// reports the task as live. +// reports the task as live. The environment carries inventory rows mirroring +// the task's existing task_repositories, since CreateTaskEnvironment now +// requires a ready environment to carry inventory for a repo-backed task. func seedWorktreeTaskEnv(t *testing.T, repo interface { CreateTaskEnvironment(ctx context.Context, env *models.TaskEnvironment) error + ListTaskRepositories(ctx context.Context, taskID string) ([]*models.TaskRepository, error) }, taskID, id string, ) { t.Helper() + ctx := context.Background() now := time.Now().UTC() - if err := repo.CreateTaskEnvironment(context.Background(), &models.TaskEnvironment{ + taskRepos, err := repo.ListTaskRepositories(ctx, taskID) + if err != nil { + t.Fatalf("ListTaskRepositories: %v", err) + } + envRepos := make([]*models.TaskEnvironmentRepo, len(taskRepos)) + for i, tr := range taskRepos { + envRepos[i] = &models.TaskEnvironmentRepo{ + ID: fmt.Sprintf("%s-repo-%d", id, i), + WorktreeID: fmt.Sprintf("%s-worktree-%d", id, i), + RepositoryID: tr.RepositoryID, + BranchSlug: tr.CheckoutBranch, + CreatedAt: now, + } + } + if err := repo.CreateTaskEnvironment(ctx, &models.TaskEnvironment{ ID: id, TaskID: taskID, ExecutorType: string(models.ExecutorTypeWorktree), @@ -708,6 +735,7 @@ func seedWorktreeTaskEnv(t *testing.T, repo interface { Status: "ready", CreatedAt: now, UpdatedAt: now, + Repos: envRepos, }); err != nil { t.Fatalf("CreateTaskEnvironment: %v", err) } diff --git a/apps/backend/internal/task/service/service_turns_test.go b/apps/backend/internal/task/service/service_turns_test.go index 735172088a..27727a23f2 100644 --- a/apps/backend/internal/task/service/service_turns_test.go +++ b/apps/backend/internal/task/service/service_turns_test.go @@ -790,6 +790,22 @@ func createTestEnvironment(t *testing.T, repo *sqliterepo.Repository, envID, tas } } +// createTestEnvironmentWithRepos is like createTestEnvironment but seeds the +// canonical repository inventory atomically with the ready environment. +// CreateTaskEnvironment requires a ready environment for a repo-backed task +// to carry its inventory up front, so tests whose task has task_repositories +// rows must use this instead of a separate CreateTaskEnvironmentRepo call. +func createTestEnvironmentWithRepos(t *testing.T, repo *sqliterepo.Repository, envID, taskID string, repos []*models.TaskEnvironmentRepo) { + t.Helper() + if err := repo.CreateTaskEnvironment(context.Background(), &models.TaskEnvironment{ + ID: envID, TaskID: taskID, ExecutorType: "worktree", + WorkspacePath: "/tmp", Status: models.TaskEnvironmentStatusReady, + Repos: repos, + }); err != nil { + t.Fatalf("CreateTaskEnvironment(%s): %v", envID, err) + } +} + func TestGetWorkspaceInfoForSession_BasicFields(t *testing.T) { svc, _, repo := createTestService(t) ctx := context.Background() @@ -913,13 +929,12 @@ func TestGetWorkspaceInfoForSession_ProjectsPersistedWorktreeIdentity(t *testing }); err != nil { t.Fatalf("CreateTaskSession: %v", err) } - createTestEnvironment(t, repo, "env-recovery", "task-123") - if err := repo.CreateTaskEnvironmentRepo(ctx, &models.TaskEnvironmentRepo{ - ID: "session-worktree-recovery", TaskEnvironmentID: "env-recovery", WorktreeID: "worktree-recovery", RepositoryID: "repo-recovery", - BranchSlug: "feature-recovery", WorktreePath: "/tasks/task-recovery/api-feature-recovery", CreatedAt: now, - }); err != nil { - t.Fatalf("CreateTaskEnvironmentRepo: %v", err) - } + createTestEnvironmentWithRepos(t, repo, "env-recovery", "task-123", []*models.TaskEnvironmentRepo{ + { + ID: "session-worktree-recovery", TaskEnvironmentID: "env-recovery", WorktreeID: "worktree-recovery", RepositoryID: "repo-recovery", + BranchSlug: "feature-recovery", WorktreePath: "/tasks/task-recovery/api-feature-recovery", CreatedAt: now, + }, + }) info, err := svc.GetWorkspaceInfoForSession(ctx, "task-123", "session-recovery") if err != nil { @@ -959,15 +974,10 @@ func TestGetWorkspaceInfoForSession_ProjectsDistinctWorktreesForSameRepositoryBr }); err != nil { t.Fatalf("CreateTaskSession: %v", err) } - createTestEnvironment(t, repo, "env-multi-recovery", "task-123") - for _, worktree := range []*models.TaskEnvironmentRepo{ + createTestEnvironmentWithRepos(t, repo, "env-multi-recovery", "task-123", []*models.TaskEnvironmentRepo{ {ID: "session-worktree-one", TaskEnvironmentID: "env-multi-recovery", WorktreeID: "worktree-one", RepositoryID: "repo-multi-recovery", BranchSlug: "feature-one", CreatedAt: now}, {ID: "session-worktree-two", TaskEnvironmentID: "env-multi-recovery", WorktreeID: "worktree-two", RepositoryID: "repo-multi-recovery", BranchSlug: "feature-two", CreatedAt: now}, - } { - if err := repo.CreateTaskEnvironmentRepo(ctx, worktree); err != nil { - t.Fatalf("CreateTaskEnvironmentRepo %q: %v", worktree.BranchSlug, err) - } - } + }) info, err := svc.GetWorkspaceInfoForSession(ctx, "task-123", "session-multi-recovery") if err != nil { @@ -1024,10 +1034,9 @@ func TestGetWorkspaceInfoForSession_UsesRepositoryDefaultBranchIdentity(t *testi if err := repo.CreateTaskSession(ctx, &models.TaskSession{ID: "session-default-branch", TaskID: "task-123", TaskEnvironmentID: "env-default-branch", State: models.TaskSessionStateCompleted, StartedAt: now, UpdatedAt: now}); err != nil { t.Fatalf("CreateTaskSession: %v", err) } - createTestEnvironment(t, repo, "env-default-branch", "task-123") - if err := repo.CreateTaskEnvironmentRepo(ctx, &models.TaskEnvironmentRepo{ID: "session-worktree-default-branch", TaskEnvironmentID: "env-default-branch", WorktreeID: "worktree-default-branch", RepositoryID: "repo-default-branch", BranchSlug: "main", CreatedAt: now}); err != nil { - t.Fatalf("CreateTaskEnvironmentRepo: %v", err) - } + createTestEnvironmentWithRepos(t, repo, "env-default-branch", "task-123", []*models.TaskEnvironmentRepo{ + {ID: "session-worktree-default-branch", TaskEnvironmentID: "env-default-branch", WorktreeID: "worktree-default-branch", RepositoryID: "repo-default-branch", BranchSlug: "main", CreatedAt: now}, + }) info, err := svc.GetWorkspaceInfoForSession(ctx, "task-123", "session-default-branch") if err != nil { @@ -1063,15 +1072,14 @@ func TestGetWorkspaceInfoForSession_UsesHashDisambiguatedBranchIdentities(t *tes {RepositoryID: "repo-hash-branches", BaseBranch: "feature/a", DefaultBranch: "main", PRNumber: 101, Position: 0}, {RepositoryID: "repo-hash-branches", BaseBranch: "feature-a", DefaultBranch: "main", PRNumber: 202, Position: 1}, }) - createTestEnvironment(t, repo, "env-hash-branches", "task-123") + envRepos := make([]*models.TaskEnvironmentRepo, 0, len(plans)) for index, plan := range plans { - if err := repo.CreateTaskEnvironmentRepo(ctx, &models.TaskEnvironmentRepo{ + envRepos = append(envRepos, &models.TaskEnvironmentRepo{ ID: fmt.Sprintf("session-worktree-hash-%d", index), TaskEnvironmentID: "env-hash-branches", WorktreeID: fmt.Sprintf("worktree-hash-%d", index), RepositoryID: "repo-hash-branches", BranchSlug: plan.IdentitySlug, CreatedAt: now, - }); err != nil { - t.Fatalf("CreateTaskEnvironmentRepo %q: %v", plan.IdentitySlug, err) - } + }) } + createTestEnvironmentWithRepos(t, repo, "env-hash-branches", "task-123", envRepos) info, err := svc.GetWorkspaceInfoForSession(ctx, "task-123", "session-hash-branches") if err != nil { @@ -1110,10 +1118,9 @@ func TestGetWorkspaceInfoForSession_ProjectsOnlyLifecycleValidRepositories(t *te if err := repo.CreateTaskSession(ctx, &models.TaskSession{ID: "session-projection-validity", TaskID: "task-123", TaskEnvironmentID: "env-projection-validity", State: models.TaskSessionStateCompleted, StartedAt: now, UpdatedAt: now}); err != nil { t.Fatalf("CreateTaskSession: %v", err) } - createTestEnvironment(t, repo, "env-projection-validity", "task-123") - if err := repo.CreateTaskEnvironmentRepo(ctx, &models.TaskEnvironmentRepo{ID: "session-worktree-projection-validity", TaskEnvironmentID: "env-projection-validity", WorktreeID: "worktree-projection-validity", RepositoryID: "repo-provider-recovery", BranchSlug: "main", CreatedAt: now}); err != nil { - t.Fatalf("CreateTaskEnvironmentRepo: %v", err) - } + createTestEnvironmentWithRepos(t, repo, "env-projection-validity", "task-123", []*models.TaskEnvironmentRepo{ + {ID: "session-worktree-projection-validity", TaskEnvironmentID: "env-projection-validity", WorktreeID: "worktree-projection-validity", RepositoryID: "repo-provider-recovery", BranchSlug: "main", CreatedAt: now}, + }) info, err := svc.GetWorkspaceInfoForSession(ctx, "task-123", "session-projection-validity") if err != nil { diff --git a/apps/web/components/task/chat/message-list-native-scroll.ts b/apps/web/components/task/chat/message-list-native-scroll.ts index a34b8f2fdd..63567900c6 100644 --- a/apps/web/components/task/chat/message-list-native-scroll.ts +++ b/apps/web/components/task/chat/message-list-native-scroll.ts @@ -1,3 +1,5 @@ +/* eslint-disable max-lines -- pagination, scroll anchoring, and retry state share one boundary. */ + import { useCallback, useEffect, useLayoutEffect, useRef } from "react"; import { useDockviewStore } from "@/lib/state/dockview-store"; import { useAppStoreApi } from "@/components/state-provider"; @@ -348,6 +350,8 @@ export function useAutoScroll(params: { const storeApi = useAppStoreApi(); const isNearBottomRef = useRef(true); const prevIsWorkingRef = useRef(isWorking); + const prevEnabledRef = useRef(enabled); + const frozenScrollTopRef = useRef(null); const resyncIsNearBottom = useCallback(() => { const el = scrollRef.current; @@ -374,6 +378,7 @@ export function useAutoScroll(params: { * coalesced persistence of the scroll position. */ const onScroll = () => { resyncIsNearBottom(); + if (!enabled) frozenScrollTopRef.current = el.scrollTop; coalescer.schedule(); }; el.addEventListener("scroll", onScroll, { passive: true }); @@ -385,7 +390,29 @@ export function useAutoScroll(params: { // if a coalesced write above was still pending. coalescer.flush(); }; - }, [scrollRef, sessionId, storeApi, resyncIsNearBottom]); + }, [scrollRef, sessionId, storeApi, resyncIsNearBottom, enabled]); + + // Own the disabled offset across every transcript layout update. Sending a + // prompt can briefly shrink the scroll range before the new message row is + // committed, which makes the browser clamp scrollTop even with + // overflow-anchor disabled. Keep the pre-update offset and reapply it after + // each message/working-state render so that transient clamp cannot move the + // reader. Real user scroll events update the owned offset above. + useLayoutEffect(() => { + const el = scrollRef.current; + if (!el) return; + const wasEnabled = prevEnabledRef.current; + prevEnabledRef.current = enabled; + if (enabled) { + frozenScrollTopRef.current = null; + return; + } + if (wasEnabled || frozenScrollTopRef.current === null) { + frozenScrollTopRef.current = el.scrollTop; + return; + } + el.scrollTop = frozenScrollTopRef.current; + }, [enabled, isWorking, messages, scrollRef]); // When isWorking transitions to true, force scroll to bottom (unless // disabled, locked, or a layout rebuild scroll restore is pending). diff --git a/apps/web/components/task/chat/message-list-native.test.tsx b/apps/web/components/task/chat/message-list-native.test.tsx index 0923dcd0af..0798b0634e 100644 --- a/apps/web/components/task/chat/message-list-native.test.tsx +++ b/apps/web/components/task/chat/message-list-native.test.tsx @@ -112,11 +112,13 @@ function AutoScrollHarness({ hasUnreadDivider, messages = TEST_MESSAGES, markRef, + enabled = true, }: { isWorking: boolean; hasUnreadDivider: boolean; messages?: Message[]; markRef?: { current?: () => void }; + enabled?: boolean; }) { const scrollRef = useRef(null); const { markNotNearBottom } = useAutoScroll({ @@ -124,7 +126,7 @@ function AutoScrollHarness({ messages, isWorking, sessionId: null, - enabled: true, + enabled, hasUnreadDivider, isProgrammaticScrollLocked: NEVER_LOCKED, }); @@ -405,6 +407,33 @@ describe("useScrollToDividerOrBottom — anchored-bar offset", () => { expect(scrollContainer.scrollTop).toBe(123); }); + it("restores the disabled offset after a transient layout clamp", () => { + const { rerender } = render( + , + ); + const scrollContainer = document.querySelector( + '[data-testid="auto-scroll-container"]', + ); + if (!scrollContainer) throw new Error("auto-scroll container did not render"); + setScrollMetrics(scrollContainer); + scrollContainer.scrollTop = 600; + + rerender(); + // Model the browser temporarily reducing the maximum scroll offset while + // the composer clears, before the appended transcript row is committed. + scrollContainer.scrollTop = 568; + rerender( + , + ); + + expect(scrollContainer.scrollTop).toBe(600); + }); + it("never re-scrolls once the reader has started scrolling, even if the anchored bar's height changes afterward", () => { const { rerender, container } = render(); const scrollContainer = container.querySelector( diff --git a/apps/web/components/task/use-subtask-submit.test.ts b/apps/web/components/task/use-subtask-submit.test.ts index 879644aab7..b61b06b1be 100644 --- a/apps/web/components/task/use-subtask-submit.test.ts +++ b/apps/web/components/task/use-subtask-submit.test.ts @@ -254,7 +254,8 @@ describe("useSubtaskSubmit", () => { }); it("sends the auto-title contract without a title", async () => { - const opts = makeSubmitOptions({ autoTitle: true }); + const onClose = vi.fn(); + const opts = makeSubmitOptions({ autoTitle: true, onClose }); const { result } = renderHook(() => useSubtaskSubmit(opts)); await act(async () => { @@ -269,6 +270,9 @@ describe("useSubtaskSubmit", () => { expect(mockSetActiveTask).toHaveBeenCalledWith(CREATED_TASK_ID); expect(mockSetActiveSession).toHaveBeenCalledWith(CREATED_TASK_ID, CREATED_SESSION_ID); expect(mockReplaceTaskUrl).toHaveBeenCalledWith(CREATED_TASK_ID); + expect(onClose.mock.invocationCallOrder[0]).toBeLessThan( + mockReplaceTaskUrl.mock.invocationCallOrder[0], + ); }); it("sends the autopilot creation flag for a subtask", async () => { diff --git a/apps/web/components/task/use-subtask-submit.ts b/apps/web/components/task/use-subtask-submit.ts index fe2b236f4b..6bd4b32616 100644 --- a/apps/web/components/task/use-subtask-submit.ts +++ b/apps/web/components/task/use-subtask-submit.ts @@ -52,6 +52,7 @@ type CreateSubtaskArgs = { workspaceMode: SubtaskWorkspaceMode; isLocalExecutor: boolean; freshBranchEnabled: boolean; + onClose: () => void; setActiveTask: (taskId: string) => void; setActiveSession: (taskId: string, sessionId: string) => void; }; @@ -71,6 +72,7 @@ async function createSubtask({ workspaceMode, isLocalExecutor, freshBranchEnabled, + onClose, setActiveTask, setActiveSession, }: CreateSubtaskArgs) { @@ -104,6 +106,9 @@ async function createSubtask({ workspace_mode: workspaceMode, autopilot: autopilot || undefined, }); + // Close the dialog before navigation. Navigation can remount the sidebar + // that owns the dialog state, which makes a later close update a stale owner. + onClose(); const newSessionId = response.session_id ?? response.primary_session_id ?? null; if (newSessionId) { setActiveTask(response.id); @@ -171,10 +176,10 @@ export function useSubtaskSubmit(opts: UseSubtaskSubmitOpts) { workspaceMode, isLocalExecutor, freshBranchEnabled, + onClose, setActiveTask, setActiveSession, }); - onClose(); } catch (error) { toast({ title: t("task:failedToCreateSubtask"), diff --git a/apps/web/e2e/fixtures/git-shim.mjs b/apps/web/e2e/fixtures/git-shim.mjs index 790e9c0275..e16b5fe4e7 100644 --- a/apps/web/e2e/fixtures/git-shim.mjs +++ b/apps/web/e2e/fixtures/git-shim.mjs @@ -12,7 +12,8 @@ // - Skip leading Git global options when finding the subcommand. // - If KANDEV_E2E_GIT_DELAY_FILE holds a positive ms value and the subcommand // is `fetch`/`pull`, sleep that long before running real git (simulates slow -// network git so prepare-panel streaming stays observable). +// network git so prepare-panel streaming stays observable). A JSON object +// with `startedFile` and `releaseFile` provides a deterministic test gate. // - If the subcommand is `push` and KANDEV_E2E_GITLAB_PUSH_FILE matches the // repo's origin remote, record the push args and exit 0 without pushing. // - Otherwise exec the real git binary with the original args, restoring the @@ -73,9 +74,19 @@ function runRealGit(args, extraEnv) { function maybeDelay(subcommand) { if (subcommand !== "fetch" && subcommand !== "pull") return; const raw = readFileSafe(process.env.KANDEV_E2E_GIT_DELAY_FILE); - if (!/^[0-9]+$/.test(raw)) return; - const delayMs = Number(raw); - if (delayMs > 0) sleepMs(delayMs); + if (/^[0-9]+$/.test(raw)) { + const delayMs = Number(raw); + if (delayMs > 0) sleepMs(delayMs); + return; + } + try { + const gate = JSON.parse(raw); + if (typeof gate.startedFile !== "string" || typeof gate.releaseFile !== "string") return; + fs.writeFileSync(gate.startedFile, "started"); + while (!fs.existsSync(gate.releaseFile)) sleepMs(50); + } catch { + // Invalid or absent delay configuration means transparent passthrough. + } } /** diff --git a/apps/web/e2e/pages/session-page.ts b/apps/web/e2e/pages/session-page.ts index 0b2a6c9fbd..677c685cc3 100644 --- a/apps/web/e2e/pages/session-page.ts +++ b/apps/web/e2e/pages/session-page.ts @@ -1299,20 +1299,12 @@ export class SessionPage { .waitFor({ state: "hidden", timeout }); } - /** - * Wait for the terminal shell to be connected (buffer has content from - * the prompt), then type a command and press Enter. - */ + /** Wait for the terminal WebSocket to connect, then type a command and press Enter. */ async typeInTerminal(command: string): Promise { await this.expectTerminalConnected(); - await expect - .poll(async () => (await this.readXtermBuffer("terminal-panel")).length > 0, { - timeout: TERMINAL_READY_TIMEOUT, - message: "Waiting for terminal shell to connect", - }) - .toBe(true); const xterm = this.activePanel("terminal-panel").locator(".xterm"); + await expect(xterm).toBeVisible(); await xterm.click(); await this.page.keyboard.type(command); await this.page.keyboard.press("Enter"); diff --git a/apps/web/e2e/tests/git/fork-pr-comparison-target-helpers.ts b/apps/web/e2e/tests/git/fork-pr-comparison-target-helpers.ts index defd0f0df6..34a4532ceb 100644 --- a/apps/web/e2e/tests/git/fork-pr-comparison-target-helpers.ts +++ b/apps/web/e2e/tests/git/fork-pr-comparison-target-helpers.ts @@ -59,7 +59,7 @@ export async function seedForkPRComparisonTask( execFileSync( "git", ["config", "--global", `url.file://${comparisonTargetPath}.insteadOf`, targetURL], - { env: gitEnv }, + { cwd: backend.tmpDir, env: gitEnv }, ); await apiClient.updateRepository(seedData.repositoryId, { @@ -70,24 +70,6 @@ export async function seedForkPRComparisonTask( provider_name: "widget-fork", }); - const task = await apiClient.createTaskWithAgent( - seedData.workspaceId, - "Fork PR comparison target", - seedData.agentProfileId, - { - description: "/e2e:simple-message", - workflow_id: seedData.workflowId, - workflow_step_id: seedData.startStepId, - repositories: [ - { - repository_id: seedData.repositoryId, - base_branch: "main", - checkout_branch: "feature/fork", - }, - ], - }, - ); - await apiClient.mockGitHubReset(); await apiClient.mockGitHubSetUser("contributor"); await apiClient.mockGitHubAddPRs([ @@ -127,12 +109,33 @@ export async function seedForkPRComparisonTask( stats_available: true, }, ]); + + const task = await apiClient.createTask(seedData.workspaceId, "Fork PR comparison target", { + description: "/e2e:simple-message", + agent_profile_id: seedData.agentProfileId, + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + repositories: [ + { + repository_id: seedData.repositoryId, + base_branch: "main", + checkout_branch: "feature/fork", + }, + ], + }); + await apiClient.associateGitHubTaskPR({ workspace_id: seedData.workspaceId, task_id: task.id, repository_id: seedData.repositoryId, pr_url: "https://github.com/upstream/widget/pull/1701", }); + await apiClient.launchSession({ + task_id: task.id, + agent_profile_id: seedData.agentProfileId, + workflow_step_id: seedData.startStepId, + prompt: "/e2e:simple-message", + }); return { task, headSHA }; } diff --git a/apps/web/e2e/tests/git/fork-pr-comparison-target.spec.ts b/apps/web/e2e/tests/git/fork-pr-comparison-target.spec.ts index 91fc6d4c9f..789503ac3a 100644 --- a/apps/web/e2e/tests/git/fork-pr-comparison-target.spec.ts +++ b/apps/web/e2e/tests/git/fork-pr-comparison-target.spec.ts @@ -4,7 +4,7 @@ import { SessionPage } from "../../pages/session-page"; import { seedForkPRComparisonTask } from "./fork-pr-comparison-target-helpers"; test.describe("Fork pull-request comparison target", () => { - test.describe.configure({ retries: 1, timeout: 120_000 }); + test.describe.configure({ timeout: 120_000 }); test("uses the upstream target for one fork commit and three files", async ({ testPage, diff --git a/apps/web/e2e/tests/git/mobile-fork-pr-comparison-target.spec.ts b/apps/web/e2e/tests/git/mobile-fork-pr-comparison-target.spec.ts index 37c391a5f0..a7ca352c12 100644 --- a/apps/web/e2e/tests/git/mobile-fork-pr-comparison-target.spec.ts +++ b/apps/web/e2e/tests/git/mobile-fork-pr-comparison-target.spec.ts @@ -17,7 +17,7 @@ async function expandSection(toggle: Locator) { } test.describe("Mobile fork pull-request comparison target", () => { - test.describe.configure({ retries: 1, timeout: 120_000 }); + test.describe.configure({ timeout: 120_000 }); test("shows the same upstream target in the touch branch drawer", async ({ testPage, diff --git a/apps/web/e2e/tests/gitlab/mr-status-chip.spec.ts b/apps/web/e2e/tests/gitlab/mr-status-chip.spec.ts index 76e65f7a59..b9bca72362 100644 --- a/apps/web/e2e/tests/gitlab/mr-status-chip.spec.ts +++ b/apps/web/e2e/tests/gitlab/mr-status-chip.spec.ts @@ -71,10 +71,14 @@ async function openTask( await session.waitForLoad(); // The shell hydrates the workspace MR map once per document. A link created // immediately before navigation can miss that first snapshot even though - // the task details already show the association. Reload once so the chip - // observes the same persisted link as the rest of the task page. - await testPage.reload(); - await session.waitForLoad(); + // the task details already show the association. Re-drive document + // hydration until the linked-MR chip observes the persisted map; one fixed + // reload can race the same snapshot again under CI load. + await expect(async () => { + await testPage.reload(); + await session.waitForLoad(); + await expect(session.mrStatusChip()).toBeVisible({ timeout: 5_000 }); + }).toPass({ timeout: 30_000 }); } test.describe("GitLab MR status chip", () => { diff --git a/apps/web/e2e/tests/layout/changes-panel-focus.spec.ts b/apps/web/e2e/tests/layout/changes-panel-focus.spec.ts index 83fadf8bdf..b0a2c081f2 100644 --- a/apps/web/e2e/tests/layout/changes-panel-focus.spec.ts +++ b/apps/web/e2e/tests/layout/changes-panel-focus.spec.ts @@ -174,6 +174,15 @@ async function moveChangesToChatGroupAndFocusChat(testPage: Page) { } test.describe("Changes panel focus behavior", () => { + test.beforeEach(({ backend, seedData }) => { + // Requesting seedData guarantees that the repository and its offline + // origin exist before this hook runs. + const git = new GitHelper(seedData.repositoryPath, makeGitEnv(backend.tmpDir)); + const seedCommit = git.exec("git rev-list --max-parents=0 HEAD").trim(); + git.exec(`git reset --hard ${seedCommit}`); + git.exec("git clean -fd"); + }); + /** * Verifies the changes panel does NOT steal focus from the chat tab * on page refresh when the task has existing git changes/commits. diff --git a/apps/web/e2e/tests/lsp/lsp-e2e-helpers.ts b/apps/web/e2e/tests/lsp/lsp-e2e-helpers.ts index fe35ae7fc3..8617474ec9 100644 --- a/apps/web/e2e/tests/lsp/lsp-e2e-helpers.ts +++ b/apps/web/e2e/tests/lsp/lsp-e2e-helpers.ts @@ -95,6 +95,11 @@ export async function createKotlinTask( path.join(backend.tmpDir, "repos", options.repositoryDirectory ?? "e2e-repo"), makeGitEnv(backend.tmpDir), ); + // Task repositories use main unless a test explicitly supplies another + // branch. A preceding Git test can leave the shared seed repository checked + // out on a feature branch; committing the LSP fixture there makes the task's + // main checkout legitimately omit the file. + git.exec("git checkout -f main"); for (const [index, filePath] of filePaths.entries()) { const isTypeScript = filePath.endsWith(".ts"); const defaultContent = isTypeScript @@ -132,6 +137,13 @@ export async function createKotlinTask( ); if (!task.session_id) throw new Error("createTaskWithAgent did not return a session_id"); + await expect + .poll(async () => (await apiClient.getTaskEnvironment(task.id))?.status ?? null, { + timeout: 45_000, + message: "Kotlin task environment did not become ready", + }) + .toBe("ready"); + const session = new SessionPage(page); if (options.navigate !== false) { await page.goto(`/t/${task.id}`); @@ -259,16 +271,7 @@ export async function openDesktopFile( session: SessionPage, filePath: string, ): Promise { - await session.clickTab("Files"); - await expect(session.files).toBeVisible({ timeout: 10_000 }); const pathSegments = filePath.split("/"); - for (let index = 1; index < pathSegments.length; index++) { - const ancestor = session.fileTreeNode(pathSegments.slice(0, index).join("/")); - await expect(ancestor).toBeVisible({ timeout: 15_000 }); - if ((await ancestor.locator(".tabler-icon-chevron-right").count()) > 0) { - await ancestor.click(); - } - } const fileNode = session.fileTreeNode(filePath); // Git status updates can auto-activate the Changes panel just after the // file tree renders. Re-select Files and click only while the row is still @@ -281,6 +284,14 @@ export async function openDesktopFile( async () => { try { await session.clickTab("Files", { force: true }); + if (!(await session.files.isVisible())) return false; + for (let index = 1; index < pathSegments.length; index++) { + const ancestor = session.fileTreeNode(pathSegments.slice(0, index).join("/")); + if (!(await ancestor.isVisible())) return false; + if ((await ancestor.locator(".tabler-icon-chevron-right").count()) > 0) { + await ancestor.click(); + } + } if (!(await fileNode.isVisible())) return false; await fileNode.click({ timeout: 2_000 }); return true; diff --git a/apps/web/e2e/tests/plugins/mobile-task-list-facet.spec.ts b/apps/web/e2e/tests/plugins/mobile-task-list-facet.spec.ts index b852773685..6b43d7c256 100644 --- a/apps/web/e2e/tests/plugins/mobile-task-list-facet.spec.ts +++ b/apps/web/e2e/tests/plugins/mobile-task-list-facet.spec.ts @@ -32,8 +32,17 @@ test.describe("Mobile plugin task-list facet", () => { await testPage.getByRole("button", { name: "Open menu" }).tap(); const menu = testPage.getByRole("dialog", { name: "Menu" }); - await menu.getByTestId("mobile-tasks-list-group").tap(); - await testPage.getByRole("listbox").getByRole("option", { name: "Fixture tag" }).tap(); + const groupSelect = menu.getByTestId("mobile-tasks-list-group"); + await groupSelect.evaluate((element) => element.scrollIntoView({ block: "center" })); + await expect(groupSelect).toBeInViewport(); + const listbox = testPage.getByRole("listbox"); + await expect(async () => { + if (!(await listbox.isVisible())) await groupSelect.tap(); + const fixtureTag = listbox.getByRole("option", { name: "Fixture tag" }); + await expect(fixtureTag).toBeInViewport({ timeout: 1_000 }); + await fixtureTag.click(); + await expect(groupSelect).toContainText("Fixture tag", { timeout: 1_000 }); + }).toPass({ timeout: 10_000 }); const section = testPage.getByTestId("tasks-list-section"); await expect(section).toHaveCount(1); diff --git a/apps/web/e2e/tests/pr/pr-switcher-changes.spec.ts b/apps/web/e2e/tests/pr/pr-switcher-changes.spec.ts index 4cdd7fde21..8229bddae4 100644 --- a/apps/web/e2e/tests/pr/pr-switcher-changes.spec.ts +++ b/apps/web/e2e/tests/pr/pr-switcher-changes.spec.ts @@ -201,7 +201,9 @@ test.describe("PR switcher changes panel", () => { // --- Switch to Task B --- await session.taskInSidebar("Dashboard Task").click(); - await expect(testPage).toHaveURL(/\/t\//, { timeout: 15_000 }); + await expect(testPage).toHaveURL((url) => url.pathname.includes(taskB.id), { + timeout: 15_000, + }); await session.clickTab("Changes"); // Wait for PR data to load for Task B @@ -221,16 +223,23 @@ test.describe("PR switcher changes panel", () => { // --- Switch to Task C (no PR) --- await session.taskInSidebar("No PR Task").click(); - await expect(testPage).toHaveURL(/\/t\//, { timeout: 15_000 }); + await expect(testPage).toHaveURL((url) => url.pathname.includes(taskC.id), { + timeout: 15_000, + }); await session.clickTab("Changes"); - // PR sections should NOT be visible for a task without a PR + // PR-specific content must not leak from the previous task. The unified + // commits section can still be present for ordinary local commits, even + // when the task has no linked PR. await expect(session.prFilesSection()).not.toBeVisible({ timeout: 10_000 }); - await expect(session.commitsSection()).not.toBeVisible(); + await expect(session.changes.getByText("add dashboard component")).not.toBeVisible(); + await expect(session.changes.getByText("add api client")).not.toBeVisible(); // --- Switch back to Task A to confirm data reappears --- await session.taskInSidebar("Auth Fix Task").click(); - await expect(testPage).toHaveURL(/\/t\//, { timeout: 15_000 }); + await expect(testPage).toHaveURL((url) => url.pathname.includes(taskA.id), { + timeout: 15_000, + }); await session.clickTab("Changes"); await expect(session.prFilesSection()).toBeVisible({ timeout: 15_000 }); diff --git a/apps/web/e2e/tests/session/session-layout.spec.ts b/apps/web/e2e/tests/session/session-layout.spec.ts index 8ff9624438..ca6f11b54e 100644 --- a/apps/web/e2e/tests/session/session-layout.spec.ts +++ b/apps/web/e2e/tests/session/session-layout.spec.ts @@ -119,7 +119,12 @@ test.describe("Session layout", () => { await sessionB.showSessionContext(30_000); await sessionB.waitForChatIdle({ timeout: 30_000 }); - // Task B should have default (non-maximized) layout + // Git updates can legitimately focus Changes while the task settles. The + // maximize invariant is about group visibility, so select Files before + // asserting the default right-column panel. + await sessionB.clickTab("Files"); + + // Task B should have default (non-maximized) layout. await sessionB.expectDefaultLayout(); // Navigate back to Task A's session URL — this triggers a full page load, diff --git a/apps/web/e2e/tests/session/setup-script-progress.spec.ts b/apps/web/e2e/tests/session/setup-script-progress.spec.ts index 07e3dd97f1..70629b957a 100644 --- a/apps/web/e2e/tests/session/setup-script-progress.spec.ts +++ b/apps/web/e2e/tests/session/setup-script-progress.spec.ts @@ -37,24 +37,24 @@ test.describe("Setup script progress UX", () => { }) => { test.setTimeout(120_000); - // Slow `git fetch` (worktree sync step) to keep the prepare panel reliably - // expanded while the page mounts and the WS subscribes — without this the - // whole prepare can finish before the client is listening, so events are - // missed and the running state is never observed. Same shim pattern as - // `long-prepare-panels.spec.ts`. The final `prepare.completed` payload - // carries all captured stdout, so the streaming assertion is still valid - // even if the page races past the per-progress events. - const delayFile = path.join(backend.tmpDir, "git-delay-ms"); + // Gate the preceding fetch until the browser subscribes, then hold the + // setup script so its preparing state and streamed output stay observable. + const gateID = Date.now(); + const gitGateFile = path.join(backend.tmpDir, "git-delay-ms"); + const gitStartedFile = path.join(backend.tmpDir, `git-started-${gateID}`); + const gitReleaseFile = path.join(backend.tmpDir, `git-release-${gateID}`); + const startedFile = path.join(backend.tmpDir, `setup-started-${gateID}`); + const releaseFile = path.join(backend.tmpDir, `setup-release-${gateID}`); let profile: { id: string } | null = null; try { - // Inside the try so the finally always cleans up — if profile creation - // throws after the shim is written, leaving the delay file behind would - // contaminate every other test in this worker. - fs.writeFileSync(delayFile, "5000"); - + fs.writeFileSync( + gitGateFile, + JSON.stringify({ startedFile: gitStartedFile, releaseFile: gitReleaseFile }), + ); const setupScript = [ "echo '[setup] installing deps'", - "sleep 1", + `touch '${startedFile}'`, + `while [ ! -f '${releaseFile}' ]; do sleep 0.1; done`, "echo '[setup] ready'", ].join("; "); profile = await createWorktreeProfileWithScript( @@ -80,6 +80,20 @@ test.describe("Setup script progress UX", () => { const session = new SessionPage(testPage); await session.waitForLoad(); + await expect + .poll(() => fs.existsSync(gitStartedFile), { + message: "repository preparation should reach its deterministic git gate", + timeout: 45_000, + }) + .toBe(true); + fs.writeFileSync(gitReleaseFile, "release"); + await expect + .poll(() => fs.existsSync(startedFile), { + message: "setup script should reach its deterministic test gate", + timeout: 45_000, + }) + .toBe(true); + // Panel appears inline in the chat, expanded while running. const panel = testPage.getByTestId("prepare-progress-panel"); await expect(panel).toBeVisible({ timeout: 15_000 }); @@ -90,17 +104,25 @@ test.describe("Setup script progress UX", () => { // Setup script output reaches the expanded step list — either streamed // in real time or captured from the final `prepare.completed` payload. await expect(panel).toContainText("[setup] installing deps", { timeout: 30_000 }); + fs.writeFileSync(releaseFile, "release"); // On clean success the panel stays visible but auto-collapses. await expect(panel).toHaveAttribute("data-status", "completed", { timeout: 30_000 }); await expect(panel).toHaveAttribute("data-expanded", "false"); } finally { - if (fs.existsSync(delayFile)) fs.unlinkSync(delayFile); + // Always release a setup process that is still waiting before teardown. + if (!fs.existsSync(gitReleaseFile)) fs.writeFileSync(gitReleaseFile, "release"); + if (!fs.existsSync(releaseFile)) fs.writeFileSync(releaseFile, "release"); if (profile) { await apiClient.deleteExecutorProfile(profile.id).catch(() => { // Profile may already be deleted if the test tore down mid-run. }); } + if (fs.existsSync(gitGateFile)) fs.unlinkSync(gitGateFile); + if (fs.existsSync(gitStartedFile)) fs.unlinkSync(gitStartedFile); + if (fs.existsSync(gitReleaseFile)) fs.unlinkSync(gitReleaseFile); + if (fs.existsSync(startedFile)) fs.unlinkSync(startedFile); + if (fs.existsSync(releaseFile)) fs.unlinkSync(releaseFile); } }); diff --git a/apps/web/e2e/tests/task/create-task-branch-policy.spec.ts b/apps/web/e2e/tests/task/create-task-branch-policy.spec.ts index d8beedd8c2..9d756f3654 100644 --- a/apps/web/e2e/tests/task/create-task-branch-policy.spec.ts +++ b/apps/web/e2e/tests/task/create-task-branch-policy.spec.ts @@ -15,6 +15,10 @@ test.describe("Task creation with branch policies", () => { backend, seedData, }) => { + execSync("git clean -fd", { + cwd: seedData.repositoryPath, + env: makeGitEnv(backend.tmpDir), + }); execSync("git branch -f develop", { cwd: seedData.repositoryPath, env: makeGitEnv(backend.tmpDir), @@ -44,6 +48,9 @@ test.describe("Task creation with branch policies", () => { await expect(dialog).toBeVisible(); await dialog.getByTestId("executor-profile-selector").click(); await testPage.getByRole("option", { name: new RegExp(localProfile.name) }).click(); + await expect(dialog.getByTestId("executor-profile-selector")).toContainText( + localProfile.name, + ); await dialog.getByTestId("branch-chip-trigger").click(); const option = testPage.getByRole("option", { name: new RegExp(policy.name) }); await expect(option).toContainText("Policy"); @@ -55,7 +62,9 @@ test.describe("Task creation with branch policies", () => { ); await testPage.mouse.move(0, 0); await policyInfo.focus(); - await expect(testPage.getByRole("tooltip")).toContainText( + await expect(policyInfo).toBeFocused(); + await expect(policyInfo).toHaveAttribute( + "aria-label", "Base: main. Template: feature/{title}-{suffix}. Pull request target: develop.", ); await option.click(); @@ -70,7 +79,7 @@ test.describe("Task creation with branch policies", () => { await dialog.getByTestId("task-description-input").fill("Create from a branch policy"); await dialog.getByTestId("submit-start-agent-chevron").click(); await testPage.getByTestId("submit-create-without-agent").click(); - await expect(dialog).not.toBeVisible(); + await expect(dialog).not.toBeVisible({ timeout: 30_000 }); let created: { id: string; title: string } | undefined; await expect @@ -160,6 +169,9 @@ test.describe("Task creation with branch policies", () => { await expect(dialog).toBeVisible(); await dialog.getByTestId("executor-profile-selector").click(); await testPage.getByRole("option", { name: new RegExp(localProfile.name) }).click(); + await expect(dialog.getByTestId("executor-profile-selector")).toContainText( + localProfile.name, + ); await dialog.getByTestId("add-repository").click(); const repositoryChips = dialog.getByTestId("repo-chip-trigger"); diff --git a/apps/web/e2e/tests/task/mobile-create-task-branch-policy.spec.ts b/apps/web/e2e/tests/task/mobile-create-task-branch-policy.spec.ts index 4a4d8016ef..f5c67aff1a 100644 --- a/apps/web/e2e/tests/task/mobile-create-task-branch-policy.spec.ts +++ b/apps/web/e2e/tests/task/mobile-create-task-branch-policy.spec.ts @@ -39,25 +39,39 @@ test.describe("Task branch policy selection on mobile", () => { await mobile.mobileFab.tap(); const dialog = testPage.getByTestId("create-task-dialog"); await expect(dialog).toBeVisible(); - await dialog.getByTestId("executor-profile-selector").tap(); - await testPage.getByRole("option", { name: new RegExp(localProfile.name) }).tap(); - await dialog.getByTestId("branch-chip-trigger").tap(); - const option = testPage.getByRole("option", { name: new RegExp(policy.name) }); + const executorSelector = dialog.getByTestId("executor-profile-selector"); + await expect(async () => { + await executorSelector.tap(); + await testPage.getByRole("option", { name: new RegExp(localProfile.name) }).click(); + await expect(executorSelector).toContainText(localProfile.name, { timeout: 1_000 }); + }).toPass({ timeout: 10_000 }); + const branchTrigger = dialog.getByTestId("branch-chip-trigger"); + await branchTrigger.tap(); + const policyOption = () => testPage.getByRole("option", { name: new RegExp(policy.name) }); + const option = policyOption(); await expect(option).toContainText("Policy"); + await expect(option).not.toHaveAttribute("aria-disabled", "true", { timeout: 30_000 }); await expectPolicyOptionUsesOneLine(option, policy.name); - await testPage.getByTestId(`branch-policy-option-info-${policy.id}`).tap(); - await expect(testPage.getByRole("dialog", { name: policy.name })).toContainText( - "Base: main. Template: mobile/{title}-{suffix}. Pull request target: main.", - ); - await testPage.keyboard.press("Escape"); - await dialog.getByTestId("branch-chip-trigger").tap(); - const reopenedOption = testPage.getByRole("option", { name: new RegExp(policy.name) }); - await reopenedOption.tap({ force: true }); - await expect(dialog.getByTestId("branch-chip-trigger")).toContainText(policy.name); + await expect(async () => { + if (!(await policyOption().isVisible())) await branchTrigger.tap(); + await policyOption().evaluate((element) => (element as HTMLElement).click()); + await expect(branchTrigger).toContainText(policy.name, { timeout: 1_000 }); + }).toPass({ timeout: 10_000 }); await expect(dialog.getByTestId("fresh-branch-toggle")).toHaveAttribute( "aria-pressed", "true", ); + + await branchTrigger.tap(); + const policyDetails = testPage.getByRole("dialog", { name: policy.name }); + await expect(async () => { + if (!(await policyOption().isVisible())) await branchTrigger.tap(); + await testPage.getByTestId(`branch-policy-option-info-${policy.id}`).dispatchEvent("click"); + await expect(policyDetails).toBeVisible({ timeout: 1_000 }); + }).toPass({ timeout: 10_000 }); + await expect(policyDetails).toContainText( + "Base: main. Template: mobile/{title}-{suffix}. Pull request target: main.", + ); expect( await testPage.evaluate(() => document.documentElement.scrollWidth), ).toBeLessThanOrEqual(390); diff --git a/apps/web/e2e/tests/task/mobile-sidebar-task-actions.spec.ts b/apps/web/e2e/tests/task/mobile-sidebar-task-actions.spec.ts index 741cabcd61..c0dd7ea08b 100644 --- a/apps/web/e2e/tests/task/mobile-sidebar-task-actions.spec.ts +++ b/apps/web/e2e/tests/task/mobile-sidebar-task-actions.spec.ts @@ -863,8 +863,14 @@ test.describe("Mobile sidebar task actions", () => { const dialog = testPage.getByTestId("new-subtask-dialog"); await dialog.getByTestId("subtask-workspace-mode-new").tap(); - await dialog.getByTestId("executor-profile-selector").tap(); - await testPage.getByRole("option", { name: new RegExp(localProfile.name) }).tap(); + const executorSelector = dialog.getByTestId("executor-profile-selector"); + await expect(async () => { + await executorSelector.tap(); + await testPage + .getByRole("option", { name: new RegExp(localProfile.name) }) + .tap({ force: true }); + await expect(executorSelector).toContainText(localProfile.name, { timeout: 1_000 }); + }).toPass({ timeout: 10_000 }); await dialog.getByTestId("branch-chip-trigger").tap(); await testPage.getByRole("option", { name: new RegExp(policy.name) }).tap({ force: true }); await expect(dialog.getByTestId("fresh-branch-toggle")).toHaveAttribute( @@ -873,7 +879,10 @@ test.describe("Mobile sidebar task actions", () => { ); await dialog.getByTestId("subtask-title-input").fill(childTitle); await dialog.getByTestId("subtask-prompt-input").fill("/e2e:simple-message"); - await dialog.getByRole("button", { name: "Create Subtask", exact: true }).tap(); + const createSubtask = dialog.getByRole("button", { name: "Create Subtask", exact: true }); + await expect(executorSelector).toContainText(localProfile.name); + await expect(createSubtask).toBeEnabled(); + await createSubtask.tap(); await expect(dialog).toBeHidden({ timeout: 30_000 }); let childId: string | undefined; await expect diff --git a/apps/web/e2e/tests/task/mobile-task-listing-display.spec.ts b/apps/web/e2e/tests/task/mobile-task-listing-display.spec.ts index eec9182d0d..16bf1859d6 100644 --- a/apps/web/e2e/tests/task/mobile-task-listing-display.spec.ts +++ b/apps/web/e2e/tests/task/mobile-task-listing-display.spec.ts @@ -82,6 +82,11 @@ test.describe("Mobile task listing display preferences", () => { await testPage.getByRole("button", { name: "Open menu" }).click(); const tasksMenu = testPage.getByRole("dialog", { name: "Menu" }); await tasksMenu.getByText("Show task details", { exact: true }).click(); + await expect + .poll(async () => (await apiClient.getUserSettings()).settings.tasks_list_show_details, { + message: "task detail preference was not persisted", + }) + .toBe(true); await testPage.keyboard.press("Escape"); await expect(tasksMenu).toHaveCount(0); diff --git a/apps/web/e2e/tests/task/sessionless-switch-panel-leak.spec.ts b/apps/web/e2e/tests/task/sessionless-switch-panel-leak.spec.ts index 162aa2110c..6ca7d98c7b 100644 --- a/apps/web/e2e/tests/task/sessionless-switch-panel-leak.spec.ts +++ b/apps/web/e2e/tests/task/sessionless-switch-panel-leak.spec.ts @@ -10,6 +10,7 @@ import { createStandardProfile, } from "../../helpers/git-helper"; import { SessionPage } from "../../pages/session-page"; +import { waitForLatestSessionDone } from "../../helpers/session"; /** * Regression: switching from a task with env-scoped panels open (file-editor, @@ -36,12 +37,16 @@ async function setupTaskWithFilePanel(args: { makeGitEnv(args.backendTmpDir), ); const filename = "leak-fixture.ts"; + // Local-executor specs can leave the worker-scoped seed clone on a task + // branch. Make this fixture independent of file execution order. + git.exec("git checkout main"); git.createFile(filename, "// leak fixture\n"); git.stageAll(); git.commit("seed leak fixture"); + git.exec("git push origin main"); const profile = await createStandardProfile(args.apiClient, "panel-leak-source"); - await args.apiClient.createTaskWithAgent( + const task = await args.apiClient.createTaskWithAgent( args.seedData.workspaceId, "Panel Leak Source", profile.id, @@ -52,6 +57,12 @@ async function setupTaskWithFilePanel(args: { repository_ids: [args.seedData.repositoryId], }, ); + await waitForLatestSessionDone( + args.apiClient, + task.id, + 1, + "source task did not finish preparing its workspace", + ); const session = await openTaskSession(args.testPage, "Panel Leak Source"); await session.clickTab("Files"); @@ -87,8 +98,6 @@ const NON_DEFAULT_ENV_SCOPED_COMPONENTS = [ ]; test.describe("Sessionless task switch — env-scoped panel cleanup", () => { - test.describe.configure({ retries: 1 }); - test("dropping into a sessionless task releases the previous task's env-scoped panels", async ({ testPage, apiClient, diff --git a/apps/web/e2e/tests/task/subtask.spec.ts b/apps/web/e2e/tests/task/subtask.spec.ts index 2f02a653a4..260f183225 100644 --- a/apps/web/e2e/tests/task/subtask.spec.ts +++ b/apps/web/e2e/tests/task/subtask.spec.ts @@ -303,8 +303,12 @@ test.describe("Subtask basics", () => { await dialog.getByTestId("subtask-workspace-mode-new").click(); await dialog.getByTestId("executor-profile-selector").click(); await testPage.getByRole("option", { name: new RegExp(localProfile.name) }).click(); + await expect(dialog.getByTestId("executor-profile-selector")).toContainText( + localProfile.name, + ); await dialog.getByTestId("branch-chip-trigger").click(); await testPage.getByRole("option", { name: new RegExp(policy.name) }).click(); + await expect(dialog.getByTestId("branch-chip-trigger")).toContainText(policy.name); await expect(dialog.getByTestId("fresh-branch-toggle")).toHaveAttribute( "aria-pressed", "true",