From 9b7fa0ec9c086707ef2ef458da1b475f9f6cf7bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Salavisa?= Date: Sat, 18 Jul 2026 13:22:32 +0100 Subject: [PATCH] fix(mcp): provide bounded access to CREATED sibling task descriptions (#1773) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(mcp): provide bounded access to CREATED sibling task descriptions Include the task description in the list_related_tasks_kandev projection so a coordinating agent can read dependency metadata (e.g. a "Depends on:" line) from a related task — including a CREATED sibling that has no session yet — without falling back to an unbounded list_tasks_kandev call. The relation graph is the bound: only parent/children/siblings/blockers are ever projected, so descriptions never leak from unrelated tasks or across workspaces. ListSiblings already returns CREATED (unstarted) tasks and loads their descriptions from storage; only the MCP projection was dropping the field. Closes #1772 Co-Authored-By: Claude Fable 5 * fix(mcp): gate related-task descriptions by caller access Address PR review: list_related_tasks_kandev ignored caller_task_id and projected any named task's description, so a caller that learned an unrelated or cross-workspace task ID could read it. Add ListRelatedForCaller, which enforces the same relation/workspace read guard the document tools use (self/ancestor/descendant/sibling/blocker, same workspace) for non-self targets, and wire the MCP handler through it (access-denied maps to forbidden). The un-gated ListRelated stays for trusted internal callers (GetTaskContext). Add tests for the blocker/blocked-by description projection and the caller access guard (unrelated + cross-workspace denied, relations allowed). Co-Authored-By: Claude Fable 5 * refactor(mcp): align related-tasks error mapping and empty-caller guard Address PR review nits: route handleListRelatedTasks errors through mapHandoffError (logging only non-access-denied errors) for parity with the document handlers, and make ListRelatedForCaller deny an empty caller against a non-self target instead of silently delegating to the ungated ListRelated. Co-Authored-By: Claude Fable 5 --------- Co-authored-by: Kandev Agent Co-authored-by: Claude Fable 5 --- .../internal/mcp/handlers/handoff_handlers.go | 15 +- .../internal/mcp/server/handoff_handlers.go | 8 +- .../task/service/handoff_related_test.go | 196 ++++++++++++++++++ .../internal/task/service/handoff_service.go | 42 +++- 4 files changed, 254 insertions(+), 7 deletions(-) create mode 100644 apps/backend/internal/task/service/handoff_related_test.go diff --git a/apps/backend/internal/mcp/handlers/handoff_handlers.go b/apps/backend/internal/mcp/handlers/handoff_handlers.go index 8c11d42c08..c569145963 100644 --- a/apps/backend/internal/mcp/handlers/handoff_handlers.go +++ b/apps/backend/internal/mcp/handlers/handoff_handlers.go @@ -35,10 +35,19 @@ func (h *Handlers) handleListRelatedTasks(ctx context.Context, msg *ws.Message) if req.TaskID == "" { return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, "task_id is required", nil) } - related, err := svc.ListRelated(ctx, req.TaskID) + caller := req.CallerTaskID + if caller == "" { + caller = req.TaskID + } + related, err := svc.ListRelatedForCaller(ctx, caller, req.TaskID) if err != nil { - h.logger.Error("list related tasks", zap.Error(err)) - return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, err.Error(), nil) + // Access denied is an expected 403; log only genuine + // infrastructure errors, then route everything through + // mapHandoffError for the same code mapping the document handlers use. + if !errors.Is(err, service.ErrAccessDenied) { + h.logger.Error("list related tasks", zap.Error(err)) + } + return mapHandoffError(msg, err) } h.enrichRelatedTasksWithPRs(ctx, related) return ws.NewResponse(msg.ID, msg.Action, related) diff --git a/apps/backend/internal/mcp/server/handoff_handlers.go b/apps/backend/internal/mcp/server/handoff_handlers.go index 233252fee1..22cd90d4ac 100644 --- a/apps/backend/internal/mcp/server/handoff_handlers.go +++ b/apps/backend/internal/mcp/server/handoff_handlers.go @@ -19,9 +19,11 @@ func (s *Server) registerRelatedTasksTool() { mcp.WithDescription( `List parent, children, siblings, blockers, and blocked tasks for the current task. Use this to discover task IDs you can reach via message_task_kandev. Each related task includes its -associated GitHub pull requests (number, url, title, state) under the "prs" field when any exist. In -office mode, document keys are also included so you can fetch documents with get_task_document_kandev. -Pass task_id to inspect a different task in the same workspace.`, +title and description, so you can read dependency metadata (e.g. a "Depends on:" line) from a sibling +that has not started yet — no need to fetch its conversation or list every task in the workflow. Each +related task also includes its associated GitHub pull requests (number, url, title, state) under the +"prs" field when any exist. In office mode, document keys are also included so you can fetch documents +with get_task_document_kandev. Pass task_id to inspect a different task in the same workspace.`, ), mcp.WithString("task_id", mcp.Description("Defaults to the current task.")), ), diff --git a/apps/backend/internal/task/service/handoff_related_test.go b/apps/backend/internal/task/service/handoff_related_test.go new file mode 100644 index 0000000000..0399fe8aa3 --- /dev/null +++ b/apps/backend/internal/task/service/handoff_related_test.go @@ -0,0 +1,196 @@ +package service + +import ( + "context" + "errors" + "testing" + + orchmodels "github.com/kandev/kandev/internal/office/models" + v1 "github.com/kandev/kandev/pkg/api/v1" +) + +// setDescription and setState mutate a task the fake repo already holds so a +// test can model a CREATED sibling (no session) that carries dependency +// metadata in its description. They reach into the fake directly because +// addTask only seeds id/parent/workspace. +func (f *fakeTaskRepo) setDescription(id, desc string) { + f.mu.Lock() + defer f.mu.Unlock() + if t := f.tasks[id]; t != nil { + t.Description = desc + } +} + +func (f *fakeTaskRepo) setState(id string, state v1.TaskState) { + f.mu.Lock() + defer f.mu.Unlock() + if t := f.tasks[id]; t != nil { + t.State = state + } +} + +// TestListRelated_CreatedSiblingDescription is the core regression for +// issue #1772: a running subtask must be able to read the description of an +// authorized CREATED sibling that has no session yet, through the bounded +// list_related_tasks projection. +func TestListRelated_CreatedSiblingDescription(t *testing.T) { + tasks := newFakeTaskRepo() + tasks.addTask("parent", "", "ws-1") + tasks.addTask("runner", "parent", "ws-1") // the running caller + tasks.addTask("dep", "parent", "ws-1") // CREATED sibling, never started + tasks.setState("runner", v1.TaskStateInProgress) + tasks.setState("dep", v1.TaskStateCreated) + tasks.setDescription("dep", "Depends on: 01 [foundation], 03 [canvas]") + svc := newCascadeService(t, tasks, newCascadeWSGroupRepo()) + + out, err := svc.ListRelated(context.Background(), "runner") + if err != nil { + t.Fatalf("list related: %v", err) + } + if len(out.Siblings) != 1 || out.Siblings[0].ID != "dep" { + t.Fatalf("siblings = %+v, want single sibling dep", out.Siblings) + } + sib := out.Siblings[0] + if sib.State != string(v1.TaskStateCreated) { + t.Errorf("sibling state = %q, want CREATED", sib.State) + } + if sib.Description != "Depends on: 01 [foundation], 03 [canvas]" { + t.Errorf("sibling description = %q, want the Depends on line", sib.Description) + } +} + +// TestListRelated_ProjectsDescriptionAcrossRelations verifies the +// description flows onto self, parent, and children too, not just siblings. +func TestListRelated_ProjectsDescriptionAcrossRelations(t *testing.T) { + tasks := newFakeTaskRepo() + tasks.addTask("parent", "", "ws-1") + tasks.addTask("self", "parent", "ws-1") + tasks.addTask("child", "self", "ws-1") + tasks.setDescription("parent", "parent desc") + tasks.setDescription("self", "self desc") + tasks.setDescription("child", "child desc") + svc := newCascadeService(t, tasks, newCascadeWSGroupRepo()) + + out, err := svc.ListRelated(context.Background(), "self") + if err != nil { + t.Fatalf("list related: %v", err) + } + if out.Task.Description != "self desc" { + t.Errorf("self description = %q", out.Task.Description) + } + if out.Parent == nil || out.Parent.Description != "parent desc" { + t.Errorf("parent description = %+v", out.Parent) + } + if len(out.Children) != 1 || out.Children[0].Description != "child desc" { + t.Errorf("children = %+v", out.Children) + } +} + +// TestListRelated_ProjectsBlockerDescriptions exercises the toRelatedByIDs +// path (Blockers / BlockedBy) which only runs when the blockers repo is +// non-nil. It guards against a silent regression in the blocker projection. +func TestListRelated_ProjectsBlockerDescriptions(t *testing.T) { + tasks := newFakeTaskRepo() + tasks.addTask("parent", "", "ws-1") + tasks.addTask("self", "parent", "ws-1") + tasks.addTask("blocker", "parent", "ws-1") + tasks.addTask("downstream", "parent", "ws-1") + tasks.setDescription("blocker", "Depends on: nothing; produces foundation") + tasks.setDescription("downstream", "Depends on: 02 [self]") + blockers := &fakeBlockerRepo{} + // self is blocked by "blocker"; "downstream" is blocked by self. + _ = blockers.CreateTaskBlocker(context.Background(), &orchmodels.TaskBlocker{TaskID: "self", BlockerTaskID: "blocker"}) + _ = blockers.CreateTaskBlocker(context.Background(), &orchmodels.TaskBlocker{TaskID: "downstream", BlockerTaskID: "self"}) + svc := newPhase4Service(t, tasks, blockers, newCascadeWSGroupRepo()) + + out, err := svc.ListRelated(context.Background(), "self") + if err != nil { + t.Fatalf("list related: %v", err) + } + if len(out.Blockers) != 1 || out.Blockers[0].Description != "Depends on: nothing; produces foundation" { + t.Errorf("blockers = %+v", out.Blockers) + } + if len(out.BlockedBy) != 1 || out.BlockedBy[0].Description != "Depends on: 02 [self]" { + t.Errorf("blocked_by = %+v", out.BlockedBy) + } +} + +// TestListRelatedForCaller_GatesUnrelatedAndCrossWorkspace covers the access +// guard: a caller may inspect itself and its relations, but not an unrelated +// task or a task in another workspace, so descriptions never leak past the +// caller's relation/workspace scope (issue #1772 acceptance criterion). +func TestListRelatedForCaller_GatesUnrelatedAndCrossWorkspace(t *testing.T) { + tasks := newFakeTaskRepo() + tasks.addTask("parent", "", "ws-1") + tasks.addTask("caller", "parent", "ws-1") + tasks.addTask("sibling", "parent", "ws-1") + tasks.addTask("unrelated-parent", "", "ws-1") + tasks.addTask("unrelated", "unrelated-parent", "ws-1") // same workspace, no relation + tasks.addTask("other-ws", "", "ws-2") // different workspace + tasks.setDescription("sibling", "Depends on: 01 [foundation]") + tasks.setDescription("unrelated", "secret plan") + svc := newCascadeService(t, tasks, newCascadeWSGroupRepo()) + + // Self is always allowed. + if _, err := svc.ListRelatedForCaller(context.Background(), "caller", "caller"); err != nil { + t.Fatalf("self should be allowed: %v", err) + } + // A sibling / parent (relation in same workspace) is allowed. + if _, err := svc.ListRelatedForCaller(context.Background(), "caller", "sibling"); err != nil { + t.Fatalf("sibling should be allowed: %v", err) + } + if _, err := svc.ListRelatedForCaller(context.Background(), "caller", "parent"); err != nil { + t.Fatalf("parent should be allowed: %v", err) + } + // An unrelated task in the same workspace is denied. + if _, err := svc.ListRelatedForCaller(context.Background(), "caller", "unrelated"); !errors.Is(err, ErrAccessDenied) { + t.Errorf("unrelated should be denied, got %v", err) + } + // A task in another workspace is denied. + if _, err := svc.ListRelatedForCaller(context.Background(), "caller", "other-ws"); !errors.Is(err, ErrAccessDenied) { + t.Errorf("cross-workspace should be denied, got %v", err) + } + // An empty caller has no identity to authorize a non-self target. + if _, err := svc.ListRelatedForCaller(context.Background(), "", "sibling"); !errors.Is(err, ErrAccessDenied) { + t.Errorf("empty caller should be denied for a non-self target, got %v", err) + } +} + +// TestListRelatedForCaller_ReturnsSiblingDescription confirms the gated entry +// point still surfaces an authorized CREATED sibling's description end-to-end. +func TestListRelatedForCaller_ReturnsSiblingDescription(t *testing.T) { + tasks := newFakeTaskRepo() + tasks.addTask("parent", "", "ws-1") + tasks.addTask("runner", "parent", "ws-1") + tasks.addTask("dep", "parent", "ws-1") + tasks.setState("dep", v1.TaskStateCreated) + tasks.setDescription("dep", "Depends on: 01 [foundation]") + svc := newCascadeService(t, tasks, newCascadeWSGroupRepo()) + + out, err := svc.ListRelatedForCaller(context.Background(), "runner", "runner") + if err != nil { + t.Fatalf("list related for caller: %v", err) + } + if len(out.Siblings) != 1 || out.Siblings[0].Description != "Depends on: 01 [foundation]" { + t.Errorf("siblings = %+v", out.Siblings) + } +} + +// TestListRelated_OmitsEmptyDescription confirms the projection stays lean +// when a related task has no description (omitempty keeps the MCP output +// usable on workflows with many historical tasks). +func TestListRelated_OmitsEmptyDescription(t *testing.T) { + tasks := newFakeTaskRepo() + tasks.addTask("parent", "", "ws-1") + tasks.addTask("self", "parent", "ws-1") + tasks.addTask("sib", "parent", "ws-1") + svc := newCascadeService(t, tasks, newCascadeWSGroupRepo()) + + out, err := svc.ListRelated(context.Background(), "self") + if err != nil { + t.Fatalf("list related: %v", err) + } + if len(out.Siblings) != 1 || out.Siblings[0].Description != "" { + t.Errorf("sibling description should be empty, got %+v", out.Siblings) + } +} diff --git a/apps/backend/internal/task/service/handoff_service.go b/apps/backend/internal/task/service/handoff_service.go index fa929f455f..4b5bf269e6 100644 --- a/apps/backend/internal/task/service/handoff_service.go +++ b/apps/backend/internal/task/service/handoff_service.go @@ -137,11 +137,17 @@ func (p WorkspacePolicy) NeedsAttachment() bool { // RelatedTask is the lightweight projection of a task surfaced through the // list_related_tasks_kandev MCP tool. Document keys are precomputed so an -// agent can decide what to fetch without a follow-up list call. +// agent can decide what to fetch without a follow-up list call. Description +// is included so a coordinating agent can read dependency metadata (e.g. a +// "Depends on:" line) from a related task — including a CREATED sibling that +// has no session yet — without an unbounded list_tasks call. The relation +// graph itself is the bound: only parent/children/siblings/blockers are ever +// projected, so descriptions never leak from unrelated tasks. type RelatedTask struct { ID string `json:"id"` Identifier string `json:"identifier,omitempty"` Title string `json:"title"` + Description string `json:"description,omitempty"` State string `json:"state"` WorkspaceID string `json:"workspace_id"` ParentID string `json:"parent_id,omitempty"` @@ -414,6 +420,39 @@ func (s *HandoffService) attachSequentialBlocker(ctx context.Context, taskID, pa }) } +// ListRelatedForCaller is the access-checked entry point behind the +// list_related_tasks_kandev MCP tool. When the caller inspects a task other +// than itself, the target must be readable under the same relation/workspace +// guard the document tools use (self / ancestor / descendant / sibling / +// blocker, same workspace). Without this gate a caller that learns an +// unrelated or cross-workspace task ID could read that task's description and +// its relatives' descriptions. Returns ErrAccessDenied for inaccessible +// targets. +// +// The un-gated ListRelated remains for trusted internal callers (e.g. +// GetTaskContext renders the context panel for a task the user already owns). +func (s *HandoffService) ListRelatedForCaller(ctx context.Context, callerTaskID, targetTaskID string) (*RelatedTasks, error) { + if targetTaskID == "" { + return nil, ErrDocumentTaskRequired + } + // Any target other than the caller itself must pass the read guard. An + // empty caller has no identity to authorize against, so canReadDocuments + // denies it (rather than silently delegating to the ungated ListRelated). + if callerTaskID != targetTaskID { + ok, err := canReadDocuments(ctx, + repoTaskLookupAdapter{r: s.tasks}, + blockerLookupAdapter{repo: s.blockers}, + callerTaskID, targetTaskID) + if err != nil { + return nil, err + } + if !ok { + return nil, ErrAccessDenied + } + } + return s.ListRelated(ctx, targetTaskID) +} + // ListRelated returns the parent, children, siblings, blockers, and // blocked-by tasks for taskID. Document keys are populated for the task // itself plus every related task so an agent can shop for documents in @@ -556,6 +595,7 @@ func (s *HandoffService) toRelated(ctx context.Context, t *models.Task) RelatedT ID: t.ID, Identifier: t.Identifier, Title: t.Title, + Description: t.Description, State: string(t.State), WorkspaceID: t.WorkspaceID, ParentID: t.ParentID,