mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-14 02:15:57 +00:00
fix(announce): propagate sender + role through team-task announce re-ingress (#1128)
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.
This commit is contained in:
1 parent
756c4dce71
commit
2c41e0907e
3 files changed
+117
-21
No files matched your search
@@ -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,
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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],
|
||||
|
||||
Reference in new issue
Block a user