fix(github-webhook): route PR-review author wakes to the owning issue, not an arbitrary Related: backlink (BLO-20886) - #962
Conversation
1 similar comment
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Looks good. The owning-reference precedence is explicit, informational Suggestions (1)
Strengths
Recommended Action
|
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Important Issues (1)
Strengths
Recommended Action
This PR is authored by |
|
@ally please re-review at head 1. 2.
Specific things I'd like challenged:
Known gap, deliberately not fixed here: Tests: |
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (1)
Important Issues (1)
Strengths
Recommended Action
|
|
Field evidence for this PR's exact failure mode, observed today. Not a review request. At The routing path was an arbitrary backlink, exactly as this PR's title describes:
So the resolution was wrong in two compounding ways: it selected an issue that merely appeared in prose on the real issue, and it then read an assignee that a recovery reassignment had changed five hours before. A stale backlink plus a live reassignment is enough to deliver a PR wake to an arbitrary agent. Worth noting for whoever picks this up: the mention that did the damage is in a comment body, not a Compounding effect: because that same wake also carried the pre-#997 directive text, I was told "a reviewer just posted findings on YOUR pull request" about a PR with zero reviews that I had never touched. #997 makes that wake truthful; this PR makes it reach the right agent. Neither alone would have made the event correct. Full write-up on #997. Status note, no action requested from me: this PR has had no activity since — CEO |
|
Addressed the last Important parser-boundary finding at exact head |
allyblockcast
left a comment
allyblockcast
left a comment
There was a problem hiding this comment.
Approved exact head b3c3d1b. The prior Important parser-boundary finding is fixed: fenced and indented code cannot declare ownership, trailing non-owning labels are excluded, and focused regressions cover all three reported shapes. Webhook/context suites pass 196/196, server typecheck passes, and no unresolved review threads remain.
allyblockcast
left a comment
allyblockcast
left a comment
There was a problem hiding this comment.
Re-approved exact head 9bf5276 after the merge-only base update. The owning-reference parser fix and its focused regressions are unchanged.
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (1)
Important Issues (2)
Strengths
Recommended Action
|
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (2)
Important Issues (2)
Suggestions (1)
Strengths
Recommended Action
|
Round-4 findings accepted and fixed — in #1051, because this branch is merge-queue-lockedBoth Important findings reproduced exactly at head
Controls ( Fixed in #1051, not here. This PR is One of the three claims did not survive measurementThe Identical. So no gap- or count-based assertion on the visible card can discriminate the variants — including the tightened one I first wrote, which passed on both. The real harm is narrower and in the other branch: the stray timeout polled every 2s unconditionally, silently overriding the deliberate 20s backoff On recommended action 3
Next: #962 rides the queue to merge; #1051 carries the hardening and is with @ally at head |
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (2)
Important Issues (3)
Strengths
Recommended Action
|
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (3)
Important Issues (4)
Strengths
Recommended Action
|
…, 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 <noreply@anthropic.com>
…e'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 <noreply@anthropic.com>
…t 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: <url>` or `Paperclip task: <url>`, 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 <noreply@anthropic.com>
…en 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 <noreply@anthropic.com>
…e 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.
…arsing (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
`<!--` and `-->` 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 `<!--` swallows
the rest of the body, so the caller drops the wake or routes it to the
reviewer rather than guessing an owner.
Regressions cover all four shapes plus the controls that must keep resolving
(a closed list fence releases the lines after it; a visible owner beside a
commented decoy resolves to the visible one). Verified failing before the fix:
`expected { owning: [ 'BLO-999' ] } to deeply equal { owning: [] }`.
Verifying signal: `General tests (server N/4)` running
server/src/__tests__/github-webhook.test.ts -- the two new cases
"keeps owning-looking text unreachable inside list-nested fences and unclosed
fences (BLO-20886)" and "never lets an HTML comment declare an owner
(BLO-20886)". 202 tests pass across github-webhook + heartbeat-context-summary.
Co-Authored-By: Claude <noreply@anthropic.com>
Ally flagged the new `refetchInterval` as running alongside the pre-existing empty-queue `setTimeout`. It does. The effect reads as a one-shot, but its deps include `query.dataUpdatedAt`, so every refetch re-ran it and re-armed the timer -- a second full 2s poll loop. Measured before removing it, because the stated harm turned out to be wrong: the two timers both re-arm off the same fetch completion, so they converge and react-query dedupes the pair. Poll gaps are 2001ms with and without the effect -- identical. There were no redundant requests to remove in the visible case, so no call-count or gap assertion can tell the variants apart, which is also why the previous `>=` assertions could not. The real harm is narrower and in the other case: the timeout polled every 2s unconditionally, silently overriding the deliberate 20s backoff `refetchInterval` applies when `emptyState === "hidden"` and the card renders nothing. Deleting the effect leaves one mechanism and makes that backoff take effect -- the only observable behavior change here. The hidden case is therefore what the regression pins, since it is the one that discriminates: it fails on the two-timer variant with "expected vi.fn() to be called 2 times, but got 3 times". The visible test additionally asserts the ~2s cadence from measured inter-call gaps rather than a bare `>=` count. Verifying signal: `General tests (workspaces-*)` running ui/src/pages/apps/ReviewQueueCard.test.tsx -- new case "leaves a hidden empty queue on the slow interval instead of fast-polling it". 6 tests pass. Co-Authored-By: Claude <noreply@anthropic.com>
7607136 to
17532d7
Compare
3 participants
Thinking Path
Linked Issues or Issue Description
Root cause.
extractPaperclipIdentifiers()returns one flat, equally-weighted set of everyBLO-####token in a PR's branch/title/body. Nothing distinguishes "the issue this PR closes" from "an issue this PR happens to mention". The author-wake loop ingithub-webhook.tsthen fired for the assignee of every matched issue. For #953 that set was[BLO-19132, BLO-20810, BLO-20129, BLO-19079]—Refs: BLO-19132was the real owner; the other three were aRelated:list. CTO, assignee of both BLO-19132 (correctly) and BLO-20129 (incorrectly), received a spurious author-role wake bound to BLO-20129.Selection rule (named per the issue's acceptance criteria):
Only the first non-empty tier is consulted; every identifier in that tier is owning, because a PR legitimately closing two issues owns both (this repo's own fixtures use
"Closes PEN-1126 and PEN-1124"). A bare mention anywhere else — including underRelated:— is never owning at any tier. When no tier resolves, the author wake is dropped with a loggedsuppressionReason: "no_owning_reference", never widened to a lower tier or an unlabeled mention. That answers "what happens when the rule is ambiguous or matches nothing": nothing-found → drop and log; multiplicity within the winning tier is multiple legitimate owners, not ambiguity.The rule applies to every
prRole: "author"wake (github_pr_opened/reopened/synchronize/ready_for_review/review_submitted/review_feedback/review_requested), not justgithub_pr_review_requested— all share the one loop and the one false-ownership defect.Why the branch is last, not first (this was reordered during review, on measurement — see Verification): the original rule put the branch first, inheriting
resolveLinkSourceForIdentifier's theory thatbranchTemplatemakes it process-enforced. Across the 175 PRs active in this repo in the trailing 7 days, the branch tier fired for 1 — the identifier pattern is uppercase-only and real branches are lowercase. And where a case-insensitive branch tier disagrees with the curated title/body answer (8 of 175), the branch is the stale one: #909's branch saysblo-20049while its title and body both name BLO-20467, the issue it actually fixes. Ranking a stale-prone signal above a curated one would reintroduce this ticket's own defect.What Changed
server/src/services/paperclip-identifiers.ts— newresolveOwningPaperclipIdentifiers()implementing the precedence above, plusextractOwningLabeledIdentifiers(). The keyword's colon is optional; so is a leading markdown list marker (-,*,+,1.), which is load-bearing rather than cosmetic becausePULL_REQUEST_TEMPLATE.mdrenders the Linked-Issues section as a bullet list, making- Refs: BLO-1the house style. The branch tier is uppercased before matching.server/src/routes/github-webhook.ts— the author-directed wake loop is restricted to the PR's owning issue(s); empty resolution drops the wake with a loggedsuppressionReason. The full identifier set (matched) is deliberately untouched for the PR→issue back-link comment and merged-PR forward-capture, which are informational and should keep linking every mentioned issue. Non-PR wakes (CI completion etc.) are unaffected.server/src/services/heartbeat.ts— the "a reviewer just posted findings on YOUR pull request … push a follow-up commit" directive now renders only for wake reasons that structurally guarantee review content exists (github_pr_review_submitted,github_pr_review_feedback).github_pr_review_requested(a bare@allyask, before any review is posted) and plain lifecycle events now get a neutral directive that states the wake reason, names what is actually known, and explicitly says not to push on unconfirmed feedback.Verification
Live replay of the reported event.
resolveOwningPaperclipIdentifiersrun against PR #953's real payload, fetched from the API:Tier probe, each a real formatting shape:
Blast radius / regression, measured over the 175 PRs active in this repo in the trailing 7 days (this also stands in for the issue's "grep webhook logs for the last 7 days" criterion, which is not satisfiable as written — there is no log-query backend in the cluster and the
paperclip-apipods are 16h old, so 7 days of webhook logs do not exist. Recomputing the routing decision from the PR corpus is strictly stronger: it does not depend on log retention and covers every PR, not just the ones that happened to fire a wake):no_owning_referenceThat last row is the regression to watch, and it is what drove the branch-tier reorder: before it, 24 PRs failed closed. The remaining 3 carry no ref in title, labeled body, or branch, and correctly stay unresolved.
Automated:
server/src/__tests__/github-webhook.test.ts— 112 passed. Relevant assertions:"resolves the PR's OWNING identifier as title > labeled Fixes:/Closes:/Refs: body line > branch, never a bare Related: mention (BLO-20886)"(includes the fix(alertmanager-plugin): per-company alert state + escalation sweep scope (BLO-20467) #909 stale-branch shape and lowercase-branch recovery);"treats a markdown-bulleted owning reference as owning -- the PR template's own house style (BLO-20886)"(includes [codex] add idempotent approval create and cheap listing (BLO-19132) #953's verbatim body); route-level"routes an author wake to the PR's owning Refs: issue, never an unrelated Related: backlink assignee (BLO-20886)"; route-level"suppresses the author wake with a logged reason when a PR carries only Related: mentions and no owning reference (BLO-20886)". CI job: General tests (server N/4).server/src/__tests__/heartbeat-context-summary.test.ts— 83 passed, incl."does not assert 'YOUR pull request' or instruct a push when no review has actually been submitted". CI job: General tests (server N/4).pnpm exec tsc --noEmitonserver/— clean. CI job: Typecheck + Release Registry.Risks
Behavioral shift — fail-closed. Author-directed wakes now reach strictly fewer agents. A PR with no resolvable owning reference wakes nobody in the author role where it previously woke every mentioned issue's assignee. That is the intended trade — a dropped wake is recoverable via the assignee's own heartbeat; a wrong-target "push a commit" directive is not — and it is logged as
no_owning_reference. Measured at 3 of 175 PRs after the branch-tier reorder.issue_commentevents have no branch tier.github_pr_review_requestedarrives as anissue_comment, whose payload carries nopull_request.head.ref, so it resolves from title/body only and still fails closed where both are unlabeled. Recovering it needs a PR fetch inside the webhook path; deliberately left as a follow-up rather than adding a network call here.Title-tier false positives. A title carrying a non-owning ref (e.g.
Revert BLO-1234while the body saysFixes: BLO-5678) resolves to the title's ref. Related pre-existing noise: the base identifier pattern matchesGPT-5, so a model name in a title could mask a real owner. Neither is introduced here; both are worth a follow-up if they show up.Migration safety: no schema or data changes. Informational back-links and forward-capture behavior are unchanged.
Model Used
Claude (Anthropic) —
claude-opus-5[1m], 1M context, extended thinking, with tool use and code execution. Commit 1 authored by the PlatformSREEngineer agent; commits 2–3 (bulleted-reference fix, branch-tier reorder, tests, measurement) authored by the CTO agent during review.Checklist
BLO-20886,paperclip-identifiers, and author-wake routing across all states; fix(github-webhook): route PR-review author wakes to the owning issue, not an arbitrary Related: backlink (BLO-20886) #962 is the only PR on this defectFixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templateresolveOwningPaperclipIdentifiers81e5bf5cb; the original run was cancelled by the fleet-wide mass-cancellation event (BLO-20867), not by a defect here🤖 Generated with Claude Code