fix(github-webhook): close two Markdown escapes in owning-reference parsing (BLO-20886)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• When a reviewer acts on a GitHub PR, the webhook wakes the agent that owns the PR's issue and hands it a directive
• BLO-20886: it picked an arbitrary Related: backlink instead of the owning issue, so an agent was told to push commits to a PR that was not its own
• fix(github-webhook): route PR-review author wakes to the owning issue, not an arbitrary Related: backlink (BLO-20886) #962 fixes the selection rule; its ownership scanner must therefore be the one thing that cannot be tricked by ordinary Markdown
• Ally's round-4 review on fix(github-webhook): route PR-review author wakes to the owning issue, not an arbitrary Related: backlink (BLO-20886) #962 found two forms that still slip past it, at the exact head
• This pull request closes both, plus the duplicate-timer suggestion from the same review
• The benefit is that an issue named only inside a code example or an HTML comment can never capture an author-directed "push a follow-up commit" wake

Linked Issues or Issue Description

• Refs: BLO-20886
• Follow-up to fix(github-webhook): route PR-review author wakes to the owning issue, not an arbitrary Related: backlink (BLO-20886) #962, which is queued for merge (position 36) and therefore branch-locked; see "Risks" for why this is a separate PR.

What Changed

• extractOwningLabeledIdentifiers recognizes a fence opener after an optional list marker, so a fence nested in a bullet (- ```md) actually opens.
• A fence now closes only on a same-or-longer marker run followed by whitespace only, per CommonMark — ``` js inside an open fence is content, not the close.
• HTML comments are stripped before the ownership label match, with state carried across lines, so a Refs: line between <!-- and --> cannot declare an owner.
• Ambiguity fails closed throughout: 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.
• ReviewQueueCard polls an empty queue on one timer instead of two.

Verification

• server/src/__tests__/github-webhook.test.ts — two new cases covering list-nested fences, unclosed fences, trailing-content closing fences, and single/multi-line/unterminated HTML comments, plus the controls that must keep resolving. Verified failing before the fix: expected { owning: [ 'BLO-999' ] } to deeply equal { owning: [] }.
• 202 tests pass across github-webhook + heartbeat-context-summary.
• ui/src/pages/apps/ReviewQueueCard.test.tsx — 6 pass; the new hidden-queue case fails on the two-timer variant with expected "vi.fn()" to be called 2 times, but got 3 times.
• tsc --noEmit clean for both server and ui.

Risks

Low. This only narrows what counts as an ownership declaration, and every ambiguous case fails closed to "no owning reference" — which the caller already handles by dropping the wake or routing it to the reviewer, never by guessing an owner.

Worth stating plainly: one of the three review claims did not survive measurement. Ally reported the duplicate ReviewQueueCard timers as issuing redundant requests. They do not — both re-arm off the same fetch completion, so they converge and react-query dedupes the pair; measured poll gaps are 2001ms with and without the second timer, identical. The real harm is narrower: the stray timer polled every 2s unconditionally, overriding the deliberate 20s backoff for emptyState === "hidden". That is the one observable behavior change here, and it is what the new test pins, because the visible cadence cannot tell the variants apart.

Separate from #962 because #962 is in the merge queue and its branch is locked. Dequeuing to push these would forfeit its position on a ~17h queue and delay both the fleet-wide routing fix and #999, which is stacked on it — to harden a parser that does not exist on master yet, so nothing currently live regresses by landing in this order. Based on #962's head; GitHub will retarget this to master when #962 merges.

Model Used

• Ally/Paperclip GitHub App generated the code changes; exact model ID is not recorded in the PR metadata. Codex GPT-5 was used only to repair PR-template metadata after the review-gate failure.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots
• I have updated relevant documentation to reflect my changes
• I have considered and documented any risks above
• All Paperclip CI gates are green
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-20886

[allyblockcast]

🔗 Paperclip issue: BLO-20886

[allyblockcast]

@ally please review head bf823a4 — this is the follow-up to your round-4 review on #962, opened separately because #962 is branch-locked in the merge queue (position 36).

Both Important findings reproduced exactly and are fixed here:

• paperclip-identifiers.ts list-contained fences (- ```md) now open a fence; closing requires a same-or-longer marker run followed by whitespace only (CommonMark), so ``` js inside a fence no longer closes it.
• HTML comments are stripped before the ownership label match, with state carried across lines.

Review focus: (1) is the fail-closed direction right — an unterminated fence or <!-- swallows the rest of the body, yielding no_owning_reference rather than a guessed owner; (2) does the list-marker-prefixed fence pattern admit anything it should not; (3) the third claim did not survive measurement — the duplicate ReviewQueueCard timers produce identical 2001ms poll gaps with and without, because both re-arm off the same fetch completion and react-query dedupes them. The real harm was the overridden 20s hidden backoff, which is what the new test pins. Please check that reasoning.

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Model Used
• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: bf823a4

Critical Issues (0)

Important Issues (3)

• [pr-review-toolkit / native-codex] server/src/services/paperclip-identifiers.ts:161 — The closer path reuses a pattern that permits a list marker, so list content can close an unrelated open fence. For example, a body containing a root ``` fence, then - ````, then Refs: BLO-999is parsed as owningBLO-999`, although CommonMark treats the list-prefixed marker as content inside the root fence.
  • Use distinct opener and closer recognizers. A closing fence may have indentation before its marker, but it must not consume an optional list marker; add this exact regression case.
• [gstack/review / native-codex] server/src/services/paperclip-identifiers.ts:179 — HTML-comment state is updated before fence detection, allowing comment syntax in a fence info string to escape the fence. With ``` <!--, ````, then--> Refs: BLO-999, the opener stores htmlComment=true; after the fence closes, stripping the literal -->synthesizes a leadingRefs:` label and misroutes ownership.
  • Give fence recognition precedence over HTML-comment stripping and never mutate comment state from fenced content. Add the three-line reproducer as a regression test.
• [gstack/review] server/src/services/paperclip-identifiers.ts:108 — The optional list prefix accepts zero or unlimited whitespace (-````, 1.~~~`, or a marker followed by arbitrary indentation). Those are not the list-contained CommonMark fences described by the change, but the scanner opens a fence and can swallow a later visible owner, dropping a valid wake.
  • Require a valid list-marker separator/indentation and reject backtick opener info strings containing backticks; cover malformed near-fences that must leave a later Refs: visible.

Suggestions (2)

• [comments] ui/src/pages/apps/ReviewQueueCard.tsx:66 — Update the new comment: “double the requests” contradicts the measured React Query deduplication documented in the PR and test. The demonstrated harm is the hidden queue’s 20-second backoff being overridden.
• [tests] ui/src/pages/apps/ReviewQueueCard.test.tsx:279 — Prefer fake timers for the 2-second/20-second cadence checks; the real-time waits add several seconds and are scheduler-sensitive.

Strengths

• The whitespace-only, same-marker, same-or-longer closing rule is directionally correct.
• The hidden queue regression test targets the actual observable polling bug rather than relying only on call counts.
• The HTML-comment helper has a narrow typed state contract and covers single-line, multiline, and unterminated comments.

Recommended Action

1. Separate opener and closer grammars and isolate comment state from fenced content.
2. Add the adversarial parser cases above before merge.
3. Reopen this exact head under an independent author before an App approval is possible; the allyblockcast App cannot review its own PR.

[kkroo]

Reviewed the parser hardening and ReviewQueueCard polling cleanup. The ownership parser now ignores owning-looking labels inside list-nested fences, unterminated fences, and HTML comments, with regression coverage for both safe drops and visible-owner controls. The UI timer cleanup is covered by the hidden empty-queue polling test. No active review threads remain; review and security-review are green.