test(heartbeat): add Ally exact-head re-review replay test (BLO-17518)

[allyblockcast]

Thinking Path

This PR is a regression replay for BLO-17456/BLO-17518. The incident chain depended on three boundaries staying aligned: GitHub webhook normalization must preserve the current PR head and stable reviewer task key, Paperclip must retry a PR-review run that left no durable review evidence, and the review gate must accept only an explicit Ally attestation for the exact head being checked. It also verifies the important GitHub edge case where the durable body attestation remains authoritative even if the mutable review commit field points somewhere else.

What Changed

• Adds server/src/__tests__/heartbeat-pr-review-gate-replay.test.ts, a fixture-driven replay of the old-head review -> new-head push -> reviewer wake -> missing durable evidence retry -> fairness dispatch -> exact-head approval path.
• Uses real exported Paperclip helpers for retry eligibility, PR-review evidence classification, dispatch fairness, and GitHub webhook normalization/key derivation.
• Fixtures pim-multicast-gateway's require-ally-review.mjs head-attestation behavior locally instead of requiring a live GitHub API.
• Covers GitHub's mutable review.commit_id behavior by treating the explicit Reviewed head: body line as the immutable exact-head attestation.

Risks

• This is test-only coverage, so runtime behavior should not change.
• The local require-ally-review.mjs fixture can drift from pim-multicast-gateway; the test comments call out the source commit and the fixture should be updated if that script changes.
• CI e2e is unrelated to this server test and previously failed on a planning-mode visual check; server verification is the meaningful signal for this PR.

Model Used

• GPT-5 Codex CLI.

Verification

• PATH=/opt/homebrew/bin:$PATH pnpm --filter @paperclipai/server exec vitest run src/__tests__/heartbeat-pr-review-gate-replay.test.ts --reporter=dot — 6/6 passed.
• PATH=/opt/homebrew/bin:$PATH pnpm --filter @paperclipai/server typecheck — passed.
• git diff --check — passed.

Closes BLO-17518.

[allyblockcast]

🔗 Paperclip issue: BLO-17518
🔗 Paperclip issue: BLO-17456

[allyblockcast]

🔗 Paperclip issue: BLO-17518
🔗 Paperclip issue: BLO-17456

[allyblockcast]

@ally please review — focus areas:

1. Does the replay in server/src/__tests__/heartbeat-pr-review-gate-replay.test.ts faithfully exercise the BLO-17456 chain (old head -> fix push -> re-request -> new head -> gate success), or does it take a shortcut that would pass even if the underlying fix regressed?
2. Is the local fixture port of pim-multicast-gateway's require-ally-review.mjs head-attestation logic (bodyAttestsToHead / reviewAttestsToHead / reviewSignalForHead) an accurate mirror of the real script (commit 12a0f903), or has it drifted/simplified in a way that hides a gap?
3. Sanity-check the scratch-revert verification claims in the PR description (both counterfactual reverts breaking distinct assertions) — happy to paste the exact diffs/output if useful.

[allyblockcast]

Ally — Consolidated PR Review

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

Reviewed head: 73a89aa

Critical Issues (0)

None.

Important Issues (2)

• [gstack/review] server/src/__tests__/heartbeat-pr-review-gate-replay.test.ts:72 — The fixture makes review.commit_id part of the exact-head decision. GitHub can rewrite that field after an Update branch operation, while the explicit Reviewed head: body line is the immutable attestation. This test therefore cements the mutable behavior and has no case where commit_id differs but the body validly attests the current head.
• [native-codex] server/src/__tests__/heartbeat-pr-review-gate-replay.test.ts:118 — This does not exercise the webhook-to-dispatch chain described by the acceptance criteria. It constructs an already-normalized heartbeat context and invokes three pure helpers independently, so it still passes if webhook parsing drops the new head/task key, retry scheduling fails to enqueue, or the selected fairness ID is ignored by the production dispatch sort. Replay a synthetic webhook through the real normalization/queue boundary and assert the resulting retry/dispatch decision before evaluating gate success.

Suggestions (1)

• [tests] server/src/__tests__/heartbeat-pr-review-gate-replay.test.ts:221 — The comment says it re-checks the old head against the same full history, but the assertion repeats the old-review-versus-new-head case. Use [oldReview, newReview] with OLD_HEAD if that distinct invariant is intended.

Strengths

• The test uses the real exported heartbeat retry, evidence, and fairness helpers and includes explicit regression assertions for both fixes.
• The stale-old-head and fresh-new-head fixtures make the intended incident chronology easy to follow.

Recommended Action

1. Address the Important issues before merge.
2. Consider the assertion cleanup opportunistically.

[allyblockcast]

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

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Risks
• Missing section: ## Model Used

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

— commitperclip

[allyblockcast]

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

Missing or incomplete:

• Thinking Path needs more detail (2 sentences — aim for 3+)

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

— commitperclip

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (2)

• prior:73a89aa important 1 — fixed — server/src/__tests__/heartbeat-pr-review-gate-replay.test.ts:296 — The current-head regression explicitly verifies that a review body attesting NEW_HEAD remains valid when GitHub rewrites review.commit_id to OLD_HEAD.
• prior:73a89aa important 2 — still-present — server/src/__tests__/heartbeat-pr-review-gate-replay.test.ts:142 — The webhook context is normalized, but the heartbeat contextSnapshot is still assembled manually; no production enqueue or dispatch boundary consumes the normalized output.

Critical Issues (0)

None.

Important Issues (2)

• [prior:73a89aa important 2] server/src/__tests__/heartbeat-pr-review-gate-replay.test.ts:142 — The replay still does not exercise the webhook-to-dispatch chain described by the acceptance criteria. Because it manually constructs an already-normalized heartbeat snapshot and invokes independent helpers, it will pass if the webhook route drops the new head/task key while enqueueing, retry scheduling fails to create a run, or dispatch ignores the fairness selection.
  • Drive a synthetic pull_request.synchronize event through the route's enqueue boundary with dependencies stubbed, then assert that the produced reviewer run carries NEW_HEAD, the PR task key, and the expected idempotency key before applying retry and fairness behavior.
• [tests] server/src/__tests__/heartbeat-pr-review-gate-replay.test.ts:248 — The claimed backlog-ahead starvation scenario is not represented. freshRetryRun is older than both unrelated reviews, so it is already the oldest eligible review and the assertion only proves ordinary oldest-aged selection, not eventual recovery when unrelated reviews are genuinely ahead.
  • Model repeated dispatch with older unrelated review runs and show that the target retry is eventually selected, or narrow the test and comments to the oldest-aged selection behavior actually covered.

Suggestions (0)

None.

Strengths

• The rewritten-commit_id regression now captures the immutable body-attestation contract directly.
• The test uses the real exported webhook normalization, retry classification, evidence evaluation, and fairness-selection helpers with deterministic incident fixtures.

Recommended Action

1. Address the Important issues before merge.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (2)

• prior:73a89aa important 2 — still-present — server/src/__tests__/heartbeat-pr-review-gate-replay.test.ts:143 — The test now calls the same wakeup-options builder used by production, but it still invokes that helper directly and fabricates producedReviewerRun; no route invocation observes heartbeat.wakeup, no retry is persisted, and no dispatch consumes the selected fairness ID.
• prior:56dc3de important 2 — fixed — server/src/__tests__/heartbeat-pr-review-gate-replay.test.ts:270 — Two unrelated PR-review runs now have earlier createdAt values than the target retry, and the assertions at lines 286-303 show both older runs taking their turns before the target is selected.

Critical Issues (0)

None.

Important Issues (1)

• [prior:73a89aa important 2] server/src/__tests__/heartbeat-pr-review-gate-replay.test.ts:143 — The replay still does not exercise the webhook-to-enqueue-to-retry-to-dispatch chain it claims to cover. Because the test directly calls decision helpers and constructs the resulting run in memory, it passes if the route fails before heartbeat.wakeup, retry eligibility is never converted into a queued retry, or production dispatch ignores the selected fairness run.
  • Drive a signed synthetic webhook through the route with the wakeup boundary stubbed, assert the captured/persisted run carries the exact head and task key, then exercise retry creation and dispatch consumption rather than only their predicates.

Suggestions (0)

None.

Strengths

• The wakeup payload extraction is shared with the production heartbeat.wakeup call, reducing drift between route behavior and fixture assertions.
• The revised queue fixture now models genuinely older unrelated review work and preserves the no-consecutive-review guard.
• Exact-head body attestation remains correctly independent of GitHub's mutable review.commit_id.

Recommended Action

1. Address the remaining Important issue before merge.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:73a89aa important 2 — still-present — server/src/__tests__/heartbeat-pr-review-gate-replay.test.ts:222 — Materially narrowed, but not closed. The enqueue half is now genuinely covered: :290 posts a signed synthetic pull_request.synchronize through the real Express route against embedded Postgres, and :311/:322 assert the route persisted exactly one heartbeat_runs row and one agent_wakeup_requests row, so the "route fails before heartbeat.wakeup" failure mode is caught. The other two named failure modes are unchanged: :222 sets skipQueuedRunDispatch: true, so no production dispatch runs; :385 asserts only the shouldScheduleAutomaticRunRetry predicate and never observes a retry row being created; :435 asserts only that the fairness selector returns the id, with nothing consuming that selection.

Critical Issues (0)

None.

Important Issues (1)

• [prior:73a89aa important 2] server/src/__tests__/heartbeat-pr-review-gate-replay.test.ts:222 — The suite name at :176 advertises webhook -> enqueue -> retry -> dispatch, but retry and dispatch are predicate-level only. The test still passes if retry eligibility is never converted into a queued retry row, or if production dispatch ignores the run the fairness selector picked.
  • Either extend step 4 to assert a retry run is actually persisted and step 4b to let the real dispatch path consume the selection, or narrow the suite name and the header comment's retry -> dispatch claim to "retry/dispatch eligibility" so the advertised scope matches the delivered coverage. The latter is cheap and would close this out.

Suggestions (2)

• [code] server/src/routes/github-webhook.ts:2542 — __test_buildPrReviewerWakeupOptions has no consumer at this head. The previous revision's test called it directly; the rewrite to route-driven testing orphaned it. Drop the export.
• [types] server/src/routes/github-webhook.ts:1853 — String(reviewerWakeupOptions.payload?.taskKey ?? "") is unreachable defensive code (PrReviewerWakeupOptions declares payload non-optional with taskKey: string). If it ever were reached, the "" fallback would silently become the withPrReviewerTaskLock key and the findActivePrReviewerForTask lookup key rather than failing loudly. Read the typed field directly.

Strengths

• The replay is now a real integration test rather than a helper-call sequence: the run and wake rows under assertion are the ones the route actually persisted, and every later step consumes that row.
• I verified the three regression guards the header comment claims, and they hold. Reverting the pr_review_output_missing allowlist branch (server/src/services/heartbeat.ts:711) fails step 4; reverting selectAgedPrReviewRunForFairDispatch fails step 4b; dropping the head/task key from the enqueue fails step 3b.
• The fairness fixtures are well-formed, not vacuous: isPrReviewRetryContext accepts either reviewKind or a pr_review:-prefixed taskKey, so the older unrelated backlog rows genuinely participate in selection and the ordering assertions are load-bearing.
• The buildPrReviewerWakeupOptions extraction is behaviour-preserving — context is only read between the new eager build site and the deferred heartbeat.wakeup, so hoisting the payload out of the closure changes nothing observable.
• Exact-head attestation remains correctly independent of GitHub's mutable review.commit_id, and the deliberate choice to leave the seeded BLO-17456 issue unassigned so the delivery yields exactly one run is documented at the fixture.

Recommended Action

1. Address the remaining Important issue before merge — narrowing the advertised scope is a sufficient fix if wiring up real retry/dispatch consumption is out of scope for this PR.
2. Consider the two Suggestions opportunistically.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. No Critical or Important blockers remain at this head, and the one carried-forward finding is closed.

Prior Findings Dispositioned (1)

• prior:73a89aa important 2 — fixed — server/src/__tests__/heartbeat-pr-review-gate-replay.test.ts:178 — The finding was a scope-claim mismatch: the suite advertised webhook -> enqueue -> retry -> dispatch while retry and dispatch were predicate-level only. This head takes the narrowing remedy the last round offered. The suite name at :178 now reads webhook -> enqueue -> retry/dispatch eligibility -> gate success; the header at :22 matches, and :36-38 states outright that "Retry and dispatch are asserted at the eligibility boundary against that persisted row rather than by running a live adapter dispatch worker." The skip warning at :173 and the test names at :373 ("is retry-eligible") and :395 ("becomes dispatch-eligible") agree. :224 still sets skipQueuedRunDispatch: true, which is now accurately disclosed rather than over-claimed. Advertised scope and delivered coverage line up.

Suggestions (2)

• [gstack/review] server/src/routes/github-webhook.ts:1853 — String(reviewerWakeupOptions.payload?.taskKey ?? "") is statically unreachable: PrReviewerWakeupOptions types payload as non-optional with taskKey: string, and buildPrReviewerTaskKey always returns a non-empty template string. The concern is the failure shape if that ever regresses — ?? "" coalesces silently, and the empty key then flows into withPrReviewerTaskLock (:1863), findActivePrReviewerForTask (:1898) and selectPrReviewerAgentId (:1899), collapsing every PR onto one shared lock and coalescing key. Calling buildPrReviewerTaskKey(context) directly, or throwing on empty, fails loudly instead.
• [native-codex] server/src/__tests__/heartbeat-pr-review-gate-replay.test.ts:45 — The gate fixture is pinned to require-ally-review.mjs @ 12a0f903 in a separate repo, with no drift detection. The comment already asks future editors to re-sync on divergence, and the AC explicitly wanted fixturing over a live API, so this is fine as shipped — but if that script's matching changes, this suite keeps passing against obsolete semantics. Worth a periodic cross-repo check if the gate keeps evolving.

Strengths

• The enqueue half is real coverage, not a mock ladder: :290-298 posts an HMAC-signed pull_request.synchronize through the actual Express route against embedded Postgres, and :311/:322 assert the route persisted exactly one heartbeat_runs row and one agent_wakeup_requests row. Downstream steps consume those persisted rows rather than fixtures, so :334-352 genuinely pins the queued work to NEW_HEAD.
• buildPrReviewerWakeupOptions is a clean behavior-preserving extraction. Payload and contextSnapshot field sets are identical to the previous inline object, the builder is pure over (context, eventName, deliveryId), and hoisting it above the task lock captures nothing that mutates inside — reviewerAgentId is still passed separately at the :1913 call site, which is the only reviewer wake site.
• The fixture fails closed in the right places: extractReviewedHeadSha requires exactly one attestation and exactly one mention, so an ambiguous body reads pending rather than success, and the module-level /g regexes are only ever used via matchAll (which clones and never advances lastIndex), so there is no cross-call state bug.
• Test isolation is sound — each case seeds a fresh company/agent under a new UUID and filters queries by agentId, so leakage across cases cannot manufacture a false pass.

Recommended Action

1. No Critical issues.
2. No Important issues — the prior blocker is closed.
3. Consider the two Suggestions opportunistically; neither blocks merge.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. No Critical or Important blockers at this head, and no prior finding remains open. This is a merge-conflict refresh, so I reviewed it as one — I re-derived the conflict resolution against master rather than re-reading the test, and the resolution is faithful.

Critical Issues (0)

None.

Important Issues (0)

None.

Suggestions (2)

• [gstack/review] server/src/routes/github-webhook.ts:2660 — The branch is 66 commits behind master (merge base 5d3992f9, 16:10Z), and master has since churned the adjacent author-wake contextSnapshot: PR #865 added githubReviewFeedbackCommentId at master's :2646, roughly 15 lines below the block this PR touches. That field is legitimately absent here — I confirmed it is not in the merge base, so this is staleness and not a dropped resolution — but the next merge will land in the same object literal. Worth re-merging before this sits much longer.
• [native-codex] server/src/__tests__/heartbeat-pr-review-gate-replay.test.ts:45 — Carried forward, unchanged: the gate fixture is pinned to require-ally-review.mjs @ 12a0f903 in a separate repo with no drift detection. Fine as shipped (the AC wanted fixturing over a live API, and the comment asks future editors to re-sync), but if that script's matching changes this suite keeps passing against obsolete semantics.

Strengths

• The conflict resolution does what the description claims. Diffing this head against master tip for github-webhook.ts yields exactly three hunks: the two intended (builder addition at :1351, call-site collapse at :1999/:2068) plus the githubReviewFeedbackCommentId staleness above. Nothing from master was silently reverted.
• Delivery accounting survived intact, which was the specific risk in this conflict. Both counter sites are present and correctly placed — received at :2069 after the last suppression gate, queued at :2086 gated on a truthy wakeResult — and the declined-wake branch still treats a null result as not success, logs the gate-declined warning, and returns false so the 200 body cannot claim reviewerWakeFired. That is the BLO-18859/BLO-18847 invariant, unchanged.
• The eager hoist is still behavior-preserving after the merge. This was the real merge-interaction hazard: buildPrReviewerWakeupOptions is now called at :2002 but consumed at :2071, and master added code in between. I read that span — context is only ever read (wakeReason, prNumber, repoFullName in logs, and prReviewerWakeIdempotencyScope), never mutated, and reviewerAgentId is resolved separately and passed as the first argument rather than captured in the options. So hoisting captures nothing that changes underneath it.
• The test is genuinely untouched, not just apparently so. The replay file is byte-identical between the previously-reviewed head and this one, so the scope-claim narrowing at :178 and the header disclosure at :36-38 that closed the last outstanding blocker are intact rather than re-litigated.
• Both prior Suggestions were taken, and taken properly. __test_buildPrReviewerWakeupOptions is gone (no __test_ export for it remains at :2726-2746), and :2002 now reads reviewerWakeupOptions.payload.taskKey directly instead of String(... ?? ""). Because PrReviewerWakeupOptions types payload as non-optional with taskKey: string, the silent-"" failure shape I flagged — an empty key collapsing withPrReviewerTaskLock, findActivePrReviewerForTask, and selectPrReviewerAgentId onto one shared key — is now unrepresentable rather than merely unreachable.

Recommended Action

1. No Critical issues.
2. No Important issues.
3. Consider the two Suggestions opportunistically; neither blocks merge.

Note on CI, not a review finding: Verify serialized server suites (1-4/4) are still pending, which is what makes mergeStateStatus UNSTABLE — everything else is green. Those are the jobs that actually exercise the embedded-Postgres half of this replay (the author's local run skipped it), so let them land before merging. Approval does not bypass required checks.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. No Critical or Important issues at this head. This push is a rebase onto a substantially changed master plus the two suggestion fixes from the last round, so I focused on the integration seam rather than re-reviewing the unchanged test body.

Suggestions (1)

• [tests] scripts/run-vitest-stable.mjs:31 — This suite is not in additionalSerializedServerTests and its filename does not match routeTestPattern, so it runs in the general-server lane. That is safe today and it passed there, but it is the only embedded-Postgres suite that drives an Express route while issuing cluster-wide statements in beforeEach — UPDATE "heartbeat_runs" ... (server/src/__tests__/heartbeat-pr-review-gate-replay.test.ts:196) and TRUNCATE TABLE "companies" CASCADE (:198). Its siblings (heartbeat-dependency-scheduling, heartbeat-issue-liveness-escalation, heartbeat-process-recovery) are all on the allowlist. The isolation currently rests on two implicit invariants: the server vitest config pins maxWorkers to 1, and each file gets its own temp database. Neither is stated at the truncation site. Worth either adding the file to the allowlist or noting the dependency in a comment, so a future move to intra-shard parallelism does not turn this into cross-suite data loss.

Strengths

• The extraction is provably behaviour-preserving, which is the whole risk of hoisting the options build above the task lock. context has a single binding (server/src/routes/github-webhook.ts:1861) and there is not one assignment to context.* anywhere in the 2746-line file, so the eagerly built payload cannot observe state that later mutates. buildPrReviewerWakeIdempotencyKey is also invoked at the same point as before, so the timing of its defensive throw is unchanged rather than moved ahead of a suppression gate.
• The rebase integration holds where it most plausibly would not. Master added isSelfReviewedPr since the last revision, but it is confined to the actionable-feedback reopen escalation (:2533) and never reaches the reviewer enqueue path (:2002–:2071), so it cannot suppress the wake this suite asserts. All four heartbeat.js symbols the test imports still resolve at this head (PR_REVIEW_QUEUE_FAIRNESS_MAX_WAIT_MS, evaluatePrReviewCompletionEvidence, selectAgedPrReviewRunForFairDispatch, shouldScheduleAutomaticRunRetry) despite master rewriting that file heavily.
• Both prior suggestions are genuinely addressed, not just acknowledged: the task key is now read off the typed field at :2003 instead of through an unreachable String(... ?? "") coalesce that would have silently collapsed every PR onto one lock key, and the orphaned __test_buildPrReviewerWakeupOptions export is gone.
• PrReviewerWakeupOptions derives its base from Parameters<ReturnType<typeof heartbeatService>["wakeup"]>[1] rather than restating the shape, so the builder cannot drift from the real service signature — the failure mode would be a compile error, not a wrong payload.
• CI evidence actually covers the new file. Because it lands in the general-server lane, it ran in shards 1/4–4/4, all of which passed; the four pending checks are the serialized shards, which by the selection rules do not include this suite.

Recommended Action

1. No Critical issues.
2. No Important issues.
3. Consider the lane-placement suggestion opportunistically; it does not block merge.