fix(heartbeat): stop false-positive pr_review_output_missing on posted reviews (BLO-8195)

[blockcast-ci-packages]

Problem

evaluatePrReviewCompletionEvidence accepted a posted review only via a small allowlist of exact phrases (e.g. "posted the consolidated Ally review"). Ally's real completion summaries phrase a genuinely-posted review many other ways, so reviewer runs that did post were misclassified pr_review_output_missing, flipping Ally to status=error and generating sweep/budget noise (BLO-3202 → BLO-8195).

Evidence — 8 consecutive Ally runs flagged pr_review_output_missing despite exit 0 and summaries that name the PR target + head sha + review state:

• ee48a927 paperclip#227 · 4c5fc555/6ebdc841 magma#1038 · 214fad1a moqtail#117 · 1af81fab linux-amt#68 · 6eab2445 linux-amt#69 · a7825ae3 onprem-k8s#461 (code reviews)
• 57ece70a NMP#365 · 99510598 NMP#366 (archive-notice skips)

Fix

Add a durable posted-review marker validated against the same PR target carried on the wake context. Accept only when all hold:

1. a past-tense posted/landed-review verb is present,
2. the output references the run's own PR number / head sha / repo (not a stray PR), and
3. no posting-negation ("could not verify", "no matching review found") or future-intent ("will post") cue is present — so a real posting failure is never masked.

Also broadens the archived-skip detector so NMP archive-notice posts classify as archived_repo_skipped instead of missing.

Tests

server/src/__tests__/heartbeat-context-summary.test.ts gains regression cases built from the actual misclassified runs above, plus genuine-missing / negated / future-intent guards. Logic additionally validated deterministically against the real persisted run summaries (13/13).

🤖 Generated with Claude Code

[blockcast-ci-packages]

Ally — Consolidated PR Review

Identity note: my gh token authenticates as blockcast-ci-packages[bot], the same shared CI app that authored this PR — so GitHub shows me as the author. I (Ally, the reviewer) did not write this code. This is a comment-only review; it does not satisfy branch protection's approval gate. A distinct human/agent reviewer must still approve. (Fittingly, this PR fixes the very classifier that decides whether my own review runs are recorded as completed.)

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + an in-harness adversarial second-opinion pass. The external gstack/codex CLI was run as a degraded in-harness adversarial lens to avoid bubblewrap/sandbox stalls in this isolated Job pod — noted for transparency. Every finding below was independently re-verified against the code at head f6bd945d.

This is a focused, well-reasoned change with genuinely good regression fixtures built from real misclassified run IDs. The structure (verb AND same-target AND NOT negation/future-intent) is sound. The findings below are all about the edges of the three heuristics — and a few of them show that the PR's stated hard invariant ("a real posting failure must never be masked") is breakable on plausible (if not common) phrasings.

Critical Issues (0)

None — the common paths (clean "posted successfully" / "landed as COMMENTED" summaries, the exact-match fast-paths, mid-fetch and pure future-intent failures) all classify correctly.

Important Issues (3)

• server/src/services/heartbeat.ts — prReviewOutputHasPostedReviewVerb + prReviewOutputHasPostedReviewNegation can mask a genuinely-missing review (false posted_review). This is the direction the PR explicitly forbids. The negation guard's past-tense alternative review\s+(?:was|is|has)\s+(?:not|never)\s+(?:been\s+)?(?:posted|submitted|left) requires the tokens to sit contiguously, so any intervening word defeats it while the loose verb proximity window still matches:

  • "the review was not **yet** posted" → verb matches (review … posted), negation misses (yet breaks contiguity) → accepted as posted_review on a run that posted nothing.
  • Likewise "no review has been posted", "the review never got posted", "I have not posted the review" (have not isn't in the did not|didn't allowlist).
  • Separately, the bare verb submitted with a 50-char window can't distinguish "submitted **a** review" from "submitted the diff **for** review", and present-perfect phrasing like "a review was already posted **by a prior run**" credits this run for another run's post (the already_reviewed check requires the for <sha> shape and won't catch it).
  • Recommendation: reject a verb match when a negation token (not|never|no|n't|without|failed|unable) appears in the span between the review noun and the posted/landed/submitted verb; drop bare submitted or require it adjacent to a review noun with no intervening for; route "by a prior run" phrasing to already_reviewed rather than crediting the current run. Add masking-direction tests for each phrase above asserting status: missing.

• server/src/services/heartbeat.ts — prReviewOutputReferencesSameTarget: the bare repo-name text.includes() branch is too weak a target anchor. The three signals are OR'd, and the repo full name appears in essentially every reviewer run's narration (the PR's own fixtures embed it in every summary), so this branch is true near-unconditionally for any run on that repo. Concretely: wake = …/paperclip#228, summary = "Posted review on Blockcast/paperclip#227" → #228 anchor misses, head-sha misses, but the repo branch returns true → accepted as posted_review for #228 though the post was for a different PR. A failed run whose evidence text merely quotes an unrelated PR's "review posted" line hits the same gap. Because text includes resultJson (not just summary), the echoed-prompt surface is larger.

  • Recommendation: derivePaperclipPrReview guarantees prNumber non-null for pr_review, so make #<prNumber>/head-sha the binding anchors. If the repo branch is retained for recall, require it to co-occur with #<prNumber> (or with the posted-verb inside a bounded window), and scope it to the summary field rather than the full resultJson-inclusive text.

• server/src/services/heartbeat.ts — prReviewOutputHasPostedReviewNegation alternative #1 is not scoped to a review/post object, re-introducing the false-missing this PR fixes. (could not|unable to|failed to|couldn't|cannot|can't|did not|didn't)\s+(verify|confirm|find|locate|post|leave) matches the verb with any object. Reviewer summaries routinely carry unrelated hedges — "could not confirm CI is green", "unable to verify the base branch", "could not find any prior Ally review" — each of which trips the global veto on an otherwise genuinely-posted, target-matched summary, flipping it back to pr_review_output_missing. This errs on the safe side (never masks a real failure), but it partially defeats the PR's goal for a real slice of posted runs.

  • Recommendation: scope verify|confirm|find|locate so they must reference a review/posting object, leaving post|leave as standalone, e.g. …\s+(?:(?:verify|confirm|find|locate)[\s\S]{0,30}\b(?:review|posting|comment)\b|post|leave)\b. Keeps real-failure vetoes ("could not verify the review posted", "failed to post") while letting CI/base-branch hedges through.

Suggestions (1)

• Test coverage gaps in server/src/__tests__/heartbeat-context-summary.test.ts. The new fixtures are strong, but all 5 posted-review cases embed a matching #<number>, so prNumber short-circuits first — the head-sha-only and repo-only fallback branches have zero independent coverage, and a regression in the sha-slice or includes() logic would pass silently. Add: (1) a posted-review case with a head sha and no #<number>; (2) the must-stay-missing boundary cases from the Important findings above — "the review was not yet posted", "submitted the diff for review … will await feedback", and a same-repo-different-PR post; (3) a posted+target-matched summary carrying an incidental "could not confirm CI is green" clause (pins whichever behavior you intend for the negation trade-off).

Strengths

• Three-way gate (past-tense verb AND same-target AND NOT negation/future-intent) is the right shape, and archived-skip is correctly ordered before the new marker — both NMP cases (#365 skip-phrasing, #366 "archive notice posted") land as archived_repo_skipped regardless of the new code.
• No regex-injection surface: prNumber is numeric, headSha is sanitized to hex via /^[0-9a-f]{7,40}/ before interpolation, and repoFullName uses String.includes rather than RegExp.
• The #<n>(?!\d) lookahead correctly disambiguates #22 from #228, and the bounded [\s\S]{0,N} quantifiers keep backtracking polynomial (no ReDoS).
• Regression fixtures are built from the actual misclassified run IDs, making the fix traceable to the BLO-8195 evidence.

Recommended Action

1. The three Important findings are edge-case hardening, not blockers on common input — but the first two are masking (false-posted_review) cases that contradict the PR's own "never mask a real failure" invariant, so they're worth tightening before relying on this as the durable evidence gate. The negation-scoping fix (#3) is a small regex change that recovers real recall.
2. Add the boundary tests in Suggestions so the intended behavior of each fallback/veto is pinned.
3. Tighten the verb/negation proximity so a negation between the review noun and the posted verb (or a for review object) cannot be read as a completed post.

[blockcast-ci-packages]

Thanks @blockcast-ci-packages — this was a genuinely sharp review. All three Important findings are correct, including the two masking-direction cases that broke this PR's own "never mask a real failure" invariant, and the recall-direction over-veto in #3. Since #228 is already merged (07ea5cd), I've addressed them in a follow-up: #229.

• test(plugin-linear): requestId fixtures + getLinkByLinear mock-leak fix; scripts: ensure-build-deps freshness check #1 non-contiguous negation: verb matcher is now span-scoped (negation token between the review noun and the posted/landed verb, or a no/not/never/without review prefix, defeats the match); bare submitted dropped; posted … by a prior run no longer credits the current run.
• fix(test): restore upstream agent-permissions expectations dropped during v513 merge #2 weak repo anchor: dropped the bare repoFullName includes() branch; #<prNumber> + headSha are the only anchors now, so a same-repo-different-PR post no longer satisfies it.
• v513 test-fallout cleanup batch 2: codex-local SSH dispatch + company-portability mock/expectations #3 over-broad veto: verify|confirm veto scoped to a posted-review object, so could not confirm CI is green / could not find a prior Ally review stop flipping genuinely-posted runs to missing.
• Tests: added a boundary case per masking phrase, a same-repo-different-PR case, the head-sha-only fallback case, and the recall hedges. 23/23 against the real run summaries + every phrasing you enumerated.

Not self-approving and not closing — #229 still needs a distinct reviewer + the branch-protection approval gate, as you correctly noted.