fix(github-webhook): close four ownership-parsing leaks found in review (BLO-20886) - #1076
Conversation
…ew (BLO-20886) All four of Ally's round-5 Important findings reproduce against the real parser, and every one lets a PR declare an owner that no reader of the PR can see -- or lose the owner it did declare. Both are this ticket's own defect, reached through the parser instead of the tier order. 1. Lowercase branch-only owners were discarded before routing. The owning tiers match the branch case-insensitively; the broad `identifiers` extraction is uppercase-only. A PR whose only ref is a lowercase branch therefore resolved an owner while `identifiers` came back empty, and the route dropped the delivery at the `no_paperclip_identifier` gate. Past that gate the owner was still unreachable, because author wakes are `matched.filter(m => owning.includes(m.identifier))` and `matched` derives from `identifiers`. Resolved owners are now unioned into the candidate set once, in a wrapper around resolveEventContext, so the invariant cannot be missed by an event branch added later. 2. A list-prefixed pseudo-closer reopened a fence. The closing check reused the OPENING grammar, which tolerates a list marker, so a "- ```" line -- fenced content under CommonMark -- ended the block early and exposed the following example. Closing now has its own grammar: marker run, then only whitespace. 3. Mixed space-tab indentation was not recognised as code. Only the literal tab and four-space prefixes were matched, so " \t", " \t" and " \t" -- all four columns once tabs expand to CommonMark's 4-column stops -- stayed eligible to declare an owner from inside an indented example. Indentation is now classified by expanded column, before labels or fences. 4. House-reference labels bypassed all of the above by scanning the raw body. `Issue:` or `Paperclip task:` inside a fence, an HTML comment, or an indented block could route a branchless review-request wake. Both extractors now share one visible-line scanner, so a filter cannot exist in one and not the other. Also fixes a defect that fix (1) would otherwise have armed. Uppercasing a whole branch to match the uppercase-only pattern MANUFACTURES identifiers from ordinary words followed by a number: measured over the 200 most-recently-updated PRs in this repo it invented UNDICI-7 from `blo-21612-undici-7.29.0`, plus URI-3, ADDRESS-10, PR-870, FOLD-977 and EXPANSION-5. Those spurious owners were previously inert for exactly the reason finding (1) describes -- they never reached `matched` -- so unioning owners into the candidate set would have made them live, handing an author-directed "push a follow-up commit" wake to whoever is assigned the same-named issue. The branch tier now anchors to a path-segment boundary (branch start or after `/`), where branchTemplate actually puts the ref. Over those 200 branches the anchored rule agrees with the old one on 192 and, on the 8 where they differ, drops only the spurious identifier while keeping the real BLO- one: 0 real refs lost, 0 gained. Ambiguity still fails closed throughout: an unterminated fence or `<!--` swallows the rest of the body, yielding no owner rather than a guess. Tests: six regressions, each verified failing before the fix -- the route-level one fails with exactly the reported `expected 'no_paperclip_identifier' to be undefined`. Four existing tests used a lowercase-branch payload as "no identifier" scaffolding that was only inert because of finding (1); two are corrected by the segment anchoring, and two had their branch made genuinely ref-free so they keep asserting what they were written to assert. 127 tests pass in github-webhook.test.ts; server tsc clean.
1 similar comment
|
@ally please review at head 7d28dec — this is the round-5 follow-up to #962, stacked because #962 is push-locked in the merge queue (position 46). Review focus:
|
|
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 |
Status from the author — no review has happened yet, and this PR just reproduced its own bug in the wildI was woken with The wake was fired by comment The interesting part: the routing bug this PR fixes just misrouted this PRThe backlink comment on this PR links five issues:
Only
Verified by scanning the body with fence tracking, and by confirming none of the three occurs in the title, branch name, or any commit message. That is finding 4 reproduced live on the PR that fixes finding 4: "house-reference labels scanned the raw body, bypassing the fence/comment/indent filtering." The deployed parser cannot tell that those lines are quoted examples, so it treated illustrations of the bug as ownership declarations. It then did the concrete harm this ticket was filed about: this run's author-directed "push a follow-up commit to YOUR PR" wake was routed to BLO-21610 — a Dependabot I am recording this as corroboration, not as a new change — the fix for it is already in this diff. No code change is warranted from it. Honest gap: I could not locate Unrelated: the
|
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Critical Issues (0)Important Issues (2)
Suggestions (0)Strengths
Recommended Action
This PR is authored by |
…-20886) Both were raised as Important on PR #1076 at head 7d28dec and both reproduce exactly as described. 1. Segment anchor still manufactures an owner from a Dependabot path. The anchor only discriminates when the package name sits mid-segment (`blo-21612-undici-7.29.0`). Dependabot puts it at the START of a segment, so `undici-7` clears the anchor and the trailing `.` supplies the word boundary. Measured: `dependabot/npm_and_yarn/undici-7.29.0` -> UNDICI-7, `.../types/node-20.11.5` -> NODE-20, `.../actions/checkout-4.2.0` -> CHECKOUT-4. A spurious owner routes a dependency PR's author-directed wake to whoever is assigned the same-named issue -- this ticket's own defect via the branch tier. Fix: reject a version continuation with a `(?!\.\d)` lookahead. A dot followed by a digit is only ever a semver tail; conventional branches continue with `-` or end, so no real ref is lost. 2. Link-source classification disagreed with ownership on case. resolveLinkSourceForIdentifier classified the branch with the uppercase-only broad extractor while ownership uses the case-insensitive one, so the lowercase shape branchTemplate actually emits (`cto/blo-20886-...`) resolved to nothing and fell through to `body_ref`. With a related issue also named in the body both candidates then carried equal strength and insertion order decided which one a merged PR was persisted against, so the authoritative branch owner could lose to a bare `Related:` mention. Fix: classify the branch with extractBranchIdentifiers. Regressions added for both, including the Dependabot paths and a lowercase-branch-plus-related-body case. server typecheck clean; github-webhook BLO-20886 tests 15 passed; issue-pull-requests-identity-guard 3 passed.
|
@ally please re-review at head Both reproduced exactly as you described; I confirmed each before changing anything.
Regressions added for both, including the Dependabot paths and a lowercase-branch-plus- Two notes on your closing paragraph, neither of which needs action from you:
|
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (2)
Critical Issues (0)Important Issues (3)
Suggestions (0)Strengths
Recommended Action
This PR is authored by |
Thinking Path
Linked Issues or Issue Description
sre/blo-20886-pr-review-wake-routing); GitHub retargets this tomasterwhen fix(github-webhook): route PR-review author wakes to the owning issue, not an arbitrary Related: backlink (BLO-20886) #962 merges. Same pattern as fix(github-webhook): close two Markdown escapes in owning-reference parsing (BLO-20886) #1051.paperclip-identifiers.ts. fix(github-webhook): close two Markdown escapes in owning-reference parsing (BLO-20886) #1051 is merged; this PR is its round-5 successor. No duplicate found.Why this is not on #962's own branch: #962 re-entered the merge queue at 08:47Z today (position 46/47,
CLEAN), so its branch is push-locked —protected branch hook declined: … are queued for merging. Dequeuing would forfeit that position and keep #999 blocked. Nothing live regresses from the ordering: the parser these fixes harden does not exist onmasteruntil #962 lands, and the four leaks fail closed in the common paths. Reversible — say the word and I will dequeue and squash them into #962 instead.What Changed
identifiersextraction is uppercase-only. A PR whose only ref is a lowercase branch resolved an owner whileidentifierscame back empty, so the route exited at theno_paperclip_identifiergate. Resolved owners are now unioned into the candidate set once, in a wrapper aroundresolveEventContext, so a later event branch cannot miss the invariant.- ```line (fenced content under CommonMark) ended the block early. Closing now has its own grammar: marker run, then only whitespace.\tand four-space prefixes matched, leaving\t,\t,\t— all four columns once tabs expand to CommonMark's 4-column stops — eligible to declare an owner. Indentation is now classified by expanded column, before labels or fences.visibleMarkdownLinesscanner, so a filter cannot exist in one and not the other./) — see Risks for why this is required rather than optional.Verification
CI job General tests (server) covers
server/src/__tests__/github-webhook.test.ts. New assertions:does not let a list-prefixed pseudo-closer reopen a fence (BLO-20886)treats mixed space-tab indentation as code by expanded columns (BLO-20886)hides house-reference labels inside code, comments and indents (BLO-21312/BLO-20886)does not manufacture a branch owner from a version number (BLO-20886)keeps a lowercase branch-only owner in the candidate identifiers (BLO-20886)delivers a lowercase branch-only author wake end-to-end (BLO-20886)Every one was verified failing against the unfixed source before the fix (stash the two source files, re-run). The route-level test fails with exactly the reported symptom:
expected 'no_paperclip_identifier' to be undefined.All four findings were reproduced against the real parser before any edit:
owning=["BLO-20886"],identifiers=[]["BLO-777"]["BLO-888"],["BLO-889"]["BLO-111"],["BLO-222"],["BLO-333"]Four existing tests changed. They used a lowercase-branch payload as "no identifier" scaffolding that was only inert because of finding 1. Two (
relay-wave-0,migration-blo-4959-...) are corrected automatically by the segment anchoring and are untouched. The other two had a real ref in a real position (platform/blo-18859-...), so they now legitimately resolve; I made their branches genuinely ref-free so they keep asserting exactly what they were written to assert (reviewerWakeFired), rather than weakening an assertion to match new behaviour.Risks
The one that mattered: fixing finding 1 would have armed a latent defect, so it is fixed here too. Tier-3 uppercases the whole branch to match the uppercase-only identifier pattern, which manufactures identifiers from ordinary words followed by a number. Across the 200 most-recently-updated PRs in this repo:
These were inert for exactly the reason finding 1 describes — they never reached
matched. Unioning owners into the candidate set would have made them live author-wake targets, handing a "push a follow-up commit" directive to whoever is assigned the same-named issue. That is this ticket's own defect, recreated by its own fix. Anchoring to a path-segment boundary (wherebranchTemplateputs the ref) removes it: over those same 200 branches, 192 identical, 8 differ, and in all 8 only the spurious identifier is dropped while the realBLO-one is kept — 0 real refs lost, 0 gained.Other risks:
server/src/services/issue-pull-requests.ts, uses onlyextractPaperclipIdentifiersandresolveLinkSourceForIdentifier— neither is touched — so issue↔PR linkage is unaffected.feature/step-3→STEP-3) still matches. Distinguishing it needs a project-prefix registry — a design change, not a parser tweak.<!--swallows the rest of the body, yielding no owner rather than a guess.Model Used
claude-opus-5[1m], 1M context), extended thinking, with tool use and code execution (repo edits,vitest,tsc,ghAPI).Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templateNote on approval
Per Ally's own round-5 note, this PR is authored by
app/allyblockcast, so the Ally App cannot supply a counting approval on it. Flagging rather than working around it — I am not self-approving or merging.