feat(metrics): emit the review-OUTPUT counter, not just request-side (BLO-27608) - #1386
Conversation
1 similar comment
…(BLO-27608)
All three existing GitHub review metrics are request-side, so "no review was
posted" is structurally undetectable. Ally was down fleet-wide for ~8.6h on
2026-08-12 (codex provider unavailability, BLO-27123) and the funnel read
received 131 / queued 131 / suppressed 0 / deferred 0 / dead_lettered 0 the
whole time: the runs were enqueued and dispatched, then died at the model call
and produced no artifact. Nothing paged.
Add paperclip_github_review_posted_total{repo,surface}, counting reviews the
configured reviewer identity actually published, and
paperclip_github_review_completion_total{status}, which separates a deliberate
non-post (self_review_skipped/already_reviewed/archived_repo_skipped) from a
real failure (missing) so intentional-skip volume cannot mask a drought.
The control plane never posts a review -- there is no POST /pulls/{n}/reviews
anywhere in server/, and Ally posts its own via `gh pr review` in its pod -- so
there is no in-process "the post succeeded" hook. Observe the signed webhook
instead, which is stronger: GitHub confirming the artifact exists rather than
the poster reporting that it published one.
The observation deliberately bypasses resolveEventContext, which returns null
for exactly what must be counted: a clean comment-shaped review is neither a
review request nor actionable feedback, and the reviewer's own formal review is
dropped downstream as a self-echo (BLO-15799). Both are correct wake decisions
that would have silently zeroed this counter.
Zero-init is load-bearing: the consuming alert is sum(increase(...)) == 0, and
on an absent series that inner expression is an empty vector, so the alert would
stay silent during exactly the outage it exists to catch.
Co-Authored-By: Claude <noreply@anthropic.com>
…ry guard (BLO-27608) githubReviewPosted and githubReviewCompletion were declared `Counter<...> | null` like every sibling, but were the only two omitted from the ensureRegistry() null-guard. Two consequences: 1. TS2322 at the return statement (CI `Typecheck` + `Build` red) — TypeScript cannot narrow a module-level `let` that the guard does not test. 2. A real defect, not just a type error: if the registry is rebuilt while these two are null, the guard short-circuits and the counters are never reconstructed, so the series silently stops. Verified: `tsc --noEmit` exits 0 (was 2 errors); the 16 tests in github-review-posted-metric.test.ts pass.
45004eb to
f96d70b
Compare
|
@ally please review at head This adds the review-output half of our review telemetry. All three existing review metrics are request-side, so an ~8.6h fleet-wide Ally blackout on 2026-08-12 read perfectly healthy (
Also note CI is green at this head: 19 success / 1 skipped / 0 failures. |
![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: f96d70b
Critical Issues (0)
None.
Important Issues (1)
-
[gstack/review + native-codex]
server/src/routes/github-webhook.ts:437— theformalsurface counts anypull_request_review.submittedfrom the reviewer login, with no body gate, so non-Ally artifacts published through the same shared credential incrementpaperclip_github_review_posted_totaland can hold the drought alert down during a real Ally outage.The comment branch (
:464) correctly recognises that author login is not sufficient on this codebase — the file's own comment at:257-262says so: "Agents post PR comments through the Paperclip GitHub App, so their comment author login IS the reviewer bot's own login." It therefore addshasAllyConsolidatedReviewHeading+ the request-marker exclusion. The formal branch applies neither, but the same shared-credential property holds forPOST /pulls/{n}/reviews.Measured on this repo just now — Ally-identity, non-dismissed formal reviews with no consolidated heading, i.e. reviews this resolver would count as Ally output:
#1382—allyblockcast[bot],COMMENTED, body opensReviewed at head \81dfdee`. I re-derived the claims in the body rather…` (another agent's pass, not an Ally consolidated review)#937—allyblockcast[bot],CHANGES_REQUESTED, body opensStructural review: changes requested at …#997—allyblockcast,APPROVED, body opensApproved exact head 38933132e9bdcb73e20c…(the merge-token seat clearing a gate, not review output)
Base rate is low (1 of 32 non-dismissed formal Ally-identity reviews across the last 40 PRs), but the rate is not the risk: these artifacts are exactly what appears during an Ally outage — another agent stepping in, or the merge-token unblocking a PR. One of them inside the 2h window makes
sum(increase(paperclip_github_review_posted_total[2h])) == 0false and the alert silent, which is the precise failureGITHUB_REVIEW_POSTED_METRIC's docblock commits to preventing ("any series a non-Ally reviewer could feed would let one … review during an Ally blackout hold the alert down").Worth noting alongside it:
isConfiguredPrReviewerAuthor(:263) hard-codesally/allyblockcast/blockcast-ci-packagesas always-matching, so this is not narrowed byprReviewerBotLoginconfig either.- Apply the same body gate on the formal branch that the comment branch already applies —
if (!hasAllyConsolidatedReviewHeading(readStringField(review, "body"))) return null;. Ally's own formal reviews all carry the heading (31 of 32 in the sample), so this costs no true positives and closes the shared-credential hole. If counting non-consolidated reviews is deliberate, make it explicit with a thirdsurfacevalue so the drought alert can select only the Ally-review surfaces, rather than leaving it implicit in a baresum().
Suggestions (3)
-
[pr-review-toolkit/code]
server/src/services/metrics.ts:2298—__resetMetricsForTest()nulls every other module-level counter handle but notgithubReviewPosted/githubReviewCompletion. Harmless today because nullingregistryalone re-enters the full init block at:1212, but these are now the only two handles that break the function's invariant, and a future move to per-counter re-init would leave them pointing at a discarded registry. Two lines besidegithubReviewRequestDeadLetterUnresolved = null;. -
[pr-review-toolkit/tests]
deploy/helm/paperclip/dashboards/github-review-request-funnel.json:306— the two new panels have no dashboard-contract test, unlike every other acceptance criterion indeploy/helm/paperclip/tests/grafana-dashboard.test.mjs("the funnel panel plots all four required delivery states", "the restart-safe dead-letter gauge is on the dashboard", "the suppression-cause counter is charted"). They are covered incidentally by the datasource-uid guard at:113, so the suite still passes 11/11 — but nothing pins the expressions, and a rename of either metric would silently leave two "No data" panels on the dashboard an operator consults during an outage. -
[native-codex]
server/src/services/heartbeat.ts:24944— whenoutcome !== "succeeded"the verdict isnot_applicableandrecordGithubReviewCompletiondrops it, so a reviewer run that fails is indistinguishable from a run that was never a reviewer run. The PR documents this ("a run that dies at the model call reaches no verdict at all"), and the double-silence signature is real — butderivePaperclipPrReview(context)can already tell that a failed run was a reviewer run, so a distinct label (e.g.run_failed) would make the 2026-08-12 signature directly attributable on one panel instead of inferred from the absence of two. Worth considering before BLO-27246 encodes the two-silence rule into alert logic.
Strengths
- The choice of observation point is argued from evidence rather than convenience: the AC asked for "increment where the post succeeds", and the docblock demonstrates no such point exists in
server/(noPOST /pulls/{n}/reviews, no octokit) before making the case that the signed webhook is stronger — first-party confirmation the artifact exists rather than the poster's self-report. That reasoning survives in the code, not just the PR body. - Deliberately bypassing
resolveEventContextis the right call and is justified precisely: a clean comment-shaped review is neither a request nor actionable feedback, and the reviewer's own formal review is dropped as a self-echo (BLO-15799) — both correct wake decisions that would have silently zeroed this counter. Placement after signature verification and insideWAKE_DRIVING_EVENTS(bothissue_commentandpull_request_revieware members) is correct, with no early return in between. - Zero-init is treated as load-bearing and explained at both the code and test level: absent vs zero render identically and mean the opposite, and the case where they diverge is the outage itself. Easy to delete as "cosmetic" in a later cleanup; the comment makes that mistake hard.
- Genuinely adversarial negative controls — human reviewer, the control plane's own back-link comment, a marker-prefixed request quoting a heading, a comment on a plain issue, and
edited/dismissed/deleted. That set is what surfaced the asymmetry above rather than hiding it. - Cardinality is measured, not asserted:
surfaceclosed at 2,repobounded by the installation at 98 repos, worst case 196 series, with the reasoning for preferring label growth over an allow-list that would silently drop a newly-onboarded repo. - Panel layout is clean (
y=24followsy=15 + h=9, no overlap) and both targets use the__PAPERCLIP_DS_UID__placeholder, so the existing BLO-21025 datasource guard covers them.
Recommended Action
- Fix Critical issues before merge. — none.
- Address Important issues this cycle. — gate the formal surface on the consolidated-review heading, or split it into its own
surfacevalue so the drought alert can select Ally-review output explicitly. - Consider Suggestions opportunistically. — the
__resetMetricsForTesthandles are a two-line fix; the panel test and therun_failedlabel are worth folding into BLO-27246 where the alert logic lands.
Note the PR's own checklist flags that the server unit tests have not been run locally — CI is the authoritative validator for github-review-posted-metric.test.ts, and this review does not substitute for that.
1 participant
Thinking Path
Linked Issues or Issue Description
What Changed
paperclip_github_review_posted_total{repo,surface}— reviews the configured reviewer identity actually published, observed from the signed webhook.surface="formal"is apull_request_review.submitted;surface="comment"is anissue_commentcarrying Ally's consolidated-review heading. Both are counted because Ally uses both and either alone under-reports (measured:#952has 4 comment-shaped / 0 formal,#937has 4 formal / 0 comment-shaped).paperclip_github_review_completion_total{status}— the terminal verdict of each reviewer run, recorded at bothevaluatePrReviewCompletionEvidencecall sites after the authoritative GitHub re-verification those sites perform. Separates a deliberate non-post (self_review_skipped,already_reviewed,archived_repo_skipped) from a real failure (missing).resolvePostedReviewObservationin the webhook receiver — reads the signed payload directly, sharing no control flow with the wake path (see Risks for why that separation is load-bearing).github-review-request-funnelGrafana dashboard — the dashboard that read healthy right through the blackout.Why the webhook, and not the post call
The AC this was written against says "increment at the point the post succeeds". There is no such point in this codebase. The control plane never posts a review: there is no
POST /pulls/{n}/reviewsanywhere inserver/, no octokit dependency, and its only two GitHub writes are a commit status and an issue back-link comment. Ally composes and posts its own review by runninggh pr reviewinside its pod.Observing the signed webhook is therefore not a compromise but the stronger option: it is GitHub telling us the artifact exists — a first-party observation of the published review, rather than the poster's own report that it published one.
Verification
The test file includes a negative control: it asserts the counter does not increment for a human's review, for the control plane's own back-link comment, for an agent's review request that quotes a heading, for a comment on a plain issue, and for
edited/dismissed/deletedactions. It fails against currentmaster, where neither metric exists.Note the live-in-prod check cannot be satisfied by merge alone —
paperclip-apiis currently 318 commits behindmaster(deployede307f937b, 2026-08-08), so the series appears only after a production deploy.Risks
Low risk overall — the change is purely additive (445 insertions, 0 deletions) and no existing behaviour is altered. Three things worth a reviewer's attention:
sum(increase(paperclip_github_review_posted_total[2h])) == 0. On an absent series that inner expression is an empty vector, theandyields nothing, and the alert stays silent during exactly the outage it exists to catch. Absent and zero render identically and mean the opposite. Removing the zero-init loops would silently disarm the alert.resolveEventContext. That resolver returnsnullfor precisely the artifacts this must count: a clean comment-shaped review is neither a review request nor actionable feedback (isActionablePrReviewCommentrequires findings), and the reviewer's own formal review is dropped downstream as a self-echo (BLO-15799). Both are correct wake decisions; routing the counter through them would have silently zeroed it.Cardinality:
surfaceis closed at 2 andrepois bounded by the GitHub App installation (98 repositories measured 2026-08-16), so the worst case is 196 series and the live set is a single-digit subset. Redeliveries may double-count; that is accepted and documented, since this counter exists to separate zero from non-zero.Model Used
claude-opus-5, 1M context window), running as the Paperclip CTO agent with tool use.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templatepnpm installin this worktree could not complete (the filesystem linked ~16 files/20s, hours to finish), so CI is the authoritative validator for them. This PR is a draft until they are green.