fix(github-webhook): route PR-review author wakes to the owning issue, not an arbitrary Related: backlink (BLO-20886)

server/src/__tests__/heartbeat-context-summary.test.ts

@@ -245,6 +245,98 @@ describe("buildPaperclipTaskMarkdown", () => {
     expect(authorMarkdown).not.toContain("Latest review body:");
   });
 
+  // BLO-20886: github_pr_review_requested fires on a bare `@ally review` ASK
+  // -- no review has been posted -- and the author-role wake loop also
+  // covers plain PR lifecycle events with no review data at all. Both used
+  // to render the review-feedback directive unconditionally, telling the
+  // woken agent "a reviewer just posted findings on YOUR pull request" and
+  // to push a follow-up commit against a PR with zero recorded reviews
+  // (observed live: Blockcast/paperclip#953).
+  //
+  // review_requested is claimed by the more specific BLO-19522 branch above,
+  // which says the same true thing in more useful words (it names the
+  // requester and carries the anti-loop instruction). What BLO-20886 adds is
+  // the allowlist that catches every OTHER reasonless wakeReason -- the
+  // lifecycle events asserted below, and any reason added later.
+  it("does not instruct a push when no review has actually been submitted", () => {
+    const requestedMarkdown = buildPaperclipTaskMarkdown({
+      issue: null,
+      prReview: {
+        wakeReason: "github_pr_review_requested",
+        prNumber: 953,
+        repoFullName: "Blockcast/paperclip",
+        event: "issue_comment",
+        prRole: "author",
+      },
+    });
+    expect(requestedMarkdown).not.toContain("just posted findings on YOUR pull request");
+    expect(requestedMarkdown).not.toContain("push a follow-up commit");
+    expect(requestedMarkdown).not.toContain("GitHub PR review feedback directive:");
+    expect(requestedMarkdown).toContain("GitHub PR review request directive:");
+
+    // A lifecycle event carries no review either, and no branch above claims
+    // it -- so it must land on the generic directive rather than fall through
+    // to the feedback one.
+    for (const wakeReason of [
+      "github_pr_opened",
+      "github_pr_reopened",
+      "github_pr_synchronize",
+      "github_pr_ready_for_review",
+    ]) {
+      const lifecycleMarkdown = buildPaperclipTaskMarkdown({
+        issue: null,
+        prReview: {
+          wakeReason,
+          prNumber: 35,
+          repoFullName: "Blockcast/paperclip",
+          event: "pull_request",
+          prRole: "author",
+        },
+      });
+      expect(lifecycleMarkdown).not.toContain("YOUR pull request");
+      expect(lifecycleMarkdown).not.toContain("push a follow-up commit");
+      expect(lifecycleMarkdown).not.toContain("GitHub PR review feedback directive:");
+      expect(lifecycleMarkdown).toContain("GitHub PR event directive:");
+      expect(lifecycleMarkdown).toContain(`"${wakeReason}"`);
+      expect(lifecycleMarkdown).toContain("No review findings are recorded for this PR yet");
+    }
+
+    // The allowlist is what makes this hold for a wakeReason nobody has
+    // written yet: unrecognized must fail into "no findings", not into a
+    // false claim that findings exist.
+    const unknownMarkdown = buildPaperclipTaskMarkdown({
+      issue: null,
+      prReview: {
+        wakeReason: "github_pr_some_future_reason",
+        prNumber: 36,
+        repoFullName: "Blockcast/paperclip",
+        event: "pull_request",
+        prRole: "author",
+      },
+    });
+    expect(unknownMarkdown).toContain("GitHub PR event directive:");
+    expect(unknownMarkdown).not.toContain("GitHub PR review feedback directive:");
+  });
+
+  // Real review content must still get the author-shaped directive -- this
+  // fix narrows WHEN "YOUR pull request" fires, it doesn't remove it.
+  it("still asserts 'YOUR pull request' for an actionable review-feedback comment wake", () => {
+    const feedbackMarkdown = buildPaperclipTaskMarkdown({
+      issue: null,
+      prReview: {
+        wakeReason: "github_pr_review_feedback",
+        prNumber: 953,
+        repoFullName: "Blockcast/paperclip",
+        event: "issue_comment",
+        prRole: "author",
+        reviewBody: "Critical: missing null check.",
+        reviewAuthorLogin: "ally",
+      },
+    });
+    expect(feedbackMarkdown).toContain("GitHub PR review feedback directive:");
+    expect(feedbackMarkdown).toContain("YOUR pull request");
+  });
+
   it("adds accepted-plan continuation guidance for standard-work issues when the wake is flagged as a plan continuation", () => {
     const acceptedConfirmation = buildPaperclipTaskMarkdown({
       issue: {

server/src/__tests__/issue-pull-requests-ownership-selection.test.ts

@@ -0,0 +1,117 @@
+/**
+ * BLO-20886 round 7: which issue a merged PR is persisted against.
+ *
+ * `issue-pull-requests.ts` ranks link sources with the branch FIRST, while
+ * `resolveOwningPaperclipIdentifiers` ranks the branch LAST -- deliberately, on
+ * the measurement that branches get repurposed and are the wrong issue in every
+ * case where they disagree with a curated title. Those two orderings coexisted
+ * harmlessly only while a lowercase branch failed to classify at all. Once
+ * classification became case-insensitive (round 6), a STALE branch ref started
+ * outranking the curated title owner and the merged PR was recorded against the
+ * wrong issue.
+ *
+ * These tests pin the reconciliation: ownership decides, link-source strength
+ * only breaks ties among equally-owning (or equally-unowning) candidates.
+ */
+import { describe, expect, it } from "vitest";
+import { __test_selectIssuePerCompany as selectIssuePerCompany } from "../services/issue-pull-requests.js";
+
+const COMPANY = "company-1";
+
+describe("merged-PR issue selection defers to ownership (BLO-20886)", () => {
+  // The exact case from the review: the branch names a stale issue, the title
+  // names the issue the PR actually fixes, and the body mentions the stale one
+  // under a non-owning label.
+  const staleBranchFields = {
+    branch: "fix/blo-1-stale",
+    title: "Fix BLO-2",
+    body: "Related: BLO-1",
+  };
+
+  it("persists against the curated title owner, not the stale branch ref", () => {
+    const chosen = selectIssuePerCompany(
+      [
+        { id: "issue-stale", companyId: COMPANY, identifier: "BLO-1" },
+        { id: "issue-owner", companyId: COMPANY, identifier: "BLO-2" },
+      ],
+      staleBranchFields,
+    ).get(COMPANY);
+
+    expect(chosen?.identifier).toBe("BLO-2");
+    expect(chosen?.issueId).toBe("issue-owner");
+    // Provenance still describes how the winner was found, and stays accurate.
+    expect(chosen?.linkSource).toBe("title_ref");
+  });
+
+  it("is independent of the order the matched issues arrive in", () => {
+    // The pre-fix rule was strength-then-first-seen, so iteration order was
+    // load-bearing. Both orders must now agree.
+    for (const matched of [
+      [
+        { id: "issue-owner", companyId: COMPANY, identifier: "BLO-2" },
+        { id: "issue-stale", companyId: COMPANY, identifier: "BLO-1" },
+      ],
+      [
+        { id: "issue-stale", companyId: COMPANY, identifier: "BLO-1" },
+        { id: "issue-owner", companyId: COMPANY, identifier: "BLO-2" },
+      ],
+    ]) {
+      expect(selectIssuePerCompany(matched, staleBranchFields).get(COMPANY)?.identifier).toBe("BLO-2");
+    }
+  });
+
+  it("still prefers the branch when the branch IS the owner", () => {
+    // Ownership consults the branch when nothing curated resolves, so a
+    // branch-only owner must keep winning over a bare body mention. This is the
+    // 21-recovered-wakes case the branch tier exists for.
+    const chosen = selectIssuePerCompany(
+      [
+        { id: "issue-mentioned", companyId: COMPANY, identifier: "BLO-99" },
+        { id: "issue-owner", companyId: COMPANY, identifier: "BLO-20886" },
+      ],
+      { branch: "cto/blo-20886-round5", title: "no ref in title", body: "Related: BLO-99" },
+    ).get(COMPANY);
+
+    expect(chosen?.identifier).toBe("BLO-20886");
+    expect(chosen?.linkSource).toBe("branch_ref");
+  });
+
+  it("falls back to link-source strength when ownership names nobody", () => {
+    // No owning reference anywhere: no closing keyword, no `Refs:`, no house
+    // label. Behaviour here is unchanged from before the fix -- strongest
+    // source wins -- so this pins that the fix did not repurpose the fallback.
+    const chosen = selectIssuePerCompany(
+      [
+        { id: "issue-body", companyId: COMPANY, identifier: "BLO-8" },
+        { id: "issue-branch", companyId: COMPANY, identifier: "BLO-7" },
+      ],
+      { branch: "cto/blo-7-work", title: "no ref", body: "loosely mentions BLO-8" },
+    ).get(COMPANY);
+
+    expect(chosen?.identifier).toBe("BLO-7");
+    expect(chosen?.linkSource).toBe("branch_ref");
+  });
+
+  it("keeps one row per company", () => {
+    const selected = selectIssuePerCompany(
+      [
+        { id: "a", companyId: "company-A", identifier: "BLO-1" },
+        { id: "b", companyId: "company-B", identifier: "BLO-2" },
+      ],
+      staleBranchFields,
+    );
+
+    expect(selected.size).toBe(2);
+    expect(selected.get("company-A")?.identifier).toBe("BLO-1");
+    expect(selected.get("company-B")?.identifier).toBe("BLO-2");
+  });
+
+  it("skips issues with no identifier", () => {
+    const selected = selectIssuePerCompany(
+      [{ id: "no-ident", companyId: COMPANY, identifier: null }],
+      staleBranchFields,
+    );
+
+    expect(selected.size).toBe(0);
+  });
+});