diff --git a/server/src/__tests__/github-webhook.test.ts b/server/src/__tests__/github-webhook.test.ts
index 91833d561d3..0d2676bea98 100644
--- a/server/src/__tests__/github-webhook.test.ts
+++ b/server/src/__tests__/github-webhook.test.ts
@@ -60,6 +60,10 @@ import {
__resetMetricsForTest,
getMetricsRegistry,
} from "../services/metrics.js";
+import {
+ resolveLinkSourceForIdentifier,
+ resolveOwningPaperclipIdentifiers,
+} from "../services/paperclip-identifiers.js";
import { PULL_REQUEST_WORK_PRODUCT_SOURCE_TRUST_ACTOR_ID } from "../services/pull-request-work-products.js";
import { issueService } from "../services/issues.js";
import { errorHandler } from "../middleware/index.js";
@@ -126,6 +130,618 @@ describe("github-webhook pure helpers", () => {
expect(__test_extractPaperclipIdentifiers("(BLO-3182): work")).toEqual(["BLO-3182"]);
});
+ it("resolves the PR's OWNING identifier as title > labeled Fixes:/Closes:/Refs: body line > branch, never a bare Related: mention (BLO-20886)", () => {
+ // Title outranks everything else.
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ branch: "fix/BLO-9-stale-branch",
+ title: "fix BLO-1 thing",
+ body: "Refs: BLO-2",
+ }),
+ ).toEqual({ owning: ["BLO-1"] });
+
+ // No title ref: a labeled body line outranks the branch. This ordering is
+ // load-bearing -- branches get repurposed, so a branch ref goes stale
+ // while the curated title/body stays current (observed: #909's branch says
+ // blo-20049 while both its title and body name BLO-20467, the issue it
+ // actually fixes). Ranking the branch above them would reintroduce this
+ // ticket's own defect.
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ branch: "fix/blo-20049-stale-branch",
+ title: "fix(alertmanager-plugin): resolve webhook token per delivery",
+ body: "- Fixes: BLO-20467",
+ }),
+ ).toEqual({ owning: ["BLO-20467"] });
+
+ // Branch is the LAST resort, and matches case-insensitively so that a
+ // conventional lowercase branch resolves at all -- the identifier pattern
+ // is uppercase-only, which left the branch tier inert and failed 24 of 175
+ // recent PRs closed to `no_owning_reference`, dropping author wakes that
+ // should have been delivered.
+ expect(
+ resolveOwningPaperclipIdentifiers({ branch: "sre/blo-20886-pr-review-wake-routing" }),
+ ).toEqual({ owning: ["BLO-20886"] });
+ expect(
+ resolveOwningPaperclipIdentifiers({ branch: "qa/blo-21079-master-artifact" }),
+ ).toEqual({ owning: ["BLO-21079"] });
+ // ...but only when nothing curated resolved: a `Related:`-only body still
+ // does not promote a Related: entry, and the branch answers instead.
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ branch: "fix/blo-1-thing",
+ body: "Related: BLO-2",
+ }),
+ ).toEqual({ owning: ["BLO-1"] });
+
+ // No branch/title: a Fixes:/Closes:/Resolves:/Refs: labeled line counts,
+ // colon optional -- this repo's own PR bodies use both "Closes: BLO-1"
+ // and the natural-language "Closes BLO-1 and BLO-2" (multiple owners).
+ expect(
+ resolveOwningPaperclipIdentifiers({ body: "Fixes: BLO-1" }),
+ ).toEqual({ owning: ["BLO-1"] });
+ expect(
+ resolveOwningPaperclipIdentifiers({ body: "Closes BLO-1 and BLO-2" }),
+ ).toEqual({ owning: ["BLO-1", "BLO-2"] });
+ expect(
+ resolveOwningPaperclipIdentifiers({ body: "closed: BLO-1" }),
+ ).toEqual({ owning: ["BLO-1"] }); // case-insensitive, closing-keyword variant
+
+ // The exact incident shape: Refs: wins, Related: never counts as owning.
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ body: "Refs: BLO-19132\nRelated: BLO-20810, BLO-20129, BLO-19079\n",
+ }),
+ ).toEqual({ owning: ["BLO-19132"] });
+
+ // A bare Related: list with no owning line at all resolves to nothing --
+ // not to the first (or any) Related: entry.
+ expect(
+ resolveOwningPaperclipIdentifiers({ body: "Related: BLO-20810, BLO-20129" }),
+ ).toEqual({ owning: [] });
+
+ // Nothing anywhere.
+ expect(resolveOwningPaperclipIdentifiers({})).toEqual({ owning: [] });
+ });
+
+ it("treats a markdown-bulleted owning reference as owning -- the PR template's own house style (BLO-20886)", () => {
+ // .github/PULL_REQUEST_TEMPLATE.md renders "## Linked Issues or Issue
+ // Description" as a bullet list, so real PR bodies in this repo write
+ // `- Refs: BLO-1`, not a bare `Refs: BLO-1` line. An earlier revision of
+ // this rule anchored the keyword to the start of the line and therefore
+ // matched nothing on the majority of real bodies, failing every such PR
+ // closed to `no_owning_reference` and dropping an author wake that should
+ // have been delivered. Each of these is a real formatting shape.
+ for (const body of [
+ "- Refs: BLO-1",
+ "* Refs: BLO-1",
+ "+ Fixes: BLO-1",
+ "1. Closes: BLO-1",
+ " - Resolves: BLO-1", // indented sub-bullet
+ "- Refs: [BLO-1](https://paperclip.blockcast.net/BLO/issues/BLO-1)", // markdown link
+ ]) {
+ expect(resolveOwningPaperclipIdentifiers({ body })).toEqual({ owning: ["BLO-1"] });
+ }
+
+ // A bulleted Related: is still never owning -- the list marker must not
+ // become a way to smuggle an informational mention into the owning tier.
+ expect(
+ resolveOwningPaperclipIdentifiers({ body: "- Related: BLO-2, BLO-3" }),
+ ).toEqual({ owning: [] });
+
+ // PR #953's body verbatim (trimmed to the section that matters): a
+ // bulleted `Refs:` owner alongside a bulleted `Related:` list. This is the
+ // shape the live misroute actually had -- the pre-existing test above uses
+ // a synthesized bare-line form that does not reproduce it.
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ body: [
+ "## Linked Issues or Issue Description",
+ "",
+ "- Refs: [BLO-19132](https://paperclip.blockcast.net/BLO/issues/BLO-19132)",
+ "- Supersedes: #945",
+ "- Related: [BLO-20810](https://paperclip.blockcast.net/BLO/issues/BLO-20810), [BLO-20129](https://paperclip.blockcast.net/BLO/issues/BLO-20129), [BLO-19079](https://paperclip.blockcast.net/BLO/issues/BLO-19079)",
+ ].join("\n"),
+ }),
+ ).toEqual({ owning: ["BLO-19132"] });
+ });
+
+ it("ignores owning-looking Markdown code and trailing non-owning labels (BLO-20886)", () => {
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ body: [
+ "```md",
+ "Refs: BLO-2",
+ "```",
+ "~~~",
+ "Fixes: BLO-3",
+ "~~~~",
+ " Closes: BLO-4",
+ "\tResolves: BLO-5",
+ "Refs: BLO-1; Related: BLO-6",
+ ].join("\n"),
+ }),
+ ).toEqual({ owning: ["BLO-1"] });
+
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ body: "```\nRefs: BLO-2\n```\n Fixes: BLO-3",
+ }),
+ ).toEqual({ owning: [] });
+ });
+
+ it("does not let a list-prefixed pseudo-closer reopen a fence (BLO-20886)", () => {
+ // A closing fence admits only the marker run and whitespace. The OPENING
+ // grammar tolerates a list marker (a fence nested in a list item is
+ // ordinary Markdown), and reusing it to detect the close meant a `- ``` `
+ // line -- which CommonMark renders as fenced CONTENT -- ended the block
+ // early and exposed the following example as an ownership claim.
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ body: ["```", "- ```", "Refs: BLO-777", "```"].join("\n"),
+ }),
+ ).toEqual({ owning: [] });
+
+ // Same for a numbered-list prefix, and for a tilde fence.
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ body: ["~~~", "1. ~~~", "Fixes: BLO-778", "~~~"].join("\n"),
+ }),
+ ).toEqual({ owning: [] });
+
+ // The genuine closer still closes: an owning line AFTER a real fence is
+ // owning, so this did not simply wedge every fence open.
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ body: ["```", "Refs: BLO-2", "```", "Refs: BLO-1"].join("\n"),
+ }),
+ ).toEqual({ owning: ["BLO-1"] });
+ });
+
+ it("closes a fence indented by its list container, without loosening a root fence (BLO-20886)", () => {
+ // CommonMark measures a closing fence's three-space allowance from the
+ // fence's CONTAINER, not from column zero. Bounding it at three raw spaces
+ // meant a fence opened inside a list item never closed: the scanner
+ // swallowed the rest of the body and suppressed every genuinely visible
+ // owning line after it, dropping a wake that should have been delivered.
+ //
+ // Every expectation here was checked against a real CommonMark
+ // implementation (marked 16.4.2) rather than read off the spec.
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ body: ["- outer", " - inner:", "", " ```md", " Refs: BLO-999", " ```", "", "Refs: BLO-555"].join("\n"),
+ }),
+ ).toEqual({ owning: ["BLO-555"] });
+
+ // A fence opened on its own list-marker line closes at the item's content
+ // indent.
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ body: ["- ```md", " Refs: BLO-999", " ```", "", "Refs: BLO-555"].join("\n"),
+ }),
+ ).toEqual({ owning: ["BLO-555"] });
+
+ // The counter-case that keeps this from becoming a leak: with NO list
+ // container, a four-space `` ``` `` is fenced content, not a closer, so the
+ // fence stays open and everything after it stays unowning. marked agrees --
+ // it renders BLO-555 inside the code block. This is the fail-closed
+ // direction and must not regress into an early close.
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ body: ["```md", "Refs: BLO-999", " ```", "", "Refs: BLO-555"].join("\n"),
+ }),
+ ).toEqual({ owning: [] });
+
+ // Same when the opener carries its own 1-3 spaces but no container: the
+ // allowance does not grow with the opener's own indent, only with the
+ // container's.
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ body: [" ```md", " Refs: BLO-999", " ```", "", "Refs: BLO-555"].join("\n"),
+ }),
+ ).toEqual({ owning: [] });
+ });
+
+ it("treats mixed space-tab indentation as code by expanded columns (BLO-20886)", () => {
+ // CommonMark expands tabs to 4-column stops, so ` \t`, ` \t` and ` \t`
+ // are all four columns of indent -- an indented code block, exactly like
+ // ` `. Matching only the two literal prefixes `\t` and ` ` left the
+ // mixed forms eligible to declare an owner from inside a code example.
+ for (const indent of [" \t", " \t", " \t", "\t", " ", " "]) {
+ expect(
+ resolveOwningPaperclipIdentifiers({ body: `${indent}Refs: BLO-888` }),
+ ).toEqual({ owning: [] });
+ }
+
+ // Up to three columns is still a normal line, not code.
+ for (const indent of ["", " ", " ", " "]) {
+ expect(
+ resolveOwningPaperclipIdentifiers({ body: `${indent}Refs: BLO-1` }),
+ ).toEqual({ owning: ["BLO-1"] });
+ }
+ });
+
+ it("hides house-reference labels inside code, comments and indents (BLO-21312/BLO-20886)", () => {
+ // The house tier was added last and scanned the RAW body, so it skipped
+ // the fence/comment/indent filtering the closing-keyword tier already had.
+ // An `Issue:` line that renders as nothing -- or as a quoted example --
+ // could therefore route an author-directed "push a follow-up commit" wake
+ // to an issue no reader of the PR would call its owner.
+ expect(
+ resolveOwningPaperclipIdentifiers({ body: ["```", "Issue: BLO-111", "```"].join("\n") }),
+ ).toEqual({ owning: [] });
+ expect(
+ resolveOwningPaperclipIdentifiers({ body: [""].join("\n") }),
+ ).toEqual({ owning: [] });
+ expect(
+ resolveOwningPaperclipIdentifiers({ body: " Paperclip task: BLO-333" }),
+ ).toEqual({ owning: [] });
+ expect(
+ resolveOwningPaperclipIdentifiers({ body: " \tPaperclip issue: BLO-334" }),
+ ).toEqual({ owning: [] });
+
+ // A fenced example does not suppress a real house label elsewhere.
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ body: ["```", "Issue: BLO-111", "```", "Issue: BLO-1"].join("\n"),
+ }),
+ ).toEqual({ owning: ["BLO-1"] });
+ });
+
+ it("measures the indented-code threshold from the list container (BLO-23893)", () => {
+ // The four-column code threshold is relative to the enclosing CONTAINER,
+ // exactly as the closing-fence allowance already was. A list continuation
+ // expands to four RAW columns while sitting only two columns inside the
+ // item's content, so measuring from column zero threw away an ordinary
+ // visible paragraph as "code" and lost the owner it declared. marked 16.4.2
+ // renders this as `item\n Refs: BLO-1` -- a paragraph, no .
+ expect(
+ resolveOwningPaperclipIdentifiers({ body: "- item\n \tRefs: BLO-1" }),
+ ).toEqual({ owning: ["BLO-1"] });
+
+ // The counter-cases that keep this from becoming a leak. Four columns PAST
+ // the container is still code, at root and at depth -- marked renders both
+ // inside . This is the direction that matters: the relative
+ // measurement must not make genuinely-fenced-off text eligible to own.
+ expect(
+ resolveOwningPaperclipIdentifiers({ body: ["- item", "", " Refs: BLO-999"].join("\n") }),
+ ).toEqual({ owning: [] });
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ body: ["- a", " - b", "", " Refs: BLO-999"].join("\n"),
+ }),
+ ).toEqual({ owning: [] });
+
+ // With no container at all the threshold is unchanged from column zero, so
+ // the BLO-20886 mixed space-tab case above does not regress.
+ expect(
+ resolveOwningPaperclipIdentifiers({ body: " \tRefs: BLO-999" }),
+ ).toEqual({ owning: [] });
+ });
+
+ it("does not open an HTML comment from an indented-code delimiter (BLO-23893)", () => {
+ // Comment state used to advance before the indented-code early-out, so a
+ // ` `, suppressing an owner a reader of the
+ // PR can plainly see. Fail-closed, but still a dropped wake.
+ expect(
+ resolveOwningPaperclipIdentifiers({ body: [" ` still closes the comment
+ // rather than wedging it open forever. marked closes it here.
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ body: ["", "Issue: BLO-3"].join("\n"),
+ }),
+ ).toEqual({ owning: ["BLO-3"] });
+
+ // And the fail-closed guarantees are untouched: a real comment still hides
+ // its body, an unterminated one still swallows the rest, and a comment
+ // opened on a fence-opener line still hides its own body.
+ expect(
+ resolveOwningPaperclipIdentifiers({ body: [""].join("\n") }),
+ ).toEqual({ owning: [] });
+ expect(
+ resolveOwningPaperclipIdentifiers({ body: [""].join("\n"),
+ }),
+ ).toEqual({ owning: [] });
+ });
+
+ it("does not manufacture a branch owner from a version number (BLO-20886)", () => {
+ // Uppercasing a whole branch to match the uppercase-only identifier
+ // pattern also turns ordinary words-followed-by-a-number into
+ // "identifiers". Measured over the 200 most recently-updated PRs in this
+ // repo, that invented UNDICI-7, URI-3, ADDRESS-10, PR-870, FOLD-977 and
+ // EXPANSION-5. A spurious owner is not harmless: it hands an
+ // author-directed "push a follow-up commit" wake to whoever is assigned
+ // the same-named issue, which is this ticket's own defect.
+ for (const branch of [
+ "blo-21612-undici-7.29.0",
+ "blo-21611-fast-uri-3.1.5",
+ "blo-21613-ip-address-10.3.1",
+ "blo-21610-brace-expansion-5.0.9",
+ "sre/blo-20867-fold-977-metrics",
+ ]) {
+ const { owning } = resolveOwningPaperclipIdentifiers({ branch });
+ expect(owning).toHaveLength(1);
+ expect(owning[0]).toMatch(/^BLO-\d+$/);
+ }
+
+ // A branch carrying no ref at all still resolves nothing, rather than
+ // coining one from a trailing digit.
+ expect(resolveOwningPaperclipIdentifiers({ branch: "relay-wave-0" })).toEqual({ owning: [] });
+ expect(
+ resolveOwningPaperclipIdentifiers({ branch: "migration-members-page" }),
+ ).toEqual({ owning: [] });
+
+ // The real ref still resolves, at the branch start or after any `/`.
+ expect(
+ resolveOwningPaperclipIdentifiers({ branch: "sre/blo-20886-pr-review-wake-routing" }),
+ ).toEqual({ owning: ["BLO-20886"] });
+ expect(resolveOwningPaperclipIdentifiers({ branch: "blo-21610-thing" })).toEqual({
+ owning: ["BLO-21610"],
+ });
+ });
+
+ it("does not manufacture a branch owner from a Dependabot path (BLO-20886 round 6)", () => {
+ // The segment anchor alone only discriminates when the package name sits
+ // MID-segment (`blo-21612-undici-7.29.0`). Dependabot puts it at the START
+ // of a segment, so `undici-7` clears the anchor and the following `.`
+ // supplies the word boundary -- the leak the anchor was believed to close.
+ // Each of these manufactured an owner before the version-continuation
+ // guard, and each would route a dependency PR's author wake to whoever is
+ // assigned the same-named issue.
+ for (const branch of [
+ "dependabot/npm_and_yarn/undici-7.29.0",
+ "dependabot/npm_and_yarn/types/node-20.11.5",
+ "dependabot/github_actions/actions/checkout-4.2.0",
+ "dependabot/npm_and_yarn/fast-uri-3.1.5",
+ ]) {
+ expect(resolveOwningPaperclipIdentifiers({ branch })).toEqual({ owning: [] });
+ }
+
+ // The guard keys on `.` + any word character, which never occurs inside a
+ // real ref. Real refs continue with `-`, `/` or end, so no shape regresses.
+ expect(
+ resolveOwningPaperclipIdentifiers({ branch: "cto/blo-20886-round5-ownership-leaks" }),
+ ).toEqual({ owning: ["BLO-20886"] });
+ expect(resolveOwningPaperclipIdentifiers({ branch: "sre/blo-20886" })).toEqual({
+ owning: ["BLO-20886"],
+ });
+ });
+
+ it("does not manufacture a branch owner from a wildcard version or a bot namespace (BLO-20886 round 7)", () => {
+ // A `.` guard left WILDCARD versions live. A dependency PR names no
+ // issue in its title or body, so the branch tier is the only one consulted
+ // and a manufactured token would be the PR's SOLE owner.
+ for (const branch of ["renovate/node-20.x", "renovate/undici-7.x", "bump-undici-7.29.0"]) {
+ expect(resolveOwningPaperclipIdentifiers({ branch })).toEqual({ owning: [] });
+ }
+
+ // A version guard cannot be the whole answer, and this is the measurement
+ // that shows it: these carry NO version suffix for the guard to key on, yet
+ // still manufactured `NODE-20` / `UNDICI-7`. Skipping the two reserved bot
+ // namespaces is what closes them -- a dependency bot names its branch after
+ // the package it bumps, so nothing in one is an ownership claim.
+ for (const branch of [
+ "renovate/node-20",
+ "dependabot/npm_and_yarn/undici-7",
+ "renovate/blo-1-not-an-owner",
+ ]) {
+ expect(resolveOwningPaperclipIdentifiers({ branch })).toEqual({ owning: [] });
+ }
+
+ // Ordinary branches keep their refs, including the sub-issue `/` form.
+ expect(resolveOwningPaperclipIdentifiers({ branch: "kkroo/blo-19132-approval-dedupe-v2" })).toEqual({
+ owning: ["BLO-19132"],
+ });
+ expect(
+ resolveOwningPaperclipIdentifiers({ branch: "blo-21610-brace-expansion-5.0.9" }),
+ ).toEqual({ owning: ["BLO-21610"] });
+ });
+
+ it("classifies a lowercase branch-only owner as branch_ref, not body_ref (BLO-20886 round 6)", () => {
+ // Ownership accepts a lowercase branch case-insensitively, but link-source
+ // classification used the uppercase-only broad extractor, so the very shape
+ // branchTemplate produces resolved to nothing here and fell through to
+ // `body_ref`. With a related issue also named in the body, both candidates
+ // then carried equal strength and insertion order decided which one a
+ // merged PR was persisted against -- losing the authoritative branch owner
+ // to a bare `Related:` mention.
+ const fields = {
+ branch: "cto/blo-20886-round5-ownership-leaks",
+ title: "fix(github-webhook): close ownership-parsing leaks",
+ body: "Refs: BLO-20886\nRelated: BLO-19132",
+ };
+ expect(resolveLinkSourceForIdentifier("BLO-20886", fields)).toBe("branch_ref");
+ // The related-only identifier is still body-sourced, and the branch tier
+ // does not start claiming identifiers it does not carry.
+ expect(resolveLinkSourceForIdentifier("BLO-19132", fields)).toBe("body_ref");
+ // An uppercase branch keeps working, and a Dependabot branch stays unowned.
+ expect(resolveLinkSourceForIdentifier("BLO-20886", { branch: "CTO/BLO-20886-x" })).toBe(
+ "branch_ref",
+ );
+ expect(
+ resolveLinkSourceForIdentifier("UNDICI-7", {
+ branch: "dependabot/npm_and_yarn/undici-7.29.0",
+ }),
+ ).toBeNull();
+ });
+
+ it("falls back to a non-closing house-reference body line when title/keyword/branch all resolve nothing (BLO-21312)", () => {
+ // `github_pr_review_requested` arrives via `issue_comment`, whose payload
+ // carries no `pull_request.head.ref` -- `branch` is never populated on
+ // that path, so the case-insensitive branch tier (BLO-20886) is
+ // structurally unreachable there. These are the real house-label shapes
+ // observed on Blockcast/paperclip#931, #963, #976, #916.
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ body: "Issue: https://paperclip.blockcast.net/BLO/issues/BLO-20172",
+ }),
+ ).toEqual({ owning: ["BLO-20172"] });
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ body: "- Paperclip task: [BLO-20396](https://paperclip.blockcast.net/BLO/issues/BLO-20396)",
+ }),
+ ).toEqual({ owning: ["BLO-20396"] });
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ body: "Paperclip QA task: https://paperclip.blockcast.net/BLO/issues/BLO-21079",
+ }),
+ ).toEqual({ owning: ["BLO-21079"] });
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ body: "Paperclip issue: https://paperclip.blockcast.net/BLO/issues/BLO-19771",
+ }),
+ ).toEqual({ owning: ["BLO-19771"] });
+
+ // Still never widens far enough to make a bare `Related:` mention owning,
+ // even alongside a house label elsewhere in the same body -- the
+ // BLO-20886 guarantee holds unchanged.
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ body: "Related: BLO-1, BLO-2\nIssue: BLO-3\n",
+ }),
+ ).toEqual({ owning: ["BLO-3"] });
+ expect(
+ resolveOwningPaperclipIdentifiers({ body: "Related: BLO-1, BLO-2\n" }),
+ ).toEqual({ owning: [] });
+
+ // Ranked below the closing-keyword tier: a Fixes:/Closes:/Refs: line
+ // still wins over a house label in the same body.
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ body: "Fixes: BLO-1\nPaperclip issue: BLO-2\n",
+ }),
+ ).toEqual({ owning: ["BLO-1"] });
+
+ // Ranked below the branch tier too: on a `pull_request` event (branch
+ // populated), the measured branch answer still wins over an unmeasured
+ // house label in the same body.
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ branch: "fix/blo-1-thing",
+ body: "Paperclip issue: BLO-2",
+ }),
+ ).toEqual({ owning: ["BLO-1"] });
+
+ // Title still outranks a house label.
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ title: "fix BLO-1 thing",
+ body: "Paperclip issue: BLO-2",
+ }),
+ ).toEqual({ owning: ["BLO-1"] });
+
+ // The colon is mandatory for this weaker tier (unlike the closing-keyword
+ // tier, where "Closes BLO-1" is unambiguous natural language). "Issue" is
+ // an ordinary noun that also starts ordinary sentences, so an optional
+ // colon here would treat prose as an ownership claim. Neither of these is
+ // a house label -- both must resolve to nothing.
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ body: "Issue filed a related bug, see BLO-1",
+ }),
+ ).toEqual({ owning: [] });
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ body: "Issue description for BLO-2",
+ }),
+ ).toEqual({ owning: [] });
+
+ // A house-reference line carrying a second, explicitly different label on
+ // the SAME line must resolve only the house label's own direct value --
+ // the semicolon-separated Related: mention must not become owning just
+ // because it shares a line with a real house label (BLO-20886's
+ // guarantee applied to this new tier).
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ body: "Issue: BLO-1; Related: BLO-2",
+ }),
+ ).toEqual({ owning: ["BLO-1"] });
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ body: "Paperclip issue: BLO-1, Related: BLO-2",
+ }),
+ ).toEqual({ owning: ["BLO-1"] });
+ });
+
+ it("keeps owning-looking text unreachable inside list-nested fences and unclosed fences (BLO-20886)", () => {
+ // A fence nested in a list item is ordinary Markdown -- it is how a bullet
+ // quotes an example PR body -- but its opening line starts with the list
+ // marker. A root-level-only fence scanner never opened a fence here, so the
+ // indented `Refs:` line inside the example declared an owner and could
+ // capture an author-directed "push a follow-up commit" wake.
+ expect(
+ resolveOwningPaperclipIdentifiers({ body: ["- ```md", " Refs: BLO-999", " ```"].join("\n") }),
+ ).toEqual({ owning: [] });
+ expect(
+ resolveOwningPaperclipIdentifiers({ body: ["1. ~~~", " Closes: BLO-998", " ~~~"].join("\n") }),
+ ).toEqual({ owning: [] });
+
+ // CommonMark allows only whitespace after a CLOSING fence's marker run, so
+ // ``` js inside an open fence is content. Treating it as the close reopened
+ // the remainder of the block to the ownership scan.
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ body: ["```", "example:", "``` js", "Refs: BLO-997", "```"].join("\n"),
+ }),
+ ).toEqual({ owning: [] });
+
+ // An unterminated fence swallows the rest of the body rather than falling
+ // back to matching -- ambiguity fails closed, never toward a guessed owner.
+ expect(
+ resolveOwningPaperclipIdentifiers({ body: ["```", "Refs: BLO-996"].join("\n") }),
+ ).toEqual({ owning: [] });
+
+ // The fence rules must not swallow the real thing: a closed fence releases
+ // the lines after it, and the repo's bulleted house style still owns.
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ body: ["- ```md", " Refs: BLO-999", " ```", "- Refs: BLO-19132"].join("\n"),
+ }),
+ ).toEqual({ owning: ["BLO-19132"] });
+ });
+
+ it("never lets an HTML comment declare an owner (BLO-20886)", () => {
+ // A comment renders as nothing, so an owner declared inside one is
+ // invisible to every human reading the PR -- an unexplainable misroute.
+ // The opener and its `-->` sit on different lines in the repo's own
+ // PULL_REQUEST_TEMPLATE.md, so the state has to cross the line loop.
+ expect(
+ resolveOwningPaperclipIdentifiers({ body: [""].join("\n") }),
+ ).toEqual({ owning: [] });
+ expect(resolveOwningPaperclipIdentifiers({ body: "" })).toEqual({
+ owning: [],
+ });
+ // A comment cannot hide the fence that would otherwise contain it, either.
+ expect(
+ resolveOwningPaperclipIdentifiers({
+ body: ["", "Refs: BLO-886"].join("\n"),
+ }),
+ ).toEqual({ owning: ["BLO-886"] });
+ // Unterminated: the rest of the body stays commented out, failing closed.
+ expect(
+ resolveOwningPaperclipIdentifiers({ body: ["", "Refs: BLO-19132"].join("\n"),
+ }),
+ ).toEqual({ owning: ["BLO-19132"] });
+ });
+
it("rejects payloads with bad signatures and accepts ones with good signatures", () => {
const secret = "test-webhook-secret-do-not-use-in-prod";
const body = Buffer.from(JSON.stringify({ action: "completed" }), "utf8");
@@ -1009,6 +1625,33 @@ describe("github-webhook pure helpers", () => {
expect(ctx ? __test_shouldFirePrReviewerWake(ctx) : true).toBe(false);
});
+ it("keeps a lowercase branch-only owner in the candidate identifiers (BLO-20886)", () => {
+ // The owning tiers uppercase the branch (real branches are lowercase and
+ // PAPERCLIP_IDENTIFIER_PATTERN is uppercase-only); the broad `identifiers`
+ // extraction does not. A PR whose ONLY ref is a lowercase branch therefore
+ // resolved an owner while `identifiers` came back empty -- and the route
+ // drops such a delivery at the `no_paperclip_identifier` gate before the
+ // owner is ever consulted. Past that gate it is still unreachable, because
+ // author wakes are `matched.filter(m => owning.includes(m.identifier))`
+ // and `matched` derives from `identifiers`. Either way the wake this
+ // module exists to deliver is lost, so the owner must appear in both.
+ const ctx = __test_resolveEventContext("pull_request_review", {
+ action: "submitted",
+ pull_request: {
+ number: 962,
+ title: "tidy up the webhook",
+ body: "No issue reference in this body at all.",
+ head: { ref: "fix/blo-20886-only", sha: "deadbeef" },
+ user: { login: "kkroo" },
+ },
+ review: { state: "changes_requested", body: "please fix", user: { login: "ally" } },
+ repository: { full_name: "Blockcast/paperclip" },
+ });
+
+ expect(ctx?.owningIdentifiers).toEqual(["BLO-20886"]);
+ expect(ctx?.identifiers).toContain("BLO-20886");
+ });
+
it("extracts review body / state / author from pull_request_review.submitted so the assignee wake can render it inline (BLO-6300)", () => {
const ctx = __test_resolveEventContext("pull_request_review", {
action: "submitted",
@@ -2837,7 +3480,14 @@ describeEmbeddedPostgres("github-webhook route", () => {
number: 18859,
title: "Delivery funnel counters",
body: null,
- head: { ref: "platform/blo-18859-github-delivery-metrics" },
+ // Deliberately carries no Paperclip ref in branch, title or body: this
+ // test is about the reviewer-wake delivery counters, and needs the
+ // route to stop at `no_paperclip_identifier` so nothing else runs. The
+ // branch used to read `platform/blo-18859-...`, which only stayed
+ // inert because a lowercase branch-only ref was silently dropped
+ // before routing (BLO-20886) -- scaffolding that stopped being inert
+ // once that was fixed.
+ head: { ref: "platform/github-delivery-metrics" },
},
repository: { full_name: "Blockcast/paperclip" },
};
@@ -2893,7 +3543,8 @@ describeEmbeddedPostgres("github-webhook route", () => {
number: 18860,
title: "Suppressed delivery",
body: null,
- head: { ref: "platform/blo-18859-suppressed" },
+ // No Paperclip ref anywhere, for the same reason as above.
+ head: { ref: "platform/suppressed-delivery" },
},
repository: { full_name: "Blockcast/paperclip" },
};
@@ -4288,6 +4939,275 @@ describeEmbeddedPostgres("github-webhook route", () => {
});
});
+ it("routes an author wake to the PR's owning Refs: issue, never an unrelated Related: backlink assignee (BLO-20886)", async () => {
+ // Reproduces the live incident (Blockcast/paperclip#953): the PR body
+ // carried `Refs: BLO-19132` (the owning issue) plus
+ // `Related: BLO-20810, BLO-20129, BLO-19079` -- three bare informational
+ // mentions. Before this fix, the author-wake loop treated every matched
+ // identifier as equally-weighted and woke BLO-20129's assignee (the
+ // THIRD Related: entry) with a "push a follow-up commit" directive for a
+ // PR that agent had no relationship to at all.
+ //
+ // All four identifiers share the BLO prefix (as in the real incident),
+ // so they're seeded as four issues under ONE company/agent set --
+ // seedIssueWithIdentifier creates a fresh company per call and company
+ // issue_prefix is unique, so four BLO- calls would collide.
+ const { companyId } = await seedCompanyAndAgent();
+ async function seedBloIssue(identifier: string) {
+ const agentId = randomUUID();
+ const issueId = randomUUID();
+ await db.insert(agents).values({
+ id: agentId,
+ companyId,
+ name: `Agent-${identifier}`,
+ role: "engineer",
+ status: "idle",
+ adapterType: "claude_k8s",
+ adapterConfig: {},
+ runtimeConfig: {},
+ permissions: {},
+ });
+ await db.insert(issues).values({
+ id: issueId,
+ companyId,
+ title: "Test issue",
+ status: "in_progress",
+ priority: "medium",
+ assigneeAgentId: agentId,
+ issueNumber: Number(identifier.split("-")[1]),
+ identifier,
+ });
+ return { agentId, issueId };
+ }
+ const refsIssue = await seedBloIssue("BLO-19132");
+ const relatedB = await seedBloIssue("BLO-20810");
+ const relatedC = await seedBloIssue("BLO-20129");
+ const relatedD = await seedBloIssue("BLO-19079");
+ const app = buildApp();
+ const payload = {
+ action: "created",
+ issue: {
+ number: 953,
+ title: "approval dedupe v2",
+ body: "Refs: BLO-19132\nRelated: BLO-20810, BLO-20129, BLO-19079\n",
+ html_url: "https://github.com/Blockcast/paperclip/pull/953",
+ pull_request: { url: "https://api.github.com/repos/Blockcast/paperclip/pulls/953" },
+ user: { login: "kkroo" },
+ },
+ comment: {
+ id: 5156328634,
+ body: "@ally review exact head d9f28c1e0e6595ce8de9515bf0158b04d136a204",
+ html_url: "https://github.com/Blockcast/paperclip/pull/953#issuecomment-5156328634",
+ user: { login: "kkroo" },
+ },
+ repository: { full_name: "Blockcast/paperclip" },
+ };
+ const { body, signature } = signedRequest(payload);
+
+ const res = await request(app)
+ .post("/api/webhooks/github")
+ .set("x-github-event", "issue_comment")
+ .set("x-hub-signature-256", signature)
+ .set("x-github-delivery", "delivery-blo-20886")
+ .set("content-type", "application/json")
+ .send(body);
+
+ expect(res.status).toBe(200);
+ expect(res.body.wakes).toEqual([{ issueIdentifier: "BLO-19132", agentId: refsIssue.agentId }]);
+
+ const allWakes = await db
+ .select({ agentId: agentWakeupRequests.agentId })
+ .from(agentWakeupRequests)
+ .where(inArray(agentWakeupRequests.agentId, [
+ refsIssue.agentId,
+ relatedB.agentId,
+ relatedC.agentId,
+ relatedD.agentId,
+ ]));
+ // Only the Refs: owner was ever woken -- not one of the three Related:
+ // assignees, and specifically never BLO-20129's (relatedC), the one that
+ // fired live.
+ expect(allWakes.map((w) => w.agentId)).toEqual([refsIssue.agentId]);
+ });
+
+ it("suppresses the author wake with a logged reason when a PR carries only Related: mentions and no owning reference (BLO-20886)", async () => {
+ // No Refs:/Fixes:/Closes:/Resolves: line and no branch/title ref -- the
+ // PR names issues but doesn't claim ownership of any of them. Acceptance
+ // criterion: this must drop with a suppressionReason, not fall through to
+ // an arbitrary Related: assignee.
+ const relatedOnly = await seedIssueWithIdentifier("BLO-20811");
+ const app = buildApp();
+ const payload = {
+ action: "created",
+ issue: {
+ number: 954,
+ title: "misc cleanup",
+ body: "Related: BLO-20811\n",
+ html_url: "https://github.com/Blockcast/paperclip/pull/954",
+ pull_request: { url: "https://api.github.com/repos/Blockcast/paperclip/pulls/954" },
+ user: { login: "kkroo" },
+ },
+ comment: {
+ id: 5156328700,
+ body: "@ally review please",
+ html_url: "https://github.com/Blockcast/paperclip/pull/954#issuecomment-5156328700",
+ user: { login: "kkroo" },
+ },
+ repository: { full_name: "Blockcast/paperclip" },
+ };
+ const { body, signature } = signedRequest(payload);
+
+ const res = await request(app)
+ .post("/api/webhooks/github")
+ .set("x-github-event", "issue_comment")
+ .set("x-hub-signature-256", signature)
+ .set("x-github-delivery", "delivery-blo-20886-no-owner")
+ .set("content-type", "application/json")
+ .send(body);
+
+ expect(res.status).toBe(200);
+ expect(res.body.wakes).toEqual([]);
+ expect(res.body.skipped).toContainEqual({
+ issueIdentifier: null,
+ reason: "no_owning_reference",
+ });
+
+ const wakes = await db
+ .select({ id: agentWakeupRequests.id })
+ .from(agentWakeupRequests)
+ .where(eq(agentWakeupRequests.agentId, relatedOnly.agentId));
+ expect(wakes).toHaveLength(0);
+ });
+
+ it("routes an issue_comment @ally-review author wake to a PR's owning issue named only by a house-reference label, not a Related: mention (BLO-21312)", async () => {
+ // github_pr_review_requested arrives via issue_comment, whose payload
+ // carries no pull_request.head.ref -- BLO-20886's branch-tier recovery
+ // can never reach this path. This PR's title and body carry no
+ // Fixes:/Closes:/Resolves:/Refs: line, only a "Paperclip issue:" house
+ // label (the real shape observed on Blockcast/paperclip#916) plus an
+ // unrelated Related: mention -- reproducing the gap and its guardrail in
+ // one payload. Both issues share the BLO prefix (as in the real PR body),
+ // so they're seeded under one company -- seedIssueWithIdentifier creates
+ // a fresh company per call and company issue_prefix is unique, so two
+ // BLO- calls would collide.
+ const { companyId } = await seedCompanyAndAgent();
+ async function seedBloIssue(identifier: string) {
+ const agentId = randomUUID();
+ const issueId = randomUUID();
+ await db.insert(agents).values({
+ id: agentId,
+ companyId,
+ name: `Agent-${identifier}`,
+ role: "engineer",
+ status: "idle",
+ adapterType: "claude_k8s",
+ adapterConfig: {},
+ runtimeConfig: {},
+ permissions: {},
+ });
+ await db.insert(issues).values({
+ id: issueId,
+ companyId,
+ title: "Test issue",
+ status: "in_progress",
+ priority: "medium",
+ assigneeAgentId: agentId,
+ issueNumber: Number(identifier.split("-")[1]),
+ identifier,
+ });
+ return { agentId, issueId };
+ }
+ const owner = await seedBloIssue("BLO-19771");
+ const relatedOnly = await seedBloIssue("BLO-20811");
+ const app = buildApp();
+ const payload = {
+ action: "created",
+ issue: {
+ number: 916,
+ title: "fix(pipelines): retire exited stage automation issues",
+ body: "## Linked Issues or Issue Description\n\nPaperclip issue: https://paperclip.blockcast.net/BLO/issues/BLO-19771\nRelated: BLO-20811\n",
+ html_url: "https://github.com/Blockcast/paperclip/pull/916",
+ pull_request: { url: "https://api.github.com/repos/Blockcast/paperclip/pulls/916" },
+ user: { login: "kkroo" },
+ },
+ comment: {
+ id: 5156328800,
+ body: "@ally review please",
+ html_url: "https://github.com/Blockcast/paperclip/pull/916#issuecomment-5156328800",
+ user: { login: "kkroo" },
+ },
+ repository: { full_name: "Blockcast/paperclip" },
+ };
+ const { body, signature } = signedRequest(payload);
+
+ const res = await request(app)
+ .post("/api/webhooks/github")
+ .set("x-github-event", "issue_comment")
+ .set("x-hub-signature-256", signature)
+ .set("x-github-delivery", "delivery-blo-21312-house-label")
+ .set("content-type", "application/json")
+ .send(body);
+
+ expect(res.status).toBe(200);
+ expect(res.body.wakes).toEqual([{ issueIdentifier: "BLO-19771", agentId: owner.agentId }]);
+ expect(res.body.skipped).not.toContainEqual(
+ expect.objectContaining({ reason: "no_owning_reference" }),
+ );
+
+ const allWakes = await db
+ .select({ agentId: agentWakeupRequests.agentId })
+ .from(agentWakeupRequests)
+ .where(inArray(agentWakeupRequests.agentId, [owner.agentId, relatedOnly.agentId]));
+ // Only the house-label owner was woken -- never the Related: mention.
+ expect(allWakes.map((w) => w.agentId)).toEqual([owner.agentId]);
+ });
+
+ it("delivers a lowercase branch-only author wake end-to-end (BLO-20886)", async () => {
+ // Route-level companion to the pure-helper test above. The owning tiers
+ // match the branch case-insensitively but the broad `identifiers`
+ // extraction is uppercase-only, so a PR whose ONLY ref is a lowercase
+ // branch resolved an owner and then died at the `no_paperclip_identifier`
+ // gate before that owner was ever consulted -- a dropped author wake that
+ // no test covered, because every existing branch fixture also carries the
+ // ref in its title or body.
+ const owner = await seedIssueWithIdentifier("BLO-20886");
+ const app = buildApp();
+ const payload = {
+ action: "submitted",
+ pull_request: {
+ number: 962,
+ title: "fix(github-webhook): tidy the receiver",
+ body: "No issue reference anywhere in this body.",
+ html_url: "https://github.com/Blockcast/paperclip/pull/962",
+ head: { ref: "sre/blo-20886-pr-review-wake-routing", sha: "17532d7f" },
+ user: { login: "kkroo" },
+ },
+ review: {
+ state: "changes_requested",
+ body: "one nit",
+ html_url: "https://github.com/Blockcast/paperclip/pull/962#pullrequestreview-1",
+ user: { login: "someone-else" },
+ },
+ repository: { full_name: "Blockcast/paperclip" },
+ };
+ const { body, signature } = signedRequest(payload);
+
+ const res = await request(app)
+ .post("/api/webhooks/github")
+ .set("x-github-event", "pull_request_review")
+ .set("x-hub-signature-256", signature)
+ .set("x-github-delivery", "delivery-blo-20886-lowercase-branch")
+ .set("content-type", "application/json")
+ .send(body);
+
+ expect(res.status).toBe(200);
+ // Specifically NOT dropped as `no_paperclip_identifier`.
+ expect(res.body.ignored).toBeUndefined();
+ expect(res.body.wakes).toEqual([
+ { issueIdentifier: "BLO-20886", agentId: owner.agentId },
+ ]);
+ });
+
it("dedupes an @ally comment redelivery on the author wake after it completed or was cancelled (BLO-18953)", async () => {
// Route-level companion to the reviewer redelivery test above. BLO-18953's
// final pass made terminal-status dedup depend on the key's SCOPE, and the
diff --git a/server/src/__tests__/heartbeat-context-summary.test.ts b/server/src/__tests__/heartbeat-context-summary.test.ts
index c7ce4eecada..2ccf8077a0f 100644
--- a/server/src/__tests__/heartbeat-context-summary.test.ts
+++ b/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: {
diff --git a/server/src/__tests__/issue-pull-requests-ownership-selection.test.ts b/server/src/__tests__/issue-pull-requests-ownership-selection.test.ts
new file mode 100644
index 00000000000..1dea04e7a4b
--- /dev/null
+++ b/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);
+ });
+});
diff --git a/server/src/routes/github-webhook.ts b/server/src/routes/github-webhook.ts
index ecdaf935b9f..a7caa33d4f2 100644
--- a/server/src/routes/github-webhook.ts
+++ b/server/src/routes/github-webhook.ts
@@ -48,7 +48,11 @@ import {
} from "../services/dependabot-alert-issues.js";
import { logger } from "../middleware/logger.js";
import type { PluginWorkerManager } from "../services/plugin-worker-manager.js";
-import { extractPaperclipIdentifiers } from "../services/paperclip-identifiers.js";
+import {
+ extractPaperclipIdentifiers,
+ resolveOwningPaperclipIdentifiers,
+ type OwningIdentifierResolution,
+} from "../services/paperclip-identifiers.js";
import {
githubReviewerIdentityMatches,
githubListIssueCommentBodies,
@@ -541,6 +545,12 @@ async function countPrReviewFeedbackCycles(
interface ResolvedEventContext {
identifiers: string[];
+ // BLO-20886: the identifier(s) that OWN this PR (branch/title/labeled
+ // Fixes:/Closes:/Refs: line), as opposed to `identifiers` which is every
+ // BLO-#### mentioned anywhere, including an informational `Related:` list.
+ // Only wakeReasons that drive an author-directed ("prRole: author") wake
+ // consult this -- see resolveOwningPaperclipIdentifiers for the rule.
+ owningIdentifiers?: string[];
wakeReason: string;
prNumber: number | null;
repoFullName: string | null;
@@ -606,7 +616,53 @@ function clampReviewBody(value: string | null | undefined): string | null {
return `${cut}\n…(truncated)`;
}
+/**
+ * Resolve a webhook payload into the routing context, guaranteeing the
+ * invariant that every resolved OWNER is also a wake candidate.
+ *
+ * `identifiers` (every ref the PR mentions anywhere) and `owningIdentifiers`
+ * (the ones that actually own it) are extracted by different rules, and the
+ * owning tiers are deliberately more permissive in one place: tier 3
+ * uppercases the branch, because real branches are lowercase
+ * (`sre/blo-20886-...`) and PAPERCLIP_IDENTIFIER_PATTERN is uppercase-only.
+ * The broad set does not. So a PR whose ONLY ref is a lowercase branch --
+ * `fix/blo-20886-only`, nothing in title or body -- resolved an owner while
+ * `identifiers` came back empty, and the route then dropped the delivery at
+ * the `no_paperclip_identifier` gate before the owner could be used. Even past
+ * that gate the owner was unreachable: author wakes are computed as
+ * `matched.filter(m => owning.includes(m.identifier))`, and `matched` derives
+ * from `identifiers`, so an owner missing from the broad set silently yields
+ * no candidates. Both failures land on the wake this module exists to deliver.
+ *
+ * The union is taken here, once, rather than in each event branch so the
+ * invariant cannot be missed by a case added later.
+ *
+ * Deliberately NOT fixed by uppercasing the branch inside the broad
+ * extraction: that would also fold stale branch refs into `identifiers` for
+ * PRs whose branch and title disagree (#909's branch says `blo-20049` while
+ * title and body both name BLO-20467, the issue it actually fixes -- 8 such
+ * disagreements across the 175 PRs measured for the tier ordering). Those refs
+ * are exactly what the tier ranking exists to keep OUT of ownership; widening
+ * the broad set with them would spread that noise to every other consumer to
+ * fix a gate problem. Unioning the resolved owners adds the one identifier the
+ * tiers already decided was authoritative, and nothing else.
+ */
function resolveEventContext(
+ eventName: string,
+ payload: Record,
+ options: Parameters[2] = {},
+): ResolvedEventContext | null {
+ const context = resolveEventContextRaw(eventName, payload, options);
+ if (!context) return null;
+ const owning = context.owningIdentifiers ?? [];
+ if (owning.length === 0) return context;
+ const identifiers = new Set(context.identifiers);
+ for (const identifier of owning) identifiers.add(identifier);
+ if (identifiers.size === context.identifiers.length) return context;
+ return { ...context, identifiers: Array.from(identifiers) };
+}
+
+function resolveEventContextRaw(
eventName: string,
payload: Record,
options: {
@@ -639,6 +695,7 @@ function resolveEventContext(
if (!pr) {
return {
ids: [] as string[],
+ owning: { owning: [] } as OwningIdentifierResolution,
number: null as number | null,
title: null as string | null,
url: null as string | null,
@@ -658,6 +715,7 @@ function resolveEventContext(
const user = pr.user as Record | undefined;
return {
ids: extractPaperclipIdentifiers(branch, title, body),
+ owning: resolveOwningPaperclipIdentifiers({ branch, title, body }),
number,
title: title ?? null,
url: githubPrUrl(repoFullName, number, readStringField(pr, "html_url")),
@@ -865,16 +923,22 @@ function resolveEventContext(
const prNumber = (issue.number as number | undefined) ?? null;
const prUrl = githubPrUrl(repoFullName, prNumber, readStringField(issue, "html_url"));
const commentUrl = readStringField(comment, "html_url");
+ const issueTitle = issue.title as string | undefined;
+ const issueBody = issue.body as string | undefined;
+ // Owning resolution deliberately excludes commentBody: the comment is
+ // the @ally ASK that triggered this event, not an ownership claim about
+ // the PR (see resolveOwningPaperclipIdentifiers). No branch tier here
+ // either -- issue_comment payloads don't carry pull_request.head.ref --
+ // so this path relies on title, a closing-keyword body line, or (BLO-21312)
+ // a non-closing house-reference body line (Issue:/Paperclip task:/etc.).
+ const owning = resolveOwningPaperclipIdentifiers({ title: issueTitle, body: issueBody });
return {
- identifiers: extractPaperclipIdentifiers(
- issue.title as string | undefined,
- issue.body as string | undefined,
- commentBody,
- ),
+ identifiers: extractPaperclipIdentifiers(issueTitle, issueBody, commentBody),
+ owningIdentifiers: owning.owning,
wakeReason: reviewerRequest ? "github_pr_review_requested" : "github_pr_review_feedback",
prNumber,
repoFullName,
- prTitle: (issue.title as string | undefined) ?? null,
+ prTitle: issueTitle ?? null,
prUrl,
eventUrl: commentUrl ?? prUrl,
commentId: (comment?.id as number | undefined) ?? null,
@@ -899,6 +963,7 @@ function resolveEventContext(
const reviewUrl = readStringField(review, "html_url");
return {
identifiers: collected.ids,
+ owningIdentifiers: collected.owning.owning,
wakeReason: "github_pr_review_submitted",
prNumber: collected.number,
repoFullName,
@@ -950,6 +1015,7 @@ function resolveEventContext(
const merged = pr?.merged === true;
return {
identifiers: collected.ids,
+ owningIdentifiers: collected.owning.owning,
wakeReason: reasonByAction[action] ?? "github_pull_request",
prNumber: collected.number,
repoFullName,
@@ -2948,7 +3014,50 @@ export function githubWebhookRoutes(db: Db, config: GithubWebhookConfig) {
);
}
- for (const issue of suppressAuthorWake ? [] : matched) {
+ // BLO-20886: an author-directed wake (prRole: "author", set below via
+ // isPrWake) asserts ownership of the PR ("YOUR pull request") and, for
+ // review-shaped reasons, instructs a push. Firing it for every issue in
+ // `matched` -- which includes issues named only via an informational
+ // `Related:` mention -- sent that directive to the assignee of an issue
+ // with no relationship to the PR at all (observed live: PR #953 matched
+ // BLO-19132 via `Refs:` and BLO-20810/BLO-20129/BLO-19079 via `Related:`;
+ // the wake landed on BLO-20129's assignee). Restrict the author-wake loop
+ // to the PR's OWNING issue(s) only -- resolveOwningPaperclipIdentifiers's
+ // branch > title > labeled Fixes:/Closes:/Refs: rule. `matched` keeps its
+ // full breadth for the back-link comment and merged-PR forward-capture
+ // above, which are informational and correctly link every mentioned
+ // issue. When no owning issue resolves (none found), the author wake is
+ // dropped with a logged suppressionReason rather than falling through to
+ // a lower-priority or unlabeled mention.
+ const isPrWake = context.wakeReason.startsWith("github_pr_") && context.prNumber !== null;
+ let authorWakeCandidates = matched;
+ if (isPrWake) {
+ const owning = context.owningIdentifiers ?? [];
+ if (owning.length === 0) {
+ authorWakeCandidates = [];
+ if (matched.length > 0) {
+ const suppressionReason = "no_owning_reference";
+ skipped.push({ issueIdentifier: null, reason: suppressionReason });
+ logger.info(
+ {
+ deliveryId,
+ event: eventName,
+ wakeReason: context.wakeReason,
+ prNumber: context.prNumber,
+ repoFullName: context.repoFullName,
+ identifiers: context.identifiers,
+ matchedIdentifiers: matched.map((m) => m.identifier),
+ suppressionReason,
+ },
+ "github webhook suppressed author-directed PR wake: no confidently-resolved owning issue",
+ );
+ }
+ } else {
+ authorWakeCandidates = matched.filter((m) => m.identifier && owning.includes(m.identifier));
+ }
+ }
+
+ for (const issue of suppressAuthorWake ? [] : authorWakeCandidates) {
// Terminal-status issues don't need to wake -- the assignee
// shouldn't reopen `done`/`cancelled` work just because a stale
// CI ping arrived.
@@ -3026,9 +3135,8 @@ export function githubWebhookRoutes(db: Db, config: GithubWebhookConfig) {
// PR-shaped wakes carry an `prRole: "author"` marker so the
// heartbeat directive flips from reviewer-shaped ("review this PR")
// to author-shaped ("a reviewer just posted findings on YOUR PR").
- // Non-PR wakes (CI completion, etc.) leave prRole unset.
- const isPrWake =
- context.wakeReason.startsWith("github_pr_") && context.prNumber !== null;
+ // Non-PR wakes (CI completion, etc.) leave prRole unset. (isPrWake is
+ // hoisted above this loop -- see the authorWakeCandidates comment.)
// BLO-13247: the actionableReviewFeedback branch above already
// precheck-and-skips on its own idempotency key before this point, but
diff --git a/server/src/services/heartbeat.ts b/server/src/services/heartbeat.ts
index 1635371ffe3..b52b8c2fe11 100644
--- a/server/src/services/heartbeat.ts
+++ b/server/src/services/heartbeat.ts
@@ -8747,6 +8747,21 @@ function buildRunEventRuntimeProgress(input: {
};
}
+// The only wakeReasons that structurally guarantee a review actually EXISTS,
+// and therefore the only ones allowed to tell a PR author that a reviewer
+// posted findings and that they should push a follow-up commit. See the
+// author branch in buildPaperclipTaskMarkdown.
+//
+// An allowlist rather than a denylist on purpose: every other wakeReason --
+// today's lifecycle events (opened/reopened/synchronize/ready_for_review),
+// review_requested, and whatever is added next -- carries no review, so an
+// unrecognized reason must fail into "no findings recorded" rather than into a
+// false claim that findings exist.
+const AUTHOR_REVIEW_CONTENT_WAKE_REASONS = new Set([
+ "github_pr_review_submitted",
+ "github_pr_review_feedback",
+]);
+
export function buildPaperclipTaskMarkdown(input: {
issue: {
id: string;
@@ -8863,6 +8878,26 @@ export function buildPaperclipTaskMarkdown(input: {
if (prReview.requestCommentBody) {
lines.push("", "The request comment:", fenceTaskText(prReview.requestCommentBody));
}
+ } else if (prReview.prRole === "author" && !AUTHOR_REVIEW_CONTENT_WAKE_REASONS.has(prReview.wakeReason)) {
+ // BLO-20886 generalizes the BLO-19522 branch above. That branch names one
+ // reasonless wakeReason; the author-role wake loop also covers plain PR
+ // lifecycle events (opened/reopened/synchronize/ready_for_review) that
+ // carry no review data at all, and those fell through to the feedback
+ // directive for the same reason review_requested did. Observed live:
+ // paperclip#953 had zero reviews (`gh api .../pulls/953/reviews` empty)
+ // when this directive told the woken agent "a reviewer just posted
+ // findings on YOUR pull request" and to push a follow-up commit.
+ //
+ // Gating on an ALLOWLIST of wakeReasons that structurally guarantee a
+ // review exists -- rather than adding lifecycle reasons to a denylist --
+ // is what makes this hold for the next wakeReason someone introduces: a
+ // new reason is reasonless until it is proven otherwise, which fails in
+ // the safe direction.
+ lines.push(
+ "",
+ "GitHub PR event directive:",
+ `Wake reason: ${quoteTaskScalar(prReview.wakeReason)}. No review findings are recorded for this PR yet, so do not push new commits on the strength of unconfirmed feedback. If you are this PR's author, confirm current state with \`gh pr view\` / \`gh api repos///pulls/${prReview.prNumber}/reviews\` before acting. Do NOT close the PR or self-approve. The PR's status is your responsibility this run; don't bounce to inbox-only mode.`,
+ );
} else if (prReview.prRole === "author") {
const reviewerLabel = prReview.reviewAuthorLogin ?? "A reviewer";
const stateLabel = prReview.reviewState ? prReview.reviewState.toUpperCase() : null;
diff --git a/server/src/services/issue-pull-requests.ts b/server/src/services/issue-pull-requests.ts
index 431e87e39d5..145ce2feb20 100644
--- a/server/src/services/issue-pull-requests.ts
+++ b/server/src/services/issue-pull-requests.ts
@@ -23,10 +23,15 @@ import { computeAuthoredLoc, type GithubPullFile } from "./authored-loc.js";
import {
extractPaperclipIdentifiers,
resolveLinkSourceForIdentifier,
+ resolveOwningPaperclipIdentifiers,
type PullRequestLinkSource,
} from "./paperclip-identifiers.js";
const GITHUB_HOST = "github.com";
+// Describes HOW a link was found, and is used to break ties only after
+// ownership has had its say -- see recordMergedPullRequest. It deliberately
+// ranks the branch first, which is the opposite of the ownership tier order,
+// and that disagreement is why it must not decide ownership on its own.
const LINK_SOURCE_STRENGTH: Record = {
branch_ref: 4,
title_ref: 3,
@@ -75,29 +80,71 @@ function githubApiHeaders(token?: string | null): Record {
}
/**
- * Persist the issue↔PR link for every matched company (one row per
- * (company, repo, PR) — the unique key). Within a company, if several matched
- * issues reference the PR, the strongest link source wins (branch ref first).
+ * Choose one issue per company: the issue the ownership resolver names wins;
+ * only when it names none does the strongest link source decide, then first
+ * seen. Pure, so the ownership-vs-strength precedence is testable without a DB.
+ *
+ * Deferring to ownership is load-bearing rather than tidy. This module ranks
+ * `branch_ref` above `title_ref`, while resolveOwningPaperclipIdentifiers ranks
+ * the branch LAST and documents the measurement behind it: branches get
+ * repurposed, so over 175 PRs a branch tier disagreed with the curated
+ * title/body answer 8 times and was the wrong issue in every one. Selecting on
+ * link-source strength alone therefore let a stale branch ref outrank the
+ * curated title owner -- for branch `fix/blo-1-stale`, title `Fix BLO-2`, body
+ * `Related: BLO-1`, ownership correctly chooses BLO-2 while strength alone
+ * persisted the PR against BLO-1.
+ *
+ * That was reachable only after resolveLinkSourceForIdentifier began reading the
+ * branch case-insensitively (BLO-20886 round 6): before, a lowercase branch
+ * scored `body_ref` here and lost to the title by accident. Fixing one half of
+ * the module's disagreement about ownership exposed the other half, so the two
+ * orderings are reconciled here instead of being left to coincide.
+ *
+ * `linkSource` still records which field carried the identifier -- that is
+ * descriptive provenance, and it stays accurate for whichever issue is chosen.
*/
-export async function recordMergedPullRequest(
- db: Db,
- input: RecordMergedPullRequestInput,
-): Promise {
- const fields = { branch: input.branch, title: input.title, body: input.body };
-
- // Choose one issue per company: strongest link source, then first seen.
+function selectIssuePerCompany(
+ matchedIssues: RecordMergedPullRequestInput["matchedIssues"],
+ fields: { branch?: string | null; title?: string | null; body?: string | null },
+): Map {
+ const owning = new Set(resolveOwningPaperclipIdentifiers(fields).owning);
const bestPerCompany = new Map<
string,
- { issueId: string; identifier: string; linkSource: PullRequestLinkSource }
+ { issueId: string; identifier: string; linkSource: PullRequestLinkSource; owning: boolean }
>();
- for (const issue of input.matchedIssues) {
+ for (const issue of matchedIssues) {
if (!issue.identifier) continue;
const linkSource = resolveLinkSourceForIdentifier(issue.identifier, fields) ?? "body_ref";
+ const isOwning = owning.has(issue.identifier);
const existing = bestPerCompany.get(issue.companyId);
- if (!existing || LINK_SOURCE_STRENGTH[linkSource] > LINK_SOURCE_STRENGTH[existing.linkSource]) {
- bestPerCompany.set(issue.companyId, { issueId: issue.id, identifier: issue.identifier, linkSource });
+ const better =
+ !existing ||
+ (isOwning && !existing.owning) ||
+ (isOwning === existing.owning &&
+ LINK_SOURCE_STRENGTH[linkSource] > LINK_SOURCE_STRENGTH[existing.linkSource]);
+ if (better) {
+ bestPerCompany.set(issue.companyId, {
+ issueId: issue.id,
+ identifier: issue.identifier,
+ linkSource,
+ owning: isOwning,
+ });
}
}
+ return bestPerCompany;
+}
+
+/**
+ * Persist the issue↔PR link for every matched company (one row per
+ * (company, repo, PR) — the unique key). See selectIssuePerCompany for how the
+ * single issue per company is chosen.
+ */
+export async function recordMergedPullRequest(
+ db: Db,
+ input: RecordMergedPullRequestInput,
+): Promise {
+ const fields = { branch: input.branch, title: input.title, body: input.body };
+ const bestPerCompany = selectIssuePerCompany(input.matchedIssues, fields);
const recorded: RecordedPullRequestRow[] = [];
for (const [companyId, choice] of bestPerCompany) {
@@ -382,3 +429,4 @@ export async function reconcileMergedPullRequests(
// Test-only re-exports.
export const __test_LINK_SOURCE_STRENGTH = LINK_SOURCE_STRENGTH;
+export const __test_selectIssuePerCompany = selectIssuePerCompany;
diff --git a/server/src/services/paperclip-identifiers.ts b/server/src/services/paperclip-identifiers.ts
index dc1fba21b19..9b8e5e702ad 100644
--- a/server/src/services/paperclip-identifiers.ts
+++ b/server/src/services/paperclip-identifiers.ts
@@ -53,13 +53,514 @@ export type PullRequestLinkSource = "branch_ref" | "title_ref" | "body_ref" | "r
* branch (option (A): the branchTemplate injects the issue ref into the branch
* name, so a branch match is the strongest, process-enforced signal), then
* title, then body. Returns null if none of the fields carry it.
+ *
+ * The branch tier MUST use the same case-insensitive, segment-anchored
+ * extractor that decides ownership (BLO-20886). Real branches are lowercase and
+ * PAPERCLIP_IDENTIFIER_PATTERN is uppercase-only, so classifying the branch with
+ * the broad extractor made a lowercase branch-only owner -- `cto/blo-20886-...`,
+ * the shape branchTemplate actually produces -- resolve to nothing here even
+ * though ownership had already accepted it. It then fell through to `body_ref`;
+ * if the body also mentioned a related issue in the same company, both
+ * candidates carried equal strength and insertion order decided which one a
+ * merged PR was persisted against, so the authoritative branch owner could lose
+ * to a bare `Related:` mention.
*/
export function resolveLinkSourceForIdentifier(
identifier: string,
fields: { branch?: string | null; title?: string | null; body?: string | null },
): PullRequestLinkSource | null {
- if (extractPaperclipIdentifiers(fields.branch).includes(identifier)) return "branch_ref";
+ if (extractBranchIdentifiers(fields.branch).includes(identifier)) return "branch_ref";
if (extractPaperclipIdentifiers(fields.title).includes(identifier)) return "title_ref";
if (extractPaperclipIdentifiers(fields.body).includes(identifier)) return "body_ref";
return null;
}
+
+// BLO-20886: a PR body commonly carries BOTH an owning reference (`Refs:`,
+// `Fixes:`, `Closes:`, `Resolves:`) and a `Related:` list of informational
+// backlinks with no ownership relationship to the PR at all. Every one of
+// those identifiers is an equally-weighted match under
+// extractPaperclipIdentifiers — nothing there distinguishes "the issue this
+// PR closes" from "an issue this PR happens to mention" — so a caller that
+// picks one to treat as the PR's owner (e.g. to address an author-directed
+// wake) must not just grab an arbitrary entry. Only a line that opens with
+// one of GitHub's own closing keywords or this repo's `Refs:` convention
+// counts as an ownership claim; a bare mention (including under `Related:`)
+// never does. The colon is optional -- existing PR bodies in this repo use
+// both "Closes: BLO-1" and the natural-language "Closes BLO-1 and BLO-2".
+//
+// A leading markdown list marker is also optional, and that is load-bearing
+// rather than cosmetic: .github/PULL_REQUEST_TEMPLATE.md renders its
+// "## Linked Issues or Issue Description" section as a bullet list, so the
+// repo's own house style for an owning reference is `- Refs: BLO-1`, not a
+// bare `Refs: BLO-1` line. PR #953 -- the live misroute this rule exists to
+// fix -- writes exactly `- Refs: [BLO-19132](...)`. Without the marker the
+// body tier silently matches nothing on the majority of real PR bodies and
+// every such PR fails closed to `no_owning_reference`, dropping an author
+// wake that should have been delivered to its owner. CommonMark permits up to
+// three leading spaces before a normal line; four spaces or a tab is code and
+// must not create ownership. Fenced code is excluded for the same reason.
+const OWNING_REFERENCE_LABEL_PATTERN =
+ /^ {0,3}(?:[-*+]|\d{1,3}[.)])?[ \t]*(?:fix(?:e[sd])?|clos(?:e[sd]?)|resolv(?:e[sd]?)|refs?)[ \t]*:?[ \t]+(.+)$/i;
+// Fenced code is what makes an example body safe to write: `Refs: BLO-1` inside
+// a code block DOCUMENTS the convention, it does not claim ownership. Two
+// perfectly ordinary Markdown forms defeated a root-level-only fence scanner,
+// and neither needs an adversarial author to appear:
+//
+// - A fence nested in a list item (`- ```md`). The opening line starts with a
+// list marker, so it never registered as a fence, leaving the indented
+// `Refs:` line inside it visible to the label match. This repo's own issue
+// bodies use exactly that shape to quote an example PR body.
+// - A line inside an open fence that repeats the marker with an info string
+// (``` js). CommonMark says a CLOSING fence may carry only whitespace after
+// its marker run, so that line is content -- but a length-only comparison
+// read it as the close, reopening the rest of the block to ownership.
+//
+// Both paths let an issue named only in an example capture an author-directed
+// "push a follow-up commit" wake, which is this ticket's defect reached through
+// the parser instead of the tier order. So: a fence opener is recognized after
+// an optional list marker, and a fence closes only on a same-or-longer marker
+// run followed by nothing but whitespace. Anything ambiguous keeps the fence
+// OPEN, which fails closed to "no owning reference" -- the safe direction,
+// since the caller then drops the wake or sends it to the reviewer rather than
+// guessing an owner.
+const MARKDOWN_FENCE_PATTERN = /^( {0,3}(?:[-*+]|\d{1,3}[.)])?[ \t]*)(`{3,}|~{3,})(.*)$/;
+// A CLOSING fence is a strictly narrower grammar than an opening one, and
+// reusing the opener here was a real leak: the opener tolerates a list marker
+// (`- ```) because a fence nested in a list item is ordinary Markdown, but
+// CommonMark gives a closing fence no such latitude -- it admits the marker run
+// and then nothing but whitespace. A line like `- ``` ` sitting inside an open
+// fence is therefore CONTENT, and accepting it as the close reopened the
+// remainder of the block to ownership matching, exposing a following `Refs:`
+// example exactly the way an unfenced one would.
+//
+// Leading whitespace is CAPTURED rather than bounded at three spaces, because
+// the three-space allowance is relative to the fence's CONTAINER, not to column
+// zero. A fence opened inside a nested list item (` - ```md`) has its content
+// and its closer indented to match that container, so the closer legitimately
+// carries four or more raw spaces. Bounding it at three meant such a fence never
+// closed: the scanner swallowed the rest of the body and suppressed every
+// genuinely visible `Refs:`/`Issue:` line after it, dropping an owning wake that
+// should have been delivered. The caller compares this indent against the
+// opener's own marker column -- see visibleMarkdownLines -- which keeps the
+// closer no more permissive than CommonMark allows for that container.
+const MARKDOWN_FENCE_CLOSE_PATTERN = /^([ \t]*)(`{3,}|~{3,})[ \t]*$/;
+const TRAILING_NON_OWNING_LABEL_PATTERN =
+ /(?:[;,][ \t]*|[ \t]+)(?:related|supersedes?|see[ \t]+also)[ \t]*:/i;
+
+/**
+ * Expand `text` to CommonMark columns, where a tab advances to the next
+ * 4-column tab stop. Every non-tab character counts as one column, so this also
+ * measures a prefix that includes a list marker.
+ */
+function expandedColumns(text: string): number {
+ let columns = 0;
+ for (const char of text) columns += char === "\t" ? 4 - (columns % 4) : 1;
+ return columns;
+}
+
+/**
+ * Leading indentation of `line` in CommonMark columns, where a tab advances to
+ * the next 4-column tab stop rather than counting as one character.
+ *
+ * Four columns makes an indented code block, which is why this matters here:
+ * a literal ` ` and a literal `\t` were both already treated as code, but
+ * the mixed forms that expand to the same width -- ` \t`, ` \t`, ` \t` --
+ * were not, so an ownership label inside an indented example stayed eligible
+ * to claim the PR. Counting columns instead of matching two literal prefixes
+ * closes the whole family at once. The full width is returned rather than
+ * stopping at the threshold, because the caller subtracts the enclosing list
+ * container's content column before comparing.
+ */
+function leadingIndentColumns(line: string): number {
+ let columns = 0;
+ for (const char of line) {
+ if (char === " ") columns += 1;
+ else if (char === "\t") columns += 4 - (columns % 4);
+ else break;
+ }
+ return columns;
+}
+
+// A list-item marker, used only to track how far the enclosing container
+// indents its content -- see the closing-fence allowance in
+// visibleMarkdownLines. Requires whitespace after the marker so that a setext
+// underline (`---`) or a `*emphasis*` line is not mistaken for a list.
+const MARKDOWN_LIST_ITEM_PATTERN = /^([ \t]*)([-*+]|\d{1,3}[.)])([ \t]+)/;
+
+/**
+ * Yield the lines of `body` that a human actually SEES rendered: fenced code,
+ * HTML comments, and indented code blocks are removed.
+ *
+ * Shared by both label extractors below. That sharing is the point rather than
+ * incidental tidiness -- the two extractors answer the same question ("does
+ * this body visibly declare an owner?") and any filter present in one but not
+ * the other is a hole in the weaker one. The house-reference tier originally
+ * scanned the raw Markdown, so `Issue: BLO-1` inside a fenced example, an HTML
+ * comment, or an indented block could route an author-directed
+ * "push a follow-up commit" wake to an issue that no reader of the PR would
+ * ever identify as its owner -- this module's founding defect, reached through
+ * the tier that was added last.
+ *
+ * Ambiguity fails CLOSED: an unterminated fence or `` regardless of indentation -- so the check is suppressed there, and
+ // an indented `-->` still closes the comment.
+ if (!htmlComment && indent - indentContainerColumn >= 4) continue;
+
+ // Comment state is advanced before the fence early-out below, so a
+ // multi-line comment that opens on a skipped fence line still hides its own
+ // body. It is advanced AFTER the indented-code check above, though: ` `, suppressing
+ // an owner a reader of the PR can plainly see.
+ const { visible, open } = stripHtmlComments(line, htmlComment);
+ htmlComment = open;
+
+ // Track list containers before reading the fence, so a fence opened on the
+ // same line as its marker (`- ```md`) sees its own item.
+ const listMatch = line.match(MARKDOWN_LIST_ITEM_PATTERN);
+ if (listMatch) {
+ const markerColumn = expandedColumns(listMatch[1] ?? "");
+ while (listContentColumns.length && listContentColumns[listContentColumns.length - 1]! > markerColumn) {
+ listContentColumns.pop();
+ }
+ listContentColumns.push(expandedColumns(`${listMatch[1] ?? ""}${listMatch[2] ?? ""}${listMatch[3] ?? ""}`));
+ } else if (line.trim()) {
+ // A non-blank line dedented past a container ends it. Blank lines do not:
+ // a list item may contain several blocks.
+ while (listContentColumns.length && listContentColumns[listContentColumns.length - 1]! > indent) {
+ listContentColumns.pop();
+ }
+ }
+
+ const fenceMatch = visible.match(MARKDOWN_FENCE_PATTERN);
+ if (fenceMatch?.[2]) {
+ const markerColumn = expandedColumns(fenceMatch[1] ?? "");
+ // The innermost container whose content this fence sits in.
+ const containerColumn = listContentColumns.filter((column) => column <= markerColumn).pop() ?? 0;
+ fence = {
+ marker: fenceMatch[2][0] as "`" | "~",
+ length: fenceMatch[2].length,
+ containerColumn,
+ };
+ continue;
+ }
+
+ yield visible;
+ }
+}
+
+/**
+ * Remove HTML-comment spans from one line, carrying `inComment` across lines.
+ *
+ * An HTML comment renders as nothing, so a `Refs:` line hidden inside one
+ * declares an owner that no human reading the PR can see -- recreating the
+ * wrong-assignee wake this module exists to prevent, with no visible cause to
+ * debug from. The opener and its `-->` routinely sit on different lines (the
+ * repo's own PULL_REQUEST_TEMPLATE.md ships multi-line instructional comments
+ * in every section), which is why the state has to survive the line loop.
+ * An unterminated `", index);
+ if (end === -1) return { visible, open: true };
+ open = false;
+ index = end + 3;
+ continue;
+ }
+ const start = line.indexOf("