diff --git a/internal/agent/delegated_lead_team.go b/internal/agent/delegated_lead_team.go new file mode 100644 index 00000000..50fc3927 --- /dev/null +++ b/internal/agent/delegated_lead_team.go @@ -0,0 +1,86 @@ +package agent + +import ( + "context" + "log/slog" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/store" + "github.com/nextlevelbuilder/goclaw/internal/tools" +) + +// delegatedLeadTeamRead is the read-only view of its own team granted to a team +// lead reached through delegate: workspace is the directory that team's tasks +// resolve to, root is the team directory above it — the one that makes files +// written by members under a different chat scope reachable. +// +// Both are allowances only. Neither may become the active workspace: in a +// delegation artifact run that stays the exchange outputs directory, and +// displacing it breaks the exchange. +type delegatedLeadTeamRead struct { + workspace string + root string +} + +func (r delegatedLeadTeamRead) ok() bool { return r.workspace != "" && r.root != "" } + +// resolveDelegatedLeadTeamRead returns the team paths a delegated lead may read, +// or the zero value when no unambiguous team answers. +// +// A team member run carries its team workspace in dispatch metadata and a lead +// addressed directly resolves one below; a delegated lead had neither, so it +// could not read its own team's files nor stage them into the delegation outputs +// directory — the team's deliverables were unreachable to the only agent able to +// hand them back (#1535). +// +// The paths mirror what injectContext resolves for a directly addressed lead and +// what team_tasks_create stores on every task the lead creates; those must agree +// or the lead is handed a sibling directory. l.dataDir is already tenant-scoped +// (resolver.go: config.TenantDataDir), so TenantLayer must NOT be reapplied here. +// +// Ambiguity is deliberately not resolved: an agent leading several active teams +// gets nothing rather than a guess, since the delegation does not say which team +// it concerns. store.GetTeamForAgent would answer in one call, but it orders by +// lead-ness and takes LIMIT 1 — it would silently pick one team, which is exactly +// the choice this must not make on its own. +func (l *Loop) resolveDelegatedLeadTeamRead(ctx context.Context, req *RunRequest) delegatedLeadTeamRead { + if l.teamStore == nil || l.agentUUID == uuid.Nil || l.dataDir == "" { + return delegatedLeadTeamRead{} + } + teams, err := l.teamStore.ListTeams(ctx) + if err != nil { + slog.Warn("delegate: cannot resolve lead team read access", + "agent_id", l.agentUUID, "error", err) + return delegatedLeadTeamRead{} + } + var lead *store.TeamData + for i := range teams { + if teams[i].LeadAgentID != l.agentUUID || teams[i].Status != store.TeamStatusActive { + continue + } + if lead != nil { + slog.Debug("delegate: agent leads several active teams, team read access not granted", + "agent_id", l.agentUUID) + return delegatedLeadTeamRead{} + } + lead = &teams[i] + } + if lead == nil { + return delegatedLeadTeamRead{} + } + + // Origin chat, not the delivery channel: team_tasks_create derives the same + // path from OriginChatIDFromCtx, which prefers the workspace chat ID. + chatID := req.WorkspaceChatID + if chatID == "" { + chatID = req.ChatID + } + return delegatedLeadTeamRead{ + workspace: tools.ResolveWorkspace(l.dataDir, + tools.TeamLayer(lead.ID), + tools.UserChatLayer(chatID, tools.IsSharedWorkspace(lead.Settings)), + ), + root: tools.ResolveWorkspace(l.dataDir, tools.TeamLayer(lead.ID)), + } +} diff --git a/internal/agent/delegated_lead_team_test.go b/internal/agent/delegated_lead_team_test.go new file mode 100644 index 00000000..ce395813 --- /dev/null +++ b/internal/agent/delegated_lead_team_test.go @@ -0,0 +1,238 @@ +package agent + +import ( + "context" + "encoding/json" + "errors" + "path/filepath" + "strings" + "testing" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/store" + "github.com/nextlevelbuilder/goclaw/internal/tools" +) + +// mockTeamStoreLead serves ListTeams and nothing else; the embedded interface is +// nil on purpose so an unexpected store call fails loudly instead of silently. +type mockTeamStoreLead struct { + store.TeamStore + teams []store.TeamData + err error +} + +func (m *mockTeamStoreLead) ListTeams(context.Context) ([]store.TeamData, error) { + return m.teams, m.err +} + +func leadTeam(id, lead uuid.UUID, status string, settings string) store.TeamData { + td := store.TeamData{ + BaseModel: store.BaseModel{ID: id}, + LeadAgentID: lead, + Status: status, + } + if settings != "" { + td.Settings = json.RawMessage(settings) + } + return td +} + +// newLeadTestLoop mirrors newArtifactTestLoop but wires a team store and pins the +// agent identity, so the resolver has something to resolve against. +func newLeadTestLoop(root string, agentID uuid.UUID, teams store.TeamStore) *Loop { + return NewLoop(LoopConfig{ + ID: "brain", + AgentUUID: agentID, + TenantID: store.MasterTenantID, + Workspace: filepath.Join(root, "agents", "brain"), + DataDir: root, + Sessions: &nopSessionStore{}, + TeamStore: teams, + }) +} + +func TestResolveDelegatedLeadTeamRead(t *testing.T) { + root := t.TempDir() + lead := uuid.New() + other := uuid.New() + teamA := uuid.New() + teamB := uuid.New() + req := &RunRequest{ChatID: "chat-1", WorkspaceChatID: "chat-1"} + + t.Run("SingleLedTeamResolves", func(t *testing.T) { + s := &mockTeamStoreLead{teams: []store.TeamData{ + leadTeam(teamA, lead, store.TeamStatusActive, ""), + leadTeam(teamB, other, store.TeamStatusActive, ""), + }} + got := newLeadTestLoop(root, lead, s).resolveDelegatedLeadTeamRead(context.Background(), req) + if !got.ok() { + t.Fatalf("nothing resolved for an agent leading exactly one team: %#v", got) + } + if !strings.Contains(got.workspace, teamA.String()) { + t.Errorf("workspace %q does not belong to the led team %s", got.workspace, teamA) + } + if strings.Contains(got.workspace, teamB.String()) || strings.Contains(got.root, teamB.String()) { + t.Errorf("resolved paths leak into an unrelated team: %#v", got) + } + // Isolated is the default: the workspace is a chat-scoped leaf below the + // root, and the root is what makes a peer's chat scope reachable at all. + if got.workspace != filepath.Join(got.root, "chat-1") { + t.Errorf("workspace %q is not the chat leaf of root %q", got.workspace, got.root) + } + }) + + // l.dataDir arrives already tenant-scoped from resolver.go + // (config.TenantDataDir), which is why the resolver must not apply + // TenantLayer itself. Nothing pinned that: reapplying it would double-join + // the tenant segment and hand the lead a directory its own tasks never use, + // and the resulting path is plausible enough to survive review. The sibling + // branch in injectContext carries the same warning in a comment. + t.Run("TenantSegmentIsNotDoubled", func(t *testing.T) { + tenantScoped := filepath.Join(root, "tenants", "acme") + s := &mockTeamStoreLead{teams: []store.TeamData{ + leadTeam(teamA, lead, store.TeamStatusActive, ""), + }} + // A tenant in context is what a reapplied TenantLayer would act on. + ctx := store.WithTenantSlug(store.WithTenantID(context.Background(), uuid.New()), "acme") + got := newLeadTestLoop(tenantScoped, lead, s).resolveDelegatedLeadTeamRead(ctx, req) + if !got.ok() { + t.Fatal("nothing resolved from a tenant-scoped data dir") + } + want := filepath.Join(tenantScoped, "teams", teamA.String(), "chat-1") + if got.workspace != want { + t.Errorf("workspace = %q, want %q", got.workspace, want) + } + if n := strings.Count(got.workspace, filepath.Join("tenants", "acme")); n != 1 { + t.Errorf("tenant segment appears %d times in %q, want exactly once — "+ + "l.dataDir is already tenant-scoped, so TenantLayer must not be reapplied", + n, got.workspace) + } + }) + + t.Run("SharedTeamCollapsesToRoot", func(t *testing.T) { + s := &mockTeamStoreLead{teams: []store.TeamData{ + leadTeam(teamA, lead, store.TeamStatusActive, `{"workspace_scope":"shared"}`), + }} + got := newLeadTestLoop(root, lead, s).resolveDelegatedLeadTeamRead(context.Background(), req) + if !got.ok() || got.workspace != got.root { + t.Errorf("a shared team must resolve workspace == root, got %#v", got) + } + }) + + t.Run("MultipleLedTeamsResolveNothing", func(t *testing.T) { + s := &mockTeamStoreLead{teams: []store.TeamData{ + leadTeam(teamA, lead, store.TeamStatusActive, ""), + leadTeam(teamB, lead, store.TeamStatusActive, ""), + }} + got := newLeadTestLoop(root, lead, s).resolveDelegatedLeadTeamRead(context.Background(), req) + if got.ok() { + t.Errorf("resolved %#v for an agent leading two teams; the delegation does not say "+ + "which team it is about, so guessing would hand out the wrong workspace", got) + } + }) + + t.Run("NonLeadResolvesNothing", func(t *testing.T) { + s := &mockTeamStoreLead{teams: []store.TeamData{ + leadTeam(teamA, other, store.TeamStatusActive, ""), + }} + if got := newLeadTestLoop(root, lead, s).resolveDelegatedLeadTeamRead(context.Background(), req); got.ok() { + t.Errorf("resolved %#v for an agent that leads no team", got) + } + }) + + t.Run("InactiveTeamIgnored", func(t *testing.T) { + s := &mockTeamStoreLead{teams: []store.TeamData{leadTeam(teamA, lead, "archived", "")}} + if got := newLeadTestLoop(root, lead, s).resolveDelegatedLeadTeamRead(context.Background(), req); got.ok() { + t.Errorf("resolved %#v from an inactive team", got) + } + }) + + t.Run("StoreFailureResolvesNothing", func(t *testing.T) { + s := &mockTeamStoreLead{err: errors.New("db down")} + if got := newLeadTestLoop(root, lead, s).resolveDelegatedLeadTeamRead(context.Background(), req); got.ok() { + t.Errorf("resolved %#v despite a store failure", got) + } + }) + + t.Run("NoTeamStoreResolvesNothing", func(t *testing.T) { + if got := newLeadTestLoop(root, lead, nil).resolveDelegatedLeadTeamRead(context.Background(), req); got.ok() { + t.Errorf("resolved %#v without a team store", got) + } + }) + + t.Run("SeparateLeadsGetSeparateWorkspaces", func(t *testing.T) { + s := &mockTeamStoreLead{teams: []store.TeamData{ + leadTeam(teamA, lead, store.TeamStatusActive, ""), + leadTeam(teamB, other, store.TeamStatusActive, ""), + }} + a := newLeadTestLoop(root, lead, s).resolveDelegatedLeadTeamRead(context.Background(), req) + b := newLeadTestLoop(root, other, s).resolveDelegatedLeadTeamRead(context.Background(), req) + if !a.ok() || !b.ok() || a.workspace == b.workspace || a.root == b.root { + t.Errorf("leads of different teams must resolve different paths, got %#v and %#v", a, b) + } + }) +} + +// The guard at the top of the artifact branch rejects req.TeamWorkspace outright, +// and the team-workspace application below it is gated on !isArtifactDelegation. +// Granting the lead read access must therefore happen inside the artifact branch +// and must not touch the active workspace: this test fails both if the run is +// refused and if the exchange outputs directory is displaced. +func TestInjectContext_DelegatedLeadReadsTeamWithoutDisplacingOutputs(t *testing.T) { + root := t.TempDir() + leadID := uuid.New() + teamID := uuid.New() + s := &mockTeamStoreLead{teams: []store.TeamData{ + leadTeam(teamID, leadID, store.TeamStatusActive, ""), + }} + req := newArtifactRunRequest(t, root) + req.ChatID = "chat-1" + req.WorkspaceChatID = "chat-1" + + setup, err := newLeadTestLoop(root, leadID, s).injectContext(context.Background(), req) + if err != nil { + t.Fatalf("delegation setup refused: %v", err) + } + if got := tools.ToolWorkspaceFromCtx(setup.ctx); got != req.DelegateOutputsPath { + t.Fatalf("active workspace = %q, want the exchange outputs %q — the team workspace "+ + "must be readable, never the run's own workspace", got, req.DelegateOutputsPath) + } + if got := tools.DelegationArtifactInputsFromCtx(setup.ctx); got != req.DelegateInputsPath { + t.Fatalf("artifact inputs = %q, want %q", got, req.DelegateInputsPath) + } + wantWs := filepath.Join(root, "teams", teamID.String(), "chat-1") + if got := tools.ToolTeamWorkspaceFromCtx(setup.ctx); got != wantWs { + t.Errorf("team workspace = %q, want %q", got, wantWs) + } + wantRoot := filepath.Join(root, "teams", teamID.String()) + if got := tools.ToolTeamRootFromCtx(setup.ctx); got != wantRoot { + t.Errorf("team root = %q, want %q", got, wantRoot) + } + // Read access only: a team ID in context switches on the workspace + // interceptor's write validation and event broadcast, which the delegated + // run has no business triggering. + if got := tools.ToolTeamIDFromCtx(setup.ctx); got != "" { + t.Errorf("team ID = %q, want empty in a delegation artifact run", got) + } +} + +// An agent that leads no team must come out of a delegated run exactly as before. +func TestInjectContext_DelegatedNonLeadGainsNothing(t *testing.T) { + root := t.TempDir() + s := &mockTeamStoreLead{teams: []store.TeamData{ + leadTeam(uuid.New(), uuid.New(), store.TeamStatusActive, ""), + }} + req := newArtifactRunRequest(t, root) + + setup, err := newLeadTestLoop(root, uuid.New(), s).injectContext(context.Background(), req) + if err != nil { + t.Fatalf("injectContext: %v", err) + } + if got := tools.ToolTeamWorkspaceFromCtx(setup.ctx); got != "" { + t.Errorf("team workspace = %q, want empty for an agent leading no team", got) + } + if got := tools.ToolTeamRootFromCtx(setup.ctx); got != "" { + t.Errorf("team root = %q, want empty for an agent leading no team", got) + } +} diff --git a/internal/agent/loop_context.go b/internal/agent/loop_context.go index d0dbab65..7a7ad219 100644 --- a/internal/agent/loop_context.go +++ b/internal/agent/loop_context.go @@ -237,6 +237,15 @@ func (l *Loop) injectContext(ctx context.Context, req *RunRequest) (contextSetup } ctx = tools.WithDelegationArtifactInputs(ctx, req.DelegateInputsPath) ctx = tools.WithToolWorkspace(ctx, req.DelegateOutputsPath) + // A delegated lead is otherwise the only team agent running without its + // own team in context, leaving the team's deliverables unreadable to it + // (#1535). Read allowance only: the active workspace set above stays the + // exchange outputs directory, and req.TeamWorkspace is still rejected as + // an override by the guard above. + if read := l.resolveDelegatedLeadTeamRead(ctx, req); read.ok() { + ctx = tools.WithToolTeamWorkspace(ctx, read.workspace) + ctx = tools.WithToolTeamRoot(ctx, read.root) + } } // Team workspace: dispatched task overrides default workspace. diff --git a/internal/tools/context_keys.go b/internal/tools/context_keys.go index 82364d10..0b2089c6 100644 --- a/internal/tools/context_keys.go +++ b/internal/tools/context_keys.go @@ -660,6 +660,27 @@ func WorkspaceChatIDFromCtx(ctx context.Context) string { return "" } +// OriginChannelFromCtx returns the channel a run ultimately originated from. +// A delegated run is delivered on the internal "delegate" channel while the +// real origin is preserved separately (see buildAgentLinkRunRequest), so team +// scoping and notification routing must follow the origin — addressing them to +// the delivery channel makes the outbound dispatcher drop them. Identity for +// non-delegated runs, where the workspace channel is unset. +func OriginChannelFromCtx(ctx context.Context) string { + if c := WorkspaceChannelFromCtx(ctx); c != "" { + return c + } + return ToolChannelFromCtx(ctx) +} + +// OriginChatIDFromCtx is the chat counterpart of OriginChannelFromCtx. +func OriginChatIDFromCtx(ctx context.Context) string { + if c := WorkspaceChatIDFromCtx(ctx); c != "" { + return c + } + return ToolChatIDFromCtx(ctx) +} + // --- Pending team task dispatch (post-turn processing) --- const ctxPendingDispatch toolContextKey = "tool_pending_team_dispatch" diff --git a/internal/tools/delegated_lead_workspace_test.go b/internal/tools/delegated_lead_workspace_test.go new file mode 100644 index 00000000..da8780c6 --- /dev/null +++ b/internal/tools/delegated_lead_workspace_test.go @@ -0,0 +1,82 @@ +package tools + +import ( + "context" + "strings" + "testing" +) + +func delegationArtifactRunCtx(ctx context.Context) context.Context { + ctx = WithDelegationID(ctx, "11111111-1111-1111-1111-111111111111") + return WithDelegationArtifactInputs(ctx, "/data/collaboration/delegations/11111111-1111-1111-1111-111111111111/inputs") +} + +func containsPrefix(prefixes []string, want string) bool { + for _, p := range prefixes { + if p == want { + return true + } + } + return false +} + +// With its team in context a delegated lead can reach the team's files: reads, +// listing and send_file all resolve paths through allowedWithTeamWorkspace. +func TestTeamPathsAreReadableInDelegationArtifactRun(t *testing.T) { + teamRoot := "/data/teams/team-1" + teamDir := teamRoot + "/chat-1" + ctx := delegationArtifactRunCtx(WithToolTeamRoot(WithToolTeamWorkspace(context.Background(), teamDir), teamRoot)) + + allowed := allowedWithTeamWorkspace(ctx, nil) + if !containsPrefix(allowed, teamDir) { + t.Errorf("team workspace %q missing from allowed read prefixes %v", teamDir, allowed) + } + // The root is the half that matters for a deliverable a member wrote under + // its own chat scope — the lead's own leaf would not cover it. + if !containsPrefix(allowed, teamRoot) { + t.Errorf("team root %q missing from allowed read prefixes %v; a member's file written "+ + "under another chat scope stays unreachable to the lead", teamRoot, allowed) + } +} + +// The root widens reads only. Writes stay in the agent's own leaf, so a lead +// cannot write across a peer's chat scope through an absolute path. +func TestTeamRootDoesNotWidenWrites(t *testing.T) { + teamRoot := "/data/teams/team-1" + teamDir := teamRoot + "/chat-1" + ctx := delegationArtifactRunCtx(WithToolTeamRoot(WithToolTeamWorkspace(context.Background(), teamDir), teamRoot)) + + if allowed := allowedWriteWithTeamWorkspace(ctx, nil); containsPrefix(allowed, teamRoot) { + t.Errorf("team root %q became writable: %v", teamRoot, allowed) + } +} + +// Without a team nothing is granted — an agent leading several teams, or none, +// must not gain a path it did not have before. +func TestNoTeamGrantsNothingExtra(t *testing.T) { + base := []string{"/data/workspace/brain"} + ctx := delegationArtifactRunCtx(context.Background()) + + if got := allowedWithTeamWorkspace(ctx, base); len(got) != len(base) { + t.Errorf("allowed prefixes grew from %v to %v without a team in context", base, got) + } +} + +// Reading the team's files must not turn into pushing them sideways: the +// delegation exchange stays hermetic and send_file keeps refusing inside an +// artifact run regardless of the new read access. +func TestSendFileStaysBlockedInDelegationArtifactRun(t *testing.T) { + teamDir := t.TempDir() + ctx := delegationArtifactRunCtx(WithToolTeamWorkspace(context.Background(), teamDir)) + + tool := &SendFileTool{} + res := tool.Execute(ctx, map[string]any{"path": teamDir + "/deliverable.html"}) + + if !res.IsError { + t.Fatal("send_file succeeded inside a delegation artifact run; files must be published " + + "through the artifact exchange when the run completes") + } + if !strings.Contains(res.ForLLM, "published only after the delegated run completes") { + t.Errorf("unexpected refusal reason: %s", res.ForLLM) + } +} diff --git a/internal/tools/team_event_helpers.go b/internal/tools/team_event_helpers.go index eff3494f..dee6d3a9 100644 --- a/internal/tools/team_event_helpers.go +++ b/internal/tools/team_event_helpers.go @@ -118,8 +118,8 @@ func WithProgress(percent int, step string) TaskEventOption { func WithContextInfo(ctx context.Context) TaskEventOption { return func(p *protocol.TeamTaskEventPayload) { p.UserID = store.UserIDFromContext(ctx) - p.Channel = ToolChannelFromCtx(ctx) - p.ChatID = ToolChatIDFromCtx(ctx) + p.Channel = OriginChannelFromCtx(ctx) + p.ChatID = OriginChatIDFromCtx(ctx) p.PeerKind = ToolPeerKindFromCtx(ctx) p.LocalKey = ToolLocalKeyFromCtx(ctx) } diff --git a/internal/tools/team_task_origin_channel_test.go b/internal/tools/team_task_origin_channel_test.go new file mode 100644 index 00000000..27b49ee4 --- /dev/null +++ b/internal/tools/team_task_origin_channel_test.go @@ -0,0 +1,204 @@ +package tools + +import ( + "context" + "strings" + "testing" + + "github.com/nextlevelbuilder/goclaw/internal/store" + "github.com/nextlevelbuilder/goclaw/pkg/protocol" +) + +// A lead reached through `delegate` runs on the internal "delegate" delivery +// channel, while the real origin is preserved separately. A task must record +// the origin: notifications addressed to the delivery channel have no +// registered handler and are dropped by the outbound dispatcher, so the caller +// never hears about completions, failures or blocker escalations. +func TestCreateRecordsDelegationOriginNotDeliveryChannel(t *testing.T) { + mb, tool, _, _, ctx := newTestTeamSetup() + + // Shape the context the way a delegated run is built (buildAgentLinkRunRequest): + // delivery channel is "delegate", origin preserved as the workspace channel/chat. + ctx = WithToolChannel(ctx, "delegate") + ctx = WithToolChatID(ctx, "system") + ctx = WithWorkspaceChannel(ctx, "telegram") + ctx = WithWorkspaceChatID(ctx, "313683273") + + ptd := NewPendingTeamDispatch() + ptd.MarkListed() + ctx = WithPendingTeamDispatch(ctx, ptd) + + result := tool.Execute(ctx, map[string]any{ + "action": "create", + "subject": "Origin routing", + "description": "Task created by a delegated lead", + "assignee": "member-agent", + }) + if result.IsError { + t.Fatalf("unexpected error: %s", result.ForLLM) + } + if !strings.Contains(result.ForLLM, "Task created") { + t.Fatalf("expected 'Task created', got: %s", result.ForLLM) + } + + mb.taskStore.mu.Lock() + var task *store.TeamTaskData + for _, v := range mb.taskStore.tasks { + task = v + } + mb.taskStore.mu.Unlock() + if task == nil { + t.Fatal("no task was created") + } + if task.Channel != "telegram" { + t.Errorf("task.Channel = %q, want %q — notifications on the delivery channel are dropped", task.Channel, "telegram") + } + if task.ChatID != "313683273" { + t.Errorf("task.ChatID = %q, want %q", task.ChatID, "313683273") + } +} + +// Without a delegation origin the resolution is the identity: a task keeps the +// channel and chat of the run that created it. +func TestCreateKeepsOwnChannelWithoutDelegationOrigin(t *testing.T) { + mb, tool, _, _, ctx := newTestTeamSetup() + + ptd := NewPendingTeamDispatch() + ptd.MarkListed() + ctx = WithPendingTeamDispatch(ctx, ptd) + + result := tool.Execute(ctx, map[string]any{ + "action": "create", + "subject": "Direct routing", + "description": "Task created by a lead talking to the user directly", + "assignee": "member-agent", + }) + if result.IsError { + t.Fatalf("unexpected error: %s", result.ForLLM) + } + + mb.taskStore.mu.Lock() + var task *store.TeamTaskData + for _, v := range mb.taskStore.tasks { + task = v + } + mb.taskStore.mu.Unlock() + if task == nil { + t.Fatal("no task was created") + } + if task.Channel != ChannelDashboard { + t.Errorf("task.Channel = %q, want %q", task.Channel, ChannelDashboard) + } + if task.ChatID != testTeamID.String() { + t.Errorf("task.ChatID = %q, want %q", task.ChatID, testTeamID.String()) + } +} + +// delegatedCtx shapes a context the way buildAgentLinkRunRequest does: the run is +// delivered on the internal "delegate" channel while the caller's origin is +// preserved as the workspace channel and chat. +func delegatedCtx(ctx context.Context, originChannel, originChat string) context.Context { + ctx = WithToolChannel(ctx, "delegate") + ctx = WithToolChatID(ctx, "system") + ctx = WithWorkspaceChannel(ctx, originChannel) + ctx = WithWorkspaceChatID(ctx, originChat) + ptd := NewPendingTeamDispatch() + ptd.MarkListed() + return WithPendingTeamDispatch(ctx, ptd) +} + +func createTaskIn(t *testing.T, tool *TeamTasksTool, ctx context.Context, subject string) string { + t.Helper() + res := tool.Execute(ctx, map[string]any{ + "action": "create", + "subject": subject, + "description": "work", + "assignee": "member-agent", + }) + if res.IsError { + t.Fatalf("create %q: %s", subject, res.ForLLM) + } + id := res.ForLLM + start := strings.Index(id, "id=") + if start < 0 { + t.Fatalf("no task id in %q", res.ForLLM) + } + id = id[start+3:] + if end := strings.IndexAny(id, ","); end >= 0 { + id = id[:end] + } + return id +} + +func completionEvent(t *testing.T, mb *mockBackend, taskID string) protocol.TeamTaskEventPayload { + t.Helper() + mb.mu.Lock() + defer mb.mu.Unlock() + for _, ev := range mb.events { + p, ok := ev.Payload.(protocol.TeamTaskEventPayload) + if ok && ev.Name == protocol.EventTeamTaskCompleted && p.TaskID == taskID { + return p + } + } + t.Fatalf("no completion event for task %s", taskID) + return protocol.TeamTaskEventPayload{} +} + +// The notification a delegated lead emits on completion must be addressed to the +// caller's origin. Addressed to the internal "delegate" delivery channel it is +// dropped by the outbound dispatcher and the user never learns the work finished. +func TestDelegatedLeadCompletionNotifiesOrigin(t *testing.T) { + mb, tool, _, _, base := newTestTeamSetup() + ctx := delegatedCtx(base, "telegram", "313683273") + + taskID := createTaskIn(t, tool, ctx, "Deliverable") + res := tool.Execute(ctx, map[string]any{ + "action": "complete", + "task_id": taskID, + "result": "done", + }) + if res.IsError { + t.Fatalf("complete: %s", res.ForLLM) + } + + ev := completionEvent(t, mb, taskID) + if ev.Channel != "telegram" || ev.ChatID != "313683273" { + t.Errorf("completion event addressed to %s/%s, want telegram/313683273 — "+ + "the delivery channel has no registered handler and the notification is dropped", + ev.Channel, ev.ChatID) + } +} + +// Origins must not bleed into each other: completing a task raised from one chat +// must not produce a notification addressed to an unrelated origin sharing the +// same board. +func TestCompletionNotificationStaysWithinItsOwnOrigin(t *testing.T) { + mb, tool, _, _, base := newTestTeamSetup() + ctxA := delegatedCtx(base, "telegram", "chat-A") + ctxB := delegatedCtx(base, "discord", "chat-B") + + taskA := createTaskIn(t, tool, ctxA, "Task A") + createTaskIn(t, tool, ctxB, "Task B") + + res := tool.Execute(ctxA, map[string]any{"action": "complete", "task_id": taskA, "result": "done"}) + if res.IsError { + t.Fatalf("complete: %s", res.ForLLM) + } + + if ev := completionEvent(t, mb, taskA); ev.Channel != "telegram" || ev.ChatID != "chat-A" { + t.Errorf("task A announced to %s/%s, want telegram/chat-A", ev.Channel, ev.ChatID) + } + + mb.mu.Lock() + defer mb.mu.Unlock() + for _, ev := range mb.events { + p, ok := ev.Payload.(protocol.TeamTaskEventPayload) + if !ok || ev.Name != protocol.EventTeamTaskCompleted { + continue + } + if p.Channel == "discord" || p.ChatID == "chat-B" { + t.Errorf("completing task A produced a notification addressed to the unrelated origin %s/%s", + p.Channel, p.ChatID) + } + } +} diff --git a/internal/tools/team_tasks_create.go b/internal/tools/team_tasks_create.go index c1bc3cac..e02c8337 100644 --- a/internal/tools/team_tasks_create.go +++ b/internal/tools/team_tasks_create.go @@ -23,7 +23,7 @@ func (t *TeamTasksTool) executeCreate(ctx context.Context, args map[string]any) // Determine if caller is a lead or a member. isLead := agentID == team.LeadAgentID - channel := ToolChannelFromCtx(ctx) + channel := OriginChannelFromCtx(ctx) if channel == ChannelTeammate || channel == ChannelSystem { isLead = true // system/teammate channels act on behalf of the lead } @@ -139,7 +139,7 @@ func (t *TeamTasksTool) executeCreate(ctx context.Context, args map[string]any) memberCfgForDispatch := ParseMemberRequestConfig(team.Settings) skipAutoDispatch := !isLead && taskType == "request" && !memberCfgForDispatch.AutoDispatch - chatID := ToolChatIDFromCtx(ctx) + chatID := OriginChatIDFromCtx(ctx) // Compute team workspace via layered pipeline: tenant → team → user/chat. shared := IsSharedWorkspace(team.Settings) @@ -226,7 +226,7 @@ func (t *TeamTasksTool) executeCreate(ctx context.Context, args map[string]any) // this same UserID. Migrating to ActorIDFromContext would hide group // members' shared work from each other. UserID: store.UserIDFromContext(ctx), - Channel: ToolChannelFromCtx(ctx), + Channel: OriginChannelFromCtx(ctx), TaskType: taskType, CreatedByAgentID: &agentID, ChatID: chatID, diff --git a/internal/tools/team_tasks_followup.go b/internal/tools/team_tasks_followup.go index b2d163cb..36224d83 100644 --- a/internal/tools/team_tasks_followup.go +++ b/internal/tools/team_tasks_followup.go @@ -39,10 +39,10 @@ func (t *TeamTasksTool) executeAskUser(ctx context.Context, args map[string]any) // Resolve channel: prefer task's channel, fallback to context channel. channel := task.Channel chatID := task.ChatID - ctxChannel := ToolChannelFromCtx(ctx) + ctxChannel := OriginChannelFromCtx(ctx) if channel == "" || channel == ChannelTeammate || channel == ChannelSystem || channel == ChannelDashboard { channel = ctxChannel - chatID = ToolChatIDFromCtx(ctx) + chatID = OriginChatIDFromCtx(ctx) } if channel == "" || channel == ChannelTeammate || channel == ChannelSystem || channel == ChannelDashboard { return ErrorResult("cannot set follow-up: no valid channel found (task has no origin channel and context channel is internal)") diff --git a/internal/tools/team_tasks_mutations.go b/internal/tools/team_tasks_mutations.go index bc037b07..9b25b6bf 100644 --- a/internal/tools/team_tasks_mutations.go +++ b/internal/tools/team_tasks_mutations.go @@ -211,7 +211,7 @@ func (t *TeamTasksTool) executeAttach(ctx context.Context, args map[string]any) return ErrorResult("task does not belong to your team") } - chatID := ToolChatIDFromCtx(ctx) + chatID := OriginChatIDFromCtx(ctx) if err := t.manager.Store().AttachFileToTask(ctx, &store.TeamTaskAttachmentData{ TaskID: taskID, TeamID: team.ID, diff --git a/internal/tools/team_tasks_read.go b/internal/tools/team_tasks_read.go index cad3f774..982e271c 100644 --- a/internal/tools/team_tasks_read.go +++ b/internal/tools/team_tasks_read.go @@ -197,11 +197,11 @@ func (t *TeamTasksTool) executeList(ctx context.Context, args map[string]any) *R // Teammate/system channels see all tasks; end users only see their own. filterUserID := "" - channel := ToolChannelFromCtx(ctx) + channel := OriginChannelFromCtx(ctx) if channel != ChannelTeammate && channel != ChannelSystem { filterUserID = store.UserIDFromContext(ctx) } - chatID := ToolChatIDFromCtx(ctx) + chatID := OriginChatIDFromCtx(ctx) // Shared workspace: show all tasks across chats. listChatID := chatID if IsSharedWorkspace(team.Settings) { @@ -375,13 +375,13 @@ func (t *TeamTasksTool) executeSearch(ctx context.Context, args map[string]any) // Teammate/system channels see all tasks; end users only see their own. filterUserID := "" - channel := ToolChannelFromCtx(ctx) + channel := OriginChannelFromCtx(ctx) if channel != ChannelTeammate && channel != ChannelSystem { filterUserID = store.UserIDFromContext(ctx) } // Acquire team create lock so search also satisfies the list-before-create gate. - chatID := ToolChatIDFromCtx(ctx) + chatID := OriginChatIDFromCtx(ctx) if ptd := PendingTeamDispatchFromCtx(ctx); ptd != nil && !ptd.HasListed() { lock := getTeamCreateLock(team.ID.String(), chatID) lock.Lock() diff --git a/internal/tools/team_tool_dispatch.go b/internal/tools/team_tool_dispatch.go index 3cfafc27..8367f5c0 100644 --- a/internal/tools/team_tool_dispatch.go +++ b/internal/tools/team_tool_dispatch.go @@ -123,11 +123,11 @@ func (m *TeamToolManager) dispatchTaskToAgent(ctx context.Context, task *store.T // Falls back to ctx values for initial dispatch (task just created, fields match ctx). originChannel := task.Channel if originChannel == "" { - originChannel = ToolChannelFromCtx(ctx) + originChannel = OriginChannelFromCtx(ctx) } originChatID := task.ChatID if originChatID == "" { - originChatID = ToolChatIDFromCtx(ctx) + originChatID = OriginChatIDFromCtx(ctx) } // Resolve lead agent key for completion announce routing. fromAgent := ToolAgentKeyFromCtx(ctx) diff --git a/internal/tools/team_tool_helpers.go b/internal/tools/team_tool_helpers.go index dc64d934..0d9ceac7 100644 --- a/internal/tools/team_tool_helpers.go +++ b/internal/tools/team_tool_helpers.go @@ -195,10 +195,10 @@ func (m *TeamToolManager) createEscalationTask(ctx context.Context, team *store. Description: description, Status: store.TeamTaskStatusPending, UserID: store.UserIDFromContext(ctx), - Channel: ToolChannelFromCtx(ctx), + Channel: OriginChannelFromCtx(ctx), TaskType: "escalation", CreatedByAgentID: &agentID, - ChatID: ToolChatIDFromCtx(ctx), + ChatID: OriginChatIDFromCtx(ctx), } if err := m.teamStore.CreateTask(ctx, task); err != nil { return ErrorResult("failed to create escalation task: " + err.Error()) diff --git a/internal/tools/team_tool_validation.go b/internal/tools/team_tool_validation.go index 7266f2a2..b7cec0ce 100644 --- a/internal/tools/team_tool_validation.go +++ b/internal/tools/team_tool_validation.go @@ -174,8 +174,8 @@ func (m *TeamToolManager) notifyLeaderCycleError(ctx context.Context, teamID uui content := fmt.Sprintf("[System] %s\nPlease recreate these tasks with corrected dependencies.\nUse team_tasks(action=\"list\") to view current task board.", cycleDesc) // Resolve routing: use context channel/chatID if available, fallback to dashboard. - channel := ToolChannelFromCtx(ctx) - chatID := ToolChatIDFromCtx(ctx) + channel := OriginChannelFromCtx(ctx) + chatID := OriginChatIDFromCtx(ctx) if channel == "" || channel == ChannelSystem || channel == ChannelTeammate { channel = "dashboard" chatID = teamID.String()