fix(mcp): provide bounded access to CREATED sibling task descriptions (#1773)
* 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: Kandev Agent <agent@kandev.dev> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
João Salavisa
@@ -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)
|
||||
|
||||
@@ -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.")),
|
||||
),
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user