feat(mcp): accept repository_url and local_path on add_branch_to_task_kandev (#1256)
* feat(mcp): accept repository_url and local_path on add_branch_to_task_kandev Mirror create_task_kandev's repo-locator surface so agents can attach a branch without first looking up the repository UUID. The MCP tool now takes repository_id OR repository_url OR local_path; the service resolves alternatives via the same workspace-scoped find-or-create path used by create_task. * fix(mcp): run executor guard before repo resolution on add_branch_to_task ResolveRepositoryRef may FindOrCreateRepository / CreateRepository inside the github_url / local_path paths. Running it before requireWorktreeExecutorForBranchAdd let a rejected add_branch call on a docker/sprites/SSH task leak an orphan Repository row into the workspace. Move the executor check ahead of resolution and add a regression test that asserts ListRepositories count is unchanged after a rejected call with a brand-new github_url. * test(mcp): assert task_id default in add_branch local_path test * fix(mcp): reject multiple repo locators on add_branch_to_task resolveRepoInput applies a silent precedence (repository_id > github_url > local_path), so an agent that accidentally supplies two of the three locators got a behaviour change with no signal. Reject at the WS handler with a clear error so the mistake is surfaced. Also refresh the AddBranchToTask docstring to document the new LocalPath / GitHubURL inputs and to correct the workspace-membership check ownership (now enforced by ResolveRepositoryRef → resolveRepoInput for the alternative paths). * fix(mcp): classify resolve_repository errors as validation, not internal Failures from ResolveRepositoryRef are user-fixable input mistakes (malformed URL, unknown local_path, cross-workspace repository_id). Without a matching case the wrapper "resolve repository: ..." message fell through to ErrorCodeInternalError, so an agent that supplied e.g. a bitbucket URL saw a server-fault code instead of a validation error it could correct. * fix(mcp): narrow add_branch error classification + use wire field name Two follow-ups to the round-3/4 review feedback: - Mutual-exclusion error mentioned the MCP tool alias `repository_url` but the WS handler sees the wire field `github_url`. A direct WS caller would see an error naming a field it never passed. - The broad `"resolve repository: "` prefix classified ALL wrapped errors as Validation, including DB / system failures from CreateRepository / ListRepositories inside ResolveRepositoryRef. Narrow to specific user-fixable patterns (GitHub URL parse failures, cross-workspace repository_id) so genuine internal errors no longer masquerade as validation rejections. * fix(mcp): mutual-exclusion check at MCP tier names agent-facing alias WS-tier rejection works for direct WS callers but its error mentions github_url, the wire field — confusing for MCP agents that supplied repository_url. Move the check up to addBranchToTaskHandler so the error reads "pass at most one of repository_id, repository_url, local_path" using the names the agent actually typed. WS handler still validates as defense-in-depth. --------- Co-authored-by: Kandev Agent <agent@kandev.dev>
This commit is contained in:
@@ -738,6 +738,8 @@ func (h *Handlers) handleAddBranchToTask(ctx context.Context, msg *ws.Message) (
|
||||
var req struct {
|
||||
TaskID string `json:"task_id"`
|
||||
RepositoryID string `json:"repository_id"`
|
||||
LocalPath string `json:"local_path"`
|
||||
GitHubURL string `json:"github_url"`
|
||||
BaseBranch string `json:"base_branch"`
|
||||
CheckoutBranch string `json:"checkout_branch"`
|
||||
}
|
||||
@@ -747,12 +749,27 @@ func (h *Handlers) handleAddBranchToTask(ctx context.Context, msg *ws.Message) (
|
||||
if req.TaskID == "" {
|
||||
return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, "task_id is required", nil)
|
||||
}
|
||||
// repository_id is optional: the service defaults to the task's only
|
||||
// repository (or its primary row) when omitted. Multi-repo tasks force
|
||||
// the agent to pass one explicitly via the service-level error.
|
||||
// Mutual exclusion across the three repo identifiers. resolveRepoInput
|
||||
// applies a silent precedence (repository_id > github_url > local_path),
|
||||
// so an agent that accidentally passes two of them gets a behaviour
|
||||
// change with no signal. Reject early instead so the agent sees the
|
||||
// mistake.
|
||||
if locatorCount := boolCount(req.RepositoryID != "", req.LocalPath != "", req.GitHubURL != ""); locatorCount > 1 {
|
||||
return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation,
|
||||
"pass at most one of repository_id, github_url, local_path", nil)
|
||||
}
|
||||
// repository_id / local_path / github_url are all optional: the service
|
||||
// defaults to the task's only repository (or its primary row) when none
|
||||
// is supplied. Multi-repo tasks force the agent to pass one explicitly
|
||||
// via the service-level error. local_path and github_url are
|
||||
// agent-ergonomic alternatives — when supplied the service resolves
|
||||
// them through the same workspace-scoped find-or-create path used by
|
||||
// create_task.
|
||||
taskRepo, err := h.taskSvc.AddBranchToTask(ctx, service.AddBranchToTaskRequest{
|
||||
TaskID: req.TaskID,
|
||||
RepositoryID: req.RepositoryID,
|
||||
LocalPath: req.LocalPath,
|
||||
GitHubURL: req.GitHubURL,
|
||||
BaseBranch: req.BaseBranch,
|
||||
CheckoutBranch: req.CheckoutBranch,
|
||||
})
|
||||
@@ -771,6 +788,19 @@ func (h *Handlers) handleAddBranchToTask(ctx context.Context, msg *ws.Message) (
|
||||
})
|
||||
}
|
||||
|
||||
// boolCount returns how many of the supplied boolean flags are true. Used
|
||||
// to enforce mutual exclusion across optional input fields without a chain
|
||||
// of nested ifs.
|
||||
func boolCount(flags ...bool) int {
|
||||
n := 0
|
||||
for _, b := range flags {
|
||||
if b {
|
||||
n++
|
||||
}
|
||||
}
|
||||
return n
|
||||
}
|
||||
|
||||
// classifyAddBranchError maps service-layer add_branch failures to ws error
|
||||
// codes so MCP agents can react to user-fixable input mistakes (missing
|
||||
// task, duplicate branch, wrong executor) instead of treating them as
|
||||
@@ -793,6 +823,15 @@ func classifyAddBranchError(err error) string {
|
||||
strings.Contains(msg, "only supported on the worktree executor"),
|
||||
strings.Contains(msg, "task_id is required"):
|
||||
return ws.ErrorCodeValidation
|
||||
case strings.Contains(msg, "GitHub URL"),
|
||||
strings.Contains(msg, "github.com/owner/repo"),
|
||||
strings.Contains(msg, "does not belong to workspace"):
|
||||
// User-fixable failures from ResolveRepositoryRef / parseGitHubRepoURL:
|
||||
// malformed URL, non-github host, cross-workspace repository_id.
|
||||
// Narrow patterns (not a broad "resolve repository:" prefix) so
|
||||
// downstream DB / system errors from CreateRepository / ListRepositories
|
||||
// still classify as InternalError.
|
||||
return ws.ErrorCodeValidation
|
||||
}
|
||||
return ws.ErrorCodeInternalError
|
||||
}
|
||||
|
||||
@@ -100,6 +100,24 @@ func (b *mcpRecordingEventBus) Publish(_ context.Context, _ string, event *bus.E
|
||||
return nil
|
||||
}
|
||||
|
||||
// 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
|
||||
// silently resolved by the resolveRepoInput precedence chain. Now it
|
||||
// surfaces as a validation error so the agent sees the contradiction.
|
||||
func TestHandleAddBranchToTask_RejectsMultipleLocators(t *testing.T) {
|
||||
h := &Handlers{}
|
||||
msg := makeWSMessage(t, ws.ActionMCPAddBranchToTask, map[string]interface{}{
|
||||
"task_id": "task-1",
|
||||
"local_path": "/tmp/x",
|
||||
"github_url": "https://github.com/acme/widgets",
|
||||
})
|
||||
|
||||
resp, err := h.handleAddBranchToTask(context.Background(), msg)
|
||||
require.NoError(t, err)
|
||||
assertWSError(t, resp, ws.ErrorCodeValidation)
|
||||
}
|
||||
|
||||
func TestHandleCreateTask_MissingTitle(t *testing.T) {
|
||||
h := &Handlers{}
|
||||
msg := makeWSMessage(t, ws.ActionMCPCreateTask, map[string]interface{}{
|
||||
|
||||
@@ -5,6 +5,7 @@ import (
|
||||
"testing"
|
||||
|
||||
ws "github.com/kandev/kandev/pkg/websocket"
|
||||
"github.com/mark3labs/mcp-go/mcp"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
@@ -333,6 +334,78 @@ func TestCreateTask_LocalPath_AllowedForSubtasks(t *testing.T) {
|
||||
assert.Equal(t, "/Users/me/projects/sibling", repos[0]["local_path"])
|
||||
}
|
||||
|
||||
// TestAddBranchToTask_ForwardsRepositoryURL verifies the agent-facing alias:
|
||||
// repository_url on the MCP tool surface translates to github_url on the WS
|
||||
// payload — mirroring create_task_kandev's wire format so the backend handler
|
||||
// can resolve through the same code path.
|
||||
func TestAddBranchToTask_ForwardsRepositoryURL(t *testing.T) {
|
||||
backend := &testBackend{
|
||||
response: map[string]interface{}{"id": "tr-1", "task_id": "task-current"},
|
||||
}
|
||||
s := newTaskModeServer(t, backend, "task-current")
|
||||
|
||||
result := callTool(t, s, "add_branch_to_task_kandev", map[string]interface{}{
|
||||
"repository_url": "https://github.com/acme/widgets",
|
||||
"checkout_branch": "feature/x",
|
||||
})
|
||||
|
||||
assert.False(t, result.IsError)
|
||||
assert.Equal(t, ws.ActionMCPAddBranchToTask, backend.lastAction)
|
||||
|
||||
payload, ok := backend.lastPayload.(map[string]interface{})
|
||||
require.True(t, ok)
|
||||
assert.Equal(t, "task-current", payload["task_id"], "task_id should default to current task")
|
||||
assert.Equal(t, "https://github.com/acme/widgets", payload["github_url"],
|
||||
"repository_url should be forwarded as github_url to match create_task wire format")
|
||||
assert.Equal(t, "feature/x", payload["checkout_branch"])
|
||||
assert.Equal(t, "", payload["repository_id"])
|
||||
}
|
||||
|
||||
// TestAddBranchToTask_ForwardsLocalPath verifies local_path is plumbed through
|
||||
// to the WS payload so the backend can find-or-create the repo in the task's
|
||||
// workspace.
|
||||
func TestAddBranchToTask_ForwardsLocalPath(t *testing.T) {
|
||||
backend := &testBackend{
|
||||
response: map[string]interface{}{"id": "tr-1", "task_id": "task-current"},
|
||||
}
|
||||
s := newTaskModeServer(t, backend, "task-current")
|
||||
|
||||
result := callTool(t, s, "add_branch_to_task_kandev", map[string]interface{}{
|
||||
"local_path": "/Users/me/projects/sibling",
|
||||
"checkout_branch": "feature/y",
|
||||
})
|
||||
|
||||
assert.False(t, result.IsError)
|
||||
payload, ok := backend.lastPayload.(map[string]interface{})
|
||||
require.True(t, ok)
|
||||
assert.Equal(t, "task-current", payload["task_id"], "task_id should default to current task")
|
||||
assert.Equal(t, "/Users/me/projects/sibling", payload["local_path"])
|
||||
assert.Equal(t, "feature/y", payload["checkout_branch"])
|
||||
assert.Equal(t, "", payload["repository_id"])
|
||||
}
|
||||
|
||||
// TestAddBranchToTask_RejectsMultipleLocators verifies the MCP-tier
|
||||
// mutual-exclusion check fires before the request hits the WS handler, so
|
||||
// the error names the agent-facing alias (repository_url) instead of the
|
||||
// wire field (github_url).
|
||||
func TestAddBranchToTask_RejectsMultipleLocators(t *testing.T) {
|
||||
backend := &testBackend{}
|
||||
s := newTaskModeServer(t, backend, "task-current")
|
||||
|
||||
result := callTool(t, s, "add_branch_to_task_kandev", map[string]interface{}{
|
||||
"repository_url": "https://github.com/acme/widgets",
|
||||
"local_path": "/Users/me/projects/sibling",
|
||||
})
|
||||
|
||||
assert.True(t, result.IsError, "passing both repository_url and local_path should error at the MCP tier")
|
||||
require.NotEmpty(t, result.Content)
|
||||
text, ok := result.Content[0].(mcp.TextContent)
|
||||
require.True(t, ok)
|
||||
assert.Contains(t, text.Text, "repository_url",
|
||||
"MCP-tier error should name the agent-facing alias, not the wire key")
|
||||
assert.Nil(t, backend.lastPayload, "request must not be forwarded to the backend")
|
||||
}
|
||||
|
||||
func TestMessageTask_ForwardsToBackend(t *testing.T) {
|
||||
backend := &testBackend{
|
||||
response: map[string]interface{}{
|
||||
|
||||
@@ -46,10 +46,27 @@ const (
|
||||
const (
|
||||
mcpKeyTaskID = "task_id"
|
||||
mcpKeyRepositoryID = "repository_id"
|
||||
mcpKeyRepositoryURL = "repository_url"
|
||||
mcpKeyLocalPath = "local_path"
|
||||
mcpKeyGitHubURL = "github_url"
|
||||
mcpKeyBaseBranch = "base_branch"
|
||||
mcpKeyCheckoutBranch = "checkout_branch"
|
||||
)
|
||||
|
||||
// locatorCount returns how many of the supplied repository-locator strings
|
||||
// are non-empty. Used by add_branch / create_task mutual-exclusion checks
|
||||
// so a chain of `if a != "" && b != "" { ... }` doesn't repeat at each call
|
||||
// site.
|
||||
func locatorCount(locators ...string) int {
|
||||
n := 0
|
||||
for _, s := range locators {
|
||||
if s != "" {
|
||||
n++
|
||||
}
|
||||
}
|
||||
return n
|
||||
}
|
||||
|
||||
// normalizeMode returns a valid MCP mode, defaulting unknown values to ModeTask.
|
||||
func normalizeMode(mode string) string {
|
||||
switch mode {
|
||||
@@ -558,12 +575,14 @@ Use this when the task should open more than one PR — same repo with different
|
||||
IMPORTANT:
|
||||
- Only works on tasks running the WORKTREE executor. Tasks on docker / sprites / local-pc / SSH / remote_docker reject this tool because sibling worktrees are a git-worktree-specific layout — other executors bind one workspace path per task and the new branch would silently never appear on disk.
|
||||
- task_id defaults to your CURRENT task when omitted — pass it explicitly only to target a different task.
|
||||
- repository_id is OPTIONAL when the task has a single repository attached (the common case). The service auto-resolves to that repo. Multi-repo tasks must pass it explicitly.
|
||||
- Repository selection (matches create_task_kandev): pass exactly one of repository_id / repository_url / local_path. For single-repo tasks all three are optional — the service auto-resolves to the task's only repository. Multi-repo tasks must identify the target repo explicitly.
|
||||
- checkout_branch is the branch the new worktree will check out. Leave empty to create a fresh feature branch from base_branch.
|
||||
- base_branch is optional; defaults to the repository's default_branch.
|
||||
- The (task_id, repository_id, base_branch, checkout_branch) tuple must be unique on the task — re-adding the same combination is an error, not a no-op.`),
|
||||
mcp.WithString("task_id", mcp.Description("The task to attach the branch to. Defaults to the current task when omitted.")),
|
||||
mcp.WithString("repository_id", mcp.Description("Repository to attach. Optional for single-repo tasks (auto-resolved). Required for multi-repo tasks.")),
|
||||
mcp.WithString("repository_id", mcp.Description("Repository UUID. Optional for single-repo tasks (auto-resolved). Required for multi-repo tasks unless repository_url or local_path is supplied.")),
|
||||
mcp.WithString("repository_url", mcp.Description("GitHub repository URL (e.g. 'https://github.com/owner/repo'). Alternative to repository_id when you don't have the UUID handy. The repository is found-or-created in the task's workspace.")),
|
||||
mcp.WithString("local_path", mcp.Description("Local repository folder path (e.g. '/Users/me/projects/myrepo'). Alternative to repository_id for the local worktree flow. The repository is found-or-created in the task's workspace.")),
|
||||
mcp.WithString("checkout_branch", mcp.Description("Existing branch to check out in the new worktree (e.g. a PR head branch). Empty to create a fresh feature branch from base_branch.")),
|
||||
mcp.WithString("base_branch", mcp.Description("Branch to base the worktree on. Defaults to the repository's default_branch.")),
|
||||
),
|
||||
@@ -580,9 +599,24 @@ func (s *Server) addBranchToTaskHandler() server.ToolHandlerFunc {
|
||||
if taskID == "" {
|
||||
return mcp.NewToolResultError("task_id is required (no current task context to default to)"), nil
|
||||
}
|
||||
// Mutual-exclusion gate at the MCP tier so the error names the
|
||||
// agent-facing alias (repository_url) instead of the WS wire field
|
||||
// (github_url). The WS handler still re-validates for direct WS
|
||||
// callers that don't go through this tool.
|
||||
repositoryID := req.GetString(mcpKeyRepositoryID, "")
|
||||
repositoryURL := req.GetString(mcpKeyRepositoryURL, "")
|
||||
localPath := req.GetString(mcpKeyLocalPath, "")
|
||||
if locatorCount(repositoryID, repositoryURL, localPath) > 1 {
|
||||
return mcp.NewToolResultError("pass at most one of repository_id, repository_url, local_path"), nil
|
||||
}
|
||||
// repository_url is the tool-facing alias used by create_task_kandev;
|
||||
// translate to github_url on the wire so the WS handler can reuse the
|
||||
// same field name as the rest of the multi-repo payloads.
|
||||
payload := map[string]interface{}{
|
||||
mcpKeyTaskID: taskID,
|
||||
mcpKeyRepositoryID: req.GetString(mcpKeyRepositoryID, ""),
|
||||
mcpKeyRepositoryID: repositoryID,
|
||||
mcpKeyLocalPath: localPath,
|
||||
mcpKeyGitHubURL: repositoryURL,
|
||||
mcpKeyCheckoutBranch: req.GetString(mcpKeyCheckoutBranch, ""),
|
||||
mcpKeyBaseBranch: req.GetString(mcpKeyBaseBranch, ""),
|
||||
}
|
||||
|
||||
@@ -15,9 +15,18 @@ import (
|
||||
// AddBranchToTaskRequest carries the parameters for adding a new branch
|
||||
// (worktree) to an existing task. The (RepositoryID, CheckoutBranch) pair
|
||||
// must not already exist on the task.
|
||||
//
|
||||
// RepositoryID, LocalPath, and GitHubURL are alternative ways to identify the
|
||||
// target repository — mirrors the create_task input shape. When RepositoryID
|
||||
// is empty and either LocalPath or GitHubURL is set, the service resolves to
|
||||
// (or creates) the matching repository in the task's workspace. Pure
|
||||
// RepositoryID is the fast path; the other two are agent-ergonomic
|
||||
// alternatives so callers don't have to look up the UUID first.
|
||||
type AddBranchToTaskRequest struct {
|
||||
TaskID string
|
||||
RepositoryID string
|
||||
LocalPath string
|
||||
GitHubURL string
|
||||
BaseBranch string
|
||||
CheckoutBranch string
|
||||
}
|
||||
@@ -31,16 +40,20 @@ type AddBranchToTaskRequest struct {
|
||||
//
|
||||
// Constraints:
|
||||
// - TaskID is required.
|
||||
// - RepositoryID is optional. When omitted, the service defaults to the
|
||||
// task's existing repository (the only one for single-repo tasks; the
|
||||
// primary by lowest position otherwise). Multi-repo tasks must pass an
|
||||
// explicit RepositoryID — defaulting would be ambiguous.
|
||||
// - RepositoryID, LocalPath, and GitHubURL are alternative ways to identify
|
||||
// the target repository. All three are optional: when none is supplied
|
||||
// the service defaults to the task's existing repository (the only one
|
||||
// for single-repo tasks; the primary by lowest position otherwise).
|
||||
// Multi-repo tasks must pass one explicitly — defaulting would be
|
||||
// ambiguous. LocalPath and GitHubURL are resolved (find-or-create)
|
||||
// through the same workspace-scoped path used by create_task.
|
||||
// - The (TaskID, RepositoryID, BaseBranch, CheckoutBranch) tuple must be
|
||||
// unique on the task — duplicates surface as a typed error so callers
|
||||
// can render a user-friendly message instead of a raw DB error.
|
||||
// - The repository must belong to the task's workspace; the caller is
|
||||
// responsible for that check (service_tasks.resolveRepoInput enforces it
|
||||
// during create, this method assumes the same upstream gating).
|
||||
// - The repository must belong to the task's workspace. The resolution
|
||||
// path (ResolveRepositoryRef → resolveRepoInput) enforces this for the
|
||||
// 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)
|
||||
if err != nil {
|
||||
@@ -107,9 +120,33 @@ func (s *Service) prepareBranchAdd(ctx context.Context, req *AddBranchToTaskRequ
|
||||
// rather than substring-matching the formatted UUID.
|
||||
return nil, nil, 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
|
||||
// workspace via FindOrCreateRepository / CreateRepository inside
|
||||
// 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
|
||||
}
|
||||
// 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
|
||||
}
|
||||
}
|
||||
existing, err := s.taskRepos.ListTaskRepositories(ctx, req.TaskID)
|
||||
if err != nil {
|
||||
return nil, nil, fmt.Errorf("list existing branches: %w", err)
|
||||
|
||||
@@ -439,6 +439,181 @@ func TestCreateTask_AllowsSameRepoDifferentBaseBranches(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestAddBranchToTask_ResolvesGitHubURL exercises the agent-ergonomic path
|
||||
// where the caller knows the GitHub URL but not the repository UUID. The
|
||||
// service must find-or-create the repository in the task's workspace and
|
||||
// attach the new branch row against the resolved id.
|
||||
func TestAddBranchToTask_ResolvesGitHubURL(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"})
|
||||
// Seed the target repo with provider info so FindOrCreateRepository
|
||||
// returns the existing row instead of creating a duplicate.
|
||||
_ = repo.CreateRepository(ctx, &models.Repository{
|
||||
ID: "repo-target", WorkspaceID: "ws-1", Name: "acme/widgets",
|
||||
Provider: "github", ProviderOwner: "acme", ProviderName: "widgets",
|
||||
DefaultBranch: "main",
|
||||
})
|
||||
_ = repo.CreateRepository(ctx, &models.Repository{
|
||||
ID: "repo-primary", WorkspaceID: "ws-1", Name: "primary", DefaultBranch: "main",
|
||||
})
|
||||
|
||||
// Multi-repo task — auto-default would fail, so URL resolution must
|
||||
// supply the repository_id.
|
||||
task, err := svc.CreateTask(ctx, &CreateTaskRequest{
|
||||
WorkspaceID: "ws-1",
|
||||
WorkflowID: "wf-1",
|
||||
WorkflowStepID: "step-1",
|
||||
Title: "Multi-repo task",
|
||||
Repositories: []TaskRepositoryInput{
|
||||
{RepositoryID: "repo-primary", BaseBranch: "main"},
|
||||
},
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("CreateTask: %v", err)
|
||||
}
|
||||
|
||||
added, err := svc.AddBranchToTask(ctx, AddBranchToTaskRequest{
|
||||
TaskID: task.ID,
|
||||
GitHubURL: "https://github.com/acme/widgets",
|
||||
CheckoutBranch: "feature/x",
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("AddBranchToTask with github_url: %v", err)
|
||||
}
|
||||
if added.RepositoryID != "repo-target" {
|
||||
t.Errorf("expected resolved repository_id=repo-target, got %q", added.RepositoryID)
|
||||
}
|
||||
if added.BaseBranch != "main" {
|
||||
t.Errorf("expected base_branch defaulted to repo default (main), got %q", added.BaseBranch)
|
||||
}
|
||||
}
|
||||
|
||||
// TestAddBranchToTask_ResolvesLocalPath exercises the local-worktree flow:
|
||||
// caller supplies the on-disk folder path, the service finds the matching
|
||||
// repository row in the task's workspace and attaches against it.
|
||||
func TestAddBranchToTask_ResolvesLocalPath(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-primary", WorkspaceID: "ws-1", Name: "primary", DefaultBranch: "main",
|
||||
})
|
||||
_ = repo.CreateRepository(ctx, &models.Repository{
|
||||
ID: "repo-by-path", WorkspaceID: "ws-1", Name: "sibling",
|
||||
LocalPath: "/tmp/sibling", DefaultBranch: "develop",
|
||||
})
|
||||
|
||||
task, err := svc.CreateTask(ctx, &CreateTaskRequest{
|
||||
WorkspaceID: "ws-1",
|
||||
WorkflowID: "wf-1",
|
||||
WorkflowStepID: "step-1",
|
||||
Title: "Multi-repo task",
|
||||
Repositories: []TaskRepositoryInput{
|
||||
{RepositoryID: "repo-primary", BaseBranch: "main"},
|
||||
},
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("CreateTask: %v", err)
|
||||
}
|
||||
|
||||
added, err := svc.AddBranchToTask(ctx, AddBranchToTaskRequest{
|
||||
TaskID: task.ID,
|
||||
LocalPath: "/tmp/sibling",
|
||||
CheckoutBranch: "feature/y",
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("AddBranchToTask with local_path: %v", err)
|
||||
}
|
||||
if added.RepositoryID != "repo-by-path" {
|
||||
t.Errorf("expected resolved repository_id=repo-by-path, got %q", added.RepositoryID)
|
||||
}
|
||||
if added.BaseBranch != "develop" {
|
||||
t.Errorf("expected base_branch defaulted to repo default (develop), got %q", added.BaseBranch)
|
||||
}
|
||||
}
|
||||
|
||||
// TestAddBranchToTask_RejectsNonWorktreeExecutor_NoOrphanRepo pins the
|
||||
// ordering guarantee documented on requireWorktreeExecutorForBranchAdd:
|
||||
// the executor gate must fire BEFORE ResolveRepositoryRef so a rejected
|
||||
// add_branch call on a non-worktree task never leaks a freshly-created
|
||||
// Repository row into the workspace via the github_url / local_path paths.
|
||||
//
|
||||
// Without this ordering, FindOrCreateRepository (github_url) and
|
||||
// CreateRepository (local_path) write before the executor check rejects,
|
||||
// leaving an orphan repository the user never asked for.
|
||||
func TestAddBranchToTask_RejectsNonWorktreeExecutor_NoOrphanRepo(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-primary", WorkspaceID: "ws-1", Name: "primary", DefaultBranch: "main"})
|
||||
|
||||
task, err := svc.CreateTask(ctx, &CreateTaskRequest{
|
||||
WorkspaceID: "ws-1",
|
||||
WorkflowID: "wf-1",
|
||||
WorkflowStepID: "step-1",
|
||||
Title: "Docker task",
|
||||
Repositories: []TaskRepositoryInput{
|
||||
{RepositoryID: "repo-primary", BaseBranch: "main"},
|
||||
},
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("CreateTask: %v", err)
|
||||
}
|
||||
|
||||
// Seed a docker executor — add_branch must reject.
|
||||
now := time.Now().UTC()
|
||||
if err := repo.CreateTaskEnvironment(ctx, &models.TaskEnvironment{
|
||||
ID: "env-1",
|
||||
TaskID: task.ID,
|
||||
ExecutorType: string(models.ExecutorTypeLocalDocker),
|
||||
Status: "ready",
|
||||
CreatedAt: now,
|
||||
UpdatedAt: now,
|
||||
}); err != nil {
|
||||
t.Fatalf("CreateTaskEnvironment: %v", err)
|
||||
}
|
||||
|
||||
before, err := repo.ListRepositories(ctx, "ws-1")
|
||||
if err != nil {
|
||||
t.Fatalf("ListRepositories before: %v", err)
|
||||
}
|
||||
beforeCount := len(before)
|
||||
|
||||
// github_url points at a repo NOT yet in the workspace — would trigger
|
||||
// FindOrCreateRepository if resolution ran. The executor check must
|
||||
// reject first.
|
||||
_, err = svc.AddBranchToTask(ctx, AddBranchToTaskRequest{
|
||||
TaskID: task.ID,
|
||||
GitHubURL: "https://github.com/acme/never-created",
|
||||
})
|
||||
if err == nil {
|
||||
t.Fatal("expected error rejecting non-worktree executor")
|
||||
}
|
||||
if !strings.Contains(err.Error(), "worktree executor") {
|
||||
t.Errorf("unexpected error: %v", err)
|
||||
}
|
||||
|
||||
after, err := repo.ListRepositories(ctx, "ws-1")
|
||||
if err != nil {
|
||||
t.Fatalf("ListRepositories after: %v", err)
|
||||
}
|
||||
if len(after) != beforeCount {
|
||||
t.Errorf("expected no new repository rows, got %d before vs %d after", beforeCount, len(after))
|
||||
}
|
||||
for _, r := range after {
|
||||
if r.ProviderOwner == "acme" && r.ProviderName == "never-created" {
|
||||
t.Errorf("orphan repository row leaked into workspace: %+v", r)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// TestCreateTask_RejectsSameRepoSameBranch verifies the dedup guard still
|
||||
// fires when the exact (repo, branch) pair is duplicated in the create
|
||||
// payload — the migration-layer constraint mirrors the in-memory guard.
|
||||
|
||||
@@ -373,6 +373,34 @@ func (s *Service) repoDisplayLabel(ctx context.Context, repoInput TaskRepository
|
||||
return repositoryID
|
||||
}
|
||||
|
||||
// ResolveRepositoryRef resolves a single TaskRepositoryInput to a
|
||||
// (repositoryID, baseBranch) pair within the given workspace, creating the
|
||||
// repository if necessary. Mirrors the resolution used during task creation
|
||||
// (`createTaskRepositories`), but builds the local-path lookup map on demand
|
||||
// so callers that only resolve one input (e.g. add_branch) don't need to
|
||||
// thread the map themselves.
|
||||
//
|
||||
// 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) {
|
||||
var repoByPath map[string]*models.Repository
|
||||
if repoInput.RepositoryID == "" && repoInput.LocalPath != "" {
|
||||
repos, listErr := s.repoEntities.ListRepositories(ctx, workspaceID)
|
||||
if listErr != nil {
|
||||
return "", "", listErr
|
||||
}
|
||||
repoByPath = make(map[string]*models.Repository, len(repos))
|
||||
for _, repo := range repos {
|
||||
if repo.LocalPath == "" {
|
||||
continue
|
||||
}
|
||||
repoByPath[repo.LocalPath] = repo
|
||||
}
|
||||
}
|
||||
return s.resolveRepoInput(ctx, workspaceID, repoInput, repoByPath)
|
||||
}
|
||||
|
||||
// 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) {
|
||||
|
||||
Reference in New Issue
Block a user