fix(task): surface add_branch_to_task failures instead of silent orphans (#1264)

* fix(worktree): promote FallbackBaseBranch when BaseBranch is empty

CreateRequest.Validate previously hard-rejected an empty BaseBranch even
when the caller supplied a FallbackBaseBranch. The fallback is plumbed
specifically so a missing primary branch can degrade into the repo's
default — this lets that path actually fire.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* feat(task): probe provider default branch in add_branch resolve path

Adds an opt-in ProviderDefaultBranchProber seam (impl wired in cmd/kandev)
and threads a ResolveProviderDefaults flag through TaskRepositoryInput so
the add_branch_to_task GitHub-URL path can resolve a real default_branch
before writing the Repository row. Also backfills the existing repo's
default_branch when the probe succeeds and the workspace already had a row
with an empty value. The create_task path keeps its zero-default contract
(executor still backfills after clone).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(task): surface add_branch_to_task failures instead of silent orphans

AddBranchToTask used to return a success envelope even when the
post-insert worktree materialize step failed on a live task. The
task_repositories row stayed in place with no on-disk worktree, and
provider-URL flows also left an orphan repositories row behind.

Three coordinated changes:
- Gate empty base_branch after resolution with a typed error so no row
  is written when nothing can be materialized.
- Track whether the repository row was created by this call and roll it
  back on any subsequent failure.
- materializeBranch now returns its error; on a live worktree-executor
  task, AddBranchToTask rolls back the task_repositories row and surfaces
  the failure. Pre-launch tasks keep the legacy best-effort path so the
  next session launch still materializes via the multi-repo prepare flow.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(mcp): classify add_branch base_branch resolution failure as validation

AddBranchToTask now returns a "cannot resolve base_branch" error when the
provider repo's default branch could not be probed. Map it to
ErrorCodeValidation so MCP agents see a user-fixable input issue rather
than an opaque internal error.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(task): defer task.updated until after worktree materializes

Publishing task.updated before materializeBranch ran meant a failed
materialize on a live task would push the new task_repository row to WS
clients and then silently roll it back, leaving stale UI state until the
next poll. Move the event after materialize success so the bus only sees
durable rows. Test now also asserts no task.updated fires on the rollback
path. Renames stubProber.lastProvid to lastProviderArg for clarity.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(task): emit repository.deleted on add_branch orphan rollback

Cleanup callback previously called repoEntities.DeleteRepository directly,
which soft-deletes the row but bypasses Service.DeleteRepository's event
publish. The create path emitted repository.created, so skipping the
delete event left WS subscribers and frontend caches holding a phantom
row after a resolve-then-fail sequence. Route through the service method
so the events stay symmetric. Test now asserts the symmetry.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(task): skip provider probe when workspace already has the repo

resolveRepoInput's add_branch probe fired on every add_branch_to_task
call with a github_url, even when the workspace already had the
provider repo registered with a non-empty default_branch. The existing
row's value would win seconds later via the repo.DefaultBranch fallback,
so the remote git ls-remote round-trip was pure waste — and unnecessarily
hit network on every call. Peek at the workspace registry first and
short-circuit when a usable default_branch is already on file.

Also adds a QA-tier test file covering probe-error fallthrough, bad URL
rejection, registered-repo probe skipping, empty-default backfill, and
the repository_id fast path.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(task): detach rollback paths from caller ctx, log probe-lookup errors

Two adjacent robustness fixes flagged on PR #1264:

1. resolveRepoInput silently discarded the GetRepositoryByProviderInfo
   error before the provider probe. A transient DB failure was
   indistinguishable from a clean "not found" lookup, so the code would
   pay for a network probe instead of failing fast. Surface the error in
   logs and skip the probe — FindOrCreateRepository below already hits
   the same DB and will return the real cause.

2. The repository-create cleanup callback in resolveBranchAddRepoRef and
   the task_repositories rollback in AddBranchToTask both captured the
   caller ctx for their DELETE queries. Rollback often runs precisely
   because that ctx was cancelled (timeout, client disconnect), and
   reusing it would kill the cleanup and orphan the row we just created.
   Use context.WithoutCancel so the rollback survives ctx death.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(task): close orphan-row gaps from caller cancel and concurrent create

Two findings from Claude's review on PR #1264:

1. (blocker) materializeBranch called taskAlreadyLaunched(ctx, ...) after
   MaterializeBranch returned, using the same caller context that often
   failed the materialize in the first place. A cancelled ctx made the
   post-failure DB lookup return (nil, err), taskAlreadyLaunched returned
   false, and the live-task rollback path silently demoted itself to the
   pre-launch best-effort branch — leaving the orphan task_repositories
   row the PR exists to prevent. Snapshot the launch state BEFORE
   MaterializeBranch (launch state can't change during the synchronous
   call) so the rollback decision is anchored to a live-ctx answer.

2. (suggestion) resolveBranchAddRepoRef inferred "this call created the
   repo" by comparing the resolved ID against a workspace ListRepositories
   snapshot taken before FindOrCreateRepository. A concurrent add_branch
   for the same GitHub URL could create the row between snapshot and
   lookup; the second request would treat it as "not in my snapshot, must
   be mine" and register cleanup against the other request's data. On any
   later failure, cleanup would DELETE a repository this call didn't
   create and emit a spurious repository.deleted event.

   Thread an explicit `created bool` out of CreateRepository through
   FindOrCreateRepository → resolveRepoInput → ResolveRepositoryRef so
   the cleanup callback fires only when this call physically inserted
   the row. The flag is set at the CreateRepository / FindOrCreateRepository
   sites themselves and cannot be spoofed by concurrent activity.

resolveRepoInput's branches were extracted into resolveRepoInputGitHub /
resolveRepoInputLocal helpers to keep the parent function inside the
cyclomatic-complexity and cognitive-complexity budgets after the signature
change.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* chore: retrigger cubic CI

---------

Co-authored-by: Kandev Agent <agent@kandev.dev>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
João Salavisa
2026-06-03 15:43:41 +01:00
committed by GitHub
parent 9aaf3170e0
commit be9821fc55
13 changed files with 906 additions and 83 deletions
+1 -1
View File
@@ -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,
@@ -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
@@ -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"),
@@ -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
@@ -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
@@ -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
}
@@ -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)
}
}
@@ -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))
}
}
@@ -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
@@ -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) {
@@ -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.
+8 -1
View File
@@ -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
}
@@ -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)
}
}