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 <carlosmflorencio@gmail.com>
This commit is contained in:
@@ -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 {
|
||||
|
||||
+125
@@ -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")
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
+572
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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...)
|
||||
|
||||
+90
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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)
|
||||
|
||||
@@ -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=<id>)
|
||||
```
|
||||
|
||||
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.
|
||||
@@ -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.
|
||||
Reference in New Issue
Block a user