diff --git a/apps/backend/cmd/kandev/orchestrator.go b/apps/backend/cmd/kandev/orchestrator.go index 1aa1b3a1d8..5f2f81c655 100644 --- a/apps/backend/cmd/kandev/orchestrator.go +++ b/apps/backend/cmd/kandev/orchestrator.go @@ -658,7 +658,7 @@ func (a *repositoryResolverAdapter) ResolveForReview( return "", "", fmt.Errorf("clone repository: %w", err) } - repo, err := a.taskSvc.FindOrCreateRepository(ctx, &taskservice.FindOrCreateRepositoryRequest{ + repo, _, err := a.taskSvc.FindOrCreateRepository(ctx, &taskservice.FindOrCreateRepositoryRequest{ WorkspaceID: workspaceID, Provider: provider, ProviderOwner: owner, diff --git a/apps/backend/internal/improvekandev/handler.go b/apps/backend/internal/improvekandev/handler.go index ac064f90e1..f468a3aab2 100644 --- a/apps/backend/internal/improvekandev/handler.go +++ b/apps/backend/internal/improvekandev/handler.go @@ -204,7 +204,7 @@ func (h *Handler) resolveOrCloneRepo(ctx context.Context, workspaceID string) (* if err != nil { return nil, err } - return h.taskSvc.FindOrCreateRepository(ctx, &taskservice.FindOrCreateRepositoryRequest{ + repo, _, err := h.taskSvc.FindOrCreateRepository(ctx, &taskservice.FindOrCreateRepositoryRequest{ WorkspaceID: workspaceID, Provider: repoProvider, ProviderOwner: repoOwner, @@ -212,6 +212,7 @@ func (h *Handler) resolveOrCloneRepo(ctx context.Context, workspaceID string) (* DefaultBranch: defaultBranch, LocalPath: localPath, }) + return repo, err } // findKandevRepoByLocalRemote returns the first repo with a local path whose diff --git a/apps/backend/internal/mcp/handlers/handlers.go b/apps/backend/internal/mcp/handlers/handlers.go index 0fe7962f3f..fcf6a978fa 100644 --- a/apps/backend/internal/mcp/handlers/handlers.go +++ b/apps/backend/internal/mcp/handlers/handlers.go @@ -821,7 +821,8 @@ func classifyAddBranchError(err error) string { return ws.ErrorCodeConflict case strings.Contains(msg, "repository_id is required"), strings.Contains(msg, "only supported on the worktree executor"), - strings.Contains(msg, "task_id is required"): + strings.Contains(msg, "task_id is required"), + strings.Contains(msg, "cannot resolve base_branch"): return ws.ErrorCodeValidation case strings.Contains(msg, "GitHub URL"), strings.Contains(msg, "github.com/owner/repo"), diff --git a/apps/backend/internal/mcp/handlers/handlers_test.go b/apps/backend/internal/mcp/handlers/handlers_test.go index e0e0784a28..d7731f8c81 100644 --- a/apps/backend/internal/mcp/handlers/handlers_test.go +++ b/apps/backend/internal/mcp/handlers/handlers_test.go @@ -100,6 +100,18 @@ func (b *mcpRecordingEventBus) Publish(_ context.Context, _ string, event *bus.E return nil } +// TestClassifyAddBranchError_UnresolvedBaseBranchIsValidation pins the +// classifier's handling of the new "cannot resolve base_branch" sentinel +// emitted by AddBranchToTask when neither base_branch nor a probed +// default_branch is available. Surface as ErrorCodeValidation so MCP agents +// see a user-fixable input issue instead of an internal error. +func TestClassifyAddBranchError_UnresolvedBaseBranchIsValidation(t *testing.T) { + err := errors.New(`cannot resolve base_branch for repository "acme/widgets": pass base_branch explicitly`) + if got := classifyAddBranchError(err); got != ws.ErrorCodeValidation { + t.Errorf("expected ErrorCodeValidation, got %q", got) + } +} + // TestHandleAddBranchToTask_RejectsMultipleLocators pins the mutual-exclusion // check at the WS handler tier: supplying two of repository_id / // repository_url / local_path is an agent mistake that previously got diff --git a/apps/backend/internal/task/service/service.go b/apps/backend/internal/task/service/service.go index 0e2e2112aa..0669025296 100644 --- a/apps/backend/internal/task/service/service.go +++ b/apps/backend/internal/task/service/service.go @@ -42,6 +42,21 @@ type TaskExecutionStopper interface { StopExecution(ctx context.Context, executionID, reason string, force bool) error } +// ProviderDefaultBranchProber resolves a provider repo's default branch +// (e.g. "main" / "master") without requiring a local clone. Used by +// AddBranchToTask to satisfy the worktree-create precondition synchronously, +// since add_branch does not trigger the executor-side backfillRepoDefaultBranch +// path. Default implementation (cmd/kandev) shells out to +// `git ls-remote --symref`; tests inject a stub via SetProviderDefaultBranchProber. +// +// Implementations MUST honour ctx cancellation so a slow / hung remote does not +// stall the calling MCP tool. Returns ("", error) on probe failure — callers +// fall through to the explicit "cannot resolve base_branch" rejection rather +// than persisting an empty-default row. +type ProviderDefaultBranchProber interface { + ProbeDefaultBranch(ctx context.Context, provider, owner, name string) (string, error) +} + // BranchMaterializer creates a worktree on disk and persists a // task_session_worktrees row for a newly added task_repository row, without // restarting the agent. Used by AddBranchToTask so MCP-driven "add a branch @@ -148,6 +163,7 @@ type Service struct { worktreeCleanup WorktreeCleanup executionStopper TaskExecutionStopper branchMaterializer BranchMaterializer + providerProber ProviderDefaultBranchProber gitArchiveCapture GitArchiveCapture workflowStepCreator WorkflowStepCreator workflowStepGetter WorkflowStepGetter @@ -197,6 +213,14 @@ func (s *Service) SetBranchMaterializer(m BranchMaterializer) { s.branchMaterializer = m } +// SetProviderDefaultBranchProber wires the synchronous default-branch probe +// used by AddBranchToTask's GitHub-URL resolution. Optional — when unset, +// add_branch with a provider URL and no base_branch falls through to the +// "cannot resolve base_branch" rejection instead of persisting an empty row. +func (s *Service) SetProviderDefaultBranchProber(p ProviderDefaultBranchProber) { + s.providerProber = p +} + // SetExecutionStopper wires the task execution stopper (orchestrator). func (s *Service) SetExecutionStopper(stopper TaskExecutionStopper) { s.executionStopper = stopper diff --git a/apps/backend/internal/task/service/service_branches.go b/apps/backend/internal/task/service/service_branches.go index 88f5d30a99..c0810f0e52 100644 --- a/apps/backend/internal/task/service/service_branches.go +++ b/apps/backend/internal/task/service/service_branches.go @@ -55,12 +55,13 @@ type AddBranchToTaskRequest struct { // LocalPath / GitHubURL flows; for the RepositoryID fast path the // workspace-membership check happens in resolveBranchRepo below. func (s *Service) AddBranchToTask(ctx context.Context, req AddBranchToTaskRequest) (*models.TaskRepository, error) { - task, existing, err := s.prepareBranchAdd(ctx, &req) + task, existing, cleanupOrphanRepo, err := s.prepareBranchAdd(ctx, &req) if err != nil { return nil, err } repo, err := s.resolveBranchRepo(ctx, task, req.RepositoryID) if err != nil { + cleanupOrphanRepo() return nil, err } @@ -68,10 +69,19 @@ func (s *Service) AddBranchToTask(ctx context.Context, req AddBranchToTaskReques if baseBranch == "" { baseBranch = repo.DefaultBranch } + // Gate before any task_repositories write: with no resolvable base branch + // the materialize step can only fail, leaving an orphan row + (worse) an + // orphan provider repository_url Repository row. Fail loud so the MCP + // caller can supply base_branch explicitly or pre-register the repo. + if baseBranch == "" { + cleanupOrphanRepo() + return nil, fmt.Errorf("cannot resolve base_branch for repository %q: pass base_branch explicitly", repoLabelOrID(repo, req.RepositoryID)) + } checkoutBranch := resolveCheckoutBranchOrAutoName(req.CheckoutBranch, existing, req.RepositoryID, baseBranch) nextPosition, dupErr := scanForBranchAddDuplicate(existing, req.RepositoryID, baseBranch, checkoutBranch, repo) if dupErr != nil { + cleanupOrphanRepo() return nil, dupErr } @@ -89,13 +99,34 @@ func (s *Service) AddBranchToTask(ctx context.Context, req AddBranchToTaskReques zap.String("repository_id", req.RepositoryID), zap.String("checkout_branch", req.CheckoutBranch), zap.Error(err)) + cleanupOrphanRepo() return nil, fmt.Errorf("create task repository: %w", err) } - // Publish task.updated so the kanban / sidebar re-render with the new row. - s.publishTaskEvent(ctx, events.TaskUpdated, task, nil) + if err := s.materializeBranch(ctx, req.TaskID, taskRepo.ID); err != nil { + // Roll back the task_repositories row so a failed materialize doesn't + // leave a dangling association the user can't see on disk. Pre-launch + // tasks short-circuit inside materializeBranch and never reach this + // branch — their worktree is built at next session launch. + // + // Detach from ctx via WithoutCancel: a caller-side timeout or cancel + // can fire mid-materialize, and the rollback must still run on the + // now-dead ctx or we'd leak the row we tried to delete. + rollbackCtx := context.WithoutCancel(ctx) + if delErr := s.taskRepos.DeleteTaskRepository(rollbackCtx, taskRepo.ID); delErr != nil { + s.logger.Error("AddBranchToTask: failed to roll back task repository after materialize failure", + zap.String("task_repository_id", taskRepo.ID), + zap.Error(delErr)) + } + cleanupOrphanRepo() + return nil, err + } - s.materializeBranchBestEffort(ctx, req.TaskID, taskRepo.ID) + // Publish task.updated only after the row is durable AND the worktree + // materialized. Emitting before materialize would push a phantom row to + // WS clients on the rollback path; emitting after keeps event truthiness + // aligned with persisted state. + s.publishTaskEvent(ctx, events.TaskUpdated, task, nil) return taskRepo, nil } @@ -107,18 +138,27 @@ func (s *Service) AddBranchToTask(ctx context.Context, req AddBranchToTaskReques // Mutates req.RepositoryID when it was omitted and a single-repo default // applies; multi-repo tasks return an error here instead of silently // guessing the wrong repo. -func (s *Service) prepareBranchAdd(ctx context.Context, req *AddBranchToTaskRequest) (*models.Task, []*models.TaskRepository, error) { +// +// Also returns a cleanupOrphanRepo callback that the caller invokes on any +// error path AFTER prepareBranchAdd returns successfully: when this call +// created a fresh Repository row via the GitHubURL/LocalPath +// find-or-create path, the callback deletes it so a later validation or +// materialize failure does not leave an orphan provider row in the DB. The +// callback is never nil and is safe to call on the happy path (no-op when +// nothing was created here). +func (s *Service) prepareBranchAdd(ctx context.Context, req *AddBranchToTaskRequest) (*models.Task, []*models.TaskRepository, func(), error) { + noopCleanup := func() {} if req.TaskID == "" { - return nil, nil, fmt.Errorf("task_id is required") + return nil, nil, noopCleanup, fmt.Errorf("task_id is required") } task, err := s.tasks.GetTask(ctx, req.TaskID) if err != nil { - return nil, nil, fmt.Errorf("get task: %w", err) + return nil, nil, noopCleanup, fmt.Errorf("get task: %w", err) } if task == nil { // Wrap the repo-tier sentinel so handlers can classify via errors.Is // rather than substring-matching the formatted UUID. - return nil, nil, fmt.Errorf("%w: %s", taskrepo.ErrTaskNotFound, req.TaskID) + return nil, nil, noopCleanup, fmt.Errorf("%w: %s", taskrepo.ErrTaskNotFound, req.TaskID) } // Executor gate runs BEFORE repository resolution so a rejection on a // non-worktree executor can't leak an orphan Repository row into the @@ -126,38 +166,25 @@ func (s *Service) prepareBranchAdd(ctx context.Context, req *AddBranchToTaskRequ // ResolveRepositoryRef. Mirrors the "keeps the DB clean" invariant // documented on requireWorktreeExecutorForBranchAdd itself. if err := s.requireWorktreeExecutorForBranchAdd(ctx, req.TaskID); err != nil { - return nil, nil, err + return nil, nil, noopCleanup, err } - // Resolve LocalPath / GitHubURL to a concrete RepositoryID up front so the - // rest of the flow (duplicate scan, row insert) operates on a stable UUID. - // Skipped when RepositoryID is already set or neither alternative - // identifier was supplied (the single-repo task default applies after - // the existing-rows lookup). - if req.RepositoryID == "" && (req.LocalPath != "" || req.GitHubURL != "") { - resolvedID, resolvedBranch, resolveErr := s.ResolveRepositoryRef(ctx, task.WorkspaceID, TaskRepositoryInput{ - LocalPath: req.LocalPath, - GitHubURL: req.GitHubURL, - BaseBranch: req.BaseBranch, - }) - if resolveErr != nil { - return nil, nil, fmt.Errorf("resolve repository: %w", resolveErr) - } - req.RepositoryID = resolvedID - if req.BaseBranch == "" { - req.BaseBranch = resolvedBranch - } + cleanupOrphanRepo, err := s.resolveBranchAddRepoRef(ctx, task, req, noopCleanup) + if err != nil { + return nil, nil, noopCleanup, err } existing, err := s.taskRepos.ListTaskRepositories(ctx, req.TaskID) if err != nil { - return nil, nil, fmt.Errorf("list existing branches: %w", err) + cleanupOrphanRepo() + return nil, nil, noopCleanup, fmt.Errorf("list existing branches: %w", err) } if req.RepositoryID == "" { req.RepositoryID, err = s.defaultRepositoryIDForTask(existing) if err != nil { - return nil, nil, err + cleanupOrphanRepo() + return nil, nil, noopCleanup, err } } - return task, existing, nil + return task, existing, cleanupOrphanRepo, nil } // resolveBranchRepo loads the Repository for req.RepositoryID and verifies @@ -254,20 +281,61 @@ func branchAddDuplicateError( return fmt.Errorf("repository %q on branch %q is already attached to this task", label, branchLabel) } -// materializeBranchBestEffort triggers the worktree manager + agentctl -// rescan asynchronously-ish. Failures are logged but don't unwind the DB -// write — the next session launch will pick the worktree up via -// prepareMultiRepo, so a transient rescan failure is recoverable. -func (s *Service) materializeBranchBestEffort(ctx context.Context, taskID, taskRepositoryID string) { +// materializeBranch triggers the worktree manager + agentctl rescan. +// +// Pre-launch tasks (no task_environments row, or no materializer wired) keep +// the legacy best-effort behaviour: failures are logged and the next session +// launch will pick the worktree up via prepareMultiRepo. Returns nil in both +// of those cases so the task_repositories row sticks around for the launcher +// to act on. +// +// Live worktree-executor tasks (already launched) surface the materialize +// failure to the caller so AddBranchToTask can roll back the dangling +// task_repositories row — leaving it in place produced the silent-success +// orphan that the original bug report describes. +func (s *Service) materializeBranch(ctx context.Context, taskID, taskRepositoryID string) error { if s.branchMaterializer == nil { - return + return nil } + // Snapshot launch state BEFORE MaterializeBranch so a caller-side cancel + // during materialize can't poison the post-failure check: launch state + // cannot change during the synchronous materialize call, but the DB + // query in taskAlreadyLaunched would fail with a context error if we + // asked after the fact — and that would silently demote a live-task + // rollback to a best-effort no-op, leaving the orphan row the PR exists + // to prevent. + alreadyLaunched := s.taskAlreadyLaunched(ctx, taskID) if err := s.branchMaterializer.MaterializeBranch(ctx, taskID, taskRepositoryID); err != nil { - s.logger.Warn("branch materialization failed; worktree will be created on next session launch", + if !alreadyLaunched { + s.logger.Warn("branch materialization failed; worktree will be created on next session launch", + zap.String("task_id", taskID), + zap.String("task_repository_id", taskRepositoryID), + zap.Error(err)) + return nil + } + s.logger.Error("branch materialization failed on a live task; rolling back", zap.String("task_id", taskID), zap.String("task_repository_id", taskRepositoryID), zap.Error(err)) + return fmt.Errorf("materialize branch: %w", err) } + return nil +} + +// taskAlreadyLaunched reports whether the task already has a task_environments +// row whose executor type is fixed — the signal that an executor backend is +// running and won't replay prepareMultiRepo for the new branch on its own. +// Pre-launch tasks (no row, or no executor type yet) return false so a +// materialize failure stays best-effort. +func (s *Service) taskAlreadyLaunched(ctx context.Context, taskID string) bool { + if s.taskEnvironments == nil { + return false + } + env, err := s.taskEnvironments.GetTaskEnvironmentByTaskID(ctx, taskID) + if err != nil || env == nil { + return false + } + return env.ExecutorType != "" } // defaultRepositoryIDForTask resolves the implicit repository to use when @@ -392,3 +460,62 @@ func repoLabelOrID(repo *models.Repository, repositoryID string) string { } return repositoryID } + +// resolveBranchAddRepoRef resolves LocalPath / GitHubURL on the add_branch +// request to a concrete RepositoryID up front so the rest of the flow +// (duplicate scan, row insert) operates on a stable UUID. Returns a cleanup +// callback that the caller must invoke on any error path between resolution +// and a successful task_repositories insert — when the call created a fresh +// Repository row, the callback deletes it so workspace state doesn't accrue +// dangling provider rows. Pure no-op when RepositoryID was already set or +// neither alternative identifier was supplied. +func (s *Service) resolveBranchAddRepoRef( + ctx context.Context, task *models.Task, req *AddBranchToTaskRequest, noopCleanup func(), +) (func(), error) { + if req.RepositoryID != "" || (req.LocalPath == "" && req.GitHubURL == "") { + return noopCleanup, nil + } + resolvedID, resolvedBranch, createdByUs, resolveErr := s.ResolveRepositoryRef(ctx, task.WorkspaceID, TaskRepositoryInput{ + LocalPath: req.LocalPath, + GitHubURL: req.GitHubURL, + BaseBranch: req.BaseBranch, + ResolveProviderDefaults: true, + }) + if resolveErr != nil { + return noopCleanup, fmt.Errorf("resolve repository: %w", resolveErr) + } + req.RepositoryID = resolvedID + if req.BaseBranch == "" { + req.BaseBranch = resolvedBranch + } + // Register cleanup only when this call actually inserted the Repository + // row. The previous heuristic (workspace pre-list snapshot vs resolvedID) + // had a TOCTOU race: a concurrent add_branch for the same GitHub URL + // could create the row between the snapshot and FindOrCreateRepository, + // then we'd register cleanup against another request's data and delete + // it on any later failure here. createdByUs is set by FindOrCreateRepository + // / CreateRepository themselves and can't be spoofed by concurrency. + if resolvedID == "" || !createdByUs { + return noopCleanup, nil + } + createdID := resolvedID + cleanup := func() { + // Route through Service.DeleteRepository so the matching + // repository.deleted event fires; the create path published + // repository.created in CreateRepository, and skipping the + // delete event would leave WS subscribers / frontend caches + // holding a phantom row. + // + // Detach from the captured ctx via WithoutCancel: the rollback + // often runs precisely because the caller's ctx was cancelled + // (timeout, client disconnect), and reusing it would also kill + // the cleanup query and orphan the row we just created. + rollbackCtx := context.WithoutCancel(ctx) + if delErr := s.DeleteRepository(rollbackCtx, createdID); delErr != nil { + s.logger.Warn("AddBranchToTask: failed to roll back created repository", + zap.String("repository_id", createdID), + zap.Error(delErr)) + } + } + return cleanup, nil +} diff --git a/apps/backend/internal/task/service/service_branches_qa_test.go b/apps/backend/internal/task/service/service_branches_qa_test.go new file mode 100644 index 0000000000..8a8ff18b8d --- /dev/null +++ b/apps/backend/internal/task/service/service_branches_qa_test.go @@ -0,0 +1,224 @@ +package service + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/kandev/kandev/internal/task/models" +) + +// TestAddBranchToTask_QA_ProberErrorFallsThrough exercises the +// error-from-probe path (not just empty-result): the rejection contract +// must hold the same way and leave no orphan rows. +func TestAddBranchToTask_QA_ProberErrorFallsThrough(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + + _ = repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-1", Name: "WS"}) + _ = repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-1", WorkspaceID: "ws-1", Name: "WF"}) + _ = repo.CreateRepository(ctx, &models.Repository{ID: "repo-1", WorkspaceID: "ws-1", Name: "frontend", DefaultBranch: "main"}) + + task, err := svc.CreateTask(ctx, &CreateTaskRequest{ + WorkspaceID: "ws-1", + WorkflowID: "wf-1", + WorkflowStepID: "step-1", + Title: "Probe error", + Repositories: []TaskRepositoryInput{{RepositoryID: "repo-1", BaseBranch: "main"}}, + }) + if err != nil { + t.Fatalf("CreateTask: %v", err) + } + seedWorktreeTaskEnv(t, repo, task.ID, "env-1") + svc.SetProviderDefaultBranchProber(&stubProber{err: errors.New("network: timeout")}) + + beforeRepos, _ := repo.ListRepositories(ctx, "ws-1") + _, err = svc.AddBranchToTask(ctx, AddBranchToTaskRequest{ + TaskID: task.ID, + GitHubURL: "https://github.com/acme/never-seen", + }) + if err == nil { + t.Fatal("expected probe error path to reject") + } + if !strings.Contains(err.Error(), "cannot resolve base_branch") { + t.Errorf("unexpected error: %v", err) + } + afterRepos, _ := repo.ListRepositories(ctx, "ws-1") + if len(afterRepos) != len(beforeRepos) { + t.Errorf("orphan repo leaked on probe-error path: before=%d after=%d", len(beforeRepos), len(afterRepos)) + } +} + +// TestAddBranchToTask_QA_BadGitHubURLRejected pins the upstream URL +// validation. parseGitHubRepoURL must surface bad URLs before any DB +// write so the workspace stays clean. Plain validation surface. +func TestAddBranchToTask_QA_BadGitHubURLRejected(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + + _ = repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-1", Name: "WS"}) + _ = repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-1", WorkspaceID: "ws-1", Name: "WF"}) + _ = repo.CreateRepository(ctx, &models.Repository{ID: "repo-1", WorkspaceID: "ws-1", Name: "frontend", DefaultBranch: "main"}) + + task, err := svc.CreateTask(ctx, &CreateTaskRequest{ + WorkspaceID: "ws-1", + WorkflowID: "wf-1", + WorkflowStepID: "step-1", + Title: "Bad URL", + Repositories: []TaskRepositoryInput{{RepositoryID: "repo-1", BaseBranch: "main"}}, + }) + if err != nil { + t.Fatalf("CreateTask: %v", err) + } + seedWorktreeTaskEnv(t, repo, task.ID, "env-1") + + beforeRepos, _ := repo.ListRepositories(ctx, "ws-1") + _, err = svc.AddBranchToTask(ctx, AddBranchToTaskRequest{ + TaskID: task.ID, + GitHubURL: "https://gitlab.com/acme/widgets", // not github + }) + if err == nil { + t.Fatal("expected rejection of non-github URL") + } + afterRepos, _ := repo.ListRepositories(ctx, "ws-1") + if len(afterRepos) != len(beforeRepos) { + t.Errorf("URL parse error path leaked an orphan: before=%d after=%d", len(beforeRepos), len(afterRepos)) + } +} + +// TestAddBranchToTask_QA_RegisteredProviderRepoSkipsProbe pins the +// happy path when the workspace already has the provider repo +// registered (with a non-empty default_branch): the probe is NOT called +// (existing row's stored default wins) and the call succeeds. +func TestAddBranchToTask_QA_RegisteredProviderRepoSkipsProbe(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + + _ = repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-1", Name: "WS"}) + _ = repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-1", WorkspaceID: "ws-1", Name: "WF"}) + _ = repo.CreateRepository(ctx, &models.Repository{ + ID: "repo-existing", WorkspaceID: "ws-1", Name: "acme/widgets", + Provider: "github", ProviderOwner: "acme", ProviderName: "widgets", + DefaultBranch: "trunk", SourceType: "provider", + }) + _ = repo.CreateRepository(ctx, &models.Repository{ID: "repo-1", WorkspaceID: "ws-1", Name: "frontend", DefaultBranch: "main"}) + + task, err := svc.CreateTask(ctx, &CreateTaskRequest{ + WorkspaceID: "ws-1", + WorkflowID: "wf-1", + WorkflowStepID: "step-1", + Title: "Skip probe", + Repositories: []TaskRepositoryInput{{RepositoryID: "repo-1", BaseBranch: "main"}}, + }) + if err != nil { + t.Fatalf("CreateTask: %v", err) + } + // Pre-launch (no env) so we don't need a materializer for this assertion. + prober := &stubProber{branch: "shouldnt-be-used"} + svc.SetProviderDefaultBranchProber(prober) + + added, err := svc.AddBranchToTask(ctx, AddBranchToTaskRequest{ + TaskID: task.ID, + GitHubURL: "https://github.com/acme/widgets", + }) + if err != nil { + t.Fatalf("AddBranchToTask: %v", err) + } + if prober.calls != 0 { + t.Errorf("probe should not run when existing repo carries default_branch; calls=%d", prober.calls) + } + if added.BaseBranch != "trunk" { + t.Errorf("expected existing repo's stored default_branch=trunk on row, got %q", added.BaseBranch) + } + if added.RepositoryID != "repo-existing" { + t.Errorf("expected to reuse existing repo id, got %q", added.RepositoryID) + } +} + +// TestAddBranchToTask_QA_BackfillsExistingEmptyDefaultBranch covers the +// FindOrCreateRepository backfill side effect: a workspace repo with an +// empty default_branch (e.g. orphan row from a prior bug-affected call) +// gets its default_branch populated when the probe returns a real value. +func TestAddBranchToTask_QA_BackfillsExistingEmptyDefaultBranch(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + + _ = repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-1", Name: "WS"}) + _ = repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-1", WorkspaceID: "ws-1", Name: "WF"}) + _ = repo.CreateRepository(ctx, &models.Repository{ + ID: "repo-empty", WorkspaceID: "ws-1", Name: "acme/widgets", + Provider: "github", ProviderOwner: "acme", ProviderName: "widgets", + DefaultBranch: "", SourceType: "provider", // legacy orphan with empty default + }) + _ = repo.CreateRepository(ctx, &models.Repository{ID: "repo-1", WorkspaceID: "ws-1", Name: "frontend", DefaultBranch: "main"}) + + task, err := svc.CreateTask(ctx, &CreateTaskRequest{ + WorkspaceID: "ws-1", + WorkflowID: "wf-1", + WorkflowStepID: "step-1", + Title: "Backfill", + Repositories: []TaskRepositoryInput{{RepositoryID: "repo-1", BaseBranch: "main"}}, + }) + if err != nil { + t.Fatalf("CreateTask: %v", err) + } + prober := &stubProber{branch: "main"} + svc.SetProviderDefaultBranchProber(prober) + + added, err := svc.AddBranchToTask(ctx, AddBranchToTaskRequest{ + TaskID: task.ID, + GitHubURL: "https://github.com/acme/widgets", + }) + if err != nil { + t.Fatalf("AddBranchToTask: %v", err) + } + if added.BaseBranch != "main" { + t.Errorf("expected base_branch=main from probe, got %q", added.BaseBranch) + } + reloaded, err := svc.GetRepository(ctx, "repo-empty") + if err != nil || reloaded == nil { + t.Fatalf("GetRepository: %v", err) + } + if reloaded.DefaultBranch != "main" { + t.Errorf("expected existing repo default_branch backfilled to main, got %q", reloaded.DefaultBranch) + } +} + +// TestAddBranchToTask_QA_RepositoryIDFastPathIgnoresProbe pins that the +// repository_id fast path never invokes the probe — the worktree path +// for already-known repos must stay zero-cost. +func TestAddBranchToTask_QA_RepositoryIDFastPathIgnoresProbe(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + + _ = repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-1", Name: "WS"}) + _ = repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-1", WorkspaceID: "ws-1", Name: "WF"}) + _ = repo.CreateRepository(ctx, &models.Repository{ID: "repo-1", WorkspaceID: "ws-1", Name: "frontend", DefaultBranch: "main"}) + + task, err := svc.CreateTask(ctx, &CreateTaskRequest{ + WorkspaceID: "ws-1", + WorkflowID: "wf-1", + WorkflowStepID: "step-1", + Title: "Fast path", + Repositories: []TaskRepositoryInput{{RepositoryID: "repo-1", BaseBranch: "main"}}, + }) + if err != nil { + t.Fatalf("CreateTask: %v", err) + } + prober := &stubProber{branch: "should-not-fire"} + svc.SetProviderDefaultBranchProber(prober) + + _, err = svc.AddBranchToTask(ctx, AddBranchToTaskRequest{ + TaskID: task.ID, + RepositoryID: "repo-1", + BaseBranch: "main", + CheckoutBranch: "feature/fast-path", + }) + if err != nil { + t.Fatalf("AddBranchToTask: %v", err) + } + if prober.calls != 0 { + t.Errorf("probe must not run on repository_id fast path; calls=%d", prober.calls) + } +} diff --git a/apps/backend/internal/task/service/service_branches_test.go b/apps/backend/internal/task/service/service_branches_test.go index 16d70826b6..4117dbc7db 100644 --- a/apps/backend/internal/task/service/service_branches_test.go +++ b/apps/backend/internal/task/service/service_branches_test.go @@ -2,10 +2,12 @@ package service import ( "context" + "fmt" "strings" "testing" "time" + "github.com/kandev/kandev/internal/events" "github.com/kandev/kandev/internal/task/models" ) @@ -642,3 +644,287 @@ func TestCreateTask_RejectsSameRepoSameBranch(t *testing.T) { t.Errorf("unexpected error: %v", err) } } + +// stubProber implements ProviderDefaultBranchProber for tests; returns the +// preset branch (which may be empty to simulate probe failure) and records +// the call so tests can assert it ran. +type stubProber struct { + branch string + err error + calls int + lastProviderArg string + lastOwner string + lastName string +} + +func (p *stubProber) ProbeDefaultBranch(_ context.Context, provider, owner, name string) (string, error) { + p.calls++ + p.lastProviderArg = provider + p.lastOwner = owner + p.lastName = name + return p.branch, p.err +} + +// stubMaterializer implements BranchMaterializer for tests; the err field +// controls whether the materialize step succeeds. +type stubMaterializer struct { + err error + calls int +} + +func (m *stubMaterializer) MaterializeBranch(_ context.Context, _ string, _ string) error { + m.calls++ + return m.err +} + +// seedWorktreeTaskEnv attaches a worktree-executor task_environments row so +// requireWorktreeExecutorForBranchAdd permits the call and taskAlreadyLaunched +// reports the task as live. +func seedWorktreeTaskEnv(t *testing.T, repo interface { + CreateTaskEnvironment(ctx context.Context, env *models.TaskEnvironment) error +}, taskID, id string, +) { + t.Helper() + now := time.Now().UTC() + if err := repo.CreateTaskEnvironment(context.Background(), &models.TaskEnvironment{ + ID: id, + TaskID: taskID, + ExecutorType: string(models.ExecutorTypeWorktree), + WorkspacePath: "/tmp/task-env", + Status: "ready", + CreatedAt: now, + UpdatedAt: now, + }); err != nil { + t.Fatalf("CreateTaskEnvironment: %v", err) + } +} + +// TestAddBranchToTask_RejectsProviderURLWithUnresolvableBaseBranch pins the +// bug fix: an add_branch call with repository_url for a repo that has no +// resolvable default branch (probe returns empty, no existing repo row in +// workspace) must return a "cannot resolve base_branch" error and leave no +// orphan rows behind — neither task_repositories nor a freshly-created +// Repository row. +func TestAddBranchToTask_RejectsProviderURLWithUnresolvableBaseBranch(t *testing.T) { + svc, eventBus, repo := createTestService(t) + ctx := context.Background() + + _ = repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-1", Name: "WS"}) + _ = repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-1", WorkspaceID: "ws-1", Name: "WF"}) + _ = repo.CreateRepository(ctx, &models.Repository{ID: "repo-1", WorkspaceID: "ws-1", Name: "frontend", DefaultBranch: "main"}) + + task, err := svc.CreateTask(ctx, &CreateTaskRequest{ + WorkspaceID: "ws-1", + WorkflowID: "wf-1", + WorkflowStepID: "step-1", + Title: "Provider URL", + Repositories: []TaskRepositoryInput{ + {RepositoryID: "repo-1", BaseBranch: "main"}, + }, + }) + if err != nil { + t.Fatalf("CreateTask: %v", err) + } + seedWorktreeTaskEnv(t, repo, task.ID, "env-1") + svc.SetProviderDefaultBranchProber(&stubProber{branch: ""}) // probe fails + + beforeRepos, _ := repo.ListRepositories(ctx, "ws-1") + beforeRows, _ := repo.ListTaskRepositories(ctx, task.ID) + + _, err = svc.AddBranchToTask(ctx, AddBranchToTaskRequest{ + TaskID: task.ID, + GitHubURL: "https://github.com/acme/never-seen", + }) + if err == nil { + t.Fatal("expected error for unresolvable base_branch") + } + if !strings.Contains(err.Error(), "cannot resolve base_branch") { + t.Errorf("unexpected error: %v", err) + } + + afterRepos, _ := repo.ListRepositories(ctx, "ws-1") + if len(afterRepos) != len(beforeRepos) { + t.Errorf("expected no orphan repositories row; before=%d after=%d", len(beforeRepos), len(afterRepos)) + } + afterRows, _ := repo.ListTaskRepositories(ctx, task.ID) + if len(afterRows) != len(beforeRows) { + t.Errorf("expected no task_repositories row insert; before=%d after=%d", len(beforeRows), len(afterRows)) + } + + // Repository.created + repository.deleted must be symmetric on the + // rollback path so WS subscribers / frontend caches don't keep a + // phantom row after the resolve-then-fail sequence. + var created, deleted int + for _, evt := range eventBus.GetPublishedEvents() { + switch evt.Type { + case events.RepositoryCreated: + created++ + case events.RepositoryDeleted: + deleted++ + } + } + if created != deleted { + t.Errorf("repository create/delete events not symmetric: created=%d deleted=%d", created, deleted) + } +} + +// TestAddBranchToTask_ProviderURLResolvesDefaultBranchAndPersists exercises +// the happy provider-URL path: the synchronous probe returns "main", which +// must populate both the new Repository's default_branch and the +// task_repositories row's base_branch. +func TestAddBranchToTask_ProviderURLResolvesDefaultBranchAndPersists(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + + _ = repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-1", Name: "WS"}) + _ = repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-1", WorkspaceID: "ws-1", Name: "WF"}) + _ = repo.CreateRepository(ctx, &models.Repository{ID: "repo-1", WorkspaceID: "ws-1", Name: "frontend", DefaultBranch: "main"}) + + task, err := svc.CreateTask(ctx, &CreateTaskRequest{ + WorkspaceID: "ws-1", + WorkflowID: "wf-1", + WorkflowStepID: "step-1", + Title: "Provider URL happy", + Repositories: []TaskRepositoryInput{ + {RepositoryID: "repo-1", BaseBranch: "main"}, + }, + }) + if err != nil { + t.Fatalf("CreateTask: %v", err) + } + // No task_environments row — exercises the pre-launch path so we don't + // also need a materializer for the happy persist assertion. + prober := &stubProber{branch: "main"} + svc.SetProviderDefaultBranchProber(prober) + + added, err := svc.AddBranchToTask(ctx, AddBranchToTaskRequest{ + TaskID: task.ID, + GitHubURL: "https://github.com/acme/widgets", + }) + if err != nil { + t.Fatalf("AddBranchToTask: %v", err) + } + if prober.calls != 1 { + t.Errorf("expected probe to run once, got %d calls", prober.calls) + } + if prober.lastProviderArg != "github" || prober.lastOwner != "acme" || prober.lastName != "widgets" { + t.Errorf("unexpected probe args: %+v", prober) + } + if added.BaseBranch != "main" { + t.Errorf("expected task_repositories.base_branch=main, got %q", added.BaseBranch) + } + created, err := svc.GetRepository(ctx, added.RepositoryID) + if err != nil || created == nil { + t.Fatalf("GetRepository: %v", err) + } + if created.DefaultBranch != "main" { + t.Errorf("expected repositories.default_branch=main, got %q", created.DefaultBranch) + } + if created.ProviderOwner != "acme" || created.ProviderName != "widgets" { + t.Errorf("unexpected provider info: %+v", created) + } +} + +// TestAddBranchToTask_MaterializeFailureRollsBackOnLiveTask covers the +// materialize-failure path on an already-launched worktree task: the +// task_repositories row must be deleted and the error must propagate to the +// caller rather than being swallowed. Also pins that no task.updated event +// is published on the rollback path — emitting one would push a phantom row +// to WS clients and require a second event to undo it. +func TestAddBranchToTask_MaterializeFailureRollsBackOnLiveTask(t *testing.T) { + svc, eventBus, repo := createTestService(t) + ctx := context.Background() + + _ = repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-1", Name: "WS"}) + _ = repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-1", WorkspaceID: "ws-1", Name: "WF"}) + _ = repo.CreateRepository(ctx, &models.Repository{ID: "repo-1", WorkspaceID: "ws-1", Name: "frontend", DefaultBranch: "main"}) + + task, err := svc.CreateTask(ctx, &CreateTaskRequest{ + WorkspaceID: "ws-1", + WorkflowID: "wf-1", + WorkflowStepID: "step-1", + Title: "Live task", + Repositories: []TaskRepositoryInput{ + {RepositoryID: "repo-1", BaseBranch: "main"}, + }, + }) + if err != nil { + t.Fatalf("CreateTask: %v", err) + } + seedWorktreeTaskEnv(t, repo, task.ID, "env-1") + mat := &stubMaterializer{err: fmt.Errorf("simulated git failure")} + svc.SetBranchMaterializer(mat) + + beforeRows, _ := repo.ListTaskRepositories(ctx, task.ID) + eventBus.ClearEvents() + _, err = svc.AddBranchToTask(ctx, AddBranchToTaskRequest{ + TaskID: task.ID, + RepositoryID: "repo-1", + BaseBranch: "main", + CheckoutBranch: "feature/x", + }) + if err == nil { + t.Fatal("expected materialize failure to propagate") + } + if !strings.Contains(err.Error(), "simulated git failure") { + t.Errorf("unexpected error: %v", err) + } + if mat.calls != 1 { + t.Errorf("expected materializer to be called once, got %d", mat.calls) + } + afterRows, _ := repo.ListTaskRepositories(ctx, task.ID) + if len(afterRows) != len(beforeRows) { + t.Errorf("expected task_repositories row rolled back; before=%d after=%d", len(beforeRows), len(afterRows)) + } + for _, evt := range eventBus.GetPublishedEvents() { + if evt.Type == events.TaskUpdated { + t.Errorf("did not expect task.updated on rollback path; got %+v", evt) + } + } +} + +// TestAddBranchToTask_MaterializeSkippedPreLaunchStillSucceeds preserves the +// legacy best-effort behaviour for pre-launch tasks: materializer failure on +// a task with no task_environments row is logged-and-continued, and the +// task_repositories row stays for the launcher to pick up. +func TestAddBranchToTask_MaterializeSkippedPreLaunchStillSucceeds(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + + _ = repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-1", Name: "WS"}) + _ = repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-1", WorkspaceID: "ws-1", Name: "WF"}) + _ = repo.CreateRepository(ctx, &models.Repository{ID: "repo-1", WorkspaceID: "ws-1", Name: "frontend", DefaultBranch: "main"}) + + task, err := svc.CreateTask(ctx, &CreateTaskRequest{ + WorkspaceID: "ws-1", + WorkflowID: "wf-1", + WorkflowStepID: "step-1", + Title: "Pre-launch", + Repositories: []TaskRepositoryInput{ + {RepositoryID: "repo-1", BaseBranch: "main"}, + }, + }) + if err != nil { + t.Fatalf("CreateTask: %v", err) + } + // No task_environments row → taskAlreadyLaunched reports false. + mat := &stubMaterializer{err: fmt.Errorf("ignored on pre-launch tasks")} + svc.SetBranchMaterializer(mat) + + added, err := svc.AddBranchToTask(ctx, AddBranchToTaskRequest{ + TaskID: task.ID, + RepositoryID: "repo-1", + BaseBranch: "main", + CheckoutBranch: "feature/x", + }) + if err != nil { + t.Fatalf("AddBranchToTask should not surface materialize error pre-launch: %v", err) + } + if added.CheckoutBranch != "feature/x" { + t.Errorf("unexpected row: %+v", added) + } + rows, _ := repo.ListTaskRepositories(ctx, task.ID) + if len(rows) != 2 { + t.Errorf("expected row to remain after pre-launch materialize failure; got %d rows", len(rows)) + } +} diff --git a/apps/backend/internal/task/service/service_requests.go b/apps/backend/internal/task/service/service_requests.go index 7e53cb4287..70eb4e8f4a 100644 --- a/apps/backend/internal/task/service/service_requests.go +++ b/apps/backend/internal/task/service/service_requests.go @@ -18,6 +18,15 @@ type TaskRepositoryInput struct { Name string `json:"name,omitempty"` DefaultBranch string `json:"default_branch,omitempty"` GitHubURL string `json:"github_url,omitempty"` + + // ResolveProviderDefaults opts the GitHub-URL resolution path into a + // synchronous default-branch probe (git ls-remote --symref) when neither + // the input nor the existing workspace repo carries one. Set only by + // callers that have no downstream backfill (e.g. add_branch_to_task on a + // live worktree-executor task — no executor relaunch will run + // backfillRepoDefaultBranch). Left zero by create_task so the pinned + // "empty default_branch is filled at clone time" contract stays intact. + ResolveProviderDefaults bool `json:"-"` } // CreateTaskRequest contains the data for creating a new task diff --git a/apps/backend/internal/task/service/service_resources.go b/apps/backend/internal/task/service/service_resources.go index 99403371e1..74989bfa2d 100644 --- a/apps/backend/internal/task/service/service_resources.go +++ b/apps/backend/internal/task/service/service_resources.go @@ -318,24 +318,43 @@ func (s *Service) GetRepositoryByProviderInfo(ctx context.Context, workspaceID, // FindOrCreateRepository looks up a repository by provider info, creating one if not found. // If the repository exists but has no LocalPath and the request provides one, updates LocalPath. -func (s *Service) FindOrCreateRepository(ctx context.Context, req *FindOrCreateRepositoryRequest) (*models.Repository, error) { +// +// Returns created=true only when CreateRepository was invoked by this call. +// Callers that register cleanup on the new row (e.g. add_branch_to_task's +// orphan-rollback path) must gate it on that flag instead of inferring +// ownership from a workspace snapshot — a concurrent request can win the +// create race between snapshot and lookup, so a snapshot-miss does NOT +// mean this call created the row. +func (s *Service) FindOrCreateRepository(ctx context.Context, req *FindOrCreateRepositoryRequest) (*models.Repository, bool, error) { existing, err := s.repoEntities.GetRepositoryByProviderInfo(ctx, req.WorkspaceID, req.Provider, req.ProviderOwner, req.ProviderName) if err != nil { - return nil, fmt.Errorf("lookup repository: %w", err) + return nil, false, fmt.Errorf("lookup repository: %w", err) } if existing != nil { + dirty := false if existing.LocalPath == "" && req.LocalPath != "" { existing.LocalPath = req.LocalPath + dirty = true + } + // Backfill default_branch when the caller carries one and the existing + // row is still empty. Lets the synchronous add_branch probe persist its + // answer onto a previously-empty Repository row (e.g. one created by + // an earlier create_task that left default_branch unset). + if existing.DefaultBranch == "" && req.DefaultBranch != "" { + existing.DefaultBranch = req.DefaultBranch + dirty = true + } + if dirty { if updateErr := s.repoEntities.UpdateRepository(ctx, existing); updateErr != nil { - s.logger.Warn("failed to update repository local path", + s.logger.Warn("failed to backfill repository fields", zap.String("repository_id", existing.ID), zap.Error(updateErr)) } } - return existing, nil + return existing, false, nil } name := fmt.Sprintf("%s/%s", req.ProviderOwner, req.ProviderName) - return s.CreateRepository(ctx, &CreateRepositoryRequest{ + created, createErr := s.CreateRepository(ctx, &CreateRepositoryRequest{ WorkspaceID: req.WorkspaceID, Name: name, SourceType: "provider", @@ -345,6 +364,10 @@ func (s *Service) FindOrCreateRepository(ctx context.Context, req *FindOrCreateR ProviderName: req.ProviderName, DefaultBranch: req.DefaultBranch, }) + if createErr != nil { + return nil, false, createErr + } + return created, true, nil } func (s *Service) UpdateRepository(ctx context.Context, id string, req *UpdateRepositoryRequest) (*models.Repository, error) { diff --git a/apps/backend/internal/task/service/service_tasks.go b/apps/backend/internal/task/service/service_tasks.go index df32540521..2382eecd01 100644 --- a/apps/backend/internal/task/service/service_tasks.go +++ b/apps/backend/internal/task/service/service_tasks.go @@ -303,7 +303,7 @@ func (s *Service) createTaskRepositories(ctx context.Context, taskID, workspaceI seen := make(map[string]bool, len(repositories)) for i, repoInput := range repositories { - repositoryID, baseBranch, err := s.resolveRepoInput(ctx, workspaceID, repoInput, repoByPath) + repositoryID, baseBranch, _, err := s.resolveRepoInput(ctx, workspaceID, repoInput, repoByPath) if err != nil { return err } @@ -383,12 +383,12 @@ func (s *Service) repoDisplayLabel(ctx context.Context, repoInput TaskRepository // Accepts inputs identified by RepositoryID, GitHubURL, or LocalPath. Returns // an empty repositoryID with no error when none of those are set, letting // callers decide whether to fall back to other defaults. -func (s *Service) ResolveRepositoryRef(ctx context.Context, workspaceID string, repoInput TaskRepositoryInput) (repositoryID, baseBranch string, err error) { +func (s *Service) ResolveRepositoryRef(ctx context.Context, workspaceID string, repoInput TaskRepositoryInput) (repositoryID, baseBranch string, created bool, err error) { var repoByPath map[string]*models.Repository if repoInput.RepositoryID == "" && repoInput.LocalPath != "" { repos, listErr := s.repoEntities.ListRepositories(ctx, workspaceID) if listErr != nil { - return "", "", listErr + return "", "", false, listErr } repoByPath = make(map[string]*models.Repository, len(repos)) for _, repo := range repos { @@ -402,8 +402,11 @@ func (s *Service) ResolveRepositoryRef(ctx context.Context, workspaceID string, } // resolveRepoInput resolves a RepositoryInput to a repositoryID and baseBranch, -// creating the repository if it doesn't exist yet. -func (s *Service) resolveRepoInput(ctx context.Context, workspaceID string, repoInput TaskRepositoryInput, repoByPath map[string]*models.Repository) (repositoryID, baseBranch string, err error) { +// creating the repository if it doesn't exist yet. Returns created=true only +// when this call inserted a new Repository row (GitHub-URL miss → CreateRepository +// or LocalPath miss → CreateRepository); callers that want to roll back a fresh +// row on a later failure key off this flag. +func (s *Service) resolveRepoInput(ctx context.Context, workspaceID string, repoInput TaskRepositoryInput, repoByPath map[string]*models.Repository) (repositoryID, baseBranch string, created bool, err error) { repositoryID = repoInput.RepositoryID baseBranch = repoInput.BaseBranch if repositoryID != "" { @@ -414,45 +417,35 @@ func (s *Service) resolveRepoInput(ctx context.Context, workspaceID string, repo // scope through FindOrCreateRepository, which is workspace-bound). repo, lookupErr := s.repoEntities.GetRepository(ctx, repositoryID) if lookupErr != nil { - return "", "", fmt.Errorf("looking up repository %q: %w", repositoryID, lookupErr) + return "", "", false, fmt.Errorf("looking up repository %q: %w", repositoryID, lookupErr) } if repo == nil || repo.WorkspaceID != workspaceID { - return "", "", fmt.Errorf("repository %q does not belong to workspace %q", repositoryID, workspaceID) + return "", "", false, fmt.Errorf("repository %q does not belong to workspace %q", repositoryID, workspaceID) } - return repositoryID, baseBranch, nil + return repositoryID, baseBranch, false, nil } // Handle GitHub URL: parse owner/name and use FindOrCreateRepository if repoInput.GitHubURL != "" { - owner, name, parseErr := parseGitHubRepoURL(repoInput.GitHubURL) - if parseErr != nil { - return "", "", parseErr - } - defaultBranch := repoInput.DefaultBranch - if defaultBranch == "" { - defaultBranch = repoInput.BaseBranch - } - repo, createErr := s.FindOrCreateRepository(ctx, &FindOrCreateRepositoryRequest{ - WorkspaceID: workspaceID, - Provider: "github", - ProviderOwner: owner, - ProviderName: name, - DefaultBranch: defaultBranch, - }) - if createErr != nil { - return "", "", createErr - } - repositoryID = repo.ID - if baseBranch == "" { - baseBranch = repo.DefaultBranch - } - return repositoryID, baseBranch, nil + return s.resolveRepoInputGitHub(ctx, workspaceID, repoInput, baseBranch) } if repoInput.LocalPath == "" { - return repositoryID, baseBranch, nil + return repositoryID, baseBranch, false, nil } + return s.resolveRepoInputLocal(ctx, workspaceID, repoInput, repoByPath, baseBranch) +} + +// resolveRepoInputLocal handles the LocalPath branch of resolveRepoInput. +// Looks the path up in the workspace snapshot; on miss, calls +// CreateRepository (and reports created=true). Extracted to keep +// resolveRepoInput inside the cyclomatic-complexity budget. +func (s *Service) resolveRepoInputLocal( + ctx context.Context, workspaceID string, repoInput TaskRepositoryInput, + repoByPath map[string]*models.Repository, baseBranch string, +) (string, string, bool, error) { repo := repoByPath[repoInput.LocalPath] + created := false if repo == nil { name := strings.TrimSpace(repoInput.Name) if name == "" { @@ -478,7 +471,7 @@ func (s *Service) resolveRepoInput(ctx context.Context, workspaceID string, repo } } } - created, createErr := s.CreateRepository(ctx, &CreateRepositoryRequest{ + createdRepo, createErr := s.CreateRepository(ctx, &CreateRepositoryRequest{ WorkspaceID: workspaceID, Name: name, SourceType: "local", @@ -486,18 +479,83 @@ func (s *Service) resolveRepoInput(ctx context.Context, workspaceID string, repo DefaultBranch: defaultBranch, }) if createErr != nil { - return "", "", createErr + return "", "", false, createErr } - repo = created + repo = createdRepo if repoByPath != nil { repoByPath[repoInput.LocalPath] = repo } + created = true } - repositoryID = repo.ID if baseBranch == "" { baseBranch = repo.DefaultBranch } - return repositoryID, baseBranch, nil + return repo.ID, baseBranch, created, nil +} + +// resolveRepoInputGitHub handles the GitHub-URL branch of resolveRepoInput: +// parse owner/name, optionally probe the provider for default_branch, then +// FindOrCreateRepository. Extracted so resolveRepoInput stays under the +// cognitive-complexity budget after adding the probe-skip and probe-error +// arms. +func (s *Service) resolveRepoInputGitHub( + ctx context.Context, workspaceID string, repoInput TaskRepositoryInput, baseBranch string, +) (string, string, bool, error) { + owner, name, parseErr := parseGitHubRepoURL(repoInput.GitHubURL) + if parseErr != nil { + return "", "", false, parseErr + } + defaultBranch := repoInput.DefaultBranch + if defaultBranch == "" { + defaultBranch = repoInput.BaseBranch + } + if defaultBranch == "" && repoInput.ResolveProviderDefaults && s.providerProber != nil { + defaultBranch = s.probeProviderDefaultBranchIfMissing(ctx, workspaceID, "github", owner, name) + } + repo, repoCreated, createErr := s.FindOrCreateRepository(ctx, &FindOrCreateRepositoryRequest{ + WorkspaceID: workspaceID, + Provider: "github", + ProviderOwner: owner, + ProviderName: name, + DefaultBranch: defaultBranch, + }) + if createErr != nil { + return "", "", false, createErr + } + if baseBranch == "" { + baseBranch = repo.DefaultBranch + } + return repo.ID, baseBranch, repoCreated, nil +} + +// probeProviderDefaultBranchIfMissing returns a default_branch resolved via +// the provider prober, but only when the workspace doesn't already hold the +// repo with a non-empty default_branch (the existing value wins downstream, +// so the remote round-trip would be pure waste). A DB lookup error skips +// the probe entirely — FindOrCreateRepository will hit the same DB and +// surface the real cause; we log the lookup failure for observability. +// Probe errors fall through to "" so the AddBranchToTask gate surfaces an +// actionable validation rejection rather than a silent orphan. +func (s *Service) probeProviderDefaultBranchIfMissing( + ctx context.Context, workspaceID, provider, owner, name string, +) string { + existing, lookupErr := s.repoEntities.GetRepositoryByProviderInfo(ctx, workspaceID, provider, owner, name) + if lookupErr != nil { + s.logger.Warn("resolveRepoInput: failed to look up existing repo before probe", + zap.String("provider", provider), + zap.String("owner", owner), + zap.String("name", name), + zap.Error(lookupErr)) + return "" + } + if existing != nil && existing.DefaultBranch != "" { + return "" + } + probed, probeErr := s.providerProber.ProbeDefaultBranch(ctx, provider, owner, name) + if probeErr != nil { + return "" + } + return probed } // parseGitHubRepoURL parses a GitHub repository URL into owner and name. diff --git a/apps/backend/internal/worktree/worktree.go b/apps/backend/internal/worktree/worktree.go index 8c80f01f00..05e0062eb4 100644 --- a/apps/backend/internal/worktree/worktree.go +++ b/apps/backend/internal/worktree/worktree.go @@ -207,7 +207,14 @@ func (r *CreateRequest) Validate() error { return ErrRepoNotGit } if r.BaseBranch == "" { - return ErrInvalidBaseBranch + // Defence-in-depth: prefer the explicit FallbackBaseBranch (typically + // the repository's default_branch carried by the caller) over an + // outright rejection. The manager's branchExists check still verifies + // the resulting ref before any git work runs. + if r.FallbackBaseBranch == "" { + return ErrInvalidBaseBranch + } + r.BaseBranch = r.FallbackBaseBranch } return nil } diff --git a/apps/backend/internal/worktree/worktree_validate_test.go b/apps/backend/internal/worktree/worktree_validate_test.go new file mode 100644 index 0000000000..d39b28f405 --- /dev/null +++ b/apps/backend/internal/worktree/worktree_validate_test.go @@ -0,0 +1,51 @@ +package worktree + +import ( + "errors" + "testing" +) + +// TestCreateRequest_Validate_FallsBackToFallbackBaseBranch pins the +// defence-in-depth path added with the add_branch_to_task fix: when the +// caller forgot to set BaseBranch but supplied a FallbackBaseBranch +// (typically the repository's persisted default_branch), Validate adopts +// the fallback instead of returning ErrInvalidBaseBranch. +func TestCreateRequest_Validate_FallsBackToFallbackBaseBranch(t *testing.T) { + r := &CreateRequest{ + TaskID: "task-1", + RepositoryPath: "/tmp/repo", + BaseBranch: "", + FallbackBaseBranch: "main", + } + if err := r.Validate(); err != nil { + t.Fatalf("Validate with fallback should succeed: %v", err) + } + if r.BaseBranch != "main" { + t.Errorf("expected BaseBranch promoted from fallback, got %q", r.BaseBranch) + } +} + +func TestCreateRequest_Validate_NoFallbackStillRejects(t *testing.T) { + r := &CreateRequest{ + TaskID: "task-1", + RepositoryPath: "/tmp/repo", + } + if err := r.Validate(); !errors.Is(err, ErrInvalidBaseBranch) { + t.Errorf("expected ErrInvalidBaseBranch, got %v", err) + } +} + +func TestCreateRequest_Validate_ExplicitBaseWinsOverFallback(t *testing.T) { + r := &CreateRequest{ + TaskID: "task-1", + RepositoryPath: "/tmp/repo", + BaseBranch: "develop", + FallbackBaseBranch: "main", + } + if err := r.Validate(); err != nil { + t.Fatalf("Validate: %v", err) + } + if r.BaseBranch != "develop" { + t.Errorf("expected explicit BaseBranch to be preserved, got %q", r.BaseBranch) + } +}