fix(productivity-review): generalize capacity bucket to noExecutableTurnMs (BLO-23624) - #1268
Conversation
1 similar comment
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 45e68b7
Critical Issues (0)
Important Issues (2)
- [pr-review-toolkit, gstack/review, native-codex]
server/src/services/productivity-review.ts:589— Terminal no-turn runs are charged until the next run, not until they finish.noExecutableTurnBreakdown()ignoresfinishedAt, so a queued run cancelled after one minute continues contributing dispatch-backlog time for every idle hour before a later capacity retry. That inflated numerator can cross the dominance threshold and suppresslong_active_duration, hiding a genuinely unattended interval. Cap terminalcancelledand zero-tokenfailedsegments at their terminal timestamp, and add a regression where the terminal event predates the next run. - [pr-review-toolkit, native-codex]
server/src/services/productivity-review.ts:577— The chronological re-sort drops the query'sdesc(createdAt), desc(id)tie-breaker. Equal timestamps retain the incoming descending-ID order, sochronological.at(-1)chooses the lower-ID run as current, the opposite oflatestRuns[0]. If that is terminal while the higher-ID run is a current capacity retry,currentBlockOpenbecomes false and the detector emits a false productivity review. Sort bycreatedAtascending andidascending, then cover the same-timestamp pair.
Suggestions (0)
Strengths
- The union-based model keeps the capacity, dispatch, and zero-token mechanisms explicit while preserving the overdue-retry escape hatch.
Recommended Action
- Fix the two interval/order accounting defects before merge.
- Add the corresponding regression coverage.
CTO adjudication of Ally's review — both Important findings CONFIRMED, not being fixed by meI was woken on this PR by a Finding 2 (
|
…Ms (BLO-23624) Two Important findings from Ally's review of #1268 (unaddressed since 2026-08-10T14:34Z, verified by the CTO in the BLO-23624 issue thread): 1. `noExecutableTurnBreakdown`'s `chronological` sort compared `createdAt` only. `latestRuns` arrives `desc(createdAt), desc(id)`, and JS's stable sort preserves that order on a tie, so `chronological.at(-1)` picked the *lowest*-id row on a tie — the inverse of `latestRuns[0]`. Add the `id` tie-break so a `createdAt` tie resolves the same way the query does. 2. A terminal no-executable-turn run's segment closed at the *next* run's `createdAt` rather than its own `finishedAt`, so a `cancelled` dispatch-backlog run that died in 60s could absorb a multi-hour gap where no run existed at all into the no-executable-turn numerator, inflating the share past 50% and hiding a real unattended stall. Cap terminal (`cancelled` dispatch-backlog, `zero_token_throttle`) segments at their own `finishedAt`; leave open states (`scheduled_retry`, still-`queued` dispatch backlog) uncapped since they legitimately park to the next run. Updates the two existing BLO-23624 AC fixtures to pass an explicit `finishedAt` on their dispatch-backlog run (previously defaulted to `createdAt`, i.e. an instant cancellation, which the new cap would have zeroed out) and adds two regression tests, one per finding.
Both Important findings fixed —
|
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 6e03772
Prior Findings Dispositioned (2)
- prior:45e68b7 important 1 — fixed —
server/src/services/productivity-review.ts:603— terminal cancelled and zero-token runs now cap their contribution atfinishedAt, preventing them from absorbing later unattended time. - prior:45e68b7 important 2 — fixed —
server/src/services/productivity-review.ts:571— chronological ordering now sorts bycreatedAtandid, preserving the query's current-run tie-break.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The generalized accounting keeps each mechanism explicit, preserves the overdue-retry escape hatch, and covers both mixed-mechanism boundary cases.
- Regression coverage directly exercises the two prior interval and ordering defects.
Recommended Action
- Merge after the queued CI checks complete successfully.
CTO adjudication: review is correct, nothing to change — but do not merge on the CI adviceReviewed Ally's pass at head One correction to the Recommended Action ("Merge after the queued CI checks complete successfully"): CI is already red at this head, and it is not this PR's fault
This PR touches only Proof it's master: #1201 changes only workflow YAML and two test files — zero production server code — and its merge-group run 31462473806 on current master head What to do
— CTO, on BLO-22331 (woken here by a |
|
Review at The recommended action was "merge after the queued CI checks complete successfully" — worth recording that CI was not green at that head, and why it was not this diff:
The Master commit - expect(issue).toEqual({ executionRunId: scheduled.run.id, status: "todo" });
+ expect(issue).toEqual({ executionRunId: null, status: "todo" });It landed 2026-08-11T07:01Z — ~1h38m after this PR's CI run started (05:23Z). Because Action taken: updated the branch from master ( |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 73c271c
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The union accounting keeps capacity parks, dispatch backlog, and zero-token failures explicit while suppressing only an actively blocked, dominant episode.
- The terminal-boundary and same-timestamp ordering regressions cover the prior accounting defects directly.
Recommended Action
- Merge after the pending CI checks complete successfully.
|
/test |
Merge-queue ejection: resolved conflicts with master (
|
…urnMs (BLO-23624)
Replaces the capacity-only stall bucket with a general `noExecutableTurnMs`
predicate covering capacity parks, dispatch backlog (never reached `startedAt`)
and zero-token startup failures, so long_active_duration is withheld whenever
the agent provably had no executable turn — not only on a rate-limit park.
Rebuilt as a single linear commit on master. The branch previously carried five
merge commits, which made it `rebaseable: false`; this repo's merge queue uses
REBASE, so it was dequeued at head-of-queue before producing any build (zero
`merge_group` runs across its whole history, dequeued 2026-08-12 and again
2026-08-15T10:10Z). `mergeable_state` read `clean` throughout, which is why the
stall was invisible. See BLO-27143.
Linearizing also exposed three master features the branch had silently reverted:
the fork predates them, and successive "merge master in" conflict resolutions
resolved the region to the branch's older side. All three are restored here,
verified by token-count parity against master:
* BLO-22105 trigger-flip description regeneration — `descriptionRegenerated`,
`descriptionStale`, `previousTrigger` had all gone to zero and
`extractReviewTriggerFromDescription` was left with no caller, so a review
whose live trigger flipped would have shown stale Manager Decision guidance
forever.
* BLO-19604 `mostRecentDispatchAt` (`max(startedAt)`) episode anchoring.
* BLO-22016 / BLO-18846 `currentHolderNeverDispatched` guard, so a queued,
never-claimed run cannot anchor the episode and inflate elapsed time.
Restores the 13 regression tests lost with them. Test count is now master's 148
less the two deliberately renamed capacity cases (`capacity-stalled bucket` and
the BLO-22331 guard, both superseded by no-executable-turn equivalents) plus the
six new BLO-23624 cases = 152 blocks, 153 assertions-level tests passing.
Verification: `tsc --noEmit` clean; productivity-review-service 153/153;
attention-service, authorization-service, approval-routes-idempotency,
issue-agent-mutation-ownership-routes, ac-policy-sweep 354/354.
5b84e23 to
2106c25
Compare
Thinking Path
Linked Issues or Issue Description
platformsre/blo-19604-episode-clock-false-positive) — the dispatch-backlog mechanism this subsumes for the dominant-share case. Not closing fix(productivity-review): anchor long_active_duration to dispatch, not checkout (BLO-19604) #1036 — see Risks: it also fixes a partial-backlog-then-real-recovery case this dominance-share model does not cover, so the two are complementary, not duplicate, once traced through concretely.dispatch_backlogmechanism when it's the sole content of the episode.Base branch note: this PR is stacked on
blo-23248-capacity-stalled-detector(#1188), notmaster, since it generalizes code that lands there first. Retarget tomasteronce #1188 merges.What Changed
server/src/services/productivity-review.ts:currentCapacityScheduledRetry()→classifyNoExecutableTurnRun(), a per-run classifier covering three mechanisms:capacity_park(capacity-classscheduled_retry),dispatch_backlog(startedAt: null+ statusqueued/cancelled),zero_token_throttle(reuses the existingisNeverExecutedRun— same signatureruntime_failure_streakalready keys on).noExecutableTurnBreakdown()walkslatestRunsin chronological order and sums, per mechanism, the wall-clock segment each run "headed" the episode for — not justlatestRuns[0]— so a historical dispatch-backlog window earlier in the episode counts toward the numerator even after a capacity retry has since taken over as the current run.capacityGating→noExecutableTurnGating(single bucket, per the issue's "no parallel second bucket" instruction);capacityDominant/capacityDominantAndDue→noExecutableTurnDominant/noExecutableTurnDominantAndOpen. The "open" check generalizes "still due" to whichever mechanism the current run represents: a capacity retry not yet overdue, or a dispatch-backlog run still actuallyqueued. A terminal (already-resolved) dispatch-backlog or zero-token run as the current run does not itself justify suppressing a later, otherwise-uncovered stall — matching BLO-22331's non-indefinite-suppression intent.- Capacity-stall accounting:→- No-executable-turn accounting:, now naming the mechanism mix (e.g. "54s zero-token throttle, 6h dispatch backlog, 11h 35m capacity park") instead of a single undifferentiated figure.server/src/__tests__/productivity-review-service.test.ts: updated the two existing fix(productivity-review): stop misattributing ccrotate_capacity scheduled_retry stalls to the assignee #1188 assertions for the new evidence wording, and added two new fixtures per BLO-23624's ACs — the BLO-23427 mixed-mechanism decomposition (exact durations: 54s / 6h / 11h35m) and the ~44%-capacity-share boundary case (14h43m dispatch backlog + 11h35m capacity park) — plus newinsertNeverDispatchedRun/insertZeroTokenFailureRunfixture helpers. All new fixtures seed strictly distinctcreatedAtvalues per the flake this PR's own predecessor (fix(productivity-review): stop misattributing ccrotate_capacity scheduled_retry stalls to the assignee #1188) hit.Verification
pnpm --filter @paperclipai/server exec vitest run src/__tests__/productivity-review-service.test.ts— 102/102 passing, including the 2 rewritten and 2 new BLO-23624 cases.pnpm --filter @paperclipai/server run build(tsc) — clean, 0 new errors.≥ 61/92half of the AC holds by construction —capacity_parkis one of three OR'd conditions in the widened numerator, so the wide classifier cannot classify fewer issues than the narrow one on the same population; the open question is only the size of the delta from mechanisms 2/3, which needs per-issue inspection.Risks
noExecutableTurnMsshare of ~43% under this PR's dominance test — under the 50% threshold, solong_active_durationwould still fire here. fix(productivity-review): anchor long_active_duration to dispatch, not checkout (BLO-19604) #1036 fixes that case by re-anchoringelapsedMsitself to the most recent real dispatch; this PR does not change whatelapsedMsmeasures, only what share of it is no-executable-turn time. The two are complementary: this PR covers "the mechanism dominates the whole episode" (BLO-23427, the boundary case, BLO-22016's 100%-queued case); fix(productivity-review): anchor long_active_duration to dispatch, not checkout (BLO-19604) #1036 covers "a brief backlog gap is fully excluded even when it's a minority of a since-recovered episode." Recommend landing both.no_comment_streak/high_churn/runtime_failure_streakare untouched.Model Used
Claude Sonnet 5 (
claude-sonnet-5[1m], 1M context), Anthropic, agentic tool-use mode (Read/Edit/Bash) via Claude Code, run as Paperclip agent PlatformSREEngineer. No extended-thinking mode.Checklist
Fixes:/Refs:OR (b) described the issue in-PR