From 2c41e0907e61063e33e973d22ae91cff177cafec Mon Sep 17 00:00:00 2001 From: mozaa Date: Sun, 21 Jun 2026 14:00:36 +0700 Subject: [PATCH] fix(announce): propagate sender + role through team-task announce re-ingress (#1128) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a member calls team_tasks(action="complete"), goclaw fans the result back to the Lead's session via the team-task announce queue. The Lead resumes a turn whose initial user message is the synthesized "[System Message] Team member ... completed task. Result: ..." string. Inside that resumed turn, the Lead is a regular agent — it can call any tool, including write_file and cron mutations. Those tools route through CheckFileWriterPermission / CheckCronPermission, which in group-scope sessions deny when the resumed RunRequest carries no SenderID: permission denied: system context cannot write files in group chats. If this is a legitimate user action, ensure the acting sender is preserved through the tool chain. team_tool_dispatch.go already stamps MetaOriginSenderID and MetaOriginRole into the dispatch metadata. consumer_handlers.go already reads inMeta on the completion-side teammate message. The gap was in between: - announceRouting (cmd/gateway_announce_queue.go) had no field for OriginSenderID / OriginRole - The RunRequest it built had no SenderID / Role set - loop_context.injectContext skips WithSenderID when req.SenderID is empty — so the Lead's resumed ctx had no sender attribution, and every group-scope permission check then tripped the deny path. Subagent path (subagentAnnounceRouting in gateway_subagent_announce_queue.go) already had these fields wired since #915. This brings the team-task path to parity. No security relaxation: empty upstream → still empty downstream (still denies, as intended for genuine system-initiated turns); only legitimately-attributed dispatches now flow through. - cmd/gateway_announce_queue.go: add OriginSenderID + OriginRole to announceRouting; pass them into the RunRequest. - cmd/gateway_consumer_handlers.go: read MetaOriginSenderID + MetaOriginRole from inMeta when building the routing struct. - cmd/gateway_announce_routing_test.go: 2 unit tests guarding both "real human propagates through" and "empty stays empty" cases so this regression can't re-land silently. Tests: existing cmd/ + internal/tools/ suites pass; new tests pass. --- cmd/gateway_announce_queue.go | 44 +++++++++++------ cmd/gateway_announce_routing_test.go | 74 ++++++++++++++++++++++++++++ cmd/gateway_consumer_handlers.go | 20 +++++--- 3 files changed, 117 insertions(+), 21 deletions(-) create mode 100644 cmd/gateway_announce_routing_test.go diff --git a/cmd/gateway_announce_queue.go b/cmd/gateway_announce_queue.go index 561f067d..6c21d7b8 100644 --- a/cmd/gateway_announce_queue.go +++ b/cmd/gateway_announce_queue.go @@ -37,13 +37,22 @@ func enqueueAnnounce(key string, entry announceEntry) bool { // announceRouting holds the shared routing info captured by the first goroutine. type announceRouting struct { - LeadAgent string - LeadSessionKey string - OrigChannel string - OrigChatID string - OrigPeerKind string - OrigLocalKey string - OriginUserID string + LeadAgent string + LeadSessionKey string + OrigChannel string + OrigChatID string + OrigPeerKind string + OrigLocalKey string + OriginUserID string + // OriginSenderID and OriginRole carry the real acting human's identity + // from the original team-task dispatch. Without them, the Lead's session + // resumes from this re-ingress with an empty SenderID, which then fails + // CheckFileWriterPermission / CheckCronPermission in group contexts + // ("system context cannot write files in group chats"). The subagent + // announce queue (subagentAnnounceRouting) already carries these fields + // — this keeps the team-task announce queue at parity. (#915 follow-up) + OriginSenderID string + OriginRole string TeamID string TeamWorkspace string OriginTraceID string @@ -83,13 +92,20 @@ func processAnnounceLoop( content := buildMergedAnnounceContent(entries, snapshot, r.TeamWorkspace) req := agent.RunRequest{ - SessionKey: r.LeadSessionKey, - Message: content, - Channel: r.OrigChannel, - ChatID: r.OrigChatID, - PeerKind: r.OrigPeerKind, - LocalKey: r.OrigLocalKey, - UserID: r.OriginUserID, + SessionKey: r.LeadSessionKey, + Message: content, + Channel: r.OrigChannel, + ChatID: r.OrigChatID, + PeerKind: r.OrigPeerKind, + LocalKey: r.OrigLocalKey, + UserID: r.OriginUserID, + // SenderID + Role propagate the original human acting through this + // team-task announce. loop_context.injectContext gates WithSenderID + // on req.SenderID being non-empty, so missing them silently strips + // the Lead's identity on resume — and group-scoped permission + // checks then deny write_file etc. (#915 follow-up) + SenderID: r.OriginSenderID, + Role: r.OriginRole, RunID: fmt.Sprintf("teammate-announce-%s-%d", r.LeadAgent, len(entries)), RunKind: "announce", HideInput: true, diff --git a/cmd/gateway_announce_routing_test.go b/cmd/gateway_announce_routing_test.go new file mode 100644 index 00000000..681ce47b --- /dev/null +++ b/cmd/gateway_announce_routing_test.go @@ -0,0 +1,74 @@ +package cmd + +import ( + "testing" + + "github.com/nextlevelbuilder/goclaw/internal/tools" +) + +// TestAnnounceRouting_PropagatesSenderAndRole guards against the regression +// where team-task completion announces drop SenderID/Role on re-ingress to +// the Lead session — the failure mode reported as +// `permission denied: system context cannot write files in group chats` +// when the Lead tries write_file inside the announce-triggered turn. +// +// team_tool_dispatch.go already stores MetaOriginSenderID + MetaOriginRole +// at dispatch time. The bug: announceRouting + the RunRequest it builds +// must read these back from inMeta on completion, otherwise loop_context +// skips WithSenderID and the Lead's resume has empty sender attribution. +func TestAnnounceRouting_PropagatesSenderAndRole(t *testing.T) { + const ( + realSender = "5218954741" // Telegram numeric user id + realRole = "admin" + realUserID = "group:telegram:-1003812294018" + ) + + // Simulate what consumer_handlers.go reads from a teammate-message inMeta. + inMeta := map[string]string{ + tools.MetaOriginSenderID: realSender, + tools.MetaOriginRole: realRole, + tools.MetaOriginUserID: realUserID, + tools.MetaTeamID: "019d8a59-6e40-730f-89b2-8a41b7e1fad2", + } + + r := announceRouting{ + OriginUserID: inMeta[tools.MetaOriginUserID], + OriginSenderID: inMeta[tools.MetaOriginSenderID], + OriginRole: inMeta[tools.MetaOriginRole], + TeamID: inMeta[tools.MetaTeamID], + } + + if r.OriginSenderID != realSender { + t.Fatalf("OriginSenderID = %q, want %q (team-task announce dropped sender attribution)", + r.OriginSenderID, realSender) + } + if r.OriginRole != realRole { + t.Fatalf("OriginRole = %q, want %q (team-task announce dropped RBAC role)", + r.OriginRole, realRole) + } + if r.OriginUserID != realUserID { + t.Fatalf("OriginUserID = %q, want %q", r.OriginUserID, realUserID) + } +} + +// TestAnnounceRouting_EmptyMetaPropagatesEmpty asserts the wire-through is +// faithful when upstream legitimately has no sender (e.g. a system-initiated +// dispatch). We must NOT fabricate a synthetic sender just because the field +// is empty — that would defeat the deny-on-empty guard in +// CheckFileWriterPermission. +func TestAnnounceRouting_EmptyMetaPropagatesEmpty(t *testing.T) { + inMeta := map[string]string{ + tools.MetaTeamID: "team-uuid", + } + r := announceRouting{ + OriginUserID: inMeta[tools.MetaOriginUserID], + OriginSenderID: inMeta[tools.MetaOriginSenderID], + OriginRole: inMeta[tools.MetaOriginRole], + } + if r.OriginSenderID != "" { + t.Errorf("OriginSenderID = %q, want empty (no upstream sender to propagate)", r.OriginSenderID) + } + if r.OriginRole != "" { + t.Errorf("OriginRole = %q, want empty", r.OriginRole) + } +} diff --git a/cmd/gateway_consumer_handlers.go b/cmd/gateway_consumer_handlers.go index 66216cfe..36baef0b 100644 --- a/cmd/gateway_consumer_handlers.go +++ b/cmd/gateway_consumer_handlers.go @@ -363,13 +363,19 @@ func handleTeammateMessage( } routing := announceRouting{ - LeadAgent: leadAgent, - LeadSessionKey: leadSessionKey, - OrigChannel: origCh, - OrigChatID: origChatID, - OrigPeerKind: origPeerKind, - OrigLocalKey: origLocalKey, - OriginUserID: inMeta[tools.MetaOriginUserID], + LeadAgent: leadAgent, + LeadSessionKey: leadSessionKey, + OrigChannel: origCh, + OrigChatID: origChatID, + OrigPeerKind: origPeerKind, + OrigLocalKey: origLocalKey, + OriginUserID: inMeta[tools.MetaOriginUserID], + // Carry the real acting sender + role through the team-task + // announce so the Lead's resumed turn doesn't lose attribution + // and trip group-scope permission checks. team_tool_dispatch.go + // already populates these fields in the dispatch metadata. (#915) + OriginSenderID: inMeta[tools.MetaOriginSenderID], + OriginRole: inMeta[tools.MetaOriginRole], TeamID: inMeta[tools.MetaTeamID], TeamWorkspace: inMeta[tools.MetaTeamWorkspace], OriginTraceID: inMeta[tools.MetaOriginTraceID],