From 02c9d9f0b0eab946fcfe68c9699eba338d9f7f93 Mon Sep 17 00:00:00 2001 From: Jiayuan Date: Thu, 2 Apr 2026 00:33:21 +0800 Subject: [PATCH] fix(server): @all mentions should not trigger agent execution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @all is a broadcast to all workspace members — it should not trigger the assignee agent's on_comment. Previously @all was treated as "includes everyone" and allowed the trigger. Changes: - commentMentionsOthersButNotAssignee now checks HasMentionAll() early and returns true (suppress) when @all is present - Fix authRequestWithAgent test helper that was making a duplicate HTTP request (one as member, one as agent) Tests: 5 new @all unit test cases, 2 new @all integration test cases. --- .../comment_trigger_integration_test.go | 50 ++++++++++++------- server/internal/handler/comment.go | 9 ++-- server/internal/handler/trigger_test.go | 13 +++++ 3 files changed, 50 insertions(+), 22 deletions(-) diff --git a/server/cmd/server/comment_trigger_integration_test.go b/server/cmd/server/comment_trigger_integration_test.go index a353ea70..eb5cf248 100644 --- a/server/cmd/server/comment_trigger_integration_test.go +++ b/server/cmd/server/comment_trigger_integration_test.go @@ -1,6 +1,7 @@ package main import ( + "bytes" "context" "encoding/json" "fmt" @@ -13,14 +14,10 @@ import ( // causing the server to resolve the actor as an agent instead of a member. func authRequestWithAgent(t *testing.T, method, path string, body any, agentID string) *http.Response { t.Helper() - resp := authRequest(t, method, path, body) - // We can't add headers after authRequest, so we build it manually: - resp.Body.Close() - var bodyReader io.Reader if body != nil { b, _ := json.Marshal(body) - bodyReader = &readCloserWrapper{data: b} + bodyReader = bytes.NewReader(b) } req, err := http.NewRequest(method, testServer.URL+path, bodyReader) if err != nil { @@ -38,20 +35,6 @@ func authRequestWithAgent(t *testing.T, method, path string, body any, agentID s return r } -type readCloserWrapper struct { - data []byte - pos int -} - -func (r *readCloserWrapper) Read(p []byte) (int, error) { - if r.pos >= len(r.data) { - return 0, io.EOF - } - n := copy(p, r.data[r.pos:]) - r.pos += n - return n, nil -} - // countPendingTasks returns the number of queued/dispatched tasks for an issue. func countPendingTasks(t *testing.T, issueID string) int { t.Helper() @@ -249,6 +232,35 @@ func TestCommentTriggerOnComment(t *testing.T) { }) } +// TestCommentTriggerAtAllSuppression verifies that @all mentions do not +// trigger agent execution — @all is a broadcast, not a direct request. +func TestCommentTriggerAtAllSuppression(t *testing.T) { + agentID := getAgentID(t) + issueID := createIssueAssignedToAgent(t, "@all suppression test", agentID) + t.Cleanup(func() { + clearTasks(t, issueID) + resp := authRequest(t, "DELETE", "/api/issues/"+issueID, nil) + resp.Body.Close() + }) + + t.Run("top-level @all comment suppresses on_comment", func(t *testing.T) { + clearTasks(t, issueID) + postComment(t, issueID, "[@All](mention://all/all) heads up everyone", nil) + if n := countPendingTasks(t, issueID); n != 0 { + t.Errorf("expected 0 pending tasks (@all should not trigger agent), got %d", n) + } + }) + + t.Run("@all in agent thread suppresses on_comment", func(t *testing.T) { + clearTasks(t, issueID) + threadID := postCommentAsAgent(t, issueID, "Here is my analysis.", agentID, nil) + postComment(t, issueID, "[@All](mention://all/all) FYI for the team", strPtr(threadID)) + if n := countPendingTasks(t, issueID); n != 0 { + t.Errorf("expected 0 pending tasks (@all in agent thread), got %d", n) + } + }) +} + // TestCommentTriggerOnAssignNoStatusGate verifies that assigning an agent to // a non-todo issue still triggers the agent (status gate was removed). func TestCommentTriggerOnAssignNoStatusGate(t *testing.T) { diff --git a/server/internal/handler/comment.go b/server/internal/handler/comment.go index 9f10aed8..c131e45a 100644 --- a/server/internal/handler/comment.go +++ b/server/internal/handler/comment.go @@ -193,19 +193,22 @@ func (h *Handler) CreateComment(w http.ResponseWriter, r *http.Request) { // anyone but does NOT @mention the issue's assignee agent. This is used to // suppress the on_comment trigger when the user is directing their comment at // someone else (e.g. sharing results with a colleague, asking another agent). +// @all is treated as a broadcast — it suppresses the trigger because the user +// is announcing to everyone, not specifically requesting work from the agent. func (h *Handler) commentMentionsOthersButNotAssignee(content string, issue db.Issue) bool { mentions := util.ParseMentions(content) if len(mentions) == 0 { return false // No mentions — normal on_comment behavior } + // @all is a broadcast to all members — suppress agent trigger. + if util.HasMentionAll(mentions) { + return true + } if !issue.AssigneeID.Valid { return true // No assignee — mentions target others } assigneeID := uuidToString(issue.AssigneeID) for _, m := range mentions { - if m.IsMentionAll() { - return false // @all includes everyone — allow trigger - } if m.ID == assigneeID { return false // Assignee is mentioned — allow trigger } diff --git a/server/internal/handler/trigger_test.go b/server/internal/handler/trigger_test.go index 82db7d57..e2358b88 100644 --- a/server/internal/handler/trigger_test.go +++ b/server/internal/handler/trigger_test.go @@ -76,6 +76,16 @@ func TestCommentMentionsOthersButNotAssignee(t *testing.T) { content: fmt.Sprintf("[@Agent](mention://agent/%s) and [@Other](mention://agent/%s)", agentAssigneeID, otherAgentID), want: false, }, + { + name: "@all mention → suppress (broadcast, not directed at agent)", + content: "[@All](mention://all/all) heads up everyone", + want: true, + }, + { + name: "@all with assignee mention → suppress (@all takes precedence)", + content: fmt.Sprintf("[@All](mention://all/all) [@Agent](mention://agent/%s) fyi", agentAssigneeID), + want: true, + }, } for _, tt := range tests { @@ -203,6 +213,9 @@ func TestOnCommentTriggerDecision(t *testing.T) { {"reply member thread, no mention", memberParent, "agreed", false}, {"reply member thread, mention other member", memberParent, fmt.Sprintf("[@Bob](mention://member/%s) ok", memberID), false}, {"reply member thread, mention assignee", memberParent, fmt.Sprintf("[@Agent](mention://agent/%s) help", agentAssigneeID), true}, + {"top-level, @all broadcast", nil, "[@All](mention://all/all) heads up team", false}, + {"reply agent thread, @all broadcast", agentParent, "[@All](mention://all/all) update for everyone", false}, + {"reply member thread, @all broadcast", memberParent, "[@All](mention://all/all) fyi", false}, } for _, tt := range tests {