refactor(recovery): fix load-bearing comment claims from Ally review at 0265d4ea (BLO-27676)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• The recovery subsystem raises harness_liveness_escalation rows when an issue in the graph has no owning next action, and suppresses re-raises so the class terminates
• BLO-27676 found that class never terminating; fix(liveness): gate liveness re-escalation on target change, not elapsed time (BLO-27676) #1394 (merged b0b623ff) fixed the real defect — an unconditional time-based re-escalation cooldown — by adding a second suppressor keyed on whether the target actually changed
• That suppressor's correctness rests on a subtle SQL argument (a updated_at bound standing in for a non-sargable resolvedAt), so fix(liveness): gate liveness re-escalation on target change, not elapsed time (BLO-27676) #1394 carried long explanatory comments — and Ally's review found two of those comments claim more than they prove
• A comment that overstates its own guarantee is worse than no comment: the next person to touch the scan bound will trust it and weaken the skew allowance it silently depends on
• This pull request corrects those claims and removes a hardcoded constant that would silently retire a boundary test
• The benefit is that the invariants guarding the suppressor stay legible and stay tested when someone tunes them

Linked Issues or Issue Description

• Refs fix(liveness): gate liveness re-escalation on target change, not elapsed time (BLO-27676) #1394 (this is the follow-up carrying its review findings)
• Refs BLO-27676 — https://paperclip.blockcast.net/BLO/issues/BLO-27676

What Changed

• heartbeat-issue-liveness-escalation.test.ts — derive the ceiling from DEFAULT_LIVENESS_UNCHANGED_TARGET_SUPPRESSION_MS instead of hardcoding 7 * 24 * 60 * 60 * 1000. The fixture is positional: completedAt 5s inside the ceiling, updatedAt 5s outside, so the row is reachable only via the skew allowance. Tuning the constant upward would put both columns well inside the wider horizon — the row would pass with or without skew and the assertions would still hold, silently retiring the boundary check exactly when someone is changing the thing it guards.
• recovery/service.ts, desc ORDER BY argument — restated as the superset property rather than a claim that stands alone. It rests entirely on LIVENESS_SUPPRESSION_SCAN_SKEW_MS. Added the counterexample showing that excluding a strict subset can move the pick; the old text asserted only excluding every candidate could.
• recovery/service.ts, residual-divergence list — rescoped from "every pre-creation exit" (by position) to "every exit that creates no row" (by outcome), which is the question a reader actually has. Added the unique-violation race as (6); it was excluded on a technicality despite producing the same divergence.
• recovery/service.ts, safe-direction inventory — replaced the stale count "the other four update(issues) sites" and marked the replacement explicitly as examples rather than an inventory.

Verification

• pnpm --filter @paperclipai/server typecheck — clean
• npx vitest run server/src/__tests__/heartbeat-issue-liveness-escalation.test.ts — 36/36 passed (78s, embedded postgres)

Enumerated every site that writes issues.completed_at to confirm the universal safe-direction claim actually holds — the review finding was only that the count was wrong, but the first replacement inventory was still incomplete, missing productivity-review.ts and issue-tree-control.ts:

site	completed_at	updated_at	direction
issues.ts:401 (applyStatusSideEffects)	2nd clock read	1st	⚠ unsafe — already called out
issues.ts:9284 (create)	new Date()	defaultNow()	safe
recovery/service.ts:9786 (escalation-close)	input.now	input.now	safe
recovery/service.ts:3519 (reopen)	null	input.now	safe
productivity-review.ts:2652 / :4058	now / input.now	same	safe
issue-tree-control.ts:886 / :989 (cancel)	null	—	safe

Also confirmed issues.updatedAt is .notNull().defaultNow() with no $onUpdate (packages/db/src/schema/issues.ts:89), and DEFAULT_LIVENESS_UNCHANGED_TARGET_SUPPRESSION_MS === 7 * 24 * 60 * 60 * 1000.

Risks

Low risk — no runtime behaviour change. The service.ts diff is comments only. The single non-comment change is a test fixture substituting a named constant for a literal of the same value, so it is behaviour-preserving today; its purpose is that it stops being behaviour-preserving (correctly, by failing) if the constant is ever retuned.

No migration, no API change, no UI change. The one thing a reviewer should genuinely check is the claim in the restated ORDER BY comment — if my superset argument is itself wrong, the comment is wrong in a new way rather than an old one. That is the specific focus I asked Ally for.

Model Used

• Claude Opus 4.5 (claude-opus-4-5), 1M context, extended thinking, with tool use and code execution (Claude Code agent harness).

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots — n/a, no UI change
• I have updated relevant documentation to reflect my changes — the comments are the documentation here
• I have considered and documented any risks above
• All Paperclip CI gates are green — in progress
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-27676

[allyblockcast]

@ally please review at head 7cdfd66 — follow-up to #1394 addressing your two Important findings at 0265d4ea.

Specific review focus:

1. The restated desc ORDER BY argument (recovery/service.ts ~7986). I claim the guarantee is the superset property given completed_at <= updated_at + skew, and that the previous "only excluding every candidate can move the pick" was wrong — a strict subset can. Please check the counterexample (X at completed_at = now-1h / updated_at = now-8d, Y at both now-3d) actually demonstrates that, and that my superset claim is not itself overstated.

2. The safe-direction inventory. Your finding was that "the other four" was a wrong count. My first pass replaced it with a named list that was still incomplete (it missed productivity-review.ts and issue-tree-control.ts). I have since verified all seven write sites and marked the list as examples rather than an inventory. Please confirm I did not miss a completed_at writer — that is the claim most likely to be wrong, and the one the skew argument depends on.

3. The test fixture change (hardcoded 7 * 24 * 60 * 60 * 1000 → DEFAULT_LIVENESS_UNCHANGED_TARGET_SUPPRESSION_MS). Behaviour-preserving today since the constant has that value. Please sanity-check the reasoning that a literal would silently retire the boundary check if the constant were tuned upward.

Verification: server typecheck clean; heartbeat-issue-liveness-escalation.test.ts 36/36 passed.

[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
• PR is titled docs: but includes source code changes (server/src/services/recovery/service.ts). Please retitle as fix:, feat:, or refactor: so the right gates run, or remove the source code changes if this is genuinely a docs: PR.

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

— commitperclip

[github-actions]

@ally head fd48fbf has been awaiting review for 80.1h with no review on either surface (pulls/1446/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 fd48fbf.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (1)

• [comments] server/src/services/recovery/service.ts:7991 — The invariant explanation is correct and valuable; keep this reasoning close to the query if the surrounding implementation changes.

Strengths

• The test now derives its boundary from DEFAULT_LIVENESS_UNCHANGED_TARGET_SUPPRESSION_MS, so tuning the production ceiling cannot silently invalidate the skew-boundary fixture.
• The query comment now states the necessary superset invariant and includes a concrete strict-subset counterexample, avoiding the previous overclaim about ordering.
• The residual-divergence documentation now accounts for the unique-violation race by outcome, and the stale call-site count has been removed.

Recommended Action

1. Consider the suggestion opportunistically.

[kkroo]

Reviewed current head fd48fbf. Ally reports zero Critical/Important findings; required checks and verify are green. The remaining note is non-blocking documentation guidance.