From fd9b78e58d3220cce517097ba83532cf2b6f5bee Mon Sep 17 00:00:00 2001 From: Henry Date: Sat, 12 Sep 2026 00:43:49 +0800 Subject: [PATCH] fix(tasks): resolve launch repositories from the task attachment set (#3582) Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com> Co-authored-by: Carlos Florencio --- .../executor/executor_environment_reuse.go | 14 +- ...ecutor_environment_reuse_inventory_test.go | 125 ++++ .../executor/executor_mocks_test.go | 41 +- .../orchestrator/executor/executor_resume.go | 75 ++- ...cutor_resume_repository_resolution_test.go | 572 ++++++++++++++++++ .../repository/sqlite/task_environment.go | 59 +- .../sqlite/task_environment_test.go | 60 ++ .../task/repository/sqlite/task_repository.go | 4 +- .../task_repository_ordering_postgres_test.go | 90 +++ .../sqlite/task_repository_ordering_test.go | 123 ++++ docs/specs/tasks/README.md | 2 + .../launch-repository-resolution.md | 325 ++++++++++ .../launch-repository-resolution.md | 457 ++++++++++++++ 13 files changed, 1909 insertions(+), 38 deletions(-) create mode 100644 apps/backend/internal/orchestrator/executor/executor_resume_repository_resolution_test.go create mode 100644 apps/backend/internal/task/repository/sqlite/task_repository_ordering_postgres_test.go create mode 100644 apps/backend/internal/task/repository/sqlite/task_repository_ordering_test.go create mode 100644 docs/specs/tasks/requirements/launch-repository-resolution.md create mode 100644 docs/specs/tasks/system-design/launch-repository-resolution.md diff --git a/apps/backend/internal/orchestrator/executor/executor_environment_reuse.go b/apps/backend/internal/orchestrator/executor/executor_environment_reuse.go index 95cba186ab..fff109cf9c 100644 --- a/apps/backend/internal/orchestrator/executor/executor_environment_reuse.go +++ b/apps/backend/internal/orchestrator/executor/executor_environment_reuse.go @@ -58,7 +58,7 @@ func (e *Executor) validateReuseEnvironmentInventory(ctx context.Context, req *L return nil } for _, spec := range specs { - if canonicalInventoryMatches(spec, rows, req.UseWorktree) != 1 { + if got := canonicalInventoryMatches(spec, rows, req.UseWorktree); got != 1 { return fmt.Errorf("%w: canonical workspace repository inventory has no matching entry for repository %q branch %q", models.ErrWorkspaceReuseUnsafe, spec.RepositoryID, launchRepoBranchIdentitySlug(spec)) } @@ -112,10 +112,18 @@ func currentTaskDirName(env *models.TaskEnvironment) string { func canonicalInventoryMatches(spec RepoSpec, rows []*models.TaskEnvironmentRepo, useWorktree bool) int { matches := 0 expectedBranchSlug := launchRepoBranchIdentitySlug(spec) - allowLegacyEmptyBranch := expectedBranchSlug != "" && !repositoryHasBranchScopedRepoRow(rows, spec.RepositoryID) + // A non-worktree launch with no expected branch slug is not "legacy data + // with unknown branch" — it is a local/local_pc resume, where + // applyResumeRepoConfig deliberately never stamps req.BaseBranch because + // LocalPreparer keeps whatever branch is already checked out on disk. + // Branch identity is not tracked for this launch at all, so it cannot be + // compared against a branch-scoped canonical row; match on repository + // identity alone, same as the untracked-inventory case below. + branchIdentityUntracked := !useWorktree && expectedBranchSlug == "" + allowLegacyEmptyBranch := !branchIdentityUntracked && expectedBranchSlug != "" && !repositoryHasBranchScopedRepoRow(rows, spec.RepositoryID) for _, row := range rows { branchMatches := worktree.SanitizeBranchSlug(row.BranchSlug) == expectedBranchSlug - if allowLegacyEmptyBranch && row.BranchSlug == "" { + if branchIdentityUntracked || (allowLegacyEmptyBranch && row.BranchSlug == "") { branchMatches = true } if row.RepositoryID != spec.RepositoryID || !branchMatches { 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 index 32321b574d..17eb27a15d 100644 --- a/apps/backend/internal/orchestrator/executor/executor_environment_reuse_inventory_test.go +++ b/apps/backend/internal/orchestrator/executor/executor_environment_reuse_inventory_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/kandev/kandev/internal/task/models" + v1 "github.com/kandev/kandev/pkg/api/v1" ) // Regression coverage for the "failed prepare leaves a permanently bricked @@ -84,3 +85,127 @@ func TestValidateReuseEnvironmentInventory_ScopedBranchPlusLegacyEmptyRowAttache t.Fatalf("validateReuseEnvironmentInventory() = %v, want nil", err) } } + +// TestPrepareResumeRepositorySettings_GuestSessionReuseValidatesAgainstResolvedBaseBranch +// is a regression found in review round 1: a guest session resuming a shared +// task environment (MaterializationSessionID belongs to a different session, +// so WorkspaceReuseRequired is true) on a clone-URL executor never got +// req.BaseBranch stamped, because applyResumeCloneURL — the only writer of +// req.BaseBranch outside the worktree path — short-circuits whenever +// WorkspaceReuseRequired is true. validateReuseEnvironmentInventory then derives +// the expected branch identity slug from req.BaseBranch (via +// topLevelLaunchRepoSpec/topLevelBranchIdentitySlug) and compared an empty +// fallback against the canonical inventory's real branch slug, refusing every +// resume of a genuinely-matching environment with ErrWorkspaceReuseUnsafe. +func TestPrepareResumeRepositorySettings_GuestSessionReuseValidatesAgainstResolvedBaseBranch(t *testing.T) { + repo := newMockRepository() + repo.repositories["repo-1"] = &models.Repository{ID: "repo-1", LocalPath: "/tmp/repo", RemoteURL: "https://example.com/repo-1.git"} + repo.taskRepositories["tr-1"] = &models.TaskRepository{ + ID: "tr-1", TaskID: "task-1", RepositoryID: "repo-1", Position: 0, BaseBranch: "feature-x", + } + repo.tasks["task-1"] = &models.Task{ID: "task-1"} + canonicalRows := []*models.TaskEnvironmentRepo{ + {TaskEnvironmentID: "env-1", RepositoryID: "repo-1", BranchSlug: "feature-x", Status: taskEnvironmentRepoStatusActive}, + } + env := &models.TaskEnvironment{ + ID: "env-1", TaskID: "task-1", ExecutorType: "local_docker", + // A different session materialized this environment: this session is a + // guest reusing it, the population WorkspaceReuseRequired gates on. + MaterializationSessionID: "sess-owner", + Repos: canonicalRows, + } + repo.taskEnvironments[env.ID] = env + repo.taskEnvironmentRepos[env.ID] = canonicalRows + + task := &v1.Task{ID: "task-1"} + session := &models.TaskSession{ID: "sess-guest", TaskID: "task-1", TaskEnvironmentID: "env-1"} + e := newTestExecutor(t, &mockAgentManager{}, repo) + req := &LaunchAgentRequest{TaskID: "task-1", SessionID: "sess-guest", ExecutorType: "local_docker"} + + if _, _, _, err := e.prepareResumeRepositorySettings(context.Background(), task, session, req); err != nil { + t.Fatalf("prepareResumeRepositorySettings() = %v, want nil: a guest session reusing a shared environment whose canonical inventory actually matches must not be refused", err) + } + if !req.WorkspaceReuseRequired { + t.Fatalf("req.WorkspaceReuseRequired = false, want true for a live guest-session reuse") + } +} + +// TestPrepareResumeRepositorySettings_LocalExecutorResumeWithUntrackedBranchMatches +// is a second review-round regression: an ordinary (non-guest) session resume +// on a local/local_pc executor also sets WorkspaceReuseRequired whenever the +// task already has a TaskEnvironmentID, which is the common case on every +// backend restart. applyResumeRepoConfig now stamps req.RepositoryID +// unconditionally on every executor type, so topLevelLaunchRepoSpec started +// returning ok=true for local/local_pc launches that previously never reached +// validateReuseEnvironmentInventory at all. But req.BaseBranch is still never +// stamped for a non-clone-URL, non-worktree executor (LocalPreparer keeps +// whatever branch is already checked out on disk), so the derived branch +// identity slug was empty while the canonical inventory's row carried a real +// branch slug — refusing every ordinary local-executor resume with +// ErrWorkspaceReuseUnsafe. Confirmed against the live code with +// KANDEV_E2E_SKIP_FRESHNESS=1 pnpm e2e:raw tests/session/session-recovery.spec.ts, +// which failed identically before this fix. +func TestPrepareResumeRepositorySettings_LocalExecutorResumeWithUntrackedBranchMatches(t *testing.T) { + repo := newMockRepository() + repo.repositories["repo-1"] = &models.Repository{ID: "repo-1", LocalPath: "/tmp/repo"} + repo.taskRepositories["tr-1"] = &models.TaskRepository{ + ID: "tr-1", TaskID: "task-1", RepositoryID: "repo-1", Position: 0, + } + repo.tasks["task-1"] = &models.Task{ID: "task-1"} + canonicalRows := []*models.TaskEnvironmentRepo{ + {TaskEnvironmentID: "env-1", RepositoryID: "repo-1", BranchSlug: "main", Status: taskEnvironmentRepoStatusActive}, + } + env := &models.TaskEnvironment{ + ID: "env-1", TaskID: "task-1", ExecutorType: "local", + Repos: canonicalRows, + } + repo.taskEnvironments[env.ID] = env + repo.taskEnvironmentRepos[env.ID] = canonicalRows + + task := &v1.Task{ID: "task-1"} + session := &models.TaskSession{ID: "sess-1", TaskID: "task-1", TaskEnvironmentID: "env-1"} + e := newTestExecutor(t, &mockAgentManager{}, repo) + req := &LaunchAgentRequest{TaskID: "task-1", SessionID: "sess-1", ExecutorType: "local", WorkspaceReuseRequired: true} + + if _, _, _, err := e.prepareResumeRepositorySettings(context.Background(), task, session, req); err != nil { + t.Fatalf("prepareResumeRepositorySettings() = %v, want nil: an ordinary local-executor resume whose canonical inventory actually matches on repository identity must not be refused for lacking a tracked branch", err) + } +} + +// TestValidateReuseEnvironmentInventory_NonWorktreeLegacyToleranceRequiresRealMismatch +// is a third regression: a non-worktree (local/local_pc) launch never +// populates TaskEnvironmentRepo.WorktreeID, since that column only has +// meaning on the worktree path. canonicalInventoryMatches used to gate its +// "legacy empty-branch" tolerance on hasBranchScopedEnvironmentRepoRows, which +// requires WorktreeID != "" to recognize a row as carrying real branch +// identity. For a non-worktree launch that check was always false — even when +// a row's BranchSlug held a real value like "main" — so the tolerance branch +// treated every non-worktree canonical inventory as "legacy", let an +// unrelated stray empty-branch-slug row (written by a distinct bug: an +// untracked-branch resume race writing a duplicate row instead of updating +// the existing branch-scoped one in place) also count as a match, and a spec +// requiring branch "main" matched twice instead of once. Confirmed against +// the live code with workflow-session-targeting.spec.ts's "delivers reuse and +// fresh initial targets" case, which failed with the same "canonical +// workspace repository inventory has no matching entry" error (got matches=2) +// before this fix. +func TestValidateReuseEnvironmentInventory_NonWorktreeLegacyToleranceRequiresRealMismatch(t *testing.T) { + repo := newMockRepository() + e := newTestExecutor(t, &mockAgentManager{}, repo) + req := &LaunchAgentRequest{ + TaskID: "task-1", + WorkspaceReuseRequired: true, + RepositoryID: "repo-1", + BaseBranch: "main", + } + env := &models.TaskEnvironment{ID: "env-1"} + repo.taskEnvironmentRepos[env.ID] = []*models.TaskEnvironmentRepo{ + {RepositoryID: "repo-1", BranchSlug: "main", Status: taskEnvironmentRepoStatusActive}, + {RepositoryID: "repo-1", BranchSlug: "", Status: taskEnvironmentRepoStatusActive}, + } + + err := e.validateReuseEnvironmentInventory(context.Background(), req, env) + if err != nil { + t.Fatalf("validateReuseEnvironmentInventory() = %v, want nil: a spec requiring branch %q must match only the branch-scoped row, not also the stray empty-branch row", err, "main") + } +} diff --git a/apps/backend/internal/orchestrator/executor/executor_mocks_test.go b/apps/backend/internal/orchestrator/executor/executor_mocks_test.go index 87a2e62245..bf983a420a 100644 --- a/apps/backend/internal/orchestrator/executor/executor_mocks_test.go +++ b/apps/backend/internal/orchestrator/executor/executor_mocks_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "github.com/kandev/kandev/internal/task/repository" + "sort" "sync" "testing" "time" @@ -294,6 +295,9 @@ type mockRepository struct { createTaskEnvironmentRepoErr error finalizeTaskEnvironmentErr error createTaskSessionFunc func(ctx context.Context, session *models.TaskSession) error + // listTaskRepositoriesFunc, when non-nil, overrides ListTaskRepositories + // entirely — used to simulate a transient attachment-set read failure. + listTaskRepositoriesFunc func(ctx context.Context, taskID string) ([]*models.TaskRepository, error) // getTaskSessionByTaskAndAgentFunc, when non-nil, overrides // GetTaskSessionByTaskAndAgent entirely — used to simulate a transient // lookup failure (e.g. the AC-003.7 re-read-after-conflict arm in @@ -391,13 +395,18 @@ func newMockRepository() *mockRepository { // Implement required repository methods func (m *mockRepository) GetPrimaryTaskRepository(ctx context.Context, taskID string) (*models.TaskRepository, error) { - // Return first matching repository for the task (matches sqlite implementation) - for _, tr := range m.taskRepositories { - if tr.TaskID == taskID { - return tr, nil - } + // Delegate to ListTaskRepositories, mirroring the sqlite implementation + // (GetPrimaryTaskRepository there is a thin wrapper over + // ListTaskRepositories()[0]) so the mock's primary selection agrees with + // its own ordering instead of raw map iteration order. + repos, err := m.ListTaskRepositories(ctx, taskID) + if err != nil { + return nil, err } - return nil, nil + if len(repos) == 0 { + return nil, nil + } + return repos[0], nil } func (m *mockRepository) GetRepository(ctx context.Context, id string) (*models.Repository, error) { @@ -809,18 +818,28 @@ func (m *mockRepository) GetTaskRepository(ctx context.Context, id string) (*mod return nil, nil } func (m *mockRepository) ListTaskRepositories(ctx context.Context, taskID string) ([]*models.TaskRepository, error) { + if m.listTaskRepositoriesFunc != nil { + return m.listTaskRepositoriesFunc(ctx, taskID) + } var out []*models.TaskRepository for _, tr := range m.taskRepositories { if tr.TaskID == taskID { out = append(out, tr) } } - // Stable order by Position so callers (and tests) see deterministic results. - for i := 1; i < len(out); i++ { - for j := i; j > 0 && out[j].Position < out[j-1].Position; j-- { - out[j], out[j-1] = out[j-1], out[j] + // Order by Position, then CreatedAt, then ID, mirroring the production + // ORDER BY position ASC, created_at ASC, id ASC: map iteration order is + // random, so ties left unbroken would make tests flaky rather than + // deterministic. + sort.Slice(out, func(i, j int) bool { + if out[i].Position != out[j].Position { + return out[i].Position < out[j].Position } - } + if !out[i].CreatedAt.Equal(out[j].CreatedAt) { + return out[i].CreatedAt.Before(out[j].CreatedAt) + } + return out[i].ID < out[j].ID + }) return out, nil } func (m *mockRepository) ListTaskWorkspaceFolders(context.Context, string) ([]*models.TaskWorkspaceFolder, error) { diff --git a/apps/backend/internal/orchestrator/executor/executor_resume.go b/apps/backend/internal/orchestrator/executor/executor_resume.go index 6d72ebd80d..ac2db51a71 100644 --- a/apps/backend/internal/orchestrator/executor/executor_resume.go +++ b/apps/backend/internal/orchestrator/executor/executor_resume.go @@ -1701,7 +1701,7 @@ func (e *Executor) applyResumeRepoConfig( if err != nil { return "", err } - repositoryID, baseBranch := resolveResumeRepoIDAndBranch(task, session) + repositoryID, baseBranch := resolveResumeRepoIDAndBranch(session, allRepos) baseBranch = resolveResumeBaseBranch(repositoryID, baseBranch, allRepos) if baseBranch != "" { req.Branch = baseBranch @@ -1709,6 +1709,26 @@ func (e *Executor) applyResumeRepoConfig( if repositoryID == "" { return "", nil } + if session.RepositoryID == "" { + e.logger.Info("resolved resume repository from task attachment set", + zap.String("task_id", task.ID), + zap.String("session_id", session.ID), + zap.String("repository_id", repositoryID)) + } + // Stamp the resolved primary's identity on every executor type, whether or + // not it has a local clone: environmentReposForLaunch's single-repository + // fallback projects an inventory row from req.RepositoryID alone, and a + // task with exactly one attachment on a non-worktree executor reaches it + // with nothing else populated. + req.RepositoryID = repositoryID + // Clone-URL executors need BaseBranch for validateReuseEnvironmentInventory's + // branch identity slug even when workspace reuse is required: applyResumeCloneURL + // is the only other writer of req.BaseBranch outside the worktree path, and it + // skips clone-URL setup entirely once reuse is required. Local executors are + // excluded so LocalPreparer keeps using the current on-disk branch. + if baseBranch != "" && e.capabilities != nil && e.capabilities.RequiresCloneURL(req.ExecutorType) { + req.BaseBranch = baseBranch + } var repository *models.Repository for _, info := range allRepos { @@ -1735,7 +1755,7 @@ func (e *Executor) applyResumeRepoConfig( } repositoryPath := repository.LocalPath - applyResumeRepoBasics(req, repository, repositoryPath) + applyResumeRepoBasics(req, repository, repositoryPath, shouldUseWorktree(req.ExecutorType)) for _, info := range allRepos { if info != nil && info.RepositoryID == repositoryID { req.ContributionDestination = info.ContributionDestination @@ -1766,19 +1786,17 @@ func (e *Executor) resumeRepoSet(ctx context.Context, taskID string, resolved .. } // resolveResumeRepoIDAndBranch picks the primary repositoryID and baseBranch -// for a resume, preferring the session's persisted values and falling back to -// the task's primary repository when the session row was created before those -// fields existed. -func resolveResumeRepoIDAndBranch(task *v1.Task, session *models.TaskSession) (string, string) { +// for a resume: the session's persisted preference when it is set, otherwise +// the primary of the resolved task attachment set — its first entry, the same +// definition GetPrimaryTaskRepository uses. A preference naming a repository +// absent from the attachment set is used unchanged; resolution of that +// repository is left to the caller's GetRepository fallback. +func resolveResumeRepoIDAndBranch(session *models.TaskSession, allRepos []*repoInfo) (string, string) { repositoryID := session.RepositoryID - if repositoryID == "" && len(task.Repositories) > 0 { - repositoryID = task.Repositories[0].RepositoryID + if repositoryID == "" && len(allRepos) > 0 && allRepos[0] != nil { + repositoryID = allRepos[0].RepositoryID } - baseBranch := session.BaseBranch - if baseBranch == "" && len(task.Repositories) > 0 && task.Repositories[0].BaseBranch != "" { - baseBranch = task.Repositories[0].BaseBranch - } - return repositoryID, baseBranch + return repositoryID, session.BaseBranch } // resolveResumeBaseBranch prefers the current task-repository row when a @@ -1804,12 +1822,26 @@ func resolveResumeBaseBranch(repositoryID, sessionBaseBranch string, repos []*re return sessionBaseBranch } -// applyResumeRepoBasics copies the repository's local path and setup script -// onto the request. Pulled out of applyResumeRepoConfig so the parent's -// cyclomatic complexity stays inside the lint budget. -func applyResumeRepoBasics(req *LaunchAgentRequest, repository *models.Repository, repositoryPath string) { +// applyResumeRepoBasics copies the repository's local path, name, and setup +// script onto the request. RepositoryPath/RepoName are stamped unconditionally +// on every executor type, mirroring the initial-launch path +// (applyRepositoryConfig): reconcileWorkspaceRepositories rejects a synthesized +// spec whose RepositoryPath or RepoName is empty, and RepoSpecs() now +// synthesizes a spec for any executor once req.RepositoryID is set. Pulled out +// of applyResumeRepoConfig so the parent's cyclomatic complexity stays inside +// the lint budget. +func applyResumeRepoBasics(req *LaunchAgentRequest, repository *models.Repository, repositoryPath string, useWorktree bool) { if repositoryPath != "" { req.RepositoryURL = repositoryPath + req.RepositoryPath = repositoryPath + } + if useWorktree { + req.RepoName = repository.Name + } else { + req.RepoName = worktree.SanitizeRepoDirName(repository.Name) + if req.RepoName == "" { + req.RepoName = worktree.SanitizeRepoDirName(req.RepositoryID) + } } if repository.SetupScript != "" { if req.Metadata == nil { @@ -1828,6 +1860,15 @@ func (e *Executor) applyResumeCloneURL(req *LaunchAgentRequest, repository *mode return nil } cloneURL := repositoryCloneURL(repository) + // Local Docker can bind-mount a task's checked-out source directory and + // clone it there, mirroring the initial-launch path (applyRepositoryConfig): + // RepositoryPath is authoritative for this launch even when the persisted + // generic repository has no provider identity or origin URL (as in + // workspace-source and E2E fixtures). Without this fallback, a repository + // attached with only a local path can launch initially but never resume. + if cloneURL == "" && req.ExecutorType == string(models.ExecutorTypeLocalDocker) { + cloneURL = dockerLocalCloneSource(req.RepositoryPath) + } if cloneURL == "" { return ErrNoCloneURL } diff --git a/apps/backend/internal/orchestrator/executor/executor_resume_repository_resolution_test.go b/apps/backend/internal/orchestrator/executor/executor_resume_repository_resolution_test.go new file mode 100644 index 0000000000..59f7f2b47b --- /dev/null +++ b/apps/backend/internal/orchestrator/executor/executor_resume_repository_resolution_test.go @@ -0,0 +1,572 @@ +package executor + +import ( + "context" + "errors" + "testing" + "time" + + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" + + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/task/models" + v1 "github.com/kandev/kandev/pkg/api/v1" +) + +type sshCloneURLCapabilities struct{ mockCapabilities } + +func (m *sshCloneURLCapabilities) RequiresCloneURL(executorType string) bool { + return executorType == string(models.ExecutorTypeSSH) || m.mockCapabilities.RequiresCloneURL(executorType) +} + +// These tests cover REQ-TASKS-LAUNCH-REPOSITORY-RESOLUTION-001 and -002: a +// launch must resolve its repositories from the task's attachment set +// (task_repositories) rather than the session's stale repository_id/base_branch +// snapshot, so a repository attached after a session already exists is not +// permanently unlaunchable. + +// TestApplyResumeRepoConfig_ResolvesPrimaryFromAttachmentSetWhenSessionHasNoPreference +// covers AC-...-001.1: a session created before its task had any attachments +// carries an empty repository preference. The launch must still resolve a +// primary from the task's current attachment set. +func TestApplyResumeRepoConfig_ResolvesPrimaryFromAttachmentSetWhenSessionHasNoPreference(t *testing.T) { + repo := newMockRepository() + repo.repositories["repo-1"] = &models.Repository{ + ID: "repo-1", + LocalPath: "/tmp/repo", + } + repo.taskRepositories["tr-1"] = &models.TaskRepository{ + ID: "tr-1", TaskID: "task-1", RepositoryID: "repo-1", Position: 0, BaseBranch: "main", + } + repo.tasks["task-1"] = &models.Task{ID: "task-1"} + task := &v1.Task{ID: "task-1"} + session := &models.TaskSession{ + ID: "sess-1", + TaskID: "task-1", + // RepositoryID intentionally empty: the repository was attached after + // this session was created. + } + exec := newTestExecutor(t, &mockAgentManager{}, repo) + + req := &LaunchAgentRequest{TaskID: "task-1", SessionID: "sess-1", ExecutorType: "worktree"} + + gotID, err := exec.applyResumeRepoConfig(context.Background(), task, session, req, nil) + if err != nil { + t.Fatalf("applyResumeRepoConfig: %v", err) + } + if gotID != "repo-1" { + t.Fatalf("resolved repositoryID = %q, want repo-1", gotID) + } +} + +// TestApplyResumeRepoConfig_LogsInfoOnlyWhenFallingBackToAttachmentSet covers +// the design's Observability requirement: a resume that falls back to the +// task attachment set because the session carried no preference must log an +// info line with the task id, session id, and resolved repository id, so the +// repaired-session population is distinguishable in logs from a session that +// always had a preference — which must not log anything. +func TestApplyResumeRepoConfig_LogsInfoOnlyWhenFallingBackToAttachmentSet(t *testing.T) { + newSetup := func(sessionRepositoryID string) (*mockRepository, *v1.Task, *models.TaskSession) { + repo := newMockRepository() + repo.repositories["repo-1"] = &models.Repository{ID: "repo-1", LocalPath: "/tmp/repo"} + repo.taskRepositories["tr-1"] = &models.TaskRepository{ + ID: "tr-1", TaskID: "task-1", RepositoryID: "repo-1", Position: 0, BaseBranch: "main", + } + repo.tasks["task-1"] = &models.Task{ID: "task-1"} + return repo, &v1.Task{ID: "task-1"}, &models.TaskSession{ID: "sess-1", TaskID: "task-1", RepositoryID: sessionRepositoryID} + } + + t.Run("empty preference logs the fallback", func(t *testing.T) { + repo, task, session := newSetup("") + core, logs := observer.New(zapcore.InfoLevel) + log, err := logger.NewFromZap(zap.New(core)) + if err != nil { + t.Fatalf("NewFromZap: %v", err) + } + exec := NewExecutor(&mockAgentManager{}, repo, log, ExecutorConfig{ShellPrefs: &mockShellPrefs{}}) + exec.SetCapabilities(&mockCapabilities{}) + req := &LaunchAgentRequest{TaskID: "task-1", SessionID: "sess-1", ExecutorType: "worktree"} + + if _, err := exec.applyResumeRepoConfig(context.Background(), task, session, req, nil); err != nil { + t.Fatalf("applyResumeRepoConfig: %v", err) + } + entries := logs.FilterMessage("resolved resume repository from task attachment set").All() + if len(entries) != 1 { + t.Fatalf("info entries = %d, want 1; all=%v", len(entries), logs.All()) + } + fields := entries[0].ContextMap() + if fields["task_id"] != "task-1" || fields["session_id"] != "sess-1" || fields["repository_id"] != "repo-1" { + t.Fatalf("log fields = %+v, want task_id=task-1 session_id=sess-1 repository_id=repo-1", fields) + } + }) + + t.Run("present preference logs nothing", func(t *testing.T) { + repo, task, session := newSetup("repo-1") + core, logs := observer.New(zapcore.InfoLevel) + log, err := logger.NewFromZap(zap.New(core)) + if err != nil { + t.Fatalf("NewFromZap: %v", err) + } + exec := NewExecutor(&mockAgentManager{}, repo, log, ExecutorConfig{ShellPrefs: &mockShellPrefs{}}) + exec.SetCapabilities(&mockCapabilities{}) + req := &LaunchAgentRequest{TaskID: "task-1", SessionID: "sess-1", ExecutorType: "worktree"} + + if _, err := exec.applyResumeRepoConfig(context.Background(), task, session, req, nil); err != nil { + t.Fatalf("applyResumeRepoConfig: %v", err) + } + if n := logs.FilterMessage("resolved resume repository from task attachment set").Len(); n != 0 { + t.Fatalf("info entries = %d, want 0 for a session with a present preference; all=%v", n, logs.All()) + } + }) +} + +// TestApplyResumeRepoConfig_KeepsSessionPreferenceWhenPresentInAttachmentSet +// covers AC-...-001.3: a non-empty session preference naming a repository +// still present in the attachment set is used unchanged, not re-derived from +// the set's ordering. +func TestApplyResumeRepoConfig_KeepsSessionPreferenceWhenPresentInAttachmentSet(t *testing.T) { + repo := newMockRepository() + repo.repositories["repo-primary"] = &models.Repository{ID: "repo-primary", LocalPath: "/tmp/primary"} + repo.repositories["repo-secondary"] = &models.Repository{ID: "repo-secondary", LocalPath: "/tmp/secondary"} + repo.taskRepositories["tr-1"] = &models.TaskRepository{ + ID: "tr-1", TaskID: "task-1", RepositoryID: "repo-primary", Position: 0, BaseBranch: "main", + } + repo.taskRepositories["tr-2"] = &models.TaskRepository{ + ID: "tr-2", TaskID: "task-1", RepositoryID: "repo-secondary", Position: 1, BaseBranch: "main", + } + repo.tasks["task-1"] = &models.Task{ID: "task-1"} + task := &v1.Task{ID: "task-1"} + session := &models.TaskSession{ + ID: "sess-1", + TaskID: "task-1", + RepositoryID: "repo-secondary", + } + exec := newTestExecutor(t, &mockAgentManager{}, repo) + + req := &LaunchAgentRequest{TaskID: "task-1", SessionID: "sess-1", ExecutorType: "worktree"} + + gotID, err := exec.applyResumeRepoConfig(context.Background(), task, session, req, nil) + if err != nil { + t.Fatalf("applyResumeRepoConfig: %v", err) + } + if gotID != "repo-secondary" { + t.Fatalf("resolved repositoryID = %q, want repo-secondary (the session's stored preference, not the set's first attachment)", gotID) + } +} + +// TestApplyResumeRepoConfig_StampsRepositoryIdentityOnNonWorktreeExecutor +// covers the mandatory non-worktree regression: the reported failure runs on +// a remote (SSH) executor, and req.RepositoryID was only ever stamped inside +// the worktree-only branch. A single-attachment launch on a non-worktree +// executor must still carry the resolved primary's identity so +// environmentReposForLaunch can produce an inventory row and the guard admits +// the environment. +func TestApplyResumeRepoConfig_StampsRepositoryIdentityOnNonWorktreeExecutor(t *testing.T) { + repo := newMockRepository() + repo.repositories["repo-1"] = &models.Repository{ID: "repo-1", LocalPath: "/tmp/repo", RemoteURL: "https://example.com/repo-1.git"} + repo.taskRepositories["tr-1"] = &models.TaskRepository{ + ID: "tr-1", TaskID: "task-1", RepositoryID: "repo-1", Position: 0, BaseBranch: "main", + } + repo.tasks["task-1"] = &models.Task{ID: "task-1"} + task := &v1.Task{ID: "task-1"} + session := &models.TaskSession{ + ID: "sess-1", + TaskID: "task-1", + // Empty preference: the repository was attached after session creation. + } + exec := newTestExecutor(t, &mockAgentManager{}, repo) + exec.SetCapabilities(&sshCloneURLCapabilities{}) + + req := &LaunchAgentRequest{TaskID: "task-1", SessionID: "sess-1", ExecutorType: "ssh"} + + if _, err := exec.applyResumeRepoConfig(context.Background(), task, session, req, nil); err != nil { + t.Fatalf("applyResumeRepoConfig: %v", err) + } + if req.RepositoryID != "repo-1" { + t.Fatalf("req.RepositoryID = %q, want repo-1: a non-worktree executor must still carry the resolved primary's identity", req.RepositoryID) + } + if req.UseWorktree { + t.Fatalf("expected UseWorktree=false for the ssh executor") + } + if req.RepositoryURL != "https://example.com/repo-1.git" { + t.Fatalf("req.RepositoryURL = %q, want the repository clone URL", req.RepositoryURL) + } + if req.BaseBranch != "main" { + t.Fatalf("req.BaseBranch = %q, want main for an SSH clone launch", req.BaseBranch) + } + inventory := environmentReposForLaunch(req, &LaunchAgentResponse{}) + if len(inventory) != 1 || inventory[0].RepositoryID != "repo-1" || inventory[0].BranchSlug != "main" { + t.Fatalf("resume inventory = %#v, want one repo-1/main slot", inventory) + } +} + +// TestApplyResumeRepoConfig_StampsRepositoryPathAndNameOnLocalExecutor covers +// a regression found in review round 1: the unconditional req.RepositoryID +// stamp above made LaunchRequest.RepoSpecs() synthesize a non-nil spec for +// the "local"/"local_pc" executor (previously RepoSpecs() returned nil there +// since neither RepositoryID nor RepositoryPath were ever set on resume). +// reconcileWorkspaceRepositories rejects a spec whose RepoName/RepositoryPath +// are empty as "invalid durable workspace repository", so every local-executor +// resume that resolves a primary now failed the launch outright. RepositoryPath +// and RepoName must get the same unconditional treatment RepositoryID did, +// mirroring the initial-launch path (applyRepositoryConfig, +// executor_execute.go:1954), which sets them together with no executor-type +// conjunct. +func TestApplyResumeRepoConfig_StampsRepositoryPathAndNameOnLocalExecutor(t *testing.T) { + repo := newMockRepository() + repo.repositories["repo-1"] = &models.Repository{ID: "repo-1", Name: "repo-1", LocalPath: "/tmp/repo"} + repo.taskRepositories["tr-1"] = &models.TaskRepository{ + ID: "tr-1", TaskID: "task-1", RepositoryID: "repo-1", Position: 0, BaseBranch: "main", + } + repo.tasks["task-1"] = &models.Task{ID: "task-1"} + task := &v1.Task{ID: "task-1"} + session := &models.TaskSession{ + ID: "sess-1", + TaskID: "task-1", + RepositoryID: "repo-1", // present preference: the previously-working case that also broke. + } + exec := newTestExecutor(t, &mockAgentManager{}, repo) + + req := &LaunchAgentRequest{TaskID: "task-1", SessionID: "sess-1", ExecutorType: "local"} + + if _, err := exec.applyResumeRepoConfig(context.Background(), task, session, req, nil); err != nil { + t.Fatalf("applyResumeRepoConfig: %v", err) + } + if req.RepositoryID != "repo-1" { + t.Fatalf("req.RepositoryID = %q, want repo-1", req.RepositoryID) + } + if req.RepositoryPath != "/tmp/repo" { + t.Fatalf("req.RepositoryPath = %q, want /tmp/repo: reconcileWorkspaceRepositories needs a non-empty path for the local executor", req.RepositoryPath) + } + if req.RepoName == "" { + t.Fatalf("req.RepoName is empty, want a filesystem-safe name: reconcileWorkspaceRepositories rejects an empty RepoName") + } +} + +// TestApplyResumeRepoConfig_StampsRepositoryIdentityOnNonWorktreeExecutorWithPresentPreference +// covers test-supervisor's must-fix: AC-001.4's non-worktree arm was only +// tested with an EMPTY session preference. The unconditional stamp added in +// review round 1 also changes behavior for a session that carries a PRESENT +// preference on a non-worktree executor — previously req.RepositoryID was +// never stamped there either (same worktree-only gate), so no inventory row +// was ever produced for that population. This is newly reachable behavior, +// not just the newly-fixed empty-preference population. +func TestApplyResumeRepoConfig_StampsRepositoryIdentityOnNonWorktreeExecutorWithPresentPreference(t *testing.T) { + repo := newMockRepository() + repo.repositories["repo-1"] = &models.Repository{ID: "repo-1", LocalPath: "/tmp/repo", RemoteURL: "https://example.com/repo-1.git"} + repo.taskRepositories["tr-1"] = &models.TaskRepository{ + ID: "tr-1", TaskID: "task-1", RepositoryID: "repo-1", Position: 0, BaseBranch: "main", + } + repo.tasks["task-1"] = &models.Task{ID: "task-1"} + task := &v1.Task{ID: "task-1"} + session := &models.TaskSession{ + ID: "sess-1", + TaskID: "task-1", + RepositoryID: "repo-1", // present preference, not the empty-preference population. + } + exec := newTestExecutor(t, &mockAgentManager{}, repo) + exec.SetCapabilities(&sshCloneURLCapabilities{}) + + req := &LaunchAgentRequest{TaskID: "task-1", SessionID: "sess-1", ExecutorType: "ssh"} + + if _, err := exec.applyResumeRepoConfig(context.Background(), task, session, req, nil); err != nil { + t.Fatalf("applyResumeRepoConfig: %v", err) + } + if req.RepositoryID != "repo-1" { + t.Fatalf("req.RepositoryID = %q, want repo-1: a present preference on a non-worktree executor must also carry the identity", req.RepositoryID) + } + if req.RepositoryURL != "https://example.com/repo-1.git" || req.BaseBranch != "main" { + t.Fatalf("SSH resume request = URL %q, base branch %q; want clone URL and main", req.RepositoryURL, req.BaseBranch) + } + inventory := environmentReposForLaunch(req, &LaunchAgentResponse{}) + if len(inventory) != 1 || inventory[0].RepositoryID != "repo-1" || inventory[0].BranchSlug != "main" { + t.Fatalf("resume inventory = %#v, want one repo-1/main slot", inventory) + } +} + +// TestApplyResumeRepoConfig_EmptyAttachmentSetAndEmptyPreferenceLaunchesWithNoRepoConfig +// covers AC-...-001.5: a genuinely repository-less task whose session carries +// no preference must launch with no repository configuration and no +// req.Repositories, not an error. Flagged as untested by test-supervisor: this +// is the Empty shadow path of resolveResumeRepoIDAndBranch/applyResumeRepoConfig, +// the exact function this diff rewrote, and every other existing call site sets +// a non-empty session.RepositoryID. +func TestApplyResumeRepoConfig_EmptyAttachmentSetAndEmptyPreferenceLaunchesWithNoRepoConfig(t *testing.T) { + repo := newMockRepository() + repo.tasks["task-1"] = &models.Task{ID: "task-1"} + task := &v1.Task{ID: "task-1"} + session := &models.TaskSession{ID: "sess-1", TaskID: "task-1"} + exec := newTestExecutor(t, &mockAgentManager{}, repo) + + req := &LaunchAgentRequest{TaskID: "task-1", SessionID: "sess-1", ExecutorType: "worktree"} + + gotID, err := exec.applyResumeRepoConfig(context.Background(), task, session, req, nil) + if err != nil { + t.Fatalf("applyResumeRepoConfig: %v", err) + } + if gotID != "" { + t.Fatalf("resolved repositoryID = %q, want empty for a repository-less task", gotID) + } + if req.RepositoryID != "" { + t.Fatalf("req.RepositoryID = %q, want empty", req.RepositoryID) + } + if req.Repositories != nil { + t.Fatalf("req.Repositories = %+v, want nil", req.Repositories) + } +} + +// TestApplyResumeRepoConfig_WorktreeBuiltInDefaultAppliesOnlyOnWorktreeExecutor +// covers AC-...-002.2's boundary, flagged untested by test-supervisor: before +// this diff, an empty-preference resume returned early before +// applyResumeWorktreeConfig could ever run, so its built-in "main" fallback +// never fired for the repaired population. It fires now. The built-in default +// must still apply only to the worktree executor's single-repository fields, +// never to any other executor type. +func TestApplyResumeRepoConfig_WorktreeBuiltInDefaultAppliesOnlyOnWorktreeExecutor(t *testing.T) { + newSetup := func() (*mockRepository, *v1.Task, *models.TaskSession) { + repo := newMockRepository() + repo.repositories["repo-1"] = &models.Repository{ID: "repo-1", LocalPath: "/tmp/repo"} + repo.taskRepositories["tr-1"] = &models.TaskRepository{ + ID: "tr-1", TaskID: "task-1", RepositoryID: "repo-1", Position: 0, + // BaseBranch empty and repository DefaultBranch empty: every rung + // of the branch-precedence chain is empty except the session, + // which also carries none. + } + repo.tasks["task-1"] = &models.Task{ID: "task-1"} + return repo, &v1.Task{ID: "task-1"}, &models.TaskSession{ID: "sess-1", TaskID: "task-1"} + } + + t.Run("worktree gets the built-in main default", func(t *testing.T) { + repo, task, session := newSetup() + exec := newTestExecutor(t, &mockAgentManager{}, repo) + req := &LaunchAgentRequest{TaskID: "task-1", SessionID: "sess-1", ExecutorType: "worktree"} + if _, err := exec.applyResumeRepoConfig(context.Background(), task, session, req, nil); err != nil { + t.Fatalf("applyResumeRepoConfig: %v", err) + } + if req.BaseBranch != "main" { + t.Fatalf("BaseBranch = %q, want main (worktree built-in default)", req.BaseBranch) + } + }) + + t.Run("non-worktree leaves base branch unset", func(t *testing.T) { + repo, task, session := newSetup() + exec := newTestExecutor(t, &mockAgentManager{}, repo) + req := &LaunchAgentRequest{TaskID: "task-1", SessionID: "sess-1", ExecutorType: "ssh"} + if _, err := exec.applyResumeRepoConfig(context.Background(), task, session, req, nil); err != nil { + t.Fatalf("applyResumeRepoConfig: %v", err) + } + if req.BaseBranch != "" { + t.Fatalf("BaseBranch = %q, want empty: the worktree built-in default must not leak to other executor types", req.BaseBranch) + } + }) +} + +// TestApplyResumeRepoConfig_StampsRepositoryIdentityWithEmptyLocalPath covers +// the mandatory empty-local-path regression: a resolved primary can have no +// local clone yet (no cloner configured, no provider identity, ...). The +// identity stamp must not be gated on repositoryPath != "", or exactly that +// population produces no inventory row and is refused by the guard. +func TestApplyResumeRepoConfig_StampsRepositoryIdentityWithEmptyLocalPath(t *testing.T) { + repo := newMockRepository() + repo.repositories["repo-1"] = &models.Repository{ + ID: "repo-1", + // LocalPath intentionally empty and no provider identity set, so + // ensureRepoLocalPathForSessionAndState resolves with no clone. + } + repo.taskRepositories["tr-1"] = &models.TaskRepository{ + ID: "tr-1", TaskID: "task-1", RepositoryID: "repo-1", Position: 0, BaseBranch: "main", + } + repo.tasks["task-1"] = &models.Task{ID: "task-1"} + task := &v1.Task{ID: "task-1"} + session := &models.TaskSession{ID: "sess-1", TaskID: "task-1"} + exec := newTestExecutor(t, &mockAgentManager{}, repo) + + req := &LaunchAgentRequest{TaskID: "task-1", SessionID: "sess-1", ExecutorType: "worktree"} + + if _, err := exec.applyResumeRepoConfig(context.Background(), task, session, req, nil); err != nil { + t.Fatalf("applyResumeRepoConfig: %v", err) + } + if req.RepositoryID != "repo-1" { + t.Fatalf("req.RepositoryID = %q, want repo-1 even though the repository has no local clone yet", req.RepositoryID) + } + if req.RepositoryPath != "" { + t.Fatalf("req.RepositoryPath = %q, want empty", req.RepositoryPath) + } +} + +// TestApplyResumeRepoConfig_BaseBranchPrefersRepositoryDefaultOverSessionValue +// pins AC-...-002.4's worked case: a session base branch of "main", exactly +// one attachment whose raw base_branch is empty, and a repository default +// branch of "develop" must resolve to "develop". "That attachment's base +// branch" in AC-...-002.4 denotes repoInfo.BaseBranch after +// resolveTaskRepoInfoForSession has already defaulted the empty raw column to +// the repository's default branch, not the raw column itself. +func TestApplyResumeRepoConfig_BaseBranchPrefersRepositoryDefaultOverSessionValue(t *testing.T) { + repo := newMockRepository() + repo.repositories["repo-1"] = &models.Repository{ + ID: "repo-1", + LocalPath: "/tmp/repo", + DefaultBranch: "develop", + } + repo.taskRepositories["tr-1"] = &models.TaskRepository{ + ID: "tr-1", TaskID: "task-1", RepositoryID: "repo-1", Position: 0, + // BaseBranch intentionally empty: the raw attachment column. + } + repo.tasks["task-1"] = &models.Task{ID: "task-1"} + task := &v1.Task{ID: "task-1"} + session := &models.TaskSession{ + ID: "sess-1", + TaskID: "task-1", + RepositoryID: "repo-1", + BaseBranch: "main", + } + exec := newTestExecutor(t, &mockAgentManager{}, repo) + + req := &LaunchAgentRequest{TaskID: "task-1", SessionID: "sess-1", ExecutorType: "worktree"} + + if _, err := exec.applyResumeRepoConfig(context.Background(), task, session, req, nil); err != nil { + t.Fatalf("applyResumeRepoConfig: %v", err) + } + if req.BaseBranch != "develop" { + t.Fatalf("BaseBranch = %q, want develop (the repository default outranks the session's stored value)", req.BaseBranch) + } +} + +// TestResumeSession_MultiRepo_PopulatesRequestRepositoriesWhenSessionHasNoPreference +// covers the multi-repo regression the applyResumeMultiRepoConfig comment +// describes: a session created before the task had any attachments must +// still resolve and configure every attachment, not just recover a primary. +func TestResumeSession_MultiRepo_PopulatesRequestRepositoriesWhenSessionHasNoPreference(t *testing.T) { + repo := newMockRepository() + const taskID = "task-multi-resume-no-pref" + const sessionID = "session-multi-resume-no-pref" + seedMultiRepoTask(t, repo, taskID) + seedWorktreeExecutor(repo) + + repo.tasks[taskID] = &models.Task{ID: taskID, WorkspaceID: "ws-1", Title: "Multi Resume No Preference"} + repo.sessions[sessionID] = &models.TaskSession{ + ID: sessionID, + TaskID: taskID, + AgentProfileID: "profile-123", + ExecutorID: models.ExecutorIDWorktree, + // RepositoryID intentionally empty: both attachments were added after + // this session was created. + State: models.TaskSessionStateCancelled, + ErrorMessage: models.SessionArchiveTreeCancelReason, + StartedAt: time.Now(), + UpdatedAt: time.Now(), + } + + var captured *LaunchAgentRequest + agentManager := &mockAgentManager{ + launchAgentFunc: func(_ context.Context, req *LaunchAgentRequest) (*LaunchAgentResponse, error) { + captured = req + return &LaunchAgentResponse{ + AgentExecutionID: "exec-resume-multi-no-pref", + WorktreePath: "/tasks/y", + Worktrees: []RepoWorktreeResult{ + {RepositoryID: "repo-front", WorktreeID: "wt-front", WorktreeBranch: "feat/y-1", WorktreePath: "/tasks/y/frontend"}, + {RepositoryID: "repo-back", WorktreeID: "wt-back", WorktreeBranch: "feat/y-2", WorktreePath: "/tasks/y/backend"}, + }, + }, nil + }, + } + exec := newTestExecutor(t, agentManager, repo) + + if _, err := exec.ResumeSession(context.Background(), repo.sessions[sessionID], false); err != nil { + t.Fatalf("ResumeSession: %v", err) + } + + if captured == nil { + t.Fatal("expected launch agent to be called") + } + if captured.RepositoryID != "repo-front" { + t.Fatalf("resolved primary repositoryID = %q, want repo-front (position 0)", captured.RepositoryID) + } + if len(captured.Repositories) != 2 { + t.Fatalf("expected req.Repositories length 2, got %d: %+v", len(captured.Repositories), captured.Repositories) + } + if captured.Repositories[0].RepositoryID != "repo-front" || captured.Repositories[1].RepositoryID != "repo-back" { + t.Errorf("unexpected repo order: %+v", captured.Repositories) + } +} + +// TestApplyResumeRepoConfig_FailsClosedWhenAttachmentSetReadFails covers +// AC-...-001.6: a failed read of the attachment set must fail the launch and +// report the read failure, not proceed as though the task had no +// attachments. +func TestApplyResumeRepoConfig_FailsClosedWhenAttachmentSetReadFails(t *testing.T) { + repo := newMockRepository() + repo.tasks["task-1"] = &models.Task{ID: "task-1"} + wantErr := errors.New("transient attachment set read failure") + repo.listTaskRepositoriesFunc = func(ctx context.Context, taskID string) ([]*models.TaskRepository, error) { + return nil, wantErr + } + task := &v1.Task{ID: "task-1"} + session := &models.TaskSession{ID: "sess-1", TaskID: "task-1"} + exec := newTestExecutor(t, &mockAgentManager{}, repo) + + req := &LaunchAgentRequest{TaskID: "task-1", SessionID: "sess-1", ExecutorType: "worktree"} + + _, err := exec.applyResumeRepoConfig(context.Background(), task, session, req, nil) + if !errors.Is(err, wantErr) { + t.Fatalf("applyResumeRepoConfig error = %v, want %v", err, wantErr) + } + if req.RepositoryID != "" { + t.Fatalf("req.RepositoryID = %q, want empty: a failed read must not proceed as though the task had no attachments", req.RepositoryID) + } +} + +// TestApplyResumeRepoConfig_LocalDockerFallsBackToRepositoryPathOnResume is a +// PR-fixup regression (greptile finding): applyResumeCloneURL, unlike the +// initial-launch path (applyRepositoryConfig, executor_execute.go), never fell +// back to dockerLocalCloneSource when the repository has no provider identity +// or remote origin. A workspace-source repository with only a local checkout +// could launch initially but never resume on local_docker, failing every +// resume with ErrNoCloneURL. +func TestApplyResumeRepoConfig_LocalDockerFallsBackToRepositoryPathOnResume(t *testing.T) { + repo := newMockRepository() + source := t.TempDir() + repo.repositories["repo-1"] = &models.Repository{ID: "repo-1", Name: "repo-1", LocalPath: source} + repo.taskRepositories["tr-1"] = &models.TaskRepository{ + ID: "tr-1", TaskID: "task-1", RepositoryID: "repo-1", Position: 0, + } + repo.tasks["task-1"] = &models.Task{ID: "task-1"} + task := &v1.Task{ID: "task-1"} + session := &models.TaskSession{ID: "sess-1", TaskID: "task-1", RepositoryID: "repo-1"} + exec := newTestExecutor(t, &mockAgentManager{}, repo) + + req := &LaunchAgentRequest{TaskID: "task-1", SessionID: "sess-1", ExecutorType: string(models.ExecutorTypeLocalDocker)} + + if _, err := exec.applyResumeRepoConfig(context.Background(), task, session, req, nil); err != nil { + t.Fatalf("applyResumeRepoConfig: %v, want nil: a local_docker resume of a repository with only a local checkout must fall back to the repository path", err) + } + if req.RepositoryURL != source { + t.Fatalf("req.RepositoryURL = %q, want %q (the repository path used as the Docker bind-mount clone source)", req.RepositoryURL, source) + } +} + +// TestApplyResumeRepoConfig_LocalDockerRejectsMissingRepositoryPathOnResume +// pins the failure case: when neither a provider/remote clone URL nor a usable +// local repository path exists, resume must still refuse with ErrNoCloneURL +// rather than silently launching with an empty clone source. +func TestApplyResumeRepoConfig_LocalDockerRejectsMissingRepositoryPathOnResume(t *testing.T) { + repo := newMockRepository() + repo.repositories["repo-1"] = &models.Repository{ID: "repo-1", Name: "repo-1"} + repo.taskRepositories["tr-1"] = &models.TaskRepository{ + ID: "tr-1", TaskID: "task-1", RepositoryID: "repo-1", Position: 0, + } + repo.tasks["task-1"] = &models.Task{ID: "task-1"} + task := &v1.Task{ID: "task-1"} + session := &models.TaskSession{ID: "sess-1", TaskID: "task-1", RepositoryID: "repo-1"} + exec := newTestExecutor(t, &mockAgentManager{}, repo) + + req := &LaunchAgentRequest{TaskID: "task-1", SessionID: "sess-1", ExecutorType: string(models.ExecutorTypeLocalDocker)} + + _, err := exec.applyResumeRepoConfig(context.Background(), task, session, req, nil) + if !errors.Is(err, ErrNoCloneURL) { + t.Fatalf("applyResumeRepoConfig error = %v, want ErrNoCloneURL", err) + } +} diff --git a/apps/backend/internal/task/repository/sqlite/task_environment.go b/apps/backend/internal/task/repository/sqlite/task_environment.go index 4a48017d9c..5736845bd8 100644 --- a/apps/backend/internal/task/repository/sqlite/task_environment.go +++ b/apps/backend/internal/task/repository/sqlite/task_environment.go @@ -521,10 +521,13 @@ func (r *Repository) reconcileTaskEnvironmentReposTx( existing, repos []*models.TaskEnvironmentRepo, replacePhysical bool, ) error { - byKey, legacyFlatByRepo := indexTaskEnvironmentReposTx(existing) + byKey, legacyFlatByRepo, soleByRepo := indexTaskEnvironmentReposTx(existing) matched := make(map[string]struct{}, len(repos)) for position, incoming := range repos { row := findTaskEnvironmentRepoTransition(byKey, legacyFlatByRepo, incoming) + if row == nil && !replacePhysical { + row = takeSoleUntrackedTaskEnvironmentRepo(byKey, soleByRepo, incoming) + } if row == nil { if incoming == nil || incoming.RepositoryID == "" { continue @@ -547,19 +550,40 @@ func (r *Repository) reconcileTaskEnvironmentReposTx( return r.tombstoneOmittedTaskEnvironmentReposTx(ctx, tx, existing, matched) } -func indexTaskEnvironmentReposTx(rows []*models.TaskEnvironmentRepo) (map[string]*models.TaskEnvironmentRepo, map[string]*models.TaskEnvironmentRepo) { +func indexTaskEnvironmentReposTx(rows []*models.TaskEnvironmentRepo) ( + map[string]*models.TaskEnvironmentRepo, + map[string]*models.TaskEnvironmentRepo, + map[string]*models.TaskEnvironmentRepo, +) { byKey := make(map[string]*models.TaskEnvironmentRepo, len(rows)) legacyFlatByRepo := make(map[string]*models.TaskEnvironmentRepo) + soleByRepo := make(map[string]*models.TaskEnvironmentRepo) + ambiguousRepo := make(map[string]struct{}) for _, row := range rows { if row == nil { continue } byKey[row.RepositoryID+"\x00"+row.BranchSlug] = row - if row.RepositoryID != "" && row.BranchSlug == "" { + if row.RepositoryID == "" { + continue + } + if row.BranchSlug == "" { legacyFlatByRepo[row.RepositoryID] = row } + if row.DeletedAt != nil || row.Status == "failed" || row.Status == worktreeRepoStatusDeleted { + continue + } + if _, ambiguous := ambiguousRepo[row.RepositoryID]; ambiguous { + continue + } + if _, seen := soleByRepo[row.RepositoryID]; seen { + delete(soleByRepo, row.RepositoryID) + ambiguousRepo[row.RepositoryID] = struct{}{} + continue + } + soleByRepo[row.RepositoryID] = row } - return byKey, legacyFlatByRepo + return byKey, legacyFlatByRepo, soleByRepo } func findTaskEnvironmentRepoTransition( @@ -582,6 +606,26 @@ func findTaskEnvironmentRepoTransition( return row } +// takeSoleUntrackedTaskEnvironmentRepo reuses the only active inventory row +// for a repository when a non-replacement transition has no branch identity. +// Local resumes intentionally do not select or switch branches, so the +// existing canonical row is the only safe branch identity to retain. +func takeSoleUntrackedTaskEnvironmentRepo( + byKey, soleByRepo map[string]*models.TaskEnvironmentRepo, + incoming *models.TaskEnvironmentRepo, +) *models.TaskEnvironmentRepo { + if incoming == nil || incoming.RepositoryID == "" || incoming.BranchSlug != "" { + return nil + } + row := soleByRepo[incoming.RepositoryID] + if row != nil { + delete(soleByRepo, incoming.RepositoryID) + delete(byKey, row.RepositoryID+"\x00"+row.BranchSlug) + byKey[incoming.RepositoryID+"\x00"] = row + } + return row +} + func (r *Repository) updateTaskEnvironmentRepoTransitionTx( ctx context.Context, tx *sqlx.Tx, @@ -589,7 +633,12 @@ func (r *Repository) updateTaskEnvironmentRepoTransitionTx( position int, replacePhysical bool, ) error { - row.BranchSlug = incoming.BranchSlug + // An incoming transition with untracked branch identity (a non-worktree + // local/local_pc resume never stamps req.BaseBranch) must not overwrite + // a row's already-known real branch with an empty one; preserve it. + if replacePhysical || incoming.BranchSlug != "" || row.BranchSlug == "" { + row.BranchSlug = incoming.BranchSlug + } if replacePhysical || incoming.WorktreeID != "" { row.WorktreeID = incoming.WorktreeID row.WorktreePath = incoming.WorktreePath 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 8288372ea4..16dd3cb99c 100644 --- a/apps/backend/internal/task/repository/sqlite/task_environment_test.go +++ b/apps/backend/internal/task/repository/sqlite/task_environment_test.go @@ -471,6 +471,66 @@ func TestPersistTaskEnvironmentTransitionReconcilesInventoryAtomically(t *testin } } +// TestPersistTaskEnvironmentTransitionUntrackedBranchResumeUpdatesSoleRowInPlace +// covers a non-worktree (local/local_pc) resume, which never stamps a base +// branch and so always transitions with an incoming empty BranchSlug. It must +// update the repository's sole existing canonical row in place rather than +// insert a duplicate empty-branch row. The test uses a repo-backed task and +// performs two transitions to model successive resumes. +func TestPersistTaskEnvironmentTransitionUntrackedBranchResumeUpdatesSoleRowInPlace(t *testing.T) { + repo := newRepoForEntityTests(t) + ctx := context.Background() + workspaceID := "workspace-untracked-resume" + taskID := "task-untracked-resume" + repositoryID := "repo-untracked-resume" + seedWorkspace(t, repo, workspaceID) + if err := repo.CreateTask(ctx, &models.Task{ID: taskID, WorkspaceID: workspaceID, Title: "untracked-resume"}); err != nil { + t.Fatalf("CreateTask: %v", err) + } + if err := repo.CreateRepository(ctx, &models.Repository{ID: repositoryID, WorkspaceID: workspaceID, Name: "repository"}); err != nil { + t.Fatalf("CreateRepository: %v", err) + } + if err := repo.CreateTaskRepository(ctx, &models.TaskRepository{TaskID: taskID, RepositoryID: repositoryID, BaseBranch: "main"}); err != nil { + t.Fatalf("CreateTaskRepository: %v", err) + } + env := &models.TaskEnvironment{ + ID: "env-untracked-resume", + TaskID: taskID, + ExecutorType: string(models.ExecutorTypeLocal), + Status: models.TaskEnvironmentStatusReady, + WorkspacePath: "/workspace/untracked-resume", + Repos: []*models.TaskEnvironmentRepo{{ + ID: "canonical", + RepositoryID: repositoryID, + BranchSlug: "main", + Position: 0, + }}, + } + if err := repo.CreateTaskEnvironment(ctx, env); err != nil { + t.Fatalf("CreateTaskEnvironment: %v", err) + } + + for resume := 0; resume < 2; resume++ { + if err := repo.PersistTaskEnvironmentTransition(ctx, env, []*models.TaskEnvironmentRepo{{ + RepositoryID: repositoryID, + Position: 0, + }}, false); err != nil { + t.Fatalf("PersistTaskEnvironmentTransition resume %d: %v", resume+1, err) + } + persisted, err := repo.GetTaskEnvironment(ctx, env.ID) + if err != nil { + t.Fatalf("GetTaskEnvironment resume %d: %v", resume+1, err) + } + if len(persisted.Repos) != 1 { + t.Fatalf("resume %d repos = %#v, want a single canonical row, not a duplicate", resume+1, persisted.Repos) + } + row := persisted.Repos[0] + if row.ID != "canonical" || row.RepositoryID != repositoryID || row.BranchSlug != "main" || row.Status != worktreeRepoStatusActive || row.DeletedAt != nil { + t.Fatalf("resume %d row = %#v, want the active canonical row with branch preserved", resume+1, row) + } + } +} + func TestCreateTaskSessionWithWorkspaceBindingElectsAndAttaches(t *testing.T) { repo := newRepoForEntityTests(t) ctx := context.Background() diff --git a/apps/backend/internal/task/repository/sqlite/task_repository.go b/apps/backend/internal/task/repository/sqlite/task_repository.go index d27007f9b0..7ca97f4a38 100644 --- a/apps/backend/internal/task/repository/sqlite/task_repository.go +++ b/apps/backend/internal/task/repository/sqlite/task_repository.go @@ -88,7 +88,7 @@ func (r *Repository) ListTaskRepositories(ctx context.Context, taskID string) ([ position, metadata, created_at, updated_at FROM task_repositories WHERE task_id = ? - ORDER BY position ASC, created_at ASC + ORDER BY position ASC, created_at ASC, id ASC `), taskID) if err != nil { return nil, err @@ -205,7 +205,7 @@ func (r *Repository) ListTaskRepositoriesByTaskIDs(ctx context.Context, taskIDs position, metadata, created_at, updated_at FROM task_repositories WHERE task_id IN (%s) - ORDER BY position ASC, created_at ASC + ORDER BY position ASC, created_at ASC, id ASC `, strings.Join(placeholders, ",")) rows, err := r.ro.QueryContext(ctx, r.ro.Rebind(query), args...) diff --git a/apps/backend/internal/task/repository/sqlite/task_repository_ordering_postgres_test.go b/apps/backend/internal/task/repository/sqlite/task_repository_ordering_postgres_test.go new file mode 100644 index 0000000000..0c87d96aa3 --- /dev/null +++ b/apps/backend/internal/task/repository/sqlite/task_repository_ordering_postgres_test.go @@ -0,0 +1,90 @@ +package sqlite + +import ( + "context" + "testing" + "time" + + "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/testutil" +) + +// TestPostgresListTaskRepositoriesOrdersByIDWhenPositionAndCreatedAtTie is the +// Postgres counterpart to TestListTaskRepositoriesOrdersByIDWhenPositionAndCreatedAtTie. +// ListTaskRepositories' ORDER BY gained an `id ASC` tiebreak (AC-...-001.2); this +// pins that the tiebreak really does run on Postgres, per the AGENTS.md +// requirement that every changed dialect-sensitive method in this package get an +// environment-gated PostgreSQL behavior test. Skips unless KANDEV_TEST_POSTGRES_DSN +// is set. +func TestPostgresListTaskRepositoriesOrdersByIDWhenPositionAndCreatedAtTie(t *testing.T) { + db := testutil.OpenIsolatedPostgres(t, testutil.PostgresDSNFromEnv(t)) + repo, err := NewWithDB(db, db, nil) + if err != nil { + t.Fatalf("init postgres schema: %v", err) + } + ctx := context.Background() + + if err := repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-pg-ordering-tiebreak", Name: "ws-pg-ordering-tiebreak"}); err != nil { + t.Fatalf("CreateWorkspace: %v", err) + } + if err := repo.CreateWorkflow(ctx, &models.Workflow{ + ID: "wf-pg-ordering-tiebreak", WorkspaceID: "ws-pg-ordering-tiebreak", Name: "Workflow", + }); err != nil { + t.Fatalf("CreateWorkflow: %v", err) + } + if err := repo.CreateTask(ctx, &models.Task{ + ID: "task-pg-ordering-tiebreak", WorkspaceID: "ws-pg-ordering-tiebreak", + WorkflowID: "wf-pg-ordering-tiebreak", Title: "Task", + }); err != nil { + t.Fatalf("CreateTask: %v", err) + } + for _, repository := range []*models.Repository{ + {ID: "repo-pg-ordering-a", WorkspaceID: "ws-pg-ordering-tiebreak", Name: "a"}, + {ID: "repo-pg-ordering-b", WorkspaceID: "ws-pg-ordering-tiebreak", Name: "b"}, + } { + if err := repo.CreateRepository(ctx, repository); err != nil { + t.Fatalf("CreateRepository %s: %v", repository.ID, err) + } + } + + // Both rows share position and created_at, so only the id ASC tiebreak can + // order them. task-repository-b is inserted second but must still sort + // first: it is lexicographically smaller. + tiedAt := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + for _, row := range []struct{ id, repositoryID string }{ + {"task-repository-pg-c", "repo-pg-ordering-a"}, + {"task-repository-pg-b", "repo-pg-ordering-b"}, + } { + if _, err := repo.db.Exec(repo.db.Rebind(` + INSERT INTO task_repositories (id, task_id, repository_id, position, created_at, updated_at) + VALUES (?, ?, ?, 0, ?, ?) + `), row.id, "task-pg-ordering-tiebreak", row.repositoryID, tiedAt, tiedAt); err != nil { + t.Fatalf("insert tied task repository %s: %v", row.id, err) + } + } + + got, err := repo.ListTaskRepositories(ctx, "task-pg-ordering-tiebreak") + if err != nil { + t.Fatalf("ListTaskRepositories: %v", err) + } + if len(got) != 2 || got[0].ID != "task-repository-pg-b" || got[1].ID != "task-repository-pg-c" { + t.Fatalf("expected id-ascending tiebreak [task-repository-pg-b task-repository-pg-c], got %+v", got) + } + + primary, err := repo.GetPrimaryTaskRepository(ctx, "task-pg-ordering-tiebreak") + if err != nil { + t.Fatalf("GetPrimaryTaskRepository: %v", err) + } + if primary == nil || primary.ID != "task-repository-pg-b" { + t.Fatalf("GetPrimaryTaskRepository = %+v, want task-repository-pg-b (the same row ListTaskRepositories[0] returns)", primary) + } + + batch, err := repo.ListTaskRepositoriesByTaskIDs(ctx, []string{"task-pg-ordering-tiebreak"}) + if err != nil { + t.Fatalf("ListTaskRepositoriesByTaskIDs: %v", err) + } + batchRows := batch["task-pg-ordering-tiebreak"] + if len(batchRows) != 2 || batchRows[0].ID != "task-repository-pg-b" || batchRows[1].ID != "task-repository-pg-c" { + t.Fatalf("ListTaskRepositoriesByTaskIDs expected id-ascending tiebreak [task-repository-pg-b task-repository-pg-c], got %+v", batchRows) + } +} diff --git a/apps/backend/internal/task/repository/sqlite/task_repository_ordering_test.go b/apps/backend/internal/task/repository/sqlite/task_repository_ordering_test.go new file mode 100644 index 0000000000..3b0b81b759 --- /dev/null +++ b/apps/backend/internal/task/repository/sqlite/task_repository_ordering_test.go @@ -0,0 +1,123 @@ +package sqlite + +import ( + "context" + "testing" + "time" + + "github.com/kandev/kandev/internal/task/models" +) + +// TestListTaskRepositoriesOrdersByIDWhenPositionAndCreatedAtTie pins +// AC-TASKS-LAUNCH-REPOSITORY-RESOLUTION-001.2: position and created_at alone +// do not total-order the attachment set, so two rows sharing both must still +// resolve to one defined primary. GetPrimaryTaskRepository must agree with +// ListTaskRepositories, since it is defined as that read's first row. +func TestListTaskRepositoriesOrdersByIDWhenPositionAndCreatedAtTie(t *testing.T) { + repo := newRepoForEntityTests(t) + ctx := context.Background() + seedWorkspace(t, repo, "ws-ordering-tiebreak") + if err := repo.CreateWorkflow(ctx, &models.Workflow{ + ID: "wf-ordering-tiebreak", WorkspaceID: "ws-ordering-tiebreak", Name: "Workflow", + }); err != nil { + t.Fatal(err) + } + if err := repo.CreateTask(ctx, &models.Task{ + ID: "task-ordering-tiebreak", WorkspaceID: "ws-ordering-tiebreak", + WorkflowID: "wf-ordering-tiebreak", Title: "Task", + }); err != nil { + t.Fatal(err) + } + for _, repository := range []*models.Repository{ + {ID: "repo-ordering-a", WorkspaceID: "ws-ordering-tiebreak", Name: "a"}, + {ID: "repo-ordering-b", WorkspaceID: "ws-ordering-tiebreak", Name: "b"}, + } { + if err := repo.CreateRepository(ctx, repository); err != nil { + t.Fatal(err) + } + } + + // Both rows share position and created_at, so only the id ASC tiebreak + // can order them. task-repository-b is inserted second but must still + // sort first: it is lexicographically smaller. + tiedAt := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + for _, row := range []struct{ id, repositoryID string }{ + {"task-repository-c", "repo-ordering-a"}, + {"task-repository-b", "repo-ordering-b"}, + } { + if _, err := repo.db.Exec(repo.db.Rebind(` + INSERT INTO task_repositories (id, task_id, repository_id, position, created_at, updated_at) + VALUES (?, ?, ?, 0, ?, ?) + `), row.id, "task-ordering-tiebreak", row.repositoryID, tiedAt, tiedAt); err != nil { + t.Fatalf("insert tied task repository %s: %v", row.id, err) + } + } + + got, err := repo.ListTaskRepositories(ctx, "task-ordering-tiebreak") + if err != nil { + t.Fatalf("ListTaskRepositories: %v", err) + } + if len(got) != 2 || got[0].ID != "task-repository-b" || got[1].ID != "task-repository-c" { + t.Fatalf("expected id-ascending tiebreak [task-repository-b task-repository-c], got %+v", got) + } + + primary, err := repo.GetPrimaryTaskRepository(ctx, "task-ordering-tiebreak") + if err != nil { + t.Fatalf("GetPrimaryTaskRepository: %v", err) + } + if primary == nil || primary.ID != "task-repository-b" { + t.Fatalf("GetPrimaryTaskRepository = %+v, want task-repository-b (the same row ListTaskRepositories[0] returns)", primary) + } +} + +// TestListTaskRepositoriesByTaskIDsOrdersByIDWhenPositionAndCreatedAtTie is the +// batch counterpart to TestListTaskRepositoriesOrdersByIDWhenPositionAndCreatedAtTie: +// ListTaskRepositoriesByTaskIDs has its own ORDER BY and must resolve the same +// tie the same way, or a caller that switches between the single-task and +// batch reads would see a different primary for the identical attachment set. +func TestListTaskRepositoriesByTaskIDsOrdersByIDWhenPositionAndCreatedAtTie(t *testing.T) { + repo := newRepoForEntityTests(t) + ctx := context.Background() + seedWorkspace(t, repo, "ws-batch-ordering-tiebreak") + if err := repo.CreateWorkflow(ctx, &models.Workflow{ + ID: "wf-batch-ordering-tiebreak", WorkspaceID: "ws-batch-ordering-tiebreak", Name: "Workflow", + }); err != nil { + t.Fatal(err) + } + if err := repo.CreateTask(ctx, &models.Task{ + ID: "task-batch-ordering-tiebreak", WorkspaceID: "ws-batch-ordering-tiebreak", + WorkflowID: "wf-batch-ordering-tiebreak", Title: "Task", + }); err != nil { + t.Fatal(err) + } + for _, repository := range []*models.Repository{ + {ID: "repo-batch-ordering-a", WorkspaceID: "ws-batch-ordering-tiebreak", Name: "a"}, + {ID: "repo-batch-ordering-b", WorkspaceID: "ws-batch-ordering-tiebreak", Name: "b"}, + } { + if err := repo.CreateRepository(ctx, repository); err != nil { + t.Fatal(err) + } + } + + tiedAt := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + for _, row := range []struct{ id, repositoryID string }{ + {"task-repository-batch-c", "repo-batch-ordering-a"}, + {"task-repository-batch-b", "repo-batch-ordering-b"}, + } { + if _, err := repo.db.Exec(repo.db.Rebind(` + INSERT INTO task_repositories (id, task_id, repository_id, position, created_at, updated_at) + VALUES (?, ?, ?, 0, ?, ?) + `), row.id, "task-batch-ordering-tiebreak", row.repositoryID, tiedAt, tiedAt); err != nil { + t.Fatalf("insert tied task repository %s: %v", row.id, err) + } + } + + got, err := repo.ListTaskRepositoriesByTaskIDs(ctx, []string{"task-batch-ordering-tiebreak"}) + if err != nil { + t.Fatalf("ListTaskRepositoriesByTaskIDs: %v", err) + } + rows := got["task-batch-ordering-tiebreak"] + if len(rows) != 2 || rows[0].ID != "task-repository-batch-b" || rows[1].ID != "task-repository-batch-c" { + t.Fatalf("expected id-ascending tiebreak [task-repository-batch-b task-repository-batch-c], got %+v", rows) + } +} diff --git a/docs/specs/tasks/README.md b/docs/specs/tasks/README.md index d739e6878b..d784941915 100644 --- a/docs/specs/tasks/README.md +++ b/docs/specs/tasks/README.md @@ -105,6 +105,7 @@ signals, and task-scoped scheduling contracts. - [Task-create dependency selector refinement](requirements/task-dependencies-create-dialog-dependency-selector.md) - [Edit task dependencies](requirements/task-dependency-detail-editing.md) - [Task Dependencies and Auto-Start Chains](requirements/task-dependencies.md) +- [Session Launch Repository Resolution](requirements/launch-repository-resolution.md) - [Task Launch Failure Recovery](requirements/task-launch-failure-recovery.md) - [Worktree metadata recovery](requirements/worktree-metadata-recovery.md) - [Task priority visibility](requirements/task-priority-visibility.md) @@ -177,6 +178,7 @@ signals, and task-scoped scheduling contracts. - [Session Delete Preserves Task Workspaces](system-design/session-delete-resource-cleanup.md) - [Task Dependencies and Auto-Start Chains](system-design/task-dependencies.md) - [Edit task dependencies](system-design/task-dependency-detail-editing.md) +- [Session Launch Repository Resolution](system-design/launch-repository-resolution.md) - [Task Launch Failure Recovery](system-design/task-launch-failure-recovery.md) - [Worktree metadata recovery](system-design/worktree-metadata-recovery.md) - [Task Create Launch Preview](system-design/task-create-launch-preview.md) diff --git a/docs/specs/tasks/requirements/launch-repository-resolution.md b/docs/specs/tasks/requirements/launch-repository-resolution.md new file mode 100644 index 0000000000..107a4ee04a --- /dev/null +++ b/docs/specs/tasks/requirements/launch-repository-resolution.md @@ -0,0 +1,325 @@ +--- +status: draft +system: tasks +created: 2026-09-09 +owners: + - kandev +--- +# Session Launch Repository Resolution Requirements + +## Overview + +A task session launch must build its launch request from the repositories the +task is attached to. Today it builds them from a snapshot on the session row, +while the persistence guard admitting the resulting environment reads the +`task_repositories` table. The two disagree whenever a task acquires its first +attachment after one of its sessions was already created with an empty +preference (the zero-to-one transition; see `## Prior art` and AC-001.3), and +every later launch of that session fails with: + +```text +create task environment: create task environment: ready status requires +repository inventory for repo-backed task (task=) +``` + +It reaches every entry point that resumes an existing session — workflow step +auto-start, a Backlog bounce, and `session.recover` with +`action: "fresh_start"` — and there is no supported recovery, because every +recovery route is itself a resume. Measured on one instance on 2026-09-09: eight +live sessions holding 6,124 conversation messages. + +This system owns the contract because launch behavior is a task-system +responsibility. The [workspace system](../../workspaces) owns repositories and +worktrees; this requirement changes only which repository set a launch is built +from. + +## Terminology + +- **Attachment set:** the task's rows in `task_repositories`, the durable record + of which repositories a task is bound to and on which base branch. This is the + set [Attach Workspace Sources](attach-workspace-sources.md) writes, and the set + the environment inventory guard reads. +- **Session repository preference:** `task_sessions.repository_id` and + `task_sessions.base_branch`, stamped from the task's primary attachment at + session creation and never rewritten. A preference, not a record of what the + task is attached to. +- **Environment repository inventory:** the `task_environment_repos` rows + describing the repository slots a materialized task environment contains. A + ready environment with an empty inventory for a task that has attachments is + rejected at the persistence boundary. +- **Resume launch:** any launch of an already-created session, whatever + triggered it. All build their request through the same resume path. + +## Prior art + +**Our own prior reasoning (wiki).** Receipt: config resolved through +`~/.obsidian-wiki/config` -> `config.henry`, giving +`OBSIDIAN_VAULT_PATH=/Users/henry/Documents/henry/wiki` and +`QMD_WIKI_COLLECTION="wiki"`. The leg could not then run: no QMD transport, no +`qmd` or `obsidian-wiki` CLI, and the vault unreadable from this worktree +(`EPERM`, macOS privacy on `~/Documents`), which rules out the grep fallback too. +It returned nothing useful — through tool unavailability, not an empty vault, so +this is no evidence the wiki lacks a position. + +**What other products shipped (saas-kb).** Receipt: the `saas-kb` MCP server and +its `search_fsm_docs` tool are absent from this session's tool list and cannot be +loaded on demand. No queries ran; it returned nothing useful. + +**Prior art inside this repository, which was readable and is the source of +the decisions below.** [Tasks Without Repositories](without-repositories.md) +created repository-less tasks and explicitly excluded "attaching a repo to a +repo-less task after creation". That exclusion has never been lifted, and this +matters for how the defect is reproduced. + +[Attach Workspace Sources](attach-workspace-sources.md) is the closest prior +contract: its `AC-TASKS-ATTACH-WORKSPACE-SOURCES-001.5` promises attachment +"shall preserve task state, plans, conversations, sessions, and existing +attachments" — the promise this defect breaks one step later, when the preserved +sessions turn out not to be launchable. But that operation is **not** the path +that produces the defect: `Service.AttachWorkspaceSources` refuses a task with no +attachments yet (`task must have a repository before attaching workspace +sources`), so it cannot perform the zero-to-one transition at all. The only +unconditional writer is the generic task update, `UpdateTask` delegating to +`replaceTaskRepositories`, which recreates the whole set with no precondition on +the current count. + +**This is the defect's only reachable precondition.** The session preference is +stamped once, at session creation, from the task's primary attachment, so it is +empty if and only if the task had no attachments then. The failure therefore +requires a task that was repository-less when its session was created and +acquired its first attachment afterwards — zero-to-one, through the task update +path. Any reproduction, regression test or end-to-end scenario must drive that +path; one built on `AttachWorkspaceSources` is rejected before it can set up the +state. + +The same divergence was found and fixed once already in the neighbouring +`applyResumeMultiRepoConfig`, whose comment records that gating on the task +record's in-memory repository collection "silently dropped every repo but the +primary on any resume of a multi-repo task". The fix there was to read the +database-backed set; this requirement applies it to the primary, left behind. + +## Requirements + +### REQ-TASKS-LAUNCH-REPOSITORY-RESOLUTION-001: Launches resolve repositories from the task attachment set + +**Intent:** A session launch and the guard admitting its environment must read +the same record of which repositories the task has, so attaching a repository to +an existing task makes it launchable rather than permanently unlaunchable. + +**User story:** As a user who attached a repository to a task already started, +I want it to launch and keep its conversation, so repairing a repository-less +task does not cost me the work already in it. + +#### Acceptance criteria + +- **AC-TASKS-LAUNCH-REPOSITORY-RESOLUTION-001.1:** When a launch builds its + request for a session that carries no session repository preference, the + system shall resolve the primary repository from the task's attachment set, + and shall select the same attachment that the task's primary-attachment read + returns. +- **AC-TASKS-LAUNCH-REPOSITORY-RESOLUTION-001.2:** When the attachment set is + ordered to select a primary, the system shall order it by `position` + ascending, then `created_at` ascending, then `task_repositories.id` + ascending, so that the selection is total and returns the same attachment on + every read of unchanged data. +- **AC-TASKS-LAUNCH-REPOSITORY-RESOLUTION-001.3:** When a session carries a + non-empty session repository preference naming a repository that is present in + the task's attachment set, the system shall use that repository as the primary + unchanged, and shall not re-derive the primary from the attachment set's + ordering. A preference naming a repository absent from the attachment set is + excluded below and is left exactly as it behaves today. +- **AC-TASKS-LAUNCH-REPOSITORY-RESOLUTION-001.4:** When a task with a non-empty + attachment set launches a session that materializes or updates its own task's + environment, and that session carries either no repository preference or a + preference naming a repository present in the set, the system shall configure + the launch with every attachment in that set, and shall record one environment + repository inventory row per attachment, whether the set holds one attachment + or many. +- **AC-TASKS-LAUNCH-REPOSITORY-RESOLUTION-001.5:** When a task has an empty + attachment set and its session carries no repository preference, the system + shall launch with no repository configuration and no environment repository + inventory rows, and shall not report a missing inventory. A task with an empty + attachment set whose session still carries a preference is excluded below. +- **AC-TASKS-LAUNCH-REPOSITORY-RESOLUTION-001.6:** When the read that resolves a + launch's repositories from the task's attachment set fails, the system shall + fail that launch and report the read failure, and shall not proceed as though + the task had no attachments. +- **AC-TASKS-LAUNCH-REPOSITORY-RESOLUTION-001.7:** When a task session was + created before a repository was attached to its task, the system shall launch + that session successfully on the next launch whose attachment-set read follows + that attachment, with its conversation, plan, and session identity intact, + without requiring a new session, a database edit, or a data migration. +- **AC-TASKS-LAUNCH-REPOSITORY-RESOLUTION-001.8:** When a repository is + attached to a task that already has sessions, the system shall leave every + existing session's stored repository preference and base branch unchanged. +- **AC-TASKS-LAUNCH-REPOSITORY-RESOLUTION-001.9:** When a session recovery + action clears the resume token and relaunches the session, the system shall + resolve repositories by the same rules as any other launch of that session. +- **AC-TASKS-LAUNCH-REPOSITORY-RESOLUTION-001.10:** When the same session is + launched repeatedly, the task's attachment set and the session's preference are + unchanged between attempts, and an attempt reaches and completes repository + resolution rather than being rejected or failing before it completes, including + the rejections and the re-read failure under + `AC-TASKS-LAUNCH-REPOSITORY-RESOLUTION-001.12`, the system shall resolve the + same primary repository, the same attachment set, and the same environment + repository inventory on every such attempt. An attempt rejected before it + reaches resolution resolves nothing and is outside this criterion. +- **AC-TASKS-LAUNCH-REPOSITORY-RESOLUTION-001.11:** When a task's attachment set + is empty at the moment a launch reads it and non-empty at the moment that + launch writes its environment, the system shall fail the launch, shall not as a + result of that launch bring any environment to ready and shall write no + repository inventory rows, and shall allow an immediate subsequent launch to + succeed against the new set. This criterion constrains only what the failed + launch itself creates or transitions. A ready environment that already existed + before the launch is left as it is, and is not required to be deleted or + invalidated: a task that was repository-less when its session started may + legitimately own one holding an empty inventory from that period, and such an + environment does not prevent the subsequent launch from succeeding. + Attachment-set changes that do not cross empty to non-empty are excluded below. +- **AC-TASKS-LAUNCH-REPOSITORY-RESOLUTION-001.12:** When two launches of the + same session run concurrently, the system shall serialize them on the + per-session lock rather than rejecting the second on arrival: the second launch + shall block until the first releases the lock, and shall re-read the session's + stored state inside the lock before deciding whether to proceed, without having + changed the environment's repository inventory before that re-read. A re-read + that fails, which is also how a session that no longer exists is reported, + shall fail the launch and report that failure rather than proceed on the state + read before the lock, resolving no repositories; + `AC-TASKS-LAUNCH-REPOSITORY-RESOLUTION-001.10` excludes it. A re-read that + succeeds has exactly three outcomes. The system shall report that an execution + is already running when it shows a non-terminal session with an execution + registered for it. It shall report the session state as superseded when it + shows a terminal state different from the one that launch requested. + When neither holds, and in particular when the first launch failed and restored + the session to the same state this launch requested, the system shall reject + nothing: the second launch shall proceed and resolve repositories by the rules + above, because no execution is then running and the second launch is a + legitimate retry rather than a duplicate. + +### REQ-TASKS-LAUNCH-REPOSITORY-RESOLUTION-002: Base branch follows the attachment that supplied the repository + +**Intent:** A primary repository resolved from the attachment set must carry +that attachment's branch, so a repaired task checks out the branch the user +configured rather than a global default. + +**User story:** As a user whose attached repository uses a non-default base +branch, I want the launch to use it, so the repair does not silently start my +work from the wrong base. + +#### Acceptance criteria + +- **AC-TASKS-LAUNCH-REPOSITORY-RESOLUTION-002.1:** When the primary repository + is resolved from the attachment set, the session carries no base branch, and + the task holds exactly one attachment for that repository, the system shall use + that attachment's base branch. When the task holds more than one attachment for + that repository the match is ambiguous, and the system shall use the session's + stored base branch instead — which, for a session carrying none, leaves the + primary's base branch unset — because the session preference predates exact row + identity and cannot name which attachment row it meant. Each attachment is + still prepared against its own base branch under + `AC-TASKS-LAUNCH-REPOSITORY-RESOLUTION-002.3`. +- **AC-TASKS-LAUNCH-REPOSITORY-RESOLUTION-002.2:** Under `AC-...-002.1`'s + antecedent, its empty session base branch included: when that attachment's base + branch is empty, the system shall use the repository's default branch. When the + repository's default branch is also empty and the launch runs on the worktree + executor, the system shall apply the built-in default base branch to the + single-repository launch fields rather than launching with no branch + configured. On every other executor type no built-in default is applied to + those fields; and on no executor type, worktree included, is one applied to the + per-attachment repository specifications a multi-attachment launch carries. In + those cases the launch proceeds with that base branch unset, exactly as today; + the gap is named under `## Out of scope` rather than closed here. +- **AC-TASKS-LAUNCH-REPOSITORY-RESOLUTION-002.3:** When the task holds more + than one attachment for the resolved primary repository, the system shall + prepare each attachment against its own base branch, and shall not apply one + attachment's branch to another. +- **AC-TASKS-LAUNCH-REPOSITORY-RESOLUTION-002.4:** When a session carries a + non-empty base branch and the task holds exactly one attachment for the + resolved primary repository, the system shall prefer that attachment's base + branch over the session's stored value when the attachment's is non-empty, and + shall otherwise keep the session's stored value, preserving the existing + recovery behavior in which a base-branch repair on the task takes effect on the + next launch. + +## Out of scope + +- **Writing `task_sessions.repository_id` when a repository is attached.** + Rejected deliberately, not overlooked: it would not repair the already-broken + sessions, which predate any such write, and a stamped value goes stale again as + soon as the task update replaces the whole attachment set. The defect here is + that two writers disagree about one fact; a third writer of it makes that + likelier, not less. The session's repository field remains a per-session + preference. +- **A repair migration or a direct `UPDATE` on existing broken sessions.** Not + required once launches resolve from the attachment set: the eight known rows + heal on their next launch. A later migration is separate work with its own + justification. +- **Any launch whose session preference names a repository that is not in the + task's attachment set.** This covers two states, and both are excluded for the + same reason. First, a *detached* preference: the task still has attachments but + the preferred repository is no longer among them. Today that repository is still + resolved as the primary and the inventory written is not self-consistent — a + task with one remaining attachment records the preferred repository, one with + several records the attachments instead. Second, a *repository-less task whose + session still carries a preference*: the launch is configured from that + preference and writes one inventory row, and the guard does not object because + there are no attachments to compare against. + + Neither is the reported defect, which requires an EMPTY preference, so every + session in the measured population is outside both. Both keep today's behavior: + `AC-...-001.3`, `AC-...-001.4` and `AC-...-001.5` are each scoped to a + preference that is empty or present in the set, so none reaches or changes + these cases. A follow-up would decide first whether a session + should keep preferring a repository the task no longer has. +- **The model-switch relaunch path.** It reads the session preference with no + attachment fallback, so it has the same shape, but performs no environment + write and cannot produce this failure. Whether it misbehaves is unverified. +- **Sessions that bind to another task's environment**, which Office + `inherit_parent` and `shared_group` membership produces. The owning task owns + that environment's inventory; a guest launch neither rewrites it nor + re-evaluates its readiness, and that is unchanged here. +- **Attachment-set changes mid-launch that do not cross empty to non-empty.** + `AC-...-001.11` is scoped to the empty-to-non-empty crossing because that is + the only transition the guards can detect: they compare a written inventory + against *whether the task has any attachments at all*, never how many or which. + A launch that reads a one-attachment set while a second is being added writes a + one-row inventory no guard rejects, and the environment becomes ready holding a + stale, partial inventory. That window is real and accepted as a residual. + + It is accepted on two grounds: it is not the reported defect, which needs no + concurrency to reproduce, and closing it means re-reading the attachment set + inside the transaction that writes the inventory and failing on mismatch — new + work at a persistence boundary this requirement leaves untouched. Nothing serializes + the write against a launch on that path: the idle check belongs to + [Attach Workspace Sources](attach-workspace-sources.md) alone, and the generic + task update takes none. A follow-up + would need to decide which deltas must fail a launch: membership changes only, + or also a base-branch edit or a `position` reorder that leaves membership + identical. +- **A built-in terminal base-branch default for executor types other than + worktree.** `AC-TASKS-LAUNCH-REPOSITORY-RESOLUTION-002.2` gives the worktree + executor's single-repository launch fields a built-in default because that path + already has one. No other executor type has an equivalent and none is added: + when an attachment's base branch, the repository's default branch and the + session's stored branch are all empty, a launch on those executors proceeds with + the primary's base branch unset, as today. The same holds for the per-attachment + specifications a multi-attachment launch carries, on any executor type including + worktree. + + This is a real gap for the population this requirement repairs, whose evidence + trail records a remote executor rather than a worktree one. It is left as it is + because it is pre-existing and unchanged by resolving the primary from the + attachment set, it needs the repository's own default branch to be missing too, + and adding a default for every executor type would change behavior on + materialization paths this requirement leaves untouched. A follow-up would + decide whether that default is per-executor or global, and whether it applies + to each attachment's specification or only the primary. +- **The environment repository inventory guards themselves.** They exist because + a ready environment with an empty inventory for a task that has attachments + breaks reuse. They are not relaxed, weakened or bypassed. +- **Detaching repositories, and any promote/demote flow** beyond what + [Attach Workspace Sources](attach-workspace-sources.md) specifies; and **the + attachment user interface and the attach operation's own validation, rollback + and materialization**, which that requirement owns. +- **Executor-specific materialization**: how a given executor clones, mounts or + worktrees a repository once the launch request names it. diff --git a/docs/specs/tasks/system-design/launch-repository-resolution.md b/docs/specs/tasks/system-design/launch-repository-resolution.md new file mode 100644 index 0000000000..30df3aeb84 --- /dev/null +++ b/docs/specs/tasks/system-design/launch-repository-resolution.md @@ -0,0 +1,457 @@ +--- +status: draft +system: tasks +requirements: + - REQ-TASKS-LAUNCH-REPOSITORY-RESOLUTION-001 + - REQ-TASKS-LAUNCH-REPOSITORY-RESOLUTION-002 +--- + +# Session Launch Repository Resolution System Design + +## Purpose and boundaries + +The task system owns how a launch request is built from durable task state. +This design fixes which record is authoritative for "what repositories does +this task have" during a launch, and makes the launch and the environment +persistence guard read that same record. + +It uses, but does not own: repository entities and clones +([workspaces](../../workspaces)), executor materialization +([executors](../../executors)), and the attach operation's own validation and +rollback ([Attach Workspace Sources](attach-workspace-sources.md)). + +## Requirement mapping + +| Requirement | Design section | +| --- | --- | +| `REQ-TASKS-LAUNCH-REPOSITORY-RESOLUTION-001` | [Two readers, one authoritative record](#two-readers-one-authoritative-record), [Decision: stamp the primary repository identity on every executor type](#decision-stamp-the-primary-repository-identity-on-every-executor-type), [Control flow](#control-flow), [Failure and recovery](#failure-and-recovery) | +| `REQ-TASKS-LAUNCH-REPOSITORY-RESOLUTION-002` | [Branch resolution](#branch-resolution) | + +## Two readers, one authoritative record + +Two components answer the same question from different places. + +**The guard** reads the table. `taskHasRepositoriesTx` +(`apps/backend/internal/task/repository/sqlite/task_environment.go:628`) runs +`SELECT COUNT(1) FROM task_repositories WHERE task_id = ?` and is consulted at +four sites: environment creation (`:59`), materialization finalization (`:323`), +and two checks inside `PersistTaskEnvironmentTransition` (`:371`) — the worktree +workspace-path guard in `validateTaskEnvironmentTransitionWorkspace` (`:412`, +consulting the guard at `:422`) and the ready-transition guard in +`validateReadyTaskEnvironment` (`:249`, consulting it at `:265`), reached through +the sibling `validateTaskEnvironmentTransitionReady` (`:431`). Those last two are +siblings, not a chain, and they enforce different invariants: `:422` fails with +"worktree-mode env requires workspace_path", so only `:59`, `:265` and `:323` +police the ready-with-empty-inventory contract. That contract is what the guard's +own comment states: only a task with no repositories configured at all may +legitimately have no inventory. + +**The launch** reads the session. `resolveResumeRepoIDAndBranch` +(`apps/backend/internal/orchestrator/executor/executor_resume.go:1775`) takes +`session.RepositoryID` and falls back to `task.Repositories[0]`. That fallback +cannot fire: `validateAndLockResume` loads the task through the raw +`Repository.GetTask` (`task.go:512`), which selects task columns only and never +attaches the one-to-many `task_repositories` rows. The neighbouring +`applyResumeMultiRepoConfig` already documents this in its own doc comment, +having been fixed for the same reason. + +So when a repository is attached after a session exists, `repositoryID` stays +empty, `applyResumeRepoConfig` returns early at `executor_resume.go:1590` +before it can populate either the single-repo fields or `req.Repositories`, +`environmentReposForLaunch` (`executor_execute.go:2622`) falls through to its +last branch and returns `nil` (`:2650-2651`), and the guard refuses a ready +environment with no inventory for a task that has one row. + +Resolving the primary correctly is necessary but **not sufficient**: even with a +resolved primary, the launch request only carries that repository's identity when +the executor is the worktree one. That is the second half of this design; see +[stamp the primary repository identity](#decision-stamp-the-primary-repository-identity-on-every-executor-type). + +**The record the launch already holds.** `applyResumeRepoConfig` opens by +calling `resumeRepoSet`, which resolves the task's attachments through +`ListTaskRepositories` — the same table the guard reads, ordered `position +ASC, created_at ASC, id ASC`. The authoritative set is therefore already in hand, +fully resolved, before the session-derived primary is chosen. The primary +selection is the only step that reaches for a different source. + +### Decision: resolve the primary from the resolved attachment set + +Take the resolved set as the source for the primary when the session carries +no preference, and delete the `task.Repositories` fallback rather than leave a +branch that reads as coverage and cannot execute. + +**Scope boundary.** This changes the empty-preference path only. A session whose +preference names a repository still present in the attachment set already +resolves to that repository and keeps doing so. A session whose preference names +a repository *absent* from the set — detached, or left behind on a task with no +attachments at all — is excluded by the requirement and must come out of this +change behaving exactly as it does today, including the inventory it writes. +That case is reached through the same `GetRepository` fallback in +`applyResumeRepoConfig`; do not "tidy" it while removing the dead branch above. +The two are adjacent in the same function and only one of them is dead. + +The alternative — populating `task.Repositories` on the resume path so the +existing fallback becomes live — was audited and rejected. Measured +2026-09-09: `task.Repositories` is read in roughly thirty non-test backend +files, and the comment on `applyResumeMultiRepoConfig` records that this +field's emptiness on the resume path is load-bearing for at least one of them. +Making it non-empty changes what every one of those readers sees on a path +they were written against. Within the executor package the only reader of +`task.Repositories` is the dead fallback itself +(`executor_resume.go:1655-1660`), so removing it costs nothing and the audit +for the alternative is the expensive one. + +### Decision: stamp the primary repository identity on every executor type + +`environmentReposForLaunch` (`executor_execute.go:2622`) projects inventory rows +from three sources in order: the per-repository worktree results, then +`req.Repositories`, then the single-repository `req.RepositoryID`. On the resume +path that last field is written at exactly one site, `executor_resume.go:1760`, +inside `applyResumeWorktreeConfig` — reached only when +`shouldUseWorktree(req.ExecutorType)` (`executor_execute.go:660`) holds, which is +true for `ExecutorTypeWorktree` alone. Neither `applyResumeRepoBasics` (`:1691`) +nor `applyResumeCloneURL` (`:1707`) writes it; both write `RepositoryURL` only. +And `applyResumeMultiRepoConfig` (`:1736`) populates `req.Repositories` only when +the resolved set holds more than one attachment. + +So a task holding exactly **one** attachment on a **non-worktree** executor +reaches `environmentReposForLaunch` with no worktree results, an empty +`req.Repositories` and an empty `req.RepositoryID`, falls through to +`:2650-2651`, returns `nil`, and is refused by the guard exactly as before — even +though the primary resolved correctly. That is not a corner case: the population +this design repairs runs on a remote executor, recorded under the intervention +class `repo-attach-breaks-ssh-launch`, and SSH is not the worktree executor. + +**The asymmetry is one conjunct.** The initial-launch path +(`applyRepositoryConfig`, `executor_execute.go:1951`) stamps the identity at +`:1954` under `repoInfo.RepositoryPath != ""` alone, with no executor-type +condition. The resume path stamps it under `shouldUseWorktree(...) && +repositoryPath != ""`. That extra conjunct is the whole of this half of the +defect. It stays invisible on a healthy task because the environment already +carries inventory rows from its initial launch, so the update branch's +`len(existingEnv.Repos) == 0` condition (`executor_execute.go:2493`) is false. The +repaired population's environment was materialized while the task was still +repository-less, so its inventory is genuinely empty and the refusal fires. + +**Responsibility.** Whenever `applyResumeRepoConfig` resolves a primary +repository, the launch request carries that repository's identity — +`req.RepositoryID` — on every executor type. `environmentReposForLaunch` then +receives an identity it already knows how to project and needs no change of its +own. + +**The stamp is unconditional on the resolved primary.** It is deliberately NOT +gated on a non-empty local repository path, which is where it departs from the +initial-launch path: `applyRepositoryConfig` conditions its whole block on +`repoInfo.RepositoryPath != ""` (`executor_execute.go:1952`). That path can be +empty on a *successful* resolution, by three routes reachable from +`ensureRepoLocalPathForSessionAndState` (`executor_resume.go:461`) — a +local-source repository with no recorded path or no cloner (`:465-466`), a +repository with no provider identity (`:485-486`), and, in the callee +`ensureRepoClonedForSessionAndState`, a nil cloner, which logs a warning and +returns no error (`:518-524`). A resolved primary with no local clone is +therefore reachable, and gating the resume stamp the way the initial path gates +its own would leave exactly that population producing no inventory row and +refused by the guard — the reported defect, unfixed. +`AC-TASKS-LAUNCH-REPOSITORY-RESOLUTION-001.4` requires one inventory row per +attachment with no local-path condition, so the stamp follows the attachment set, +not the clone. + +**This is the repository's identity only.** It does not extend the worktree +executor's built-in terminal base-branch default to any other executor type, and +it does not give the per-attachment specifications one. `AC-...-002.2` and the +requirement's matching `## Out of scope` bullet keep that boundary exactly where +they put it; see [Branch resolution](#branch-resolution). The two travel on +different fields and only the identity moves here. + +**Blast radius.** Stamping unconditionally does introduce one request shape the +initial-launch path never produces — `RepositoryID` non-empty while +`RepositoryPath` is empty — because on that path the two are set together inside +the same `RepositoryPath != ""` block. It is not enough to observe that these +readers already see a non-empty identity on a non-worktree initial launch; each +was checked against the new shape specifically, and none reads the path in a way +that changes its outcome: + +- `failingLaunchRepositoryIdentity` (`executor_execute.go:1462`) returns the + identity pair for launch-failure attribution and never reads the path. + `handleEarlyLaunchFailure` (`:1556`) already passes an empty task-repository id + across the same boundary, so a partially-populated identity is pre-existing + there. +- `environmentReposForLaunch` (`:2650`, `:2667`) projects the identity and a + branch slug. `TaskEnvironmentRepo` has no repository-path column, and the + physical-worktree fields are filled from the launch response, not the request. +- `topLevelLaunchRepoSpec` (`executor_environment_reuse.go:379`) copies the path + into a `RepoSpec`, but its only reachable non-worktree consumer is + `canonicalInventoryMatches` (`:112`), which matches on `RepositoryID` and + branch slug alone. Its other caller, `reuseExistingRepositoryWorktrees` + (`:271`), returns early on `!req.UseWorktree`. + +Whether a launch that resolves a primary it cannot clone then succeeds is +materialization's concern, not this design's. The inventory row records the +repository slot the task is attached to, which is true whether or not a clone +exists yet. + +**Rejected alternative: drop the `len(allRepos) > 1` gate** so a single +attachment also populates `req.Repositories`. It satisfies the same criteria, but +`applyResumeMultiRepoConfig` also sets `req.TaskDirName` and routes the launch +down the multi-repository preparer path, so it would change materialization for +every single-repository worktree resume — tasks that are not broken. The narrower +change touches only the executor types that are. + +### Decision: no session back-fill + +`task_sessions.repository_id` stays a per-session preference written once at +`prepareSession` (`executor_execute.go:939`) from +`GetPrimaryTaskRepository`. Neither the attach path +(`service_workspace_sources.go`) nor `replaceTaskRepositories` +(`service_tasks.go:1843`) writes it, and neither should start. + +Resolving at launch repairs the already-broken sessions on their next launch, +with no migration. A back-fill would not: it can only stamp sessions from the +moment it ships, so the existing rows would still need a repair migration. +`replaceTaskRepositories` also deletes and recreates the whole attachment set, +so a stamped value can be left naming a row that no longer exists — the same +divergence, re-created by a third writer of the same fact. + +## Components and responsibilities + +| Component | Responsibility | +| --- | --- | +| `Executor.applyResumeRepoConfig` (`executor_resume.go:1573`) | Owns launch-time repository resolution. Already resolves the authoritative set; gains ownership of the primary selection, and of stamping the resolved primary's identity on the request for every executor type rather than only the worktree one. | +| `resolveResumeRepoIDAndBranch` (`executor_resume.go:1775`) | Selects the primary. Must take the resolved set as input instead of the task record, and lose the unreachable branch. | +| `Repository.ListTaskRepositories` (`task_repository.go:84`) | Authoritative ordered read of the attachment set. Its `ORDER BY` is the primary-selection order. | +| `Repository.GetPrimaryTaskRepository` (`task_repository.go:255`) | Defines "primary" as the first row of that read. The launch must agree with it, not define a second primary. | +| `environmentReposForLaunch` (`executor_execute.go:2622`) | Projects the launch request into inventory rows. Unchanged in itself — but it is correct only once the request actually carries the primary's identity, which on a non-worktree executor it does not today. See [stamp the primary repository identity](#decision-stamp-the-primary-repository-identity-on-every-executor-type); without that half, this component still returns `nil` at `:2650-2651` for a single-attachment non-worktree launch and the guard still refuses. | +| `taskHasRepositoriesTx` (`task_environment.go:628`) | The guard. Unchanged. | + +Backend only. No frontend component changes: the user-visible outcome is that +an existing launch action succeeds where it previously failed. + +## Data and contracts + +- `task_repositories` — the attachment set. `UNIQUE(task_id, repository_id, + base_branch, checkout_branch)`, so one repository may legitimately appear + more than once with different branches. `position INTEGER DEFAULT 0`. +- `task_sessions.repository_id`, `task_sessions.base_branch` — the session + preference. Both `TEXT DEFAULT ''`; empty means "not stated", not + "repository-less". +- `task_environment_repos` — the environment inventory the guard requires. +- `LaunchAgentRequest.RepositoryID` / `.Repositories` — the launch-side + projection consumed by `environmentReposForLaunch`. + +**Ordering.** `ListTaskRepositories` orders `position ASC, created_at ASC`, +which is not total: two rows with equal `position` written in the same +timestamp tick have no defined order, so "the primary" is undefined for them. +The order must be made total by appending `id ASC` as a final tiebreak, and +the launch must select through the same definition +`GetPrimaryTaskRepository` uses rather than introducing a second one. + +## Control flow + +1. A launch of an existing session enters through `LaunchSession` with + `IntentResume`. Workflow auto-start, a Backlog bounce, and + `RecoverSession` (`session_launch.go:419`) with any of `resume`, + `resume_new_branch`, `fresh_start`, or `runtime_retry` all converge here; + `fresh_start` differs only in clearing the resume token first. +2. `validateAndLockResume` takes the per-session lock and loads the task + (without its attachments). +3. `buildResumeRequestAtCredentialBoundaryWithOptions` resolves the attachment + set once and passes it into `applyResumeRepoConfig` + (`executor_resume.go:1266`). +4. `applyResumeRepoConfig` selects the primary — from the session preference + when set, otherwise from the resolved set — resolves the branch, populates + the single-repository fields **including the primary's identity, which must be + stamped on every executor type and not only the worktree one, and whether or + not that repository has a local clone**, and populates + `req.Repositories` when the set holds more than one attachment. The "when set" + arm is unchanged and still fires for a preference that names a repository + absent from the set; per the scope boundary above that arm is out of scope, + not a target. +5. The launch runs; `persistTaskEnvironment` projects the request into + inventory rows and writes the environment, where the guard admits it. + +The single-repository and multi-repository cases differ only in step 4's last +clause: `environmentReposForLaunch` produces one row from the single-repository +fields, or one row per entry of `req.Repositories`. Both satisfy the guard — +but the single-repository branch does so only when step 4 stamped the primary's +identity, which is why that stamp is a named responsibility above rather than an +implementation detail. A launch that resolves the right primary and does not +stamp it produces no rows at all and is refused exactly as it is today. + +## Branch resolution + +`resolveResumeBaseBranch` (`executor_resume.go:1671`) already implements the +required precedence and is unchanged; it simply becomes reachable with a +resolved primary. It matches the primary against the resolved set: an +unambiguous match's non-empty base branch wins over the session's stored +value, so a base-branch repair on the task takes effect on the next launch; +an ambiguous match (the same repository attached twice) falls back to the +session value, because `task_sessions` predates exact row identity and cannot +name which of the two rows it meant. So does an *unambiguous* match whose base +branch is empty: `resolveResumeBaseBranch` returns the session's stored value in +all three of its non-winning cases — no match, an ambiguous match, and a single +match whose branch is empty. **The precedence is therefore four rungs, not +three:** the attachment's branch, then the repository's default branch, then the +session's stored branch, then — on the worktree executor alone — the built-in +constant. `AC-...-002.2` picks that chain up at the second rung and runs it to +the fourth, under `AC-...-002.1`'s antecedent, in which the session carries no +branch so the third is empty by construction; the requirement's `## Out of scope` +bullet on the terminal default states the same chain with the session rung +present. + +**Where `AC-...-002.4` sits on that chain.** `AC-...-002.4` takes the +complementary antecedent — the session's stored branch is NON-empty — and its +phrase "that attachment's base branch" denotes the value already resolved through +the SECOND rung: `repoInfo.BaseBranch` after `resolveTaskRepoInfoForSession` has +defaulted an empty attachment branch to the repository's default branch. It is not +the raw `task_repositories.base_branch` column. That resolved value is exactly what +`resolveResumeBaseBranch` receives, so the AC's "when the attachment's is +non-empty" tests the second rung's output, and its "otherwise" arm — keep the +session's stored value — is reached only when the attachment's branch and the +repository's default branch are BOTH empty. Worked case: a session branch of +`main`, one attachment whose raw `base_branch` is empty, and a repository default +branch of `develop` resolve to `develop`, because the repository default outranks +the session's stored value exactly as the four-rung order above states. +`AC-...-002.2` uses the same phrase for the RAW column instead, because its +antecedent sits at the first rung's input rather than the second's output. The two +criteria describe one chain from two different points on it; neither introduces a +second ordering. + +For the population this design repairs the session's +stored branch is itself empty, so the primary's base branch is left unset and only +the per-attachment specifications carry branches; `AC-...-002.1` states that +outcome rather than leaving a test author to discover it. `resolveTaskRepoInfoForSession` has +already defaulted an empty attachment branch to the repository's default branch +before this point, and `backfillRepoDefaultBranch` fills that default from the +local clone when the repository row itself lacks one. When all three are still +empty the single-repo worktree path falls back to the `defaultBaseBranch` constant +(`executor.go:165`, currently `"main"`), applied at `executor_resume.go:1764` +inside `applyResumeWorktreeConfig`. That is the only site applying it on this +path, and it is reached only when `shouldUseWorktree(req.ExecutorType)` +(`executor_execute.go:660`) holds — true for `ExecutorTypeWorktree` alone, not +for `ssh`, `local_docker`, `remote_docker`, `sprites`, `k8s` or `local` +(`internal/task/models/models.go:1962-1969`). The per-attachment specifications +built by `buildRepoSpecs` receive no such fallback either. The terminal default +is therefore real but narrow, and `AC-...-002.2` is scoped to exactly that +worktree path: on other executor types a launch whose attachment branch, +repository default branch and session branch are all empty proceeds with the +primary's base branch unset. Both the fallback and its narrowness are existing +behavior, unchanged by this design; they are stated here because `AC-...-002.2` +now draws a boundary through them, and the requirement's `## Out of scope` +records the missing equivalent elsewhere as a named pre-existing gap. + +## Failure and recovery + +- **Attachment read failure.** The read that resolves the launch's repositories + fails the launch and surfaces the error; that read is what `AC-...-001.6` is + scoped to. It fails closed in the same DIRECTION as `taskIsRepoBacked` + (`executor_execute.go:2571`) — neither treats a read error as "no + repositories", which is what would produce the ready-with-empty-inventory state + the guards exist to prevent — but not by the same mechanism, and the AC does not + reach it. `taskIsRepoBacked` logs the error and returns `true` (`:2578-2579`), + so it cannot surface it to its caller: a failure of the guards' own read is + either absorbed, when this launch wrote inventory, or reported as the + missing-inventory message at `:2496` rather than as the read error. That is the + guards' behaviour, unchanged here, and the requirement's `## Out of scope` keeps + the guards themselves outside this change. +- **Attachment set changes mid-launch.** Assume nothing serializes an + attachment-set write against a launch. `workspaceSourcesIdle` + (`service_workspace_sources.go:694`) has exactly one caller, + `AttachWorkspaceSources` (`:106`). It rejects an attach while any session has an + active turn, but it does not serialize against a launch that has not yet + produced a turn — and, decisive here, that operation refuses a task with no + attachments (`:114`), so it cannot perform the empty-to-non-empty crossing at + all. The only writer that can is the generic task update through + `replaceTaskRepositories` (`service_tasks.go:1843`), which takes no idleness + check. So on the path that produces `AC-...-001.11`'s scenario there is no + synchronization to rely on, and a race test must create the window itself rather + than assume the idle check narrows it. What the guards do in that window depends + entirely on which transition occurred, and only one of the two is covered. + + **Empty to non-empty is covered.** The launch resolved an empty set and so + writes no inventory rows, and it is refused before any environment reaches + ready. Which check refuses it depends on the branch, and the two are not the + same mechanism. Creating a fresh environment, the guard re-reads the table + inside the same transaction as the INSERT and refuses ahead of it (`:59`), so + nothing partial lands. Updating an environment that already exists, the first + refusal comes earlier and outside any transaction, from `taskIsRepoBacked` + (`executor_execute.go:2571`) reading the attachment set afresh before the + transition is attempted (`executor_execute.go:2485`). It restores the previous + status and fails with its own distinct message, "persist task environment: ready + status requires repository inventory" (`:2496`), and it deliberately stands + down while the environment is still `CREATING`, leaving the initial + materializer responsible for publishing or failing the inventory; the + transactional `:265` check is the backstop behind both. Both branches fail + closed, and neither brings an environment to ready nor writes an inventory row, + which is what `AC-...-001.11` asserts. A ready + environment that already existed is left untouched — a task repository-less at + session creation may own one carrying an empty inventory — and it does not block + the next launch, which resolves the new set and succeeds. + + **Non-empty to a different non-empty is NOT covered, and this design does not + close it.** An earlier draft claimed the same guard handled the general case. + It does not: every `taskHasRepositoriesTx` call site tests only whether the + task has *any* attachments, and compares that against an inventory that is + *empty*. A launch that resolved a one-attachment set while a second attachment + was being added writes one row, `len(env.Repos) == 0` is false at every site, + no branch fires, and the environment becomes ready holding a stale partial + inventory — the state these guards exist to prevent, reached by a path they + cannot see. Closing it means re-reading the attachment set inside the + transaction that writes the inventory and failing on mismatch, which is new + work at a persistence boundary this design otherwise leaves untouched. It is + recorded as an accepted residual in the requirement's `## Out of scope` rather + than claimed as handled here. +- **Concurrent launches of one session.** Serialized by the per-session lock + taken in `validateAndLockResume` (`executor_resume.go:1033`). The lock is + blocking, so the second launch is not refused on arrival: it waits for the + first to release, then re-reads the session row inside the lock and is rejected + there, before it can resolve repositories or write an environment. Two distinct + rejections, both reachable: `SessionStateSupersededError` when the re-read shows + a terminal state different from the one this launch requested + (`executor_resume.go:1092`), and `ErrExecutionAlreadyRunning` + (`executor.go:170`) when the session is non-terminal and an execution is already + registered for it (`executor_resume.go:1106`). Neither fires in a third case, + which `AC-...-001.12` now names rather than leaving to be discovered: if the + first launch failed and `rollbackResumeStateAfterFailure` restored the session + to the same state the second launch requested, that state is terminal but not + *different*, so the superseded branch does not fire; and being terminal it + skips the already-running check entirely (`:1103-1107`). The second launch then + proceeds and resolves repositories by the rules above, which is correct — no + execution is running and it is a retry, not a duplicate. The in-lock re-read is + what makes all three outcomes safe: the caller fetched the session before taking + the lock, so its copy of the state may already be stale. The lock is held for + the whole of `ResumeSession` (`defer unlock()`, `executor_resume.go:782`), not + merely for the re-read, so a second launch cannot observe a half-built + environment. This is `AC-...-001.12`; the mechanism is unchanged by this design + and is documented here only because the AC asserts it. +- **Repository row missing.** Unchanged: `GetRepository` fails and the launch + fails, whether the id came from the session or the attachment set. + +## Persistence + +No schema change beyond the `ORDER BY` tiebreak, which affects a read only. No +migration and no backfill: the sessions currently in the broken state are +repaired by the next launch resolving correctly. Restart behavior is +unchanged, and startup recovery reaches this path through the same resume +entry point as any other launch. + +## Security + +None affected. Repository resolution stays inside the task's own attachment +set and its workspace; no authorization boundary moves. Git credential +issuance continues to key off the resolved repositories, so a launch that +previously carried none and now carries the task's attachments requests +exactly the credentials that task's repositories already require. + +## Observability + +The failure is currently visible only as the guard's error text. Resolution +that falls back to the attachment set because the session carried no +preference should be logged at info with the task id, session id, and the +resolved repository id, so the repaired-session case is distinguishable in +logs from a session that always had a preference. No new metric. + +## Related decisions + +None. This design records a defect correction inside an existing contract +([Attach Workspace Sources](attach-workspace-sources.md)) and does not +establish a new boundary.