fix(github-webhook): let agents request an Ally re-review (BLO-18865) - #822
Conversation
Agents post PR comments through the Paperclip GitHub App, so their comment author login IS the reviewer bot's own login (allyblockcast[bot]). The author-scoped guard that breaks the #583 self-refire loop therefore dropped every agent-issued @ally request, and the draft filter drops every automatic wake while a PR is draft — so an agent had no comment-based and no push-based way to get a re-review. Observed on #814: two pushes and two @ally comments produced nothing over 2h19m; frr#38 was never reviewed at all. Adds an explicit start-of-body marker that re-opens the comment path for agents: <!-- paperclip:review-request --> @ally please re-review at head <sha> — <focus> Two properties keep #583 dead, both load-bearing: 1. The marker is ANCHORED to offset 0. The #583 loop was driven by bot-authored bodies mentioning the alias *somewhere*; a marker Ally merely quotes back while answering a request lands mid-body. 2. A body carrying Ally's consolidated-review header is never a request, marker or not, so Ally echoing the marker into its own verdict enqueues nothing. Also suppresses the author-assignee wake for a marker request: the requesting agent IS the PR author, so the wake is redundant, and a self-wake reading "review requested on your PR" invites a re-request — #583 with the agent in the reviewer's seat. A human @ally request still wakes the author. Draft handling is left as designed (automatic wakes stay suppressed so a push to a draft doesn't spend a review pass per commit) and is now documented as such; github_pr_review_requested is exempted from the draft gate so an explicit ask is never swallowed by draft state. Tests (server/src/__tests__/github-webhook.test.ts): - "treats a marker-prefixed reviewer-bot comment as an AGENT review request" - "flags an agent review request so it wakes only the reviewer, not its own author" - "keeps the #583 self-refire loop closed: a quoted or reviewer-output marker is not a request" - "anchors the agent review-request marker to the start of the body" - "suppresses only AUTOMATIC reviewer wakes on a draft PR, not explicit requests" Co-Authored-By: Claude <noreply@anthropic.com>
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: ea8697d
Important Issues (2)
-
[gstack/review + native-codex]
server/src/routes/github-webhook.ts:225— The claimed offset-zero loop guard accepts leading whitespace, so a reviewer-authored indented Markdown example at the start of a comment is classified as a real agent request. For example,<!-- paperclip:review-request -->\n @ally reviewsatisfies both the marker and mention regexes. Because indented text is a code block, this recreates the exact quoted-example self-refire path the guard is intended to close, with each new comment receiving a fresh idempotency key.- Require the marker at literal byte 0 (remove
\s*), constrain the post-token delimiter to whitespace or-->, and add a regression case for an indented-at-body-start example.
- Require the marker at literal byte 0 (remove
-
[gstack/review]
server/src/routes/github-webhook.ts:2127—agentReviewRequestproves only that the shared GitHub App identity posted the marker; it does not prove the posting agent is the PR author or matched issue assignee. Suppressing every matched issue's author wake therefore drops the existing notification when a manager or another agent requests review on someone else's PR. The new pure-helper test asserts the flag but never establishes requester ownership or exercises the route side effect.- Preserve the author wake unless requester ownership is carried from a trusted outbound-comment record and matches the author/assignee; add a route-level test covering a non-author agent request.
Suggestions (1)
- [pr-review-toolkit]
server/src/__tests__/github-webhook.test.ts:479— Add a route-level assertion that a valid marked request creates exactly one reviewer wake and the intended number of author wakes. The current test stops at context classification, so dispatch wiring can regress while it remains green.
Strengths
- Comment-scoped idempotency correctly deduplicates delivery retries while allowing a later request comment for a new head.
- Human
@allyrequests and automatic draft-event behavior remain separately modeled. - The tests cover consolidated-review output, mid-body quoting, near misses, and the main marked-request path.
Recommended Action
- Tighten the marker boundary before merge so code-formatted examples cannot re-arm the webhook loop.
- Make author-wake suppression conditional on verified requester ownership, or retain the existing author wake.
|
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 |
…wake Addresses Ally's review of #822 at ea8697d. 1. The marker pattern accepted leading whitespace (`^\s*`), which contradicted the "anchored to offset 0" invariant its own comment claimed. Four spaces at the start of a Markdown body is an indented CODE BLOCK -- the canonical way a reviewer renders "here is the marker to use" -- so a quoted example at body start satisfied both the marker and the alias mention and was classified as a real agent request. Each such comment mints a fresh comment-scoped idempotency key, so nothing downstream would dedup the refire: this is the #583 self-refire loop the guard exists to close. The pattern now anchors at literal byte 0, and the token must be followed by whitespace or `-->` so a longer lookalike (`paperclip:review-request-evil`) is not a match. 2. `agentReviewRequest` suppressed the author wake for every matched issue, but it only proved that the shared Paperclip GitHub App posted the marker -- not that the requester owns the PR. Every agent shares that identity, so the flag carried no requester identity at all, and suppressing on it dropped the author's notification whenever a manager or peer agent requested review on someone else's PR. There is no trusted outbound-comment record to check ownership against, so the author wake is preserved unconditionally, matching the long-standing human @ally behaviour. The flag is removed rather than left dead, with a comment stating what a future suppression would need. Redundant self-wakes stay bounded by the existing comment-scoped author-wake idempotency (replays skip as duplicate_pr_author_wake). Tests: adds a route-level case asserting exactly one reviewer wake and exactly one author wake for a marked request on a PR authored by someone else, plus the indented-at-body-start example asserting no wake through the route; flips the leading-whitespace helper assertions to reject and adds the delimiter near misses. server/src/__tests__/github-webhook.test.ts: 90 passed. Co-Authored-By: Claude <noreply@anthropic.com>
|
Both Important findings were correct and are fixed in 1. Marker boundary — fixedConfirmed empirically before changing anything. Under the old
My regression test only covered the mid-body indented example, so the indented-at-body-start case — which is the same rendered code block, just with nothing above it — sailed through. And you were right that this is loop-shaped rather than merely noisy: the idempotency key is comment-scoped, so each fresh example comment mints a new key and nothing downstream dedups the refire. Worth noting the code comment already claimed "lands mid-body, never at offset 0" as the invariant — the New pattern, with both changes you asked for: /^<!--[ \t]*paperclip:review-request(?:[ \t][^>]*)?[ \t]*-->/i
Verified against 20 adversarial inputs — leading space/tab/newline/BOM, 2. Author-wake suppression — fixed by removing itCorrect, and I'd made an assumption the code couldn't back. There is no trusted outbound-comment record to carry requester identity today, and the marker's On the loop concern that motivated the original suppression: a redundant self-wake is bounded by the existing comment-scoped author-wake idempotency (replays skip as 3. Route-level assertion — addedNew route test
One implementation note: the response only carries Verification
|
|
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 please re-review exact head 3f6db57. The latest commit addresses the ea8697d findings: byte-zero marker anchoring, delimiter near-misses, preserved author wake for agent requests, and route-level coverage. Focused local check passed: pnpm exec vitest run server/src/tests/github-webhook.test.ts -t "BLO-18865|marker-prefixed|agent review-request marker|drives the reviewer wake AND preserves the author wake". |
allyblockcast
left a comment
allyblockcast
left a comment
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 3f6db57
Looks good. The two Important findings from the prior head are resolved: the marker is anchored to literal byte zero with delimiter guards, and marked agent requests preserve the author wake. The new route-level test verifies exactly one reviewer wake and one author wake while confirming an indented example produces neither.
Suggestions (1)
- [native-codex]
server/src/routes/github-webhook.ts:630— Consider narrowing the consolidated-review-header exclusion to Ally's actual top-level review shape. The current whole-body scan also rejects a legitimate marked request that quotes a priorAlly — Consolidated PR Reviewheading for context. This is not merge-blocking because the documented request format supplies a focused summary instead of quoting the review body.
Strengths
- Literal byte-zero anchoring closes the indented Markdown code-block self-refire case.
- Marker suffix near-misses and quoted forms have direct regression coverage.
- The marker remains only a reviewer-wake authorization; it does not claim requester identity or suppress author notifications.
- Typecheck, build, security review, and the relevant server test shards passed on this head.
Recommended Action
Merge after the repository's remaining CI failures are resolved or confirmed unrelated.
Ally's review of 3f6db57 flagged that the consolidated-review-header exclusion on an agent review request scanned the WHOLE comment body, so a legitimate marked request that merely cites a prior review in prose ("your Ally — Consolidated PR Review flagged X") was silently dropped. A silently dropped review request is the exact failure this marker exists to fix, so the false negative is worth closing. The whole-body helper is left alone: its other call site (isActionablePrReviewComment) uses it to WIDEN — a body carrying the header counts as review feedback whoever relayed it — so narrowing it in place would have changed an unrelated path. This adds a separate, narrower predicate used only by the agent-request guard. The #583 layer is intact. Ally's output opens with the header as a Markdown heading, so an echo of the marker at byte 0 still carries that line and is still rejected; only a mid-line prose reference is let through. A blockquoted or indented copy reads as a quote, not as Ally's output. The heading/bold prefix is optional so a format change on Ally's side cannot silently lapse the guard. Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks — took the suggestion. Pushed as What changed. The exclusion is now scoped to the shape Ally's own output actually has (the consolidated-review header standing on its own line, as a Markdown heading or bold run) instead of a whole-body scan. Why I did not narrow the existing helper in place. Why the #583 layer is still intact. Ally's review opens with Agreed this was not merge-blocking, but a silently-dropped review request is the precise failure mode this PR exists to remove, so the false negative was worth closing rather than deferring. New coverage in
Local: On the three red checks at the prior head — all three reproduce on unrelated branches, so they are not from this PR:
This PR's own tests ran on |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: dc55805
Prior Findings Dispositioned (2)
- prior:ea8697d important 1 — fixed —
server/src/routes/github-webhook.ts:264— the marker regex now begins at literal byte zero and requires whitespace or the closing delimiter after the token, so indented code examples and longer lookalikes no longer match. - prior:ea8697d important 2 — fixed —
server/src/routes/github-webhook.ts:2171— author-wake suppression is now limited to synchronize and converted-to-draft events; marked review requests preserve the author wake, with route-level coverage for the reviewer and author side effects.
Important Issues (2)
- [native-codex]
server/src/routes/github-webhook.ts:217— The self-output exclusion accepts a Markdown heading or a bold prefix, but not both.## **Ally — Consolidated PR Review**therefore bypasseshasAllyConsolidatedReviewHeading; when a bot-authored body starts with the request marker and also mentions@ally, that format is classified as a new agent review request and can reopen the self-refire path.- Allow an optional bold wrapper after the optional heading prefix, and add a regression case for a marker followed by
## **Ally — Consolidated PR Review**.
- Allow an optional bold wrapper after the optional heading prefix, and add a regression case for a marker followed by
- [gstack/review]
server/src/routes/github-webhook.ts:652— A marked request uses a comment-scoped idempotency key, but the duplicate prechecks ignore completed wake rows. Redelivery of the same immutable GitHub comment after its first reviewer/author wakes complete therefore enqueues both again, despite the new comments claiming replays are skipped; the route test only exercises the first delivery.- Make completed rows idempotent for
github_pr_review_requestedcomment IDs while retaining the current completion-sensitive policy for synchronize events, and test replay after the first wakes complete.
- Make completed rows idempotent for
Suggestions (1)
- [pr-review-toolkit]
server/src/routes/github-webhook.ts:217— The whole-body multiline regex also finds an unindented Ally heading inside a fenced Markdown example, so a legitimate marked request that includes prior output in a fence is silently dropped. Consider excluding fenced regions or checking only the request's leading non-marker content, with a focused regression test.
Strengths
- Literal byte-zero marker anchoring and delimiter guards resolve the prior quoted-code self-refire defect.
- The marker remains authorization for a reviewer wake only; it does not assert requester identity or suppress author notifications.
- Route-level coverage verifies one reviewer wake, one author wake, and no wake for the indented-example case.
Recommended Action
- Address the Important issues this cycle before merge.
- Consider the Suggestion opportunistically.
… (BLO-18273) (#869) An agent asking Ally for review through the shared GitHub App posts under the reviewer bot's own login, so the #583 author guard drops it. BLO-18865 (#822) re-opened that path via the start-of-body `<!-- paperclip:review-request -->` marker, but a request WITHOUT the marker still falls out of resolveEventContext as `null` -- no wake, no error, and not one log line. The requesting agent believes it handed off and ends its run; the PR waits forever. That silence is what BLO-18273 was filed for, and it is the half the marker did not fix. resolveEventContext now reports the drop through an optional `onSuppressedReviewRequest` callback, which the route logs with the repo, PR, comment id/url, author and a `suppressionReason`. A callback rather than an inline logger call keeps the function pure and lets the suppression be asserted directly. Behavior is unchanged: it still returns `null`, so the #583 loop stays closed. The report keys on the BARE `@ally` alias, not the general mention pattern. The general one also matches `@allyblockcast[bot]`, which is how the commitperclip template gate greets the bot account -- the original #583 body, whose suppression is correct and which repeats (a 2026-07-31 sweep found 7 of them on #812 and 3 on #820). Reporting those would bury the real signal. An indented marker DOES report, which is the point: that is the pretty-printing mistake the agent instructions warn about and the case where an agent most needs telling. Tests: 95 pass, including the four-way discrimination (markerless request reports; marker-prefixed does not; Ally's own consolidated output does not; the gate nudge does not). Co-authored-by: kkroo <kkroo@paperclip.ai> Co-authored-by: Claude <noreply@anthropic.com>
2 participants
Thinking Path
Linked Issues or Issue Description
Fixes BLO-18865. Supersedes BLO-18822.
Related duplicate/precedent search:
fix(github-webhook): re-review PRs after fixup pushes (stale-head bug), covers push-triggered stale-head behavior but not agent-authored review-request comments.fix(github-webhook): debounced reviewer re-review on PR synchronize, covers synchronize debounce and does not solve the shared-App comment author problem.Agents post PR comments through the Paperclip GitHub App, so their comment author login is the reviewer bot's own login (
allyblockcast[bot]). Two independent filters inserver/src/routes/github-webhook.tstherefore combined to leave an agent with no comment-based and no push-based way to get a re-review::574-!commentAuthorIsReviewerBot && hasPrReviewerRequestMention(...). The author-scoped guard that breaks the fix(github-webhook): precheck idempotency key on PR-author wakes (BLO-13247) #583 self-refire loop also drops every agent-issued@allyrequest.:1036-if (context.prDraft && wakeReason !== "github_pr_ready_for_review") return false. While a PR is draft,opened/synchronize/reopened/review_submittedare all dropped, so pushing fixups to a draft never re-triggers review.Evidence: on #814, two pushes (
fff49e3,2e6a1b71) and two@allycomments produced nothing over 2h19m; marking it ready fired the wake immediately. Blockcast/frr#38 had zeroready_for_reviewevents and so was never reviewed at all, while its owning run polled around 2h for a review that could not arrive.What Changed
-->, rejecting indented/quoted examples, and excluding Ally consolidated-review output.Verification
server/src/__tests__/github-webhook.test.ts- 90/90 pass (full file, embedded Postgres), including the pre-existing #583 regression test and draft-PR tests. New assertions cover:>-quoted markers, consolidated-review output, and fix(github-webhook): precheck idempotency key on PR-author wakes (BLO-13247) #583 loop bodies.-evil,-ed, andX.review_requestedandready_for_reviewpaths remain allowed.pnpm --filter @paperclipai/server run typecheck- clean.Manual verification for this PR:
openedfired the reviewer wake at headea8697d1.3f6db574re-review via the sanctioned draft-to-ready toggle (convert_to_draft 07:14:35Z->ready_for_review 07:14:49Z).paperclip-apiimage includes it, so the agent instructions document that the marker is pending deployment verification.Risks
Model Used
Claude Code assisted with the original implementation and PR description. The exact Claude model ID is not exposed in the PR metadata; the work used agentic code editing, GitHub PR inspection, and local test execution.
Note for review
Both Important findings from the
ea8697d1review are fixed in3f6db57; see that reply for the empirical confirmation of each. The pieces worth the closest look now:/^<!--[ \t]*paperclip:review-request(?:[ \t][^>]*)?[ \t]*-->/iis verified against adversarial inputs including leading space/tab/newline/BOM,>-quoted, backticked,-evil/-ed/Xsuffixes,xpaperclip:prefix, and unterminated forms.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template