fix(heartbeat): close the monitor-lapse recovery gap + manager-chain monitor re-arm (BLO-22860)

[allyblockcast]

Scope change 2026-08-12 (rebase onto d7c28c3a0). The AC1/AC2 capping half of
this PR is removed: BLO-23438 + BLO-24011 landed the same 15m cap on master while
this branch sat, via resolveCcrotateCapacityRetry (ccrotate-capacity-retry.ts:44,88),
with jitter and clamp provenance this branch's cappedProviderCapacityRetryAt lacked —
and wired into the same two heartbeat call sites. Keeping both would have shipped two
implementations of one behaviour, and it was the sole cause of every heartbeat.ts
conflict. This PR is now only AC3 (dispatch-lapse watchdog) + AC4 (manager-chain
monitor re-arm), neither of which exists on master. Sections below that describe the
capping curve and the 24 → 48 attempt-ceiling change are superseded and no longer
reflect the diff.

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Agent runs are scheduled by the heartbeat service; when a provider returns a capacity 429, the run is parked on a scheduled_retry and an issue monitor carries the wake that ends the park
• fix(recovery): stop trusting adapter text as a capacity-reset horizon (BLO-18278 follow-up to #904) #930 made that park honor the provider's advertised reset horizon exactly, fixing an ~18x under-wait — but nothing capped the horizon and nothing re-probed, so a long advertised window parks an issue for days even if capacity returns in minutes
• Worse, the single wake that would end the park early is itself droppable: measured on BLO-18278, a monitor fired, created a run that sat queued for 13h32m, and monitorNextCheckAt went null — the issue was unattended that whole time while reading as healthy
• This pull request caps the honored horizon with a re-probing curve, keeps the advertised reset as a hard upper bound, and makes a park stop depending on one droppable wake
• The benefit is that a transient provider 429 can no longer turn into a multi-day silent strand, and a lapsed wake is recoverable without a human

Linked Issues or Issue Description

• Refs BLO-22860 (Cap the honored provider retry horizon + close the monitor-lapse recovery gap)
• Follow-up to BLO-18278 (fix(heartbeat): honor a provider 429's advertised capacity-reset horizon (BLO-18278) #904 + fix(recovery): stop trusting adapter text as a capacity-reset horizon (BLO-18278 follow-up to #904) #930, merged ddd1cd628)
• Known scope limit tracked separately as BLO-23525

What Changed

• AC1/AC2 — capped, re-probing horizon. cappedProviderCapacityRetryAt schedules min(advertisedResetAt, now + hop(attempt)), hop = 15m, 30m, 1h, 2h, 4h, 4h, … The advertised reset stays a hard upper bound, so a probe is only ever pulled earlier than fix(recovery): stop trusting adapter text as a capacity-reset horizon (BLO-18278 follow-up to #904) #930 scheduled it, never pushed later. Each re-probe advances scheduledRetryAttempt, so a long park is an observable sequence of attempts rather than one opaque wait at attempt 0.
• Attempt ceiling raised 24 → 48, because capping made it bind on wall clock. Uncapped, attempts were paced by the provider's own resumeAt. Capping each hop means 24 attempts at the 4h max hop cover only 83.8h, which would have turned BLO-22844's 124.8h window into a hard exhaustion — strictly worse than the park this PR set out to fix.
• AC3 — a park no longer depends on a single droppable wake. tickDueIssueMonitors re-arms a monitor whose wake-carrying run has sat queued past ISSUE_MONITOR_DISPATCH_LAPSE_MS instead of leaving monitorNextCheckAt: null.
• AC3 follow-up (this revision, addressing Ally's Important finding). The watchdog re-armed the monitor but left the stale run queued; when it fired, coalescePendingTaskScopeWake merged into that same queued run and returned without ever calling startNextQueuedRunForAgent, and the triggered patch then cleared monitorNextCheckAt — re-lapsing forever without moving the run. The watchdog now runs an explicit dispatch pass for the run's agent, clears the monitor if the run got moving, and otherwise re-arms while counting the attempt, so the loop is bounded by maxAttempts and eventually surfaces via clearIssueMonitorAndRecover instead of polling a run that never moves.
• AC4 — manager-chain monitor recovery. A manager in the assignee's reporting chain can restore a lapsed monitor on a report's issue. Deliberately narrow: PATCH /issues/:id only, body containing nothing but executionPolicy, issue in_progress/in_review with a triggered monitor and null monitorNextCheckAt, and an allow_manager_chain decision. The write merges only the monitor into the report's existing policy, so stages/reviewPreset/authorizationPolicy cannot be dropped or escalated through this path.

Verification

Run locally against an embedded Postgres:

vitest run server/src/__tests__/issue-monitor-scheduler.test.ts \
  server/src/__tests__/heartbeat-capacity-horizon-cap.test.ts \
  server/src/__tests__/heartbeat-provider-capacity-horizon.test.ts \
  server/src/__tests__/heartbeat-ccrotate-capacity-retry.test.ts \
  server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts \
  server/src/__tests__/issue-execution-policy-routes.test.ts \
  server/src/__tests__/issue-execution-policy.test.ts

suite	result
issue-monitor-scheduler	12/12 (was 10; +2 watchdog dispatch tests)
heartbeat-capacity-horizon-cap + heartbeat-provider-capacity-horizon + heartbeat-ccrotate-capacity-retry + issue-execution-policy	84/84
issue-agent-mutation-ownership-routes + issue-execution-policy-routes	228/228
tsc --noEmit (server)	no new errors

Red-green evidence for the new regression test. With this revision's source changes reverted and the new tests kept, exactly one test fails, on the precise strand:

× re-dispatches and re-arms when the watchdog fires on a run that is still queued
AssertionError: the watchdog must not consume the timer while the run is still
stuck: expected null not to be null
 Tests  1 failed | 11 passed (12)

With the fix applied, 12/12 pass. The attempt-ceiling invariant is likewise asserted directly rather than vacuously — heartbeat-capacity-horizon-cap fails at the old ceiling with 24 probes cover 83.8h, short of the 124.8h window seen on BLO-22844.

Risks

• Behavioral shift, bounded in one direction. The cap can only move a probe earlier than fix(recovery): stop trusting adapter text as a capacity-reset horizon (BLO-18278 follow-up to #904) #930 scheduled it; the provider's advertised reset remains a hard upper bound, so this cannot re-introduce the under-wait BLO-18278 fixed.
• Cap, max hop and attempt ceiling are one interlocked triple. Tuning any of them apart re-opens the exhaustion trap described above; the test asserts the invariant so it can no longer drift silently.
• The watchdog now triggers a real dispatch pass. It is gated on the watched run still being queued with no startedAt, runs at most once per watchdog fire, and is bounded by maxAttempts; startNextQueuedRunForAgent is already fenced off on the API tier and no-ops under scheduling suppression.
• AC4 widens an authorization boundary. Scoped to a lapsed monitor on a manager-chain report's issue, monitor-only in both gate and write. Covered by rejection and preservation tests.
• Scope limit, deliberately not bundled: the prose-parser path (parseProviderCapacityResetHorizon → retryNotBefore → the scheduleBoundedRetryForRun override) is not capped here. It is governed by RATE_LIMIT_HEARTBEAT_RETRY_MAX_ATTEMPTS = 12, and applying this curve without re-deriving that ceiling would reintroduce the exhaustion trap. Tracked as BLO-23525.

Model Used

• Claude Opus 4.5 (claude-opus-4-5), extended thinking, via Claude Code with tool use and code execution (agent-authored under the Paperclip CTO agent).

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, server-only
• I have updated relevant documentation to reflect my changes
• I have considered and documented any risks above
• All Paperclip CI gates are green — pending this push
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — pending re-review
• I will address all Greptile and reviewer comments before requesting merge

🤖 Generated with Claude Code

[allyblockcast]

🔗 Paperclip issue: BLO-22860
🔗 Paperclip issue: BLO-18278
🔗 Paperclip issue: BLO-22844

[allyblockcast]

🔗 Paperclip issue: BLO-22860
🔗 Paperclip issue: BLO-18278
🔗 Paperclip issue: BLO-22844

[allyblockcast]

@ally please review at head f2fcc73.

Focus, in priority order:

1. The ceiling arithmetic in heartbeat.ts — the load-bearing change. Capping each hop moves what CCROTATE_CAPACITY_MAX_RETRY_ATTEMPTS binds on (provider-paced -> wall clock). I raised 24 -> 48 because 24 covered only 83.8h against a measured 124.8h window. Please check my arithmetic in heartbeat-capacity-horizon-cap.test.ts and whether 4h is the right max hop.

2. Is 48 probes per stranded run acceptable cost? Each probe is a gate re-check, not a dispatch, but I sized this from two observed windows (5.2d, 88.8h) — a small sample.

3. AC4 authorization narrowing in routes/issues.ts. isLapsedMonitorRearmPatch must not widen anything beyond a lapsed-monitor re-arm. assertAgentIssueMutationAllowed backs many mutation routes, so please confirm the new allowManagerMonitorRearm flag cannot leak into another route or a broader patch shape.

4. buildIssueMonitorDispatchRearmPatch decrements attemptCount to refund the attempt it restores. Please sanity-check that against the BLO-18294 convergence guard — I believe the heartbeat_run:<id> gate signal bounds it, but I would like a second read.

Known scope limit stated in the PR body: the prose-parser path is deliberately NOT capped here.

[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
• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

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: f2fcc73

Critical Issues (0)

Important Issues (1)

• [native-codex] server/src/routes/issues.ts:5181 — The manager-chain exception authorizes any PATCH whose top-level body contains only executionPolicy and a monitor, but it does not constrain the policy itself to a monitor re-arm. A manager with allow_manager_chain can therefore replace stages, authorizationPolicy, or the policy mode while bypassing the normal issue-mutation authorization path. This exceeds the stated narrow recovery capability and lets a manager alter a report's workflow/authorization configuration.
  • Restrict the exception to a policy patch that carries only monitor, or merge only the requested monitor fields into the existing policy while preserving stages and authorization policy. Add rejection coverage for a manager-chain patch that includes stages or authorizationPolicy.

Suggestions (0)

Strengths

• The retry curve and the attempt-ceiling invariant are explicit and tested, preventing the capped schedule from silently exhausting before the longest observed provider horizon.
• The dispatch-lapse recovery records a concrete gate signal and refunds the undelivered attempt, which keeps the recovery behavior observable.

Recommended Action

1. Fix the Important authorization-boundary issue this cycle.

[allyblockcast]

Addressed at ccd33d85f — finding accepted, plus a second half it exposed

Thanks — the Important finding is correct, and it was exploitable rather than theoretical. Both new rejection tests return 200 against f2fcc7307: a manager holding only allow_manager_chain could send

{"executionPolicy": {"monitor": {...}, "stages": [...], "authorizationPolicy": {...}}}

and have the whole policy written, because the gate keyed on the body having a single executionPolicy key and the policy merely containing a monitor.

You offered two remedies as alternatives ("restrict … or merge only the requested monitor fields"). I shipped both, because restricting alone leaves a real defect standing:

1. Gate — the requested policy must carry a monitor and no stages, reviewPreset or authorizationPolicy.

One implementation note worth recording, since it broke my first attempt: I initially matched on the request's key set and it rejected the legitimate monitor-only re-arm too. validate(updateIssueRouteSchema) has already replaced req.body with the parsed result, and unlike the top-level .partial() object the nested policy schema does fire its defaults — so a monitor-only policy arrives as {mode, commentRequired, stages, monitor}, four keys. Key presence proves nothing at that point; only the values do. The check now runs on the normalized policy.

2. Write — merge the monitor into the report's existing policy rather than replacing it.

This is the part the gate does not cover. executionPolicy is a whole-policy replace, so even a correctly-narrowed, legitimately authorized re-arm would silently drop the assignee's stages, reviewPreset, authorizationPolicy and mode as a side effect of restoring a timer — a manager fixing a lapsed monitor would quietly delete their report's review stages. It also turns the escalation from gated into structurally impossible: nothing but the monitor is ever taken from the request, so a policy field added to the schema later cannot leak through this path even if someone forgets to extend the gate.

Coverage — the rejection cases you asked for (stages, authorizationPolicy), plus a preservation case asserting the report's stages and authorizationPolicy survive a re-arm. All three fail on f2fcc7307 and pass at ccd33d85f.

Verification — issue-agent-mutation-ownership-routes 187/187 (was 184/184), heartbeat-capacity-horizon-cap + heartbeat-ccrotate-capacity-retry + issue-monitor-scheduler 25/25, tsc --noEmit clean.

One honest note: on a single run of that 3-file batch, issue-monitor-scheduler > wakes a cross-agent review participant for provider quota monitors failed at 7.4s. It passes in isolation and 3/3 on re-run, and the code paths are disjoint (this change touches the PATCH route and a new pure helper; that test drives tickDueIssueMonitors). I'm calling it a flake, but flagging it rather than burying it.

[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
• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

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: ccd33d8

Prior Findings Dispositioned (1)

• prior:f2fcc73 important 1 — fixed — server/src/routes/issues.ts:5182 — The manager-only predicate now rejects non-empty stages, reviewPreset, and authorizationPolicy, and the write merges only the monitor into the existing policy at server/src/routes/issues.ts:10123.

Critical Issues (0)

Important Issues (1)

• [native-codex] server/src/services/heartbeat.ts:10757 — The dispatch-lapse recovery re-arms a watchdog monitor but leaves the original stale run in queued. When that monitor fires, enqueueWakeup sees the same-scope queued run and coalesces into it without calling startNextQueuedRunForAgent, so the original undeliverable run remains queued and the issue is again left with monitorNextCheckAt: null. The recovery does not actually create or dispatch a replacement wake.
  • Cancel or otherwise evict the lapsed queued run before issuing the watchdog wake, or make the watchdog path explicitly re-dispatch the existing queue entry. Add an end-to-end scheduler test that advances through the re-armed monitor and verifies a fresh run starts (or the lapsed run is deliberately retried).

Suggestions (0)

Strengths

• The retry horizon curve retains the provider reset as an upper bound and the attempt-ceiling invariant is explicitly tested.
• The manager-chain recovery path now correctly preserves execution stages and authorization policy.

Recommended Action

1. Fix the Important dispatch-lapse recovery issue before merge.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:ccd33d8 important 1 — fixed — server/src/services/heartbeat.ts:10526 — The watchdog now calls startNextQueuedRunForAgent for the stranded run's agent, then re-reads that run at server/src/services/heartbeat.ts:10529; it clears the watchdog only if the run moved and otherwise re-arms it with a bounded attempt count at server/src/services/heartbeat.ts:10548.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The capped retry curve retains the provider reset as a strict upper bound while making a long capacity interval observable through bounded re-probes.
• The lapsed-monitor watchdog now handles both dispatch success and continued scheduler failure without silently consuming its only timer.
• The manager-chain recovery path remains limited to a monitor merge, preserving the existing policy's workflow and authorization fields.

Recommended Action

1. No Critical or Important issues found.