Skip to content

Commit 23dab31

Browse files
authored
fix(backend): honor base_branch for same-repo subtasks via MCP (kdlbs#1093)
* fix(backend): forward base_branch override for same-repo subtasks via MCP The create_task_kandev MCP handler dropped base_branch silently unless the caller also passed a repository identifier. Same-repo subtasks then inherited the parent's repos verbatim, so "branch off feature/X" calls landed on the parent's branch instead. Forward base_branch at the top level of the WS payload and apply it as an override on the resolved repo list when no per-repo entries were supplied; explicit per-repo BaseBranch wins as before. * docs(backend): correct BaseBranch field comment to match actual fallback semantics The struct doc on `req.BaseBranch` claimed it won over explicit per-repo entries, but the override only fires when `len(req.Repositories) == 0`. Reword as a fallback applied to inherited/resolved repos when no per-repo list is supplied; explicit per-repo BaseBranch stays authoritative when set. Flagged by CodeRabbit and Greptile. * refactor(backend): forward top-level base_branch only when no per-repo entries When the MCP caller supplies a repo identifier, the per-repo base_branch is already authoritative on the backend side, so emitting a top-level base_branch in the same payload was dead weight that the backend's override gate explicitly ignored. Move the top-level forward into the else of hasRepo so the two paths are mutually exclusive and the wire contract reads as "top-level base_branch means inherit-with-override". Flagged by the Claude review bot.
1 parent ec8039c commit 23dab31

4 files changed

Lines changed: 136 additions & 0 deletions

File tree

apps/backend/internal/mcp/handlers/handlers.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -366,6 +366,7 @@ func (h *Handlers) handleCreateTask(ctx context.Context, msg *ws.Message) (*ws.M
366366
ExecutorProfileID string `json:"executor_profile_id"`
367367
StartAgent *bool `json:"start_agent"` // nil means default to true for backward compatibility
368368
Repositories []mcpRepositoryInput `json:"repositories"` // explicit repositories for top-level tasks
369+
BaseBranch string `json:"base_branch"` // top-level fallback applied to every resolved repo only when no per-repo entries are supplied; explicit per-repo BaseBranch is authoritative when Repositories is set
369370
BlockedBy []string `json:"blocked_by"` // task IDs that must complete before this task
370371
AssigneeAgentProfileID string `json:"assignee_agent_profile_id"` // agent instance to assign the task to
371372
// Office task-handoffs phase 4 — workspace policy.
@@ -395,6 +396,17 @@ func (h *Handlers) handleCreateTask(ctx context.Context, msg *ws.Message) (*ws.M
395396
return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, err.Error(), nil)
396397
}
397398
repos := resolved.Repos
399+
// Top-level base_branch override: when the caller passes base_branch
400+
// without any per-repo entries, apply it to every repo in the resolved
401+
// list. This is the only path that lets a same-repo subtask override
402+
// the parent's inherited base_branch without also restating the
403+
// repository identifier. When the caller provided explicit per-repo
404+
// entries we leave their BaseBranch alone — those are authoritative.
405+
if req.BaseBranch != "" && len(req.Repositories) == 0 {
406+
for i := range repos {
407+
repos[i].BaseBranch = req.BaseBranch
408+
}
409+
}
398410
if req.WorkspaceID == "" {
399411
req.WorkspaceID = resolved.WorkspaceID
400412
}

apps/backend/internal/mcp/handlers/handlers_test.go

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -380,6 +380,94 @@ func TestCreateSubtaskFromParent_DifferentRepoUsesNewRepoDefault(t *testing.T) {
380380
assert.Equal(t, "trunk", subtask.Repositories[0].BaseBranch, "cross-repo subtask should anchor to the new repo's default_branch, not parent's pr-metrics")
381381
}
382382

383+
// TestHandleCreateTask_SubtaskBaseBranchOverride pins the bug-fix path:
384+
// when an MCP caller passes base_branch at the top level (no per-repo
385+
// entries) for a same-repo subtask, the override beats the parent's
386+
// inherited base_branch. Previously the top-level base_branch was
387+
// silently dropped unless the caller also restated repository_id, so a
388+
// "give me a child task that branches off feature/X" call quietly
389+
// landed on the parent's branch instead.
390+
func TestHandleCreateTask_SubtaskBaseBranchOverride(t *testing.T) {
391+
svc, repo := newTestTaskService(t)
392+
ctx := context.Background()
393+
parentID := seedParentWithRepo(t, svc, repo)
394+
395+
log := testLogger(t)
396+
h := &Handlers{taskSvc: svc, logger: log.WithFields()}
397+
398+
msg := makeWSMessage(t, ws.ActionMCPCreateTask, map[string]interface{}{
399+
"title": "Child",
400+
"description": "do the thing",
401+
"parent_id": parentID,
402+
"base_branch": "feature/create-new-page-endp-05z",
403+
"start_agent": false,
404+
})
405+
406+
resp, err := h.handleCreateTask(ctx, msg)
407+
require.NoError(t, err)
408+
require.Equalf(t, ws.MessageTypeResponse, resp.Type, "create_task should succeed; payload: %s", string(resp.Payload))
409+
410+
var created struct {
411+
ID string `json:"id"`
412+
}
413+
require.NoError(t, json.Unmarshal(resp.Payload, &created))
414+
require.NotEmpty(t, created.ID)
415+
416+
subtask, err := svc.GetTask(ctx, created.ID)
417+
require.NoError(t, err)
418+
require.Len(t, subtask.Repositories, 1, "subtask should inherit parent's repository")
419+
assert.Equal(t, "repo-parent", subtask.Repositories[0].RepositoryID, "subtask should still bind to parent's repo")
420+
assert.Equal(t, "feature/create-new-page-endp-05z", subtask.Repositories[0].BaseBranch,
421+
"top-level base_branch must override parent's inherited base_branch when no explicit repos are passed")
422+
}
423+
424+
// TestHandleCreateTask_SubtaskBaseBranchOverride_ExplicitReposWin asserts
425+
// the inverse: when the caller provides per-repo entries, those are
426+
// authoritative — the top-level base_branch must NOT clobber an explicit
427+
// per-repo BaseBranch. This preserves cross-repo and multi-repo callers
428+
// that already control branch selection per entry.
429+
func TestHandleCreateTask_SubtaskBaseBranchOverride_ExplicitReposWin(t *testing.T) {
430+
svc, repo := newTestTaskService(t)
431+
ctx := context.Background()
432+
parentID := seedParentWithRepo(t, svc, repo)
433+
434+
require.NoError(t, repo.CreateRepository(ctx, &models.Repository{
435+
ID: "repo-sibling", WorkspaceID: "ws-1", Name: "Sibling", DefaultBranch: "trunk",
436+
}))
437+
438+
log := testLogger(t)
439+
h := &Handlers{taskSvc: svc, logger: log.WithFields()}
440+
441+
msg := makeWSMessage(t, ws.ActionMCPCreateTask, map[string]interface{}{
442+
"title": "Cross-repo child",
443+
"description": "do the thing",
444+
"parent_id": parentID,
445+
// Explicit per-repo entry with its own base_branch.
446+
"repositories": []map[string]interface{}{
447+
{"repository_id": "repo-sibling", "base_branch": "develop"},
448+
},
449+
// Top-level base_branch should be ignored because explicit repos win.
450+
"base_branch": "should-not-be-applied",
451+
"start_agent": false,
452+
})
453+
454+
resp, err := h.handleCreateTask(ctx, msg)
455+
require.NoError(t, err)
456+
require.Equalf(t, ws.MessageTypeResponse, resp.Type, "create_task should succeed; payload: %s", string(resp.Payload))
457+
458+
var created struct {
459+
ID string `json:"id"`
460+
}
461+
require.NoError(t, json.Unmarshal(resp.Payload, &created))
462+
463+
subtask, err := svc.GetTask(ctx, created.ID)
464+
require.NoError(t, err)
465+
require.Len(t, subtask.Repositories, 1)
466+
assert.Equal(t, "repo-sibling", subtask.Repositories[0].RepositoryID)
467+
assert.Equal(t, "develop", subtask.Repositories[0].BaseBranch,
468+
"explicit per-repo base_branch must win over the top-level override")
469+
}
470+
383471
func TestResolveTaskRepositories_ParentWithExplicitRepos_OverridesRepoButInheritsWorkspace(t *testing.T) {
384472
svc, repo := newTestTaskService(t)
385473
parentID := seedParentWithRepo(t, svc, repo)

apps/backend/internal/mcp/server/handlers.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,13 @@ func (s *Server) createTaskHandler() server.ToolHandlerFunc {
184184
repo["base_branch"] = baseBranch
185185
}
186186
payload["repositories"] = []map[string]string{repo}
187+
} else if baseBranch != "" {
188+
// Forward base_branch at the top level only when the caller
189+
// supplied no repo identifier — the backend uses it as a fallback
190+
// applied to inherited subtask repos. When explicit repo entries
191+
// are present, the per-repo base_branch above is authoritative
192+
// and a top-level value here would be ignored.
193+
payload["base_branch"] = baseBranch
187194
}
188195

189196
var result map[string]interface{}

apps/backend/internal/mcp/server/handlers_test.go

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -252,6 +252,35 @@ func TestCreateTask_WithRepositoryURL(t *testing.T) {
252252
assert.Equal(t, "main", repos[0]["base_branch"])
253253
}
254254

255+
// TestCreateTask_BaseBranchOnly_ForwardsTopLevel pins the bug-fix wiring:
256+
// when the caller passes only base_branch (no repository_id / local_path /
257+
// repository_url), the MCP server forwards it at the top level of the WS
258+
// payload so the backend can apply it as an override on inherited
259+
// subtask repos. Previously base_branch was silently dropped when no
260+
// repo identifier was passed.
261+
func TestCreateTask_BaseBranchOnly_ForwardsTopLevel(t *testing.T) {
262+
backend := &testBackend{
263+
response: map[string]interface{}{"id": "subtask-1", "parent_id": "task-current"},
264+
}
265+
s := newTaskModeServer(t, backend, "task-current")
266+
267+
result := callTool(t, s, "create_task_kandev", map[string]interface{}{
268+
"title": "Stacked PR child",
269+
"parent_id": "self",
270+
"description": "branch off the parent's PR branch",
271+
"base_branch": "feature/create-new-page-endp-05z",
272+
})
273+
274+
assert.False(t, result.IsError)
275+
276+
payload, ok := backend.lastPayload.(map[string]interface{})
277+
require.True(t, ok)
278+
assert.Equal(t, "feature/create-new-page-endp-05z", payload["base_branch"],
279+
"base_branch should be forwarded at the top level even when no repo identifier is supplied")
280+
_, hasRepos := payload["repositories"]
281+
assert.False(t, hasRepos, "no repositories slice should be produced when only base_branch is supplied")
282+
}
283+
255284
func TestCreateTask_RepositoryURL_AllowedForSubtasks(t *testing.T) {
256285
backend := &testBackend{
257286
response: map[string]interface{}{"id": "task-new", "title": "Subtask with URL"},

0 commit comments

Comments
 (0)