fix(notifications): exclude the comment actor so they never self-notify

plugins/rw-backend-module-notifications/src/CommentNotifier.test.ts

@@ -36,14 +36,28 @@ describe("CommentNotifier", () => {
 
     expect(send).toHaveBeenCalledTimes(1);
     const arg = send.mock.calls[0][0];
-    expect(arg.recipients).toEqual({ type: "entity", entityRef: ["group:default/team"] });
+    // The recipient is a group and the actor is forwarded as excludeEntityRef: this is what
+    // lets Backstage's resolver drop a group-owning actor from the expanded set (so a member
+    // commenting on their own entity is not self-notified).
+    expect(arg.recipients).toEqual({
+      type: "entity",
+      entityRef: ["group:default/team"],
+      excludeEntityRef: "user:default/alice",
+    });
     expect(arg.payload.title).toBe("Alice Smith commented on Setup Guide · My Service");
     expect(arg.payload.description).toBe("please review");
     expect(arg.payload.link).toBe("/catalog/default/component/site/docs/guide/setup#comment-c1");
     expect(arg.payload.scope).toBe("rw:comment:c1");
     expect(arg.payload.severity).toBe("normal");
   });
 
+  it("drops a payload with no actorRef without sending (exclusion would be a no-op)", async () => {
+    const send = jest.fn().mockResolvedValue(undefined);
+    const notifier = new CommentNotifier({ notifications: { send } as any, logger });
+    await notifier.handle(payload({ actorRef: "" }));
+    expect(send).not.toHaveBeenCalled();
+  });
+
   it("participants/created → '<actor> replied on <subject>'", async () => {
     const send = jest.fn().mockResolvedValue(undefined);
     const notifier = new CommentNotifier({ notifications: { send } as any, logger });

plugins/rw-backend-module-notifications/src/CommentNotifier.ts

@@ -4,10 +4,12 @@ import { NotificationService } from "@backstage/plugin-notifications-node";
 import { CommentEventPayload } from "@rwdocs/backstage-plugin-rw-common";
 
 /** Subscriber-side: turns a self-contained `rw.comments` event into a native notification.
- *  Pure presentation + delivery — no DB, no recipient resolution. The one catalog-path
- *  convention (`/catalog/<ns>/<kind>/<name>`) is isolated here: the backend can't resolve
- *  the frontend's catalog routeRef, so the app-relative link is composed from the entity ref
- *  by convention. Best-effort: catches + logs, always resolves. */
+ *  Presentation + delivery — no DB. The publisher emits raw owner/participant recipients (which may
+ *  include the actor); this is the single point of actor exclusion: the actor is forwarded as
+ *  `excludeEntityRef` so Backstage's resolver drops them after expanding groups. The one catalog-path
+ *  convention (`/catalog/<ns>/<kind>/<name>`) is isolated here: the backend can't resolve the
+ *  frontend's catalog routeRef, so the app-relative link is composed from the entity ref by
+ *  convention. Best-effort: catches + logs, always resolves. */
 export class CommentNotifier {
   private readonly notifications: NotificationService;
   private readonly logger: LoggerService;
@@ -26,7 +28,13 @@ export class CommentNotifier {
     }
     try {
       await this.notifications.send({
-        recipients: { type: "entity", entityRef: payload.recipients },
+        // Single point of actor exclusion (see class doc): the resolver drops the actor from
+        // the resolved recipients, including from an expanded group.
+        recipients: {
+          type: "entity",
+          entityRef: payload.recipients,
+          excludeEntityRef: payload.actorRef,
+        },
         payload: {
           title: this.title(payload),
           description: this.description(payload),
@@ -51,12 +59,16 @@ export class CommentNotifier {
 
   /** Defensive shape guard at the subscriber boundary: the module casts the raw event
    *  payload to `CommentEventPayload`, so a malformed or foreign event on the topic would
-   *  otherwise flow straight into `send`. Validates the fields this notifier relies on. */
+   *  otherwise flow straight into `send`. Validates the fields this notifier relies on —
+   *  including `actorRef`, the sole basis for actor exclusion (passed as `excludeEntityRef`);
+   *  without it the exclusion silently becomes a no-op and the actor self-notifies. */
   private isValid(payload: CommentEventPayload): boolean {
     return (
       !!payload &&
       (payload.kind === "created" || payload.kind === "resolved") &&
       typeof payload.rootId === "string" &&
+      typeof payload.actorRef === "string" &&
+      payload.actorRef.length > 0 &&
       Array.isArray(payload.recipients) &&
       payload.recipients.length > 0
     );

plugins/rw-backend-module-notifications/src/module.test.ts

@@ -101,6 +101,7 @@ describe("rw notifications module — real startTestBackend integration", () =>
     expect(send.mock.calls[0][0].recipients).toEqual({
       type: "entity",
       entityRef: ["group:default/team"],
+      excludeEntityRef: "user:default/alice",
     });
   });
 

plugins/rw-backend/src/comments/CommentEventPublisher.test.ts

@@ -77,6 +77,7 @@ describe("CommentEventPublisher", () => {
       kind: "created",
       audience: "owner",
       recipients: ["group:default/team"],
+      actorRef: "user:default/alice", // forwarded as excludeEntityRef by the notifier
       entityRef: "component:default/site",
       pageRef: "sec-1#setup",
       deepLinkSuffix: "/docs/guide/setup#comment-c1",
@@ -187,7 +188,43 @@ describe("CommentEventPublisher", () => {
     expect(published).toHaveLength(0);
   });
 
-  it("commenter-side: reply notifies prior participants minus the actor", async () => {
+  it("owner-side: still publishes when the section owner IS the actor (recipients=[actor], dropped at delivery)", async () => {
+    // Guards the refactor: the publisher no longer strips the actor from owner recipients, so an
+    // owner commenting on their own section still emits an owner-side event (recipients=[actor]);
+    // the notifier's excludeEntityRef drops them at delivery. If the literal-ref filter were
+    // re-added here, recipients would empty out and this event would silently stop publishing —
+    // re-opening the self-notify hole. (The other owner-side fixtures use owner=group != actor,
+    // so the deleted filter was a no-op in them and would NOT catch such a regression.)
+    const { events, published } = fakeEvents();
+    const sections = {
+      getSection: async () => ({
+        site_ref: "component:default/site",
+        section_ref: "sec-1",
+        section_path: "guide",
+        parent_section_ref: null,
+        entity_ref: "component:default/site",
+        entity_owner_ref: "user:default/alice",
+      }),
+    } as any;
+    const pub = new CommentEventPublisher({
+      events,
+      sections,
+      comments: { participantsOf: async () => [] } as any,
+      logger,
+      pages: fakePages(),
+    });
+
+    await pub.onCommentCreated(row({ parent_id: null }), "user:default/alice");
+
+    expect(published).toHaveLength(1);
+    expect(published[0].eventPayload).toMatchObject({
+      audience: "owner",
+      recipients: ["user:default/alice"],
+      actorRef: "user:default/alice",
+    });
+  });
+
+  it("commenter-side: reply recipients include all thread participants (actor not stripped here)", async () => {
     const { events, published } = fakeEvents();
     const sections = {
       getSection: async () => ({
@@ -221,14 +258,19 @@ describe("CommentEventPublisher", () => {
       audience: "participants",
       commentId: "c2",
       rootId: "c1",
-      recipients: ["user:default/alice"],
+      // The actor (bob) is NOT stripped here — recipients are the raw participant set; the
+      // notifier forwards the actor as excludeEntityRef so the resolver drops them at delivery.
+      recipients: ["user:default/alice", "user:default/bob"],
       // deeplink anchors on the thread root (c1), not the reply (c2),
       // so opening the notification lands on the whole thread.
       deepLinkSuffix: "/docs/guide/setup#comment-c1",
     });
   });
 
-  it("commenter-side: no publish when the actor is the only participant", async () => {
+  it("commenter-side: still publishes when the actor is the only participant (resolver drops them at delivery)", async () => {
+    // The publisher no longer short-circuits the actor-only case; it emits the raw participant
+    // set and the notifier's excludeEntityRef makes the resolver drop the actor, yielding no
+    // notification. (Slightly more work than the old early-exit, but a single exclusion path.)
     const { events, published } = fakeEvents();
     const comments = { participantsOf: async () => ["user:default/bob"] } as any;
     const pub = new CommentEventPublisher({
@@ -242,7 +284,8 @@ describe("CommentEventPublisher", () => {
       row({ id: "c2", parent_id: "c1", author_ref: "user:default/bob" }),
       "user:default/bob",
     );
-    expect(published).toHaveLength(0);
+    expect(published).toHaveLength(1);
+    expect(published[0].eventPayload).toMatchObject({ recipients: ["user:default/bob"] });
   });
 
   it("owner-side: actorName comes from author_profile in the row", async () => {
@@ -271,7 +314,7 @@ describe("CommentEventPublisher", () => {
     expect(published[0].eventPayload).toMatchObject({ actorName: "Alice Smith" });
   });
 
-  it("resolve: notifies all participants minus the resolver; uses passed-in actorName", async () => {
+  it("resolve: recipients include all participants (resolver not stripped here); uses passed-in actorName", async () => {
     const { events, published } = fakeEvents();
     const sections = { getSection: async () => undefined } as any; // degraded link OK
     const comments = {
@@ -295,7 +338,8 @@ describe("CommentEventPublisher", () => {
     expect(published[0].eventPayload).toMatchObject({
       kind: "resolved",
       audience: "participants",
-      recipients: ["user:default/alice"],
+      // resolver (bob) is not stripped here; excludeEntityRef drops them at delivery.
+      recipients: ["user:default/alice", "user:default/bob"],
       entityRef: null, // degraded: no section row
       actorName: "Bob Builder",
     });

plugins/rw-backend/src/comments/CommentEventPublisher.ts

@@ -16,10 +16,13 @@ import { authorFromRow } from "./author";
 import { CommentRow, subpathOf } from "./types";
 
 /** Publishes self-contained `rw.comments` domain events after a comment write commits.
- *  Owns recipient + deep-link resolution (it has the comments + sections tables) so the
- *  notifications module can stay a thin sender. Best-effort: every method catches and
- *  logs, and always resolves, so a publish failure can never affect the comment write.
- *  Callers invoke fire-and-forget. */
+ *  Owns recipient + deep-link resolution from the comments + sections tables, so the
+ *  notifications module stays a thin sender. `recipients` are the raw section owner / thread
+ *  participants and may include the actor; actor exclusion is done entirely subscriber-side via
+ *  `excludeEntityRef` (see CommentNotifier), which the recipient resolver applies after expanding
+ *  groups — the one place that also catches an actor who owns the section through a group.
+ *  Best-effort: every method catches and logs, and always resolves, so a publish failure can
+ *  never affect the comment write. Callers invoke fire-and-forget. */
 export class CommentEventPublisher {
   private readonly events: EventsService;
   private readonly sections: SectionsReader;
@@ -78,8 +81,8 @@ export class CommentEventPublisher {
   private async publishOwnerSide(row: CommentRow, actorRef: string): Promise<void> {
     const section = await this.sections.getSection(row.site_ref, row.section_ref);
     if (!section || !section.entity_owner_ref) return; // new/unowned section: inbox catches it
-    const recipients = [section.entity_owner_ref].filter((ref) => ref !== actorRef);
-    if (recipients.length === 0) return;
+    // Actor left in even when they own the section; excluded at delivery (see class doc).
+    const recipients = [section.entity_owner_ref];
     const subpath = subpathOf(row.page_ref);
     const viewerPath = joinNonEmpty([section.section_path, subpath], "/");
     const actorName = authorFromRow(row).name;
@@ -103,10 +106,10 @@ export class CommentEventPublisher {
     actorRef: string,
     resolvedActorName?: string,
   ): Promise<void> {
-    // participantsOf already returns distinct refs, so no extra dedup is needed here.
-    const participants = await this.comments.participantsOf(rootId);
-    const recipients = participants.filter((ref) => ref !== actorRef);
-    if (recipients.length === 0) return;
+    // participantsOf returns distinct refs (no extra dedup); the actor is left in and excluded
+    // at delivery (see class doc).
+    const recipients = await this.comments.participantsOf(rootId);
+    if (recipients.length === 0) return; // defensive: thread with no live participants (e.g. root deleted)
     // The section row provides entityRef (link target) + section_path (path prefix); degrade gracefully if absent: entityRef -> null (module emits no link), viewerPath -> bare subpath.
     const section = await this.sections.getSection(row.site_ref, row.section_ref);
     const subpath = subpathOf(row.page_ref);

plugins/rw-common/src/commentEvents.ts

@@ -13,8 +13,8 @@ export interface CommentEventPayload {
   siteRef: string;
   sectionRef: string;
   pageRef: string; // identifies the page within the section ("<sectionRef>#<subpath>")
-  actorRef: string; // already removed from `recipients`
-  recipients: string[]; // catalog entity refs, non-empty
+  actorRef: string; // the user who triggered the event; forwarded as excludeEntityRef so the resolver drops them (see CommentNotifier)
+  recipients: string[]; // catalog entity refs, non-empty; raw owner/participants — may include actorRef
   entityRef: string | null; // owning entity (sections.entity_ref); null = degraded link
   // prefix-free deep-link suffix, always "/docs/<viewerPath>#comment-<id>". Normal: viewerPath = section_path + "/" + subpath. Degraded (no section row): viewerPath = subpath only (section prefix omitted).
   deepLinkSuffix: string;