diff --git a/server/cmd/server/notification_listeners.go b/server/cmd/server/notification_listeners.go index 3133ddf2a..5761ee7a1 100644 --- a/server/cmd/server/notification_listeners.go +++ b/server/cmd/server/notification_listeners.go @@ -755,20 +755,34 @@ func registerNotificationListeners(bus *events.Bus, queries *db.Queries) { // The comment payload can come as handler.CommentResponse from the // HTTP handler, or as map[string]any from the agent comment path in // task.go. Handle both. - var issueID, commentID, commentContent string + var issueID, commentID, commentContent, authorType string switch c := payload["comment"].(type) { case handler.CommentResponse: issueID = c.IssueID commentID = c.ID commentContent = c.Content + authorType = c.AuthorType case map[string]any: issueID, _ = c["issue_id"].(string) commentID, _ = c["id"].(string) commentContent, _ = c["content"].(string) + authorType, _ = c["author_type"].(string) default: return } + // Platform-authored system comments (MUL-2538 child-done parent + // notify) must NOT create inbox rows or parse mentions from their + // body — the comment is a controlled platform signal, not a human + // commenter. Mention parsing is the dangerous bit: if the body + // transcluded a child title containing `mention://member/`, + // the parent's assignee inbox would light up via the generic path. + // Skip the listener entirely; the WS broadcast still delivers the + // comment to the issue timeline. + if authorType == "system" { + return + } + issueTitle, _ := payload["issue_title"].(string) issueStatus, _ := payload["issue_status"].(string) diff --git a/server/cmd/server/notification_listeners_test.go b/server/cmd/server/notification_listeners_test.go index b63539c3f..3c6fe2815 100644 --- a/server/cmd/server/notification_listeners_test.go +++ b/server/cmd/server/notification_listeners_test.go @@ -388,6 +388,105 @@ func TestNotification_CommentCreated(t *testing.T) { } } +// TestNotification_SystemCommentSkipsInboxAndMentions guards the MUL-2538 +// must-fix: a comment with author_type='system' (the platform-generated +// child-done parent notify) must NOT create any inbox rows for parent +// subscribers and must NOT spawn mention-inbox rows even if the body string +// contains markdown mentions. The reviewer's concern was that a child title +// containing `mention://member/` would silently light up that member's +// inbox once the title was transcluded into the system comment body — +// because the generic comment:created listener treated all comments +// identically. The fix is to gate at author_type='system'. +func TestNotification_SystemCommentSkipsInboxAndMentions(t *testing.T) { + queries := db.New(testPool) + bus := newNotificationBus(t, queries) + + // Subscriber on the issue who would normally receive new_comment. + subEmail := "notif-system-comment-sub@multica.ai" + subID := createTestUser(t, subEmail) + t.Cleanup(func() { cleanupTestUser(t, subEmail) }) + + // A second member whose UUID we will smuggle into the system-comment + // body as a fake mention to prove the listener does not parse it. + targetEmail := "notif-system-comment-target@multica.ai" + targetID := createTestUser(t, targetEmail) + t.Cleanup(func() { cleanupTestUser(t, targetEmail) }) + + issueID := createTestIssue(t, testWorkspaceID, testUserID) + t.Cleanup(func() { + cleanupInboxForIssue(t, issueID) + cleanupTestIssue(t, issueID) + }) + + addTestSubscriber(t, issueID, "member", subID, "manual") + + // Publish a system-authored comment that transcludes a member mention + // in the body — the exact attack vector the reviewer flagged. If the + // generic listener path runs, the new_comment row will fire for `sub` + // and the mention path will fire for `target`. + bus.Publish(events.Event{ + Type: protocol.EventCommentCreated, + WorkspaceID: testWorkspaceID, + ActorType: "system", + ActorID: "", + Payload: map[string]any{ + "comment": handler.CommentResponse{ + ID: "00000000-0000-0000-0000-000000000000", + IssueID: issueID, + AuthorType: "system", + AuthorID: "00000000-0000-0000-0000-000000000000", + Content: "Sub-issue done — see [@Target](mention://member/" + targetID + ").", + Type: "system", + }, + "issue_title": "system comment isolation", + "issue_status": "in_progress", + }, + }) + + if items := inboxItemsForRecipient(t, queries, subID); len(items) != 0 { + t.Errorf("expected 0 inbox rows for issue subscriber, got %d", len(items)) + } + if items := inboxItemsForRecipient(t, queries, targetID); len(items) != 0 { + t.Errorf("expected 0 inbox rows for smuggled @mention target, got %d", len(items)) + } +} + +// TestSubscriberSystemCommentDoesNotSubscribe guards the same boundary on +// the subscriber listener: a system-authored comment must NOT be treated as +// "a commenter joined the conversation." The CHECK constraint on +// issue_subscriber.user_type only permits ('member','agent'); without the +// author_type='system' early-return, AddIssueSubscriber would log a noisy +// constraint violation on every child-done event. +func TestSubscriberSystemCommentDoesNotSubscribe(t *testing.T) { + queries := db.New(testPool) + bus := events.New() + registerSubscriberListeners(bus, queries) + + issueID := createTestIssue(t, testWorkspaceID, testUserID) + t.Cleanup(func() { cleanupTestIssue(t, issueID) }) + + bus.Publish(events.Event{ + Type: protocol.EventCommentCreated, + WorkspaceID: testWorkspaceID, + ActorType: "system", + ActorID: "", + Payload: map[string]any{ + "comment": handler.CommentResponse{ + ID: "00000000-0000-0000-0000-000000000000", + IssueID: issueID, + AuthorType: "system", + AuthorID: "00000000-0000-0000-0000-000000000000", + Content: "platform notify", + Type: "system", + }, + }, + }) + + if count := subscriberCount(t, queries, issueID); count != 0 { + t.Fatalf("expected 0 subscribers after system comment, got %d", count) + } +} + // TestNotification_AssigneeChanged verifies the full assignee change flow: // - New assignee gets "issue_assigned" (Direct) // - Old assignee gets "unassigned" (Direct) diff --git a/server/cmd/server/subscriber_listeners.go b/server/cmd/server/subscriber_listeners.go index 5bfdffa83..3fe392d23 100644 --- a/server/cmd/server/subscriber_listeners.go +++ b/server/cmd/server/subscriber_listeners.go @@ -105,6 +105,16 @@ func registerSubscriberListeners(bus *events.Bus, queries *db.Queries) { return } + // Platform-authored system comments (MUL-2538 child-done parent notify) + // have author_type='system' and a zero UUID author. They must NOT + // add a subscriber row: issue_subscriber.user_type is constrained to + // ('member','agent'), and a "system" subscriber has no inbox to read + // anyway. Skip them at the side-effect boundary so the system event + // stays a pure WS broadcast for the timeline. + if authorType == "system" { + return + } + addSubscriber(bus, queries, e.WorkspaceID, issueID, authorType, authorID, "commenter") }) } diff --git a/server/internal/handler/github.go b/server/internal/handler/github.go index 41743137e..823d388f6 100644 --- a/server/internal/handler/github.go +++ b/server/internal/handler/github.go @@ -1051,6 +1051,15 @@ func (h *Handler) advanceIssueToDone(ctx context.Context, issue db.Issue, worksp slog.Warn("github: advance issue to done failed", "err", err) return } + + // Fire the platform parent-notification path on the same transition the + // HTTP UpdateIssue / BatchUpdateIssues paths use. A merged PR is one of + // the most common ways a sub-issue actually reaches `done`, and skipping + // it here would leave the parent silent for the dominant completion path. + // notifyParentOfChildDone re-checks every guard (prev != done, parent + // exists, parent not terminal), so calling it unconditionally is safe. + h.notifyParentOfChildDone(ctx, issue, updated) + prefix := h.getIssuePrefix(ctx, issue.WorkspaceID) resp := issueToResponse(updated, prefix) h.publish(protocol.EventIssueUpdated, workspaceID, "system", "", map[string]any{ diff --git a/server/internal/handler/github_test.go b/server/internal/handler/github_test.go index 313efc7dd..9d341f3e4 100644 --- a/server/internal/handler/github_test.go +++ b/server/internal/handler/github_test.go @@ -11,6 +11,7 @@ import ( "net/http" "net/http/httptest" "reflect" + "strings" "testing" "time" @@ -1336,3 +1337,140 @@ func TestGitHubInstallationBroadcastRedaction(t *testing.T) { t.Errorf("installation_id leaked into broadcast JSON: %s", string(raw)) } } + +// TestWebhook_MergedPR_ChildWithParent_NotifiesParent guards the MUL-2538 +// must-fix: a merged PR is the dominant path by which a sub-issue actually +// reaches `done`, and that path goes through advanceIssueToDone — not the +// HTTP UpdateIssue / BatchUpdateIssues handlers that originally wired up +// notifyParentOfChildDone. Without the helper call inside advanceIssueToDone, +// the parent receives nothing when a child is closed by merging its PR. +// This test fires a `pull_request closed merged` webhook against a child +// issue and verifies the parent gets exactly one platform-generated system +// comment with the child's real workspace identifier. +func TestWebhook_MergedPR_ChildWithParent_NotifiesParent(t *testing.T) { + if testHandler == nil { + t.Skip("handler test fixture not initialized (no DB?)") + } + ctx := context.Background() + secret := "merge-parent-notify-secret" + t.Setenv("GITHUB_WEBHOOK_SECRET", secret) + + // Create parent (open) + child (in_progress) pair. + w := httptest.NewRecorder() + req := newRequest("POST", "/api/issues?workspace_id="+testWorkspaceID, map[string]any{ + "title": "PR-merge parent " + time.Now().Format(time.RFC3339Nano), + "status": "in_progress", + }) + testHandler.CreateIssue(w, req) + if w.Code != http.StatusCreated { + t.Fatalf("CreateIssue parent: %d %s", w.Code, w.Body.String()) + } + var parent IssueResponse + json.NewDecoder(w.Body).Decode(&parent) + + w = httptest.NewRecorder() + req = newRequest("POST", "/api/issues?workspace_id="+testWorkspaceID, map[string]any{ + "title": "PR-merge child " + time.Now().Format(time.RFC3339Nano), + "status": "in_progress", + "parent_issue_id": parent.ID, + }) + testHandler.CreateIssue(w, req) + if w.Code != http.StatusCreated { + t.Fatalf("CreateIssue child: %d %s", w.Code, w.Body.String()) + } + var child IssueResponse + json.NewDecoder(w.Body).Decode(&child) + + t.Cleanup(func() { + testPool.Exec(ctx, `DELETE FROM issue_pull_request WHERE issue_id IN ($1, $2)`, child.ID, parent.ID) + testPool.Exec(ctx, `DELETE FROM github_pull_request WHERE workspace_id = $1`, testWorkspaceID) + testPool.Exec(ctx, `DELETE FROM github_installation WHERE workspace_id = $1`, testWorkspaceID) + testPool.Exec(ctx, `DELETE FROM activity_log WHERE issue_id IN ($1, $2)`, child.ID, parent.ID) + testPool.Exec(ctx, `DELETE FROM comment WHERE issue_id IN ($1, $2)`, child.ID, parent.ID) + testPool.Exec(ctx, `DELETE FROM issue WHERE id = $1`, child.ID) + testPool.Exec(ctx, `DELETE FROM issue WHERE id = $1`, parent.ID) + }) + + const installationID int64 = 88990011 + if _, err := testHandler.Queries.CreateGitHubInstallation(ctx, db.CreateGitHubInstallationParams{ + WorkspaceID: parseUUID(testWorkspaceID), + InstallationID: installationID, + AccountLogin: "merge-parent-acct", + AccountType: "User", + }); err != nil { + t.Fatalf("CreateGitHubInstallation: %v", err) + } + + body, _ := json.Marshal(map[string]any{ + "action": "closed", + "pull_request": map[string]any{ + "number": 4242, + "html_url": "https://github.com/acme/widget/pull/4242", + "title": "Fix " + child.Identifier, + "body": "", + "state": "closed", + "draft": false, + "merged": true, + "merged_at": "2026-04-29T00:00:00Z", + "closed_at": "2026-04-29T00:00:00Z", + "created_at": "2026-04-28T00:00:00Z", + "updated_at": "2026-04-29T00:00:00Z", + "head": map[string]any{"ref": "fix/child"}, + "user": map[string]any{"login": "octocat", "avatar_url": ""}, + }, + "repository": map[string]any{ + "name": "widget", + "owner": map[string]any{"login": "acme"}, + }, + "installation": map[string]any{"id": installationID}, + }) + mac := hmac.New(sha256.New, []byte(secret)) + mac.Write(body) + sig := "sha256=" + hex.EncodeToString(mac.Sum(nil)) + + w = httptest.NewRecorder() + req2 := httptest.NewRequest("POST", "/api/webhooks/github", bytes.NewReader(body)) + req2.Header.Set("X-GitHub-Event", "pull_request") + req2.Header.Set("X-Hub-Signature-256", sig) + testHandler.HandleGitHubWebhook(w, req2) + if w.Code != http.StatusAccepted { + t.Fatalf("webhook: expected 202, got %d (%s)", w.Code, w.Body.String()) + } + + // Child must now be done (sanity check — the existing path). + updatedChild, err := testHandler.Queries.GetIssue(ctx, parseUUID(child.ID)) + if err != nil { + t.Fatalf("GetIssue child: %v", err) + } + if updatedChild.Status != "done" { + t.Fatalf("expected child status 'done', got %q", updatedChild.Status) + } + + // Parent must have received exactly one platform-generated system comment. + var sysCount int + if err := testPool.QueryRow(ctx, + `SELECT count(*) FROM comment WHERE issue_id = $1 AND author_type = 'system'`, + parent.ID, + ).Scan(&sysCount); err != nil { + t.Fatalf("count system comments on parent: %v", err) + } + if sysCount != 1 { + t.Fatalf("expected 1 system comment on parent after PR-merge auto-done, got %d", sysCount) + } + + var content string + if err := testPool.QueryRow(ctx, + `SELECT content FROM comment WHERE issue_id = $1 AND author_type = 'system' LIMIT 1`, + parent.ID, + ).Scan(&content); err != nil { + t.Fatalf("read system comment: %v", err) + } + if !strings.Contains(content, child.Identifier) { + t.Errorf("system comment should reference child identifier %q, got: %s", child.Identifier, content) + } + for _, banned := range []string{"mention://agent/", "mention://member/", "mention://squad/"} { + if strings.Contains(content, banned) { + t.Errorf("system comment must not include %q mention, got: %s", banned, content) + } + } +}