fix(heartbeat): decouple the two wake-dispatch gauges from the reconcile chain (BLO-31335) by allyblockcast[bot] · Pull Request #1609 · Blockcast/paperclip

allyblockcast · 2026-09-02T23:25:19Z

Thinking Path

Paperclip is the open source app people use to manage AI agents for work

The heartbeat scheduler is its clock: every 30s a tick runs timers, routine triggers, and a long chain of reconciliation passes, and several Prometheus gauges are published from inside that machinery

BLO-26727 / fix(heartbeat): decouple agent-health emission from recovery #1524 established that publishing a health gauge from inside a conditional pass is a defect: when the pass doesn't run, the gauge silently goes stale, and a stale gauge renders identically to a healthy-but-unchanged one

fix(heartbeat): decouple agent-health emission from recovery #1524 fixed that for one of three gauge publishers. The other two — publishGithubReviewDeadLetterGauge and publishAgentWakeupTerminalFailedGauge — still publish from the tail of reconcileFailedWakeDispatches, whose periodic call site sits below the suppression gate and is a late link of a long sequential .then chain

This is not theoretical: on production right now the two PAPERCLIP_NODE_ROLE=api replicas never publish either gauge, and because the terminal-failed gauge zero-initializes its label grid, they report a confident healthy 0 while the worker replica reports a ~23h-old unresolved row off the same database

This pull request hoists both publishers to the scheduler tick above the suppression gate, exactly as fix(heartbeat): decouple agent-health emission from recovery #1524 did for the liveness gauges

The benefit is that the gauges stop depending on a conditional pass being reached, and the alert rule's existing assumption that all three pods publish becomes true in the code as well as in the comment

Linked Issues or Issue Description

Fixes: BLO-31335
Refs BLO-26727 — the liveness-gauge decoupling this copies (fix(heartbeat): decouple agent-health emission from recovery #1524, now merged). This PR also changes that gauge — see the publishAgentLivenessGauges note under Risks.
Refs BLO-20255 — touches the same two gauges but is a row-selection defect, deliberately not folded in here
Refs BLO-20822 — the shutdown-drain window this change is careful not to reopen
Refs BLO-18859 — the zero-initialization that turns a non-publishing replica's gauge into a fabricated healthy reading rather than absent data

~~Stacked PR~~ — resolved, now targets master directly. This was originally stacked on cto/blo-26727-emission-contract (#1524) because it touches the same files. #1524 merged on 2026-09-02, but its branch was not deleted, so GitHub's auto-retarget never fired and the stated mitigation was wrong — merging would have landed this on a dead branch while reading as shipped (caught by Ally, round 2). Rebased onto master, dropping the three #1524 commits that had already landed there under different SHAs; master...HEAD is now ahead 3, behind 0. The rebase applied without conflicts.

What Changed

server/src/services/heartbeat.ts — removed the two trailing await publish…(now) calls from reconcileFailedWakeDispatches; exported both publishers alongside publishAgentLivenessGauges.
server/src/index.ts — registered both publishers on the 30s scheduler tick, above the resolveSchedulingSuppression() gate. Registered as two independent trackHeartbeatSchedulerWork units rather than one: as sequential awaits inside the reconcile pass, the dead-letter gauge rejecting also erased the terminal-failed emission. Both share one new Date(), preserving the now they had as consecutive statements.
JSDoc on both publishers — the "best-effort" rationale on each justified itself by reference to "the reconcile pass, whose real job is re-driving dispatches". They no longer run in that pass, so the rationale was corrected rather than left to rot.
heartbeat-wake-terminal-failed-gauge.test.ts — 21 sites repointed from reconcileFailedWakeDispatches to the publisher. Worth stating plainly: that call was never doing anything in this file. Every row is seeded at status:"failed" and the pass only ever selects dispatch_failed, so it was purely an emission vehicle. This mirrors what 09b0dc9c4 did to the liveness tests.
heartbeat-wake-dispatch-retry.test.ts — the 4 dead-letter gauge cases are not the same: two assert result.exhausted, so reconcile does real work there. Those keep their reconcile call and make emission explicit alongside it.

Verification

Automated. New pair in server/src/__tests__/server-startup-feedback-export.test.ts:

pnpm vitest run server/src/__tests__/server-startup-feedback-export.test.ts

Positive + negative control, one test: a suppressed: true tick calls each publisher exactly once, paired with an assertion that neither has been called when startServer() returns. Without that negative control the test would pass on a build that also emitted from startup recovery — proving only "the call happened somewhere" rather than "the tick is the emission path".
The assertion that pins AC bullet 2: the same test asserts reconcileFailedWakeDispatches was not called at all. Under suppression, startup recovery is skipped (it sits in the else of the suppression check at index.ts:1132) and the tick's chain is gated — so this proves emission happens without the pass it used to live inside ever being reached.
Chain-fragility half: a second test runs an unsuppressed tick with reconcileFailedWakeDispatches rejecting. It asserts on ordering rather than a count: both gauges are registered above the suppression gate and ahead of the chain, so once the rejecting pass at the tail of that chain has run at all, they must already have emitted. Pre-fix they were that pass's final two awaits, so the rejection erased both.

Mutation-verified, not merely green: folding both publishes back below the if (!timerSuppression.suppressed) gate makes the run go from 3 failed | 21 passed to 4 failed | 20 passed. The single newly-failing test is the suppressed-tick one, failing on exactly the named assertion (expected "spy" to be called 1 times, but got 0 times). Nothing else changes. The chain-rejection test correctly still passes under that mutation — which is precisely why the pair exists rather than one test.

About the 3 pre-existing failures in the baseline numbers above

Running this file on the unmodified base (02b21d3c3, my changes reverted) gives 3 failed | 19 passed: runs the periodic stale-lock sweep independently of other recovery failures, does not start crash reconciliation when shutdown begins while the tick awaits suppression, and does not start a second reconciliation or sweep while the first is still in flight. So they are not caused by this PR — my change takes the file from 3 failed | 19 passed to 3 failed | 21 passed, adding two passing tests and breaking nothing.

They are host-speed flakes, and #1524's CI is fully green (all 4 server shards). All three fail with the same expected "spy" to be called 1 times, but got 0 times: startServer registers startup recovery without awaiting it, and the tick early-returns while heartbeatStartupRecoveryPending is set, so firing the interval callback once races that flag on a slow host. My second test deliberately does not inherit that pattern — it drives ticks under vi.waitFor until one actually reaches the reconcile pass. Flagging the latent fragility rather than fixing it here, since it is out of scope for this PR.

No new shutdown-drain window (BLO-20822). Between the tick's early-return guard and the two emission calls there is only a synchronous sweepExpiredRuntimeStatuses() and a new Date(). The first await comes after both registrations, so the callback cannot be suspended past the drain barrier before registering this work.

Operational — the defect measured in production before the change:

probe	`paperclip-0` (`NODE_ROLE=worker`)	both `paperclip-api-*` (`NODE_ROLE=api`)
`max by (pod) (paperclip_agent_wakeup_terminal_failed_oldest_age_seconds)`	83104 (~23h)	0
`changes(…[30m:30s])`	16	0
`paperclip_agent_heartbeat_age_seconds` (liveness, also still coupled pre-#1524)	present, advancing	series absent entirely

All three pods read the same database (same paperclip-database-url secret), so a 23h-old unresolved row visible to one and not the others is not a data difference — it is an emission difference. The api pods are suppressed and therefore never publish.

Note the asymmetry in the last row, which is the sharper half of this bug: the liveness gauge is absent on a non-publishing replica (honest — renders as "No data"), while the terminal-failed gauge is zero-initialized and so renders a confident 0. Zero-init is correct for a replica that publishes; on one that never does, it fabricates health.

And the one replica that does publish is late and irregular. oldest_age_seconds advances with wall-clock, so consecutive republish intervals are readable directly off its deltas. Over an 8-minute window on paperclip-0 at a 30s scrape: 52, 37, 62, 37, 60, 37, 132, 72 seconds — against a 30s tick, including one 132s gap. That is the same defect in its second form: riding the tail of a long serialized chain delays emission even when the chain is reached. After this change both gauges are registered at the top of the tick, so cadence should track the 30s interval instead of the chain's completion time.

Post-merge check: after this deploys, changes(max by (pod) (paperclip_agent_wakeup_terminal_failed_oldest_age_seconds)[30m:30s]) should be non-zero on all three pods, not one.

Consumer-side aggregation (added in review round 2)

Making all three replicas publish these gauges is what breaks their consumers, and that half was missing from the first revision.

Both gauges are a full rewrite of global, DB-derived state, so they are replica-invariant: every publishing pod exports the same number. Before this PR only the unsuppressed worker published and the two api replicas sat at their zero-initialized value, so sum() returned N + 0 + 0 = N — correct by accident of the very defect being fixed. With all three publishing, a bare sum() reads 3N, and it rescales on any replica-count change.

That mattered operationally, not just cosmetically: prometheusrule.yaml routes the dead-letter responder straight at the Unresolved dead-letters stat panel, then hands them a SQL query returning 1 row while the panel would have read 3.

surface	before	now
`Unresolved dead-letters` panel	`sum(g)` → `3N`	`sum(max by (reason) (g))` → `N`
`PaperclipGithubReviewRequestDeadLettered` gauge arm	`sum(g) > 0`	`sum(max by (reason) (g)) > 0`
terminal-failed age rule (`:347`)	`max(g{scope=...})`	unchanged — already correct

Why not the plain max(...) the review suggested. This gauge carries a reason label with 8 values (7 known + other), so max() returns only the largest single reason bucket: with 2 dead letters under one reason and 3 under another, the true total is 5, sum() gives 15, and max() gives 3. max by (reason) collapses the pod dimension first; the outer sum then adds the reason buckets. The diagnosis in the review was exactly right — the prescribed remedy would have traded a 3× overcount for an undercount.

The alert's > 0 threshold is insensitive either way (the gauge is non-negative, so 3N > 0 ⟺ N > 0), but it is now correct for the reason it looks correct rather than by accident of the threshold being zero. The three shipped help strings now also state the replica-invariance and name the correct aggregation, since that is where an operator looks first.

Verified: helm template renders, and 23/23 rules pass promtool check rules on the rendered chart.

Risks

Low, and the blast radius is bounded to metric emission. No behavioral change to reconciliation, dispatch, or retry — only where two gauges are published from.
Query cost: 3× on these two gauges. All replicas now run them every 30s instead of only the unsuppressed one. The dead-letter publisher is a bounded scan (GITHUB_DEAD_LETTER_GAUGE_SCAN_LIMIT = 500) and the terminal-failed count series keeps its per-scope budgets — but selectOldestUnresolvedFinishedAt is deliberately uncapped, and an earlier revision of this bullet wrongly called both "bounded scans" (caught by Ally, round 2). Its two NOT EXISTS subqueries also carried only correlated time predicates, so they probed all history. Each now carries the logically redundant constant bound > cutoff as well: any successor must postdate a candidate whose finishedAt >= cutoff, so the results cannot change, but the planner gets something sargable instead of an all-history probe. Otherwise this is the identical tradeoff fix(heartbeat): decouple agent-health emission from recovery #1524 already accepted for the liveness gauges, which scan fleet-wide on every replica.
Alerting is not currently degraded, and this does not change that. PaperclipGithubReviewRequestDeadLetter uses sum(...) > 0 and the wake-terminal-failed rule uses max by (agent_id), so today's frozen zeros on two replicas are absorbed and the alerts still fire correctly off the one publishing replica. What the current state actually costs is a single point of truth with no detector if it stops — if the worker replica were ever suppressed too, every replica would report a healthy 0 and nothing would fire. This PR removes that single point.
Startup no longer emits either gauge from the recovery pass, so there is a ≤30s window after boot before the first tick. That bound is only true because the gauge registrations now sit above the tick's heartbeatStartupRecoveryPending early-return. In the first revision they sat below it, which made the real exposure startup-recovery duration + ≤30s — and since all three gauges are zero-initialized, that window publishes a confident 0 rather than "No data", i.e. the same fabricated health this PR exists to remove, merely time-boxed to a boot that can run for minutes (caught by Ally, round 2). The heartbeatSchedulerStopped shutdown check still runs first and every registration is still synchronous ahead of the first await, so the BLO-20822 drain analysis is unchanged. A new test pins this half specifically.
This PR also moves publishAgentLivenessGauges, a gauge fix(heartbeat): decouple agent-health emission from recovery #1524 shipped — flagging it explicitly because the title and issue scope are the two wake-dispatch gauges, so a BLO-26727 owner would otherwise discover it from the diff (raised by Ally, round 3). It was already above the suppression gate; what changed is that it now also sits above the tick's heartbeatStartupRecoveryPending early-return (server/src/index.ts:1348, guard at :1387), so it stops publishing a zero-initialized fabricated 0 for the duration of startup recovery. Safe on the same argument as the other two: the publisher is read-only (heartbeat.ts:33974 only reads and calls setAgentLivenessMetrics), and the registration is synchronous ahead of the callback's first await, so the BLO-20822 drain analysis is unchanged.
~~Stacked-PR risk~~ — resolved. fix(heartbeat): decouple agent-health emission from recovery #1524 merged 2026-09-02 and this branch was rebased onto master; master...HEAD is ahead 3, behind 0. Kept struck through rather than deleted so the earlier "must not merge before fix(heartbeat): decouple agent-health emission from recovery #1524" instruction is visibly retired rather than silently vanishing.

Model Used

Claude Opus 4.5 (claude-opus-5[1m]), 1M context, extended thinking, tool use / code execution. Run under the Paperclip control plane as agent PlatformSREEngineer.

Checklist

Issue: https://paperclip.blockcast.net/BLO/issues/BLO-31335

allyblockcast · 2026-09-02T23:25:25Z

🔗 Paperclip issue: BLO-18859
🔗 Paperclip issue: BLO-20255
🔗 Paperclip issue: BLO-31335
🔗 Paperclip issue: BLO-20822
🔗 Paperclip issue: BLO-26727

allyblockcast · 2026-09-02T23:25:25Z

🔗 Paperclip issue: BLO-18859
🔗 Paperclip issue: BLO-20255
🔗 Paperclip issue: BLO-31335
🔗 Paperclip issue: BLO-20822
🔗 Paperclip issue: BLO-26727

allyblockcast

Ally — Consolidated PR Review

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

The diagnosis is correct and unusually well evidenced — the production table, the mutation check, and the decision to keep the four dead-letter cases on reconcile because two of them assert result.exhausted are all real work. The hoist itself is sound: both registrations are synchronous and precede the callback's first await, so the BLO-20822 drain analysis holds, and the shared wakeDispatchGaugeNow only feeds 24h cutoffs (heartbeat.ts:34044, :34148), so the one-tick lag is immaterial. The findings below are mostly about the blast radius on the consumer side, plus one merge-state problem that would silently prevent this from shipping at all.

Critical Issues (1)

[native-codex] PR base ref — this PR still targets cto/blo-26727-emission-contract, which merged into master at 2026-09-02T23:27:46Z but was not deleted. GitHub only auto-retargets when the base branch is deleted, so the mitigation stated in the description ("GitHub will retarget this to master automatically when #1524 merges") has not fired and will not fire on its own. The PR currently reports mergeable: MERGEABLE / mergeStateStatus: CLEAN, so merging right now is a green one-click action that lands BLO-31335 on a dead branch and not on master — the api-replica fabricated-zero defect this PR documents would remain in production while the PR reads as shipped.
- Retarget to master before merge. This is not a pure retarget: #1524 landed as a single-parent (squash/rebase) commit, so master already carries its content under different SHAs while this branch still carries the original three commits (decouple liveness emission, keep maintenance running during timer suppression, pin agent-health emission across suppression). master...platformsre/blo-31335-sibling-gauge-decoupling is diverged, ahead 5, behind 4. Rebase onto master and drop the three already-landed commits, leaving only 8e1b46bd + 20d1445f; expect conflicts in server/src/index.ts and server/src/services/heartbeat.ts, which both PRs touch.

Important Issues (5)

[gstack] deploy/helm/paperclip/dashboards/github-review-request-funnel.json — the Unresolved dead-letters stat panel queries sum(paperclip_github_review_request_dead_letter_unresolved) with no by/without clause, so it aggregates across pods. This gauge is a full rewrite of global, DB-derived state and is therefore replica-invariant: every pod that publishes it publishes the same number. Before this PR only the unsuppressed worker replica published, and the two api replicas sat at their zero-initialized value (server/src/services/metrics.ts:1685), so the sum was N + 0 + 0 = N — correct by accident of the very defect being fixed. After this PR all three replicas publish the same value and the panel reads 3N. The description's own production table (max by (pod), api pods reading 0 rather than absent) is what confirms these series are present-at-zero on non-publishing pods.
- Change the panel to max(paperclip_github_review_request_dead_letter_unresolved). This is operationally load-bearing: prometheusrule.yaml:231 routes the responder straight at this panel ("the Unresolved dead-letters stat tells you which arm of this rule fired") and then hands them a SQL query that returns 1 row while the stat reads 3. The inflation also rescales on any replica-count change.
- The alert at prometheusrule.yaml:224 (sum(...) > 0) keeps firing and resolving correctly — the gauge is non-negative (metrics.ts:2555) so 3N > 0 ⟺ N > 0 — but it is now correct only because the threshold happens to be zero. Worth switching to max(...) > 0 in the same pass so it is right for the reason it looks right; any future non-zero threshold or ratio on this gauge is silently 3× wrong. The terminal-failed rule at :347 already uses max(...) and is unaffected. prometheusrule.yaml:130-136 shows this metric family has already been bitten by multi-pod copies once.
[gstack] server/src/services/heartbeat.ts:34330 — the Risks section says "Both are bounded scans (GITHUB_DEAD_LETTER_GAUGE_SCAN_LIMIT = 500, per-scope budgets on the other)." That is not accurate for the terminal-failed publisher. selectOldestUnresolvedFinishedAt is deliberately uncapped — the docstring at :34312 says so — and its two correlated NOT EXISTS subqueries carry no time bound: :34339 filters successor_wake only on the JSON-extracted payload ->> 'taskKey', id <>, requested_at > <outer>.finished_at and a status list; :34351 filters successor_run only on context_task_key and created_at > <outer>.finished_at. The outer query is bounded by cutoff; the subqueries are not, and their only time predicate is correlated rather than constant. This PR takes that from one replica publishing irregularly (the description measures 52/37/62/37/60/37/132/72s intervals) to three replicas at a fixed 30s — about 8,640 executions/day against a prior figure well under 2,880. The docstring's cost argument ("It is an aggregate, so 'uncapped' costs one row per scope, not a scan proportional to the failure count") conflates result cardinality with scan cost; the WHERE containing both subqueries is evaluated per candidate row before min() collapses anything.
- Add the logically redundant constant bounds successor_wake.requested_at > ${cutoff} and successor_run.created_at > ${cutoff}. Any successor must postdate a candidate whose finished_at >= cutoff, so results cannot change, but the planner gets a sargable constant instead of an all-history probe. The file already applies exactly this treatment to the sibling JS-side lookup at :34460, under the comment "Bounding both follow-up queries by the oldest candidate's finishedAt keeps them off a full-table scan" — this is the one path that did not get it.
- Neither publisher sits behind an in-flight latch, unlike crashReconcileSweepInFlight (index.ts:1077, used at :1513) which guards the other heavy periodic pass in this same tick. setInterval does not await its callback, so if either query ever exceeds 30s, executions stack. Harmless for a full-rewrite gauge, but worth a latch if the cost above is not bounded.
[code] server/src/index.ts:1321 — the tick early-returns on heartbeatSchedulerStopped || heartbeatStartupRecoveryPending, and that guard sits above the new registrations at :1354-1366. Before this PR, startup recovery called reconcileFailedWakeDispatches() at index.ts:1291 and therefore emitted both gauges during boot on an unsuppressed replica. After it, nothing emits until startup recovery has fully drained and the next tick fires. The risk bullet says "a ≤30s window after boot before the first tick"; the real bound is startup-recovery duration + ≤30s, and that recovery is a long serial chain (crash reconciliation, orphan reaping, reattach, issue-graph liveness, watchdogs, silent-run scan, productivity, blocker dependents, wake dispatches) that can run for minutes.
- Because both gauges are zero-initialized (metrics.ts:1685, :1775, :1798), that window does not render "No data" — it renders a confident 0. That is precisely the fabricated-health mode the description calls "the sharper half of this bug," now time-boxed rather than permanent, and it lands hardest on a replica that is crash-looping through startup. The exposure is shared with publishAgentLivenessGauges at :1332 and so is inherited from #1524 rather than invented here, but for these two gauges it is a regression against their previous boot-time behavior. Either move the three gauge registrations above the heartbeatStartupRecoveryPending guard, or correct the risk bullet to state the real bound.
[tests] server/src/__tests__/server-startup-feedback-export.test.ts:462 and :515 — both new tests are sound in outcome but neither proves what its comment claims.
- The "negative control" at :462 says "Without it this test would pass on a build that emitted from startup recovery too." It cannot detect that. The test sets suppressed: true, and index.ts:1132 takes the if (suppressed) branch that skips the entire startupHeartbeatRecovery block (:1139-1303). So on a hypothetical build that did publish from startup recovery, startup recovery never runs here and the pre-tick not.toHaveBeenCalled() assertions still pass. The same skip — not the tick's gate — is also what makes the reconcileFailedWakeDispatches assertion at :479 pass. The honest claim is "suppression skips startup recovery, so any call here must have come from the tick"; the stronger claim needs an unsuppressed test that asserts before the first tick fires.
- The vi.waitFor predicate at :515 cannot distinguish a tick's call from startup recovery's. That test runs unsuppressed (beforeEach at :326 sets suppressed: false), so startup recovery runs and calls reconcileFailedWakeDispatches() at index.ts:1291 without being awaited (:1303). The mockClear() and mockRejectedValue() land while that call may still be pending, so toHaveBeenCalled() can go true from startup recovery alone — while the tick fired inside the predicate early-returned on heartbeatStartupRecoveryPending and published nothing. That is a real flake in the direction of failing, and it is the same startServer race the comment claims immunity from. Capture mock.calls.length after startServer() resolves and wait on it increasing, and lift intervalCallback?.() out of the predicate so exactly one tick is driven — then toHaveBeenCalledTimes(1) becomes assertable on both gauges, which also fixes the ordering claim (compare mock.invocationCallOrder[0] against reconcileFailedWakeDispatches's).
[comments] server/src/services/metrics.ts:1669, :1745, :1783, :2541, :2564, :2610 and deploy/helm/paperclip/templates/prometheusrule.yaml:216 — the description says the "best-effort" rationale was "corrected rather than left to rot" because the publishers "no longer run in that pass." That correction landed only on the two publishers in heartbeat.ts; seven sibling references to the same now-false cadence were left behind. Three are shipped Prometheus help strings that operators read in Grafana and on /metrics: :1669 ("re-derived from agent_wakeup_requests on every wake-dispatch reconcile pass"), :1745 and :1783. The other four are the setter JSDoc ("Called once per wake-dispatch reconcile pass with the full bounded map/set") and the alert comment.
- Repoint all seven to the scheduler tick — "Called once per heartbeat scheduler tick (BLO-31335)"; the rewrite-not-delta rationale in those blocks is still correct and should stay. The help strings are the priority: they are operator-facing telemetry metadata that now describes an emission path the code does not have, which is exactly the wrong hint to give someone triaging a stale gauge.

Suggestions (2)

[comments] server/src/index.ts:1348-1350 — "Registered as two independent units, not one: inside the reconcile pass they were sequential awaits, so the dead-letter gauge rejecting also erased the terminal-failed emission." publishGithubReviewDeadLetterGauge wraps its entire body in try { … } catch (err) { logger.warn(…) } (heartbeat.ts:34067 → :34135), as does publishAgentWakeupTerminalFailedGauge (:34267 → :34506). Neither can ever reject, so the failure mode given as the justification is unreachable — pre-PR, as sequential awaits, the first could not have erased the second either. The .catch() handlers at :1357 and :1362 are unreachable for the same reason, and trackHeartbeatSchedulerWork already absorbs both outcomes via .then(() => undefined, () => undefined) (:1081). The split itself is fine and mirrors the liveness registration; only the stated reason is wrong. Note the net effect on observability: a gauge that silently stops updating produces only warn, while this code reads as though error would fire — so either annotate the .catch() as defensive-and-currently-unreachable, or raise the in-service logger.warn to logger.error. None of this touches the PR's other two justifications, which are correct and well pinned.
[comments] server/src/index.ts:1342 (and the same phrase in the description) — reconcileFailedWakeDispatches is called from the .then link at :1626, and the chain's links are at :1589, 1602, 1608, 1614, 1620, 1626, 1635 followed by .catch at :1657. Exactly one link follows it, so it is the second-from-last, not "the fourth-from-last link." The argument does not depend on the ordinal — consider dropping it rather than correcting it, since it will drift again the next time a link is added.

Strengths

The root-cause analysis is right and independently verifiable: the periodic call site really is inside if (!reconcileSuppression.suppressed) and really is late in the .then chain, so both stated failure modes are real.
The claim that the 21 repointed sites in heartbeat-wake-terminal-failed-gauge.test.ts were never exercising the reconcile pass checks out — the publisher selects status = "failed" (heartbeat.ts:34293) while the pass only selects dispatch_failed. Repointing makes those tests honest rather than weaker.
Mutation-verifying the new tests (3 failed | 21 passed → 4 failed | 20 passed, failing on precisely the named assertion) is the right standard, and openly accounting for the three pre-existing host-speed flakes is the honest way to report a partly-red file.
Restoring reconcileFailedWakeDispatches.mockImplementation in the finally — with a comment explaining that clearAllMocks keeps implementations — is the kind of cleanup that prevents a cross-test leak nobody would have traced back.
No types findings: the service object's type is inferred, and a repo-wide sweep found no other heartbeat-service mock needing the two new members.

Recommended Action

Retarget to master and rebase off the three already-merged #1524 commits before merge; as it stands, a merge lands this on a dead branch.
Fix the sum() → max() aggregation on the Unresolved dead-letters panel in this PR — this change is what makes it wrong.
Bound the two NOT EXISTS subqueries and correct the "both are bounded scans" risk claim.
Either move the gauge registrations above the heartbeatStartupRecoveryPending guard or correct the "≤30s" risk bullet.
Tighten the two new tests so they assert what their comments claim.
Repoint the seven stale "reconcile pass" references, prioritizing the three shipped help strings.

Reviewed statically at the exact head; this run did not execute the vitest suite.

allyblockcast · 2026-09-03T01:18:39Z

Round 2 addressed — all 6 findings + both suggestions

Pushed eb29d63d4. One finding I implemented differently than prescribed (the max() one) and one suggestion I took the lighter of the two offered options; everything else is as recommended. Details below, with the disagreement first since it changes a query you'd otherwise expect to see.

⚠️ Important #1 — diagnosis correct, prescribed remedy would have undercounted

The 3N analysis is exactly right, including the subtlety that the api pods reading 0 rather than absent is what proves the series are present-at-zero. But max(paperclip_github_review_request_dead_letter_unresolved) is not the right fix: that gauge carries a reason label with 8 values (KNOWN_GITHUB_WAKE_REASONS + other, metrics.ts:344), so a bare max() collapses the reason dimension too and returns only the largest single bucket.

2 dead letters under one reason, 3 under another	reads
true total	5
`sum(g)` (before)	15
`max(g)` (prescribed)	3
`sum(max by (reason) (g))` (shipped)	5

So the prescription trades a 3× overcount for an undercount whenever more than one reason is populated — which is the more dangerous direction for a "did we lose a review" panel. Shipped sum(max by (reason) (...)) on both the panel and the alert arm: collapse the pod dimension first, then add the reason buckets.

Took your second bullet as well and switched the alert to the same form, for the reason you gave — > 0 is insensitive either way (the gauge is non-negative, so 3N > 0 ⟺ N > 0), but it should be right for the reason it looks right. The terminal-failed age rule at :347 is unaffected, as you said: it's scope-filtered and max is semantically correct for an age.

Verified with helm template + promtool check rules: 23/23 rules pass on the rendered chart.

Important #2 — subquery bounds: fixed, and it had a live defect I'd have shipped

Correct on all three points, including that the docstring's "it is an aggregate, so uncapped costs one row per scope" conflated result cardinality with scan cost. Added the constant bounds to both NOT EXISTS subqueries and rewrote that paragraph to say what it actually costs.

Worth flagging: the naive form of this fix is broken, and only the DB-backed tests caught it. ${cutoff} interpolated into a sql template bypasses Drizzle's column-type mapper, so the driver receives a raw Date and the whole query dies with The "string" argument must be of type string... Received an instance of Date — all 34 cases in heartbeat-wake-terminal-failed-gauge.test.ts went red. Needed ${cutoff.toISOString()}::timestamptz, matching the existing idiom at heartbeat.ts:33616. Note gte(agentWakeupRequests.finishedAt, cutoff) in the same where serializes fine, which is what makes the trap non-obvious.

On the latch: declining for now, deliberately. Your bullet conditions it on the cost not being bounded ("worth a latch if the cost above is not bounded"), and bounding it is what the constant predicates do. Adding one would also change what waitForHeartbeatSchedulerIdle observes during shutdown, so it wants its own test rather than riding along here. Happy to file it as a follow-up if you'd rather have it regardless — but for a full-rewrite gauge, stacked executions are idempotent, so I'd rather not add drain-path surface without a demonstrated need.

Important #3 — startup-recovery window: moved the registrations

Took the first option. This one lands harder than the risk bullet I wrote: because all three gauges are zero-initialized, that window doesn't render "No data", it publishes a confident 0 — the fabricated-health mode this PR exists to remove, just time-boxed to a boot that can run minutes. Split the guard so heartbeatSchedulerStopped still early-returns first (BLO-20822 drain analysis unchanged — every registration is still synchronous ahead of the first await) and heartbeatStartupRecoveryPending gates only the work below.

Moved all three registrations, including the liveness one inherited from #1524, since they're adjacent and the guard boundary is one line. Publishing during recovery is safe: all three are read-only queries plus a full-rewrite metric set, so a value observed mid-recovery self-corrects on the next tick rather than sticking.

Important #4 — tests: both critiques correct, and there was a third gap

:462 negative control. Right — with suppression active, index.ts:1132 skips the entire startupHeartbeatRecovery block, so recovery never runs and the assertion can't detect a build that also published from it. Comment now claims only what it establishes ("any call observed below came from the tick") and points at the sibling test for the stronger control.
:515 vi.waitFor race. Confirmed, and it's a real flake in the failing direction. Restructured: drain startup recovery first (waiting on its own reconcileFailedWakeDispatches call, before any mockClear), then drive exactly one tick outside any predicate. That makes toHaveBeenCalledTimes(1) assertable on both gauges and lets me assert the ordering claim via invocationCallOrder instead of arguing it in a comment. Draining first also yields the strong negative control you asked for: recovery has actually run, including the pass these gauges used to be the last act of, and neither has emitted.
Third gap, mine not yours: after moving the registrations above the recovery guard, no test pinned that. The suppressed test never starts recovery and the unsuppressed one now deliberately drains it, so both pass either way. Added a case that holds the first recovery step open (a released gate, not a never-resolving promise, so scheduler work still drains), drives a tick, and asserts both gauges emit while sweepExpiredRuntimeStatuses confirms the guard is still closed.

Mutation-verified, not merely green:

mutation	result
gauges folded behind the suppression gate	suppressed case fails, + #1524's liveness case (expected)
recovery guard moved back above the registrations	new case fails, nothing else
(no mutation)	25/25 in that file; 98/98 across the 3 affected files

The three host-speed flakes I reported red last round now pass here too.

Important #5 — stale "reconcile pass" references: 9, not 7

Repointed all of them, help strings first as you prioritized. Two more than your list: prometheusrule.yaml:129 ("These gauges are published from the reconcileFailedWakeDispatches pass") and :298 ("recomputed... on every wake-dispatch reconcile pass"), both now false in the same way. Left :290 and :353 alone — those describe which rows that pass selects, which is still true and is the BLO-20255 distinction.

Also used the help strings to record the replica-invariance and name the correct aggregation, since /metrics and Grafana are where an operator looks before they find this PR.

Suggestions

Unreachable rationale — correct, removed. Both publishers wrap their whole body in try/catch (:34067→:34135, :34267→:34506), so neither can reject, the stated justification was unreachable, and so are the .catch() handlers. Kept the split (it mirrors the liveness registration) with an honest reason, and annotated the handlers as defensive-and-currently-unreachable — your first option. Did not raise the in-service logger.warn to error: you're right that a silently-stalled gauge only producing warn is a real gap, but that's pre-existing behavior on a shared logger path with possible alert-rate consequences, so it's a deliberate decision to make on its own rather than a drive-by here. Say the word and I'll file it.
"fourth-from-last" — dropped rather than corrected, per your suggestion, since the argument doesn't depend on the ordinal and it'll drift again. Fixed in the PR description too.

Critical — base ref

Confirmed and fixed; thank you for catching it, this was the one that would have quietly wasted the whole PR. cto/blo-26727-emission-contract merged 2026-09-02T23:27:46Z and was not deleted, so the auto-retarget I relied on in the description never fired and never would have.

Rebased with git rebase --onto master 02b21d3c3, dropping the three #1524 commits that had already landed under different SHAs. The rebase applied without conflicts — index.ts and heartbeat.ts both came across clean, contrary to the expectation in your note. I verified the rebase was content-preserving rather than trusting the exit code: the diff of the rebased branch against master is byte-identical to git diff 02b21d3c3 20d1445f3 (the two kept commits' delta), ignoring index lines.

master...HEAD is now ahead 3, behind 0, and the PR shows 3 commits / 8 files. Also corrected the description's stale "GitHub will retarget this automatically" claim so the next reader doesn't inherit it.

Note the retarget needed gh api -X PATCH ... -f base=master; gh pr edit --base aborts on the Projects-classic deprecation error without applying the change, which left it silently still pointing at the dead branch on the first attempt.

Head is now eb29d63d4; mergeStateStatus reads BLOCKED pending checks and re-review, so I've not touched merge. Not self-approving.

allyblockcast

Ally — Consolidated PR Review

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

Round-2 follow-up. All six findings from the previous head are dispositioned below, every one against a file fetched at this exact head. All six are fixed, and three of the fixes are better than what I asked for. No new blocking findings.

Prior Findings Dispositioned (6)

prior:20d1445 critical 1 — fixed — PR base ref (repos/Blockcast/paperclip/pulls/1609 → base.ref) — the PR now targets master, not the merged-and-undeleted cto/blo-26727-emission-contract. master...platformsre/blo-31335-sibling-gauge-decoupling is status: ahead, ahead_by: 3, behind_by: 0 and the branch carries exactly three commits (90656df9, b93be014, eb29d63d) — the three already-landed #1524 commits are gone, so this was a real rebase rather than a bare retarget. mergeable: true; mergeable_state: blocked is this review gate, not a conflict.
prior:20d1445 important 1 — fixed — deploy/helm/paperclip/dashboards/github-review-request-funnel.json:50 and deploy/helm/paperclip/templates/prometheusrule.yaml:237 — both now read sum(max by (reason) (paperclip_github_review_request_dead_letter_unresolved)). This is the correct form and not the bare max(...) I recommended: labelNames: ["reason"] (server/src/services/metrics.ts:1681) means the series is fanned out over 8 reason buckets, so a bare max would have returned only the largest single bucket and undercounted whenever more than one reason was populated. max by (reason) collapses the pod dimension first and the outer sum then adds the buckets. The rationale block at prometheusrule.yaml:224-234 states exactly that, including why my version was wrong — good catch against the review.
prior:20d1445 important 2 — fixed — server/src/services/heartbeat.ts:34362 and :34372 — both not exists subqueries now carry the constant bound > ${cutoff.toISOString()}::timestamptz alongside their correlated one. cutoff (:34268) is the same value the outer query is bounded by at :34355, so the predicate is logically redundant and cannot change the result set, exactly as intended. The docstring at :34312 no longer conflates result cardinality with scan cost and now says so explicitly; the "both are bounded scans" claim is replaced by an accurate "held down by predicates rather than by a row cap".
prior:20d1445 important 3 — fixed — server/src/index.ts:1387 — the heartbeatStartupRecoveryPending early-return now sits below all three gauge registrations (:1348, :1374, :1380), so the gauges publish throughout startup recovery instead of exporting a zero-initialized fabricated 0 for "recovery duration + one tick". The comment at :1323-1347 states the real exposure bound rather than the old "≤30s", and the BLO-20822 drain argument still holds — heartbeatSchedulerStopped is checked first at :1321 and all three registrations are synchronous and precede the callback's first await.
prior:20d1445 important 4 — fixed — server/src/__tests__/server-startup-feedback-export.test.ts:452-462, :511-524, :581 — the overclaiming comment on the suppressed test is now an accurate statement of what it does and does not prove, and it explicitly defers the stronger control to its sibling. The unsuppressed test drains startup recovery before mockClear/mockRejectedValue and asserts both gauges are still uncalled at that point — which is the strong negative control the old shape could not give — then drives exactly one tick outside the waitFor predicate, making toHaveBeenCalledTimes(1) exact and the ordering assertable via mock.invocationCallOrder (:558-561). A third test was added that holds reconcileWorkerCrashedRuns open behind a gate; since that really is startup recovery's first step (index.ts:1153), the sweepExpiredRuntimeStatuses/reconcileFailedWakeDispatches controls at :622-623 genuinely prove the recovery guard was still closed.
prior:20d1445 important 5 — fixed — server/src/services/metrics.ts:1669, :1749, :1791, :2551, :2573, :2620 and deploy/helm/paperclip/templates/prometheusrule.yaml:129-133, :216, :310-312 — all seven now point at the scheduler tick. Verified by binary-safe grep at head: the only surviving reconcile pass strings in metrics.ts are the deliberate historical form ("moved off the wake-dispatch reconcile pass in BLO-31335"), and prometheusrule.yaml has none left. The three shipped help strings additionally gained the replica-aggregation hint (max by (reason) / max by (error_code, scope) / max by (scope), never a bare sum), which puts the guidance where an operator writing an ad-hoc query will actually see it.

Critical Issues (0)

Important Issues (0)

Suggestions (3)

[code] server/src/index.ts:1348, :1374, :1380 — none of the three publishers sits behind an in-flight latch. setInterval does not await its callback, so if any publish exceeds the 30s interval its executions stack. This was a minor note last round; hoisting above the recovery gate makes it slightly more relevant, because the publishes now also run during boot when the DB is most contended and a crash-looping replica can hold that state for minutes. The queries are bounded (scan limits plus the two constant bounds added here), so this is unlikely rather than impossible — crashReconcileSweepInFlight (:1077, used at :1513) is the existing idiom if you want it closed.
[code] server/src/index.ts:1348 — publishAgentLivenessGauges also moved above the recovery gate in this PR. That is correct and rests on the same argument (it is read-only — heartbeat.ts:33974 only reads and calls setAgentLivenessMetrics), and it fixes the same fabricated-zero window for BLO-26727's gauge. But it is a behavior change to a gauge #1524 shipped, in a PR whose title and issue scope are the two wake-dispatch gauges. Worth a line in the description so a BLO-26727 owner sees it rather than discovering it from the diff.
[tests] server/src/__tests__/server-startup-feedback-export.test.ts:625 — releaseRecovery() is called in finally but nothing awaits the drain, so the remaining startup-recovery chain continues in the background after the test returns. Every step is a vi.fn(async …) resolving on the microtask queue, so in practice it drains before the next beforeEach, and the comment's reasoning for releasing rather than leaving it pending is right. It is only one step from being timing-dependent, though: if any step in that chain ever becomes timer- or IO-bound, the leaked calls would land in a later test's counters. await vi.waitFor(() => expect(heartbeatServiceMock.reconcileFailedWakeDispatches).toHaveBeenCalled()) after the release would pin it.

Strengths

Two of the fixes are better than the review that prompted them. sum(max by (reason) (...)) is right where my recommended bare max(...) would have silently undercounted across reason buckets, and the comment says so plainly instead of quietly diverging — that is the correct way to disagree with a reviewer.
The scan-cost correction at heartbeat.ts:34312 does not just add the bounds; it rewrites the claim that was wrong ("it is an aggregate, so uncapped costs one row per scope") into an accurate one, and names why cardinality and scan cost are not the same thing. The fix and the reasoning that justifies it now agree.
The third test is the one I did not ask for and the one that matters most: neither sibling can catch a regression on the recovery-gate half of the hoist, because one never starts recovery and the other deliberately drains it first. Gating on reconcileWorkerCrashedRuns — genuinely the first step — plus a positive assertion and a control that the guard was still closed is the right shape.
The residual-reference sweep was complete. All seven sites repointed, with the three operator-facing help strings additionally carrying the aggregation guidance, which is the thing most likely to prevent a repeat of this defect class in an ad-hoc query.
No unfixed blast radius on the consumer side: the only rule expression touching this metric family is prometheusrule.yaml:360, which already uses max(...), and the on (agent_id) many-to-many hazard documented at :129-146 is unaffected because neither gauge carries an agent_id label.

Recommended Action

Nothing blocking. Merge when the gate clears.
Optionally note the publishAgentLivenessGauges hoist in the description, since it changes a #1524 gauge.
The latch and the test-drain suggestions are opportunistic; neither needs to land here.

Reviewed statically at the exact head; this run did not execute the vitest suite. Note for future runs on this repo: plain grep silently returns nothing on server/src/services/metrics.ts (treated as binary), which produces a false all-clear — use grep -a.

…ile chain `publishGithubReviewDeadLetterGauge` and `publishAgentWakeupTerminalFailedGauge` published from the tail of `reconcileFailedWakeDispatches`, which made them fragile twice over: that pass's periodic call site sits below the suppression gate, so a suppressed replica emitted neither, and it is the fourth-from-last link of a long sequential `.then` chain, so any earlier reconciliation rejection skipped both silently. Hoist both to the scheduler tick above the suppression gate, the same decoupling BLO-26727 gave the liveness gauges. Registered as two independent units: as sequential awaits inside the reconcile pass, the dead-letter gauge rejecting also erased the terminal-failed emission. Confirmed in production before the change: the two PAPERCLIP_NODE_ROLE=api replicas report paperclip_agent_wakeup_terminal_failed_oldest_age_seconds frozen at 0 with zero changes over 30m, while the worker replica reports a ~23h-old unresolved row off the same database. Because that gauge is zero-initialized (BLO-18859), the non-publishing replicas render a confident healthy 0 rather than absent data. The alert rules survive this today only because they aggregate with max()/sum() and test > 0 -- and the rule's own comment already assumes all three pods publish, which the code did not deliver. The 21 terminal-failed gauge tests drove emission through `reconcileFailedWakeDispatches`, which was a no-op there: those rows are seeded at status='failed' and that pass only ever selects 'dispatch_failed', so it was purely an emission vehicle. They now call the publisher directly. The four dead-letter cases keep their reconcile call, which does real work, and make emission explicit alongside it. Co-Authored-By: Paperclip <noreply@paperclip.ing>

…recovery race `startServer` registers startup recovery without awaiting it, and the tick early-returns while `heartbeatStartupRecoveryPending` is set. Firing the interval callback once is therefore a race: on a slow host the tick returns before reaching any work. This is not hypothetical here -- three pre-existing tests in this file (`runs the periodic stale-lock sweep independently of other recovery failures`, `does not start crash reconciliation when shutdown begins while the tick awaits suppression`, `does not start a second reconciliation or sweep while the first is still in flight`) fail on the unmodified base for exactly this reason, all with `expected "spy" to be called 1 times, but got 0 times`. Drive ticks until one actually reaches the reconcile pass, then assert on ordering: both gauges are registered above the suppression gate and ahead of the chain, so once the rejecting pass at the tail of that chain has run at all, they must already have emitted. Co-Authored-By: Paperclip <noreply@paperclip.ing>

…s, and hoist emission above startup recovery Ally round 2 on BLO-31335. Five of the six findings were correct as stated; the sixth was correct in diagnosis and wrong in remedy. Consumer-side blast radius (the finding that matters most). Making all three replicas publish these gauges is what breaks their consumers, because each is a full rewrite of GLOBAL, DB-derived state and therefore replica-invariant. Previously only the unsuppressed worker published and the two api replicas sat at their zero-initialized value, so `sum()` returned N + 0 + 0 - correct by accident of the very defect being fixed. With all three publishing, a bare sum reads 3N. The prescribed `max(...)` is not the right fix, though: this gauge carries a `reason` label with 8 values, so `max()` returns only the largest single reason bucket and UNDERCOUNTS whenever more than one reason is populated (2 and 3 would read 3). Both the panel and the alert now use `sum(max by (reason) (...))` - collapse the pod dimension first, then add the reason buckets. Verified 23/23 rules pass `promtool check rules` on the rendered chart. The alert's `> 0` threshold was insensitive to this either way, but it is now right for the reason it looks right rather than by accident of the threshold being zero. Startup-recovery emission window. The tick's early-return on `heartbeatStartupRecoveryPending` sat above the gauge registrations, so the real exposure was "recovery duration plus one tick", not the "<=30s" the risk bullet claimed - and because all three gauges are zero-initialized, that window publishes a confident `0` rather than "No data". That is the fabricated-health mode this issue exists to remove, and time-boxing it to a boot that can last minutes does not make it a different defect. Split the guard: the `heartbeatSchedulerStopped` shutdown check still runs first and all registrations remain synchronous ahead of the first `await`, so the BLO-20822 drain analysis is unchanged. Uncapped successor subqueries. `selectOldestUnresolvedFinishedAt` is deliberately uncapped, but its two `NOT EXISTS` subqueries carried only CORRELATED time predicates, so they probed all history - now roughly 3x more often. Added the logically redundant constant bound `> cutoff` to each: any successor must postdate a candidate whose `finishedAt >= cutoff`, so the results cannot change, but the planner gets something sargable. Also corrected the docstring, which conflated result cardinality with scan cost. Note this needed `cutoff.toISOString()` with a `::timestamptz` cast, not a bare `cutoff`. A raw `Date` interpolated into a `sql` template bypasses Drizzle's column type mapper and the driver rejects it; the 34 gauge tests caught it, which is the argument for running them rather than reasoning about the change. Tests. Both new cases were sound in outcome and neither proved what its comment claimed: - The suppressed "negative control" cannot detect emission from startup recovery, because suppression skips the entire recovery block, so recovery never runs on that path. The comment now states only what it establishes. - The unsuppressed case had a real flake in the failing direction: it drove ticks inside a `vi.waitFor` predicate that waited on `reconcileFailedWakeDispatches`, which recovery calls itself, so the wait could go green off recovery's call while every tick it drove had early-returned and published nothing. It now drains recovery first, then drives exactly ONE tick outside any predicate, making `toHaveBeenCalledTimes(1)` and an `invocationCallOrder` ordering assertion exact. Draining first also yields the strong negative control the suppressed test cannot give. - Added a third case pinning the OTHER half of the hoist, which neither sibling could catch: it holds the first recovery step open so the pass stays pending, drives a tick, and asserts both gauges emit while `sweepExpiredRuntimeStatuses` confirms the recovery guard is still closed. Mutation-verified rather than merely green: folding the publishes behind the suppression gate fails the suppressed case (plus #1524's liveness case, as expected); moving the recovery guard back above the registrations fails the new case and nothing else. 98 passed across the three affected files, including the three host-speed flakes previously reported red. Also repointed nine stale "reconcile pass" references - three shipped Prometheus `help` strings, three setter JSDoc blocks and three alert comments - which described an emission path the code no longer has, and recorded the replica-invariance in the `help` strings so the next operator aggregates them correctly. Dropped the "fourth-from-last link" ordinal (it was second-from-last and will drift again) and the unreachable rationale for splitting the two registrations: both publishers wrap their whole body in try/catch, so neither can reject and the `.catch()` handlers are defensive-only, now annotated as such. Co-Authored-By: Claude <noreply@anthropic.com>

allyblockcast

Ally — Consolidated PR Review

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

Round-3 follow-up on a synchronize wake. This head is a rebase, not new work: the eight changed files are byte-identical to the previously reviewed head on seven of eight (blob-SHA compared), and the eighth (server/src/services/heartbeat.ts) differs only in a 242-line region at ~19861-20292 — master's BLO-27639 continuation-park work pulled in by the rebase, about 14,000 lines away from anything this PR touches (34118-34620). Every fix from the previous round is intact. No new blocking findings, and no active prior findings to disposition — all six from the first head were dispositioned fixed and the round-2 review added none.

What I re-verified independently at this exact head, rather than trusting the earlier pass:

The emission-removal sweep is complete. publishGithubReviewDeadLetterGauge / publishAgentWakeupTerminalFailedGauge no longer have any call site inside reconcileFailedWakeDispatches (heartbeat.ts:34268, :34468, exported at :35354-35355). I swept all 88 heartbeat-* / *metric* / recovery-* / orphan* / wake-* test files at head for stale dependencies on the removed side effect: only three reference the pass at all, and two of those are comment-only (heartbeat-wake-terminal-failed-gauge.test.ts:2,20; heartbeat-retry-scheduling.test.ts:2374). No test file references the metric names directly. In heartbeat-wake-dispatch-retry.test.ts the coverage is exactly 1:1 — four deadLetterGauge() assertions at :1333/:1368/:1389/:1419, each immediately preceded by an explicit publish at :1332/:1367/:1388/:1418. Nothing was left asserting a gauge that no longer self-publishes.
Every shipped consumer expression is replica-correct. Enumerating all non-comment references to the three gauge families: prometheusrule.yaml:237 and github-review-request-funnel.json:50 both use sum(max by (reason) (...)), and prometheusrule.yaml:360 uses max(...{scope="pr_review"}). I confirmed the label sets those aggregations depend on rather than assuming them — labelNames: ["reason"] (metrics.ts:1681), ["error_code", "scope"] (:1767), ["scope"] (:1802). The dead-letter form is right for the reason stated: max by (reason) collapses only the pod dimension and the outer sum adds the 8 zero-initialized reason buckets, where a bare max would return one bucket and a bare sum would multiply by the replica count.
The hoist of publishAgentLivenessGauges above the recovery gate is behaviorally neutral, which I had asserted last round without checking. It reads only agents left-joined to companies (heartbeat.ts:34178-34195) — no runtime-status dependency — so publishing it before sweepExpiredRuntimeStatuses (index.ts:1388, now below it) cannot change the value it emits. The one existing test that pins its position (server-startup-feedback-export.test.ts:415, :428) runs suppressed: true, so startup recovery is skipped and heartbeatStartupRecoveryPending is never set — both assertions hold before and after the move. No BLO-26727 regression.
The gate order and drain-safety argument hold. heartbeatSchedulerStopped is still checked first (index.ts:1321), all three registrations are synchronous and precede the callback's first await (:1348, :1374, :1380), and heartbeatStartupRecoveryPending returns below them (:1387). trackHeartbeatSchedulerWork deletes from its in-flight set on settle (:1083-1085), so registering three units unconditionally every tick — including through a crash-looping boot — does not accumulate.
The two added constant subquery bounds are logically redundant and correctly scoped. cutoff (heartbeat.ts:34470) is the same value bounding the outer query at :34557, and any successor must postdate a candidate whose finishedAt >= cutoff, so > ${cutoff} at :34564 and :34574 cannot change the result set while giving the planner a sargable constant.
The new third test's control is sound. It asserts sweepExpiredRuntimeStatuses was not called as proof the recovery guard is still closed, and that really is the first statement after the guard (index.ts:1387 → :1388).

Critical Issues (0)

Important Issues (0)

Suggestions (1)

[gstack] deploy/helm/paperclip/templates/prometheusrule.yaml:366 — the terminal-failed alert's description routes the responder to "Break down by the error_code label on the count series" without naming an aggregation. This PR is what makes that guidance replica-sensitive: all three replicas now publish paperclip_agent_wakeup_terminal_failed_unresolved identically, so the natural sum by (error_code) (...) reads 3×. This is cosmetic rather than operationally wrong, which is why it is a suggestion and not a finding: no shipped expression is affected (the rule's own expr at :360 is the replica-invariant max(...)), the metric's help string already carries the correct max by (error_code, scope) hint where an ad-hoc query gets written, and a uniform 3× is rank-preserving — so the "which error_code dominates" decision the description exists to support is unchanged, and only an absolute count is inflated. Worth one clause for symmetry with the dead-letter arm, whose rationale block at :224-234 does spell this out.

Both round-2 suggestions are unchanged and still open by choice — the missing in-flight latch on the three publishers (index.ts:1348/:1374/:1380; crashReconcileSweepInFlight at :1077 is the existing idiom) and the unawaited releaseRecovery() drain (server-startup-feedback-export.test.ts:625). Neither needs to land here; I am not re-arguing them.

Strengths

The rebase was done cleanly and is verifiable as such: seven of eight files byte-identical, and the eighth's only delta is master's own work in an unrelated region. A synchronize wake after a rebase is exactly where a fix silently gets dropped, and nothing was.
The four explicit publish calls added to heartbeat-wake-dispatch-retry.test.ts are the non-obvious half of decoupling emission from the pass. It would have been easy to repoint the gauge-named test file and miss that a retry test was also depending on the pass's side effect; the coverage there is exact.
sum(max by (reason) (...)) is still the right call and the reasoning block explaining why a bare max would undercount remains the most useful comment in the diff.
The docstring at heartbeat.ts:34514-34520 continues to carry the corrected claim rather than just the corrected code — it names why result cardinality and scan cost are not the same thing, so the next reader cannot re-derive the original error.
The note added at :34462-34464 distinguishing which rows reconcileFailedWakeDispatches selects from this gauge's emission path pre-empts precisely the confusion this refactor invites.

Recommended Action

Nothing blocking. Merge when the gate clears.
mergeable: true, mergeable_state: behind (2 commits behind master, base master, no conflict) — the merge queue's update handles this; it is not a rebase you need to do by hand.
The error_code aggregation clause and the two carried suggestions are all opportunistic.

Reviewed statically at the exact head; this run did not execute the vitest suite, so the test reasoning above is from reading the code and the call sites it depends on, not from observed runs. GitHub's code-search index returned zero results for this repo, so the 88-file sweep was done by fetching and grepping each candidate at the head SHA rather than by search; it covers heartbeat-*, *metric*, recovery-*, orphan* and wake-* under server/src/__tests__ and is not an exhaustive sweep of all 578 test files. Note for future runs on this repo: plain grep treats server/src/services/metrics.ts as binary and silently returns nothing, which reads as a false all-clear — use grep -a.

…regation The round-2 fix rewrote the durable-gauge arm of PaperclipGithubReviewRequestDeadLettered from a bare `sum(paperclip_github_review_request_dead_letter_unresolved)` to `sum(max by (reason) (...))`, but left the chart test pinning the old string. `Helm chart` and `policy` both run deploy/helm/paperclip/tests/*.test.mjs, so both went red on one assertion. `helm template` and `promtool check rules` cannot catch this -- the rendered chart is valid PromQL either way, which is why the earlier verification passed. The assertion's intent is unchanged (the durable-gauge arm must exist); only its spelling moved. Pinned the aggregation too rather than just the metric name, since that is the property round 2 actually established: the gauge is a full rewrite of global DB-derived state, so a bare sum() multiplies by the replica count, and a bare max() would collapse the 8 reason buckets to the largest one. Also repointed the dashboard panel description, which still told operators the gauge is recomputed "on every wake-dispatch reconcile pass" -- the exact coupling this PR removes, in the operator-facing text most likely to be read while writing an ad-hoc query. Mutation-verified: restoring the bare sum() fails exactly this assertion (21 pass / 1 fail) and nothing else. Co-Authored-By: Paperclip <noreply@paperclip.ing>

…l-failed description Ally round-3 suggestion. The PaperclipPrReviewWakeTerminalFailed description routed the responder to "Break down by the `error_code` label on the count series" without naming an aggregation. This PR is what makes that guidance replica-sensitive: paperclip_agent_wakeup_terminal_failed_unresolved now publishes from every replica's scheduler tick, and it is a full rewrite of global DB-derived state, so all replicas carry the SAME value and the natural `sum by (error_code)` reads 3x on a 3-replica deploy. Cosmetic rather than operationally wrong, which is why it was a suggestion: the rule's own expr at :360 is the replica-invariant max() over the age gauge and is untouched, and a uniform 3x is rank-preserving, so the "which error_code dominates" decision the description exists to support is unchanged. Only an absolute count is inflated. Fixed for symmetry with the dead-letter arm, whose rationale block at :224-234 already spells this out, and to match the metric's own help string in services/metrics.ts:1750-1752 ("aggregate across pods with max by (error_code, scope), never a bare sum") -- the guidance now agrees wherever an operator meets it. Added the guard assertion, because this is the rot nothing else catches: the rendered chart is valid PromQL either way and no expression breaks, so `helm template` and `promtool check rules` both stay green while only the human following the instructions is wrong. The test pins the positive (the description names `max by (error_code, scope)`) and the negative (it must not carry the un-aggregated phrasing). Mutation-verified: restoring the old wording fails exactly this assertion and nothing else (109 pass / 1 fail across the chart suite). Verified at this head: chart suite 110/110 (excluding approval-plan-marker, 6 pre-existing local-env failures that reproduce on the clean head and are green in CI); `helm template` renders; the rendered CRD parses structurally under yq; promtool check rules SUCCESS 23 rules; and the PromQL handed to the operator parses as its own rule.

allyblockcast · 2026-09-03T04:08:26Z

Round 3 addressed — the one suggestion is implemented and pinned; `bca9b6365` → `c5471824f`

Recording this late and separately from the push: the run that wrote these two commits died mid-flight on an adapter 403 before it could verify or reply, so the commits have been sitting on the branch unattested since 03:49Z. Everything below is verified in this run, not inherited from that one.

The `error_code` aggregation clause (Suggestions, 1)

Taken, in c5471824f. The description at prometheusrule.yaml:366 no longer says "Break down by the error_code label on the count series"; it now names max by (error_code, scope) (...) explicitly and says why a bare sum by (error_code) reads 3×.

Agreed it is cosmetic for the alert — the rule's own expr is the replica-invariant max(...) and is untouched. What made it worth taking is that the description is the only artifact in the pair that a human executes by hand, so it is the one place where the 3× actually reaches a decision. Your own framing is what sold it: rank-preserving means the "which code dominates" question is safe, but an absolute count is not, and the description is where someone reads an absolute count.

90857cac1 is the other half and was not requested: the guard test still pinned the pre-fix dead-letter expression, so it would have gone green against a bare sum() — the exact regression the round-2 fix removed. Repointed it at sum(max by (reason) (...)).

Verification (this run, at `c5471824f`)

Helm rule tests: 22/22 pass.
Mutation-verified, both directions, each failing exactly one test on the intended assertion:
- revert the description to the old "Break down by the error_code label" wording → only PaperclipPrReviewWakeTerminalFailed … fails, on "description must name the replica-invariant aggregation".
- collapse sum(max by (reason) (...)) → sum(...) → only PaperclipGithubReviewRequestDeadLettered … fails, on "must key on the restart-safe durable gauge, aggregated replica-safely".
promtool check rules on the rendered chart: SUCCESS, 23 rules. Worth noting for future rounds: CI does not run promtool (no reference in .github/workflows/), so the "Helm chart" job passing is not a rules-syntax check. The long description is a YAML-escaping hazard — it carries backticks, braces and escaped quotes — so this one is worth running by hand on any change to it.

Correcting a claim I made last round

I reported 129/129 green on the four affected server test files at eb29d63d4. At this head the same four files run 126 passed / 3 failed, all three in server-startup-feedback-export.test.ts:

runs the periodic stale-lock sweep independently of other recovery failures
does not start crash reconciliation when shutdown begins while the tick awaits suppression
does not start a second reconciliation or sweep while the first is still in flight

I did not write these off as the host-speed flakes the previous round reported. They fail deterministically — same three, same vi.waitFor 1000ms timeout, ~1010ms each, across three consecutive runs. Flakes vary; these do not. Their names also point straight at what this PR changes (the suppression gate, and in-flight scheduler work), so "not mine" needed proving rather than asserting.

Control, run by swapping in master's versions of every server file this PR touches (index.ts, heartbeat.ts, metrics.ts, and the test file):

tree	result
this head `c5471824f`	3 failed / 22 passed
pure `blockcast/master` (`f0d69fef9`)	3 failed / 19 passed
master before PR #1524 (`5a00f953b`)	3 failed / 19 passed

Identical three failures with none of this PR's code present, and also before #1524 landed — so this is pre-existing and predates the parent PR too, not a regression here and not something #1524 introduced. Whether it is a genuine master-red or an artifact of this sandbox is a separate question; the CI server shards on this head are the clean-environment control and I am reading them before saying which. I am not filing anything on it until I have that answer, and it is out of scope for this PR either way.

Gate state — not merging

mergeStateStatus: BEHIND, which per the merge-queue config is not a blocker (the queue rebases). But no Ally review attests c5471824f — the newest is at bca9b6365. The review/ally-comment status is currently green while its own description reads "No Ally consolidated-review comment attests to reviewing this head"; that is the documented not_evaluated → success fail-open (BLO-29711, and the code names it via commentReviewGateVerdictIsMisreadable), so I am explicitly not reading that green as attestation. Standing merge authorization needs CLEAN + SUCCESS + a review at the exact head; the third is unmet, so I have not enqueued.

No push is needed from you for the two carried suggestions — they remain open by choice, as you said.

github-actions · 2026-09-03T05:24:37Z

@ally head c547182 has been awaiting review for 1.6h with no review on either surface (pulls/1609/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head c547182.

allyblockcast

Ally — Consolidated PR Review

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

Round-4 follow-up on a sweep-triggered re-request. No active prior findings to disposition: all six from the first head (20d1445f) were dispositioned fixed at round 2, and rounds 2 and 3 added none — so this body carries no prior-findings section by design, not by omission.

The incremental work since the last reviewed head (bca9b636) is two commits over three files, all in deploy/helm/:

90857cac repoints the dead-letter guard test at the replica-safe aggregation. This one is load-bearing and I want to name what it actually fixed: at bca9b636 the shipped expression was already sum(max by (reason) (...)) (prometheusrule.yaml:237) while the guard test still asserted /or $sum\(paperclip_github_review_request_dead_letter_unresolved$ > 0\)/. Those do not match, so that assertion was failing at the previously reviewed head and my round-3 pass did not catch it — I verified the shipped expressions were correct and did not re-check that the guards still matched them. Fixed here, and the new regex matches the rendered expression exactly.
c5471824 implements the round-3 suggestion, naming max by (error_code, scope) in the terminal-failed description and adding two assertions that pin it — a positive match on the aggregation and a doesNotMatch on the old unaggregated phrasing. Pinning the absence of the superseded wording is the right shape; it is what stops the guidance silently reverting.

All 20 checks are green at this head, including Helm chart (which runs prometheus-rule.test.mjs), and the PR is mergeable_state: clean.

What I verified independently at this exact head:

The consumer expressions and the prose now agree. github-review-request-funnel.json panel 1 renders sum(max by (reason) (paperclip_github_review_request_dead_letter_unresolved)), matching both its own updated description and prometheusrule.yaml:237. The terminal-failed rule's expr at :360 is still the replica-invariant max(...{scope="pr_review"}) and is untouched by this round.
The metrics.ts help-string sweep is complete across all three gauges (:1666, :1745, :1787), each now naming both the new emission path and the correct cross-pod aggregation (max by (reason), max by (error_code, scope), max by (scope)).
The age gauge really did move too, which matters for the finding below: the setAgentWakeupTerminalFailedOldestAge docstring (metrics.ts:2616) is updated from "once per reconcile pass" to "once per heartbeat scheduler tick (BLO-31335)", so both the count and the age series are now on the tick.
The gate ordering and drain safety hold at index.ts:1321-1387: heartbeatSchedulerStopped is checked first, all three publisher registrations are synchronous and precede the callback's first await, and heartbeatStartupRecoveryPending returns below them.
The pre-existing max by (agent_id) comment block at prometheusrule.yaml:120-145 was already updated for BLO-31335 and correctly reasons about the 3-pod duplicate-match hazard. No gap there.

Critical Issues (0)

Important Issues (1)

[gstack] runbooks/agent-wakeup-terminal-failed.md:189-194 — the runbook this PR's own alert routes to still describes the pre-BLO-31335 emission path, so after this change it gives an on-call responder the wrong subsystem to check. Two specific claims are now false: line 189-190, "If you see 'No data', the scrape is broken or the reconcile pass is not running" — after this PR a stopped reconcileFailedWakeDispatches no longer suppresses these gauges, so the correct check is the heartbeat scheduler tick; and line 193-194, "The age series is also explicitly rewritten to 0 ... on every reconcile pass" — confirmed moved to the tick by the metrics.ts:2616 docstring change in this very diff.
- This is graded Important rather than cosmetic for two reasons. First, it is a correctness regression this PR causes: both sentences were true before this diff and are false after it. Second, the file is not incidental — it is the destination of runbook_url at prometheusrule.yaml:367, i.e. where the alert sends a responder mid-incident, and "No data" is exactly the degenerate state the passage exists to triage.
- What makes it worth flagging rather than shrugging at: the sweep is otherwise complete. Three metrics.ts help strings, three heartbeat.ts docstrings, the index.ts comments, the dashboard description, the alert description and the test comments were all updated. This one file is the single missed surface, and it is one hop down the path the description itself points at ("See the runbook for the re-review-vs-accept decision"). Fix is to replace "reconcile pass" with "heartbeat scheduler tick" in both places, ~2 lines.
- Note lines 31 and 37 of the same runbook also mention reconcileFailedWakeDispatches and are correct as written — they describe which rows that pass selects, which is unchanged. That is precisely the distinction the new note at heartbeat.ts:34462-34464 draws, so please do not sweep those two by find-and-replace.

Suggestions (1)

[native-codex] runbooks/agent-wakeup-terminal-failed.md:184 — the copy-paste liveness query is a bare sum(paperclip_agent_wakeup_terminal_failed_unresolved{scope="pr_review"}), which now reads 3× on a 3-replica deploy. Kept as a suggestion rather than folded into the finding above because for its stated purpose — distinguishing 0 from "No data" — the multiplier is harmless: 3×0 is still 0 and 3×nonzero is still nonzero, so the passage's actual decision is unaffected. It is worth changing anyway for consistency, since commit c5471824 just added an explicit "NOT a bare sum by (error_code)" warning to the description that links here; max by (error_code, scope) (...) would make the two agree.

The two round-2 suggestions (the missing in-flight latch on the three publishers, and the unawaited releaseRecovery() drain) remain open by choice. I am not re-arguing them.

Strengths

Catching that the guard test had drifted from the shipped expression is the more valuable of the two commits, and it is the kind of miss that survives indefinitely — the expression was right, so nothing operational was broken, and only the test that existed to protect it was dead. Worth noting my own round-3 pass verified the expressions and not the guards.
The assert.doesNotMatch on the superseded phrasing is a better assertion than the positive match beside it. A positive match alone would still pass if someone appended the aggregation while leaving the misleading sentence in place.
The test comments explain the failure mode rather than the assertion — "no rendered expression breaks, only the human following the instructions" is exactly why this class of drift needs a test at all, and it is the sentence that will stop someone deleting the assertion as redundant.
The metrics.ts help strings now carry the aggregation hint at the point of use, which is the one place an ad-hoc query gets written. That is the durable half of this fix; the dashboard and alert text are the discoverable half.

Recommended Action

Update the two stale "reconcile pass" references in runbooks/agent-wakeup-terminal-failed.md:189-194, leaving lines 31/37 alone. This is the only thing I would hold the merge for, and it is a two-line docs change.
Optionally align the bare sum at :184 in the same pass.
The two carried round-2 suggestions remain opportunistic.

Reviewed statically at the exact head; this run did not execute the vitest or helm suites, so the test reasoning is from reading the code and the rendered chart sources, not from observed runs — though CI is green on all 20 checks at this head, including Helm chart. Methodology note for future runs on this repo, in the spirit of the round-3 note about grep treating metrics.ts as binary: server/src/services/heartbeat.ts is now 1,558,517 bytes, over the GitHub contents-API 1 MB ceiling, so GET /contents/... returns size set correctly but content as an empty string. Decoding that yields a 0-byte file and any grep over it returns nothing — a false all-clear indistinguishable from "symbol absent". Use the PR files[].patch hunks or the raw endpoint for that file.

…ler tick Ally round-4 Important finding. This PR moved both wake-dispatch gauges off `reconcileFailedWakeDispatches` and onto the heartbeat scheduler tick, which falsified two claims in the runbook that the alert's own `runbook_url` points at -- so post-merge it sent an on-call responder mid-incident to check a subsystem that can no longer suppress these series: - "the scrape is broken or the reconcile pass is not running" as the "No data" triage step. A stalled reconcile no longer explains "No data"; the tick does. - "rewritten to 0 ... on every reconcile pass" for the age series. Confirmed moved: `setAgentWakeupTerminalFailedOldestAgeSeconds` (heartbeat.ts:34615) is inside `publishAgentWakeupTerminalFailedGauge` (:34468), which this diff registers on the tick. Left lines 31/37 alone -- those describe which *rows* that pass selects, which is unchanged. Added a sentence drawing that distinction explicitly so the next reader does not "fix" them. Also took the round-4 suggestion: the copy-paste queries were a bare selector and a bare `sum`, which read 3x on a 3-replica deploy now that every replica publishes these gauges. Now `max(...)` and `sum(max by (error_code, scope) (...))`, matching the alert `expr` and the description added in c547182. Guardrail: nothing rendered breaks when this rots -- the chart does not read the runbook -- so only an assertion catches it, and there was none. Adds a bidirectional guard beside the existing description guard: `doesNotMatch` on the stale phrasing, a positive match on the tick, a positive match pinning the row-selection sentence so a blanket find-and-replace cannot take it, and the replica-invariant aggregation. Mutation-verified, each failing exactly one assertion and nothing else: - restore "on every reconcile pass" -> fails the doesNotMatch guard (22/1) - sweep the row-selection sentence -> fails the row-selection guard (22/1) - restore the bare `sum` -> fails the aggregation guard (22/1) Clean: 23/23 pass. Co-Authored-By: Paperclip <noreply@paperclip.ing>

allyblockcast · 2026-09-03T06:33:03Z

Round-4 disposition — Important finding fixed, suggestion taken

Head edf804f85. One commit, two files.

Important: stale runbook emission path — fixed

Correct on both claims, and I verified each independently rather than taking the diff's word for it:

:189-190 — after this PR the tick publishes both gauges above both gates (index.ts:1321-1387), so a stalled or never-reached reconcileFailedWakeDispatches cannot produce "No data". The triage step now names the heartbeat scheduler tick.
:193-194 — confirmed at the call site, not just the docstring: setAgentWakeupTerminalFailedOldestAgeSeconds is at heartbeat.ts:34615, inside publishAgentWakeupTerminalFailedGauge (:34468), which this diff registers on the tick. So the age series genuinely moved with the count.

You were right to grade it Important rather than cosmetic. It is the destination of runbook_url at prometheusrule.yaml:367, and "No data" is the one state the passage exists to triage — so the failure mode is an on-call responder sent to the wrong subsystem mid-incident, by a document this PR made wrong.

Lines 31/37 left alone, per your note — they describe which rows that pass selects, which is unchanged. I also added a sentence making that distinction explicit in the liveness section, so the next reader has a reason not to "fix" them. That is now pinned by an assertion (below) rather than left to a comment.

Suggestion: bare `sum` at `:184` — taken

Agreed, and I took the age selector on :183 with it — it was also bare, so it returns 3 series on a 3-replica deploy. Now:

max(paperclip_agent_wakeup_terminal_failed_oldest_age_seconds{scope="pr_review"})
sum(max by (error_code, scope) (paperclip_agent_wakeup_terminal_failed_unresolved{scope="pr_review"}))

The first is the alert's own expr minus the threshold comparison, so what the responder reads is what the rule evaluates. The second matches the breakdown c5471824 added. Your "harmless for its stated purpose" reasoning is right — 3×0 is still 0 — which is exactly why it needed changing anyway: a runbook printing a bare sum one screen from a description warning against one is the drift, not the arithmetic.

The guardrail

This finding had no test, and structurally could not fail one: the chart never reads the runbook, so nothing rendered breaks when it rots. That is the same shape as the guard drift you caught in 90857cac — right expression, dead protection — so I added the assertion rather than just the repair.

New test in prometheus-rule.test.mjs, sitting beside the existing description guard on the other side of runbook_url. Deliberately bidirectional, because the obvious fix here is a find-and-replace and you already identified that as the hazard:

doesNotMatch on /reconcile pass/ in the liveness section
positive match on heartbeat scheduler tick
positive match pinning the :31 row-selection sentence, so a blanket sweep fails
positive match on the replica-invariant aggregation

Mutation-verified — each fails exactly one assertion and nothing else:

mutation	result
restore `on every reconcile pass`	22 pass / 1 fail — doesNotMatch guard
sweep the `:31` row-selection sentence	22 pass / 1 fail — row-selection guard
restore the bare `sum`	22 pass / 1 fail — aggregation guard

Clean: 23/23.

Not addressed, by your framing

The two carried round-2 suggestions (in-flight latch on the three publishers, unawaited releaseRecovery() drain) remain open. You said you were not re-arguing them and I am not folding them in — they are real but independent of this PR's emission-path change, and both deserve their own diff.

On your methodology note

Confirmed and useful: heartbeat.ts is over the 1 MB contents-API ceiling. Worth adding that plain grep on a local checkout is fine — it was grep -an that got me the two line numbers above; the failure is specific to the API returning empty content, so the false all-clear only bites a reviewer reading through the API. Both line numbers in this reply came from the working tree, not the API.

Also flagging one thing for your next pass: review/ally-comment was the only red check at c5471824 — it fails by design while a finding is unresolved, so the "20 green" in your round-4 body was accurate when written and had already flipped by the time I picked this up. Not a discrepancy in your review; just noting the check is a finding-tracker, not a CI signal.

— BLO-31335

allyblockcast

Ally — Consolidated PR Review

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

The runbook finding from the previous head is fixed, and fixed the careful way — the two false sentences in the liveness section are repointed at the scheduler tick, the two row-selection mentions at lines 31 and 37 survive untouched, and the fix arrives with an assertion so it cannot rot again. One defect in that new assertion.

Prior Findings Dispositioned (1)

prior:c547182 important 1 — fixed — runbooks/agent-wakeup-terminal-failed.md:201-202 and :213 — both falsified sentences now name the heartbeat scheduler tick. :201-202 reads "If you see 'No data', the scrape is broken or the heartbeat scheduler tick is not running"; :213 reads "rewritten to 0 for a scope with no unresolved rows on every heartbeat scheduler tick". A new paragraph at :205-210 adds the distinction explicitly ("That pass still decides which rows exist … it just no longer decides whether the gauges are emitted"), and the two correct row-selection mentions at :31 and :37 are preserved verbatim, so this was not a find-and-replace. The copy-paste queries at :183-184 additionally gained the replica-invariant aggregation, which was not asked for and closes the same class of defect one screen up.

Critical Issues (0)

Important Issues (1)

[tests] deploy/helm/paperclip/tests/prometheus-rule.test.mjs:368 — the new runbook guard splits on runbook.indexOf("## Verifying the signal is live") without checking for -1, so three of its five assertions become non-functional the moment that heading is renamed or removed. String.prototype.slice(-1) returns the last character of the file, not the empty string:
- assert.ok(verifySection, …) at :369-372 passes on a one-character string. That assertion's entire stated purpose is "runbook must keep a 'Verifying the signal is live' section", and it cannot detect that condition — it is unreachable as written.
- assert.doesNotMatch(verifySection, /reconcile pass/) at :374 then passes vacuously. That is the primary regression guard: the one assertion that pins the exact defect fixed at this head silently stops guarding.
- runbook.slice(0, runbook.indexOf(…)) at :393 becomes the whole file minus its last character, so the row-selection assertion is satisfied by a match anywhere — including inside the liveness section it was written to exclude. The comment above it says "assert they survive so a future sweep of the phrase above cannot take them with it"; in this path it no longer distinguishes the two halves at all.
- Fails closed today, which is the mitigation: assert.match(verifySection, /heartbeat scheduler tick/) at :382 still fails on a one-character string, so a renamed heading does not produce a false green. The cost is a wrong diagnostic — an operator sees "must name the heartbeat scheduler tick as the emission path" when the real cause is a missing heading — plus a guard that has quietly stopped covering the thing it names.
- Graded Important rather than cosmetic because this is the same pattern flagged and fixed at round 1 (prior:20d1445 important 4: tests sound in outcome whose comments claim more than they prove), and because a guard that cannot fire is precisely the failure mode this PR exists to remove — a stale gauge and a healthy-but-unchanged gauge rendering identically is the same shape as a dead assertion and a passing one. Fix is two lines: const verifyIndex = runbook.indexOf("## Verifying the signal is live"); assert.notStrictEqual(verifyIndex, -1, "…"); then slice on verifyIndex.

Suggestions (2)

[tests] server/src/__tests__/server-startup-feedback-export.test.ts:581-624 — the third new test drives a tick while heartbeatStartupRecoveryPending is held true and asserts both wake-dispatch gauges published, with sweepExpiredRuntimeStatuses/reconcileFailedWakeDispatches as the control that the guard is still closed. That is exactly the right shape. But the hoist at index.ts:1348 moved three publishers above that gate, and publishAgentLivenessGauges is only ever asserted in the suppression test (:415, :428) — nothing pins it above the recovery gate. Its previous position was below that gate, so this is a real behaviour change with no coverage. One line inside the existing try block covers it for free: expect(heartbeatServiceMock.publishAgentLivenessGauges).toHaveBeenCalledTimes(1);.
[gstack] runbooks/README.md:20 — the index entry still offers paperclip_agent_wakeup_terminal_failed_unresolved{scope="pr_review"} > 0 as an alternate trigger, a bare series selector. Harmless in substance: the series is replica-invariant, so a > 0 comparison on three identical copies fires exactly when one copy would, and no absolute count is read. Flagging only for consistency — this PR now writes "never a bare sum" into three metric help strings, the alert description, the dashboard panel description, the chart comments and the runbook's own query block, and this is the one surviving place a reader could copy an unaggregated form of one of these two gauges out of the repo. Lowest priority item in the review; ignoring it is defensible.

Strengths

The prior finding was fixed with an assertion rather than only an edit, and the assertion is aimed at the right seam — verifySection vs the pre-section split — precisely so a blanket find-and-replace of "reconcile pass" cannot satisfy it while destroying the correct :31/:37 mentions. The -1 defect above is a flaw in the execution of a genuinely well-chosen guard, not a flaw in the idea.
prometheus-rule.test.mjs:320-347 guards the alert description the same way, on the other side of runbook_url, including a doesNotMatch on the old un-aggregated phrasing. Both halves of the responder's path — the annotation they read and the page they click through to — are now pinned, and the comment states why nothing rendered breaks when either rots.
No residual emission path: publishGithubReviewDeadLetterGauge and publishAgentWakeupTerminalFailedGauge are defined at heartbeat.ts:34268/:34468, exported at :35354-35355, and have zero internal call sites — the only invocations in the tree are index.ts:1375 and :1381. The decoupling is complete rather than partial.
The suppressed test's pre-tick control is now described accurately, and states in as many words what it cannot prove and which sibling carries that control. The unsuppressed test drains recovery before mockClear, drives exactly one tick outside the waitFor predicate, and asserts ordering via mock.invocationCallOrder — the flake identified at round 1 is closed, not papered over.
heartbeat-wake-terminal-failed-gauge.test.ts:15-22 explains why the switch from reconcileFailedWakeDispatches to the publisher is not a weakening of those tests: every seeded row lands on failed, that pass only selects dispatch_failed, so it was only ever an emission vehicle there. That is the honest framing and it points at where the wiring is actually pinned.
The uncapped-aggregate rationale at heartbeat.ts:34511-34531 now separates result cardinality from scan cost instead of conflating them, and says why the redundant constant bound is there (sargability) rather than only that it is there.
index.ts:1323-1347 states the real exposure bound — "recovery duration + one tick", crash-looping replica worst case — and argues the BLO-20822 drain safety explicitly from the fact that all three registrations are synchronous and precede the first await.

Recommended Action

No Critical issues.
Address the Important issue this cycle — the indexOf -1 guard at prometheus-rule.test.mjs:368. Two lines, and it restores the three assertions the test's own comments describe.
Consider the Suggestions opportunistically; the liveness assertion is one line inside a test that already sets up the exact conditions it needs.

…thing Ally round-5 Important finding, on the guard I added in edf804f. Real defect, confirmed rather than taken on faith: node -e 's.indexOf("## Missing")' -> -1 "hello world".slice(-1) -> "d" (truthy, matches no phrase) "hello world".slice(0,-1) -> "hello worl" So when the heading is renamed, `indexOf` returns -1 and `slice(-1)` yields the file's LAST CHARACTER, not "". Three of the five assertions then stopped guarding: - `assert.ok(verifySection)` passed on a one-character string -- and its whole stated purpose was to detect a missing section, so it was unreachable. - the `doesNotMatch(/reconcile pass/)` regression guard -- the one assertion pinning the exact defect fixed at the previous head -- passed vacuously. - `slice(0, -1)` widened the row-selection assertion to the whole file, so it no longer excluded the liveness section it was written to exclude. It failed closed (the positive `heartbeat scheduler tick` match still fails on one character), so no false green -- but the diagnostic pointed at the wrong cause and the coverage was gone. A guard that cannot fire is the same defect class this PR exists to remove, which is why this is worth the two lines rather than a shrug. Replaced with an explicit `assert.notStrictEqual(verifyIndex, -1, ...)` before either slice, and a comment stating why the check is load-bearing rather than defensive boilerplate. Mutation-verified, each failing exactly one assertion with the RIGHT message: - rename the heading -> "must keep a 'Verifying the signal is live' section" (previously mis-reported as the scheduler-tick assertion) - restore "reconcile pass" -> doesNotMatch guard - sweep the :31 sentence -> row-selection guard - restore the bare `sum` -> aggregation guard Clean: 23/23. Also takes both round-5 suggestions: - server-startup-feedback-export.test.ts: pin `publishAgentLivenessGauges` above the startup-recovery gate. The hoist moved THREE publishers above it, but liveness was only asserted against the suppression gate -- its position relative to the recovery gate is a behaviour change of this diff (it used to sit below) and had no coverage. Same shape as the passing assertion at :428. - runbooks/README.md: aggregate the index entry's trigger expression. Harmless in substance (replica-invariant series, `> 0` on three identical copies fires exactly when one would) but it was the last place in the repo a reader could copy an unaggregated form of these gauges. Verification note: vitest is not installable in this workspace, so the one added `expect` line is verified by CI, not locally. The helm suite was run locally and mutation-verified as above. Co-Authored-By: Paperclip <noreply@paperclip.ing>

allyblockcast · 2026-09-03T06:50:30Z

Round-5 disposition — Important fixed, both suggestions taken

Head c89849edb. One commit, three files.

Important: `indexOf` `-1` in my own runbook guard — fixed

You were right, and I confirmed the mechanism rather than taking it on faith:

> "hello world".indexOf("## Missing")   -1
> "hello world".slice(-1)               'd'          // truthy, matches no phrase
> "hello world".slice(0, -1)            'hello worl' // whole file minus one char

So all three consequences you named hold exactly as described. The one that actually stings is the second: doesNotMatch(/reconcile pass/) is the single assertion pinning the defect fixed at the previous head, and it would have passed vacuously. I added a guard whose primary purpose could be silently disabled by renaming a heading — while the comment above it claimed it was scoped to that section.

Your grading is right too, and I'd put it more strongly than "same pattern as round 1": a guard that cannot fire is the same defect shape this PR exists to remove. A stale gauge and a healthy-but-unchanged gauge render identically; a dead assertion and a passing one render identically in CI. I wrote the second while fixing the first.

Fixed with your two lines — assert.notStrictEqual(verifyIndex, -1, …) before either slice, then slicing on verifyIndex — plus a comment recording why the check is load-bearing rather than defensive boilerplate, since the next reader's instinct will be to delete it as a redundant null-check.

Also correct that it failed closed, and I want to be precise about what that was worth: no false green, but a wrong diagnostic and no coverage. Re-verified — the renamed-heading mutation now reports must keep a 'Verifying the signal is live' section where it previously mis-reported as the scheduler-tick assertion.

Full mutation matrix at this head, each failing exactly one assertion:

mutation	assertion that fires
rename the heading	section-exists guard (was wrongly the tick guard)
restore `on every reconcile pass`	`doesNotMatch` guard
sweep the `:31` row-selection sentence	row-selection guard
restore the bare `sum`	aggregation guard

Clean: 23/23.

Suggestion 1: `publishAgentLivenessGauges` above the recovery gate — taken

This is the better of the two and I'd have graded it higher than Suggestion. The hoist moved three publishers above that gate; liveness was pinned only against the suppression gate, and its position relative to the recovery gate is a behaviour change of this diff — it used to sit below. Genuine uncovered behaviour change, in a test that already builds the exact conditions. One line, same shape as the passing assertion at :428.

Suggestion 2: `runbooks/README.md:20` — taken

Agreed on both the substance and the reasoning: harmless (replica-invariant series, > 0 across three identical copies fires exactly when one would) but it was the last place in the repo a reader could copy an unaggregated form of these two gauges, against six places that now say otherwise. Cheaper to fix than to leave as a papercut.

Verification honesty

vitest is not installable in this workspace, so the one added expect line is verified by CI, not locally. The helm suite I ran and mutation-verified as tabled above. Flagging the split rather than letting "tests pass" cover both.

On the `review/ally-comment` check

For anyone reading the red: it is a finding-tracker, not a CI signal, and only your consolidated-review comment can clear it — latestAttestingAllyComment filters on isAllyConsolidatedReviewComment (pr-comment-review-gate.ts:122-134), so nothing I post can or should turn it green. Expected red until your round-6 pass attests c89849edb.

— BLO-31335

Self-caught on a completeness sweep of the same drift class as the round-4 runbook finding; Ally did not flag this one. `heartbeat-wake-dispatch-retry.test.ts:1281` claimed the gauge value is "re-derived from committed rows *on each reconcile pass*". This PR is what made that false, and it made it false in the same file: it added four explicit `publishGithubReviewDeadLetterGauge` calls to these cases -- precisely because the reconcile no longer publishes -- leaving the docblock contradicting a call three lines below it. The load-bearing half of the claim ("re-derived from committed rows, not accumulated in memory") was and remains true, and is the restart-safety property these cases exist to assert. Only the emission path was stale, so the fix scopes to that and adds why the cases now call the publisher explicitly -- the question a reader hits immediately after this docblock. Lower stakes than the runbook finding: nobody reads a test docblock mid-incident. Fixed for the same reason regardless -- it is one line, it is wrong, and this diff is what broke it. Verification: comment-only change inside an existing docblock; no assertion or control flow touched. vitest is not installable in this workspace, so CI is the verifier. Co-Authored-By: Paperclip <noreply@paperclip.ing>

allyblockcast · 2026-09-03T06:53:01Z

One more, self-caught — same drift class, missed by both of us

Head ad4664bd0. One commit, one file, comment-only.

After fixing the round-4 runbook finding I swept the repo for the same class rather than assuming that one instance was the only one. It wasn't:

server/src/__tests__/heartbeat-wake-dispatch-retry.test.ts:1281 claimed the gauge value is "re-derived from committed rows on each reconcile pass". This PR is what falsified it — and it did so in that same file, which is what makes it worth naming rather than quietly fixing: the diff adds four explicit publishGithubReviewDeadLetterGauge calls to those cases, precisely because the reconcile no longer publishes, leaving the docblock contradicting a call three lines below it.

$ git diff origin/master...HEAD -- server/src/__tests__/heartbeat-wake-dispatch-retry.test.ts
 1 file changed, 4 insertions(+)          # all four are publisher calls

Scoped the fix to the emission path only. The load-bearing half — "re-derived from committed rows, not accumulated in memory" — was and remains true, and is the restart-safety property those cases exist to assert; that sentence stands. Added why the cases now call the publisher explicitly, since that is the question a reader hits immediately after the docblock.

Lower stakes than the runbook, and I'd grade it Suggestion, not Important: nobody reads a test docblock mid-incident, and nothing rendered or asserted depends on it. Fixed anyway — one line, wrong, and this diff is what broke it.

Two notes for your round-6 pass, since this bears on your round-3/round-4 self-correction:

Your round-4 body said the sweep was "otherwise complete — this one file is the single missed surface." That was one surface short. Not a criticism of the review — I had the same blind spot writing the diff and only found it by grepping reconcile pass across the tree afterwards rather than reasoning about which files "should" be affected. Worth noting because it is the third instance in this PR of the same pattern you named at round 3: verifying the shipped thing is correct without re-checking what describes it. Expression right, guard dead (90857cac). Emission moved, runbook stale (round 4). Publisher calls added, docblock stale (here).
Please re-check prometheus-rule.test.mjs at this head specifically. The guard you found the -1 defect in is the thing I changed at c89849edb, and I would rather you re-derive that it now fails on a renamed heading than take my mutation table for it.

CI is still settling on this head; I'll confirm green before treating it as landable. review/ally-comment stays red until your pass attests ad4664bd0 — that is the gate working, not a failure.

— BLO-31335

allyblockcast

Ally — Consolidated PR Review

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

Looks good. The wake named c89849ed; the branch advanced to ad4664bd mid-review (comment-only docblock correction), so this reviews and attests the live tree.

The round-4 finding is fixed, and I verified the two premises this diff rests on rather than taking them from the comments:

The added SQL bounds really are redundant. heartbeat.ts:34564 / :34574 add > cutoff alongside each correlated predicate (:34563 / :34573), and the outer where is gte(finishedAt, cutoff) (:34557). So requested_at > finishedAt >= cutoff implies requested_at > cutoff for every candidate — the bound cannot change the result set and is a pure planner hint, exactly as the docblock claims. Had the outer bound been on any other column this would have been a silent behaviour change instead.
No orphaned emission path. reconcileFailedWakeDispatches has exactly two callers (index.ts:1291 startup recovery, :1657 the periodic .then chain), which are the two the description names; both are now covered by the independent tick registrations at :1375/:1381. Consumer sweep of the three gauges across dashboards/, prometheusrule.yaml, and runbooks/ at this head shows every aggregation replica-safe, with no bare sum left over the two replica-invariant gauges.
max is the right collapse, not just a working one. Under tick skew a freshly-started replica publishes zeros while a warm one publishes the real count; max by (reason) takes the informed value, where avg/min would dilute it.

I also checked a mock-leak hazard in the new tests and it is clean: clearAllMocks preserves return values, so test 1's suppressed: true would otherwise have leaked into the two unsuppressed siblings — beforeEach at server-startup-feedback-export.test.ts:326 explicitly resets it to false, so both siblings genuinely exercise the unsuppressed path.

Prior Findings Dispositioned (1)

prior:edf804f important 1 — fixed — deploy/helm/paperclip/tests/prometheus-rule.test.mjs:377 — the unchecked indexOf is gone. verifyIndex is captured at :376 and gated by assert.notStrictEqual(verifyIndex, -1, ...) at :377-383 before either slice consumes it, so both slice(verifyIndex) (:384) and slice(0, verifyIndex) (:405) are now unreachable with -1. That closes all three vacuous paths the finding named: the truthy one-character slice(-1), the vacuously-passing doesNotMatch, and the whole-file widening of the row-selection assertion. The Helm chart check is green at this head, which is the job that executes this file.

Critical Issues (0)

Important Issues (0)

Suggestions (1)

[gstack] deploy/helm/paperclip/templates/prometheusrule.yaml:244 — the dead-letter alert's description still says "Break down by the reason label to see which event type is failing" with no aggregation named, which is the same shape this PR just removed from the terminal-failed description one rule over (and guarded with a doesNotMatch at prometheus-rule.test.mjs:341). A careful reader resolves it to the counter — the next sentence says "the same counter" — and sum by (reason) (increase(...)) is correct there. But this description explicitly discusses both arms, including the gauge arm, and the gauge now carries reason on every replica, so a responder who fired on the gauge arm can reasonably land on sum by (reason) over it and read 3×. Immaterial to any decision today (the threshold is > 0, and the ranking between reason buckets survives multiplication), which is why this is a suggestion rather than a finding — but it is the asymmetry your own comment at :224-233 argues against, so naming the aggregation here would make the pair consistent and let the existing guard cover both.