fix(issues): isolate system comments + wire GH merge path (MUL-2538)

Addresses the two must-fix items from the PR #3055 second review:

1. The platform-generated `comment:created` event (author_type='system')
   was running through the generic comment listeners, which (a) tried to
   subscribe the zero-UUID author and (b) parsed @mentions from the body
   for inbox notifications. Both subscriber_listeners and
   notification_listeners now early-return on author_type='system' so the
   event becomes a pure WS broadcast for the timeline — no inbox rows,
   no transcluded-mention attack surface.

2. advanceIssueToDone (the GitHub merge auto-done path) only published
   issue:updated and skipped notifyParentOfChildDone, so a child closed
   via merged PR — the dominant completion path — left the parent
   silent. The helper is now invoked on the same prev/updated pair, with
   the existing guards (transition + parent state) protecting double-fire.

Tests:
- New cmd/server/notification_listeners_test:
  TestNotification_SystemCommentSkipsInboxAndMentions (parent subscribers
  and smuggled @mention targets stay quiet),
  TestSubscriberSystemCommentDoesNotSubscribe (zero-UUID never reaches
  AddIssueSubscriber).
- New internal/handler/github_test:
  TestWebhook_MergedPR_ChildWithParent_NotifiesParent fires a real
  pull_request closed-merged webhook against a child and asserts the
  parent receives exactly one safe system comment with the workspace's
  real identifier (no `mention://agent|member|squad` links).

Co-authored-by: multica-agent <github@multica.ai>

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/<uuid>`,
+		// 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)
 

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/<uuid>` 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)

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")
 	})
 }

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{

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)
+		}
+	}
+}