mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-11 12:18:59 +00:00
fix(teams): grant a delegated lead read access to its own team (#1537)
* fix(teams): scope team tasks to the delegation origin, not the delivery channel
A delegated run is delivered on the internal "delegate" channel while its real
origin is preserved separately — buildAgentLinkRunRequest is explicit about it:
// preserves the origin's authorization-bearing identity while keeping
// delegation on its internal delivery channel.
Channel: "delegate",
WorkspaceChannel: req.Channel,
WorkspaceChatID: req.ChatID,
The team tools did not consult that origin, so a task created by a lead reached
through delegate was stamped with the delivery channel. Nothing is registered
for it, so every notification about that task — completion, failure, blocker
escalation, ask_user — was dropped:
unknown channel for outbound message channel=delegate
The delegatee's first answer still arrived, because it travels back as the
delegation result rather than through a channel; everything the lead said after
the delegation closed was lost. One session produced 16 such drops, including a
blocker escalation the user needed to see.
Add OriginChannelFromCtx / OriginChatIDFromCtx next to the existing workspace
scope propagation helpers, and use them where team scoping and notification
routing are decided: task records, list/search scoping, dispatch fallbacks,
event payloads, escalation tasks, ask_user and leader notifications. The
resolution is the identity when no delegation origin is present, so
non-delegated flows are unchanged.
Deliberately not touched: the channel used in authorization decisions
(checkTeamAccess, requireLead, approve/reject lead bypass). Those ask "how did
this call arrive", not "where should the answer go", and widening them is a
separate question.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(teams): cover delegated-lead completion routing and origin isolation
Triage on #1529 named two gates this PR had not met: regression coverage for
delegated lead task completion, and a guard that unrelated origins cannot
receive the notification. The existing tests only covered what create persists.
TestDelegatedLeadCompletionNotifiesOrigin completes a task raised in a delegated
context and asserts the completion event is addressed to the caller's origin.
Reverting team_event_helpers.go to the delivery channel fails it with
"addressed to delegate/system".
TestCompletionNotificationStaysWithinItsOwnOrigin puts two tasks from different
origins on one board, completes one, and asserts no completion notification
carries the other origin.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(teams): grant a delegated lead read access to its own team
Fixes #1535. Second attempt: the first one set RunRequest.TeamWorkspace on the
delegated run, which loop_context.go rejects outright and deliberately so — that
field also becomes ToolWorkspace, which would displace the exchange outputs
directory. Every delegation then died at setup with "invalid delegation artifact
workspace". This does not go near that field.
The separation the lead needs already exists in the code. A lead addressed
directly resolves its team at loop_context.go:285-321 and gets
ctx = tools.WithToolTeamWorkspace(ctx, wsDir)
ctx = tools.WithToolTeamRoot(ctx, teamRoot)
with no WithToolWorkspace call — a read allowance, not an override. Only the
!isArtifactDelegation gate keeps a delegated lead out of it. So the grant is
made inside the artifact branch instead, from the same inputs, and the guard on
req.TeamWorkspace stays exactly as strict as it was.
Both paths are needed. The workspace alone covers only the lead's own chat leaf;
the deliverable in the report sits at teams/<id>/system/review-....md, written by
a member under a different chat scope. buildAllowedPrefixes adds the team root
for reads and not for writes (filesystem.go:366-370), which is precisely the
asymmetry this case wants: the lead reads the team's output, per-chat write
isolation is untouched.
Team ID is deliberately not set. It switches on the workspace interceptor's
write validation, file-change broadcast and task attachment (workspace_
interceptor.go:36,114,203) — none of which a delegated run should trigger, and
none of which reading needs.
Ambiguity is not resolved silently, as triage asked: an agent leading more than
one active team gets nothing. store.GetTeamForAgent would answer in one call but
it is ORDER BY (lead_agent_id = $1) DESC LIMIT 1 and would quietly pick a team.
Hermeticity is unchanged: send_file and message still refuse inside an artifact
run, publication still happens through delegation outputs on completion.
Tests: TestInjectContext_DelegatedLeadReadsTeamWithoutDisplacingOutputs drives
injectContext through the guard that broke the first attempt and pins both
halves — the run is not refused, and ToolWorkspace stays the outputs directory.
Resolution rules, the shared/isolated path shapes, read-not-write on the team
root, and send_file staying blocked are covered alongside.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(teams): pin that the delegated lead resolver does not re-scope by tenant
l.dataDir arrives already tenant-scoped from resolver.go (config.TenantDataDir),
which is why resolveDelegatedLeadTeamRead must not apply TenantLayer itself.
That was carried only by a comment, here and in the sibling branch of
injectContext, and nothing failed if someone added the layer back.
The failure it guards against is quiet: the path stays plausible, it just gains
a second tenant segment — teams/<id> under tenants/<slug>/tenants/<slug> — so
the lead is handed a directory its own tasks never write to, and reads come back
empty rather than denied.
Verified the assertion has teeth by reintroducing the layer: the subtest fails
with "tenant segment appears 2 times". Prompted by #1546 in this same area,
where a contract that lived outside the unit under test went unpinned and the
feature shipped green and did nothing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
60cf79de51
commit
305bd7c50d
14 files changed
+658
-18
No files matched your search
@@ -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)),
|
||||
}
|
||||
}
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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.
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -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,
|
||||
|
||||
@@ -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)")
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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())
|
||||
|
||||
@@ -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()
|
||||
|
||||
Reference in new issue
Block a user