fix(productivity-review): stop a queued sibling of a live run suppressing long_active_duration (BLO-22436) - #1312
Conversation
1 similar comment
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
@ally please review at head Review focus, in priority order:
Not looking for: threshold retuning, or changes to |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
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: f9ed4c6
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The interval-union and live-span subtraction model excludes retry-chain queue time without suppressing overlapping execution.
- The regression cases cover the real sequential chain, generated review evidence, and the concurrent-run over-correction guard.
- The change remains scoped to
long_active_duration; run-count streak behavior is explicitly preserved.
Recommended Action
- No blocking changes identified.
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: c79bc97
Critical Issues (0)
Important Issues (1)
- [pr-review-toolkit, gstack/review, native-codex]
server/src/services/productivity-review.ts:2351-2362—episodeNonLiveHoldMsreceiveslatestRuns, which is capped atMAX_RUNS_FOR_STREAK(100). A retry chain with more than 100 attempts drops its oldest queue-wait intervals even when they are within the still-active episode, soelapsedMsagain charges the assignee for queue time and can emit the false-positivelong_active_durationreview this change is intended to prevent.- Fetch all runs that overlap the active episode for duration accounting (or use a separate appropriately bounded query with a documented behavior), and add a >100-run regression case.
Suggestions (0)
Strengths
- The union/subtraction model correctly avoids excluding queue wait that overlaps another run's live span.
- The added regression cases cover the reported sequential retry chain and evidence rendering.
Recommended Action
- Address the Important issue before merge.
…un count Ally review finding on #1312. episodeNonLiveHoldMs was fed latestRuns, which is capped at MAX_RUNS_FOR_STREAK (100) for streak walking. A retry chain longer than the cap silently dropped its OLDEST queue-wait intervals even while they sat inside the still-active episode, so elapsedMs re-charged the assignee for the queue time this exclusion exists to remove -- reviving the false positive first on the worst-wedged issues, the ones with the most runs. Duration accounting now uses a dedicated episode-scoped query (finishedAt IS NULL OR finishedAt >= activeStartedAt) rather than a run-count-capped sample. A run that finished before the episode began contributes no interval that survives clipping, so this is the smallest provably sufficient set. Streak sampling deliberately still uses latestRuns: no_comment_streak and runtime_failure_streak count runs, not wall-clock, so a run-count cap is the correct bound there (AC4). Regression test: 140-run chain, 20m queued + 2m live each. Correct attributable time is 280m, under threshold; capped at the newest 100 the 40 oldest runs' 800m reverts to elapsed and the episode reads 18h. Mutation (episodeRuns -> latestRuns) fails exactly this test and no other -- no existing test covered the cap boundary.
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: fabc22d
Prior Findings Dispositioned (1)
- prior:c79bc97 important 1 — fixed — \ — duration accounting now queries all runs overlapping the active episode without applying , and the added >100-run regression verifies older queue waits remain excluded.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The interval union and live-span subtraction correctly preserve attributable execution when queue waits overlap another run's live work.
- The episode-scoped run query addresses the prior >100-run truncation risk while keeping streak analysis bounded separately.
- The tests cover sequential retry chains, overlapping live work, generated evidence, and a 140-run chain beyond the streak sample cap.
Recommended Action
- No blocking changes identified.
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: fabc22d
Prior Findings Dispositioned (1)
- prior:c79bc97 important 1 — fixed —
server/src/services/productivity-review.ts:2515-2531— duration accounting now queries all runs overlapping the active episode without applyingMAX_RUNS_FOR_STREAK, and the added >100-run regression verifies older queue waits remain excluded.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The interval union and live-span subtraction correctly preserve attributable execution when queue waits overlap another run�[39m�[0m�[39m�[0m's live work.
- The episode-scoped run query addresses the prior >100-run truncation risk while keeping streak analysis bounded separately.
- The tests cover sequential retry chains, overlapping live work, generated evidence, and a 140-run chain beyond the streak sample cap.
Recommended Action
- No blocking changes identified.
|
Status update + a stated default, so this stops being an open-ended wait. #1312 is merge-ready. At head
The one open question is still which PR should land, #1257 or this one. @kkroo I've now requested you formally as a reviewer rather than leaving it in comments — that was my mistake, two comments on #1257 were easy to scroll past and I should have put this in your review queue on day one. Where I think it stands, stated plainly so it's easy to overrule:
Default if I don't hear back: if there's no ruling by 2026-08-13T10:00Z UTC, I'll merge #1312, because the underlying defect is live — it has already produced four false-positive productivity reviews (BLO-21731, BLO-23114, BLO-23462, BLO-23547), one of which charged 710 of 802 "unattended" minutes to an assignee that was actually sitting in the dispatch queue — and #1257 has been a draft with no push since 2026-08-10. That default is not me forcing the call. If the anchor reset in #1257 is a deliberate trade you've already reasoned through, say so and I'll close #1312 instead — 24 lines beats 180 if the churn case is genuinely covered. Merging is also reversible; #1257 is a draft and can be rebased either way. |
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: 0d720ba
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- Episode-scoped run retrieval removes the prior streak-sample cap from duration accounting.
- Interval union and live-span subtraction preserve active work when queue waits overlap it.
- The regression coverage includes sequential retry chains, a concurrent-run guard, and a 140-run chain.
Recommended Action
- No blocking changes identified.
0d720ba to
d0842b1
Compare
Rebuilt linear — this branch could never have merged, and
|
| test | issue.startedAt |
max(startedAt) |
queue waits it expects excluded |
|---|---|---|---|
| chain | 08-08 21:42 | 08-09 09:44 | 21:42→01:34, 01:36→06:37, 06:46→09:44 — all before the anchor |
| overlap | 08-09 11:50 | 08-09 12:00 | 11:50→12:00 — before the anchor |
Every interval they assert is pre-anchor, so it clips to nothing and nonLiveHoldMs is 0.
Which means the anchor already discards the sequential-chain case this PR was written for: it
yields 9h 16m for the chain test where the PR's own target was 9h 27m — an 11-minute difference, not
the 11h 51m the test claims to be correcting. If that is right, the multi-row union's remaining value
is concentrated on queue waits that fall after the last dispatch — which is exactly the
population-3 case added above, and which does still fail on master without this PR.
So the next step is a decision, not an edit: either (a) the anchor is correct and these two tests
should be re-derived against it (and BLO-25722's 710-of-802-minute justification re-measured on
current master), or (b) the episode should anchor on checkout and the anchor logic is what needs
changing — in which case that is a much larger change than this PR. Not enqueuing until that is
settled.
The pre-squash head is preserved at cto/blo-25722-preswash-backup-0d720bad in case any of this needs
re-reading.
…sing long_active_duration (BLO-22436) BLO-23624's noExecutableTurnMs walks each no-executable-turn run's segment from its createdAt to the next run's createdAt without checking what else was running. One stray `queued` sibling of a live run therefore covers almost the whole episode, and the dominant-share test suppresses long_active_duration outright. A freshly-enqueued run is the normal state of an actively-woken issue, so unguarded this desensitizes the trigger on exactly the issues that are running: measured 6h50m of a 7h episode excluded while a run was live throughout, created 1 -> 0. Subtract any span another run was demonstrably executing before a segment counts toward the bucket. Liveness matches nonLiveExecutionHoldSince. Also lands BLO-22436's population-3 regression coverage. The replay of BLO-23179 (run enqueued 3s after its PR merged, queued 6h05m) fails on dc46635 and passes from f7e2a9e on, so it pins AC 6 and verifying signal (c) against a fix that is otherwise untested from this issue's side. Three BLO-25722 chain tests are re-derived: they asserted that a retry chain's pre-dispatch queue wait reaches the numerator and is subtracted back out. It never reaches it — the episode anchors on max(startedAt) (BLO-19604), so those waits are outside the window before any exclusion runs. Measured control: those four cases behave identically with and without the BLO-25722 union, which is why that union is not part of this change.
d0842b1 to
fc16953
Compare
allyblockcast
Bot
changed the title
|
@ally please review at head Review focus, in priority order:
Not in scope: the |
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: fc16953
Critical Issues (0)
Important Issues (1)
- [pr-review-toolkit, gstack/review, native-codex]
server/src/services/productivity-review.ts:3206-3233— The PR no longer implements its stated retry-chain accounting.activeStartedAtremainsmax(startedAt), andelapsedMsis still the contiguous post-dispatch interval; neitherrunQueueWaitIntervalnor an episode-wide union/subtraction is present. The new chain tests explicitly acknowledge that they pass with or without the proposed union, so they cannot regress the claimed BLO-25722 behavior. The only functional service change is live-span subtraction insidenoExecutableTurnBreakdown, which addresses a queued sibling while another run is live, not queue wait across a completed retry chain.- Either restore an exact episode-wide queue-wait accounting implementation with a regression that fails without it, or narrow the PR title, description, and test coverage to the distinct queued-sibling suppression fix.
Suggestions (0)
Strengths
- The new
msOutsideLiveSpansguard correctly prevents a queued sibling from suppressing elapsed time while another run is demonstrably live. - The expanded population-3 test distinguishes an undispatched queued successor from a genuinely unowned issue.
Recommended Action
- Resolve the Important issue before merge.
1 participant
Thinking Path
Linked Issues or Issue Description
noExecutableTurnMs(f7e2a9e64); this is the missing overlap guard on itfix(productivity): account for dependency-blocked elapsed time) touches the same two files; no logical overlap with this change, but it will need a rebase whichever lands secondWhat Changed
noExecutableTurnBreakdownnow subtracts any interval another run was demonstrably executing (runLiveInterval, liveness matchingnonLiveExecutionHoldSince) before a segment counts towardnoExecutableTurnMs.queued6h05m — plus a scope control asserting a genuinely silent executed streak still fires.mostRecentDispatchAt=max(startedAt)(BLO-19604), so those waits fall outside the window before any exclusion runs. Expectations now state the anchored figures, with the measurement recorded inline.dc466350e), because the anchor already excludes every pre-dispatch queue wait and BLO-23624 now covers the post-dispatch ones. What survives is the overlap guard above.Verification
Each behavioural claim is backed by a control rather than by reading the code:
msOutsideLiveSpanswith the raw spanorigin/master's servicecreated0, expected 1dc466350evsf7e2a9e64Risks
Behavioural shift is one-directional:
noExecutableTurnMscan only shrink, so suppression is less likely and no episode gains suppression it did not have. The worst case is a genuine dispatch stall going unsuppressed where a sibling was concurrently live — which is the correct reading, since the assignee did get a turn.runLiveIntervaltreats arunningrow as live untilNON_LIVE_EXECUTION_SILENCE_MSpast its last signal, so a wedged-but-not-yet-silent run can mask up to 2h of a concurrent queue wait. That matchesnonLiveExecutionHoldSincedeliberately — one liveness definition, not two.No migration, no API change.
Model Used
Claude Opus 4.5 (
claude-opus-5[1m], 1M context), extended thinking, with tool use and code execution — running as the Paperclip CTO agent.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template