diff --git a/apps/backend/internal/agent/runtime/lifecycle/env_preparer.go b/apps/backend/internal/agent/runtime/lifecycle/env_preparer.go index 0e53c84a72..4c2b93ad25 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/env_preparer.go +++ b/apps/backend/internal/agent/runtime/lifecycle/env_preparer.go @@ -2,14 +2,55 @@ package lifecycle import ( "context" + "fmt" "strings" "time" "github.com/kandev/kandev/internal/agent/executor" + "github.com/kandev/kandev/internal/agent/runtime/routingerr" "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/task/models" ) +// RepositoryPreparationError identifies the repository whose preparation +// prevented a multi-repository launch. Its wrapped cause remains available for +// errors.Is/errors.As, while Error() exposes only bounded, credential-safe +// detail to launch and task error surfaces. +type RepositoryPreparationError struct { + RepositoryID string + TaskRepositoryID string + RepositoryName string + Cause error +} + +func (e *RepositoryPreparationError) Error() string { + if e == nil { + return "repository preparation failed" + } + name := strings.TrimSpace(e.RepositoryName) + identity := strings.TrimSpace(e.RepositoryID) + label := identity + if name != "" && identity != "" { + label = fmt.Sprintf("%s (%s)", name, identity) + } else if name != "" { + label = name + } + if label == "" { + label = "unknown repository" + } + if e.Cause == nil { + return fmt.Sprintf("repository %s preparation failed", label) + } + return fmt.Sprintf("repository %s preparation failed: %s", label, routingerr.Sanitize(e.Cause.Error())) +} + +func (e *RepositoryPreparationError) Unwrap() error { + if e == nil { + return nil + } + return e.Cause +} + // PrepareStepStatus represents the status of a preparation step. type PrepareStepStatus string @@ -26,26 +67,29 @@ const ( // carried at the top level. When EnvPrepareRequest.Repositories is non-empty, // each entry produces one prepared worktree under the shared TaskDirName. type RepoPrepareSpec struct { - TaskRepositoryID string - RepositoryID string - RepositoryPath string - RepoName string - BaseBranch string - DefaultBranch string // Repository's default_branch, used as fallback when BaseBranch is missing - CheckoutBranch string - PRNumber int // GitHub PR number when CheckoutBranch is a PR head; enables refs/pull//head fetch for fork PRs. - RemoteContribution *models.RemoteContribution - WorktreeID string + TaskRepositoryID string + RepositoryID string + RepositoryPath string + RepoName string + BaseBranch string + DefaultBranch string // Repository's default_branch, used as fallback when BaseBranch is missing + CheckoutBranch string + PRNumber int // GitHub PR number when CheckoutBranch is a PR head; enables refs/pull//head fetch for fork PRs. + RemoteContribution *models.RemoteContribution + WorktreeID string // WorkspaceReuseRequired makes preparation attach to the exact canonical // environment. It forbids worktree creation/recreation and all repository // mutating setup paths. WorkspaceReuseRequired bool - WorktreeBranch string - WorktreeBranchPrefix string - WorktreeBranchTemplate string - WorktreeBranchTicket string - PullBeforeWorktree bool - RemoteSyncHandled bool + WorktreeBranch string + WorktreeBranchPrefix string + WorktreeBranchTemplate string + WorktreeBranchTicket string + PullBeforeWorktree bool + RemoteSyncHandled bool + // RefreshRepository is an optional provider-authenticated refresh deferred + // until worktree materialization. A valid reusable worktree bypasses it. + RefreshRepository func(context.Context) error RepoSetupScript string ContributionDestination *models.ContributionDestination // BranchSlug, when set, suffixes the worktree path as @@ -92,6 +136,9 @@ type EnvPrepareRequest struct { WorktreeBranchTicket string PullBeforeWorktree bool RemoteSyncHandled bool + // RefreshRepository is an optional provider-authenticated refresh deferred + // until worktree materialization. A valid reusable worktree bypasses it. + RefreshRepository func(context.Context) error TaskDirName string // Per-task directory name within the workspace (e.g. "task-abc123") RepoName string // Repository slug used with TaskDirName to locate checkouts @@ -139,6 +186,7 @@ func (r *EnvPrepareRequest) RepoSpecs() []RepoPrepareSpec { WorktreeBranchTicket: r.WorktreeBranchTicket, PullBeforeWorktree: r.PullBeforeWorktree, RemoteSyncHandled: r.RemoteSyncHandled, + RefreshRepository: r.RefreshRepository, RepoSetupScript: r.RepoSetupScript, BranchSlug: r.BranchSlug, BranchIdentitySlug: r.BranchIdentitySlug, diff --git a/apps/backend/internal/agent/runtime/lifecycle/env_preparer_worktree.go b/apps/backend/internal/agent/runtime/lifecycle/env_preparer_worktree.go index 79fdc17eaf..b64c160476 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/env_preparer_worktree.go +++ b/apps/backend/internal/agent/runtime/lifecycle/env_preparer_worktree.go @@ -302,6 +302,7 @@ func buildWorktreeCreateRequest(req *EnvPrepareRequest) worktree.CreateRequest { WorktreeBranchTicket: req.WorktreeBranchTicket, PullBeforeWorktree: req.PullBeforeWorktree, RemoteSyncHandled: req.RemoteSyncHandled, + RefreshRepository: req.RefreshRepository, WorktreeID: req.WorktreeID, ReuseRequired: req.WorkspaceReuseRequired, TaskDirName: req.TaskDirName, @@ -417,6 +418,12 @@ func (p *WorktreePreparer) prepareMultiRepo( steps = newSteps stepIdx = nextIdx if err != nil { + err = &RepositoryPreparationError{ + RepositoryID: spec.RepositoryID, + TaskRepositoryID: spec.TaskRepositoryID, + RepositoryName: spec.RepoName, + Cause: err, + } p.rollbackWorktrees(ctx, createdIDs) return &EnvPrepareResult{ Success: false, @@ -528,6 +535,7 @@ func (p *WorktreePreparer) prepareOneRepo( subReq.WorktreeBranchTicket = spec.WorktreeBranchTicket subReq.PullBeforeWorktree = spec.PullBeforeWorktree subReq.RemoteSyncHandled = spec.RemoteSyncHandled + subReq.RefreshRepository = spec.RefreshRepository subReq.BranchSlug = spec.BranchSlug subReq.BranchIdentitySlug = repoBranchIdentitySlug(spec) // Strip the multi-repo list to avoid re-entering the multi-repo branch. @@ -607,5 +615,7 @@ func applySyncProgressEvent(step *PrepareStep, event worktree.SyncProgressEvent) now := time.Now() step.Status = PrepareStepCompleted step.EndedAt = &now + case worktree.SyncProgressFailed: + completeStepError(step, event.Error) } } diff --git a/apps/backend/internal/agent/runtime/lifecycle/env_preparer_worktree_invalid_repo_test.go b/apps/backend/internal/agent/runtime/lifecycle/env_preparer_worktree_invalid_repo_test.go index fe38aca064..8b0e0e2c06 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/env_preparer_worktree_invalid_repo_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/env_preparer_worktree_invalid_repo_test.go @@ -96,7 +96,10 @@ func TestWorktreePreparer_MultiRepo_ValidateRepository_FailsOnNonGitPath(t *test if !strings.Contains(res.ErrorMessage, "not a git repository") { t.Errorf("ErrorMessage = %q, want it to mention the path is not a git repository", res.ErrorMessage) } - if !strings.Contains(res.ErrorMessage, staleSecondary) { - t.Errorf("ErrorMessage = %q, want it to name the offending path %q", res.ErrorMessage, staleSecondary) + if strings.Contains(res.ErrorMessage, staleSecondary) { + t.Errorf("ErrorMessage = %q, must not expose the offending absolute path %q", res.ErrorMessage, staleSecondary) + } + if !strings.Contains(res.ErrorMessage, "***") { + t.Errorf("ErrorMessage = %q, want credential-safe sanitization marker", res.ErrorMessage) } } diff --git a/apps/backend/internal/agent/runtime/lifecycle/env_preparer_worktree_multi_test.go b/apps/backend/internal/agent/runtime/lifecycle/env_preparer_worktree_multi_test.go index 7aa74ae81f..229864e4aa 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/env_preparer_worktree_multi_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/env_preparer_worktree_multi_test.go @@ -2,6 +2,7 @@ package lifecycle import ( "context" + "errors" "os" "os/exec" "path/filepath" @@ -293,6 +294,51 @@ func TestWorktreePreparer_MultiRepo_RollbackOnPartialFailure(t *testing.T) { } } +func TestWorktreePreparer_MultiRepo_RequiredRefreshIdentifiesFailingRepository(t *testing.T) { + repoA := initBareGitRepo(t, "good-refresh") + repoB := initBareGitRepo(t, "bad-refresh") + + preparer, mgr := newPreparerForTest(t) + req := &EnvPrepareRequest{ + TaskID: "task-refresh-fail", + SessionID: "sess-refresh-fail", + TaskTitle: "Required refresh failure", + ExecutorType: executor.NameStandalone, + TaskDirName: "refresh-fail_eee", + Repositories: []RepoPrepareSpec{ + {TaskRepositoryID: "tr-good", RepositoryID: "repo-good", RepositoryPath: repoA, RepoName: "good", BaseBranch: "main"}, + {TaskRepositoryID: "tr-bad", RepositoryID: "repo-bad", RepositoryPath: repoB, RepoName: "backend", BaseBranch: "main", PullBeforeWorktree: true}, + }, + } + + res, err := preparer.Prepare(context.Background(), req, nil) + if err != nil { + t.Fatalf("prepare returned hard error: %v", err) + } + if res.Success { + t.Fatal("required refresh failure unexpectedly succeeded") + } + if !strings.Contains(res.ErrorMessage, "repo-bad") || !strings.Contains(res.ErrorMessage, "backend") { + t.Fatalf("error = %q, want failing repository identity and name", res.ErrorMessage) + } + var preparationErr *RepositoryPreparationError + if !errors.As(res.Error, &preparationErr) { + t.Fatalf("error = %T %v, want RepositoryPreparationError", res.Error, res.Error) + } + if preparationErr.RepositoryID != "repo-bad" || preparationErr.TaskRepositoryID != "tr-bad" { + t.Fatalf("preparation error identity = %+v, want repo-bad/tr-bad", preparationErr) + } + if all, listErr := mgr.GetAllByTaskID(context.Background(), "task-refresh-fail"); listErr != nil { + t.Fatalf("list worktrees: %v", listErr) + } else { + for _, wt := range all { + if wt.Status == worktree.StatusActive { + t.Fatalf("active worktree after required refresh failure = %s, want none", wt.ID) + } + } + } +} + func TestWorktreePreparer_MultiRepo_RollbackKeepsReusedWorktrees(t *testing.T) { repoExisting := initBareGitRepo(t, "existing") repoNew := initBareGitRepo(t, "new") diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_launch.go b/apps/backend/internal/agent/runtime/lifecycle/manager_launch.go index 4f48d94dde..21545888a1 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_launch.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_launch.go @@ -1043,6 +1043,7 @@ func buildEnvPrepareRequest(req *LaunchRequest, workspacePath string, execName e WorktreeBranchTicket: req.WorktreeBranchTicket, PullBeforeWorktree: req.PullBeforeWorktree, RemoteSyncHandled: req.RemoteSyncHandled, + RefreshRepository: req.RefreshRepository, TaskDirName: req.TaskDirName, RepoName: req.RepoName, BranchSlug: req.BranchSlug, @@ -1075,6 +1076,7 @@ func buildEnvPrepareRequest(req *LaunchRequest, workspacePath string, execName e WorktreeBranchTicket: r.WorktreeBranchTicket, PullBeforeWorktree: r.PullBeforeWorktree, RemoteSyncHandled: r.RemoteSyncHandled, + RefreshRepository: r.RefreshRepository, RepoSetupScript: setup, BranchSlug: r.BranchSlug, BranchIdentitySlug: r.BranchIdentitySlug, diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_launch_error_chain_test.go b/apps/backend/internal/agent/runtime/lifecycle/manager_launch_error_chain_test.go index 7ef9e1b476..e64402f2a5 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_launch_error_chain_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_launch_error_chain_test.go @@ -207,3 +207,31 @@ func TestWorktreePreparerMultiRepoFailureCarriesError(t *testing.T) { require.False(t, result.Success) require.Error(t, result.Error) } + +func TestLaunchRequiredPreparationFailureDoesNotCreateRuntime(t *testing.T) { + profileResolver := &countingProfileResolver{info: &AgentProfileInfo{ + ProfileID: "profile-refresh-failure", + AgentName: "auggie", + }} + mgr, backend := newEnvironmentExecutionTestManagerWithProfileResolver(t, nil, profileResolver) + mgr.preparerRegistry = NewPreparerRegistry(mgr.logger) + mgr.preparerRegistry.Register("worktree", &directErrorPreparer{err: &RepositoryPreparationError{ + RepositoryID: "repo-back", + TaskRepositoryID: "tr-back", + RepositoryName: "backend", + Cause: worktree.ErrGitCommandFailed, + }}) + + _, err := mgr.Launch(context.Background(), &LaunchRequest{ + TaskID: "task-refresh-failure", + SessionID: "session-refresh-failure", + AgentProfileID: "profile-refresh-failure", + ExecutorType: "worktree", + RepositoryPath: "/tmp/repo", + UseWorktree: true, + BaseBranch: "main", + }) + require.Error(t, err) + require.ErrorIs(t, err, worktree.ErrGitCommandFailed) + require.Equal(t, int32(0), backend.createCount.Load(), "runtime must not be created after preparation failure") +} diff --git a/apps/backend/internal/agent/runtime/lifecycle/types.go b/apps/backend/internal/agent/runtime/lifecycle/types.go index 6a7d45e115..4ef05e7a53 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/types.go +++ b/apps/backend/internal/agent/runtime/lifecycle/types.go @@ -750,22 +750,25 @@ func (ae *AgentExecution) EndSessionSpan() { // the top level. When LaunchRequest.Repositories is set, each entry produces // one prepared worktree under the shared TaskDirName. type RepoLaunchSpec struct { - TaskRepositoryID string - RepositoryID string - RepositoryPath string - RepositoryURL string // Clone URL for remote executors that need to clone - RepoName string // Repository name used as subdirectory inside TaskDirName - BaseBranch string - DefaultBranch string // Repository's default_branch, used as fallback when BaseBranch is missing - CheckoutBranch string - PRNumber int // GitHub PR number when CheckoutBranch is a PR head; enables refs/pull//head fetch for fork PRs. - RemoteContribution *models.RemoteContribution - WorktreeID string // Existing worktree ID to reuse (skip creation if set) - WorktreeBranchPrefix string - WorktreeBranchTemplate string - WorktreeBranchTicket string - PullBeforeWorktree bool - RemoteSyncHandled bool + TaskRepositoryID string + RepositoryID string + RepositoryPath string + RepositoryURL string // Clone URL for remote executors that need to clone + RepoName string // Repository name used as subdirectory inside TaskDirName + BaseBranch string + DefaultBranch string // Repository's default_branch, used as fallback when BaseBranch is missing + CheckoutBranch string + PRNumber int // GitHub PR number when CheckoutBranch is a PR head; enables refs/pull//head fetch for fork PRs. + RemoteContribution *models.RemoteContribution + WorktreeID string // Existing worktree ID to reuse (skip creation if set) + WorktreeBranchPrefix string + WorktreeBranchTemplate string + WorktreeBranchTicket string + PullBeforeWorktree bool + RemoteSyncHandled bool + // RefreshRepository is an optional provider-authenticated refresh deferred + // until worktree materialization. A valid reusable worktree bypasses it. + RefreshRepository func(context.Context) error RepoSetupScript string // Repository-level setup script (optional) RepoCleanupScript string // Repository-level cleanup script (optional) CopyFiles string // Comma-separated paths/globs to copy from the source repo (gitignored .env / config files) @@ -890,22 +893,25 @@ type LaunchRequest struct { CopyFiles string // Worktree configuration - UseWorktree bool // Whether to use a Git worktree for isolation - WorktreeID string // Existing worktree ID to reuse (skip creation if set) - RepositoryID string // Repository ID for worktree tracking - TaskRepositoryID string // Exact task_repositories row for worktree recovery - RepositoryPath string // Path to the main repository (for worktree creation) - BaseBranch string // Base branch for the worktree (e.g., "main") - DefaultBranch string // Repository's default_branch, used as fallback when BaseBranch is missing - CheckoutBranch string // Branch to fetch and checkout after worktree creation (e.g., PR head branch) - PRNumber int // GitHub PR number when CheckoutBranch is a PR head; enables refs/pull//head fetch for fork PRs. - RemoteContribution *models.RemoteContribution - ComparisonTarget *models.ComparisonTarget - WorktreeBranchPrefix string // Branch prefix for worktree branches - WorktreeBranchTemplate string // Branch name template for worktree branches - WorktreeBranchTicket string // External ticket value for branch templates - PullBeforeWorktree bool // Whether to pull from remote before creating the worktree - RemoteSyncHandled bool // Authenticated provider refresh already completed + UseWorktree bool // Whether to use a Git worktree for isolation + WorktreeID string // Existing worktree ID to reuse (skip creation if set) + RepositoryID string // Repository ID for worktree tracking + TaskRepositoryID string // Exact task_repositories row for worktree recovery + RepositoryPath string // Path to the main repository (for worktree creation) + BaseBranch string // Base branch for the worktree (e.g., "main") + DefaultBranch string // Repository's default_branch, used as fallback when BaseBranch is missing + CheckoutBranch string // Branch to fetch and checkout after worktree creation (e.g., PR head branch) + PRNumber int // GitHub PR number when CheckoutBranch is a PR head; enables refs/pull//head fetch for fork PRs. + RemoteContribution *models.RemoteContribution + ComparisonTarget *models.ComparisonTarget + WorktreeBranchPrefix string // Branch prefix for worktree branches + WorktreeBranchTemplate string // Branch name template for worktree branches + WorktreeBranchTicket string // External ticket value for branch templates + PullBeforeWorktree bool // Whether to pull from remote before creating the worktree + RemoteSyncHandled bool // Authenticated provider refresh already completed + // RefreshRepository is an optional provider-authenticated refresh deferred + // until worktree materialization. A valid reusable worktree bypasses it. + RefreshRepository func(context.Context) error ContributionDestination *models.ContributionDestination // Task directory mode: place worktree at ~/.kandev/tasks/{TaskDirName}/{RepoName}/ @@ -957,6 +963,7 @@ func (r *LaunchRequest) RepoSpecs() []RepoLaunchSpec { WorktreeBranchTicket: r.WorktreeBranchTicket, PullBeforeWorktree: r.PullBeforeWorktree, RemoteSyncHandled: r.RemoteSyncHandled, + RefreshRepository: r.RefreshRepository, CopyFiles: r.CopyFiles, BranchSlug: r.BranchSlug, BranchIdentitySlug: r.BranchIdentitySlug, diff --git a/apps/backend/internal/backendapp/adapters.go b/apps/backend/internal/backendapp/adapters.go index 433ed531e3..9f9819fe9e 100644 --- a/apps/backend/internal/backendapp/adapters.go +++ b/apps/backend/internal/backendapp/adapters.go @@ -252,6 +252,7 @@ func buildLifecycleLaunchRequest( WorktreeBranchTicket: req.WorktreeBranchTicket, PullBeforeWorktree: req.PullBeforeWorktree, RemoteSyncHandled: req.RemoteSyncHandled, + RefreshRepository: req.RefreshRepository, TaskDirName: req.TaskDirName, RepoName: req.RepoName, BranchSlug: req.BranchSlug, @@ -314,6 +315,7 @@ func lifecycleRepoLaunchSpecs(repos []executor.RepoSpec) []lifecycle.RepoLaunchS WorktreeBranchTicket: r.WorktreeBranchTicket, PullBeforeWorktree: r.PullBeforeWorktree, RemoteSyncHandled: r.RemoteSyncHandled, + RefreshRepository: r.RefreshRepository, RepoSetupScript: r.RepoSetupScript, RepoCleanupScript: r.RepoCleanupScript, CopyFiles: r.CopyFiles, diff --git a/apps/backend/internal/backendapp/adapters_test.go b/apps/backend/internal/backendapp/adapters_test.go index dcf1183b22..7b48559b32 100644 --- a/apps/backend/internal/backendapp/adapters_test.go +++ b/apps/backend/internal/backendapp/adapters_test.go @@ -99,6 +99,14 @@ func (f *distinctFiller) fill(t *testing.T, v reflect.Value, name string) { f.fill(t, value, name+"-value") m.SetMapIndex(key, value) v.Set(m) + case reflect.Func: + v.Set(reflect.MakeFunc(v.Type(), func([]reflect.Value) []reflect.Value { + outputs := make([]reflect.Value, v.Type().NumOut()) + for i := range outputs { + outputs[i] = reflect.Zero(v.Type().Out(i)) + } + return outputs + })) default: t.Fatalf("distinctFiller: unsupported kind %s for field %s — extend the helper", v.Kind(), name) } @@ -125,6 +133,12 @@ func assertSameNamedFieldsEqual(t *testing.T, label string, src, dst any) { continue } want := srcVal.Field(i).Interface() + if srcVal.Field(i).Kind() == reflect.Func { + if srcVal.Field(i).IsNil() != dstField.IsNil() { + t.Errorf("%s: field %s nil = %v, want %v", label, name, dstField.IsNil(), srcVal.Field(i).IsNil()) + } + continue + } if got := dstField.Interface(); !reflect.DeepEqual(got, want) { t.Errorf("%s: field %s = %#v, want %#v", label, name, got, want) } diff --git a/apps/backend/internal/orchestrator/executor/executor.go b/apps/backend/internal/orchestrator/executor/executor.go index 12aaa2141d..b8a9a17072 100644 --- a/apps/backend/internal/orchestrator/executor/executor.go +++ b/apps/backend/internal/orchestrator/executor/executor.go @@ -439,6 +439,9 @@ type LaunchAgentRequest struct { WorktreeBranchTicket string // External ticket value for branch templates PullBeforeWorktree bool // Whether to pull from remote before creating the worktree RemoteSyncHandled bool // Provider-authenticated origin refresh already completed + // RefreshRepository is an optional provider-authenticated refresh deferred + // until worktree materialization. A valid reusable worktree bypasses it. + RefreshRepository func(context.Context) error // Task directory mode: place worktree at ~/.kandev/tasks/{TaskDirName}/{RepoName}/ TaskDirName string // Semantic task directory name (e.g. "fix-bug_ab12") @@ -487,9 +490,12 @@ type RepoSpec struct { WorktreeBranchTicket string PullBeforeWorktree bool RemoteSyncHandled bool - RepoSetupScript string - RepoCleanupScript string - CopyFiles string + // RefreshRepository is an optional provider-authenticated refresh deferred + // until worktree materialization. A valid reusable worktree bypasses it. + RefreshRepository func(context.Context) error + RepoSetupScript string + RepoCleanupScript string + CopyFiles string // BranchSlug, when non-empty, suffixes the repo dir so the same repo can // host multiple branch worktrees as siblings within one task. Set by the // orchestrator when buildRepoSpecs detects multiple rows sharing a @@ -880,6 +886,13 @@ type authenticatedRepoCloner interface { ) (string, error) } +type strictAuthenticatedRepoCloner interface { + RefreshWorkspaceRepositoryWithBasicAuth( + ctx context.Context, workspaceID, provider, providerHost, + cloneURL, owner, name, repositoryPath, username, password string, + ) error +} + const providerAzureDevOps = "azure_devops" func (e *Executor) ensureClonedWithWorkspaceAuth( diff --git a/apps/backend/internal/orchestrator/executor/executor_environment_reuse.go b/apps/backend/internal/orchestrator/executor/executor_environment_reuse.go index bca2b9d326..f76f5db01a 100644 --- a/apps/backend/internal/orchestrator/executor/executor_environment_reuse.go +++ b/apps/backend/internal/orchestrator/executor/executor_environment_reuse.go @@ -307,6 +307,7 @@ func topLevelLaunchRepoSpec(req *LaunchAgentRequest) (RepoSpec, bool) { WorktreeBranchTicket: req.WorktreeBranchTicket, PullBeforeWorktree: req.PullBeforeWorktree, RemoteSyncHandled: req.RemoteSyncHandled, + RefreshRepository: req.RefreshRepository, CopyFiles: req.CopyFiles, BranchIdentitySlug: topLevelBranchIdentitySlug(req), }, true diff --git a/apps/backend/internal/orchestrator/executor/executor_execute.go b/apps/backend/internal/orchestrator/executor/executor_execute.go index 7a3d1d1371..5e3c608533 100644 --- a/apps/backend/internal/orchestrator/executor/executor_execute.go +++ b/apps/backend/internal/orchestrator/executor/executor_execute.go @@ -1354,6 +1354,10 @@ func failingLaunchRepositoryIdentity( if len(req.Repositories) == 0 { return req.RepositoryID, req.TaskRepositoryID } + var preparationErr *lifecycle.RepositoryPreparationError + if errors.As(launchErr, &preparationErr) && preparationErr != nil { + return preparationErr.RepositoryID, preparationErr.TaskRepositoryID + } branch := extractLaunchFailureBranch(launchErr) if len(req.Repositories) == 1 && branch == "" { @@ -1750,6 +1754,7 @@ func buildRepoSpecs(allRepos []*repoInfo) []RepoSpec { WorktreeBranchTemplate: info.WorktreeBranchTemplate, PullBeforeWorktree: info.PullBeforeWorktree, RemoteSyncHandled: info.RemoteSyncHandled, + RefreshRepository: info.RefreshRepository, } if info.Repository != nil { spec.RepoName = info.Repository.Name @@ -1820,6 +1825,7 @@ func (e *Executor) applyRepositoryConfig(req *LaunchAgentRequest, task *v1.Task, req.WorktreeBranchTemplate = repoInfo.WorktreeBranchTemplate req.PullBeforeWorktree = repoInfo.PullBeforeWorktree req.RemoteSyncHandled = repoInfo.RemoteSyncHandled + req.RefreshRepository = repoInfo.RefreshRepository if repoInfo.Repository != nil { req.DefaultBranch = repoInfo.Repository.DefaultBranch if req.UseWorktree { diff --git a/apps/backend/internal/orchestrator/executor/executor_launch_failure_classification_test.go b/apps/backend/internal/orchestrator/executor/executor_launch_failure_classification_test.go index d2d0c41fa1..9d259db6c4 100644 --- a/apps/backend/internal/orchestrator/executor/executor_launch_failure_classification_test.go +++ b/apps/backend/internal/orchestrator/executor/executor_launch_failure_classification_test.go @@ -3,8 +3,10 @@ package executor import ( "context" "errors" + "strings" "testing" + "github.com/kandev/kandev/internal/agent/runtime/lifecycle" "github.com/kandev/kandev/internal/task/models" "github.com/kandev/kandev/internal/worktree" ) @@ -74,3 +76,21 @@ func TestLaunchFailureReviewActionRequiresSuccessfulEligibilityResolver(t *testi t.Fatalf("failed eligibility lookup exposed recovery actions = %#v", errorValue.RecoveryActions) } } + +func TestBuildLastAgentErrorSanitizesRepositoryPreparationDetails(t *testing.T) { + exec := &Executor{} + launchErr := &lifecycle.RepositoryPreparationError{ + RepositoryID: "repo-back", + RepositoryName: "backend", + Cause: errors.New("fatal: https://user:ghp_abcdefghijklmnopqrstuvwxyz1234567890AB@example.com/repo.git"), + } + + errorValue := exec.buildLastAgentError(context.Background(), "task-1", "task-repo-2", launchErr) + if !strings.Contains(errorValue.Details, "repo-back") || !strings.Contains(errorValue.Details, "backend") { + t.Fatalf("launch details = %q, want repository identity", errorValue.Details) + } + if strings.Contains(errorValue.Details, "ghp_abcdefghijklmnopqrstuvwxyz1234567890AB") || + strings.Contains(errorValue.Details, "user:") { + t.Fatalf("launch details exposed credential-bearing URL: %q", errorValue.Details) + } +} diff --git a/apps/backend/internal/orchestrator/executor/executor_multi_repo_test.go b/apps/backend/internal/orchestrator/executor/executor_multi_repo_test.go index a60df95762..96a4641cb1 100644 --- a/apps/backend/internal/orchestrator/executor/executor_multi_repo_test.go +++ b/apps/backend/internal/orchestrator/executor/executor_multi_repo_test.go @@ -232,6 +232,24 @@ func TestFailingLaunchRepositoryID_UsesExactBranchToken(t *testing.T) { } } +func TestFailingLaunchRepositoryIdentityUsesPreparationError(t *testing.T) { + req := &LaunchAgentRequest{Repositories: []RepoSpec{ + {RepositoryID: "repo-front", TaskRepositoryID: "tr-1", BaseBranch: "main"}, + {RepositoryID: "repo-back", TaskRepositoryID: "tr-2", BaseBranch: "main"}, + }} + launchErr := &lifecycle.RepositoryPreparationError{ + RepositoryID: "repo-back", + TaskRepositoryID: "tr-2", + RepositoryName: "backend", + Cause: errors.New("required refresh failed"), + } + + repositoryID, taskRepositoryID := failingLaunchRepositoryIdentity(req, launchErr) + if repositoryID != "repo-back" || taskRepositoryID != "tr-2" { + t.Fatalf("failing launch identity = %q/%q, want repo-back/tr-2", repositoryID, taskRepositoryID) + } +} + func TestLaunchPreparedSession_MultiRepo_PersistsPerRepoEnvironmentAndWorktreeRows(t *testing.T) { repo := newMockRepository() taskID := "task-multi-2" diff --git a/apps/backend/internal/orchestrator/executor/executor_resume.go b/apps/backend/internal/orchestrator/executor/executor_resume.go index 85256c99ba..c61b60c9b9 100644 --- a/apps/backend/internal/orchestrator/executor/executor_resume.go +++ b/apps/backend/internal/orchestrator/executor/executor_resume.go @@ -12,6 +12,7 @@ import ( "github.com/kandev/kandev/internal/agent/runtime/lifecycle" "github.com/kandev/kandev/internal/common/gitref" + "github.com/kandev/kandev/internal/integrations/cloneauth" "github.com/kandev/kandev/internal/repoclone" "github.com/kandev/kandev/internal/task/models" "github.com/kandev/kandev/internal/worktree" @@ -75,6 +76,7 @@ type repoInfo struct { WorktreeBranchTemplate string PullBeforeWorktree bool RemoteSyncHandled bool + RefreshRepository func(context.Context) error Repository *models.Repository } @@ -223,18 +225,61 @@ func (e *Executor) resolveTaskRepoInfoForSession( if info.BaseBranch == "" && repo.DefaultBranch != "" { info.BaseBranch = repo.DefaultBranch } - if info.PullBeforeWorktree && isPluginManagedRepository(repo) { - if err := e.refreshPluginRepositoryForSession(ctx, tr.TaskID, sessionID, repo); err != nil { - return nil, err + if info.PullBeforeWorktree { + refreshRequired, refreshErr := e.shouldRefreshRepositoryForSession(ctx, repo) + if refreshErr != nil { + return nil, refreshErr + } + if !refreshRequired { + return info, nil + } + prNumber, checkoutBranch := info.PRNumber, info.CheckoutBranch + info.RefreshRepository = func(refreshCtx context.Context) error { + return e.refreshManagedRepositoryForSession( + refreshCtx, tr.TaskID, sessionID, repo, prNumber, checkoutBranch, + ) } - info.PullBeforeWorktree = false - info.RemoteSyncHandled = true } return info, nil } +func hasProviderRepositoryIdentity(repo *models.Repository) bool { + if repo == nil { + return false + } + return (strings.TrimSpace(repo.ProviderOwner) != "" && strings.TrimSpace(repo.ProviderName) != "") || + (strings.TrimSpace(repo.ProviderScope) != "" && strings.TrimSpace(repo.ProviderRepoID) != "") +} + +func (e *Executor) shouldRefreshRepositoryForSession( + ctx context.Context, repo *models.Repository, +) (bool, error) { + if repo == nil || repo.SourceType == sourceTypeLocal { + return false, nil + } + provider := strings.ToLower(strings.TrimSpace(repo.Provider)) + if provider == gitLabProviderID { + return hasProviderRepositoryIdentity(repo), nil + } + if provider == providerAzureDevOps { + return strings.TrimSpace(repo.ProviderOwner) != "" && strings.TrimSpace(repo.ProviderName) != "", nil + } + if !isGitHubRepository(repo) { + return isPluginManagedRepository(repo), nil + } + policy := TaskGitCredentialPolicy{Mode: taskGitCredentialsModeManaged} + if e.githubCredentialPolicyResolver != nil { + resolved, err := e.githubCredentialPolicyResolver.ResolveTaskGitCredentialPolicy(ctx, repo.WorkspaceID) + if err != nil { + return false, fmt.Errorf("resolve task Git credential policy: %w", err) + } + policy = resolved + } + return policy.Mode != taskGitCredentialsModeExecutor, nil +} + func isPluginManagedRepository(repo *models.Repository) bool { - if repo == nil || repo.SourceType == sourceTypeLocal || repo.ProviderOwner == "" || repo.ProviderName == "" { + if repo == nil || repo.SourceType == sourceTypeLocal || !hasProviderRepositoryIdentity(repo) { return false } switch strings.ToLower(strings.TrimSpace(repo.Provider)) { @@ -245,11 +290,11 @@ func isPluginManagedRepository(repo *models.Repository) bool { } } -func (e *Executor) refreshPluginRepositoryForSession( - ctx context.Context, taskID, sessionID string, repo *models.Repository, +func (e *Executor) refreshManagedRepositoryForSession( + ctx context.Context, taskID, sessionID string, repo *models.Repository, prNumber int, checkoutBranch string, ) error { if e.repoCloner == nil || repo.LocalPath == "" { - return errors.New("plugin repository refresh is unavailable") + return errors.New("managed repository refresh is unavailable") } cloneURL := repositoryCloneURL(repo) if cloneURL == "" { @@ -261,10 +306,49 @@ func (e *Executor) refreshPluginRepositoryForSession( return ErrNoCloneURL } } + credentialOrigin, token := "", "" + provider := strings.ToLower(strings.TrimSpace(repo.Provider)) + if provider == gitLabProviderID && e.gitlabCredentials != nil { + var err error + credentialOrigin, token, err = e.gitlabCredentials.ResolveGitLabExecutionCredentials(ctx, repo.WorkspaceID) + if err != nil { + return fmt.Errorf("resolve GitLab refresh credentials: %w", err) + } + } + if provider == providerAzureDevOps && strings.HasPrefix(strings.ToLower(cloneURL), "https://") { + return e.refreshAzureDevOpsRepositoryForSession( + ctx, repo, cloneURL, + ) + } + request := repositoryGitCredentialRequest(taskID, sessionID, repo, cloneURL) + if isGitHubRepository(repo) { + request.PRNumber = prNumber + request.CheckoutBranch = checkoutBranch + } if err := e.repoCloner.RefreshWorkspaceRepositoryWithCredentialRequest( - ctx, repositoryGitCredentialRequest(taskID, sessionID, repo, cloneURL), repo.LocalPath, "", "", + ctx, request, repo.LocalPath, credentialOrigin, token, ); err != nil { - return fmt.Errorf("refresh plugin repository before worktree: %w", err) + return fmt.Errorf("refresh repository before worktree: %w", err) + } + return nil +} + +func (e *Executor) refreshAzureDevOpsRepositoryForSession( + ctx context.Context, repo *models.Repository, cloneURL string, +) error { + authCloner, ok := e.repoCloner.(strictAuthenticatedRepoCloner) + if !ok || e.secretStore == nil { + return errors.New("azure DevOps repository refresh authentication is unavailable") + } + pat, err := e.secretStore.Reveal(ctx, cloneauth.AzureDevOpsPATKey(repo.WorkspaceID)) + if err != nil { + return fmt.Errorf("read Azure DevOps refresh credential: %w", err) + } + if err := authCloner.RefreshWorkspaceRepositoryWithBasicAuth( + ctx, repo.WorkspaceID, repo.Provider, repo.ProviderHost, cloneURL, + repo.ProviderOwner, repo.ProviderName, repo.LocalPath, "kandev", pat, + ); err != nil { + return fmt.Errorf("refresh Azure DevOps repository before worktree: %w", err) } return nil } @@ -286,7 +370,7 @@ func (e *Executor) ensureRepoLocalPath(ctx context.Context, repo *models.Reposit func (e *Executor) ensureRepoLocalPathForSession( ctx context.Context, taskID, sessionID string, repo *models.Repository, ) error { - if repo.SourceType == sourceTypeLocal || repo.ProviderOwner == "" || repo.ProviderName == "" { + if repo.SourceType == sourceTypeLocal || !hasProviderRepositoryIdentity(repo) { return nil } if repo.LocalPath != "" && isLocalGitRepo(repo.LocalPath) && @@ -949,6 +1033,7 @@ func (e *Executor) buildResumeRequestAtCredentialBoundary( if len(allRepos) > 0 { req.PullBeforeWorktree = allRepos[0].PullBeforeWorktree req.RemoteSyncHandled = allRepos[0].RemoteSyncHandled + req.RefreshRepository = allRepos[0].RefreshRepository } e.reuseExistingEnvironment(ctx, req, existingEnv) diff --git a/apps/backend/internal/orchestrator/executor/executor_resume_clone_transport_test.go b/apps/backend/internal/orchestrator/executor/executor_resume_clone_transport_test.go index af4e133668..404b251d52 100644 --- a/apps/backend/internal/orchestrator/executor/executor_resume_clone_transport_test.go +++ b/apps/backend/internal/orchestrator/executor/executor_resume_clone_transport_test.go @@ -223,7 +223,7 @@ func TestEnsureRepoClonedUsesConfiguredProtocolForLegacyRepository(t *testing.T) } } -func TestResolveTaskRepoInfoAuthenticatesPluginRefreshBeforeWorktree(t *testing.T) { +func TestResolveTaskRepoInfoDefersPluginRefreshUntilWorktreeMaterialization(t *testing.T) { repoPath := initGitRepoWithOrigin(t, "https://bitbucket.org/acme/widgets.git") repositoryStore := newMockRepository() repositoryStore.repositories["repo-1"] = &models.Repository{ @@ -243,8 +243,15 @@ func TestResolveTaskRepoInfoAuthenticatesPluginRefreshBeforeWorktree(t *testing. if err != nil { t.Fatalf("resolveTaskRepoInfoForSession(): %v", err) } - if cloner.refreshCalls != 1 || !info.RemoteSyncHandled { - t.Fatalf("refresh calls = %d, remote sync handled = %v", cloner.refreshCalls, info.RemoteSyncHandled) + if cloner.refreshCalls != 0 || info.RemoteSyncHandled || !info.PullBeforeWorktree || info.RefreshRepository == nil { + t.Fatalf("deferred refresh state = calls %d, remote sync handled %v, pull before worktree %v, callback nil %v", + cloner.refreshCalls, info.RemoteSyncHandled, info.PullBeforeWorktree, info.RefreshRepository == nil) + } + if err := info.RefreshRepository(context.Background()); err != nil { + t.Fatalf("RefreshRepository(): %v", err) + } + if cloner.refreshCalls != 1 { + t.Fatalf("refresh calls after materialization = %d, want 1", cloner.refreshCalls) } if got := cloner.refreshRequest; got.TaskID != "task-1" || got.SessionID != "session-1" || got.RepositoryID != "repo-1" || got.CloneURL != "https://bitbucket.org/acme/widgets.git" { @@ -255,6 +262,178 @@ func TestResolveTaskRepoInfoAuthenticatesPluginRefreshBeforeWorktree(t *testing. } } +func TestResolveTaskRepoInfoManagedGitHubRefreshCarriesPRHead(t *testing.T) { + repoPath := initGitRepoWithOrigin(t, "https://github.com/acme/widgets.git") + repositoryStore := newMockRepository() + repositoryStore.repositories["repo-1"] = &models.Repository{ + ID: "repo-1", WorkspaceID: "workspace-1", SourceType: "provider", + Provider: "github", ProviderHost: "https://github.com", + ProviderOwner: "acme", ProviderName: "widgets", + RemoteURL: "https://github.com/acme/widgets.git", LocalPath: repoPath, + DefaultBranch: "main", PullBeforeWorktree: true, + } + cloner := &cloneTransportTestCloner{} + exec := newTestExecutor(t, &mockAgentManager{}, repositoryStore) + exec.SetRepoCloner(cloner, nil) + exec.SetTaskGitCredentialPolicyResolver(fakeTaskGitCredentialPolicyResolver{ + policy: TaskGitCredentialPolicy{Mode: taskGitCredentialsModeManaged}, + }) + + info, err := exec.resolveTaskRepoInfoForSession(context.Background(), "session-1", &models.TaskRepository{ + ID: "task-repo-1", TaskID: "task-1", RepositoryID: "repo-1", BaseBranch: "main", + CheckoutBranch: "feature/pr", Metadata: map[string]interface{}{"pr_number": 42}, + }) + if err != nil { + t.Fatalf("resolveTaskRepoInfoForSession(): %v", err) + } + if info.RefreshRepository == nil { + t.Fatal("resolveTaskRepoInfoForSession() returned no managed refresh callback") + } + if err := info.RefreshRepository(context.Background()); err != nil { + t.Fatalf("RefreshRepository(): %v", err) + } + if got := cloner.refreshRequest; got.CheckoutBranch != "feature/pr" || got.PRNumber != 42 { + t.Fatalf("refresh request PR identity = %q/%d, want feature/pr/42", got.CheckoutBranch, got.PRNumber) + } +} + +func TestResolveTaskRepoInfoFailsWhenManagedGitHubRefreshFails(t *testing.T) { + repoPath := initGitRepoWithOrigin(t, "https://github.com/acme/widgets.git") + repositoryStore := newMockRepository() + repositoryStore.repositories["repo-1"] = &models.Repository{ + ID: "repo-1", WorkspaceID: "workspace-1", SourceType: "provider", + Provider: "github", ProviderHost: "https://github.com", + ProviderOwner: "acme", ProviderName: "widgets", + RemoteURL: "https://github.com/acme/widgets.git", LocalPath: repoPath, + DefaultBranch: "main", PullBeforeWorktree: true, + } + cloner := &cloneTransportTestCloner{refreshErr: fmt.Errorf("refresh failed: authentication denied")} + exec := newTestExecutor(t, &mockAgentManager{}, repositoryStore) + exec.SetRepoCloner(cloner, nil) + exec.SetTaskGitCredentialPolicyResolver(fakeTaskGitCredentialPolicyResolver{ + policy: TaskGitCredentialPolicy{Mode: taskGitCredentialsModeManaged}, + }) + + info, err := exec.resolveTaskRepoInfoForSession(context.Background(), "session-1", &models.TaskRepository{ + ID: "task-repo-1", TaskID: "task-1", RepositoryID: "repo-1", BaseBranch: "main", + }) + if err != nil { + t.Fatalf("resolveTaskRepoInfoForSession() error = %v, want deferred callback", err) + } + if info.RefreshRepository == nil { + t.Fatal("resolveTaskRepoInfoForSession() returned no managed refresh callback") + } + err = info.RefreshRepository(context.Background()) + if err == nil || !strings.Contains(err.Error(), "authentication denied") { + t.Fatalf("RefreshRepository() error = %v, want managed refresh failure", err) + } +} + +func TestResolveTaskRepoInfoLeavesExecutorGitHubRefreshToWorktreeManager(t *testing.T) { + repoPath := initGitRepoWithOrigin(t, "git@github.com:acme/widgets.git") + repositoryStore := newMockRepository() + repositoryStore.repositories["repo-1"] = &models.Repository{ + ID: "repo-1", WorkspaceID: "workspace-1", SourceType: "provider", + Provider: "github", ProviderOwner: "acme", ProviderName: "widgets", + LocalPath: repoPath, DefaultBranch: "main", PullBeforeWorktree: true, + } + cloner := &cloneTransportTestCloner{ + cloneURL: "git@github.com:acme/widgets.git", refreshErr: fmt.Errorf("refresh must not run"), + } + exec := newTestExecutor(t, &mockAgentManager{}, repositoryStore) + exec.SetRepoCloner(cloner, nil) + exec.SetTaskGitCredentialPolicyResolver(fakeTaskGitCredentialPolicyResolver{ + policy: TaskGitCredentialPolicy{Mode: taskGitCredentialsModeExecutor}, + }) + + info, err := exec.resolveTaskRepoInfoForSession(context.Background(), "session-1", &models.TaskRepository{ + ID: "task-repo-1", TaskID: "task-1", RepositoryID: "repo-1", BaseBranch: "main", + }) + if err != nil { + t.Fatalf("resolveTaskRepoInfoForSession() error = %v", err) + } + if cloner.refreshCalls != 0 || info.RemoteSyncHandled || !info.PullBeforeWorktree { + t.Fatalf("executor route = refresh calls %d, remote sync handled %v, pull before worktree %v", + cloner.refreshCalls, info.RemoteSyncHandled, info.PullBeforeWorktree) + } +} + +func TestResolveTaskRepoInfoRefreshesManagedGitLabBeforeWorktree(t *testing.T) { + repoPath := initGitRepoWithOrigin(t, "https://gitlab.example/acme/widgets.git") + repositoryStore := newMockRepository() + repositoryStore.repositories["repo-1"] = &models.Repository{ + ID: "repo-1", WorkspaceID: "workspace-1", SourceType: "provider", + Provider: "gitlab", ProviderHost: "https://gitlab.example", + ProviderOwner: "acme", ProviderName: "widgets", + RemoteURL: "https://gitlab.example/acme/widgets.git", LocalPath: repoPath, + DefaultBranch: "main", PullBeforeWorktree: true, + } + cloner := &cloneTransportTestCloner{} + exec := newTestExecutor(t, &mockAgentManager{}, repositoryStore) + exec.SetRepoCloner(cloner, nil) + exec.SetGitLabCredentialResolver(&fakeGitLabCredentialResolver{byWorkspace: map[string]struct{ host, token string }{ + "workspace-1": {host: "https://gitlab.example", token: "gitlab-token"}, + }}) + + info, err := exec.resolveTaskRepoInfoForSession(context.Background(), "session-1", &models.TaskRepository{ + ID: "task-repo-1", TaskID: "task-1", RepositoryID: "repo-1", BaseBranch: "main", + }) + if err != nil { + t.Fatalf("resolveTaskRepoInfoForSession() error = %v", err) + } + if cloner.refreshCalls != 0 || info.RemoteSyncHandled || !info.PullBeforeWorktree || info.RefreshRepository == nil { + t.Fatalf("GitLab deferred route = refresh calls %d, remote sync handled %v, pull before worktree %v, callback nil %v", + cloner.refreshCalls, info.RemoteSyncHandled, info.PullBeforeWorktree, info.RefreshRepository == nil) + } + if err := info.RefreshRepository(context.Background()); err != nil { + t.Fatalf("GitLab RefreshRepository(): %v", err) + } + if cloner.refreshCalls != 1 { + t.Fatalf("GitLab refresh calls after materialization = %d, want 1", cloner.refreshCalls) + } + if cloner.refreshCredentialOrigin != "https://gitlab.example" || cloner.refreshToken != "gitlab-token" { + t.Fatalf("GitLab refresh credentials = %q/%q", cloner.refreshCredentialOrigin, cloner.refreshToken) + } +} + +func TestResolveTaskRepoInfoRefreshesAzureDevOpsBeforeWorktree(t *testing.T) { + repoPath := initGitRepoWithOrigin(t, "https://dev.azure.com/acme/Platform/_git/widgets") + repositoryStore := newMockRepository() + repositoryStore.repositories["repo-1"] = &models.Repository{ + ID: "repo-1", WorkspaceID: "workspace-1", SourceType: "provider", + Provider: providerAzureDevOps, ProviderHost: "https://dev.azure.com/acme", + ProviderOwner: "Platform", ProviderName: "widgets", + RemoteURL: "https://dev.azure.com/acme/Platform/_git/widgets", LocalPath: repoPath, + DefaultBranch: "main", PullBeforeWorktree: true, + } + cloner := &cloneTransportTestCloner{} + exec := newTestExecutor(t, &mockAgentManager{}, repositoryStore) + exec.SetRepoCloner(cloner, nil) + exec.secretStore = &mockSecretStore{secrets: map[string]string{ + "azure_devops:workspace-1:pat": "azure-token", + }} + + info, err := exec.resolveTaskRepoInfoForSession(context.Background(), "session-1", &models.TaskRepository{ + ID: "task-repo-1", TaskID: "task-1", RepositoryID: "repo-1", BaseBranch: "main", + }) + if err != nil { + t.Fatalf("resolveTaskRepoInfoForSession() error = %v", err) + } + if cloner.basicRefreshCalls != 0 || info.RemoteSyncHandled || !info.PullBeforeWorktree || info.RefreshRepository == nil { + t.Fatalf("Azure DevOps deferred route = basic refresh calls %d, remote sync handled %v, pull before worktree %v, callback nil %v", + cloner.basicRefreshCalls, info.RemoteSyncHandled, info.PullBeforeWorktree, info.RefreshRepository == nil) + } + if err := info.RefreshRepository(context.Background()); err != nil { + t.Fatalf("Azure DevOps RefreshRepository(): %v", err) + } + if cloner.basicRefreshCalls != 1 { + t.Fatalf("Azure DevOps refresh calls after materialization = %d, want 1", cloner.basicRefreshCalls) + } + if cloner.basicRefreshPassword != "azure-token" || cloner.basicRefreshUsername != "kandev" { + t.Fatalf("Azure DevOps refresh credentials = %q/%q", cloner.basicRefreshUsername, cloner.basicRefreshPassword) + } +} + func TestBuildResumeRequestPreparesEachAttachedRepositoryOnce(t *testing.T) { if _, err := exec.LookPath("git"); err != nil { t.Skip("git not available") @@ -312,16 +491,24 @@ func (u *localPathRecordingRepoUpdater) UpdateRepositoryDefaultBranch(context.Co } type cloneTransportTestCloner struct { - cloneURL string - requestedCloneURL string - returnPath string - setOriginErr error - refreshCalls int - refreshRequest repoclone.GitCredentialRequest - refreshPath string - setOriginPaths []string + cloneURL string + requestedCloneURL string + returnPath string + setOriginErr error + refreshCalls int + refreshErr error + refreshRequest repoclone.GitCredentialRequest + refreshPath string + refreshCredentialOrigin string + refreshToken string + basicRefreshCalls int + basicRefreshUsername string + basicRefreshPassword string + setOriginPaths []string } +var _ authenticatedRepoCloner = (*cloneTransportTestCloner)(nil) + func (c *cloneTransportTestCloner) EnsureWorkspaceClonedWithCredentialRequest( _ context.Context, request repoclone.GitCredentialRequest, _, _ string, ) (string, error) { @@ -330,12 +517,30 @@ func (c *cloneTransportTestCloner) EnsureWorkspaceClonedWithCredentialRequest( } func (c *cloneTransportTestCloner) RefreshWorkspaceRepositoryWithCredentialRequest( - _ context.Context, request repoclone.GitCredentialRequest, repositoryPath, _, _ string, + _ context.Context, request repoclone.GitCredentialRequest, repositoryPath, credentialOrigin, token string, ) error { c.refreshCalls++ c.refreshRequest = request c.refreshPath = repositoryPath - return nil + c.refreshCredentialOrigin = credentialOrigin + c.refreshToken = token + return c.refreshErr +} + +func (c *cloneTransportTestCloner) RefreshWorkspaceRepositoryWithBasicAuth( + _ context.Context, _, _, _, _, _, _, repositoryPath, username, password string, +) error { + c.basicRefreshCalls++ + c.refreshPath = repositoryPath + c.basicRefreshUsername = username + c.basicRefreshPassword = password + return c.refreshErr +} + +func (c *cloneTransportTestCloner) EnsureWorkspaceClonedWithBasicAuth( + _ context.Context, _, _, _, _, _, _, _, _ string, +) (string, error) { + return c.returnPath, nil } func (c *cloneTransportTestCloner) ShouldRecloneForWorkspace(string, string) bool { return false } diff --git a/apps/backend/internal/orchestrator/executor/launch_failure.go b/apps/backend/internal/orchestrator/executor/launch_failure.go index ed331396d0..eaded569a1 100644 --- a/apps/backend/internal/orchestrator/executor/launch_failure.go +++ b/apps/backend/internal/orchestrator/executor/launch_failure.go @@ -6,6 +6,7 @@ import ( "strings" "time" + "github.com/kandev/kandev/internal/agent/runtime/routingerr" "go.uber.org/zap" "github.com/kandev/kandev/internal/task/models" @@ -70,10 +71,15 @@ func (e *Executor) buildLastAgentError( } } occurredAt := time.Now().UTC() + details := "" + if launchErr != nil { + details = routingerr.Sanitize(launchErr.Error()) + } return models.LastAgentError{ Message: classification.message, OccurredAt: occurredAt, Code: classification.code, + Details: details, RecoveryActions: launchFailureRecoveryActions(taskRepositoryID, markReviewDone), TaskRepositoryID: taskRepositoryID, StampValue: models.StableLaunchErrorStamp( diff --git a/apps/backend/internal/repoclone/clone.go b/apps/backend/internal/repoclone/clone.go index a4c0743bb7..e6608eccac 100644 --- a/apps/backend/internal/repoclone/clone.go +++ b/apps/backend/internal/repoclone/clone.go @@ -84,6 +84,8 @@ type GitCredentialRequest struct { CloneURL string Owner string Name string + CheckoutBranch string + PRNumber int } // NewCloner creates a new Cloner with the given configuration. @@ -416,7 +418,34 @@ func (c *Cloner) RefreshWorkspaceRepositoryWithCredentialRequest( if err != nil { return err } + return c.refreshWorkspaceRepository(ctx, targetPath, cloneURL, auth, request.PRNumber) +} +// RefreshWorkspaceRepositoryWithBasicAuth strictly refreshes one existing +// workspace-managed checkout with the same basic-auth contract as cloning. +func (c *Cloner) RefreshWorkspaceRepositoryWithBasicAuth( + ctx context.Context, workspaceID, provider, providerHost, + cloneURL, owner, name, repositoryPath, username, password string, +) error { + targetPath, err := c.WorkspaceProviderRepoPath(workspaceID, provider, providerHost, owner, name) + if err != nil { + return err + } + if !sameFilesystemPath(targetPath, repositoryPath) { + return errors.New("repository path does not match the workspace checkout") + } + origin, err := gitCredentialOrigin(cloneURL) + if err != nil { + return err + } + return c.refreshWorkspaceRepository(ctx, targetPath, cloneURL, &cloneAuth{ + origin: origin, username: username, password: password, + }, 0) +} + +func (c *Cloner) refreshWorkspaceRepository( + ctx context.Context, targetPath, cloneURL string, auth *cloneAuth, prNumber int, +) error { mu := c.repoMu(targetPath) mu.Lock() defer mu.Unlock() @@ -426,15 +455,31 @@ func (c *Cloner) RefreshWorkspaceRepositoryWithCredentialRequest( if err := c.setOriginURLLocked(ctx, targetPath, cloneURL); err != nil { return err } - cmd := subproc.NewGitCommand(ctx, "-C", targetPath, "fetch", "--prune", "--force", gitNoTags, "origin") - cleanup, err := configureGitCommand(cmd, auth) - if err != nil { + runFetch := func(refspec string) error { + args := []string{"-C", targetPath, "fetch", "--prune", "--force", gitNoTags, "origin"} + if refspec != "" { + args = append(args, refspec) + } + cmd := subproc.NewGitCommand(ctx, args...) + cleanup, err := configureGitCommand(cmd, auth) + if err != nil { + return err + } + defer cleanup() + if out, runErr := subproc.RunGitCombinedOutputClass(ctx, subproc.GitLifecycle, cmd); runErr != nil { + return fmt.Errorf("refresh scoped workspace repository: %s: %w", + redactCloneOutput(string(out), authToken(auth)), runErr) + } + return nil + } + if err := runFetch(""); err != nil { return err } - defer cleanup() - if out, runErr := subproc.RunGitCombinedOutputClass(ctx, subproc.GitLifecycle, cmd); runErr != nil { - return fmt.Errorf("refresh scoped workspace repository: %s: %w", - redactCloneOutput(string(out), authToken(auth)), runErr) + if prNumber > 0 { + prRefspec := fmt.Sprintf("pull/%d/head:refs/remotes/origin/pr/%d", prNumber, prNumber) + if err := runFetch(prRefspec); err != nil { + return fmt.Errorf("refresh scoped workspace pull request: %w", err) + } } return nil } diff --git a/apps/backend/internal/repoclone/clone_auth_test.go b/apps/backend/internal/repoclone/clone_auth_test.go index 4a1affb40e..74606a2fca 100644 --- a/apps/backend/internal/repoclone/clone_auth_test.go +++ b/apps/backend/internal/repoclone/clone_auth_test.go @@ -133,7 +133,7 @@ func TestScopedWorkspaceRefreshUsesOnlyAuthenticatedOrigin(t *testing.T) { fakeGit := `#!/bin/sh if [ "$3" = "config" ]; then printf '%s\n' "https://bitbucket.org/acme/repository.git"; exit 0; fi if [ "$3" = "remote" ]; then exit 0; fi -printf '%s\n' "$@" > "$CAPTURE_PATH.args" + printf '%s\n' "$@" >> "$CAPTURE_PATH.args" printf '%s\n' "$GIT_CONFIG_KEY_1" > "$CAPTURE_PATH.scope" helper=${GIT_CONFIG_VALUE_1#!} "$helper" get > "$CAPTURE_PATH.helper" @@ -152,6 +152,7 @@ helper=${GIT_CONFIG_VALUE_1#!} WorkspaceID: "workspace-a", TaskID: "task-a", SessionID: "session-a", RepositoryID: "repo-a", Provider: "bitbucket", ProviderHost: "https://bitbucket.org", CloneURL: "https://bitbucket.org/acme/repository.git", Owner: "acme", Name: "repository", + CheckoutBranch: "feature/pr", PRNumber: 42, } repositoryPath, err := cloner.WorkspaceProviderRepoPath( request.WorkspaceID, request.Provider, request.ProviderHost, request.Owner, request.Name, @@ -172,7 +173,8 @@ helper=${GIT_CONFIG_VALUE_1#!} if err != nil { t.Fatal(err) } - if got := string(args); !strings.Contains(got, "fetch\n--prune\n--force\n--no-tags\norigin\n") || strings.Contains(got, "--all") { + if got := string(args); !strings.Contains(got, "fetch\n--prune\n--force\n--no-tags\norigin\n") || + !strings.Contains(got, "pull/42/head:refs/remotes/origin/pr/42\n") || strings.Contains(got, "--all") { t.Fatalf("refresh args = %q", got) } scope, err := os.ReadFile(capturePath + ".scope") @@ -195,6 +197,78 @@ helper=${GIT_CONFIG_VALUE_1#!} } } +func TestScopedWorkspaceBasicAuthRefreshUsesExactCheckout(t *testing.T) { + root := t.TempDir() + binDir := filepath.Join(root, "bin") + if err := os.MkdirAll(binDir, 0o755); err != nil { + t.Fatal(err) + } + capturePath := filepath.Join(root, "git-basic-refresh") + fakeGit := `#!/bin/sh +if [ "$3" = "config" ]; then printf '%s\n' "https://dev.azure.com/acme/p/_git/repository"; exit 0; fi +if [ "$3" = "remote" ]; then exit 0; fi +printf '%s\n' "$@" > "$CAPTURE_PATH.args" +printf '%s\n' "$GIT_CONFIG_KEY_1" > "$CAPTURE_PATH.scope" +helper=${GIT_CONFIG_VALUE_1#!} +"$helper" get > "$CAPTURE_PATH.helper" +` + if err := os.WriteFile(filepath.Join(binDir, "git"), []byte(fakeGit), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", binDir+string(os.PathListSeparator)+os.Getenv("PATH")) + t.Setenv("CAPTURE_PATH", capturePath) + + basePath := filepath.Join(root, "repos") + cloner := NewCloner(Config{BasePath: basePath}, ProtocolHTTPS, "", logger.Default()) + repositoryPath, err := cloner.WorkspaceProviderRepoPath( + "workspace-a", "azure_devops", "https://dev.azure.com/acme", "p", "repository", + ) + if err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Join(repositoryPath, ".git"), 0o755); err != nil { + t.Fatal(err) + } + + type basicAuthRefresher interface { + RefreshWorkspaceRepositoryWithBasicAuth( + context.Context, string, string, string, string, string, string, string, string, string, + ) error + } + refresher, ok := any(cloner).(basicAuthRefresher) + if !ok { + t.Fatal("Cloner does not expose strict basic-auth refresh") + } + if err := refresher.RefreshWorkspaceRepositoryWithBasicAuth( + context.Background(), "workspace-a", "azure_devops", "https://dev.azure.com/acme", + "https://dev.azure.com/acme/p/_git/repository", "p", "repository", repositoryPath, + "kandev", "secret-pat", + ); err != nil { + t.Fatalf("RefreshWorkspaceRepositoryWithBasicAuth(): %v", err) + } + args, err := os.ReadFile(capturePath + ".args") + if err != nil { + t.Fatal(err) + } + if got := string(args); !strings.Contains(got, "fetch\n--prune\n--force\n--no-tags\norigin\n") { + t.Fatalf("refresh args = %q", got) + } + scope, err := os.ReadFile(capturePath + ".scope") + if err != nil { + t.Fatal(err) + } + if string(scope) != "credential.https://dev.azure.com.helper\n" { + t.Fatalf("credential scope = %q", scope) + } + helper, err := os.ReadFile(capturePath + ".helper") + if err != nil { + t.Fatal(err) + } + if string(helper) != "username=kandev\npassword=secret-pat\n" { + t.Fatalf("helper output = %q", helper) + } +} + // canonicalTempDir returns t.TempDir() with symlinks resolved so the Git child // and the assertions agree on paths (macOS /var -> /private/var). func canonicalTempDir(t *testing.T) string { diff --git a/apps/backend/internal/worktree/errors.go b/apps/backend/internal/worktree/errors.go index 3f1a2f73e9..abdab7fa40 100644 --- a/apps/backend/internal/worktree/errors.go +++ b/apps/backend/internal/worktree/errors.go @@ -136,6 +136,16 @@ func isRemoteRefMissingError(err error) bool { strings.Contains(msg, "remote head refers to nonexistent ref") } +// isRemoteBranchMissingError reports the Git fetch result for a branch that +// does not exist on an otherwise usable remote. Transport and authentication +// failures must remain generic so callers do not mistake an unavailable +// remote for a deleted branch. +func isRemoteBranchMissingError(output string) bool { + lowerOutput := strings.ToLower(output) + return strings.Contains(lowerOutput, "couldn't find remote ref") || + (strings.Contains(lowerOutput, "remote ref") && strings.Contains(lowerOutput, "not found")) +} + // ClassifyGitError wraps a raw git error with a user-friendly sentinel error // based on the command output. func ClassifyGitError(output string, _ error) error { diff --git a/apps/backend/internal/worktree/manager_checkout_branch_test.go b/apps/backend/internal/worktree/manager_checkout_branch_test.go index 657e926272..b6c188875c 100644 --- a/apps/backend/internal/worktree/manager_checkout_branch_test.go +++ b/apps/backend/internal/worktree/manager_checkout_branch_test.go @@ -2,6 +2,7 @@ package worktree import ( "context" + "fmt" "os" "os/exec" "path/filepath" @@ -226,6 +227,111 @@ func TestCreateWorktree_UsesAlreadyRefreshedRemoteWithoutNetwork(t *testing.T) { } } +func TestCreateWorktree_ManagedRefreshRunsOnlyForMaterialization(t *testing.T) { + mgr, err := NewManager(newTestConfig(t), newMockStore(), newTestLogger()) + if err != nil { + t.Fatalf("NewManager failed: %v", err) + } + repoPath := initGitRepoWithRemote(t) + refreshCalls := 0 + req := CreateRequest{ + TaskID: "task-refresh", SessionID: "session-refresh", RepositoryID: "repo-refresh", + RepositoryPath: repoPath, BaseBranch: "main", PullBeforeWorktree: true, + TaskDirName: "task-refresh", RepoName: "repo-refresh", + RefreshRepository: func(context.Context) error { + refreshCalls++ + return nil + }, + } + if _, err := mgr.Create(context.Background(), req); err != nil { + t.Fatalf("first Create(): %v", err) + } + if refreshCalls != 1 { + t.Fatalf("refresh calls after materialization = %d, want 1", refreshCalls) + } + if _, err := mgr.Create(context.Background(), req); err != nil { + t.Fatalf("reusing Create(): %v", err) + } + if refreshCalls != 1 { + t.Fatalf("refresh calls after valid reuse = %d, want 1", refreshCalls) + } +} + +func TestCreateWorktree_ManagedRefreshUsesRefreshedPRHead(t *testing.T) { + repoPath, wantSHA := initGitRepoWithPullRef(t, 974, "feature/fork-pr") + runGit(t, repoPath, "fetch", "origin", "pull/974/head:refs/remotes/origin/pr/974") + runGit(t, repoPath, "remote", "set-url", "origin", "https://127.0.0.1:1/never.git") + + mgr, err := NewManager(newTestConfig(t), newMockStore(), newTestLogger()) + if err != nil { + t.Fatalf("NewManager failed: %v", err) + } + refreshCalls := 0 + wt, err := mgr.Create(context.Background(), CreateRequest{ + TaskID: "task-pr-refresh", SessionID: "session-pr-refresh", RepositoryID: "repo-pr-refresh", + RepositoryPath: repoPath, BaseBranch: "main", CheckoutBranch: "feature/fork-pr", PRNumber: 974, + PullBeforeWorktree: true, TaskDirName: "task-pr-refresh", RepoName: "repo-pr-refresh", + RefreshRepository: func(context.Context) error { + refreshCalls++ + return nil + }, + }) + if err != nil { + t.Fatalf("Create(): %v", err) + } + if refreshCalls != 1 { + t.Fatalf("refresh calls = %d, want 1", refreshCalls) + } + if got := strings.TrimSpace(runGit(t, wt.Path, "rev-parse", "HEAD")); got != wantSHA { + t.Fatalf("worktree HEAD = %q, want refreshed PR head %q", got, wantSHA) + } +} + +func TestCreateWorktree_ManagedRefreshUsesRemoteWhenLocalCheckoutBranchIsBehind(t *testing.T) { + repoPath := initGitRepoWithRemote(t) + wantSHA := advanceRemoteBranch(t, repoPath, "feature/pr-branch") + runGit(t, repoPath, "remote", "set-url", "origin", "https://127.0.0.1:1/never.git") + + mgr, err := NewManager(newTestConfig(t), newMockStore(), newTestLogger()) + if err != nil { + t.Fatalf("NewManager failed: %v", err) + } + wt, err := mgr.Create(context.Background(), CreateRequest{ + TaskID: "task-managed-branch", SessionID: "session-managed-branch", RepositoryID: "repo-managed-branch", + RepositoryPath: repoPath, BaseBranch: "main", CheckoutBranch: "feature/pr-branch", + PullBeforeWorktree: true, TaskDirName: "task-managed-branch", RepoName: "repo-managed-branch", + RefreshRepository: func(context.Context) error { return nil }, + }) + if err != nil { + t.Fatalf("Create(): %v", err) + } + if got := strings.TrimSpace(runGit(t, wt.Path, "rev-parse", "HEAD")); got != wantSHA { + t.Fatalf("worktree HEAD = %q, want refreshed branch head %q", got, wantSHA) + } +} + +func TestCreateWorktree_ManagedRefreshUsesRemotePRHeadWhenLocalCheckoutBranchIsBehind(t *testing.T) { + repoPath, wantSHA := initManagedPRCheckoutBranch(t, 976, "feature/managed-fork-pr") + runGit(t, repoPath, "remote", "set-url", "origin", "https://127.0.0.1:1/never.git") + + mgr, err := NewManager(newTestConfig(t), newMockStore(), newTestLogger()) + if err != nil { + t.Fatalf("NewManager failed: %v", err) + } + wt, err := mgr.Create(context.Background(), CreateRequest{ + TaskID: "task-managed-pr-behind", SessionID: "session-managed-pr-behind", RepositoryID: "repo-managed-pr-behind", + RepositoryPath: repoPath, BaseBranch: "main", CheckoutBranch: "feature/managed-fork-pr", PRNumber: 976, + PullBeforeWorktree: true, TaskDirName: "task-managed-pr-behind", RepoName: "repo-managed-pr-behind", + RefreshRepository: func(context.Context) error { return nil }, + }) + if err != nil { + t.Fatalf("Create(): %v", err) + } + if got := strings.TrimSpace(runGit(t, wt.Path, "rev-parse", "HEAD")); got != wantSHA { + t.Fatalf("worktree HEAD = %q, want refreshed PR head %q", got, wantSHA) + } +} + // TestCreateWorktree_RemoteBaseRefDoesNotSetUpstream verifies that when a worktree // is created with a remote-tracking ref as the base (e.g. origin/feature/foo), // the new branch does NOT inherit upstream tracking from that ref. @@ -318,6 +424,47 @@ func initGitRepoWithRemote(t *testing.T) string { return cloneDir } +func advanceRemoteBranch(t *testing.T, repoPath, branch string) string { + t.Helper() + originURL := strings.TrimSpace(runGit(t, repoPath, "remote", "get-url", "origin")) + remoteClone := filepath.Join(t.TempDir(), "remote-clone") + runGit(t, filepath.Dir(remoteClone), "clone", originURL, remoteClone) + runGit(t, remoteClone, "config", "user.email", "remote@example.com") + runGit(t, remoteClone, "config", "user.name", "Remote User") + runGit(t, remoteClone, "config", "commit.gpgsign", "false") + runGit(t, remoteClone, "checkout", branch) + writeRepoFile(t, remoteClone, "remote-refresh.txt", "refreshed\n") + runGit(t, remoteClone, "add", "remote-refresh.txt") + runGit(t, remoteClone, "commit", "-m", "refreshed branch commit") + wantSHA := strings.TrimSpace(runGit(t, remoteClone, "rev-parse", "HEAD")) + runGit(t, remoteClone, "push", "origin", branch) + runGit(t, repoPath, "fetch", "origin", branch) + return wantSHA +} + +func initManagedPRCheckoutBranch(t *testing.T, prNumber int, branch string) (string, string) { + t.Helper() + repoPath, _ := initGitRepoWithPullRef(t, prNumber, branch) + prRef := fmt.Sprintf("pull/%d/head:refs/remotes/origin/pr/%d", prNumber, prNumber) + runGit(t, repoPath, "fetch", "origin", prRef) + runGit(t, repoPath, "branch", branch, fmt.Sprintf("origin/pr/%d", prNumber)) + originURL := strings.TrimSpace(runGit(t, repoPath, "remote", "get-url", "origin")) + remoteClone := filepath.Join(t.TempDir(), "remote-pr-clone") + runGit(t, filepath.Dir(remoteClone), "clone", originURL, remoteClone) + runGit(t, remoteClone, "config", "user.email", "remote@example.com") + runGit(t, remoteClone, "config", "user.name", "Remote User") + runGit(t, remoteClone, "config", "commit.gpgsign", "false") + runGit(t, remoteClone, "fetch", "origin", prRef) + runGit(t, remoteClone, "checkout", "-b", branch, fmt.Sprintf("origin/pr/%d", prNumber)) + writeRepoFile(t, remoteClone, "remote-pr-refresh.txt", "refreshed PR\n") + runGit(t, remoteClone, "add", "remote-pr-refresh.txt") + runGit(t, remoteClone, "commit", "-m", "refreshed PR commit") + wantSHA := strings.TrimSpace(runGit(t, remoteClone, "rev-parse", "HEAD")) + runGit(t, remoteClone, "push", "origin", "HEAD:"+pullHeadRef(prNumber)) + runGit(t, repoPath, "fetch", "origin", prRef) + return repoPath, wantSHA +} + func initGitRepoForWorktreeTest(t *testing.T) string { t.Helper() diff --git a/apps/backend/internal/worktree/manager_git.go b/apps/backend/internal/worktree/manager_git.go index b910c98510..0cb346811e 100644 --- a/apps/backend/internal/worktree/manager_git.go +++ b/apps/backend/internal/worktree/manager_git.go @@ -111,46 +111,67 @@ func (m *Manager) checkoutBranchExistsAnywhere(ctx context.Context, repoPath, br return remote } -func (m *Manager) preferRefreshedRemoteRef(ctx context.Context, repoPath, branch string) string { +func (m *Manager) preferRefreshedRemoteRef(ctx context.Context, repoPath, branch string) (string, error) { localBranch := strings.TrimPrefix(branch, "origin/") if localBranch == "" { - return branch + return branch, nil } remoteRef := "origin/" + localBranch - if exists, err := m.branchExists(ctx, repoPath, remoteRef); err == nil && exists { - return remoteRef - } - return branch + return m.selectContainingRef(ctx, repoPath, branch, remoteRef) } -// prepareCheckoutFromRefreshedOrigin makes a refreshed remote branch available -// as a local branch without contacting origin. It returns false when neither a -// local nor remote-tracking ref exists, preserving the existing "create a new -// branch from base" behavior. +// prepareCheckoutFromRefreshedOrigin verifies a refreshed remote branch and +// returns whether a usable local or remote ref exists without contacting +// origin. It returns false when neither ref exists, preserving the existing +// "create a new branch from base" behavior. func (m *Manager) prepareCheckoutFromRefreshedOrigin(ctx context.Context, repoPath, branch string) (bool, error) { - if exists, err := m.branchExists(ctx, repoPath, branch); err != nil { - return false, err - } else if exists { - return true, nil + selected, err := m.prepareBranchFromRefreshedOrigin(ctx, repoPath, branch, branch, 0) + return selected != "", err +} + +// prepareBranchFromRefreshedOrigin selects a provider-refreshed source branch +// without contacting origin. A PR number selects the dedicated origin/pr/ +// ref, which is also how fork PR heads are kept available after the +// authenticated refresh. When both refs exist, the selected ref is the one +// that contains the other. A local-only ref is preserved, a refreshed remote +// ref is returned to the caller as the worktree start point, and divergence or +// an unverified relationship fails closed. +func (m *Manager) prepareBranchFromRefreshedOrigin( + ctx context.Context, repoPath, localBranch, sourceBranch string, prNumber int, +) (string, error) { + remoteRef := "origin/" + sourceBranch + if prNumber > 0 { + remoteRef = fmt.Sprintf("origin/pr/%d", prNumber) } - remoteRef := "origin/" + branch - if exists, err := m.branchExists(ctx, repoPath, remoteRef); err != nil { - return false, err - } else if !exists { - return false, nil + localExists, err := m.branchExists(ctx, repoPath, localBranch) + if err != nil { + return "", err + } + remoteExists, err := m.branchExists(ctx, repoPath, remoteRef) + if err != nil { + return "", err + } + if !localExists && !remoteExists { + return "", nil + } + if !remoteExists { + return "", fmt.Errorf("required fetched remote ref %q is missing", remoteRef) + } + if !localExists { + release, err := subproc.AcquireGit(ctx, subproc.GitLifecycle) + if err != nil { + return "", err + } + defer release() + cmd := m.newNonInteractiveGitCmd(ctx, repoPath, "branch", "--track", localBranch, remoteRef) + if output, runErr := cmd.CombinedOutput(); runErr != nil { + return "", fmt.Errorf("create local branch %q from refreshed origin: %s: %w", + localBranch, strings.TrimSpace(string(output)), runErr) + } + return localBranch, nil } - release, err := subproc.Git().Acquire(ctx) - if err != nil { - return false, err - } - defer release() - cmd := m.newNonInteractiveGitCmd(ctx, repoPath, "branch", "--track", branch, remoteRef) - if output, runErr := cmd.CombinedOutput(); runErr != nil { - return false, fmt.Errorf("create local branch %q from refreshed origin: %s: %w", - branch, strings.TrimSpace(string(output)), runErr) - } - return true, nil + return m.selectContainingRef(ctx, repoPath, localBranch, remoteRef) } // Branch recovery statuses reported by BranchRecoveryStatus. @@ -182,13 +203,9 @@ func (m *Manager) BranchRecoveryStatus(ctx context.Context, repoPath, branch str // refContains reports whether container already includes every commit in // contained, i.e. `git merge-base --is-ancestor contained container`. -// -// Used to decide whether resolving a worktree start point to one ref can lose -// commits that only exist on the other. A probe that cannot answer (throttle -// wait, timeout, missing ref) returns false, and callers must treat that as -// "cannot rule out loss": being slightly stale is recoverable, silently -// dropping the user's unpushed commits is not. -func (m *Manager) refContains(ctx context.Context, repoPath, container, contained string) bool { +// A non-zero ancestry result is distinct from a failed probe: the former is a +// proven negative, while the latter must stop required refresh preparation. +func (m *Manager) refContains(ctx context.Context, repoPath, container, contained string) (bool, error) { // Same Acquire-then-build-execCtx ordering as branchExists. release, err := subproc.AcquireGit(ctx, subproc.GitLifecycle) if err != nil { @@ -197,13 +214,24 @@ func (m *Manager) refContains(ctx context.Context, repoPath, container, containe zap.String("container", container), zap.String("contained", contained), zap.Error(err)) - return false + return false, fmt.Errorf("acquire Git ancestry check: %w", err) } defer release() inspectCtx, cancel := context.WithTimeout(ctx, m.inspectTimeout) defer cancel() cmd := m.newNonInteractiveGitCmd(inspectCtx, repoPath, "merge-base", "--is-ancestor", contained, container) - return cmd.Run() == nil + runErr := cmd.Run() + if ctxErr := inspectCtx.Err(); ctxErr != nil { + return false, fmt.Errorf("git ancestry check timed out: %w", ctxErr) + } + if runErr == nil { + return true, nil + } + var exitErr *exec.ExitError + if errors.As(runErr, &exitErr) && exitErr.ExitCode() == 1 { + return false, nil + } + return false, fmt.Errorf("git ancestry check failed for %q and %q: %w", contained, container, runErr) } func (m *Manager) currentBranch(ctx context.Context, repoPath string) string { @@ -257,15 +285,17 @@ func classifyGitFallbackReason(cmdErr error, cmdOutput string, ctxErr error) str return "git_command_failed" } -// pullBaseBranch fetches the latest changes from origin and returns the best ref to use -// for creating a new worktree. The function handles three scenarios: +// pullBaseBranch fetches the latest changes from origin and returns the safest +// ref to use for creating a new worktree. The function handles three scenarios: // // 1. baseBranch is already a remote ref (e.g., "origin/main"): fetch and use it directly // 2. baseBranch is a local branch and we're currently on it: pull --ff-only to update // 3. baseBranch is a local branch but we're on a different branch: use origin/ instead // -// On fetch/pull failure, errors are logged but the function continues with the best available ref. -func (m *Manager) pullBaseBranch(ctx context.Context, repoPath, baseBranch string, onProgress SyncProgressCallback) string { +// A failed required fetch or an unprovable base relationship returns an error. +func (m *Manager) pullBaseBranch( + ctx context.Context, repoPath, baseBranch string, onProgress SyncProgressCallback, +) (string, error) { localBranch := strings.TrimPrefix(baseBranch, "origin/") isRemoteRef := localBranch != baseBranch stepName := "Sync base branch" @@ -287,13 +317,25 @@ func (m *Manager) pullBaseBranch(ctx context.Context, repoPath, baseBranch strin } output, err, execCtxErr := m.runGitCombinedAfterAcquire(ctx, m.fetchTimeout, repoPath, fetchArgs...) if err != nil { - return m.handleFetchFallback(baseBranch, stepName, onProgress, execCtxErr, output, err) + reason := classifyGitFallbackReason(err, string(output), execCtxErr) + return "", m.failRequiredSync( + stepName, baseBranch, onProgress, reason, + fmt.Errorf("required refresh of %q failed (%s): %w", baseBranch, reason, syncFailureCause(reason, err, execCtxErr)), + ) } if isRemoteRef { resolved := "origin/" + localBranch + if exists, branchErr := m.branchExists(ctx, repoPath, resolved); branchErr != nil { + return "", m.failRequiredSync(stepName, baseBranch, onProgress, "base_ref_unverified", branchErr) + } else if !exists { + return "", m.failRequiredSync( + stepName, baseBranch, onProgress, "missing_remote_ref", + fmt.Errorf("required fetched remote ref %q is missing", resolved), + ) + } m.reportSyncCompleted(stepName, onProgress, fmt.Sprintf("Synced and using %s", resolved), "") - return resolved + return resolved, nil } return m.resolveLocalBaseRef(ctx, repoPath, baseBranch, localBranch, stepName, onProgress) @@ -314,68 +356,110 @@ func (m *Manager) reportSyncCompleted(stepName string, onProgress SyncProgressCa }) } -func (m *Manager) handleFetchFallback(baseBranch, stepName string, onProgress SyncProgressCallback, ctxErr error, output []byte, cmdErr error) string { - reason := classifyGitFallbackReason(cmdErr, string(output), ctxErr) - m.logger.Warn("git fetch failed before worktree creation; continuing with fallback ref", - zap.String("branch", baseBranch), +func (m *Manager) failRequiredSync( + stepName, branch string, onProgress SyncProgressCallback, reason string, err error, +) error { + m.logger.Warn("required git refresh failed before worktree creation", + zap.String("branch", branch), zap.String("reason", reason), - zap.String("fallback_ref", baseBranch), - zap.String("output", string(output)), - zap.Error(cmdErr)) - m.reportSyncCompleted(stepName, onProgress, fmt.Sprintf("Fetch %s; using fallback ref %s", reason, baseBranch), string(output)) - return baseBranch + zap.Error(err)) + m.reportSyncProgress(onProgress, SyncProgressEvent{ + StepName: stepName, + Status: SyncProgressFailed, + Output: fmt.Sprintf("Required refresh failed for %s (%s)", branch, reason), + Error: reason, + }) + return err } func (m *Manager) resolveLocalBaseRef( ctx context.Context, repoPath, baseBranch, localBranch, stepName string, onProgress SyncProgressCallback, -) string { +) (string, error) { remoteRef := "origin/" + localBranch if m.currentBranch(ctx, repoPath) == baseBranch { return m.pullCurrentBranchOrFallback(ctx, repoPath, baseBranch, remoteRef, stepName, onProgress) } - // Best-effort sync: a timeout / error here is treated the same as a - // missing remote ref — caller falls back to the local baseBranch. - if exists, _ := m.branchExists(ctx, repoPath, remoteRef); exists { - m.reportSyncCompleted(stepName, onProgress, fmt.Sprintf("Synced and using %s", remoteRef), "") - return remoteRef + resolved, err := m.selectContainingRef(ctx, repoPath, baseBranch, remoteRef) + if err != nil { + return "", m.failRequiredSync(stepName, baseBranch, onProgress, "base_ref_unverified", err) } - m.reportSyncCompleted(stepName, onProgress, fmt.Sprintf("Remote ref not found; using %s", baseBranch), "") - return baseBranch + m.reportSyncCompleted(stepName, onProgress, fmt.Sprintf("Synced and using %s", resolved), "") + return resolved, nil } func (m *Manager) pullCurrentBranchOrFallback( ctx context.Context, repoPath, baseBranch, remoteRef, stepName string, onProgress SyncProgressCallback, -) string { +) (string, error) { // Same Acquire-then-build-execCtx ordering as the fetch path. output, err, execCtxErr := m.runGitCombinedAfterAcquire(ctx, m.pullTimeout, repoPath, "pull", "--ff-only", "origin", baseBranch) if err != nil { reason := classifyGitFallbackReason(err, string(output), execCtxErr) - // The preceding fetch succeeded, so origin/ holds the latest - // remote state and is normally the better start point. It is only - // better when it is *ahead* of the local branch, though: if the local - // branch carries commits origin does not have, starting from the - // remote ref silently drops them from the new worktree, and creation - // still succeeds so nothing surfaces the loss. Keep the local branch - // in that case, matching handleFetchFallback and the non-current-branch - // path in resolveLocalBaseRef, which both resolve to it on failure. - fallbackRef := remoteRef - if !m.refContains(ctx, repoPath, remoteRef, baseBranch) { - fallbackRef = baseBranch + resolved, selectErr := m.selectContainingRef(ctx, repoPath, baseBranch, remoteRef) + if selectErr != nil { + return "", m.failRequiredSync(stepName, baseBranch, onProgress, reason, selectErr) } - m.logger.Warn("git pull failed before worktree creation; continuing with fallback ref", - zap.String("branch", baseBranch), - zap.String("reason", reason), - zap.String("remote_ref", remoteRef), - zap.String("fallback_ref", fallbackRef), - zap.String("output", string(output)), - zap.Error(err)) - m.reportSyncCompleted(stepName, onProgress, fmt.Sprintf("Pull %s; using %s", reason, fallbackRef), string(output)) - return fallbackRef + m.reportSyncCompleted(stepName, onProgress, fmt.Sprintf("Pull %s; using %s", reason, resolved), "") + return resolved, nil } m.reportSyncCompleted(stepName, onProgress, fmt.Sprintf("Synced and using %s", baseBranch), "") - return baseBranch + return baseBranch, nil +} + +func (m *Manager) selectContainingRef( + ctx context.Context, repoPath, localRef, remoteRef string, +) (string, error) { + remoteExists, err := m.branchExists(ctx, repoPath, remoteRef) + if err != nil { + return "", fmt.Errorf("could not verify fetched remote ref %q: %w", remoteRef, err) + } + if !remoteExists { + return "", fmt.Errorf("required fetched remote ref %q is missing", remoteRef) + } + if localRef == remoteRef { + return remoteRef, nil + } + localExists, err := m.branchExists(ctx, repoPath, localRef) + if err != nil { + return "", fmt.Errorf("could not verify local base ref %q: %w", localRef, err) + } + if !localExists { + return remoteRef, nil + } + remoteContainsLocal, err := m.refContains(ctx, repoPath, remoteRef, localRef) + if err != nil { + return "", err + } + if remoteContainsLocal { + return remoteRef, nil + } + localContainsRemote, err := m.refContains(ctx, repoPath, localRef, remoteRef) + if err != nil { + return "", err + } + if localContainsRemote { + return localRef, nil + } + return "", fmt.Errorf("base refs %q and %q diverged", localRef, remoteRef) +} + +// syncFailureCause intentionally suppresses cmdErr because Git output can +// contain credentials. Callers expose only a bounded failure class and keep +// raw command output in internal logs where the existing redaction policy +// applies. +func syncFailureCause(reason string, _ error, contextErr error) error { + if contextErr != nil { + return contextErr + } + switch reason { + case "timeout": + return context.DeadlineExceeded + case "non_interactive_auth_failed": + return ErrAuthFailed + default: + return ErrGitCommandFailed + } } // runGitCombinedAfterAcquire acquires the backend git throttle slot, diff --git a/apps/backend/internal/worktree/manager_lifecycle.go b/apps/backend/internal/worktree/manager_lifecycle.go index ae334183be..9d368cb86b 100644 --- a/apps/backend/internal/worktree/manager_lifecycle.go +++ b/apps/backend/internal/worktree/manager_lifecycle.go @@ -121,6 +121,9 @@ func (m *Manager) Create(ctx context.Context, req CreateRequest) (*Worktree, err } return wt, err } + if err := m.refreshRepositoryForMaterialization(ctx, &req); err != nil { + return nil, err + } // Check repository is a git repo if !m.isGitRepo(req.RepositoryPath) { @@ -160,6 +163,22 @@ func (m *Manager) Create(ctx context.Context, req CreateRequest) (*Worktree, err return wt, nil } +// refreshRepositoryForMaterialization runs a provider-authenticated refresh +// only when Create is going to materialize or recreate a worktree. Reuse is +// checked by the caller first, so a valid existing worktree never refreshes +// origin or incurs provider authentication work. +func (m *Manager) refreshRepositoryForMaterialization(ctx context.Context, req *CreateRequest) error { + if req == nil || req.RemoteSyncHandled || req.RefreshRepository == nil { + return nil + } + if err := req.RefreshRepository(ctx); err != nil { + return err + } + req.RemoteSyncHandled = true + req.PullBeforeWorktree = false + return nil +} + // reuseRequiredWorktree resolves the exact canonical worktree for an // additional session. It intentionally performs no Git operation and does // not run contribution setup or repository scripts: another active session @@ -288,9 +307,12 @@ func requestBranchIdentitySlug(req CreateRequest) string { func (m *Manager) resolveBaseRefWithFallback(ctx context.Context, req *CreateRequest) (baseRef, warning, detail string, err error) { baseRef = req.BaseBranch if req.RemoteSyncHandled { - baseRef = m.preferRefreshedRemoteRef(ctx, req.RepositoryPath, req.BaseBranch) + baseRef, err = m.preferRefreshedRemoteRef(ctx, req.RepositoryPath, req.BaseBranch) } else if req.PullBeforeWorktree { - baseRef = m.pullBaseBranch(ctx, req.RepositoryPath, req.BaseBranch, req.OnSyncProgress) + baseRef, err = m.pullBaseBranch(ctx, req.RepositoryPath, req.BaseBranch, req.OnSyncProgress) + } + if err != nil { + return "", "", "", err } baseExists, baseErr := m.branchExists(ctx, req.RepositoryPath, baseRef) @@ -306,7 +328,10 @@ func (m *Manager) resolveBaseRefWithFallback(ctx context.Context, req *CreateReq fallback := strings.TrimSpace(req.FallbackBaseBranch) resolvedFallback := "" if fallback != "" && fallback != baseRef { - resolvedFallback = m.resolveFallbackRef(ctx, req, fallback) + resolvedFallback, err = m.resolveFallbackRef(ctx, req, fallback) + if err != nil { + return "", "", "", err + } fallbackExists, fallbackErr := m.branchExists(ctx, req.RepositoryPath, resolvedFallback) if fallbackErr != nil { return "", "", "", fmt.Errorf("could not verify fallback base branch %q: %w", resolvedFallback, fallbackErr) @@ -329,7 +354,10 @@ func (m *Manager) resolveBaseRefWithFallback(ctx context.Context, req *CreateReq if liveFallback == baseRef { return "", "", "", fmt.Errorf("%w: %s", ErrInvalidBaseBranch, baseRef) } - resolvedFallback = m.resolveFallbackRef(ctx, req, liveFallback) + resolvedFallback, err = m.resolveFallbackRef(ctx, req, liveFallback) + if err != nil { + return "", "", "", err + } fallbackExists, fallbackErr := m.branchExists(ctx, req.RepositoryPath, resolvedFallback) if fallbackErr != nil { return "", "", "", fmt.Errorf("could not verify live fallback base branch %q: %w", resolvedFallback, fallbackErr) @@ -341,14 +369,14 @@ func (m *Manager) resolveBaseRefWithFallback(ctx context.Context, req *CreateReq return m.finishBaseFallback(req, baseRef, fallback, resolvedFallback) } -func (m *Manager) resolveFallbackRef(ctx context.Context, req *CreateRequest, fallback string) string { +func (m *Manager) resolveFallbackRef(ctx context.Context, req *CreateRequest, fallback string) (string, error) { if req.RemoteSyncHandled { return m.preferRefreshedRemoteRef(ctx, req.RepositoryPath, fallback) } if req.PullBeforeWorktree { return m.pullBaseBranch(ctx, req.RepositoryPath, fallback, nil) } - return fallback + return fallback, nil } func (m *Manager) finishBaseFallback(req *CreateRequest, baseRef, fallback, resolvedFallback string) (string, string, string, error) { @@ -391,13 +419,24 @@ func (m *Manager) createInTaskDir(ctx context.Context, req CreateRequest, baseRe } if req.CheckoutBranch != "" { if req.RemoteSyncHandled { - prepared, prepareErr := m.prepareCheckoutFromRefreshedOrigin(ctx, req.RepositoryPath, req.CheckoutBranch) + selectedRef, prepareErr := m.prepareBranchFromRefreshedOrigin( + ctx, req.RepositoryPath, req.CheckoutBranch, req.CheckoutBranch, req.PRNumber, + ) if prepareErr != nil { return nil, prepareErr } - if !prepared { + if selectedRef == "" { + if req.PRNumber > 0 { + return nil, fmt.Errorf("%w: refreshed pull request head %d was not materialized", ErrBranchUnrecoverable, req.PRNumber) + } branchName = req.CheckoutBranch checkoutMode.CheckoutBranch = "" + } else { + // A refreshed remote that contains the old local checkout branch + // must be the actual worktree start point. Passing it only to the + // preparation probe would still let git worktree add check out the + // stale local branch. + startPoint = selectedRef } } else { // PRNumber != 0 means the caller wants the refs/pull//head ref; @@ -420,7 +459,9 @@ func (m *Manager) createInTaskDir(ctx context.Context, req CreateRequest, baseRe branchName = req.CheckoutBranch checkoutMode.CheckoutBranch = "" } else { - fetchResult, err = m.fetchBranchToLocal(ctx, req.RepositoryPath, req.CheckoutBranch, req.PRNumber) + fetchResult, err = m.fetchBranchToLocalWithPolicy( + ctx, req.RepositoryPath, req.CheckoutBranch, req.PRNumber, req.PullBeforeWorktree, + ) if err != nil { return nil, err } @@ -569,7 +610,13 @@ func (m *Manager) addWorktreeForBranch(ctx context.Context, req CreateRequest, w } // Try checking out the PR branch directly (common case: single task per PR). - id, err := m.gitAddWorktreeExisting(ctx, req.RepositoryPath, req.CheckoutBranch, worktreePath) + var id string + var err error + if req.RemoteSyncHandled && startPoint != req.CheckoutBranch { + id, err = m.gitAddWorktreeExistingAtRef(ctx, req.RepositoryPath, req.CheckoutBranch, worktreePath, startPoint) + } else { + id, err = m.gitAddWorktreeExisting(ctx, req.RepositoryPath, req.CheckoutBranch, worktreePath) + } if err == nil { m.setUpstreamIfExists(ctx, worktreePath, req.CheckoutBranch, req.CheckoutBranch) return id, req.CheckoutBranch, nil @@ -626,9 +673,16 @@ type FetchBranchResult struct { // the base repo, so this is the only way to materialize a fork PR's head // without adding the fork as a remote. func (m *Manager) fetchBranchToLocal(ctx context.Context, repoPath, branch string, prNumber int) (*FetchBranchResult, error) { + return m.fetchBranchToLocalWithPolicy(ctx, repoPath, branch, prNumber, false) +} + +func (m *Manager) fetchBranchToLocalWithPolicy( + ctx context.Context, repoPath, branch string, prNumber int, required bool, +) (*FetchBranchResult, error) { m.logger.Info("syncing checkout branch", zap.String("branch", branch), zap.Int("pr_number", prNumber), + zap.Bool("required", required), zap.String("repo_path", repoPath)) // Acquire the throttle slot FIRST, then build fetchCtx with the @@ -652,11 +706,31 @@ func (m *Manager) fetchBranchToLocal(ctx context.Context, repoPath, branch strin // the local ref. Retry by fetching only the remote-tracking ref (origin/branch), // which is always safe regardless of worktree state. if isFetchRefusedCheckedOut(outputStr) { - if result := m.retryFetchAsRemoteTrackingRef(ctx, repoPath, branch, prNumber); result != nil { + if result := m.retryFetchAsRemoteTrackingRef(ctx, repoPath, branch, prNumber, required); result != nil { + if required && result.Warning != "" { + return nil, fmt.Errorf( + "required refresh of checkout branch %q could not verify the refreshed ref: %w", + branch, ErrGitCommandFailed, + ) + } return result, nil } } + if required { + if isRemoteBranchMissingError(outputStr) { + return nil, fmt.Errorf( + "required refresh of checkout branch %q found no remote ref: %w", + branch, ErrInvalidBaseBranch, + ) + } + reason := classifyGitFallbackReason(err, outputStr, fetchCtxErr) + return nil, fmt.Errorf( + "required refresh of checkout branch %q failed (%s): %w", + branch, reason, syncFailureCause(reason, err, fetchCtxErr), + ) + } + m.logger.Warn("fetch from origin failed, checking local branch", zap.String("branch", branch), zap.String("output", outputStr), @@ -691,7 +765,9 @@ func (m *Manager) fetchBranchToLocal(ctx context.Context, repoPath, branch strin // Uses ctx (not the original fetchCtx) and a fresh m.fetchTimeout budget via // runGitCombinedAfterAcquire — the parent's fetchCtx is already consumed by // the first attempt, so reusing it would leave no room for the retry. -func (m *Manager) retryFetchAsRemoteTrackingRef(ctx context.Context, repoPath, branch string, prNumber int) *FetchBranchResult { +func (m *Manager) retryFetchAsRemoteTrackingRef( + ctx context.Context, repoPath, branch string, prNumber int, required bool, +) *FetchBranchResult { retryRef := branch if prNumber > 0 { retryRef = fmt.Sprintf("pull/%d/head", prNumber) @@ -702,6 +778,12 @@ func (m *Manager) retryFetchAsRemoteTrackingRef(ctx context.Context, repoPath, b m.logger.Info("fetched via remote-tracking ref (branch checked out elsewhere)", zap.String("branch", branch)) if prNumber > 0 { + if required { + // A fork PR has no origin/ ref. FETCH_HEAD is the only + // ref produced by the successful pull//head retry, and is + // therefore the safe start point for a required refresh. + return &FetchBranchResult{StartPoint: "FETCH_HEAD"} + } // Fork PRs have no origin/ ref — the bare // pull//head retry only updates FETCH_HEAD, so the local // `branch` (already populated by the prior worktree) is the @@ -730,7 +812,8 @@ func (m *Manager) retryFetchAsRemoteTrackingRef(ctx context.Context, repoPath, b // handleFetchFallback and the non-current-branch path in resolveLocalBaseRef. func (m *Manager) resolveRetryStartPoint(ctx context.Context, repoPath, branch string) *FetchBranchResult { remoteRef := "origin/" + branch - if m.refContains(ctx, repoPath, remoteRef, branch) { + remoteContainsLocal, remoteErr := m.refContains(ctx, repoPath, remoteRef, branch) + if remoteErr == nil && remoteContainsLocal { return &FetchBranchResult{StartPoint: remoteRef} } m.logger.Info("using local branch (remote-tracking ref does not contain all local commits)", @@ -745,9 +828,14 @@ func (m *Manager) resolveRetryStartPoint(ctx context.Context, repoPath, branch s // really may miss commits that are on origin, so the existing FetchWarning // surface has to say so rather than report a clean success. The wording // covers both, because this branch cannot tell them apart. - if m.refContains(ctx, repoPath, branch, remoteRef) { + localContainsRemote, localErr := m.refContains(ctx, repoPath, branch, remoteRef) + if localErr == nil && localContainsRemote { return &FetchBranchResult{} } + warningDetail := fmt.Sprintf("git merge-base --is-ancestor %s %s reported non-zero", branch, remoteRef) + if remoteErr != nil || localErr != nil { + warningDetail = "git merge-base ancestry could not be verified" + } return &FetchBranchResult{ Warning: fmt.Sprintf( "Could not confirm that %s contains every commit on local branch %q. Using the local version so unpushed commits are kept; it may not include the latest changes from origin.", @@ -755,7 +843,7 @@ func (m *Manager) resolveRetryStartPoint(ctx context.Context, repoPath, branch s // No git command failed here, so there is no raw output to carry: the // probe ran and answered "no" (or could not answer). Name the probe // rather than inventing a fake error string. - WarningDetail: fmt.Sprintf("git merge-base --is-ancestor %s %s reported non-zero", branch, remoteRef), + WarningDetail: warningDetail, } } @@ -764,24 +852,40 @@ func (m *Manager) resolveRetryStartPoint(ctx context.Context, repoPath, branch s // it automatically prunes and retries. If the repository uses git-crypt, it creates // the worktree without checkout, then unlocks git-crypt and performs the checkout. func (m *Manager) gitAddWorktreeExisting(ctx context.Context, repoPath, branchName, worktreePath string) (string, error) { + return m.gitAddWorktreeExistingAtRef(ctx, repoPath, branchName, worktreePath, "") +} + +// gitAddWorktreeExistingAtRef creates a worktree for an existing branch at a +// selected start point. A non-empty start point resets the branch with git's +// -B worktree option, which is safe here because the caller has already proved +// that the refreshed remote contains the old local branch. +func (m *Manager) gitAddWorktreeExistingAtRef(ctx context.Context, repoPath, branchName, worktreePath, startPoint string) (string, error) { release, err := acquireWorktreeTargetPath(ctx, worktreePath) if err != nil { return "", fmt.Errorf("acquire worktree target path: %w", err) } defer release() - return m.gitAddWorktreeExistingLocked(ctx, repoPath, branchName, worktreePath) + return m.gitAddWorktreeExistingLocked(ctx, repoPath, branchName, worktreePath, startPoint) } -func (m *Manager) gitAddWorktreeExistingLocked(ctx context.Context, repoPath, branchName, worktreePath string) (string, error) { +func (m *Manager) gitAddWorktreeExistingLocked(ctx context.Context, repoPath, branchName, worktreePath, startPoint string) (string, error) { worktreeID := uuid.New().String() usesGitCrypt := m.usesGitCrypt(repoPath) // Build worktree add command args := []string{"worktree", "add"} + if startPoint != "" { + args = append(args, "-B", branchName) + } if usesGitCrypt { args = append(args, "--no-checkout") } - args = append(args, worktreePath, branchName) + args = append(args, worktreePath) + if startPoint == "" { + args = append(args, branchName) + } else { + args = append(args, startPoint) + } cmd := newGitCommand(ctx, args...) cmd.Dir = repoPath @@ -804,7 +908,7 @@ func (m *Manager) gitAddWorktreeExistingLocked(ctx context.Context, repoPath, br if isGitCryptSmudgeError(outStr) && !usesGitCrypt { m.logger.Warn("git-crypt smudge error detected, retrying with --no-checkout", zap.String("output", outStr)) - return m.gitAddWorktreeExistingWithGitCrypt(ctx, repoPath, branchName, worktreePath) + return m.gitAddWorktreeExistingWithGitCrypt(ctx, repoPath, branchName, worktreePath, startPoint) } if !isBranchCheckedOutError(outStr) { @@ -820,18 +924,26 @@ func (m *Manager) gitAddWorktreeExistingLocked(ctx context.Context, repoPath, br } // Retry after pruning stale worktree - return m.retryWorktreeExisting(ctx, repoPath, branchName, worktreePath, usesGitCrypt) + return m.retryWorktreeExisting(ctx, repoPath, branchName, worktreePath, usesGitCrypt, startPoint) } // retryWorktreeExisting retries worktree creation after pruning stale worktrees. -func (m *Manager) retryWorktreeExisting(ctx context.Context, repoPath, branchName, worktreePath string, usesGitCrypt bool) (string, error) { +func (m *Manager) retryWorktreeExisting(ctx context.Context, repoPath, branchName, worktreePath string, usesGitCrypt bool, startPoint string) (string, error) { worktreeID := uuid.New().String() args := []string{"worktree", "add"} + if startPoint != "" { + args = append(args, "-B", branchName) + } if usesGitCrypt { args = append(args, "--no-checkout") } - args = append(args, worktreePath, branchName) + args = append(args, worktreePath) + if startPoint == "" { + args = append(args, branchName) + } else { + args = append(args, startPoint) + } retryCmd := newGitCommand(ctx, args...) retryCmd.Dir = repoPath @@ -861,10 +973,20 @@ func (m *Manager) retryWorktreeExisting(ctx context.Context, repoPath, branchNam // gitAddWorktreeExistingWithGitCrypt creates a worktree for an existing branch // using --no-checkout, then unlocks git-crypt. Used as fallback when smudge error detected. -func (m *Manager) gitAddWorktreeExistingWithGitCrypt(ctx context.Context, repoPath, branchName, worktreePath string) (string, error) { +func (m *Manager) gitAddWorktreeExistingWithGitCrypt(ctx context.Context, repoPath, branchName, worktreePath, startPoint string) (string, error) { worktreeID := uuid.New().String() - cmd := newGitCommand(ctx, "worktree", "add", "--no-checkout", worktreePath, branchName) + args := []string{"worktree", "add"} + if startPoint != "" { + args = append(args, "-B", branchName) + } + args = append(args, "--no-checkout", worktreePath) + if startPoint == "" { + args = append(args, branchName) + } else { + args = append(args, startPoint) + } + cmd := newGitCommand(ctx, args...) cmd.Dir = repoPath output, err := runGitCmdCombinedOutput(ctx, cmd) if err != nil { @@ -1187,22 +1309,30 @@ func (m *Manager) gitAddWorktreeWithGitCryptLocked(ctx context.Context, repoPath // for the recreate path, retrying with --no-checkout when a git-crypt smudge // error is detected. Returns the effective usesGitCrypt flag (forced to true // if the retry path was taken so the caller knows to unlock+checkout). -func (m *Manager) gitAddWorktreeForRecreate(ctx context.Context, repoPath, branch, worktreePath string) (bool, error) { +func (m *Manager) gitAddWorktreeForRecreate(ctx context.Context, repoPath, branch, worktreePath, startPoint string) (bool, error) { release, err := acquireWorktreeTargetPath(ctx, worktreePath) if err != nil { return false, fmt.Errorf("acquire worktree target path: %w", err) } defer release() - return m.gitAddWorktreeForRecreateLocked(ctx, repoPath, branch, worktreePath) + return m.gitAddWorktreeForRecreateLocked(ctx, repoPath, branch, worktreePath, startPoint) } -func (m *Manager) gitAddWorktreeForRecreateLocked(ctx context.Context, repoPath, branch, worktreePath string) (bool, error) { +func (m *Manager) gitAddWorktreeForRecreateLocked(ctx context.Context, repoPath, branch, worktreePath, startPoint string) (bool, error) { usesGitCrypt := m.usesGitCrypt(repoPath) args := []string{"worktree", "add"} + if startPoint != "" { + args = append(args, "-B", branch) + } if usesGitCrypt { args = append(args, "--no-checkout") } - args = append(args, worktreePath, branch) + args = append(args, worktreePath) + if startPoint == "" { + args = append(args, branch) + } else { + args = append(args, startPoint) + } cmd := newGitCommand(ctx, args...) cmd.Dir = repoPath @@ -1214,7 +1344,17 @@ func (m *Manager) gitAddWorktreeForRecreateLocked(ctx context.Context, repoPath, if isGitCryptSmudgeError(outStr) && !usesGitCrypt { m.logger.Warn("git-crypt smudge error during recreate, retrying with --no-checkout", zap.String("output", outStr)) - retryCmd := newGitCommand(ctx, "worktree", "add", "--no-checkout", worktreePath, branch) + retryArgs := []string{"worktree", "add"} + if startPoint != "" { + retryArgs = append(retryArgs, "-B", branch) + } + retryArgs = append(retryArgs, "--no-checkout", worktreePath) + if startPoint == "" { + retryArgs = append(retryArgs, branch) + } else { + retryArgs = append(retryArgs, startPoint) + } + retryCmd := newGitCommand(ctx, retryArgs...) retryCmd.Dir = repoPath if retryOutput, retryErr := runGitCmdCombinedOutput(ctx, retryCmd); retryErr != nil { m.logger.Error("failed to recreate worktree (--no-checkout)", @@ -1272,6 +1412,19 @@ func (m *Manager) copyConfiguredFiles(ctx context.Context, req CreateRequest, wt // recreate recreates a worktree from stored metadata. func (m *Manager) recreate(ctx context.Context, existing *Worktree, req CreateRequest) (*Worktree, error) { + if err := m.refreshRepositoryForMaterialization(ctx, &req); err != nil { + return nil, err + } + // Recreate bypasses the new-worktree path, so perform the same required + // base refresh before touching the existing worktree path. A failed refresh + // must leave the retryable on-disk state intact. + if req.PullBeforeWorktree && !req.RemoteSyncHandled { + refreshReq := req + if _, _, _, err := m.resolveBaseRefWithFallback(ctx, &refreshReq); err != nil { + return nil, err + } + } + // Clean up existing directory if present if existing.Path != "" { if err := os.RemoveAll(existing.Path); err != nil { @@ -1322,6 +1475,26 @@ func (m *Manager) recreate(ctx context.Context, existing *Worktree, req CreateRe return nil, materializeErr } } + refreshedStartPoint := "" + if req.RemoteSyncHandled && req.RemoteContribution == nil { + sourceBranch := existing.Branch + if req.CheckoutBranch != "" { + sourceBranch = req.CheckoutBranch + } + selectedRef, prepareErr := m.prepareBranchFromRefreshedOrigin( + ctx, req.RepositoryPath, existing.Branch, sourceBranch, req.PRNumber, + ) + if prepareErr != nil { + return nil, prepareErr + } + if selectedRef == "" { + return nil, fmt.Errorf("%w: refreshed branch %q was not materialized", ErrBranchUnrecoverable, sourceBranch) + } + if selectedRef != existing.Branch { + refreshedStartPoint = selectedRef + } + exists = true + } if !exists { if req.RemoteContribution != nil { branchCmd := m.newNonInteractiveGitCmd(ctx, req.RepositoryPath, "branch", existing.Branch, contributionRef) @@ -1329,7 +1502,10 @@ func (m *Manager) recreate(ctx context.Context, existing *Worktree, req CreateRe return nil, fmt.Errorf("restore contribution branch: %s: %w", strings.TrimSpace(string(output)), branchErr) } } else { - if _, fetchErr := m.fetchBranchToLocal(ctx, req.RepositoryPath, existing.Branch, req.PRNumber); fetchErr != nil { + if _, fetchErr := m.fetchBranchToLocalWithPolicy( + ctx, req.RepositoryPath, existing.Branch, req.PRNumber, + req.PullBeforeWorktree && !req.RemoteSyncHandled, + ); fetchErr != nil { m.logger.Warn("failed to restore worktree branch during recreate", zap.String("worktree_id", existing.ID), zap.String("branch", existing.Branch), @@ -1352,7 +1528,7 @@ func (m *Manager) recreate(ctx context.Context, existing *Worktree, req CreateRe } // Try to add worktree using existing branch - usesGitCrypt, err := m.gitAddWorktreeForRecreate(ctx, req.RepositoryPath, existing.Branch, worktreePath) + usesGitCrypt, err := m.gitAddWorktreeForRecreate(ctx, req.RepositoryPath, existing.Branch, worktreePath, refreshedStartPoint) if err != nil { return nil, err } diff --git a/apps/backend/internal/worktree/manager_pull_fallback_test.go b/apps/backend/internal/worktree/manager_pull_fallback_test.go index 2eef5dec43..18d2baf797 100644 --- a/apps/backend/internal/worktree/manager_pull_fallback_test.go +++ b/apps/backend/internal/worktree/manager_pull_fallback_test.go @@ -2,6 +2,7 @@ package worktree import ( "context" + "os" "path/filepath" "strings" "testing" @@ -83,7 +84,10 @@ func TestPullBaseBranch_PullFailureKeepsLocalCommits(t *testing.T) { mgr.pullTimeout = 0 var events []SyncProgressEvent - resolved := mgr.pullBaseBranch(context.Background(), repoPath, "main", captureSyncProgress(&events)) + resolved, err := mgr.pullBaseBranch(context.Background(), repoPath, "main", captureSyncProgress(&events)) + if err != nil { + t.Fatalf("pullBaseBranch() error = %v", err) + } if resolved == "" { t.Fatal("pullBaseBranch returned an empty ref") } @@ -115,6 +119,128 @@ func TestPullBaseBranch_PullFailureKeepsLocalCommits(t *testing.T) { } } +func TestResolveBaseRefWithFallback_RequiredRefreshFailureStopsPreparation(t *testing.T) { + repoPath, _ := initGitRepoWithOriginAheadLocal(t) + mgr, err := NewManager(newTestConfig(t), newMockStore(), newTestLogger()) + if err != nil { + t.Fatalf("NewManager failed: %v", err) + } + scriptDir := writeFakeGitScript(t, ` +case "${1:-}" in + fetch) + echo "fatal: Authentication failed" >&2 + exit 1 + ;; + *) + exit 0 + ;; +esac +`) + t.Setenv("PATH", scriptDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + req := CreateRequest{ + RepositoryPath: repoPath, BaseBranch: "main", PullBeforeWorktree: true, + } + var events []SyncProgressEvent + req.OnSyncProgress = captureSyncProgress(&events) + _, _, _, err = mgr.resolveBaseRefWithFallback(context.Background(), &req) + if err == nil { + t.Fatal("resolveBaseRefWithFallback() succeeded after required refresh failure") + } + if len(events) != 2 || events[0].Status != SyncProgressRunning || events[1].Status != SyncProgressFailed { + t.Fatalf("required refresh progress = %#v, want running then failed", events) + } + if events[1].Error == "" || strings.Contains(events[1].Error, "Authentication") { + t.Fatalf("required refresh progress exposed unsafe error detail: %#v", events[1]) + } +} + +func TestPreferRefreshedRemoteRefSelectsSafeContainingRef(t *testing.T) { + t.Run("local contains remote", func(t *testing.T) { + repoPath, _ := initGitRepoWithOriginAheadLocal(t) + mgr := newRecreateTestManager(t) + + resolved, err := mgr.preferRefreshedRemoteRef(context.Background(), repoPath, "main") + if err != nil { + t.Fatalf("preferRefreshedRemoteRef() error: %v", err) + } + if resolved != "main" { + t.Fatalf("resolved ref = %q, want local main", resolved) + } + }) + + t.Run("remote contains local", func(t *testing.T) { + repoPath := initGitRepoWithRemote(t) + otherPath := filepath.Join(t.TempDir(), "other") + originURL := strings.TrimSpace(runGit(t, repoPath, "remote", "get-url", "origin")) + runGit(t, filepath.Dir(otherPath), "clone", originURL, otherPath) + runGit(t, otherPath, "config", "user.email", "other@example.com") + runGit(t, otherPath, "config", "user.name", "Other User") + runGit(t, otherPath, "config", "commit.gpgsign", "false") + writeRepoFile(t, otherPath, "remote-only.txt", "pushed\n") + runGit(t, otherPath, "add", "remote-only.txt") + runGit(t, otherPath, "commit", "-m", "remote-only commit") + runGit(t, otherPath, "push", "origin", "main") + runGit(t, repoPath, "fetch", "origin", "main") + + mgr := newRecreateTestManager(t) + resolved, err := mgr.preferRefreshedRemoteRef(context.Background(), repoPath, "main") + if err != nil { + t.Fatalf("preferRefreshedRemoteRef() error: %v", err) + } + if resolved != "origin/main" { + t.Fatalf("resolved ref = %q, want origin/main", resolved) + } + }) +} + +func TestPreferRefreshedRemoteRefRejectsDivergedRefs(t *testing.T) { + repoPath := initGitRepoWithRemote(t) + writeRepoFile(t, repoPath, "local-only.txt", "local\n") + runGit(t, repoPath, "add", "local-only.txt") + runGit(t, repoPath, "commit", "-m", "local-only commit") + + otherPath := filepath.Join(t.TempDir(), "other") + originURL := strings.TrimSpace(runGit(t, repoPath, "remote", "get-url", "origin")) + runGit(t, filepath.Dir(otherPath), "clone", originURL, otherPath) + runGit(t, otherPath, "config", "user.email", "other@example.com") + runGit(t, otherPath, "config", "user.name", "Other User") + runGit(t, otherPath, "config", "commit.gpgsign", "false") + writeRepoFile(t, otherPath, "remote-only.txt", "remote\n") + runGit(t, otherPath, "add", "remote-only.txt") + runGit(t, otherPath, "commit", "-m", "remote-only commit") + runGit(t, otherPath, "push", "origin", "main") + runGit(t, repoPath, "fetch", "origin", "main") + + mgr := newRecreateTestManager(t) + if _, err := mgr.preferRefreshedRemoteRef(context.Background(), repoPath, "main"); err == nil { + t.Fatal("preferRefreshedRemoteRef() accepted diverged local and remote refs") + } +} + +func TestPreferRefreshedRemoteRefRejectsUncertainAncestry(t *testing.T) { + scriptDir := writeFakeGitScript(t, ` +case "${1:-}" in + rev-parse) + exit 0 + ;; + merge-base) + echo "fatal: unable to inspect repository" >&2 + exit 128 + ;; + *) + exit 0 + ;; +esac +`) + t.Setenv("PATH", scriptDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + mgr := newRecreateTestManager(t) + if _, err := mgr.preferRefreshedRemoteRef(context.Background(), t.TempDir(), "main"); err == nil { + t.Fatal("preferRefreshedRemoteRef() accepted an ancestry probe failure") + } +} + // TestPullBaseBranch_SuccessfulPullStillAdoptsRemoteCommits guards the happy // path the fix must not regress: when the pull succeeds, the base branch is // fast-forwarded and the worktree starts from the updated local branch, which @@ -152,7 +278,10 @@ func TestPullBaseBranch_SuccessfulPullStillAdoptsRemoteCommits(t *testing.T) { t.Fatalf("NewManager failed: %v", err) } - resolved := mgr.pullBaseBranch(context.Background(), repoPath, "main", nil) + resolved, err := mgr.pullBaseBranch(context.Background(), repoPath, "main", nil) + if err != nil { + t.Fatalf("pullBaseBranch() error = %v", err) + } resolvedSHA := strings.TrimSpace(runGit(t, repoPath, "rev-parse", resolved)) if resolvedSHA != remoteHead { t.Fatalf( diff --git a/apps/backend/internal/worktree/manager_recreate_recovery_test.go b/apps/backend/internal/worktree/manager_recreate_recovery_test.go index 6b1c07d744..0870ccb14d 100644 --- a/apps/backend/internal/worktree/manager_recreate_recovery_test.go +++ b/apps/backend/internal/worktree/manager_recreate_recovery_test.go @@ -67,6 +67,54 @@ func TestRecreate_FetchesBranchFromOriginWhenLocalDeleted(t *testing.T) { } } +func TestRecreate_RequiredBaseRefreshFailureStopsPreparation(t *testing.T) { + repoPath := initGitRepoWithRemote(t) + worktreePath := filepath.Join(t.TempDir(), "task-required", "repo-1") + if err := os.MkdirAll(worktreePath, 0755); err != nil { + t.Fatalf("create worktree placeholder: %v", err) + } + + scriptDir := writeFakeGitScript(t, ` +case "${1:-}" in + fetch) + echo "fatal: Authentication failed" >&2 + exit 1 + ;; + *) + exit 0 + ;; +esac +`) + t.Setenv("PATH", scriptDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + mgr := newRecreateTestManager(t) + existing := &Worktree{ + ID: "wt-required", + SessionID: "session-required", + TaskID: "task-required", + RepositoryID: "repo-1", + RepositoryPath: repoPath, + Path: worktreePath, + Branch: "feature/pr-branch", + Status: StatusDeleted, + } + + _, err := mgr.recreate(context.Background(), existing, CreateRequest{ + SessionID: "session-required", + TaskID: "task-required", + RepositoryID: "repo-1", + RepositoryPath: repoPath, + BaseBranch: "main", + PullBeforeWorktree: true, + }) + if err == nil { + t.Fatal("recreate() succeeded after required base refresh failure") + } + if _, statErr := os.Stat(worktreePath); statErr != nil { + t.Fatalf("required refresh failure removed the existing worktree path: %v", statErr) + } +} + // TestRecreate_BranchGoneEverywhereReturnsUnrecoverable pins the degraded // path: local branch deleted AND the branch never made it to origin (or was // deleted there too). recreate must return ErrBranchUnrecoverable so callers @@ -162,6 +210,95 @@ func TestRecreate_ForkPRFetchesPullHeadRef(t *testing.T) { } } +func TestRecreate_ManagedRefreshUsesRefreshedPRHeadWithoutNetwork(t *testing.T) { + repoPath, wantSHA := initGitRepoWithPullRef(t, 975, "feature/managed-fork-pr") + runGit(t, repoPath, "fetch", "origin", "pull/975/head:refs/remotes/origin/pr/975") + runGit(t, repoPath, "remote", "set-url", "origin", "https://127.0.0.1:1/never.git") + worktreePath := filepath.Join(t.TempDir(), "task-managed-pr", "repo-1") + if err := os.MkdirAll(worktreePath, 0755); err != nil { + t.Fatalf("create worktree placeholder: %v", err) + } + + mgr := newRecreateTestManager(t) + refreshCalls := 0 + wt, err := mgr.recreate(context.Background(), &Worktree{ + ID: "wt-managed-pr", SessionID: "session-managed-pr", TaskID: "task-managed-pr", + RepositoryID: "repo-1", RepositoryPath: repoPath, Path: worktreePath, + Branch: "feature/managed-fork-pr", Status: StatusDeleted, + }, CreateRequest{ + SessionID: "session-managed-pr", TaskID: "task-managed-pr", RepositoryID: "repo-1", + RepositoryPath: repoPath, CheckoutBranch: "feature/managed-fork-pr", PRNumber: 975, + PullBeforeWorktree: true, + RefreshRepository: func(context.Context) error { + refreshCalls++ + return nil + }, + }) + if err != nil { + t.Fatalf("recreate(): %v", err) + } + if refreshCalls != 1 { + t.Fatalf("refresh calls = %d, want 1", refreshCalls) + } + if got := strings.TrimSpace(runGit(t, wt.Path, "rev-parse", "HEAD")); got != wantSHA { + t.Fatalf("worktree HEAD = %q, want refreshed PR head %q", got, wantSHA) + } +} + +func TestRecreate_ManagedRefreshUsesRemoteWhenLocalCheckoutBranchIsBehind(t *testing.T) { + repoPath := initGitRepoWithRemote(t) + wantSHA := advanceRemoteBranch(t, repoPath, "feature/pr-branch") + runGit(t, repoPath, "remote", "set-url", "origin", "https://127.0.0.1:1/never.git") + worktreePath := filepath.Join(t.TempDir(), "task-managed-branch", "repo-1") + if err := os.MkdirAll(worktreePath, 0755); err != nil { + t.Fatalf("create worktree placeholder: %v", err) + } + + mgr := newRecreateTestManager(t) + wt, err := mgr.recreate(context.Background(), &Worktree{ + ID: "wt-managed-branch", SessionID: "session-managed-branch", TaskID: "task-managed-branch", + RepositoryID: "repo-1", RepositoryPath: repoPath, Path: worktreePath, + Branch: "feature/pr-branch", Status: StatusDeleted, + }, CreateRequest{ + SessionID: "session-managed-branch", TaskID: "task-managed-branch", RepositoryID: "repo-1", + RepositoryPath: repoPath, CheckoutBranch: "feature/pr-branch", PullBeforeWorktree: true, + RefreshRepository: func(context.Context) error { return nil }, + }) + if err != nil { + t.Fatalf("recreate(): %v", err) + } + if got := strings.TrimSpace(runGit(t, wt.Path, "rev-parse", "HEAD")); got != wantSHA { + t.Fatalf("worktree HEAD = %q, want refreshed branch head %q", got, wantSHA) + } +} + +func TestRecreate_ManagedRefreshUsesRemotePRHeadWhenLocalCheckoutBranchIsBehind(t *testing.T) { + repoPath, wantSHA := initManagedPRCheckoutBranch(t, 977, "feature/managed-fork-pr-behind") + runGit(t, repoPath, "remote", "set-url", "origin", "https://127.0.0.1:1/never.git") + worktreePath := filepath.Join(t.TempDir(), "task-managed-pr-behind", "repo-1") + if err := os.MkdirAll(worktreePath, 0755); err != nil { + t.Fatalf("create worktree placeholder: %v", err) + } + + mgr := newRecreateTestManager(t) + wt, err := mgr.recreate(context.Background(), &Worktree{ + ID: "wt-managed-pr-behind", SessionID: "session-managed-pr-behind", TaskID: "task-managed-pr-behind", + RepositoryID: "repo-1", RepositoryPath: repoPath, Path: worktreePath, + Branch: "feature/managed-fork-pr-behind", Status: StatusDeleted, + }, CreateRequest{ + SessionID: "session-managed-pr-behind", TaskID: "task-managed-pr-behind", RepositoryID: "repo-1", + RepositoryPath: repoPath, CheckoutBranch: "feature/managed-fork-pr-behind", PRNumber: 977, + PullBeforeWorktree: true, + RefreshRepository: func(context.Context) error { return nil }, + }) + if err != nil { + t.Fatalf("recreate(): %v", err) + } + if got := strings.TrimSpace(runGit(t, wt.Path, "rev-parse", "HEAD")); got != wantSHA { + t.Fatalf("worktree HEAD = %q, want refreshed PR head %q", got, wantSHA) + } +} + // TestCreate_RestoresReleasedWorktreeAfterArchive is the whole unarchive // round trip at the worktree layer. Archiving a task removes the worktree // directory and releases its reference (status=deleted + deleted_at) while diff --git a/apps/backend/internal/worktree/manager_test.go b/apps/backend/internal/worktree/manager_test.go index 87a015f750..165a081c31 100644 --- a/apps/backend/internal/worktree/manager_test.go +++ b/apps/backend/internal/worktree/manager_test.go @@ -771,7 +771,10 @@ esac } repoPath := t.TempDir() - ref := mgr.pullBaseBranch(context.Background(), repoPath, "origin/master", nil) + ref, err := mgr.pullBaseBranch(context.Background(), repoPath, "origin/master", nil) + if err != nil { + t.Fatalf("pullBaseBranch() error = %v", err) + } if ref != "origin/master" { t.Fatalf("pullBaseBranch() ref = %q, want %q", ref, "origin/master") } @@ -788,7 +791,7 @@ esac } } -func TestPullBaseBranch_FetchTimeoutFallsBackQuickly(t *testing.T) { +func TestPullBaseBranch_FetchTimeoutFailsQuickly(t *testing.T) { scriptDir := writeFakeGitScript(t, ` case "${1:-}" in fetch) @@ -820,11 +823,14 @@ esac repoPath := t.TempDir() start := time.Now() - ref := mgr.pullBaseBranch(context.Background(), repoPath, "master", nil) + ref, err := mgr.pullBaseBranch(context.Background(), repoPath, "master", nil) elapsed := time.Since(start) - if ref != "master" { - t.Fatalf("pullBaseBranch() ref = %q, want %q", ref, "master") + if err == nil { + t.Fatal("pullBaseBranch() error = nil, want required refresh failure") + } + if ref != "" { + t.Fatalf("pullBaseBranch() ref = %q, want empty on failure", ref) } // Allow CI scheduling variance while still asserting we timed out // well before the fake 2s fetch command completes. @@ -871,7 +877,10 @@ esac mgr.pullTimeout = 300 * time.Millisecond repoPath := t.TempDir() - ref := mgr.pullBaseBranch(context.Background(), repoPath, "master", nil) + ref, err := mgr.pullBaseBranch(context.Background(), repoPath, "master", nil) + if err != nil { + t.Fatalf("pullBaseBranch() error = %v", err) + } if ref != "origin/master" { t.Fatalf("pullBaseBranch() ref = %q, want %q", ref, "origin/master") } @@ -954,6 +963,84 @@ esac } } +func TestFetchBranchToLocal_RequiredRefreshRejectsLocalFallback(t *testing.T) { + scriptDir := writeFakeGitScript(t, ` +case "${1:-}" in + fetch) + echo "fatal: Authentication failed" >&2 + exit 1 + ;; + rev-parse) + if [ "${2:-}" = "--verify" ]; then + exit 0 + fi + exit 0 + ;; + *) + exit 0 + ;; +esac +`) + t.Setenv("PATH", scriptDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + mgr, err := NewManager(newTestConfig(t), newMockStore(), newTestLogger()) + if err != nil { + t.Fatalf("NewManager failed: %v", err) + } + + result, err := mgr.fetchBranchToLocalWithPolicy( + context.Background(), t.TempDir(), "feature/pr-branch", 0, true, + ) + if err == nil { + t.Fatal("required checkout refresh succeeded after fetch authentication failure") + } + if result != nil { + t.Fatalf("required checkout refresh returned a fallback result: %+v", result) + } + if strings.Contains(err.Error(), "Using local") { + t.Fatalf("required checkout refresh exposed a stale local fallback: %v", err) + } +} + +func TestFetchBranchToLocal_RequiredRefreshReportsMissingBranch(t *testing.T) { + scriptDir := writeFakeGitScript(t, ` +case "${1:-}" in + fetch) + echo "fatal: couldn't find remote ref feature/pr-branch" >&2 + exit 128 + ;; + rev-parse) + if [ "${2:-}" = "--verify" ]; then + exit 1 + fi + exit 0 + ;; + *) + exit 0 + ;; +esac +`) + t.Setenv("PATH", scriptDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + mgr, err := NewManager(newTestConfig(t), newMockStore(), newTestLogger()) + if err != nil { + t.Fatalf("NewManager failed: %v", err) + } + + result, err := mgr.fetchBranchToLocalWithPolicy( + context.Background(), t.TempDir(), "feature/pr-branch", 0, true, + ) + if err == nil { + t.Fatal("required checkout refresh succeeded for a missing remote branch") + } + if result != nil { + t.Fatalf("required checkout refresh returned a result: %+v", result) + } + if !errors.Is(err, ErrInvalidBaseBranch) { + t.Fatalf("error = %v, want ErrInvalidBaseBranch", err) + } +} + func TestFetchBranchToLocal_FetchFailsNoBranch(t *testing.T) { scriptDir := writeFakeGitScript(t, ` case "${1:-}" in diff --git a/apps/backend/internal/worktree/worktree.go b/apps/backend/internal/worktree/worktree.go index 92f63e6c7b..d39b13e40b 100644 --- a/apps/backend/internal/worktree/worktree.go +++ b/apps/backend/internal/worktree/worktree.go @@ -1,6 +1,7 @@ package worktree import ( + "context" "fmt" "time" @@ -13,6 +14,7 @@ type SyncProgressStatus string const ( SyncProgressRunning SyncProgressStatus = "running" SyncProgressCompleted SyncProgressStatus = "completed" + SyncProgressFailed SyncProgressStatus = "failed" ) // SyncProgressEvent reports pre-worktree base-branch synchronization progress. @@ -204,6 +206,12 @@ type CreateRequest struct { // tracking refs only and must not perform another network operation. RemoteSyncHandled bool + // RefreshRepository is an optional provider-authenticated refresh deferred + // until this request needs to materialize or recreate a worktree. A valid + // reusable worktree must bypass it. On success, Create marks the refresh as + // handled before selecting local refs. + RefreshRepository func(context.Context) error + // WorktreeID is the ID of an existing worktree to reuse (optional). // If provided and valid, the existing worktree is returned instead of creating a new one. WorktreeID string diff --git a/apps/web/e2e/fixtures/test-base.ts b/apps/web/e2e/fixtures/test-base.ts index c97aacd9a9..46ea6ca35d 100644 --- a/apps/web/e2e/fixtures/test-base.ts +++ b/apps/web/e2e/fixtures/test-base.ts @@ -387,6 +387,10 @@ export const test = backendFixture.extend< apiClient.mockGitLabReset(seedData.workspaceId).catch(() => undefined), apiClient.clearGitLabRepositoryRemote(seedData.repositoryId).catch(() => undefined), ]); + // GitLab reset removes origin from the shared seed checkout. Restore the + // fixture's offline origin before the test so pull-enabled worktree + // preparation starts from the same valid repository state every time. + restoreSeedRepositoryOrigin(seedData); try { await use(); } finally { @@ -446,6 +450,28 @@ export function pointSeedRepositoryAtUnresolvedOrigin(seedData: SeedData, tmpDir } } +/** Points the seed repository at a valid cached checkout with an unreachable origin. */ +export function pointSeedRepositoryAtFailingOrigin(seedData: SeedData, tmpDir: string) { + const remoteDir = path.join( + tmpDir, + "repos", + `e2e-failing-remote-${Date.now()}-${process.pid}.git`, + ); + try { + execFileSync( + "git", + ["-C", seedData.repositoryPath, "remote", "set-url", "origin", `file://${remoteDir}`], + { env: makeGitEnv(tmpDir), stdio: "ignore" }, + ); + } catch { + execFileSync( + "git", + ["-C", seedData.repositoryPath, "remote", "add", "origin", `file://${remoteDir}`], + { env: makeGitEnv(tmpDir), stdio: "ignore" }, + ); + } +} + // Reset the active workspace pointer before every test so that specs which // do not use the testPage fixture (e.g. API-only routing tests) start from // a known workspace_id instead of whatever a previous test's completeOnboarding diff --git a/apps/web/e2e/helpers/api-client.ts b/apps/web/e2e/helpers/api-client.ts index 3a9f755877..8c26851b11 100644 --- a/apps/web/e2e/helpers/api-client.ts +++ b/apps/web/e2e/helpers/api-client.ts @@ -804,6 +804,7 @@ export class ApiClient { provider_host?: string; provider_owner?: string; provider_name?: string; + pull_before_worktree?: boolean; }, ): Promise<{ id: string }> { return this.request("POST", `/api/v1/workspaces/${workspaceId}/repositories`, { @@ -815,6 +816,9 @@ export class ApiClient { ...(opts?.provider_host ? { provider_host: opts.provider_host } : {}), ...(opts?.provider_owner ? { provider_owner: opts.provider_owner } : {}), ...(opts?.provider_name ? { provider_name: opts.provider_name } : {}), + ...(opts?.pull_before_worktree !== undefined + ? { pull_before_worktree: opts.pull_before_worktree } + : {}), }); } @@ -909,6 +913,7 @@ export class ApiClient { repositoryId: string, updates: { default_branch?: string; + pull_before_worktree?: boolean; provider?: string; provider_repo_id?: string; provider_host?: string; diff --git a/apps/web/e2e/tests/chat/quick-chat.spec.ts b/apps/web/e2e/tests/chat/quick-chat.spec.ts index c2217d0e1a..089d124828 100644 --- a/apps/web/e2e/tests/chat/quick-chat.spec.ts +++ b/apps/web/e2e/tests/chat/quick-chat.spec.ts @@ -401,6 +401,10 @@ test.describe("Quick Chat", () => { const sourceRepo = path.join(backend.tmpDir, "repos", "e2e-repo"); const contextBranch = "quick-chat-context-branch"; execFileSync("git", ["branch", "-f", contextBranch], { cwd: sourceRepo }); + // Repository-backed quick chat performs a required refresh before it + // materializes the worktree. Publish the local context branch to the + // fixture's offline origin so that refresh succeeds deterministically. + execFileSync("git", ["push", "--force", "origin", contextBranch], { cwd: sourceRepo }); try { const dialog = await openQuickChatSetup(testPage); await expect(dialog.getByTestId("quick-chat-introduction")).toContainText( @@ -418,7 +422,7 @@ test.describe("Quick Chat", () => { await dialog.getByTestId("repo-chip-trigger").click(); await testPage.getByRole("option").first().click(); await dialog.getByTestId("branch-chip-trigger").click(); - await testPage.getByRole("option", { name: contextBranch }).click(); + await testPage.locator(`[role="option"][data-value="${contextBranch}"]`).click(); const startRequest = testPage.waitForRequest( (request) => request.url().includes("/quick-chat") && request.method() === "POST", @@ -438,6 +442,7 @@ test.describe("Quick Chat", () => { }).trim(), ).toBe("main"); } finally { + execFileSync("git", ["push", "origin", "--delete", contextBranch], { cwd: sourceRepo }); execFileSync("git", ["branch", "-D", contextBranch], { cwd: sourceRepo }); } }); diff --git a/apps/web/e2e/tests/git/changes-panel-multi-repo-indent.spec.ts b/apps/web/e2e/tests/git/changes-panel-multi-repo-indent.spec.ts index fb4ad17f30..5452d1201f 100644 --- a/apps/web/e2e/tests/git/changes-panel-multi-repo-indent.spec.ts +++ b/apps/web/e2e/tests/git/changes-panel-multi-repo-indent.spec.ts @@ -39,6 +39,7 @@ test("nests multi-repository changes below each repository on desktop and mobile execSync('git commit --allow-empty -m "init"', { cwd: extraRepoDir, env: gitEnv }); const extraRepo = await apiClient.createRepository(seedData.workspaceId, extraRepoDir, "main", { name: "indent-extra-repo", + pull_before_worktree: false, }); const task = await apiClient.createTaskWithAgent( diff --git a/apps/web/e2e/tests/git/git-changes-panel.spec.ts b/apps/web/e2e/tests/git/git-changes-panel.spec.ts index 6ddd78a092..df3e226c98 100644 --- a/apps/web/e2e/tests/git/git-changes-panel.spec.ts +++ b/apps/web/e2e/tests/git/git-changes-panel.spec.ts @@ -1350,7 +1350,7 @@ test.describe("Git Changes Panel", () => { expect(await diffViewerOpen(), "no cumulative diff panel before clicking Diff").toBe(false); // Click the "Diff" button in the header to open the cumulative diff view - await session.changes.getByRole("button", { name: "Diff" }).click(); + await session.changes.getByRole("button", { name: "Diff", exact: true }).click(); // Assert what the click actually opens. Without this the checks below are // vacuous: the two commit texts were already visible before the click and @@ -1492,22 +1492,12 @@ test.describe("Git Changes Panel", () => { }) => { const profile = await createStandardProfile(apiClient, "Git PR Dedup Profile"); - const task = await apiClient.createTaskWithAgent( - seedData.workspaceId, - "Git PR Dedup Test", - profile.id, - { - description: "Testing PR commit deduplication", - workflow_id: seedData.workflowId, - workflow_step_id: seedData.startStepId, - repository_ids: [seedData.repositoryId], - }, - ); - - const session = await openTaskSession(testPage, "Git PR Dedup Test"); - - // Set up git helper - const repoDir = path.join(backend.tmpDir, "repos", "e2e-repo"); + // This test mutates repository history. Use a disposable local repository + // so commits from earlier cumulative-diff tests cannot leak into the + // unified list. The fixture has no fetchable origin, so keep materialization + // offline while testing PR commit deduplication. + const repoDir = path.join(backend.tmpDir, "repos", "git-pr-dedup-repo"); + fs.mkdirSync(repoDir, { recursive: true }); const gitEnv = { ...process.env, HOME: backend.tmpDir, @@ -1516,6 +1506,28 @@ test.describe("Git Changes Panel", () => { GIT_COMMITTER_NAME: "E2E Test", GIT_COMMITTER_EMAIL: "e2e@test.local", }; + execSync("git init -b main", { cwd: repoDir, env: gitEnv }); + execSync('git commit --allow-empty -m "init"', { cwd: repoDir, env: gitEnv }); + const repository = await apiClient.createRepository(seedData.workspaceId, repoDir, "main", { + name: "Git PR Dedup Repo", + pull_before_worktree: false, + }); + + const task = await apiClient.createTaskWithAgent( + seedData.workspaceId, + "Git PR Dedup Test", + profile.id, + { + description: "Testing PR commit deduplication", + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + repository_ids: [repository.id], + }, + ); + + const session = await openTaskSession(testPage, "Git PR Dedup Test"); + + // Set up git helper const git = new GitHelper(repoDir, gitEnv); // Create a commit diff --git a/apps/web/e2e/tests/gitlab/gitlab-mr-creation.spec.ts b/apps/web/e2e/tests/gitlab/gitlab-mr-creation.spec.ts index dc1ceee368..07f421cb9c 100644 --- a/apps/web/e2e/tests/gitlab/gitlab-mr-creation.spec.ts +++ b/apps/web/e2e/tests/gitlab/gitlab-mr-creation.spec.ts @@ -24,6 +24,9 @@ test.describe("GitLab merge request creation", () => { provider_host: backend.baseUrl, provider_owner: "platform", provider_name: "kandev", + // The mock GitLab remote implements push for this test, but not + // upload-pack/fetch. Keep the setup offline while exercising MR create. + pull_before_worktree: false, }); const task = await apiClient.createTaskWithAgent( diff --git a/apps/web/e2e/tests/gitlab/gitlab-mr-push-autolink.spec.ts b/apps/web/e2e/tests/gitlab/gitlab-mr-push-autolink.spec.ts index 9a567193c9..60463fe28e 100644 --- a/apps/web/e2e/tests/gitlab/gitlab-mr-push-autolink.spec.ts +++ b/apps/web/e2e/tests/gitlab/gitlab-mr-push-autolink.spec.ts @@ -29,6 +29,7 @@ test.describe("GitLab MR push-detection auto-link", () => { provider_host: backend.baseUrl, provider_owner: "platform", provider_name: "kandev", + pull_before_worktree: false, }); // First turn: commit a change but do NOT push yet, so the branch has no diff --git a/apps/web/e2e/tests/gitlab/mobile-gitlab-parity.spec.ts b/apps/web/e2e/tests/gitlab/mobile-gitlab-parity.spec.ts index 5e20866e2e..10c6fae999 100644 --- a/apps/web/e2e/tests/gitlab/mobile-gitlab-parity.spec.ts +++ b/apps/web/e2e/tests/gitlab/mobile-gitlab-parity.spec.ts @@ -37,6 +37,7 @@ async function seedMultiRepoGitLabTask( provider_host: GITLAB_HOST, provider_owner: "platform", provider_name: "kandev", + pull_before_worktree: false, }); const secondaryRepoDir = path.join(tmpDir, "repos", "mobile-gitlab-secondary"); fs.mkdirSync(secondaryRepoDir, { recursive: true }); @@ -53,6 +54,7 @@ async function seedMultiRepoGitLabTask( provider_host: GITLAB_HOST, provider_owner: "platform", provider_name: "docs", + pull_before_worktree: false, }, ); return apiClient.createTask(seedData.workspaceId, "Mobile contextual GitLab link", { @@ -83,6 +85,7 @@ test.describe("Mobile GitLab parity", () => { provider_host: GITLAB_HOST, provider_owner: "platform", provider_name: "kandev", + pull_before_worktree: false, }); const gitlab = new GitLabPage(testPage); @@ -126,6 +129,7 @@ test.describe("Mobile GitLab parity", () => { provider_host: GITLAB_HOST, provider_owner: "platform", provider_name: "kandev", + pull_before_worktree: false, }); const gitlab = new GitLabPage(testPage); @@ -345,6 +349,7 @@ test.describe("Mobile GitLab parity", () => { provider_host: backend.baseUrl, provider_owner: "platform", provider_name: "kandev", + pull_before_worktree: false, }); const task = await apiClient.createTaskWithAgent( seedData.workspaceId, diff --git a/apps/web/e2e/tests/lsp/lsp-file-intelligence.spec.ts b/apps/web/e2e/tests/lsp/lsp-file-intelligence.spec.ts index 05f41c1a5f..fba7c65176 100644 --- a/apps/web/e2e/tests/lsp/lsp-file-intelligence.spec.ts +++ b/apps/web/e2e/tests/lsp/lsp-file-intelligence.spec.ts @@ -847,7 +847,7 @@ test.describe("LSP file intelligence", () => { seedData.workspaceId, repositoryDirectory, "main", - { name: repositoryName }, + { name: repositoryName, pull_before_worktree: false }, ); const task = await createKotlinTask(testPage, apiClient, seedData, backend, { diff --git a/apps/web/e2e/tests/pr/pr-push-autolink.spec.ts b/apps/web/e2e/tests/pr/pr-push-autolink.spec.ts index bd9edd4a37..618a75b45e 100644 --- a/apps/web/e2e/tests/pr/pr-push-autolink.spec.ts +++ b/apps/web/e2e/tests/pr/pr-push-autolink.spec.ts @@ -38,6 +38,7 @@ test.describe("GitHub PR push-detection auto-link", () => { provider_host: "https://github.com", provider_owner: "testorg", provider_name: "testrepo", + pull_before_worktree: false, }); await apiClient.mockGitHubReset(); await apiClient.mockGitHubSetUser("test-user"); diff --git a/apps/web/e2e/tests/search/task-workspace-content-search.spec.ts b/apps/web/e2e/tests/search/task-workspace-content-search.spec.ts index 4287e3a743..1a13ae69c7 100644 --- a/apps/web/e2e/tests/search/task-workspace-content-search.spec.ts +++ b/apps/web/e2e/tests/search/task-workspace-content-search.spec.ts @@ -75,7 +75,7 @@ test("@search searches all task repositories and opens the selected match", asyn seedData.workspaceId, extraRepoDir, "main", - { name: EXTRA_REPOSITORY_NAME }, + { name: EXTRA_REPOSITORY_NAME, pull_before_worktree: false }, ); const task = await apiClient.createTaskWithAgent( diff --git a/apps/web/e2e/tests/session/mobile-multi-repository-session-picker.spec.ts b/apps/web/e2e/tests/session/mobile-multi-repository-session-picker.spec.ts index 052d7a9951..190f0c1b0e 100644 --- a/apps/web/e2e/tests/session/mobile-multi-repository-session-picker.spec.ts +++ b/apps/web/e2e/tests/session/mobile-multi-repository-session-picker.spec.ts @@ -27,13 +27,13 @@ test.describe("mobile: multi-repository session picker", () => { seedData.workspaceId, primaryRepoDir, "main", - { name: "Mobile primary" }, + { name: "Mobile primary", pull_before_worktree: false }, ); const secondaryRepo = await apiClient.createRepository( seedData.workspaceId, secondaryRepoDir, "main", - { name: "Mobile secondary" }, + { name: "Mobile secondary", pull_before_worktree: false }, ); const task = await apiClient.createTaskWithAgent( seedData.workspaceId, diff --git a/apps/web/e2e/tests/task/add-workspace-sources.spec.ts b/apps/web/e2e/tests/task/add-workspace-sources.spec.ts index a73f8809a7..845c8af4fb 100644 --- a/apps/web/e2e/tests/task/add-workspace-sources.spec.ts +++ b/apps/web/e2e/tests/task/add-workspace-sources.spec.ts @@ -24,7 +24,7 @@ async function chooseDirectory( const picker = page.locator('[data-testid="folder-picker-popover"][data-state="open"]'); await expect(picker).toBeVisible(); for (const segment of relativeDirectory.split(path.sep).filter(Boolean)) { - await picker.getByTestId("folder-picker-entry").filter({ hasText: segment }).click(); + await picker.getByRole("button", { name: segment, exact: true }).click(); } await picker.getByTestId("folder-picker-choose").click(); } @@ -32,6 +32,7 @@ async function chooseDirectory( function createSourceDirectories(root: string) { const gitEnv = makeGitEnv(root); const repositoryPath = path.join(root, "sources", "second-local-repository"); + const originPath = path.join(root, "sources", "second-local-repository-origin.git"); const folderPath = path.join(root, "sources", "plain-local-folder"); fs.mkdirSync(repositoryPath, { recursive: true }); fs.mkdirSync(folderPath, { recursive: true }); @@ -40,6 +41,18 @@ function createSourceDirectories(root: string) { execFileSync("git", ["init", "-b", "main"], { cwd: repositoryPath, env: gitEnv }); execFileSync("git", ["add", "."], { cwd: repositoryPath, env: gitEnv }); execFileSync("git", ["commit", "-m", "initial source"], { cwd: repositoryPath, env: gitEnv }); + // Workspace-source materialization refreshes required branches before it + // creates the attached worktree. Give this local fixture a real offline + // origin so that refresh succeeds deterministically. + execFileSync("git", ["init", "--bare", "-b", "main", originPath], { env: gitEnv }); + execFileSync("git", ["remote", "add", "origin", originPath], { + cwd: repositoryPath, + env: gitEnv, + }); + execFileSync("git", ["push", "--set-upstream", "origin", "main"], { + cwd: repositoryPath, + env: gitEnv, + }); return { repositoryPath, folderPath }; } diff --git a/apps/web/e2e/tests/task/create-task-github-url.spec.ts b/apps/web/e2e/tests/task/create-task-github-url.spec.ts index 2d358446f2..687d670261 100644 --- a/apps/web/e2e/tests/task/create-task-github-url.spec.ts +++ b/apps/web/e2e/tests/task/create-task-github-url.spec.ts @@ -44,6 +44,7 @@ test.describe("Task creation from GitHub URL", () => { provider: "github", provider_owner: "test-owner", provider_name: "test-repo", + pull_before_worktree: false, }); // Seed mock GitHub branches for the repo we'll reference @@ -116,6 +117,7 @@ test.describe("Task creation from GitHub URL", () => { provider: "github", provider_owner: "test-owner", provider_name: "test-repo", + pull_before_worktree: false, }); await apiClient.mockGitHubAddBranches("test-owner", "test-repo", [ @@ -195,6 +197,7 @@ test.describe("Task creation from GitHub URL", () => { provider: "github", provider_owner: "owner-a", provider_name: "repo-a", + pull_before_worktree: false, }); await apiClient.mockGitHubAddBranches("owner-a", "repo-a", [{ name: "main" }]); @@ -216,6 +219,7 @@ test.describe("Task creation from GitHub URL", () => { provider: "github", provider_owner: "owner-b", provider_name: "repo-b", + pull_before_worktree: false, }); await apiClient.mockGitHubAddBranches("owner-b", "repo-b", [{ name: "main" }]); @@ -269,6 +273,7 @@ test.describe("Task creation from GitHub URL", () => { provider: "github", provider_owner: "test-owner", provider_name: "test-repo", + pull_before_worktree: false, }); // Seed branches including the PR head branch @@ -345,6 +350,7 @@ test.describe("Task creation from GitHub URL", () => { provider: "github", provider_owner: "test-owner", provider_name: "test-repo", + pull_before_worktree: false, }); await apiClient.mockGitHubAddBranches("test-owner", "test-repo", [ @@ -455,6 +461,7 @@ test.describe("Task creation from GitHub URL", () => { provider: "github", provider_owner: "pr-owner", provider_name: "pr-wt-repo", + pull_before_worktree: false, }); // Seed mock GitHub branches and PR @@ -568,6 +575,7 @@ test.describe("Task creation from GitHub URL", () => { provider: "github", provider_owner: "warn-owner", provider_name: "warn-repo", + pull_before_worktree: false, }); await apiClient.mockGitHubAddBranches("warn-owner", "warn-repo", [ @@ -686,6 +694,7 @@ test.describe("Task creation from GitHub URL", () => { provider: "github", provider_owner: "shared-owner", provider_name: "shared-repo", + pull_before_worktree: false, }); await apiClient.mockGitHubAddBranches("shared-owner", "shared-repo", [ diff --git a/apps/web/e2e/tests/task/editors-menu-worktree-picker.spec.ts b/apps/web/e2e/tests/task/editors-menu-worktree-picker.spec.ts index 5d135714f1..305b1479c4 100644 --- a/apps/web/e2e/tests/task/editors-menu-worktree-picker.spec.ts +++ b/apps/web/e2e/tests/task/editors-menu-worktree-picker.spec.ts @@ -30,6 +30,7 @@ test.describe("Editors menu worktree picker", () => { execSync('git commit --allow-empty -m "init"', { cwd: repoDir, env: gitEnv }); const repo = await apiClient.createRepository(seedData.workspaceId, repoDir, "main", { name, + pull_before_worktree: false, }); repoIds.push(repo.id); } diff --git a/apps/web/e2e/tests/task/launch-failure-recovery.spec.ts b/apps/web/e2e/tests/task/launch-failure-recovery.spec.ts index 2285fc4d55..1937a9f368 100644 --- a/apps/web/e2e/tests/task/launch-failure-recovery.spec.ts +++ b/apps/web/e2e/tests/task/launch-failure-recovery.spec.ts @@ -1,4 +1,5 @@ import { + pointSeedRepositoryAtFailingOrigin, pointSeedRepositoryAtUnresolvedOrigin, restoreSeedRepositoryOrigin, test, @@ -184,6 +185,10 @@ test.describe("task launch failure recovery", () => { events: { on_enter: [{ type: "auto_start_agent" }] }, }); + await apiClient.updateRepository(seedData.repositoryId, { + default_branch: "default-branch-that-no-longer-exists", + pull_before_worktree: false, + }); const task = await apiClient.createTask( seedData.workspaceId, "Missing base branch recovery fixture", @@ -205,9 +210,6 @@ test.describe("task launch failure recovery", () => { const taskRepository = storedTask.repositories?.[0]; if (!taskRepository) throw new Error("launch fixture did not create a task repository row"); - await apiClient.updateRepository(seedData.repositoryId, { - default_branch: "default-branch-that-no-longer-exists", - }); pointSeedRepositoryAtUnresolvedOrigin(seedData, backend.tmpDir); try { @@ -273,7 +275,90 @@ test.describe("task launch failure recovery", () => { }); } finally { restoreSeedRepositoryOrigin(seedData); - await apiClient.updateRepository(seedData.repositoryId, { default_branch: "main" }); + await apiClient.updateRepository(seedData.repositoryId, { + default_branch: "main", + pull_before_worktree: true, + }); + } + }); + + test("persists a required refresh failure and retries after the origin recovers", async ({ + testPage, + apiClient, + seedData, + backend, + }, testInfo) => { + test.setTimeout(150_000); + + const { workflow, waiting, review } = await recoveryWorkflow( + apiClient, + seedData.workspaceId, + `Required refresh recovery ${Date.now()}`, + ); + const task = await apiClient.createTask( + seedData.workspaceId, + "Required refresh failure recovery fixture", + { + description: "/e2e:simple-message", + workflow_id: workflow.id, + workflow_step_id: waiting.id, + agent_profile_id: seedData.agentProfileId, + executor_profile_id: seedData.worktreeExecutorProfileId, + repositories: [{ repository_id: seedData.repositoryId, base_branch: "main" }], + }, + ); + const storedTask = await apiClient.getTask(task.id); + const taskRepository = storedTask.repositories?.[0]; + if (!taskRepository) throw new Error("refresh fixture did not create a task repository row"); + + pointSeedRepositoryAtFailingOrigin(seedData, backend.tmpDir); + try { + await apiClient.moveTask(task.id, workflow.id, review.id); + await testPage.goto(`/t/${task.id}`); + const session = new SessionPage(testPage); + await session.waitForLoad(); + + const launchError = await waitForTaskLaunchError(apiClient, seedData.workspaceId, task.id); + expect(launchError.category).toBe("generic_launch_failure"); + expect(launchError.task_repository_id).toBe(taskRepository.id); + expect(launchError.recovery_actions).toEqual(["retry_default", "pick_base_branch"]); + + const card = testPage.getByTestId("task-launch-error-entry"); + await expect(card).toHaveCount(1, { timeout: 30_000 }); + await expect(card).toContainText("The task could not start."); + + await testPage.reload(); + await session.waitForLoad(); + await expect(testPage.getByTestId("task-launch-error-entry")).toHaveCount(1); + + restoreSeedRepositoryOrigin(seedData); + await testPage.getByTestId("task-launch-pick_base_branch-button").click(); + await expect(testPage.getByTestId("task-launch-branch-picker-option-main")).toBeVisible({ + timeout: 30_000, + }); + await testPage.getByTestId("task-launch-branch-picker-option-main").click(); + await expect(testPage.getByTestId("task-launch-branch-picker-option-main")).toHaveCount(0); + await waitForLaunchErrorCleared(apiClient, seedData.workspaceId, task.id); + + await expect + .poll( + async () => { + const { sessions } = await apiClient.listTaskSessions(task.id); + return sessions.some((item) => + ["RUNNING", "WAITING_FOR_INPUT", "IDLE", "COMPLETED"].includes(item.state), + ); + }, + { timeout: 60_000, message: "waiting for the recovered refresh session to launch" }, + ) + .toBe(true); + + await assertNoDocumentHorizontalOverflow(testPage, "desktop required refresh recovery"); + await testPage.screenshot({ + path: testInfo.outputPath("required-refresh-recovery-desktop.png"), + fullPage: true, + }); + } finally { + restoreSeedRepositoryOrigin(seedData); } }); }); diff --git a/apps/web/e2e/tests/task/mobile-add-workspace-sources.spec.ts b/apps/web/e2e/tests/task/mobile-add-workspace-sources.spec.ts index 23277522c0..dff4e90569 100644 --- a/apps/web/e2e/tests/task/mobile-add-workspace-sources.spec.ts +++ b/apps/web/e2e/tests/task/mobile-add-workspace-sources.spec.ts @@ -20,7 +20,7 @@ async function chooseDirectory( const picker = page.locator('[data-testid="folder-picker-popover"][data-state="open"]'); await expect(picker).toBeVisible(); for (const segment of relativeDirectory.split(path.sep).filter(Boolean)) { - await picker.getByTestId("folder-picker-entry").filter({ hasText: segment }).tap(); + await picker.getByRole("button", { name: segment, exact: true }).tap(); } await picker.getByTestId("folder-picker-choose").tap(); } @@ -35,6 +35,11 @@ test("mobile Files drawer attaches sources with fixed controls and persisted wor test.setTimeout(120_000); const gitEnv = makeGitEnv(backend.tmpDir); const repositoryPath = path.join(backend.tmpDir, "mobile-sources", "mobile-local-repository"); + const originPath = path.join( + backend.tmpDir, + "mobile-sources", + "mobile-local-repository-origin.git", + ); const folderPath = path.join(backend.tmpDir, "mobile-sources", "mobile-local-folder"); fs.mkdirSync(repositoryPath, { recursive: true }); fs.mkdirSync(folderPath, { recursive: true }); @@ -43,6 +48,17 @@ test("mobile Files drawer attaches sources with fixed controls and persisted wor execFileSync("git", ["init", "-b", "main"], { cwd: repositoryPath, env: gitEnv }); execFileSync("git", ["add", "."], { cwd: repositoryPath, env: gitEnv }); execFileSync("git", ["commit", "-m", "initial source"], { cwd: repositoryPath, env: gitEnv }); + // Source worktrees perform a required refresh. Use a local bare origin so + // this fixture remains offline while satisfying that production contract. + execFileSync("git", ["init", "--bare", "-b", "main", originPath], { env: gitEnv }); + execFileSync("git", ["remote", "add", "origin", originPath], { + cwd: repositoryPath, + env: gitEnv, + }); + execFileSync("git", ["push", "--set-upstream", "origin", "main"], { + cwd: repositoryPath, + env: gitEnv, + }); const task = await apiClient.createTaskWithAgent( seedData.workspaceId, "Mobile attach local sources", diff --git a/apps/web/e2e/tests/task/mobile-launch-failure-recovery.spec.ts b/apps/web/e2e/tests/task/mobile-launch-failure-recovery.spec.ts index e145191106..04b5306125 100644 --- a/apps/web/e2e/tests/task/mobile-launch-failure-recovery.spec.ts +++ b/apps/web/e2e/tests/task/mobile-launch-failure-recovery.spec.ts @@ -1,5 +1,6 @@ // Filename starts with "mobile-" so this runs in the mobile-chrome project. import { + pointSeedRepositoryAtFailingOrigin, pointSeedRepositoryAtUnresolvedOrigin, restoreSeedRepositoryOrigin, test, @@ -42,6 +43,10 @@ test.describe("mobile task launch failure recovery", () => { events: { on_enter: [{ type: "auto_start_agent" }] }, }); + await apiClient.updateRepository(seedData.repositoryId, { + default_branch: "mobile-default-branch-that-no-longer-exists", + pull_before_worktree: false, + }); const task = await apiClient.createTask( seedData.workspaceId, "Mobile missing base branch recovery fixture", @@ -63,9 +68,6 @@ test.describe("mobile task launch failure recovery", () => { const taskRepository = storedTask.repositories?.[0]; if (!taskRepository) throw new Error("mobile fixture did not create a task repository row"); - await apiClient.updateRepository(seedData.repositoryId, { - default_branch: "mobile-default-branch-that-no-longer-exists", - }); pointSeedRepositoryAtUnresolvedOrigin(seedData, backend.tmpDir); try { @@ -140,7 +142,121 @@ test.describe("mobile task launch failure recovery", () => { }); } finally { restoreSeedRepositoryOrigin(seedData); - await apiClient.updateRepository(seedData.repositoryId, { default_branch: "main" }); + await apiClient.updateRepository(seedData.repositoryId, { + default_branch: "main", + pull_before_worktree: true, + }); + } + }); + + test("keeps a required refresh failure visible and retries on the phone", async ({ + testPage, + apiClient, + seedData, + backend, + }, testInfo) => { + test.setTimeout(150_000); + + const workflow = await apiClient.createWorkflow( + seedData.workspaceId, + `Mobile required refresh recovery ${Date.now()}`, + ); + const waiting = await apiClient.createWorkflowStep(workflow.id, "Waiting", 0); + const review = await apiClient.createWorkflowStep(workflow.id, "Review", 1); + await apiClient.updateWorkflowStep(review.id, { + events: { on_enter: [{ type: "auto_start_agent" }] }, + }); + + const task = await apiClient.createTask( + seedData.workspaceId, + "Mobile required refresh failure recovery fixture", + { + description: "/e2e:simple-message", + workflow_id: workflow.id, + workflow_step_id: waiting.id, + agent_profile_id: seedData.agentProfileId, + executor_profile_id: seedData.worktreeExecutorProfileId, + repositories: [{ repository_id: seedData.repositoryId, base_branch: "main" }], + }, + ); + const storedTask = await apiClient.getTask(task.id); + const taskRepository = storedTask.repositories?.[0]; + if (!taskRepository) + throw new Error("mobile refresh fixture did not create a task repository row"); + + pointSeedRepositoryAtFailingOrigin(seedData, backend.tmpDir); + try { + await apiClient.moveTask(task.id, workflow.id, review.id); + await testPage.goto(`/t/${task.id}`); + const session = new SessionPage(testPage); + await session.waitForLoad(); + await expect + .poll( + async () => { + const { tasks } = await apiClient.listTasks(seedData.workspaceId); + const error = tasks.find((candidate) => candidate.id === task.id)?.status_summary + ?.active_error; + return Boolean(error?.stamp && error.category === "generic_launch_failure"); + }, + { timeout: 60_000, message: "waiting for the mobile required refresh launch error" }, + ) + .toBe(true); + + const launchError = (await apiClient.listTasks(seedData.workspaceId)).tasks.find( + (candidate) => candidate.id === task.id, + )?.status_summary?.active_error; + expect(launchError?.task_repository_id).toBe(taskRepository.id); + + const card = testPage.getByTestId("task-launch-error-entry"); + await expect(card).toHaveCount(1, { timeout: 30_000 }); + await expect(card).toContainText("The task could not start."); + await testPage.reload(); + await session.waitForLoad(); + await expect(testPage.getByTestId("task-launch-error-entry")).toHaveCount(1); + + restoreSeedRepositoryOrigin(seedData); + const pickBranch = testPage.getByTestId("task-launch-pick_base_branch-button"); + await pickBranch.tap(); + await expect(testPage.getByTestId("task-launch-branch-picker-mobile")).toBeVisible({ + timeout: 30_000, + }); + const branchOption = testPage.getByTestId("task-launch-branch-picker-option-main"); + await expect(branchOption).toBeVisible({ timeout: 30_000 }); + await expect(branchOption).toBeInViewport(); + await branchOption.tap(); + await expect(testPage.getByTestId("task-launch-branch-picker-mobile")).toHaveCount(0); + + await expect + .poll( + async () => { + const { tasks } = await apiClient.listTasks(seedData.workspaceId); + return ( + tasks.find((candidate) => candidate.id === task.id)?.status_summary?.active_error ?? + null + ); + }, + { timeout: 60_000, message: "waiting for the mobile required refresh error to clear" }, + ) + .toBeNull(); + await expect + .poll( + async () => { + const { sessions } = await apiClient.listTaskSessions(task.id); + return sessions.some((item) => + ["RUNNING", "WAITING_FOR_INPUT", "IDLE", "COMPLETED"].includes(item.state), + ); + }, + { timeout: 60_000, message: "waiting for the mobile refresh retry to launch" }, + ) + .toBe(true); + + await assertNoDocumentHorizontalOverflow(testPage, "mobile required refresh recovery"); + await testPage.screenshot({ + path: testInfo.outputPath("required-refresh-recovery-mobile.png"), + fullPage: true, + }); + } finally { + restoreSeedRepositoryOrigin(seedData); } }); }); diff --git a/docs/decisions/2026-08-25-required-worktree-refresh-fails-closed.md b/docs/decisions/2026-08-25-required-worktree-refresh-fails-closed.md new file mode 100644 index 0000000000..57450e2390 --- /dev/null +++ b/docs/decisions/2026-08-25-required-worktree-refresh-fails-closed.md @@ -0,0 +1,83 @@ +# ADR-2026-08-25-required-worktree-refresh-fails-closed: Required Worktree Refresh Fails Closed + +**Status:** accepted +**Date:** 2026-08-25 +**Area:** backend, security, operations + +## Context + +A repository can require Kandev to pull before it creates a worktree. The +current worktree manager treats fetch and pull failures as warnings. It then +uses a local fallback ref and starts the agent. An authentication or network +failure can therefore produce a successful-looking task that edits an old copy +of the repository. The later push fails, after the agent has already spent time +on work based on stale state. + +Kandev also supports an explicit offline mode. A repository with +pull-before-worktree disabled can use local state without contacting a remote. +The design must preserve that choice and must preserve local-only commits. + +Provider-managed credentials and executor-inherited credentials have different +owners. A required refresh must use the route selected by the task Git policy. +It cannot force a managed HTTPS route over an executor-inherited SSH origin. + +## Decision + +Pull-before-worktree is an admission gate for every new or recreated worktree. +When the setting is enabled, Kandev starts no agent until the configured remote +refresh succeeds and the selected base ref is proven to include the fetched +remote base. + +A provider-managed checkout uses the backend's exact-scope credential refresh. +A host or executor checkout uses its reconciled origin and non-interactive Git +environment. A successful provider refresh marks remote sync as handled so the +worktree manager does not run a second unauthenticated fetch. + +Fetch failure stops preparation. After a successful fetch, Kandev preserves a +local ref only when it contains the fetched remote ref. It uses the remote ref +when the remote contains the local ref. Diverged refs and failed ancestry +checks stop preparation without changing either ref. + +Pull-before-worktree disabled remains the explicit offline opt-out. This path +can use available local refs and makes no freshness guarantee. + +Required-refresh errors use the existing durable task launch-error projection. +They identify the affected repository and failure class without exposing +credential material. + +## Consequences + +- Authentication, SSH, network, and timeout failures become visible before an + agent starts. +- Users do not lose agent time on a checkout that Kandev knows it failed to + refresh. +- Existing offline workflows continue when pull-before-worktree is disabled. +- Local-only commits remain available when the local branch contains current + remote state. +- Diverged branches require a user to reconcile history before retrying launch. +- The executor and worktree APIs become fallible at the base-refresh boundary. +- Multi-repository launch stops before runtime startup when one required + repository cannot refresh. + +## Alternatives Considered + +### Continue with a local ref and show a warning + +Rejected. A warning does not prevent the agent from producing changes against +known stale state. The failure can remain hidden until push. + +### Require remote access for every worktree + +Rejected. Kandev supports local and offline repositories. The existing setting +is the explicit user choice between required freshness and local availability. + +### Use a setup script or agent prompt as the gate + +Rejected. Setup-script failures are nonfatal, and an agent prompt runs after +worktree selection. Neither boundary can prevent launch on stale state. + +### Reset the local base to the fetched remote base + +Rejected. A reset can discard local-only commits. Kandev can select a safe +containing ref or stop without mutating user history. + diff --git a/docs/decisions/INDEX.md b/docs/decisions/INDEX.md index 6cb2820f36..7671c64600 100644 --- a/docs/decisions/INDEX.md +++ b/docs/decisions/INDEX.md @@ -206,3 +206,4 @@ Read individual ADRs for full context. Create new ones via `/record decision` or | 2026-08-24-opt-in-ssh-task-directory-reclamation | [Opt-in reclamation of remote SSH task directories](2026-08-24-opt-in-ssh-task-directory-reclamation.md) | accepted | backend, frontend, operations | 2026-08-24 | | 2026-08-24-agentctl-local-managed-runtime-cache-repair | [Run cache repair where npm runs](2026-08-24-agentctl-local-managed-runtime-cache-repair.md) | accepted | backend, agentctl, protocol, security | 2026-08-24 | | 2026-08-24-unified-fork-approval-label | [Use One Maintainer Approval Label for Contributor PR Automation](2026-08-24-unified-fork-approval-label.md) | accepted | infra, workflow, security | 2026-08-24 | +| 2026-08-25-required-worktree-refresh-fails-closed | [Required Worktree Refresh Fails Closed](2026-08-25-required-worktree-refresh-fails-closed.md) | accepted | backend, security, operations | 2026-08-25 | diff --git a/docs/plans/worktree-refresh-fail-closed/plan.md b/docs/plans/worktree-refresh-fail-closed/plan.md new file mode 100644 index 0000000000..0d35bb35eb --- /dev/null +++ b/docs/plans/worktree-refresh-fail-closed/plan.md @@ -0,0 +1,170 @@ +--- +created: 2026-08-25 +status: implemented +requirements: + - REQ-WORKSPACES-WORKTREE-BASE-REFRESH-001 +system_design: + - ../../specs/workspaces/system-design/worktree-base-refresh.md +legacy_specs: [] +--- + +# Implementation Plan: Fail-Closed Worktree Refresh + +## Overview + +Make a required worktree refresh stop task launch when remote state cannot be +verified. The implementation first routes each repository through the correct +credential boundary. It then makes worktree fetch and base-ref selection +fallible. The final work verifies durable launch errors on desktop and mobile +and updates the public Git guidance. + +## Confirmed root cause + +- `internal/worktree.Manager.pullBaseBranch` logs fetch failure and returns the + requested local ref through `handleFetchFallback`. +- `pullCurrentBranchOrFallback` also converts pull failure into a successful + local or remote ref. It does not reject diverged refs. +- `internal/repoclone.Cloner` treats fetch failure as nonfatal when it finds an + existing cached checkout through an `Ensure*` path. +- `executor.resolveTaskRepoInfoForSession` runs an exact-scope strict refresh + only for plugin-managed repositories. Other managed providers can reach the + worktree manager without a successful authenticated refresh. +- The repository setting is presented as “Always pull before creating a new + worktree.” Continuing from a stale ref violates this user contract. +- Existing fallback tests confirm that the current behavior is intentional, + not an isolated command error. + +## Scope + +### In scope + +- Treat pull-before-worktree as a required remote-refresh gate for new and + recreated worktrees. +- Select a strict refresh route that preserves managed-provider credentials and + executor-inherited Git transport. +- Stop preparation on fetch failure, missing fetched base, divergent refs, or + uncertain ancestry. +- Preserve offline behavior when pull-before-worktree is disabled. +- Preserve a local base that contains the fetched remote base. +- Stop multi-repository launch before agent startup when one required refresh + fails. +- Project a credential-safe, repository-specific launch error through the + existing task error surface. +- Document SSH setup and the fail-closed behavior in the public Git guidance. + +### Out of scope + +- Making setup scripts fatal. +- Changing agent Git transport after launch. +- Refreshing an existing worktree that Kandev reuses without recreation. +- Resetting, merging, rebasing, or deleting user refs. +- Adding a new recovery action or user-interface layout. +- Adding a background refresh worker. + +## Technical approach + +The executor will resolve the task Git credential policy before lifecycle +preparation. A backend-managed provider checkout will pass a deferred strict +`repoclone` refresh callback with the same provider-specific credentials as +clone. The callback runs only when a worktree is materialized or recreated; a +valid reusable worktree bypasses it. An executor-inherited checkout will retain +its reconciled origin and enter the strict worktree-manager fetch path. Only a +completed refresh can set `RemoteSyncHandled`. + +The worktree manager will return `(ref, error)` from required sync and +base-selection functions. Fetch failure will propagate instead of returning a +fallback ref. After a successful fetch, ancestry checks will select a ref only +when one ref contains the other. Divergence and an uncertain check will return +an error without changing either branch. + +Lifecycle and executor code will retain repository identity while they wrap +the error. Existing task launch-failure persistence will project the failure. +No database or WebSocket schema change is planned. + +## Work orders + +- [completed] [Task 01: Route Required Repository Refresh](task-01-route-required-refresh.md) +- [completed] [Task 02: Reject Stale Worktree Fallbacks](task-02-reject-stale-fallbacks.md) +- [completed] [Task 03: Project Refresh Launch Failures](task-03-project-refresh-launch-failures.md) +- [completed] [Task 04: Document Required Repository Refresh](task-04-document-required-refresh.md) + +## Dependency order + +```text +Task 01 -> Task 02 -> Task 03 -> Task 04 +``` + +The package is sequential. Task 02 consumes the refresh-route contract from +Task 01. Task 03 verifies the combined launch boundary. Task 04 documents the +verified behavior. + +## Verification strategy + +- Unit tests prove credential-route selection and exact-scope managed refresh. +- Worktree integration tests prove fetch failure, comparable refs, divergence, + timeout, and the disabled-policy path. +- Orchestrator tests prove repository-specific failure and no agent startup for + multi-repository tasks. +- Existing launch-failure E2E tests prove the durable error on desktop and + mobile after reload. +- Public documentation validators prove links, commands, and headings. +- Backend race tests and lint cover the combined affected packages. + +## Verification results + +- Worktree, lifecycle, orchestrator, and executor race suites passed, including + 6,049 tests in the combined affected-package run. +- Backend build, E2E plugin packaging, and production E2E build passed. +- The launch-failure recovery spec passed all 3 Chromium tests and both + `mobile-chrome` tests. +- Backend lint, public-doc validators, specification lint, and `git diff + --check` passed. + +### Review remediation results + +- Rebuilt the PR branch from the focused worktree-refresh, launch-failure, and + required fixture-origin commits; unrelated automation and broad E2E timing + changes are no longer part of the branch. +- Added Create and recreate regressions for normal managed branches and + `origin/pr/` refs with a pre-existing local branch behind the refreshed + source. The refreshed source is now passed to `git worktree add -B` after + ancestry selection. +- Reverified 2,972 affected backend race tests, zero backend lint issues, 3/3 + desktop launch-failure tests, and 2/2 mobile launch-failure tests with strict + no-retry E2E execution. + +### PR fixup remediation results + +- Reproduced the six failed E2E shard causes at the focused branch head: local + origin fixture collisions, ambiguous folder and branch locators, missing + offline refresh opt-outs, shared Git history, and a GitLab setup that needed + to remain offline. +- Kept the remediation scoped to the refresh contract: exact folder/branch + locators, explicit offline options for local fixtures, and a disposable + repository for the PR-deduplication history assertion. No generic timing + changes were restored. +- With retries disabled, the affected desktop command passed 21/21 tests, the + affected mobile command passed 2/2 tests, and the complete Git Changes Panel + file passed 21/21 tests. Prettier and `git diff --check` also passed. + +## Risks + +- A managed refresh that ignores task Git policy can replace an + executor-inherited SSH origin with HTTPS. Route selection needs explicit + policy tests. +- A naive remote-first rule can hide local-only commits. The ancestry table in + the system design is the required selection rule. +- Existing tests currently require stale fallback behavior. Replace those + assertions with fail-closed tests rather than removing coverage. +- `executor_resume.go` is active code with broad resume coverage. Keep the + route helper narrow and run fresh-launch and resume suites together. +- Error wrapping can leak raw Git output. Use existing redaction and bounded + launch-error helpers. +- A multi-repository preparation attempt can refresh earlier repositories + before a later one fails. This is safe on-disk cache progress, but agent + startup must remain atomic. + +## Package handoff + +Implementation followed the sequential TDD work orders. Each work order and +the plan status are updated after its verification commands passed. diff --git a/docs/plans/worktree-refresh-fail-closed/task-01-route-required-refresh.md b/docs/plans/worktree-refresh-fail-closed/task-01-route-required-refresh.md new file mode 100644 index 0000000000..fc5f3e526a --- /dev/null +++ b/docs/plans/worktree-refresh-fail-closed/task-01-route-required-refresh.md @@ -0,0 +1,115 @@ +--- +id: "01-route-required-refresh" +title: "Route required repository refresh" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-WORKSPACES-WORKTREE-BASE-REFRESH-001 +acceptance_criteria: + - AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.2 + - AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.3 +system_design: + - ../../specs/workspaces/system-design/worktree-base-refresh.md +--- + +# Task 01: Route Required Repository Refresh + +## Summary + +Choose the strict refresh boundary from repository provider and task Git +credential policy. Preserve executor-inherited SSH origins and use exact-scope +backend credentials only for managed provider routes. + +## In scope + +- Add failing tests for GitHub managed mode, GitHub executor mode, plugin + providers, GitLab, and Azure DevOps. +- Resolve the route before lifecycle preparation in initial launch and resume; + defer a managed refresh until the lifecycle actually materializes or + recreates a worktree so valid reuse performs no refresh. +- Generalize the existing plugin strict-refresh helper into a + provider-credential-aware helper. +- Reuse the clone credential path for strict GitLab and Azure DevOps refresh. +- Set `RemoteSyncHandled` only after the deferred strict provider refresh + succeeds. +- Leave `PullBeforeWorktree` enabled for executor-inherited and local routes so + the worktree manager owns their fetch. +- Preserve the SSH origin selected by `gitHubCheckoutOriginURL` in executor + mode. + +## Out of scope + +- Worktree fallback and ancestry behavior. +- Task error projection or frontend changes. +- New credential sources, provider settings, or schema. +- Changing repository clone behavior when no existing checkout is present. + +## Acceptance + +- A managed provider checkout runs one authenticated strict refresh when a + worktree is materialized or recreated, and does not run an unauthenticated + follow-up fetch. A valid reusable worktree bypasses the refresh. +- An executor-inherited GitHub checkout retains its reconciled origin and + delegates required fetch to the worktree manager. +- A strict provider refresh error returns from initial launch and resume with + exact task, session, and repository scope. + +## Verification + +Start with a regression that proves a managed first-party checkout stops after +its authenticated refresh fails. Confirm that it fails before the production +change. Then run: + +```bash +# From apps/backend: +rtk go test ./internal/repoclone ./internal/orchestrator/executor -run 'RefreshWorkspaceRepository|ResolveTaskRepoInfo|CloneTransport' -race +rtk go test ./internal/orchestrator/executor/... -run 'Fresh|Resume|Repository' -race +``` + +## Files likely touched + +- `apps/backend/internal/orchestrator/executor/executor.go` +- `apps/backend/internal/orchestrator/executor/executor_resume.go` +- `apps/backend/internal/orchestrator/executor/executor_resume_clone_transport_test.go` +- `apps/backend/internal/repoclone/clone.go` +- `apps/backend/internal/repoclone/clone_auth_test.go` + +## Dependencies + +None. + +## Risks + +- Provider-specific clone credentials can drift from refresh credentials. Use + the same resolver and redaction path for both operations. +- Executor mode must not call a backend-managed GitHub refresh because that can + rewrite the SSH origin to HTTPS. +- Exact credential scope must include task ID, session ID, and repository ID. + +## Parallelism + +`sequential` + +## Inputs + +- `REQ-WORKSPACES-WORKTREE-BASE-REFRESH-001`. +- `docs/specs/workspaces/system-design/worktree-base-refresh.md`. +- `ADR-2026-07-27-task-git-credential-policy`. +- Existing plugin strict-refresh tests and GitHub origin-reconciliation tests. + +## Results + +- Added managed-provider refresh routing for GitHub, GitLab, Azure DevOps, and + plugin repositories while preserving executor-inherited GitHub transport. +- Deferred managed refresh until worktree materialization or recreation, while + preserving exact repository identity through multi-repository preparation and + bypassing refresh for valid worktree reuse. +- Added authenticated pull-request-head fetching into `origin/pr/` for + managed checkouts, including fork pull requests. +- Added strict Azure DevOps basic-auth refresh support that reuses the clone + credential boundary and validates the exact workspace checkout path. +- Verified with: + - `rtk go test ./internal/repoclone ./internal/orchestrator/executor -run 'RefreshWorkspaceRepository|ResolveTaskRepoInfo|CloneTransport' -race` + - `rtk go test ./internal/orchestrator/executor/... -run 'Fresh|Resume|Repository' -race` diff --git a/docs/plans/worktree-refresh-fail-closed/task-02-reject-stale-fallbacks.md b/docs/plans/worktree-refresh-fail-closed/task-02-reject-stale-fallbacks.md new file mode 100644 index 0000000000..22ad6424d5 --- /dev/null +++ b/docs/plans/worktree-refresh-fail-closed/task-02-reject-stale-fallbacks.md @@ -0,0 +1,132 @@ +--- +id: "02-reject-stale-fallbacks" +title: "Reject stale worktree fallbacks" +status: done +wave: 2 +depends_on: + - "01-route-required-refresh" +plan: "plan.md" +requirements: + - REQ-WORKSPACES-WORKTREE-BASE-REFRESH-001 +acceptance_criteria: + - AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.1 + - AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.3 + - AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.4 + - AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.5 +system_design: + - ../../specs/workspaces/system-design/worktree-base-refresh.md +--- + +# Task 02: Reject Stale Worktree Fallbacks + +## Summary + +Make required worktree sync return errors. Select a base only when it is proven +to contain current fetched remote state and any preserved local commits. + +## In scope + +- Add failing worktree tests for authentication failure, network failure, + timeout, missing remote base, diverged refs, and uncertain ancestry. +- Change `pullBaseBranch` and its required-sync helpers to return errors. +- Propagate sync errors through `resolveBaseRefWithFallback`, `Create`, and + recreation callers. +- Replace stale-ref success events with failed sync progress events. +- Select the remote ref when it contains the local ref. +- Select the local ref when it contains the fetched remote ref. +- Apply the selected refreshed ref when materializing an existing managed + checkout branch during Create or recreate, including managed PR refs. +- Preserve local-only behavior when `PullBeforeWorktree` is false. +- Keep Git commands non-interactive and bounded by the existing admission and + timeout contracts. + +## Out of scope + +- Credential-route selection from Task 01. +- Reset, merge, rebase, or automatic divergence recovery. +- Refreshing an active reusable worktree without recreating it. +- Task error rendering. + +## Acceptance + +- Required fetch failure returns an error and creates no worktree from a local + fallback ref. +- Comparable local and remote refs select the containing ref without losing + commits. Diverged or unverified refs return an error without mutation. +- Managed checkout branches use the refreshed remote start point when the local + branch is behind it, for both normal branches and `origin/pr/` refs during + Create and recreate. +- Pull-before-worktree disabled retains current offline local behavior. + +## Verification + +Change one current fallback test to expect an error and confirm that it fails +before the production change. Then run: + +```bash +# From apps/backend: +rtk go test ./internal/worktree -run 'PullBaseBranch|RequiredRefresh|BaseRef|Fallback' -race +rtk go test ./internal/worktree/... ./internal/agent/runtime/lifecycle/... -race +``` + +## Files likely touched + +- `apps/backend/internal/worktree/manager_git.go` +- `apps/backend/internal/worktree/manager_lifecycle.go` +- `apps/backend/internal/worktree/manager_pull_fallback_test.go` +- `apps/backend/internal/worktree/manager_fetch_retry_fallback_test.go` +- `apps/backend/internal/worktree/manager_test.go` +- `apps/backend/internal/agent/runtime/lifecycle/env_preparer_worktree_test.go` + +## Dependencies + +- Task 01 defines when `RemoteSyncHandled` is valid and when the manager owns + the required fetch. + +## Risks + +- Returning errors changes several internal method signatures. Update all + fallback and live-default call sites before broad tests. +- A boolean ancestry helper cannot distinguish a negative result from a failed + probe. Return a typed result or error so uncertain ancestry fails safely. +- Fallback base-branch recovery must not bypass a required refresh. + +## Parallelism + +`sequential` + +## Inputs + +- `AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.1` and + `AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.3` through `.5`. +- The base-ref selection table in the system design. +- Existing unpushed-commit preservation tests. + +## Results + +- Required worktree refresh and checkout/recreate fetch failures now propagate + instead of selecting stale local fallbacks. +- Base-ref selection accepts only verified containing refs and rejects diverged + or uncertain ancestry without mutation. +- Managed checkout preparation now compares local and refreshed normal/PR refs, + preserves a local-ahead branch, and passes a refreshed remote start point to + Create and recreate instead of checking out a stale local branch. +- Disabled pull-before-worktree behavior remains available for offline local + workflows. +- Verified with: + - `rtk go test ./internal/worktree -run 'PullBaseBranch|RequiredRefresh|BaseRef|Fallback' -race` + - `rtk go test ./internal/worktree/... ./internal/agent/runtime/lifecycle/... -race` + - `rtk go test ./internal/worktree -run 'Test(CreateWorktree|Recreate)_ManagedRefreshUsesRemote.*Behind' -count=1` + - `rtk go test ./internal/worktree ./internal/orchestrator/executor ./internal/agent/runtime/lifecycle ./internal/repoclone -race -count=1` + - `rtk make -C apps/backend lint` + - `rtk pnpm e2e:run --project chromium tests/task/launch-failure-recovery.spec.ts` + - `rtk pnpm e2e:run --project mobile-chrome tests/task/mobile-launch-failure-recovery.spec.ts` + +PR fixup remediation: + +- Reproduced all six failed shard causes and fixed only refresh-contract fixture + and locator issues. Offline local repositories now opt out explicitly, and + the Git Changes Panel PR-deduplication case uses a disposable repository. +- With `--retries=0`, the affected desktop E2E command passed 21/21 tests, the + affected mobile command passed 2/2 tests, and the complete Git Changes Panel + file passed 21/21 tests. Prettier and `git diff --check` passed. diff --git a/docs/plans/worktree-refresh-fail-closed/task-03-project-refresh-launch-failures.md b/docs/plans/worktree-refresh-fail-closed/task-03-project-refresh-launch-failures.md new file mode 100644 index 0000000000..3daf27f18d --- /dev/null +++ b/docs/plans/worktree-refresh-fail-closed/task-03-project-refresh-launch-failures.md @@ -0,0 +1,120 @@ +--- +id: "03-project-refresh-launch-failures" +title: "Project refresh launch failures" +status: done +wave: 3 +depends_on: + - "02-reject-stale-fallbacks" +plan: "plan.md" +requirements: + - REQ-WORKSPACES-WORKTREE-BASE-REFRESH-001 +acceptance_criteria: + - AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.6 + - AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.7 +system_design: + - ../../specs/workspaces/system-design/worktree-base-refresh.md +--- + +# Task 03: Project Refresh Launch Failures + +## Summary + +Carry a required-refresh failure through lifecycle and task launch boundaries. +Prove that Kandev starts no agent and shows the durable repository-specific +error on desktop and mobile. + +## In scope + +- Add a failing multi-repository launch test where the second repository cannot + complete required refresh. +- Preserve exact repository identity while wrapping refresh errors. +- Use the existing generic typed launch-failure category unless a focused test + proves a new category is necessary for a valid recovery action. +- Bound and redact error detail through existing launch-error helpers. +- Prove that no agent runtime starts after a required refresh error. +- Extend existing desktop and mobile launch-failure recovery E2E fixtures with + a stale-origin refresh failure and reload assertion. +- Prove that normal retry succeeds after the remote and credentials are fixed. + +## Out of scope + +- A new recovery action, WebSocket schema, or task-error store. +- New UI components, responsive layout, or translations. +- Partial agent launch for multi-repository tasks. + +## Acceptance + +- A required refresh error prevents runtime startup and names the exact failed + repository without exposing credentials or an authenticated URL. +- The active task error survives reload and renders through the existing task + error surface on desktop and mobile. +- A later normal retry reruns refresh and can launch after the underlying Git + problem is corrected. + +## Verification + +Start with the multi-repository no-start regression and confirm it fails before +error propagation is complete. Then run: + +```bash +# From apps/backend: +rtk go test ./internal/orchestrator/... ./internal/orchestrator/executor/... -run 'LaunchFailure|RepositoryRefresh|MultiRepo' -race +rtk go test ./internal/orchestrator/... ./internal/worktree/... -race + +# From the repository root: +rtk make -C apps/backend build +rtk make -C apps/backend e2e-plugin-package + +# From apps/web: +rtk pnpm run build:e2e +rtk pnpm e2e:raw --project=chromium e2e/tests/task/launch-failure-recovery.spec.ts +rtk pnpm e2e:raw --project=mobile-chrome e2e/tests/task/mobile-launch-failure-recovery.spec.ts + +# From the repository root: +rtk make -C apps/backend lint +``` + +## Files likely touched + +- `apps/backend/internal/orchestrator/executor/executor_resume.go` +- `apps/backend/internal/orchestrator/executor/executor_execute.go` +- `apps/backend/internal/orchestrator/executor/executor_launch_failure_classification_test.go` +- `apps/backend/internal/orchestrator/task_launch_failure_test.go` +- `apps/web/e2e/tests/task/launch-failure-recovery.spec.ts` +- `apps/web/e2e/tests/task/mobile-launch-failure-recovery.spec.ts` + +## Dependencies + +- Task 02 provides the required-refresh error from the worktree boundary. + +## Risks + +- Raw Git output can contain a credential-bearing URL. Test redaction with a + synthetic secret and assert that it is absent from logs and projected detail. +- An E2E fixture that fails clone instead of refresh does not prove this bug. + Seed a valid cached checkout, make its origin fail, and request a new + worktree. +- Backend E2E artifacts must be rebuilt before Playwright starts. + +## Parallelism + +`sequential` + +## Inputs + +- `AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.6` and `.7`. +- `REQ-TASKS-TASK-LAUNCH-FAILURE-RECOVERY-001` as an adjacent task-system + contract. +- Existing desktop and mobile launch-failure recovery E2E tests. + +## Results + +- Multi-repository preparation preserves exact repository identity and rolls + back preparation without starting an agent runtime. +- Launch error details use existing credential-safe sanitization while keeping + repository-specific recovery actions. +- Desktop and mobile launch-failure fixtures cover a valid cached checkout with + a failing origin, reload persistence, and successful retry after recovery. +- Verified with the focused and full orchestrator/worktree race suites, backend + build, E2E plugin package, production E2E build, Chromium and mobile-chrome + specs, and backend lint. diff --git a/docs/plans/worktree-refresh-fail-closed/task-04-document-required-refresh.md b/docs/plans/worktree-refresh-fail-closed/task-04-document-required-refresh.md new file mode 100644 index 0000000000..b154c0c2c3 --- /dev/null +++ b/docs/plans/worktree-refresh-fail-closed/task-04-document-required-refresh.md @@ -0,0 +1,105 @@ +--- +id: "04-document-required-refresh" +title: "Document required repository refresh" +status: done +wave: 4 +depends_on: + - "03-project-refresh-launch-failures" +plan: "plan.md" +requirements: + - REQ-WORKSPACES-WORKTREE-BASE-REFRESH-001 +acceptance_criteria: + - AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.1 + - AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.2 + - AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.3 +system_design: + - ../../specs/workspaces/system-design/worktree-base-refresh.md +--- + +# Task 04: Document Required Repository Refresh + +## Summary + +Document the SSH configuration boundary, required-refresh failure behavior, +offline opt-out, and safe retry procedure in the public Git guidance. + +## In scope + +- Update executor documentation to state where host and remote executors read + Git and SSH credentials. +- Update Git operations documentation to explain that pull-before-worktree + stops launch when refresh fails. +- Document `gh config set git_protocol ssh --host github.com` for a host that + uses executor-inherited GitHub access. +- State that Kandev must restart after host GitHub protocol changes so managed + checkout origins are reconciled before the next task launch. +- Explain how to disable pull-before-worktree for an intentional offline local + workflow. +- Add credential-safe troubleshooting steps for authentication and network + errors. + +## Out of scope + +- New settings, screenshots, or UI copy. +- Provider-specific SSH installation guides beyond the existing supported + configuration surface. +- Documentation for setup-script failure policy. + +## Acceptance + +- Public docs distinguish workspace automation identity from task Git + credential policy and selected Git transport. +- Public docs state that a required refresh failure stops launch and does not + use a stale local fallback. +- Commands, internal links, and headings pass the public documentation + validators. + +## Verification + +```bash +# From the repository root: +rtk node --test scripts/validate-public-docs.test.mjs +rtk node scripts/validate-public-docs.mjs +rtk python3 scripts/lint-spec-files.py --all +rtk git diff --check +``` + +## Files likely touched + +- `docs/public/executors.md` +- `docs/public/git-operations.md` +- `docs/plans/worktree-refresh-fail-closed/plan.md` +- `docs/plans/worktree-refresh-fail-closed/task-04-document-required-refresh.md` + +## Dependencies + +- Task 03 verifies the exact launch and retry behavior that the public docs + describe. + +## Risks + +- Documentation can imply that a named GitHub CLI automation connection makes + SSH available to every executor. State the separate task policy and runtime + boundary explicitly. +- Remote executors need their own SSH key and known-host configuration. Do not + describe host configuration as portable to Docker or SSH executors. + +## Parallelism + +`sequential` + +## Inputs + +- Verified behavior from Tasks 01 through 03. +- `ADR-2026-07-27-task-git-credential-policy`. +- `docs/public/executors.md` and `docs/public/git-operations.md`. + +## Results + +- Updated [Executors](../../public/executors.md) with workspace automation versus + task Git credential boundaries, executor-local SSH requirements, host GitHub + SSH configuration, and restart guidance. +- Updated [Git Operations](../../public/git-operations.md) with required-refresh + failure behavior, the offline opt-out, and credential-safe troubleshooting. +- Verified with public-doc unit and live validators, specification lint, and + `git diff --check`. diff --git a/docs/public/executors.md b/docs/public/executors.md index 192e6e03f3..f6900918ab 100644 --- a/docs/public/executors.md +++ b/docs/public/executors.md @@ -171,11 +171,34 @@ ownership check and does not change **Inherit executor Git credentials** mode. A already open keeps the environment from its launch; reopen it after a new session launch, resume, or a Git credential-policy change. +### Workspace automation identity and task Git transport + +The workspace GitHub connection is the automation identity Kandev uses for provider operations, +such as finding or creating a pull request. The task Git credential policy is separate: it decides +whether Git uses Kandev-managed credentials or the selected executor's own Git and SSH setup. +Changing the workspace connection does not install an SSH key in an executor. + +For **Inherit executor Git credentials**, configure the executor where Git runs. Local and Worktree +use the Kandev host's Git configuration, SSH agent, known-hosts file, and credential helpers. +Docker uses the credentials in the container, while SSH and Sprites use the credentials configured +on the remote environment. A host `gh` login or `~/.ssh` file is not automatically available in a +Docker, SSH, or Sprite executor. + +To make host GitHub CLI operations prefer SSH, run this on the Kandev host: + +```bash +gh config set git_protocol ssh --host github.com +``` + +Restart Kandev after changing the host GitHub protocol. The restart lets Kandev reconcile managed +repository origins before the next task launch. For an SSH or Docker executor, configure the +equivalent GitHub SSH access, known-hosts entry, and agent or key on that executor instead. + ## Worktree Worktree creates a dedicated host Git worktree and runs the standalone `agentctl` service against it. It separates branches and files between tasks, but the process still has the Kandev user's host permissions, network access, and readable credentials. -Repository settings control base branch, branch naming, pull-before-create, repository setup/cleanup scripts, and optional copies of ignored files. Copy ignored files narrowly: `.env` and similar files often contain production secrets. Multi-repository tasks receive one materialized worktree per attachment; use the per-repository setup scripts because the profile-level prepare script is currently skipped for that path. +Repository settings control base branch, branch naming, pull-before-create, repository setup/cleanup scripts, and optional copies of ignored files. With **Always pull before creating a new worktree** enabled, the base refresh is required for a new or recreated worktree. An authentication, network, timeout, missing-ref, divergent-ref, or uncertain-ancestry failure stops the launch; Kandev does not use a stale local fallback. Disable this setting only for an intentional offline local workflow. Copy ignored files narrowly: `.env` and similar files often contain production secrets. Multi-repository tasks receive one materialized worktree per attachment; use the per-repository setup scripts because the profile-level prepare script is currently skipped for that path. Normal stop keeps the task environment available. Task deletion or **Reset Environment** removes the tracked worktree when configured to clean worktrees. Preserve or push valuable changes first; see [Git Operations](git-operations.md). @@ -183,6 +206,7 @@ Typical failures: - dirty or conflicting source repository state; - base branch missing locally or remotely; +- required refresh credentials, network access, or remote ancestry cannot be verified; - worktree path already registered in Git metadata; - setup dependencies absent on the host; - repository cleanup failure leaving a stale worktree. diff --git a/docs/public/git-operations.md b/docs/public/git-operations.md index 567d416c5e..51b0dfa595 100644 --- a/docs/public/git-operations.md +++ b/docs/public/git-operations.md @@ -18,7 +18,7 @@ Use the task's **Changes** panel to inspect, stage, discard, commit, push, reset ## Prerequisites and trust boundary -The repository must be a valid Git checkout in the executor workspace and the session's `agentctl` must be reachable. Remote commands use the remote named `origin`; configure its URL and credentials before relying on Pull, Push, or change-request creation. Rebase and Merge use `origin` when it exists, or a local base branch when it does not. +The repository must be a valid Git checkout in the executor workspace and the session's `agentctl` must be reachable. Remote commands use the remote named `origin`; configure its URL and credentials in the executor where the command runs before relying on Pull, Push, or change-request creation. Rebase and Merge use `origin` when it exists, or a local base branch when it does not. The workspace's provider automation identity does not replace the task's Git credential policy or executor-local SSH setup; see [Executors](executors.md#workspace-automation-identity-and-task-git-transport). Git commands from the agent shell and Git actions from the **Changes** panel use different permission paths. The Changes panel sends its Git action through Kandev to `agentctl`. It does not run the action inside the agent shell. The agent shell remains subject to the selected agent's permission mode. @@ -79,9 +79,14 @@ For a new task branch, the repository default template is: feature/{title}-{suffix} ``` -`{title}` is an ASCII-safe, lower-case task-title slug and `{suffix}` is a short collision-avoidance value. Repository settings can change the template. When `pull_before_worktree` is omitted it defaults to `true`: Kandev best-effort fetches the base branch before creating the worktree. The public configuration defaults both fetch and fast-forward pull timeouts to 60 seconds. An authentication, network, or timeout failure can fall back to an available local or remote-tracking ref with a visible warning; a base branch that cannot be resolved, including its configured fallback, stops creation. +`{title}` is an ASCII-safe, lower-case task-title slug and `{suffix}` is a short collision-avoidance value. Repository settings can change the template. When `pull_before_worktree` is omitted it defaults to `true`: Kandev must refresh and verify the base branch before creating or recreating the worktree. The public configuration defaults both fetch and fast-forward pull timeouts to 60 seconds. An authentication, network, timeout, missing-ref, divergent-ref, or uncertain-ancestry failure stops task preparation and records a repository-specific launch error. Kandev does not create the worktree from a stale local or remote-tracking fallback. -When a task opens an existing branch or GitHub PR, Kandev fetches that branch; for a numbered GitHub PR it can fetch `refs/pull/NUMBER/head`, including fork PRs. If the intended branch is already checked out in another worktree, the new worktree uses a suffixed local branch and tracks the original `origin` branch when available. If remote fetch fails but the local branch exists, Kandev can continue with that possibly stale branch and reports the fallback. +If the repository is intentionally offline, open its workspace repository settings and disable +**Always pull before creating a new worktree**. This preserves the local workflow, but it also +opts out of the freshness guarantee and allows the task to use the local base state. Re-enable the +setting before relying on remote changes for later task launches. + +When a task opens an existing branch or GitHub PR, Kandev fetches that branch; for a numbered GitHub PR it can fetch `refs/pull/NUMBER/head`, including fork PRs. If the intended branch is already checked out in another worktree, the new worktree uses a suffixed local branch and tracks the original `origin` branch when available. The required-refresh rule still applies before that new worktree is created. Tasks created without an initial title can expose the one-shot `set_task_title_kandev` handoff when **Settings → General → Task Actions → Agent-generated task titles** is enabled. After the owning @@ -235,7 +240,8 @@ Before deleting a task or performing a hard reset, commit and push anything you ## Troubleshooting - **No agent/client available:** launch or prepare the session and confirm its executor is healthy. Workspace Git actions can reconstruct runtime control after a backend restart, but still need a valid task environment. -- **Remote/authentication error:** test `git fetch origin` inside the same executor workspace. Verify SSH agent forwarding, token/credential helper availability, remote URL, DNS, and firewall access there. +- **Remote/authentication error during worktree preparation:** test `git fetch origin` inside the same executor workspace. Verify the remote URL, SSH agent or key, known-hosts entry, token or credential-helper availability, DNS, and firewall access there. Do not paste command output containing tokens or authenticated URLs into a task or issue. +- **Host GitHub SSH setup:** for host-based Git credentials, run `gh config set git_protocol ssh --host github.com`, restart Kandev, and retry the task. For Docker, SSH, or Sprites, configure the same Git and SSH access in that executor instead of relying on the host. - **Merge or Rebase without `origin`:** the selected base branch must exist locally. If it does not, Kandev returns `base branch "BASE" does not exist locally` before changing history. If `origin` exists, Kandev does not fall back to a local branch after a fetch or authentication error. - **Pull fetched the wrong branch:** Kandev always uses `origin` and, once any upstream exists, the current local branch name. Align local and remote branch names or use an explicit terminal command. - **Rebase failed but no rebase remains:** detected rebase conflicts are auto-aborted. Use the returned `conflict_files`, resolve with a manual workflow, or merge instead. diff --git a/docs/specs/workspaces/README.md b/docs/specs/workspaces/README.md index 8842d001ca..fbc01781d3 100644 --- a/docs/specs/workspaces/README.md +++ b/docs/specs/workspaces/README.md @@ -41,6 +41,7 @@ Git state. - [Repository Sets](requirements/repository-sets.md) - [Copy and Move Secrets Between Scopes](requirements/secret-scope-transfer.md) - [Workspace Base-Branch Propagation](requirements/workspace-base-branch-propagation.md) +- [Worktree Base Refresh](requirements/worktree-base-refresh.md) - [Worktree Branch Templates](requirements/worktree-branch-templates.md) ### System design @@ -49,6 +50,7 @@ Git state. - [Improve Kandev](system-design/improve-kandev.md) - [Copy and Move Secrets Between Scopes](system-design/secret-scope-transfer.md) +- [Worktree Base Refresh](system-design/worktree-base-refresh.md) ## Migration record diff --git a/docs/specs/workspaces/requirements/worktree-base-refresh.md b/docs/specs/workspaces/requirements/worktree-base-refresh.md new file mode 100644 index 0000000000..ab16b62b7c --- /dev/null +++ b/docs/specs/workspaces/requirements/worktree-base-refresh.md @@ -0,0 +1,80 @@ +--- +status: draft +system: workspaces +created: 2026-08-25 +owners: + - kandev +--- + +# Worktree Base Refresh Requirements + +## Overview + +The workspace system owns repository state and the base ref used to create a +task worktree. When a workspace enables pull-before-worktree behavior, users +expect each new or recreated worktree to start from verified remote state. +Kandev must not start an agent on an unverified local copy after a required +refresh fails. + +## Terminology + +- **Required refresh:** The remote synchronization gate enabled by a + repository's pull-before-worktree setting. +- **Fetched remote base:** The remote-tracking ref produced by the successful + fetch for the requested base branch. +- **Comparable refs:** Two refs where either ref contains every commit in the + other ref. + +## Requirements + +### REQ-WORKSPACES-WORKTREE-BASE-REFRESH-001: Verified Worktree Base + +**Intent:** Prevent an agent from starting on stale repository state when the +workspace requires a remote refresh before worktree creation. + +**User story:** As a workspace user, I want a required repository refresh to +stop task launch when it fails, so that an agent cannot edit and push from an +old copy without my knowledge. + +#### Acceptance criteria + +- **AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.1:** When pull-before-worktree is + disabled, Kandev shall allow a new or recreated worktree to use available + local refs without requiring remote access. +- **AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.2:** When pull-before-worktree is + enabled, Kandev shall complete the remote refresh through the repository's + configured transport and credential route before it starts an agent in a new + or recreated worktree. +- **AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.3:** When a required fetch fails + because of authentication, network access, timeout, cancellation, or another + Git error, Kandev shall stop worktree preparation and shall not use a stale + local fallback ref. +- **AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.4:** When the required fetch + succeeds, Kandev shall choose a start ref that contains the fetched remote + base. It can preserve local-only commits only when the local base also + contains the fetched remote base. +- **AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.5:** When the local base and fetched + remote base have diverged, or Kandev cannot prove their ancestry, Kandev + shall stop worktree preparation without resetting, deleting, or hiding either + ref. +- **AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.6:** When required refresh fails + for any repository in a multi-repository task, Kandev shall not start the + task agent and shall identify the affected repository in a credential-safe + launch error. +- **AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.7:** When required refresh fails + during initial launch or a resume that creates or recreates a worktree, + Kandev shall expose the failure through the existing durable task launch-error + projection. + +## Out of scope + +- Making setup-script failures fatal. +- Refreshing a worktree that Kandev reuses without creating or recreating it. +- Adding a background repository-refresh service. +- Automatically resetting, rebasing, merging, or deleting local refs. +- Changing agent Git transport after the agent starts. + +## System design + +- [Worktree Base Refresh System Design](../system-design/worktree-base-refresh.md) + diff --git a/docs/specs/workspaces/system-design/worktree-base-refresh.md b/docs/specs/workspaces/system-design/worktree-base-refresh.md new file mode 100644 index 0000000000..7dd02d453a --- /dev/null +++ b/docs/specs/workspaces/system-design/worktree-base-refresh.md @@ -0,0 +1,195 @@ +--- +status: draft +system: workspaces +requirements: + - REQ-WORKSPACES-WORKTREE-BASE-REFRESH-001 +--- + +# Worktree Base Refresh System Design + +## Purpose and boundaries + +The workspace system owns the repository checkout, remote state, and base ref +used to create a worktree. This design makes `repositories.pull_before_worktree` +an admission gate for new and recreated worktrees. + +The task system continues to own session launch state and error presentation. +It consumes preparation errors through the existing launch-failure contract in +[Task Launch Failure Recovery](../../tasks/system-design/task-launch-failure-recovery.md). +The integration system continues to own provider credentials. The executor +continues to own credentials and SSH configuration inherited from its runtime. + +This design does not refresh a worktree that Kandev can reuse without +recreation. It also does not change Git commands that an agent runs after +launch. + +## Requirement mapping + +| Requirement | Design section | +| --- | --- | +| `AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.1` | [Refresh policy](#refresh-policy) | +| `AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.2` | [Refresh routing](#refresh-routing) | +| `AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.3` | [Failure and recovery](#failure-and-recovery) | +| `AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.4` | [Base-ref selection](#base-ref-selection) | +| `AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.5` | [Base-ref selection](#base-ref-selection) | +| `AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.6` | [Multi-repository launch](#multi-repository-launch) | +| `AC-WORKSPACES-WORKTREE-BASE-REFRESH-001.7` | [Launch-error projection](#launch-error-projection) | + +## Components and responsibilities + +- `internal/orchestrator/executor` resolves every task repository and selects + the refresh route that matches its provider and task Git credential policy. + It passes `PullBeforeWorktree`, `RemoteSyncHandled`, and a provider-authenticated + refresh callback to the lifecycle layer. The callback is not invoked until a + worktree must be materialized or recreated. +- `internal/repoclone.Cloner` performs a strict refresh for a Kandev-managed + provider checkout when provider credentials belong to the backend refresh + boundary. `RefreshWorkspaceRepositoryWithCredentialRequest` binds the fetch + to the exact workspace, task, session, and repository scope. +- `internal/worktree.Manager` performs strict non-interactive fetch and base-ref + selection when the checkout uses the host or executor Git route. It creates + no worktree until the required sync and ancestry checks succeed. +- `internal/agent/runtime/lifecycle` propagates repository preparation errors + without starting the agent runtime. +- The task launch-failure projection stores and renders the resulting bounded, + credential-safe error. This design does not add a parallel workspace error + store. + +## Data and contracts + +`repositories.pull_before_worktree` remains the persisted user policy. No +schema change is required. + +The existing worktree preparation fields have these contracts: + +| Field | Contract | +| --- | --- | +| `PullBeforeWorktree` | A required remote refresh remains to be completed before base-ref selection. | +| `RemoteSyncHandled` | A strict provider-authenticated refresh completed for the exact checkout. The worktree manager must not run a second unauthenticated fetch. | +| `RefreshRepository` | An optional exact-scope refresh callback. Lifecycle invokes it only when creating or recreating a worktree; valid reuse bypasses it. | + +`RemoteSyncHandled` is valid only after a successful strict refresh. A +best-effort fetch or a failed refresh cannot set it. + +## Refresh policy + +When `PullBeforeWorktree` is false, base-ref resolution keeps its offline local +behavior. This path does not claim that the checkout contains current remote +state. + +When `PullBeforeWorktree` is true, worktree materialization requires one +successful refresh. A caller cannot convert a refresh error into a warning and +local ref. The rule applies to initial launch and to resume whenever Kandev +creates or recreates a worktree. A valid reusable worktree is returned before +the callback is invoked. + +## Refresh routing + +The executor resolves one of two refresh routes before lifecycle preparation: + +1. A Kandev-managed provider route supplies a callback that calls the strict + `repoclone` refresh with the exact credential request. The lifecycle layer + invokes it only when it must materialize or recreate a worktree. On success, + worktree creation clears `PullBeforeWorktree` and sets `RemoteSyncHandled`. +2. A host or executor route leaves `PullBeforeWorktree` set. The worktree + manager uses the reconciled `origin` URL and the non-interactive host Git + environment. This includes an executor-inherited GitHub checkout whose + origin uses the protocol selected by the host `gh` configuration. + +When a managed checkout carries a pull request number, the strict refresh also +fetches `refs/pull//head` into a local `origin/pr/` remote-tracking ref. +Worktree creation uses that ref for the checkout branch, including fork pull +requests, without a second unauthenticated fetch. + +Provider selection must follow the existing task Git credential policy. A +managed GitHub workspace uses the backend-managed credential route. An +executor-inherited GitHub workspace must not replace the reconciled SSH origin +with a managed HTTPS origin. + +The plugin-provider strict-refresh behavior remains the model for exact +credential scope. GitLab and Azure DevOps keep their existing provider-specific +credential resolution. + +## Base-ref selection + +After a successful fetch, the worktree manager compares the local base `L` +with the fetched remote base `R`: + +| Relationship | Start ref | Reason | +| --- | --- | --- | +| `R` contains `L` | `R` | Includes all local commits and current remote commits. | +| `L` contains `R` | `L` | Preserves local-only commits and includes current remote commits. | +| Neither contains the other | Error | The refs diverged and either choice would omit commits. | +| Ancestry cannot be proven | Error | Kandev cannot prove that the start ref is safe. | + +A successful fast-forward pull normally produces the first or second state. +If pull fails after fetch, the same table selects the safe ref. The manager does +not reset, rebase, merge, or delete a branch. + +`pullBaseBranch`, `resolveLocalBaseRef`, and +`pullCurrentBranchOrFallback` return errors to their callers. The errors retain +Git failure classification but exclude tokens, credential helper output, and +secret-bearing URLs. + +## Multi-repository launch + +The executor resolves and prepares repository specs before agent startup. If a +required refresh fails for one spec, lifecycle preparation returns an error and +does not start the runtime. The error carries stable repository identity and a +safe display name so same-provider sibling repositories remain distinguishable. + +Preparation can leave an already refreshed checkout on disk. This state is +safe and retryable. A later launch performs the required checks again. Kandev +does not report a partial task launch because no agent process started. + +## Launch-error projection + +The executor returns required-refresh errors through its existing launch +failure path. The task system stores the active error in the current typed +summary and renders it on desktop and mobile after reload. The generic launch +failure category is sufficient unless implementation evidence shows that a +distinct category enables a safe recovery action. + +The bounded detail names the repository and failure class. It does not include +credentials, tokens, raw authenticated URLs, or unrestricted Git output. Retry +uses the normal task launch action after the user corrects network, SSH, or +credential configuration. + +## Failure and recovery + +- Fetch authentication, network, timeout, cancellation, and Git command errors + stop required preparation. +- A missing fetched remote base stops preparation. It cannot fall back to a + local ref when refresh is required. +- A failed ancestry probe stops preparation. +- A divergent base stops preparation and preserves both refs. +- Disabling pull-before-worktree is the explicit offline opt-out. Kandev does + not disable it automatically after a failure. +- Retrying task launch reruns remote refresh and base-ref selection. + +## Security + +Managed refresh uses the provider credential boundary and exact task, session, +and repository scope. Executor refresh uses only credentials available to the +selected host or executor route. Neither route copies a provider token into an +error, progress event, or log field. + +Git remains non-interactive. `GIT_TERMINAL_PROMPT=0`, batch-mode SSH, bounded +command contexts, and Git subprocess admission prevent a hidden prompt from +blocking task launch. + +## Observability + +The existing sync progress callback reports a running event and then either a +completed event or a failed event. A failed event contains the bounded failure +class and repository identity, not a fallback-ref success message. + +Structured logs record the repository ID, task ID, session ID, provider, +configured transport, refresh route, and failure class. Logs do not record +credential material or secret-bearing remote URLs. + +## Related decisions + +- [Required Worktree Refresh Fails Closed](../../../decisions/2026-08-25-required-worktree-refresh-fails-closed.md) +- [Separate GitHub Automation From Task Git Credential Policy](../../../decisions/2026-07-27-task-git-credential-policy.md) +- [Provider-Neutral Git Credential Broker](../../../decisions/2026-07-31-provider-neutral-git-credential-broker.md)