From ce98b1c9efdfde01fedfcf37abcc7f48cf5ff5ae Mon Sep 17 00:00:00 2001 From: Bohan Jiang <52446949+Bohan-J@users.noreply.github.com> Date: Mon, 25 May 2026 17:12:33 +0800 Subject: [PATCH] fix(squad): skip leader on agent reply to explicit member @-mention (MUL-2624) (#3217) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a user explicitly @-mentions an agent on an issue assigned to a squad, the existing rule already suppresses the squad leader on the mention comment itself — the user is routing deliberately, the mentioned agent owns the next step. The leader was still woken on the agent's reply, though, so it would re-@ the user every time the agent answered. Extend the suppression to the second leg of that explicit exchange: when an agent reply lands as a child of a member comment that carried a routing @mention (agent/member/squad/all — issue cross-refs still ignored), the leader stays out. The CreateComment handler already pins agent parent_id == task.TriggerCommentID, so this fires exactly when the agent's reply is provably tied to the upstream routing comment. Top-level agent comments and agent-to-agent threads continue to wake the leader so coordination keeps working everywhere else. Co-authored-by: multica-agent --- server/internal/handler/comment.go | 10 +- server/internal/handler/squad.go | 47 +++- .../handler/squad_comment_trigger_test.go | 209 +++++++++++++++++- 3 files changed, 245 insertions(+), 21 deletions(-) diff --git a/server/internal/handler/comment.go b/server/internal/handler/comment.go index 63a7fe186f..4568e1b1a6 100644 --- a/server/internal/handler/comment.go +++ b/server/internal/handler/comment.go @@ -745,10 +745,12 @@ func (h *Handler) CreateComment(w http.ResponseWriter, r *http.Request) { } // Squad trigger: if the issue is assigned to a squad, trigger the squad leader. - // Skip when the comment author is the leader (prevent internal loops), or - // when a member explicitly @mentions anyone (agent/member/squad/all) — that - // counts as deliberate routing and the leader stays out. - if h.shouldEnqueueSquadLeaderOnComment(r.Context(), issue, comment.Content, authorType, authorID) { + // Skip when the comment author is the leader (prevent internal loops), when + // a member explicitly @mentions anyone (agent/member/squad/all) — deliberate + // routing — or when an agent's reply continues an explicitly-routed exchange + // the human already opened (parent is a member comment with routing mentions, + // MUL-2624). + if h.shouldEnqueueSquadLeaderOnComment(r.Context(), issue, comment.Content, authorType, authorID, parentComment) { h.enqueueSquadLeaderTask(r.Context(), issue, comment.ID, authorType, authorID) } diff --git a/server/internal/handler/squad.go b/server/internal/handler/squad.go index 0e580186c3..8c4adcc019 100644 --- a/server/internal/handler/squad.go +++ b/server/internal/handler/squad.go @@ -896,15 +896,27 @@ func (h *Handler) RecordSquadLeaderEvaluation(w http.ResponseWriter, r *http.Req // ── Squad Trigger Logic ───────────────────────────────────────────────────── // shouldEnqueueSquadLeaderOnComment returns true if the issue is assigned to a -// squad and the comment author is NOT a member of that squad (anti-loop). -// commentContent is the new comment's markdown body; when a member explicitly -// @mentions anyone (agent, member, squad, or @all) in that body, the leader -// is skipped — the @ marks deliberate routing and the leader would otherwise -// just observe and record no_action. Issue cross-reference mentions -// (mention://issue/...) are NOT a routing signal and do not suppress the -// leader. Agent-authored comments always go through the leader (subject to -// the leader self-trigger guard) so agent updates still drive coordination. -func (h *Handler) shouldEnqueueSquadLeaderOnComment(ctx context.Context, issue db.Issue, commentContent, authorType, authorID string) bool { +// squad and the comment should wake the squad leader. commentContent is the +// new comment's markdown body; parentComment is the row referenced by the +// comment's parent_id (nil for a top-level comment). +// +// Suppression rules — leader stays out when: +// +// 1. Comment author is the leader itself AND its last activity on this issue +// was a leader-role task (self-trigger loop guard). +// 2. Member explicitly @mentions anyone (agent/member/squad/@all) — the @ +// marks deliberate routing and the leader would just no_action. +// 3. Agent's reply lands as a child of a member comment that itself carried +// a routing @mention. The human routed the conversation explicitly; the +// agent's reply is the second leg of that exchange, not new work for the +// leader to coordinate. Without this, the leader would re-@ the user on +// every agent reply to an explicit ping. (MUL-2624) +// +// Issue cross-reference mentions (mention://issue/...) are NOT a routing +// signal and never suppress the leader. Agent comments that are NOT replies +// to a routing member comment still go through the leader so agent-initiated +// updates keep driving coordination. +func (h *Handler) shouldEnqueueSquadLeaderOnComment(ctx context.Context, issue db.Issue, commentContent, authorType, authorID string, parentComment *db.Comment) bool { if !issue.AssigneeType.Valid || issue.AssigneeType.String != "squad" || !issue.AssigneeID.Valid { return false } @@ -931,13 +943,24 @@ func (h *Handler) shouldEnqueueSquadLeaderOnComment(ctx context.Context, issue d // Member explicitly @mentioned someone → that someone owns the next step, // skip the leader. Covers @agent / @member / @squad / @all; issue - // cross-references do NOT count as routing. Agent-authored comments are - // intentionally exempt: when an agent posts a result that @mentions - // another agent, the leader still needs to coordinate the thread. + // cross-references do NOT count as routing. if authorType == "member" && commentMentionsAnyone(commentContent) { return false } + // Agent reply directly under a member comment that explicitly @mentioned + // someone is the continuation of that routed exchange — same rule as the + // member-side skip above, applied to the second leg. The parent comment + // here is the comment-triggered task's trigger comment (enforced in the + // CreateComment handler: agent parent_id must equal task.TriggerCommentID), + // so this only fires when the agent's reply is provably tied to that + // upstream routing comment. + if authorType == "agent" && parentComment != nil && + parentComment.AuthorType == "member" && + commentMentionsAnyone(parentComment.Content) { + return false + } + // Verify leader agent is ready (has runtime, not archived). agent, err := h.Queries.GetAgent(ctx, squad.LeaderID) if err != nil || !agent.RuntimeID.Valid || agent.ArchivedAt.Valid { diff --git a/server/internal/handler/squad_comment_trigger_test.go b/server/internal/handler/squad_comment_trigger_test.go index 99b4a5cc5b..e1469be6d8 100644 --- a/server/internal/handler/squad_comment_trigger_test.go +++ b/server/internal/handler/squad_comment_trigger_test.go @@ -193,7 +193,7 @@ func TestShouldEnqueueSquadLeaderOnComment_SkipsWhenMemberMentionsAnyone(t *test for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - got := testHandler.shouldEnqueueSquadLeaderOnComment(ctx, fx.Issue, tc.content, tc.authorType, tc.authorID) + got := testHandler.shouldEnqueueSquadLeaderOnComment(ctx, fx.Issue, tc.content, tc.authorType, tc.authorID, nil) if got != tc.want { t.Fatalf("%s\n content=%q author=%s/%s\n got=%v want=%v", tc.description, tc.content, tc.authorType, tc.authorID, got, tc.want) @@ -202,6 +202,205 @@ func TestShouldEnqueueSquadLeaderOnComment_SkipsWhenMemberMentionsAnyone(t *test } } +// TestShouldEnqueueSquadLeaderOnComment_SkipsAgentReplyToMemberRoutingMention +// pins MUL-2624: when a human explicitly @-mentions an agent (which already +// suppresses the leader on the mention comment), the agent's reply to that +// comment must ALSO skip the leader. Otherwise the leader would re-@ the user +// on every agent reply in an explicit one-on-one exchange. +// +// The decision is keyed on the agent reply's parent comment (the comment the +// CreateComment handler stores as parent_id). For comment-triggered agent +// tasks this is the trigger comment by construction — the handler refuses +// any parent_id mismatch. The cases below enumerate which parent shapes do +// and do not trip the new rule. +func TestShouldEnqueueSquadLeaderOnComment_SkipsAgentReplyToMemberRoutingMention(t *testing.T) { + if testHandler == nil || testPool == nil { + t.Skip("database not available") + } + fx := newSquadCommentTriggerFixture(t) + ctx := context.Background() + issueID := uuidToString(fx.Issue.ID) + + t.Cleanup(func() { + testPool.Exec(context.Background(), `DELETE FROM comment WHERE issue_id = $1`, issueID) + }) + + // Helper: insert a comment row directly so we don't go through the full + // CreateComment side effects (we're unit-testing the helper here; the + // downstream call-site behavior is covered by the full-stack test below). + insertComment := func(authorType, authorID, content string) db.Comment { + t.Helper() + var cid string + if err := testPool.QueryRow(ctx, ` + INSERT INTO comment (issue_id, workspace_id, author_type, author_id, content, type) + VALUES ($1, $2, $3, $4, $5, 'comment') + RETURNING id + `, issueID, uuidToString(fx.Issue.WorkspaceID), authorType, authorID, content).Scan(&cid); err != nil { + t.Fatalf("insert comment: %v", err) + } + c, err := testHandler.Queries.GetComment(ctx, util.MustParseUUID(cid)) + if err != nil { + t.Fatalf("load comment: %v", err) + } + return c + } + + memberRouting := insertComment("member", testUserID, + "[@Other](mention://agent/"+fx.OtherID+") please take this") + memberPlain := insertComment("member", testUserID, "any update?") + agentPlain := insertComment("agent", fx.OtherID, "on it") + + cases := []struct { + name string + content string + authorType string + authorID string + parent *db.Comment + want bool + description string + }{ + { + name: "agent reply to member routing mention skips leader", + content: "here is the result", + authorType: "agent", + authorID: fx.OtherID, + parent: &memberRouting, + want: false, + description: "MUL-2624: human @ agent → that agent's reply must not wake the leader", + }, + { + name: "agent reply to plain member comment still wakes leader", + content: "here is the result", + authorType: "agent", + authorID: fx.OtherID, + parent: &memberPlain, + want: true, + description: "no routing mention upstream → leader still coordinates as today", + }, + { + name: "agent reply to another agent's comment still wakes leader", + content: "ack", + authorType: "agent", + authorID: fx.OtherID, + parent: &agentPlain, + want: true, + description: "MUL-2624 narrows to member-authored parents only; agent-to-agent stays under leader", + }, + { + name: "top-level agent comment still wakes leader", + content: "starting work", + authorType: "agent", + authorID: fx.OtherID, + parent: nil, + want: true, + description: "no parent → no upstream routing signal to inherit", + }, + { + name: "member reply does not inherit own previous routing mention", + content: "any update?", + authorType: "member", + authorID: testUserID, + parent: &memberRouting, + want: true, + description: "member-authored comments still inspect their own body only — MUL-2624 only changes the agent branch", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := testHandler.shouldEnqueueSquadLeaderOnComment(ctx, fx.Issue, tc.content, tc.authorType, tc.authorID, tc.parent) + if got != tc.want { + t.Fatalf("%s\n content=%q author=%s/%s parent=%+v\n got=%v want=%v", + tc.description, tc.content, tc.authorType, tc.authorID, tc.parent, got, tc.want) + } + }) + } +} + +// TestCreateComment_AgentReplyToMemberRoutingMentionSkipsLeader is the +// full-stack regression for MUL-2624. A member @-mentions a non-leader agent; +// when that agent replies (via the agent HTTP path, parent_id = trigger +// comment), the squad leader must NOT be re-enqueued. +func TestCreateComment_AgentReplyToMemberRoutingMentionSkipsLeader(t *testing.T) { + if testHandler == nil || testPool == nil { + t.Skip("database not available") + } + ctx := context.Background() + fx := newSquadCommentTriggerFixture(t) + issueID := uuidToString(fx.Issue.ID) + + t.Cleanup(func() { + testPool.Exec(context.Background(), `DELETE FROM agent_task_queue WHERE issue_id = $1`, issueID) + testPool.Exec(context.Background(), `DELETE FROM comment WHERE issue_id = $1`, issueID) + }) + + countLeaderQueued := func() int { + var n int + if err := testPool.QueryRow(ctx, + `SELECT count(*) FROM agent_task_queue WHERE issue_id = $1 AND agent_id = $2 AND status = 'queued'`, + issueID, fx.LeaderID, + ).Scan(&n); err != nil { + t.Fatalf("count leader tasks: %v", err) + } + return n + } + + // 1. Member @-mentions OtherAgent. Leader stays out (existing rule); the + // mention path enqueues a task for OtherAgent — capture its ID so we + // can authenticate the agent reply with the same X-Task-ID the daemon + // would send. + w := httptest.NewRecorder() + r := newRequest("POST", "/api/issues/"+issueID+"/comments", map[string]any{ + "content": "[@Other](mention://agent/" + fx.OtherID + ") please handle", + }) + r = withURLParam(r, "id", issueID) + testHandler.CreateComment(w, r) + if w.Code != http.StatusCreated { + t.Fatalf("member CreateComment: expected 201, got %d: %s", w.Code, w.Body.String()) + } + var memberResp CommentResponse + if err := json.NewDecoder(w.Body).Decode(&memberResp); err != nil { + t.Fatalf("decode member comment: %v", err) + } + if got := countLeaderQueued(); got != 0 { + t.Fatalf("after member @Other: expected 0 leader tasks (skipped), got %d", got) + } + + // Promote the mention-path task to running so resolveActor accepts it as + // the agent's authentication for the reply (the handler also requires + // parent_id == task.TriggerCommentID, so the task must reference the + // member comment we just posted — which the mention path already does). + var otherTaskID string + if err := testPool.QueryRow(ctx, ` + UPDATE agent_task_queue SET status = 'running' + WHERE issue_id = $1 AND agent_id = $2 AND status = 'queued' + RETURNING id + `, issueID, fx.OtherID).Scan(&otherTaskID); err != nil { + t.Fatalf("promote other-agent task: %v", err) + } + + // 2. OtherAgent posts its reply via the agent HTTP path. Per the handler's + // parent_id enforcement, parent_id MUST equal task.TriggerCommentID — + // i.e. the member's @ comment from step 1. + w2 := httptest.NewRecorder() + r2 := newRequest("POST", "/api/issues/"+issueID+"/comments", map[string]any{ + "content": "done — here is the answer", + "parent_id": memberResp.ID, + }) + r2.Header.Set("X-Agent-ID", fx.OtherID) + r2.Header.Set("X-Task-ID", otherTaskID) + r2 = withURLParam(r2, "id", issueID) + testHandler.CreateComment(w2, r2) + if w2.Code != http.StatusCreated { + t.Fatalf("agent reply CreateComment: expected 201, got %d: %s", w2.Code, w2.Body.String()) + } + + // 3. The agent reply must NOT have woken the leader. + if got := countLeaderQueued(); got != 0 { + t.Fatalf("after agent reply to @-mention: expected 0 leader tasks (MUL-2624), got %d", got) + } +} + // TestShouldEnqueueSquadLeaderOnComment_LeaderSelfTriggerByRole covers the // role-aware self-trigger guard added for MUL-2218. The leader agent itself // should be skipped only when its last activity on the issue was a leader @@ -241,7 +440,7 @@ func TestShouldEnqueueSquadLeaderOnComment_LeaderSelfTriggerByRole(t *testing.T) t.Run("no prior task wakes leader (fresh external trigger)", func(t *testing.T) { clearTasks() - if got := testHandler.shouldEnqueueSquadLeaderOnComment(ctx, fx.Issue, "noted", "agent", fx.LeaderID); !got { + if got := testHandler.shouldEnqueueSquadLeaderOnComment(ctx, fx.Issue, "noted", "agent", fx.LeaderID, nil); !got { t.Fatalf("no prior task: expected leader to be enqueued, got skip") } }) @@ -249,7 +448,7 @@ func TestShouldEnqueueSquadLeaderOnComment_LeaderSelfTriggerByRole(t *testing.T) t.Run("prior leader task suppresses self-trigger", func(t *testing.T) { clearTasks() insertTask(true, "completed") - if got := testHandler.shouldEnqueueSquadLeaderOnComment(ctx, fx.Issue, "noted", "agent", fx.LeaderID); got { + if got := testHandler.shouldEnqueueSquadLeaderOnComment(ctx, fx.Issue, "noted", "agent", fx.LeaderID, nil); got { t.Fatalf("after leader task: expected skip (anti-loop), got enqueue") } }) @@ -257,7 +456,7 @@ func TestShouldEnqueueSquadLeaderOnComment_LeaderSelfTriggerByRole(t *testing.T) t.Run("prior worker task still wakes leader (dual-role agent)", func(t *testing.T) { clearTasks() insertTask(false, "completed") - if got := testHandler.shouldEnqueueSquadLeaderOnComment(ctx, fx.Issue, "result", "agent", fx.LeaderID); !got { + if got := testHandler.shouldEnqueueSquadLeaderOnComment(ctx, fx.Issue, "result", "agent", fx.LeaderID, nil); !got { t.Fatalf("after worker task: expected leader to be enqueued (MUL-2218), got skip") } }) @@ -266,7 +465,7 @@ func TestShouldEnqueueSquadLeaderOnComment_LeaderSelfTriggerByRole(t *testing.T) clearTasks() insertTask(true, "completed") // older leader task insertTask(false, "completed") // newer worker task - if got := testHandler.shouldEnqueueSquadLeaderOnComment(ctx, fx.Issue, "result", "agent", fx.LeaderID); !got { + if got := testHandler.shouldEnqueueSquadLeaderOnComment(ctx, fx.Issue, "result", "agent", fx.LeaderID, nil); !got { t.Fatalf("latest task is worker: expected leader to be enqueued, got skip") } })