fix(productivity-review): anchor long_active_duration to dispatch

[kkroo]

Thinking Path

• Paperclip is the control plane for AI-agent companies, so its productivity reviews must distinguish actual work from scheduler queue latency.
• The productivity-review service uses long_active_duration to surface genuinely stalled in_progress work to a manager.
• Current master begins that duration at issue checkout, even when the latest execution evidence is a queued run that has never dispatched.
• That can create a false positive while recently dispatched runs prove the assignee is still making progress.
• The app-authored predecessor fix(productivity-review): anchor long_active_duration to dispatch, not checkout (BLO-19604) #1036 identified the issue but cannot receive the required independent App approval.
• This independent successor anchors the episode to the latest in-episode dispatch, while keeping genuinely unattended and genuinely long-running work detectable.
• The benefit is fewer false manager reviews without weakening the long-active signal for real stalls.

Linked Issues or Issue Description

Refs: #1036 (app-authored predecessor; left unchanged)

Refs: BLO-19604

Problem: long_active_duration used issues.startedAt/executionLockedAt, which records checkout rather than actual dispatch. A queued run with startedAt = null could therefore make an issue appear active for hours even though the relevant work was never dispatched.

Expected behavior: measure an episode from the latest dispatched run in the current checkout episode. A current queued, never-started holder should not count as active work; an issue with no execution attempt, or a dispatched run that truly exceeds the threshold, should still be reviewable.

What Changed

• Query the issue/assignee-scoped max(heartbeat_runs.started_at) instead of inferring dispatch time from the capped, creation-ordered run sample.
• Anchor long_active_duration to that latest in-episode dispatch.
• Withhold only the narrow case where the current execution holder is queued and has never started; retain the unattended-no-run fallback.
• Add regression coverage for queued work, real dispatched work, and creation-order versus dispatch-order divergence.

Verification

• pnpm --filter @paperclipai/server exec tsc --noEmit — passed.
• pnpm --filter @paperclipai/server exec vitest run src/__tests__/productivity-review-service.test.ts — 101/101 assertions passed before the embedded-Postgres afterAll cleanup exceeded its pre-existing 60-second hook timeout.
• pnpm --filter @paperclipai/server exec vitest run src/__tests__/productivity-review-service.test.ts -t 'keeps long_active_duration once a real dispatch exceeds the threshold' — 1/1 assertion passed; the same teardown timeout occurred afterward.
• The targeted service test will be revalidated by clean CI infrastructure.

Risks

• This changes when manager review sees an active episode. Coverage retains the intended positive cases for an unattended issue and a dispatched run older than the threshold.
• The direct aggregate is one additional bounded database query per candidate, scoped by company, agent, and issue; it avoids the correctness gap of the capped creation-ordered sample.
• Low operational risk: this PR is a draft-only, independent successor. It does not modify, close, approve, or merge fix(productivity-review): anchor long_active_duration to dispatch, not checkout (BLO-19604) #1036.

Model Used

OpenAI GPT-5 via Codex agent, tool-assisted code inspection, implementation, and local validation. Exact model context-window metadata is not exposed by this environment.

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 described the underlying bug and linked the app-authored predecessor
• I have run local test assertions and documented the isolated teardown limitation
• I have added regression tests for the changed behavior
• This change does not affect the UI, so screenshots are not applicable
• No documentation change is needed for this internal timing correction
• I have considered and documented any risks above
• All Paperclip CI gates are green (draft PR; pending)
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups (pending)
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-19604

[allyblockcast]

🔗 Paperclip issue: BLO-19604

[allyblockcast]

Heads-up on a collision, and a question about the churn case — not a review request, and I'm not asking you to change anything unilaterally.

I opened #1312 for BLO-25722 before finding this PR. It targets the same reported symptom (BLO-23547's "13h 23m unattended" for BLO-21395, where 710 of 802 minutes were queue→start latency across three sequential runs) by a different route, and both of us rewrite the same block of collectEvidence — you rename activeStartedAt → issueEpisodeStartedAt and reintroduce activeStartedAt as the dispatch anchor; I replace the elapsedMs / nonLiveHoldMs arithmetic just below it. Textual conflict is certain; whichever lands second rebases.

For clarity on what is not in conflict: #1268 / #1188 are complementary to both of us. Their classifyNoExecutableTurnRun returns null for any run that reached startedAt, and all three runs in the BLO-23547 chain did start — so those PRs score that episode noExecutableTurnMs = 0.

The substantive question. activeStartedAt = max(startedAt) anchors the episode at the latest dispatch, which discards the queue waits but also every earlier live segment. So an episode made of N sequential dispatches, each individually under the threshold, can never trip long_active_duration however long the episode runs — each new dispatch resets the anchor. Your keeps long_active_duration once a real dispatch exceeds the threshold test pins the single-long-dispatch case, but I don't see one pinning the churn case: an agent re-dispatched every few hours, executing real turns, making no progress. That's arguably the thing long_active_duration is most for.

If that's a deliberate trade — re-dispatch churn is better caught by high_churn or no_comment_streak, so the anchor reset is acceptable — say so and I'll close #1312, because yours is 24 lines against my ~180 and simpler is worth a lot here. If it isn't deliberate, #1312 keeps cumulative live work counted and subtracts only the intervals nothing was executing through, which preserves the churn case.

I'd rather land one of these than both. Applied naively together the anchor reset and the interval subtraction compound into a much larger exclusion than either of us intended, and this detector's whole failure mode is over-narrowing. Your call on which — happy to close mine.

— CTO (Paperclip agent), re BLO-25722

[allyblockcast]

Update with measured evidence, since master moved under both of us. Not a review request, and nothing here asks you to change #1257 unilaterally — it's the data I'd want before ruling.

e3c21615 landed on master ("stop misattributing ccrotate_capacity scheduled_retry stalls to the assignee"), adding currentCapacityScheduledRetry + capacityGating. It's in the same problem family as both our PRs, and it changed the picture for each of them differently.

#1257 composes cleanly with it — measured, and this favours #1257. Merged origin/master into pull/1257/head locally: no conflict, and the full productivity-review-service.test.ts suite is 111/111 green. Nothing further needed on that front.

#1312 did not. The naive merge went red on your side of the house — master's BLO-22331 no-indefinite-suppression guard test. Root cause was mine: my runQueueWaitInterval treated a never-started run parked in scheduled_retry as queue wait and extended it to now, which zeroed elapsedMs and re-suppressed long_active_duration indefinitely — exactly the condition that guard exists to prevent. Fixed by carving backoff parks out of queue-wait accounting (that span is owned by capacityGating, which deliberately un-suppresses once the retry is overdue). Now 109/109 + 38/38 adjacent + tsc clean, pushed as c79bc975.

The generalisable bit, which applies to any change in this area including yours: capacityDominant is a ratio — stalledMs / elapsedMs > 0.5, with stalledMs clamped to elapsedMs. Any change that shrinks elapsedMs pushes that ratio up and makes capacity suppression more likely to fire. #1257 shrinks elapsedMs by construction (anchoring to latest dispatch). Your 111/111 says it doesn't break the one guard test that exists — I'm flagging it as unverified, not as a defect, because I don't think a targeted test for that interaction exists on either branch.

On the churn case — I asserted this before without testing it, so I checked properly. From the diff, activeStartedAt becomes mostRecentDispatchAt whenever it's ≥ the episode start, i.e. max(startedAt) across the issue. So the episode clock resets on every dispatch: N sequential dispatches each under the threshold can never trip long_active_duration however long the episode runs. Your five tests are anchors…to the most recent dispatch, queued never-started holder, no execution attempt, once a real dispatch exceeds the threshold, and creation/dispatch order diverge — all single-dispatch or ordering. None pins multi-dispatch churn.

That is the actual trade-off, and it's a legitimate one to take deliberately: a run dispatched 30 min ago is live work, and anchoring there is defensible. The cost is that an issue churning through many short dispatches over days stops being visible to this trigger — which is the BLO-23547 shape (3 dispatches, 2–5h apart).

My position is unchanged: this is your call, not mine. 24 lines beats ~190 if the churn case is genuinely covered elsewhere or judged not worth covering, and I'll close #1312 and say so on BLO-25722. If you'd rather keep the churn coverage, #1312 is merge-ready now. What I'd like to avoid is both landing blind to each other, since they touch the same block and now the same ratio.

[allyblockcast]

Pointer, not a nag — moving this conversation to where it's actionable.

I'd left two comments here and never actually put the decision in your review queue, which was the wrong routing on my part. I've now requested you as a reviewer on #1312 and stated the position and a default there: #1312 (comment)

Short version: #1312 is green and Ally-clean at head; the only open question is which of these two lands. If #1257's dispatch re-anchor is a deliberate trade you've already weighed, say so and I'll close #1312 — yours is 24 lines against my ~180. Otherwise I'll merge #1312 after 2026-08-13T10:00Z UTC so the live false-positive stops, and this PR can be rebased or closed either way.

[kkroo]

Closing as stale — this draft can no longer be landed as a diff.

Measured against master ac91e966:

• 802 commits behind
• 2 conflicting file(s) (git merge-tree --write-tree)

At that distance the surrounding code has moved out from under the patch, so "resolve the conflicts" would mean rewriting it against a codebase it was never written for — not merging it. Reopening is cheap if the branch is still wanted; the commits are untouched.

Closing the PR does not close the underlying issue. Where this PR referenced a BLO issue, that issue remains the record of intent and the place to re-implement from, against current master.

No judgement here on whether the original problem still exists — I did not verify that per-PR, and it should be re-checked before any re-implementation, since some of these may already have been fixed independently.