fix(events): Render each linked Event person once

packages/junior-evals/evals/coding/subscriptions.eval.ts

@@ -1,6 +1,9 @@
 import { describe, expect } from "vitest";
 import { githubWebhook, slackMention } from "@junior-evals/fixture/inputs";
-import { insertWatch } from "@junior-evals/fixture/insert";
+import {
+  insertLinkedGitHubIdentity,
+  insertWatch,
+} from "@junior-evals/fixture/insert";
 import { rubric, RubricJudge } from "@junior-evals/fixture/judge";
 import { sendDuringFirstModelRequest } from "@junior-evals/fixture/progress";
 import { completedToolCalls, toolOutput } from "@junior-evals/fixture/results";
@@ -293,6 +296,54 @@ describe("Watches", () => {
     expect(delivery.replies).toHaveLength(1);
   });
 
+  test("when the person who asked reviews the PR, address them once as you", async ({
+    run,
+  }) => {
+    const conversation = await run(
+      slackMention(`Let me know here when ${pullRequest} lands.`),
+    );
+    // The Slack person who asked also reviews on GitHub as this login.
+    await insertLinkedGitHubIdentity({ login: "octo-requester" });
+    await insertWatch({
+      conversation,
+      events: ["pull_request.review.changes_requested"],
+      identifier: pullRequest,
+      intent:
+        "When a reviewer submits a review, notify the person who asked for the pull request by @-mentioning their Slack user ID from the originating thread, with the reviewer, the review outcome, and the pull request link. Do not change the pull request.",
+      label: `GitHub PR ${pullRequest}`,
+      resourceType: "pull_request",
+    });
+
+    const delivery = await conversation.continue(
+      githubWebhook("pull_request_review", {
+        action: "submitted",
+        pull_request: { number: 691 },
+        repository,
+        review: {
+          body: "Please add a regression test.",
+          state: "changes_requested",
+          user: { login: "octo-requester" },
+        },
+      }),
+    );
+    await expect(delivery).toSatisfyJudge(
+      RubricJudge,
+      rubric({
+        pass: [
+          `The reply says changes were requested on GitHub PR ${pullRequest}.`,
+          "The reply treats the person it notifies as the reviewer, for example by saying that you requested changes.",
+        ],
+        fail: [
+          "Do not present the reviewer as a different person from the person the reply notifies.",
+          'Do not name "octo-requester" as the reviewer.',
+        ],
+      }),
+    );
+
+    expect(delivery.replies).toHaveLength(1);
+    expect(delivery.replies[0]?.text).not.toMatch(/octo-requester/i);
+  });
+
   test("when directly mentioned during a watch delivery, answer the mention in its own turn", async ({
     run,
   }) => {

packages/junior-evals/src/fixture/insert.ts

@@ -16,7 +16,7 @@ import { createSlackDestination } from "@/chat/destination";
 import { createEventAutomation } from "@/chat/event-automations/store";
 import { createWatch } from "@/chat/events/store";
 import type { EventAutomation } from "@/chat/event-automations/types";
-import { upsertIdentity } from "@/chat/identities/sql";
+import { upsertIdentity, upsertLinkedIdentity } from "@/chat/identities/sql";
 import { createPluginEmbedder } from "@/chat/plugins/model";
 import {
   insertReviewItem as insertStoredReviewItem,
@@ -106,6 +106,26 @@ async function insertSlackIdentity(author: Required<SlackAuthor>) {
   });
 }
 
+/**
+ * Link a GitHub login to a Slack person, as account linking does, so Events
+ * that name the login resolve to that person.
+ */
+export async function insertLinkedGitHubIdentity(args: {
+  author?: SlackAuthor;
+  login: string;
+}): Promise<void> {
+  const slack = await insertSlackIdentity(resolveAuthor(args.author));
+  if (!slack.userId) {
+    throw new Error("Slack identity has no Junior user to link");
+  }
+  await upsertLinkedIdentity(getSqlExecutor(), slack.userId, {
+    handle: args.login,
+    kind: "user",
+    provider: "github",
+    providerSubjectId: `github-user-${args.login}`,
+  });
+}
+
 /**
  * Store the Slack thread where a person created an automation. Like the live
  * Slack event of that thread, it records the channel as public. Automation

packages/junior/src/chat/task-input.ts

@@ -55,26 +55,81 @@ export interface EventDoneBy {
   handle?: string;
 }
 
+/** A linked Event doer, merged across the Events of one task message. */
+interface Doer {
+  mention: string;
+  /** What the person did, such as "did Event 2". */
+  did: string;
+  /** Provider handles that the Event data uses for the same person. */
+  handles: string[];
+}
+
+function quoted(values: readonly string[]): string {
+  return values.map((value) => `"${value}"`).join(" or ");
+}
+
 /**
- * Connect the mention to the provider handle in the Event text. Without this,
- * the agent can mention a person and then name them again by handle as if
- * they were someone else. The "you" rule also applies when the doer is not in
- * People involved, because instructions can still ask the agent to mention
- * them, such as a requester who reviews their own pull request.
+ * Render each linked person once. A doer in People involved gets a note in
+ * that list. Other doers get a Done by line. Without one line per person, the
+ * agent can mention a person and then name them again by handle as if they
+ * were someone else. The "you" rule covers doers outside People involved too,
+ * because instructions can still ask to mention them, such as a requester who
+ * reviews their own pull request.
  */
-function doneByLine(doneBy: EventDoneBy, people: readonly string[]): string {
-  const mention = oneLine(doneBy.mention);
-  const handle = doneBy.handle ? oneLine(doneBy.handle) : undefined;
-  const asYou = `as "you"${handle ? `, and do not also call them "${handle}"` : ""}.`;
+function peopleLines(
+  people: readonly string[],
+  doers: readonly Doer[],
+): string[] {
+  const label = (doer: Doer) =>
+    `${doer.mention} (${[
+      doer.did,
+      ...(doer.handles.length > 0
+        ? [`called ${quoted(doer.handles)} in the Event data`]
+        : []),
+    ].join("; ")})`;
+  const doerByMention = new Map(doers.map((doer) => [doer.mention, doer]));
+  const involved = people.map((person) => {
+    const doer = doerByMention.get(person);
+    return doer ? label(doer) : person;
+  });
   return [
-    `Done by: ${mention}. This person did what this Event describes.`,
-    ...(handle
-      ? [`The Event calls them "${handle}". That is the same person.`]
-      : []),
-    people.includes(mention)
-      ? `They are also in People involved. Write to them ${asYou}`
-      : `If you mention them, write to them ${asYou}`,
-  ].join(" ");
+    ...(involved.length > 0 ? [`People involved: ${involved.join(", ")}`] : []),
+    ...doers
+      .filter((doer) => !people.includes(doer.mention))
+      .map((doer) => `Done by: ${label(doer)}`),
+    ...doers.map(
+      (doer) =>
+        `If you mention ${doer.mention}, write to them as "you"${
+          doer.handles.length > 0
+            ? `, and do not also call them ${quoted(doer.handles)}`
+            : ""
+        }.`,
+    ),
+  ];
+}
+
+/** Merge the doers of numbered Events, in first-Event order. */
+function batchDoers(
+  events: readonly { doneBy?: EventDoneBy; number: number }[],
+): Doer[] {
+  const merged = new Map<string, { handles: string[]; numbers: number[] }>();
+  for (const { doneBy, number } of events) {
+    if (!doneBy) continue;
+    const mention = oneLine(doneBy.mention);
+    const entry = merged.get(mention) ?? { handles: [], numbers: [] };
+    entry.numbers.push(number);
+    const handle = doneBy.handle ? oneLine(doneBy.handle) : undefined;
+    if (handle && !entry.handles.includes(handle)) entry.handles.push(handle);
+    merged.set(mention, entry);
+  }
+  return [...merged].map(([mention, { handles, numbers }]) => ({
+    mention,
+    did:
+      numbers.length === 1
+        ? `did Event ${numbers[0]}`
+        : `did Events ${numbers.slice(0, -1).join(", ")} and ${numbers.at(-1)}`,
+    handles,
+  }));
 }
 
 /**
@@ -128,8 +183,18 @@ export function renderTaskInput(args: {
           `Created by: ${oneLine(creator)}. This person is the Automation creator. "The Automation creator", "me", and "my" in the instructions mean this person. Where the instructions refer to this person, write this mention in the message. If they do not, do not mention this person.`,
         ]
       : []),
-    ...(people.length > 0 ? [`People involved: ${people.join(", ")}`] : []),
-    ...(args.doneBy ? [doneByLine(args.doneBy, people)] : []),
+    ...peopleLines(
+      people,
+      args.doneBy
+        ? [
+            {
+              mention: oneLine(args.doneBy.mention),
+              did: "did what this Event describes",
+              handles: args.doneBy.handle ? [oneLine(args.doneBy.handle)] : [],
+            },
+          ]
+        : [],
+    ),
     `Instructions: ${creator ? mentionCreator(instructions, oneLine(creator)) : instructions}`,
   ];
 
@@ -195,14 +260,7 @@ export interface TaskBatchGroup {
   events: readonly TaskBatchEvent[];
 }
 
-function pushEventFacts(
-  lines: string[],
-  event: TaskBatchEvent,
-  people: readonly string[],
-): void {
-  if (event.doneBy) {
-    lines.push(doneByLine(event.doneBy, people));
-  }
+function pushEventFacts(lines: string[], event: TaskBatchEvent): void {
   const trustedSummary = event.trustedSummary?.trim();
   if (trustedSummary) {
     lines.push(`Trusted summary: ${trustedSummary}`);
@@ -258,10 +316,15 @@ export function renderTaskBatchInput(args: {
     const people = group.people
       .map((person) => oneLine(person))
       .filter(Boolean);
+    const numbered = group.events.map((event, index) => ({
+      ...event,
+      number: number + index + 1,
+    }));
+    number += group.events.length;
     lines.push(
       "",
       ...(about ? [`About: ${oneLine(about)}`] : []),
-      ...(people.length > 0 ? [`People involved: ${people.join(", ")}`] : []),
+      ...peopleLines(people, batchDoers(numbered)),
       `Instructions: ${instructions}`,
     );
 
@@ -281,10 +344,9 @@ export function renderTaskBatchInput(args: {
       );
     }
 
-    for (const event of group.events) {
-      number += 1;
-      lines.push("", `Event ${number}: ${event.eventType}`);
-      pushEventFacts(lines, event, people);
+    for (const event of numbered) {
+      lines.push("", `Event ${event.number}: ${event.eventType}`);
+      pushEventFacts(lines, event);
     }
   }
 

packages/junior/tests/component/events/watch-people.test.ts

@@ -141,13 +141,17 @@ describe("Watch Event people", () => {
     const [succeeded, failed] = (work?.messages ?? []).map(
       (message) => message.input.text,
     );
-    expect(succeeded).toContain("People involved: <@U123>, <@U456>\n");
+    // The doer is in People involved, so they are listed once, with the
+    // handle that the Event uses for them.
     expect(succeeded).toContain(
-      'Done by: <@U123>. This person did what this Event describes. The Event calls them "REQUESTER". That is the same person. They are also in People involved.',
+      'People involved: <@U123> (did what this Event describes; called "REQUESTER" in the Event data), <@U456>\n',
     );
+    expect(succeeded).not.toContain("Done by:");
     expect(succeeded).toContain("Tell the people involved.");
     // Watch guidance applies only to its own event type.
-    expect(failed).toContain("People involved: <@U123>, <@U456>\n");
+    expect(failed).toContain(
+      'People involved: <@U123> (did what this Event describes; called "REQUESTER" in the Event data), <@U456>\n',
+    );
     expect(failed).not.toContain("Tell the people involved.");
   });
 });

packages/junior/tests/integration/event-wake-delay.test.ts

@@ -284,15 +284,16 @@ describe("event wake delay", () => {
     expect(text).toContain(
       [
         `About: ${prWatch.label}`,
-        "People involved: <@U123>",
+        'People involved: <@U123> (did Event 1; called "dcramer" in the Event data)',
+        'If you mention <@U123>, write to them as "you", and do not also call them "dcramer".',
         `Instructions: ${prWatch.intent}`,
       ].join("\n"),
     );
     expect(text).toContain(
       "- pull_request.review_comment.created: Reply on the thread.",
     );
-    expect(text).toMatch(
-      /Event 1: pull_request\.review\.approved\nDone by: <@U123>\. [^\n]*"dcramer"[^\n]*\nTrusted summary: Approved by dcramer\.\n\nEvent 2: pull_request\.review_comment\.created/,
+    expect(text).toContain(
+      "Event 1: pull_request.review.approved\nTrusted summary: Approved by dcramer.\n\nEvent 2: pull_request.review_comment.created",
     );
     expect(text).toContain(
       [

packages/junior/tests/unit/chat/task-input.test.ts

@@ -37,8 +37,8 @@ describe("renderTaskInput", () => {
 
       About: GitHub PR getsentry/junior#691
       Created by: <@U123>. This person is the Automation creator. "The Automation creator", "me", and "my" in the instructions mean this person. Where the instructions refer to this person, write this mention in the message. If they do not, do not mention this person.
-      People involved: <@U456>
-      Done by: <@U456>. This person did what this Event describes. The Event calls them "octocat". That is the same person. They are also in People involved. Write to them as "you", and do not also call them "octocat".
+      People involved: <@U456> (did what this Event describes; called "octocat" in the Event data)
+      If you mention <@U456>, write to them as "you", and do not also call them "octocat".
       Instructions: Fix failed checks on this PR.
 
       Trusted summary: CI failed on workflow test.
@@ -65,7 +65,11 @@ describe("renderTaskInput", () => {
     });
 
     expect(text).toContain(
-      'Done by: <@U456>. This person did what this Event describes. The Event calls them "octocat". That is the same person. If you mention them, write to them as "you", and do not also call them "octocat".\n',
+      [
+        'Done by: <@U456> (did what this Event describes; called "octocat" in the Event data)',
+        'If you mention <@U456>, write to them as "you", and do not also call them "octocat".',
+        "Instructions:",
+      ].join("\n"),
     );
   });
 
@@ -146,7 +150,8 @@ describe("renderTaskBatchInput", () => {
       You received 4 events.
 
       About: GitHub PR getsentry/junior#2245
-      People involved: <@U123>
+      People involved: <@U123> (did Event 1; called "dcramer" in the Event data)
+      If you mention <@U123>, write to them as "you", and do not also call them "dcramer".
       Instructions: Maintain this pull request.
 
       Additional guidance:
@@ -155,7 +160,6 @@ describe("renderTaskBatchInput", () => {
       - pull_request.review_comment.created: Resolve the thread after a fix.
 
       Event 1: pull_request.review.approved
-      Done by: <@U123>. This person did what this Event describes. The Event calls them "dcramer". That is the same person. They are also in People involved. Write to them as "you", and do not also call them "dcramer".
       Trusted summary: Approved by dcramer.
 
       Event 2: pull_request.review_comment.created