fix(review-desk): Close Review items that no agent can act on

packages/junior/src/chat/automations/edit.ts

@@ -8,6 +8,7 @@ import type { AutomationEditFields } from "./edit-schema";
 import type { TaskOutcome, User } from "@sentry/junior-plugin-api";
 import type { AutomationEdit, AutomationUpdate } from "@/api/schema/automation";
 import { getDb } from "@/chat/db";
+import { dismissOpenReviewItems } from "@/chat/review-items/store";
 import { resolveTaskOutcomes } from "@/chat/task-outcomes";
 import {
   deleteEventAutomation,
@@ -393,15 +394,31 @@ export async function deleteViewerTask(user: User, id: string): Promise<void> {
 
 /**
  * Delete one Automation that the caller may manage. The row stays for
- * history. Returns undefined when it was already deleted.
+ * history. No run can act on its open Review items after the delete, so the
+ * same transaction dismisses them. Returns undefined when it was already
+ * deleted.
  */
 export async function deleteAutomation(
   current: TaskCandidate,
 ): Promise<TaskCandidate | undefined> {
-  if (current.kind === "event") {
-    const task = await deleteEventAutomation(getDb(), current.task.id);
-    return task ? { ...current, task } : undefined;
-  }
-  const task = await deleteScheduledAutomation(getDb(), current.task.id);
-  return task ? { ...current, task } : undefined;
+  return await getDb().transaction(async (tx) => {
+    // The delete takes the lock of the Automation. It holds until this
+    // transaction ends, so the dismiss runs under it.
+    const dismissItems = () =>
+      dismissOpenReviewItems(tx, {
+        automationId: current.task.id,
+        nowMs: Date.now(),
+        reason: "Automation deleted",
+      });
+    if (current.kind === "event") {
+      const task = await deleteEventAutomation(tx, current.task.id);
+      if (!task) return undefined;
+      await dismissItems();
+      return { ...current, task };
+    }
+    const task = await deleteScheduledAutomation(tx, current.task.id);
+    if (!task) return undefined;
+    await dismissItems();
+    return { ...current, task };
+  });
 }

packages/junior/src/chat/conversations/sql/purge.ts

@@ -2,6 +2,7 @@ import { and, asc, eq, inArray, isNull, or, sql } from "drizzle-orm";
 import type { JuniorSqlDatabase } from "@/db/db";
 import type { JuniorDestinationVisibility } from "@/db/schema/destinations";
 import { requestAttachmentDeletion } from "@/chat/attachments/store";
+import { dismissOpenReviewItems } from "@/chat/review-items/store";
 import {
   juniorConversationEvents,
   juniorConversationBriefs,
@@ -119,6 +120,12 @@ export async function selectExpiredRoots(
         where bindings.parent_conversation_id = tree.conversation_id
            or bindings.child_conversation_id = tree.conversation_id
       )
+      or exists (
+        select 1 from junior_review_items review_items
+        where review_items.added_by_conversation_id = tree.conversation_id
+          and review_items.status = 'open'
+          and review_items.automation_id is null
+      )
       or (
         ${juniorDestinations.visibility} is distinct from 'public'
         and (
@@ -297,6 +304,12 @@ export async function purgeConversationTree(
                 inArray(juniorAgentBindings.childConversationId, ids),
               ),
             );
+          // No agent can act on an item after its Conversation is purged.
+          await dismissOpenReviewItems(executor.db(), {
+            addedByConversationIds: ids,
+            nowMs: args.nowMs,
+            reason: "Conversation purged",
+          });
           await requestAttachmentDeletion(executor, ids, args.nowMs);
           const scrubMetadata = args.retention
             ? !isPublic

packages/junior/src/chat/ingress/slack-webhook.ts

@@ -561,17 +561,16 @@ async function routeParsedMessage(args: {
     return;
   }
 
-  // Passive thread messages are not checked for the allowed domains yet.
-  if (
-    route === "mention" &&
-    !(await isSlackAuthorDomainAllowed({
+  const isAuthorDomainAllowed = () =>
+    isSlackAuthorDomainAllowed({
       allowedBotUserIds: args.allowedBotUserIds,
       allowedDomains: args.allowedDomains,
       state: args.state,
       teamId: args.installation.teamId,
       user: args.event.user,
-    }))
-  ) {
+    });
+  // Passive thread messages are not checked for the allowed domains yet.
+  if (route === "mention" && !(await isAuthorDomainAllowed())) {
     return;
   }
 
@@ -613,15 +612,17 @@ async function routeParsedMessage(args: {
       route,
       state: args.state,
     });
-  if (route !== "mention") {
-    await persist();
-    return;
-  }
   // A message from a person to Junior answers the open questions that the
-  // agent asked in this Conversation, like a click on their cards.
+  // agent asked in this Conversation, like a click on their cards. A reply in
+  // a subscribed thread starts a Turn too, so it answers in the same way. It
+  // skips the allowed domain check above. The close checks the domain only
+  // when an open item can match, so most replies need no profile lookup.
   const closeAnswered = async () => {
     if (message.author.isBot === true) return;
     await closeReviewItemsAnsweredByReply({
+      ...(route === "subscribed"
+        ? { canAnswer: isAuthorDomainAllowed }
+        : undefined),
       conversationId,
       getSlackAdapter: () => args.adapter,
       queue: args.queue,

packages/junior/src/chat/review-items/README.md

@@ -10,8 +10,22 @@ behavior.
 - A `notice` only tells a person something. An `action` asks for an answer.
   Its `request` (`userInputRequestSchema`) is a question and 1-4 fields.
 - An action item is the only way the agent asks a person for input.
-- The status is `open`, `done`, or `dismissed`. Only a person or an agent
-  closes an item. Nothing closes it on its own.
+- The status is `open`, `done`, or `dismissed`. An open item closes only in
+  one of these ways:
+  - A person answers it, marks a notice done, or dismisses it (see "Person
+    actions").
+  - A person replies in its thread (see "Reply in the thread").
+  - An agent closes it with `updateReviewItem`.
+  - Junior purges the Conversation that added it. The item becomes
+    `dismissed` with the reason `Conversation purged`. An item from an
+    Automation run stays open, because a later run can still change it.
+  - Someone deletes the Automation that added it. The item becomes
+    `dismissed` with the reason `Automation deleted`.
+
+  A purge and an Automation delete keep the row. They send no message and
+  do not change the Slack cards. A click on an old card tells the person
+  that the item is closed.
+
 - An assignee is a User or **Everyone**. Everyone is a flag on the item. It
   means that any person who can see the item can act on it. When the agent
   names no assignee, a new item goes to the User of the Actor, or else to the
@@ -103,9 +117,11 @@ parent with `reportBlocked`, and the parent adds the item.
 
 A person can answer an action item with a message instead of the card
 (`closeReviewItemsAnsweredByReply` in `actions.ts`). The message is a Slack
-mention or DM to Junior, or a dashboard message, in a Conversation. Junior
-saves the message first, so it always reaches the agent. Then it closes each
-item that matches all of these:
+mention or DM to Junior, a Slack reply without a mention in a thread that
+Junior follows (when the `passive-routing` feature is on), or a dashboard
+message, in a Conversation. A Slack author must pass the allowed domain
+check. Junior saves the message first, so it always reaches the agent. Then
+it closes each item that matches all of these:
 
 - The item is an open `action` item.
 - The agent added it in this Conversation (`addedByConversationId`). Items

packages/junior/src/chat/review-items/actions.ts

@@ -257,6 +257,11 @@ function messageAnswer(
  * a message that was sent does not go again.
  */
 export async function closeReviewItemsAnsweredByReply(args: {
+  /**
+   * Return false when the replier may not answer, such as a Slack author
+   * outside the allowed domains. It runs only when an item can match.
+   */
+  canAnswer?: () => Promise<boolean>;
   conversationId: string;
   getSlackAdapter?: () => SlackAdapter;
   nowMs?: number;
@@ -267,6 +272,7 @@ export async function closeReviewItemsAnsweredByReply(args: {
   const db = getDb();
   const items = await listReplyActionItems(db, args.conversationId);
   if (items.length === 0) return;
+  if (args.canAnswer && !(await args.canAnswer())) return;
   const user = await args.resolveUser();
   const nowMs = args.nowMs ?? Date.now();
   for (const item of items.filter((candidate) =>

packages/junior/src/chat/review-items/store.ts

@@ -400,6 +400,45 @@ export async function closeReviewItem(
   return updated.length > 0;
 }
 
+/**
+ * Dismiss the open Review items that an Automation or a set of
+ * Conversations added, when no agent can act on them any more. No person
+ * closes them. It sends no message and does not change the Slack cards. A
+ * close by Conversations skips the items of an Automation, because a later
+ * run of the Automation can still change them. An Automation delete closes
+ * those.
+ */
+export async function dismissOpenReviewItems(
+  db: JuniorDatabase,
+  input: { nowMs: number; reason: string } & (
+    | { addedByConversationIds: readonly string[] }
+    | { automationId: string }
+  ),
+): Promise<void> {
+  const now = new Date(input.nowMs);
+  await db
+    .update(juniorReviewItems)
+    .set({
+      closeReason: input.reason,
+      closedAt: now,
+      status: "dismissed",
+      updatedAt: now,
+    })
+    .where(
+      and(
+        eq(juniorReviewItems.status, "open"),
+        "automationId" in input
+          ? eq(juniorReviewItems.automationId, input.automationId)
+          : and(
+              inArray(juniorReviewItems.addedByConversationId, [
+                ...input.addedByConversationIds,
+              ]),
+              isNull(juniorReviewItems.automationId),
+            ),
+      ),
+    );
+}
+
 /**
  * Make one User the only assignee of an open Review item. Return false when
  * the item is closed.

packages/junior/src/chat/tools/review-items.ts

@@ -339,7 +339,7 @@ export function createUpdateReviewItemTool(
 ) {
   return zodTool({
     description:
-      "Change an open review desk item, or close it. Set `status` to `done` or `dismissed` with a `reason`, for example when its pull request merged. An item never closes on its own. You cannot submit the answer to an action item: only an assignee can.",
+      "Change an open review desk item, or close it. Set `status` to `done` or `dismissed` with a `reason`, for example when its pull request merged. An item also closes when a person answers, dismisses, or replies in its thread, when its Conversation is purged, or when its Automation is deleted. You cannot submit the answer to an action item: only an assignee can.",
     exposure: "deferred",
     source: REVIEW_DESK_TOOL_SOURCE,
     annotations: {