From 12584e475a6c50ccfda91c7f8560fb209ca07567 Mon Sep 17 00:00:00 2001 From: Paperclip CTO Date: Sun, 2 Aug 2026 10:52:41 +0000 Subject: [PATCH 01/13] fix(github-webhook): route PR-review author wakes to the owning issue, not an arbitrary Related: backlink (BLO-20886) extractPaperclipIdentifiers treated every BLO-#### token in a PR's branch/title/body as equally-weighted, so the author-directed wake loop (prRole: "author") fired for the assignee of EVERY matched issue -- including ones named only under an informational `Related:` list. Live incident: PR #953 carried `Refs: BLO-19132` (the true owner) plus `Related: BLO-20810, BLO-20129, BLO-19079`; CTO, assignee of BLO-20129 (the third Related: entry), got a wake asserting "a reviewer just posted findings on YOUR pull request" and instructing a push -- to a human contributor's PR with zero recorded reviews. Fix: - paperclip-identifiers.ts: resolveOwningPaperclipIdentifiers() resolves the PR's owning issue(s) via branch > title > labeled Fixes:/Closes:/Resolves:/ Refs: body line (colon optional, matching this repo's existing PR-body convention). A bare/Related: mention is never owning. - github-webhook.ts: the author-wake loop is now restricted to the owning issue(s) only. `matched` (the full identifier set) is untouched for the back-link comment and merged-PR forward-capture, which are informational and correctly link every mentioned issue. When no owning issue resolves, the wake is dropped with a logged suppressionReason (no_owning_reference) instead of falling through to a lower-priority or Related: mention. - heartbeat.ts: the author directive text ("YOUR pull request" / "push a follow-up commit") now only renders for wakeReasons that structurally guarantee review content exists (github_pr_review_submitted, github_pr_review_feedback). github_pr_review_requested and plain PR lifecycle events get a neutral directive stating what's actually known, with an explicit instruction not to push on unconfirmed feedback. Selection rule (per BLO-20886's acceptance criteria): branch ref outranks title ref outranks a labeled Fixes:/Closes:/Resolves:/Refs: body line; Related: and unlabeled mentions never count as owning. Co-Authored-By: Claude Sonnet 5 --- server/src/__tests__/github-webhook.test.ts | 191 ++++++++++++++++++ .../heartbeat-context-summary.test.ts | 58 ++++++ server/src/routes/github-webhook.ts | 82 +++++++- server/src/services/heartbeat.ts | 72 ++++--- server/src/services/paperclip-identifiers.ts | 70 +++++++ 5 files changed, 437 insertions(+), 36 deletions(-) diff --git a/server/src/__tests__/github-webhook.test.ts b/server/src/__tests__/github-webhook.test.ts index 7a645608e374..90711f617cb6 100644 --- a/server/src/__tests__/github-webhook.test.ts +++ b/server/src/__tests__/github-webhook.test.ts @@ -55,6 +55,7 @@ import { __resetMetricsForTest, getMetricsRegistry, } from "../services/metrics.js"; +import { resolveOwningPaperclipIdentifiers } from "../services/paperclip-identifiers.js"; /** * Sum {@link GITHUB_REVIEW_REQUEST_DELIVERY_METRIC} across every `reason` @@ -118,6 +119,56 @@ describe("github-webhook pure helpers", () => { expect(__test_extractPaperclipIdentifiers("(BLO-3182): work")).toEqual(["BLO-3182"]); }); + it("resolves the PR's OWNING identifier as branch > title > labeled Fixes:/Closes:/Refs: body line, never a bare Related: mention (BLO-20886)", () => { + // Branch wins even when the body disagrees. + expect( + resolveOwningPaperclipIdentifiers({ + branch: "fix/BLO-1-thing", + title: "irrelevant", + body: "Related: BLO-2", + }), + ).toEqual({ owning: ["BLO-1"] }); + + // No branch: title wins over a labeled body line. + expect( + resolveOwningPaperclipIdentifiers({ + branch: null, + title: "fix BLO-1 thing", + body: "Refs: 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("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"); @@ -3031,6 +3082,146 @@ 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("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 794ad4638299..bbf2ae0044fb 100644 --- a/server/src/__tests__/heartbeat-context-summary.test.ts +++ b/server/src/__tests__/heartbeat-context-summary.test.ts @@ -191,6 +191,64 @@ 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). + it("does not assert 'YOUR pull request' or 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("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 event directive:"); + expect(requestedMarkdown).toContain('"github_pr_review_requested"'); + expect(requestedMarkdown).toContain("No review findings are recorded for this PR yet"); + + const openedMarkdown = buildPaperclipTaskMarkdown({ + issue: null, + prReview: { + wakeReason: "github_pr_opened", + prNumber: 35, + repoFullName: "Blockcast/paperclip", + event: "pull_request", + prRole: "author", + }, + }); + expect(openedMarkdown).not.toContain("YOUR pull request"); + expect(openedMarkdown).not.toContain("push a follow-up commit"); + }); + + // 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/routes/github-webhook.ts b/server/src/routes/github-webhook.ts index 5a63fddf82b5..ede2b2379baa 100644 --- a/server/src/routes/github-webhook.ts +++ b/server/src/routes/github-webhook.ts @@ -47,7 +47,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 { githubListIssueCommentBodies, githubPostIssueComment, @@ -466,6 +470,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; @@ -550,6 +560,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, @@ -569,6 +580,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")), @@ -744,16 +756,20 @@ 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. + 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, @@ -778,6 +794,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, @@ -829,6 +846,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, @@ -2543,7 +2561,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. @@ -2621,9 +2682,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 d034967d6917..1368c6253dbb 100644 --- a/server/src/services/heartbeat.ts +++ b/server/src/services/heartbeat.ts @@ -7942,32 +7942,54 @@ export function buildPaperclipTaskMarkdown(input: { } if (prReview.event) lines.push(`- GitHub event: ${quoteTaskScalar(prReview.event)}`); if (prReview.prRole === "author") { - const reviewerLabel = prReview.reviewAuthorLogin ?? "A reviewer"; - const stateLabel = prReview.reviewState ? prReview.reviewState.toUpperCase() : null; - lines.push( - "", - "GitHub PR review feedback directive:", - stateLabel - ? `${reviewerLabel} just submitted a review on YOUR pull request (state: ${stateLabel}).` - : `${reviewerLabel} just posted findings on YOUR pull request.`, - ); - if (prReview.reviewBody) { - lines.push("", "Latest review body:", fenceTaskText(prReview.reviewBody)); + // BLO-20886: only wakeReasons that structurally guarantee a review + // actually exists (a submitted pull_request_review, or an actionable + // review-feedback comment) may claim "a reviewer just posted findings" + // and instruct a push. github_pr_review_requested fires on a bare + // `@ally review` ASK -- no review has been posted yet -- and the + // author-role wake loop also covers plain PR lifecycle events + // (opened/reopened/synchronize/ready_for_review) that carry no review + // data at all. Observed live: PR #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. + const hasReviewContent = + prReview.wakeReason === "github_pr_review_submitted" || + prReview.wakeReason === "github_pr_review_feedback"; + if (!hasReviewContent) { + 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 { + const reviewerLabel = prReview.reviewAuthorLogin ?? "A reviewer"; + const stateLabel = prReview.reviewState ? prReview.reviewState.toUpperCase() : null; + lines.push( + "", + "GitHub PR review feedback directive:", + stateLabel + ? `${reviewerLabel} just submitted a review on YOUR pull request (state: ${stateLabel}).` + : `${reviewerLabel} just posted findings on YOUR pull request.`, + ); + if (prReview.reviewBody) { + lines.push("", "Latest review body:", fenceTaskText(prReview.reviewBody)); + } + // BLO-19067: the closing instruction must agree with the review state. + // It used to unconditionally say "push a follow-up commit addressing + // them", so an APPROVED review told the author to make an implementation + // pass that has no findings to act on. A no-op push invalidates the + // approval it just earned and restarts CI, looping for hours. + const normalizedReviewState = prReview.reviewState?.trim().toLowerCase().replace(/-/g, "_") ?? null; + const commonClosing = + "Do NOT close the PR or self-approve. The PR's status is your responsibility this run; don't bounce to inbox-only mode."; + lines.push( + "", + normalizedReviewState === "approved" + ? `Read the latest review on the PR above (use \`gh pr view\` / \`gh api\` if the body is missing here). It APPROVED your PR, so no implementation pass is required: do NOT push a no-op or invented follow-up commit, because any new push invalidates this approval and restarts CI. Act on a note only if it identifies a real defect; otherwise proceed to merge once required checks pass. ${commonClosing}` + : `Read the latest review on the PR above (use \`gh pr view\` / \`gh api\` if the body is missing here). If the findings are correct, push a follow-up commit addressing them. If they are wrong or out of scope, reply on the PR with rationale. ${commonClosing}`, + ); } - // BLO-19067: the closing instruction must agree with the review state. - // It used to unconditionally say "push a follow-up commit addressing - // them", so an APPROVED review told the author to make an implementation - // pass that has no findings to act on. A no-op push invalidates the - // approval it just earned and restarts CI, looping for hours. - const normalizedReviewState = prReview.reviewState?.trim().toLowerCase().replace(/-/g, "_") ?? null; - const commonClosing = - "Do NOT close the PR or self-approve. The PR's status is your responsibility this run; don't bounce to inbox-only mode."; - lines.push( - "", - normalizedReviewState === "approved" - ? `Read the latest review on the PR above (use \`gh pr view\` / \`gh api\` if the body is missing here). It APPROVED your PR, so no implementation pass is required: do NOT push a no-op or invented follow-up commit, because any new push invalidates this approval and restarts CI. Act on a note only if it identifies a real defect; otherwise proceed to merge once required checks pass. ${commonClosing}` - : `Read the latest review on the PR above (use \`gh pr view\` / \`gh api\` if the body is missing here). If the findings are correct, push a follow-up commit addressing them. If they are wrong or out of scope, reply on the PR with rationale. ${commonClosing}`, - ); } else { lines.push( "", diff --git a/server/src/services/paperclip-identifiers.ts b/server/src/services/paperclip-identifiers.ts index dc1fba21b194..54851249aa1a 100644 --- a/server/src/services/paperclip-identifiers.ts +++ b/server/src/services/paperclip-identifiers.ts @@ -63,3 +63,73 @@ export function resolveLinkSourceForIdentifier( 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". +const OWNING_REFERENCE_LABEL_PATTERN = + /^[ \t]*(?:fix(?:e[sd])?|clos(?:e[sd]?)|resolv(?:e[sd]?)|refs?)[ \t]*:?[ \t]+(.+)$/gim; + +/** Identifiers that appear on a labeled owning-reference line in `body` (see above). */ +export function extractOwningLabeledIdentifiers(body: string | null | undefined): string[] { + if (!body) return []; + const found = new Set(); + OWNING_REFERENCE_LABEL_PATTERN.lastIndex = 0; + for (const match of body.matchAll(OWNING_REFERENCE_LABEL_PATTERN)) { + const rest = match[1]; + if (!rest) continue; + for (const identifier of extractPaperclipIdentifiers(rest)) found.add(identifier); + } + return Array.from(found); +} + +export interface OwningIdentifierResolution { + // The PR's authoritative issue identifier(s) -- empty when none was found. + owning: string[]; +} + +/** + * Resolve the identifier(s) that OWN a PR, as opposed to ones the PR body + * merely mentions. Priority, most authoritative first: + * + * 1. branch ref -- the branchTemplate injects the issue ref into the + * branch name, a process-enforced signal (see + * resolveLinkSourceForIdentifier above). + * 2. title ref + * 3. body line(s) explicitly labeled Fixes:/Closes:/Resolves:/Refs: + * + * A bare mention anywhere else in the body -- including under a `Related:` + * label -- is never owning. Only the first non-empty tier is consulted, and + * EVERY identifier in that tier is owning: a PR legitimately closing two + * issues ("Closes BLO-1 and BLO-2") owns both, so multiplicity within the + * winning tier is not ambiguity, just multiple owners. Empty means no owning + * reference was found at all -- the caller's cue to fall back to a + * non-assignee target (e.g. the reviewer) or drop with a logged reason, + * never to widen the search to a lower-priority tier or an unlabeled mention + * (BLO-20886: doing so routed an author-directed "push a follow-up commit" + * wake to the assignee of an unrelated issue named only under `Related:`). + */ +export function resolveOwningPaperclipIdentifiers(fields: { + branch?: string | null; + title?: string | null; + body?: string | null; +}): OwningIdentifierResolution { + const tiers = [ + extractPaperclipIdentifiers(fields.branch), + extractPaperclipIdentifiers(fields.title), + extractOwningLabeledIdentifiers(fields.body), + ]; + for (const tier of tiers) { + if (tier.length > 0) return { owning: tier }; + } + return { owning: [] }; +} From 281cea18345446ee7cdff61a1edf1669df7aee8a Mon Sep 17 00:00:00 2001 From: Paperclip CTO Date: Mon, 3 Aug 2026 08:17:11 +0000 Subject: [PATCH 02/13] fix(github-webhook): match bulleted owning references, the PR template's own style (BLO-20886) Review follow-up on the owning-reference rule. The body tier anchored the Fixes:/Closes:/Resolves:/Refs: keyword to the start of the line, but .github/PULL_REQUEST_TEMPLATE.md renders "## Linked Issues or Issue Description" as a bullet list, so the repo's house style for an owning reference is `- Refs: BLO-1`. PR #953 -- the live misroute this rule exists to fix -- writes exactly `- Refs: [BLO-19132](...)`. Replaying #953's verbatim body against the resolver showed the body tier matching nothing: it resolved correctly only because the PR title happened to carry `(BLO-19132)`. Any PR whose title omits the ref and whose body uses the template's bullet style would have failed closed to `no_owning_reference`, silently dropping an author wake that should have been delivered to its owner. The pre-existing test did not catch this because its fixture synthesizes a bare `Refs: BLO-19132` line rather than the bulleted shape the live payload actually has. Allow an optional leading list marker (-, *, +, or `1.`). `Related:` still never resolves as owning, bulleted or not. Tests: github-webhook.test.ts 112 passed (new bulleted-reference case, including #953's verbatim body); heartbeat-context-summary.test.ts 83 passed; server tsc --noEmit clean. Co-Authored-By: Claude --- server/src/__tests__/github-webhook.test.ts | 42 ++++++++++++++++++++ server/src/services/paperclip-identifiers.ts | 12 +++++- 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/server/src/__tests__/github-webhook.test.ts b/server/src/__tests__/github-webhook.test.ts index 90711f617cb6..c2c66c9a123d 100644 --- a/server/src/__tests__/github-webhook.test.ts +++ b/server/src/__tests__/github-webhook.test.ts @@ -168,6 +168,48 @@ describe("github-webhook pure helpers", () => { 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("rejects payloads with bad signatures and accepts ones with good signatures", () => { const secret = "test-webhook-secret-do-not-use-in-prod"; diff --git a/server/src/services/paperclip-identifiers.ts b/server/src/services/paperclip-identifiers.ts index 54851249aa1a..ee8533f3d7ef 100644 --- a/server/src/services/paperclip-identifiers.ts +++ b/server/src/services/paperclip-identifiers.ts @@ -76,8 +76,18 @@ export function resolveLinkSourceForIdentifier( // 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. const OWNING_REFERENCE_LABEL_PATTERN = - /^[ \t]*(?:fix(?:e[sd])?|clos(?:e[sd]?)|resolv(?:e[sd]?)|refs?)[ \t]*:?[ \t]+(.+)$/gim; + /^[ \t]*(?:[-*+]|\d{1,3}[.)])?[ \t]*(?:fix(?:e[sd])?|clos(?:e[sd]?)|resolv(?:e[sd]?)|refs?)[ \t]*:?[ \t]+(.+)$/gim; /** Identifiers that appear on a labeled owning-reference line in `body` (see above). */ export function extractOwningLabeledIdentifiers(body: string | null | undefined): string[] { From 25fd7ddf0b9b42516c8c5ea344a26cf429fe434a Mon Sep 17 00:00:00 2001 From: Paperclip CTO Date: Mon, 3 Aug 2026 08:32:29 +0000 Subject: [PATCH 03/13] fix(github-webhook): demote the branch tier to a case-insensitive last resort (BLO-20886) Second review follow-up, both halves measured against the 175 PRs active in Blockcast/paperclip over the trailing 7 days rather than assumed. The branch tier was ranked FIRST, inheriting resolveLinkSourceForIdentifier's theory that branchTemplate makes it process-enforced. Two findings falsify that: 1. It never fires. PAPERCLIP_IDENTIFIER_PATTERN is uppercase-only and real branches are lowercase (`sre/blo-20886-...`), so tier 1 matched on 1 of 175 PRs. That silence is why 24 of them resolved to no owner and failed closed, dropping author wakes they should have received -- PRs that name their issue as `Issue: ` or `Paperclip task: `, labels outside the closing-keyword set, while carrying the correct ref in the branch. 2. Branches go stale. Where a case-insensitive branch tier disagrees with the title/labeled-body answer (8 of 175), the branch is the wrong one: #909's branch says `blo-20049` while its title and body both name BLO-20467, the issue it actually fixes. Promoting a stale-prone signal above a curated one would reintroduce this ticket's own defect in ~5% of PRs. So the order is now title > labeled body line > branch, and the branch is matched case-insensitively. Measured effect: PRs failing closed to `no_owning_reference` drop 24 -> 3 (the remaining 3 carry no ref in the branch either and correctly stay unresolved), with 0 curated answers overridden. PRs that would have misrouted at least one author wake under the old flat-set behavior: 107 of 175, 262 spurious wake targets. Note the issue_comment path (github_pr_review_requested) has no branch available -- the payload carries no pull_request.head.ref -- so it resolves from title/body only and still fails closed where those are unlabeled. Recovering it needs a PR fetch in the webhook path; left as follow-up. Tests: github-webhook.test.ts 112 passed (precedence test rewritten for the new order, incl. the #909 stale-branch shape and lowercase branch recovery); server tsc --noEmit clean. Co-Authored-By: Claude --- server/src/__tests__/github-webhook.test.ts | 43 ++++++++++++++++---- server/src/services/paperclip-identifiers.ts | 37 ++++++++++++++--- 2 files changed, 65 insertions(+), 15 deletions(-) diff --git a/server/src/__tests__/github-webhook.test.ts b/server/src/__tests__/github-webhook.test.ts index c2c66c9a123d..b52dcbdafc7c 100644 --- a/server/src/__tests__/github-webhook.test.ts +++ b/server/src/__tests__/github-webhook.test.ts @@ -119,22 +119,47 @@ describe("github-webhook pure helpers", () => { expect(__test_extractPaperclipIdentifiers("(BLO-3182): work")).toEqual(["BLO-3182"]); }); - it("resolves the PR's OWNING identifier as branch > title > labeled Fixes:/Closes:/Refs: body line, never a bare Related: mention (BLO-20886)", () => { - // Branch wins even when the body disagrees. + 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-1-thing", - title: "irrelevant", - body: "Related: BLO-2", + branch: "fix/BLO-9-stale-branch", + title: "fix BLO-1 thing", + body: "Refs: BLO-2", }), ).toEqual({ owning: ["BLO-1"] }); - // No branch: title wins over a labeled body line. + // 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: null, - title: "fix BLO-1 thing", - body: "Refs: BLO-2", + 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"] }); diff --git a/server/src/services/paperclip-identifiers.ts b/server/src/services/paperclip-identifiers.ts index ee8533f3d7ef..81adc2ada15e 100644 --- a/server/src/services/paperclip-identifiers.ts +++ b/server/src/services/paperclip-identifiers.ts @@ -111,11 +111,9 @@ export interface OwningIdentifierResolution { * Resolve the identifier(s) that OWN a PR, as opposed to ones the PR body * merely mentions. Priority, most authoritative first: * - * 1. branch ref -- the branchTemplate injects the issue ref into the - * branch name, a process-enforced signal (see - * resolveLinkSourceForIdentifier above). - * 2. title ref - * 3. body line(s) explicitly labeled Fixes:/Closes:/Resolves:/Refs: + * 1. title ref + * 2. body line(s) explicitly labeled Fixes:/Closes:/Resolves:/Refs: + * 3. branch ref, case-insensitively -- LAST resort, see below. * * A bare mention anywhere else in the body -- including under a `Related:` * label -- is never owning. Only the first non-empty tier is consulted, and @@ -127,6 +125,30 @@ export interface OwningIdentifierResolution { * never to widen the search to a lower-priority tier or an unlabeled mention * (BLO-20886: doing so routed an author-directed "push a follow-up commit" * wake to the assignee of an unrelated issue named only under `Related:`). + * + * On the branch tier being LAST and case-insensitive, both of which are + * measured rather than assumed. resolveLinkSourceForIdentifier above ranks + * the branch FIRST on the theory that branchTemplate injects the ref, making + * it process-enforced. Two things falsify that here: + * + * - PAPERCLIP_IDENTIFIER_PATTERN is uppercase-only and real branches are + * lowercase (`sre/blo-20886-...`), so an uppercase branch tier is inert: + * across 175 PRs active in the trailing 7 days it fired for 1. That + * silence is what made 24 of those PRs resolve to no owner at all and + * fail closed, losing an author wake they should have received. Matching + * case-insensitively recovers 21 of the 24; the remaining 3 carry no ref + * in the branch either and correctly stay unresolved. + * - Branches get repurposed, so a branch ref goes stale while the title + * stays current. Over the same 175 PRs a case-insensitive branch tier + * agrees with the title/labeled-body answer 142 times and disagrees 8 -- + * and in the disagreements the branch is the wrong one (#909's branch + * says `blo-20049` while both its title and its body name BLO-20467, the + * issue it actually fixes). Ranking a stale-prone signal above a curated + * one would reintroduce this ticket's own defect, misrouting an + * author-directed wake in ~5% of cases. + * + * So the branch is consulted only when nothing curated resolved, where its + * choices are 21 recovered wakes against 0 overridden answers. */ export function resolveOwningPaperclipIdentifiers(fields: { branch?: string | null; @@ -134,9 +156,12 @@ export function resolveOwningPaperclipIdentifiers(fields: { body?: string | null; }): OwningIdentifierResolution { const tiers = [ - extractPaperclipIdentifiers(fields.branch), extractPaperclipIdentifiers(fields.title), extractOwningLabeledIdentifiers(fields.body), + // Uppercased so a conventional lowercase branch (`sre/blo-20886-fix`) + // matches the uppercase-only identifier pattern. Safe to normalize here + // because a branch ref carries no prose that case could disambiguate. + extractPaperclipIdentifiers(fields.branch?.toUpperCase()), ]; for (const tier of tiers) { if (tier.length > 0) return { owning: tier }; From a231b404b8cbfb6a013899a21c239837a5cc2657 Mon Sep 17 00:00:00 2001 From: Omar Ramadan Date: Tue, 4 Aug 2026 22:34:26 +0000 Subject: [PATCH 04/13] fix(github-webhook): bound owning-reference parsing --- server/src/__tests__/github-webhook.test.ts | 24 +++++++++++++++++ server/src/services/paperclip-identifiers.ts | 28 ++++++++++++++++---- 2 files changed, 47 insertions(+), 5 deletions(-) diff --git a/server/src/__tests__/github-webhook.test.ts b/server/src/__tests__/github-webhook.test.ts index b52dcbdafc7c..c0dd0e99c28c 100644 --- a/server/src/__tests__/github-webhook.test.ts +++ b/server/src/__tests__/github-webhook.test.ts @@ -235,6 +235,30 @@ describe("github-webhook pure helpers", () => { ).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("rejects payloads with bad signatures and accepts ones with good signatures", () => { const secret = "test-webhook-secret-do-not-use-in-prod"; diff --git a/server/src/services/paperclip-identifiers.ts b/server/src/services/paperclip-identifiers.ts index 81adc2ada15e..1b6e5035dbeb 100644 --- a/server/src/services/paperclip-identifiers.ts +++ b/server/src/services/paperclip-identifiers.ts @@ -85,17 +85,35 @@ export function resolveLinkSourceForIdentifier( // 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. +// 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 = - /^[ \t]*(?:[-*+]|\d{1,3}[.)])?[ \t]*(?:fix(?:e[sd])?|clos(?:e[sd]?)|resolv(?:e[sd]?)|refs?)[ \t]*:?[ \t]+(.+)$/gim; + /^ {0,3}(?:[-*+]|\d{1,3}[.)])?[ \t]*(?:fix(?:e[sd])?|clos(?:e[sd]?)|resolv(?:e[sd]?)|refs?)[ \t]*:?[ \t]+(.+)$/i; +const MARKDOWN_FENCE_PATTERN = /^ {0,3}(`{3,}|~{3,})/; +const TRAILING_NON_OWNING_LABEL_PATTERN = + /(?:[;,][ \t]*|[ \t]+)(?:related|supersedes?|see[ \t]+also)[ \t]*:/i; /** Identifiers that appear on a labeled owning-reference line in `body` (see above). */ export function extractOwningLabeledIdentifiers(body: string | null | undefined): string[] { if (!body) return []; const found = new Set(); - OWNING_REFERENCE_LABEL_PATTERN.lastIndex = 0; - for (const match of body.matchAll(OWNING_REFERENCE_LABEL_PATTERN)) { - const rest = match[1]; + let fence: { marker: "`" | "~"; length: number } | null = null; + for (const line of body.split(/\r?\n/)) { + const fenceMatch = line.match(MARKDOWN_FENCE_PATTERN); + if (fenceMatch?.[1]) { + const marker = fenceMatch[1][0] as "`" | "~"; + if (!fence) { + fence = { marker, length: fenceMatch[1].length }; + } else if (marker === fence.marker && fenceMatch[1].length >= fence.length) { + fence = null; + } + continue; + } + if (fence || line.startsWith("\t") || line.startsWith(" ")) continue; + + const match = line.match(OWNING_REFERENCE_LABEL_PATTERN); + const rest = match?.[1]?.split(TRAILING_NON_OWNING_LABEL_PATTERN, 1)[0]; if (!rest) continue; for (const identifier of extractPaperclipIdentifiers(rest)) found.add(identifier); } From 69ec61d3d70f21f4a5020d7029177900ac66c9a8 Mon Sep 17 00:00:00 2001 From: Omar Ramadan Date: Tue, 4 Aug 2026 20:36:50 -0700 Subject: [PATCH 05/13] fix(apps): keep empty review queues fresh --- ui/src/pages/apps/ReviewQueueCard.test.tsx | 19 +++++++++++++++---- ui/src/pages/apps/ReviewQueueCard.tsx | 18 +++++++++++++++--- 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/ui/src/pages/apps/ReviewQueueCard.test.tsx b/ui/src/pages/apps/ReviewQueueCard.test.tsx index bca33f7fa807..90cf8239b02f 100644 --- a/ui/src/pages/apps/ReviewQueueCard.test.tsx +++ b/ui/src/pages/apps/ReviewQueueCard.test.tsx @@ -263,8 +263,11 @@ describe("ReviewQueueCard", () => { }); }); - it("refreshes an empty mounted queue so externally-created pending requests appear", async () => { - listActionRequestsMock.mockResolvedValue({ actionRequests: [] }); + it("keeps refreshing an empty mounted queue so externally-created pending requests appear", async () => { + let pendingCreated = false; + listActionRequestsMock.mockImplementation(async () => ({ + actionRequests: pendingCreated ? [pendingRequest()] : [], + })); await render(); @@ -273,11 +276,19 @@ describe("ReviewQueueCard", () => { expect(document.body.textContent).toContain("Nothing is waiting for your OK right now."); }); - listActionRequestsMock.mockResolvedValue({ actionRequests: [pendingRequest()] }); + await vi.waitFor( + () => { + expect(listActionRequestsMock.mock.calls.length).toBeGreaterThanOrEqual(3); + expect(document.body.textContent).toContain("Nothing is waiting for your OK right now."); + }, + { timeout: 3_500 }, + ); + + pendingCreated = true; await vi.waitFor( () => { - expect(listActionRequestsMock).toHaveBeenCalledTimes(3); + expect(listActionRequestsMock.mock.calls.length).toBeGreaterThanOrEqual(4); expect(buttonContaining("Allow once")).toBeTruthy(); }, { timeout: 3_500 }, diff --git a/ui/src/pages/apps/ReviewQueueCard.tsx b/ui/src/pages/apps/ReviewQueueCard.tsx index dbeea750a0d2..2fa5657168c1 100644 --- a/ui/src/pages/apps/ReviewQueueCard.tsx +++ b/ui/src/pages/apps/ReviewQueueCard.tsx @@ -42,12 +42,16 @@ export function ReviewQueueCard({ enabled: !!selectedCompanyId, staleTime: 0, refetchOnMount: false, - refetchInterval: 20_000, + refetchInterval: (state) => { + const visibleItems = filterActionRequests(state.state.data?.actionRequests, connectionId); + return emptyState !== "hidden" && visibleItems.length === 0 + ? VISIBLE_EMPTY_QUEUE_REFRESH_MS + : 20_000; + }, }); const items = useMemo(() => { - const all = query.data?.actionRequests ?? []; - return connectionId ? all.filter((item) => item.connectionId === connectionId) : all; + return filterActionRequests(query.data?.actionRequests, connectionId); }, [query.data, connectionId]); useEffect(() => { @@ -96,6 +100,14 @@ export function ReviewQueueCard({ ); } +function filterActionRequests( + actionRequests: ToolActionRequestListItem[] | undefined, + connectionId?: string, +) { + const all = actionRequests ?? []; + return connectionId ? all.filter((item) => item.connectionId === connectionId) : all; +} + function ReviewRow({ companyId, item }: { companyId: string; item: ToolActionRequestListItem }) { const queryClient = useQueryClient(); const { pushToast } = useToast(); From 8a106ea58e889f27fceee816d8dfca6051bc0396 Mon Sep 17 00:00:00 2001 From: Paperclip CTO Date: Tue, 4 Aug 2026 00:51:29 +0000 Subject: [PATCH 06/13] fix(github-webhook): resolve PR owner from a house-reference label when no branch tier is available (BLO-21312) github_pr_review_requested arrives via issue_comment, whose payload carries no pull_request.head.ref, so the BLO-20886 case-insensitive branch tier is structurally unreachable on that path -- a PR naming its owner only via Issue:/Paperclip task:/Paperclip issue:/Paperclip QA task: (real shapes on Add a fourth, lowest-priority tier to resolveOwningPaperclipIdentifiers for these house labels. Ranked below both the closing-keyword and branch tiers so pull_request-sourced resolution is unchanged; it only activates when title, closing keyword, and branch (when available) are all empty. Co-Authored-By: Claude Sonnet 5 --- server/src/__tests__/github-webhook.test.ts | 149 +++++++++++++++++++ server/src/routes/github-webhook.ts | 4 +- server/src/services/paperclip-identifiers.ts | 64 +++++++- 3 files changed, 211 insertions(+), 6 deletions(-) diff --git a/server/src/__tests__/github-webhook.test.ts b/server/src/__tests__/github-webhook.test.ts index c0dd0e99c28c..ab173e7df832 100644 --- a/server/src/__tests__/github-webhook.test.ts +++ b/server/src/__tests__/github-webhook.test.ts @@ -259,6 +259,72 @@ describe("github-webhook pure helpers", () => { ).toEqual({ owning: [] }); }); + 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"] }); + }); + it("rejects payloads with bad signatures and accepts ones with good signatures", () => { const secret = "test-webhook-secret-do-not-use-in-prod"; @@ -3313,6 +3379,89 @@ describeEmbeddedPostgres("github-webhook route", () => { 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("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/routes/github-webhook.ts b/server/src/routes/github-webhook.ts index ede2b2379baa..e8f20a62da03 100644 --- a/server/src/routes/github-webhook.ts +++ b/server/src/routes/github-webhook.ts @@ -761,7 +761,9 @@ function resolveEventContext( // 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. + // 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(issueTitle, issueBody, commentBody), diff --git a/server/src/services/paperclip-identifiers.ts b/server/src/services/paperclip-identifiers.ts index 1b6e5035dbeb..c3ab60c496ec 100644 --- a/server/src/services/paperclip-identifiers.ts +++ b/server/src/services/paperclip-identifiers.ts @@ -120,6 +120,42 @@ export function extractOwningLabeledIdentifiers(body: string | null | undefined) return Array.from(found); } +// BLO-21312: `github_pr_review_requested` -- the exact wake reason BLO-20886 +// was filed over -- arrives via `issue_comment` (an `@ally review` mention), +// not `pull_request`, and an issue_comment payload carries no +// `pull_request.head.ref`: the branch tier below is structurally unavailable +// on this path, not merely unmeasured, so the "match case-insensitively" +// fix for the branch tier cannot reach it. Real PR bodies on this path +// (#931, #963, #976, #916) name their owner with one of this repo's own +// non-closing house labels instead of a GitHub closing keyword: `Issue:`, +// `Paperclip task:`, `Paperclip issue:`, `Paperclip QA task:`. +// +// These are weaker ownership claims than a closing keyword -- an author +// writing "Issue: filed a related bug, see BLO-1" is not asserting the PR +// closes BLO-1 the way "Fixes: BLO-1" does -- so this tier is ranked below +// BOTH the closing-keyword tier and the branch tier in +// resolveOwningPaperclipIdentifiers. On `pull_request` events, which do +// carry a branch, the already-measured branch tier still wins whenever it +// resolves; this tier only ever activates when title, closing keyword, AND +// branch are all empty -- on `issue_comment` events, where fields.branch is +// never populated, that reduces to "title and closing keyword are both +// empty", making this the issue_comment path's practical last resort. +const HOUSE_REFERENCE_LABEL_PATTERN = + /^[ \t]*(?:[-*+]|\d{1,3}[.)])?[ \t]*(?:paperclip[ \t]+qa[ \t]+task|paperclip[ \t]+task|paperclip[ \t]+issue|issue)[ \t]*:?[ \t]+(.+)$/gim; + +/** Identifiers that appear on a labeled house-reference line in `body` (see above). */ +export function extractHouseReferenceLabeledIdentifiers(body: string | null | undefined): string[] { + if (!body) return []; + const found = new Set(); + HOUSE_REFERENCE_LABEL_PATTERN.lastIndex = 0; + for (const match of body.matchAll(HOUSE_REFERENCE_LABEL_PATTERN)) { + const rest = match[1]; + if (!rest) continue; + for (const identifier of extractPaperclipIdentifiers(rest)) found.add(identifier); + } + return Array.from(found); +} + export interface OwningIdentifierResolution { // The PR's authoritative issue identifier(s) -- empty when none was found. owning: string[]; @@ -131,7 +167,11 @@ export interface OwningIdentifierResolution { * * 1. title ref * 2. body line(s) explicitly labeled Fixes:/Closes:/Resolves:/Refs: - * 3. branch ref, case-insensitively -- LAST resort, see below. + * 3. branch ref, case-insensitively -- LAST resort among the two + * process-signal tiers, see below. + * 4. body line(s) explicitly labeled with a non-closing house reference + * (Issue:/Paperclip task:/Paperclip issue:/Paperclip QA task:) -- LAST + * resort overall, see below (BLO-21312). * * A bare mention anywhere else in the body -- including under a `Related:` * label -- is never owning. Only the first non-empty tier is consulted, and @@ -144,10 +184,11 @@ export interface OwningIdentifierResolution { * (BLO-20886: doing so routed an author-directed "push a follow-up commit" * wake to the assignee of an unrelated issue named only under `Related:`). * - * On the branch tier being LAST and case-insensitive, both of which are - * measured rather than assumed. resolveLinkSourceForIdentifier above ranks - * the branch FIRST on the theory that branchTemplate injects the ref, making - * it process-enforced. Two things falsify that here: + * On the branch tier being LAST among title/keyword/branch and case- + * insensitive, both of which are measured rather than assumed. + * resolveLinkSourceForIdentifier above ranks the branch FIRST on the theory + * that branchTemplate injects the ref, making it process-enforced. Two + * things falsify that here: * * - PAPERCLIP_IDENTIFIER_PATTERN is uppercase-only and real branches are * lowercase (`sre/blo-20886-...`), so an uppercase branch tier is inert: @@ -167,6 +208,18 @@ export interface OwningIdentifierResolution { * * So the branch is consulted only when nothing curated resolved, where its * choices are 21 recovered wakes against 0 overridden answers. + * + * The house-reference tier (BLO-21312) exists for `github_pr_review_requested` + * wakes that arrive via `issue_comment` rather than `pull_request`: that + * payload shape carries no `pull_request.head.ref`, so `fields.branch` is + * never populated and tier 3 is structurally empty regardless of case- + * insensitivity. Real PR bodies on that path (#931, #963, #976, #916) name + * their owner with a non-closing house label instead of a GitHub closing + * keyword. That is a weaker ownership claim than `Fixes:`/`Closes:` -- it has + * not been measured the way the branch tier was -- so it is ranked below the + * branch tier too: for `pull_request` events (which do carry a branch), the + * measured branch tier still wins whenever it resolves, and this tier only + * activates when title, closing keyword, AND branch are all empty. */ export function resolveOwningPaperclipIdentifiers(fields: { branch?: string | null; @@ -180,6 +233,7 @@ export function resolveOwningPaperclipIdentifiers(fields: { // matches the uppercase-only identifier pattern. Safe to normalize here // because a branch ref carries no prose that case could disambiguate. extractPaperclipIdentifiers(fields.branch?.toUpperCase()), + extractHouseReferenceLabeledIdentifiers(fields.body), ]; for (const tier of tiers) { if (tier.length > 0) return { owning: tier }; From baf6406fb00a5dfca1ba98e0e364e556aa3f76f2 Mon Sep 17 00:00:00 2001 From: Paperclip CTO Date: Tue, 4 Aug 2026 10:57:21 +0000 Subject: [PATCH 07/13] fix(github-webhook): require colon and stop at same-line labels in the house-reference tier (BLO-21312) Ally review on #999 flagged two smuggling paths in the new house-reference fallback: the optional colon let ordinary "Issue ..." prose match as an ownership label, and the captured remainder let a same-line `Related:` mention ride along as owning. Require the colon and truncate the captured value at any secondary label on the same line. --- server/src/__tests__/github-webhook.test.ts | 32 ++++++++++++++++++++ server/src/services/paperclip-identifiers.ts | 32 ++++++++++++++++++-- 2 files changed, 62 insertions(+), 2 deletions(-) diff --git a/server/src/__tests__/github-webhook.test.ts b/server/src/__tests__/github-webhook.test.ts index ab173e7df832..a44696865257 100644 --- a/server/src/__tests__/github-webhook.test.ts +++ b/server/src/__tests__/github-webhook.test.ts @@ -323,6 +323,38 @@ describe("github-webhook pure helpers", () => { 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"] }); }); diff --git a/server/src/services/paperclip-identifiers.ts b/server/src/services/paperclip-identifiers.ts index c3ab60c496ec..f3ad0e3e0774 100644 --- a/server/src/services/paperclip-identifiers.ts +++ b/server/src/services/paperclip-identifiers.ts @@ -140,8 +140,34 @@ export function extractOwningLabeledIdentifiers(body: string | null | undefined) // branch are all empty -- on `issue_comment` events, where fields.branch is // never populated, that reduces to "title and closing keyword are both // empty", making this the issue_comment path's practical last resort. +// +// Unlike OWNING_REFERENCE_LABEL_PATTERN above, the colon here is MANDATORY, +// not optional. The closing keywords (`Fixes`/`Closes`/`Resolves`/`Refs`) are +// verbs that only ever start a labeled reference line, so "Closes BLO-1" is +// unambiguous natural language. `Issue` is an ordinary noun that also starts +// ordinary sentences -- "Issue filed a related bug, see BLO-1" and "Issue +// description for BLO-2" are real English, not an ownership label -- so an +// optional colon here would route a branchless wake off of prose that never +// claimed ownership. Requiring the colon keeps this tier fail-closed on +// prose while still matching every observed house-label shape, all of which +// use a colon. const HOUSE_REFERENCE_LABEL_PATTERN = - /^[ \t]*(?:[-*+]|\d{1,3}[.)])?[ \t]*(?:paperclip[ \t]+qa[ \t]+task|paperclip[ \t]+task|paperclip[ \t]+issue|issue)[ \t]*:?[ \t]+(.+)$/gim; + /^[ \t]*(?:[-*+]|\d{1,3}[.)])?[ \t]*(?:paperclip[ \t]+qa[ \t]+task|paperclip[ \t]+task|paperclip[ \t]+issue|issue)[ \t]*:[ \t]+(.+)$/gim; + +// BLO-21312: a house-reference line can still carry a second, distinctly +// labeled reference later on the SAME line -- `Issue: BLO-1; Related: +// BLO-2` -- and a naive "extract every identifier in the captured remainder" +// would resolve both, waking the assignee of BLO-2 even though it is +// explicitly marked non-owning right there on the line. The captured +// remainder is truncated at the first such secondary label so only the house +// label's own direct reference value is ever treated as owning. +const TRAILING_LABEL_REFERENCE_PATTERN = + /[;,|][ \t]*(?:fix(?:e[sd])?|clos(?:e[sd]?)|resolv(?:e[sd]?)|refs?|relate[ds]?|see[ \t]+also|paperclip[ \t]+qa[ \t]+task|paperclip[ \t]+task|paperclip[ \t]+issue|issue)[ \t]*:/i; + +function stripTrailingLabelReference(text: string): string { + const match = text.match(TRAILING_LABEL_REFERENCE_PATTERN); + return match && typeof match.index === "number" ? text.slice(0, match.index) : text; +} /** Identifiers that appear on a labeled house-reference line in `body` (see above). */ export function extractHouseReferenceLabeledIdentifiers(body: string | null | undefined): string[] { @@ -151,7 +177,9 @@ export function extractHouseReferenceLabeledIdentifiers(body: string | null | un for (const match of body.matchAll(HOUSE_REFERENCE_LABEL_PATTERN)) { const rest = match[1]; if (!rest) continue; - for (const identifier of extractPaperclipIdentifiers(rest)) found.add(identifier); + const directValue = stripTrailingLabelReference(rest); + if (!directValue) continue; + for (const identifier of extractPaperclipIdentifiers(directValue)) found.add(identifier); } return Array.from(found); } From 7c689686ac5a365f3282ef4b33859ff15cf99282 Mon Sep 17 00:00:00 2001 From: "allyblockcast[bot]" <290875700+allyblockcast[bot]@users.noreply.github.com> Date: Wed, 5 Aug 2026 16:46:12 +0000 Subject: [PATCH 08/13] fix(github-webhook): close two Markdown escapes in owning-reference parsing (BLO-20886) Ally's round-4 review found the exact-head scanner still reachable by two ordinary Markdown forms. Both reproduce, and both let an issue named only in an example capture an author-directed "push a follow-up commit" wake -- this ticket's own defect, reached through the parser rather than the tier order. - A fence nested in a list item (`- ```md`) never opened a fence, because the opening line starts with the list marker. The indented `Refs:` line inside the example stayed visible to the label match. Fence openers are now recognized after an optional list marker. - A line inside an open fence repeating the marker with an info string (``` js) closed it, because the comparison was length-only. CommonMark allows only whitespace after a closing fence's marker run, so that line is content; treating it as the close reopened the rest of the block. Closing now requires a same-or-longer run followed by whitespace only. - A multi-line HTML comment could declare an owner. A `Refs:` line between `` resolved its identifier while the PR rendered no ownership declaration at all -- a misroute with no visible cause to debug from, and the repo's own PULL_REQUEST_TEMPLATE.md ships multi-line comments in every section. Comments are now stripped before the label match, with state carried across lines. Ambiguity fails closed in every case: an unterminated fence or `` 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"; diff --git a/server/src/services/paperclip-identifiers.ts b/server/src/services/paperclip-identifiers.ts index f3ad0e3e0774..b5fd09c627f8 100644 --- a/server/src/services/paperclip-identifiers.ts +++ b/server/src/services/paperclip-identifiers.ts @@ -90,29 +90,102 @@ export function resolveLinkSourceForIdentifier( // 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; -const MARKDOWN_FENCE_PATTERN = /^ {0,3}(`{3,}|~{3,})/; +// 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,})(.*)$/; const TRAILING_NON_OWNING_LABEL_PATTERN = /(?:[;,][ \t]*|[ \t]+)(?:related|supersedes?|see[ \t]+also)[ \t]*:/i; +/** + * 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(""].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("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("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 @@ -1231,6 +1341,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", @@ -2051,7 +2188,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" }, }; @@ -2107,7 +2251,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" }, }; @@ -3559,6 +3704,52 @@ describeEmbeddedPostgres("github-webhook route", () => { 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/routes/github-webhook.ts b/server/src/routes/github-webhook.ts index e8f20a62da03..00f1451cb6d2 100644 --- a/server/src/routes/github-webhook.ts +++ b/server/src/routes/github-webhook.ts @@ -535,7 +535,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: { diff --git a/server/src/services/paperclip-identifiers.ts b/server/src/services/paperclip-identifiers.ts index b5fd09c627f8..ea4a024ab4e0 100644 --- a/server/src/services/paperclip-identifiers.ts +++ b/server/src/services/paperclip-identifiers.ts @@ -113,9 +113,90 @@ const OWNING_REFERENCE_LABEL_PATTERN = // 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 up to three +// leading spaces, the marker run, 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. +const MARKDOWN_FENCE_CLOSE_PATTERN = /^ {0,3}(`{3,}|~{3,})[ \t]*$/; const TRAILING_NON_OWNING_LABEL_PATTERN = /(?:[;,][ \t]*|[ \t]+)(?:related|supersedes?|see[ \t]+also)[ \t]*:/i; +/** + * 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. Stops early: nothing above the threshold + * needs a precise width. + */ +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; + if (columns >= 4) return columns; + } + return columns; +} + +/** + * 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 `". Classified as code first now. The comment check is suppressed while a comment is open, because indentation does not create a code block inside an HTML block, so an indented "-->" must still close it. Comment state is still advanced ahead of the FENCE early-out, preserving the case that ordering was written for. Every expectation was checked against marked 16.4.2 rather than read off the spec, matching the bar set by the existing fence tests. Both fixes were reverted individually to confirm each is load-bearing. server/src/__tests__/github-webhook.test.ts: 133 passed, including the BLO-20886 negative assertions unchanged -- a Related: mention still never becomes owning. --- server/src/__tests__/github-webhook.test.ts | 69 ++++++++++++++++++++ server/src/services/paperclip-identifiers.ts | 34 +++++++--- 2 files changed, 94 insertions(+), 9 deletions(-) diff --git a/server/src/__tests__/github-webhook.test.ts b/server/src/__tests__/github-webhook.test.ts index 5c10c5403f14..a50bf8a1650d 100644 --- a/server/src/__tests__/github-webhook.test.ts +++ b/server/src/__tests__/github-webhook.test.ts @@ -380,6 +380,75 @@ describe("github-webhook pure helpers", () => { ).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
diff --git a/server/src/services/paperclip-identifiers.ts b/server/src/services/paperclip-identifiers.ts
index 6666c994b7a2..9b8e5e702ad1 100644
--- a/server/src/services/paperclip-identifiers.ts
+++ b/server/src/services/paperclip-identifiers.ts
@@ -167,8 +167,9 @@ function expandedColumns(text: string): number {
  * 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. Stops early: nothing above the threshold
- * needs a precise width.
+ * 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;
@@ -176,7 +177,6 @@ function leadingIndentColumns(line: string): number {
     if (char === " ") columns += 1;
     else if (char === "\t") columns += 4 - (columns % 4);
     else break;
-    if (columns >= 4) return columns;
   }
   return columns;
 }
@@ -228,16 +228,32 @@ function* visibleMarkdownLines(body: string): Generator {
       continue;
     }
 
-    // Comment state is advanced before any early-out below, so a multi-line
-    // comment that opens on a skipped line still hides its own body.
-    const { visible, open } = stripHtmlComments(line, htmlComment);
-    htmlComment = open;
-
     // Indentation is read from the raw line because that is what determines
     // CommonMark block structure, and checked before the fence so an indented
     // ``` is code rather than a fence opener.
+    //
+    // Four columns makes an indented code block, but the threshold is measured
+    // from the enclosing CONTAINER rather than from column zero -- the same
+    // relativity the closing fence already honours. A list continuation like
+    // `- item` / ` \tRefs: BLO-1` expands to four raw columns while sitting only
+    // two columns inside the item's content, so measuring absolutely discarded
+    // an ordinary visible paragraph as code and lost the owner it declared.
     const indent = leadingIndentColumns(line);
-    if (indent >= 4) continue;
+    const indentContainerColumn = listContentColumns.filter((column) => column <= indent).pop() ?? 0;
+    // Indentation only opens a code block in a leaf-block position. Inside an
+    // open HTML comment there is no such position -- the block runs to its
+    // `-->` 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.