fix(productivity-review): exclude dependency-gate cancellations from streak, skip blocked issues (BLO-22436) - #1158
Conversation
|
🔗 Paperclip issue: BLO-22436 |
1 similar comment
|
🔗 Paperclip issue: BLO-22436 |
![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: da3cae1
The core change is sound and well-argued. The isInfraFailureRun / isDependencyBlockedRun split is genuinely disjoint (productivity-review.ts:752 short-circuits the infra predicate), so the BLO-21769 population is behaviourally untouched, and the new gate's predicate (unresolvedBlockerCount > 0, productivity-review.ts:2083) is the same predicate the dispatch gate uses at heartbeat.ts:15098 — the exemption tracks the cancellation exactly, which is the right invariant. Two findings below concern what the gate does to state that already exists, and where it was placed.
Critical Issues (0)
None.
Important Issues (2)
-
[gstack/review]
server/src/services/productivity-review.ts:2083— An issue that becomes dependency-blocked after a review was already minted strands that review open indefinitely: it is never refreshed and never closed.- The early
return nullmakescollectEvidenceskip the candidate beforecreateOrUpdateReview(productivity-review.ts:3259), which is the only path that refreshes an open review. There is no counterpart incloseOpenSuppressedReviews(productivity-review.ts:1785), which today auto-closes open reviews for the two other suppression classes and reports them asclosedSuppressedMonitorReviews/closedTerminalSourceReviews. - This lands squarely on the loop the PR is closing. BLO-20815's remediation for a flagged review is to model the fault as a
blockedByedge — so the documented remedy now leaves a stale open review pointing at an assignee who provably cannot act on it, instead of closing it as no-longer-applicable. Generation is fixed; the already-generated review is not. - Recommendation: add a
dependencyBlockedbranch tocloseOpenSuppressedReviewsalongside the existing monitor-scheduled/terminal-source branches, with its own result counter, so a blocked source closes its open review rather than freezing it.
- The early
-
[native-codex]
server/src/services/productivity-review.ts:2078— Placing the gate insidecollectEvidencealso disables the continuation-hold enforcement path, which is outside the PR's stated scope and untested.collectEvidencehas a second caller,isProductivityReviewContinuationHoldActive(productivity-review.ts:3346), which maps anullreturn toheld: false. That result is consumed atheartbeat.ts:11579, whereheldis what suppresses a liveness continuation and writes theissue.productivity_review_continuation_heldactivity record. So adding a blocker to an issue with an open soft-stop review now silently releases that hold.- The net outcome is mostly preserved — the continuation is enqueued and then cancelled by the dispatch gate — but it converts a clean hold into enqueue/cancel churn, drops the activity-log signal, and (per
heartbeat.ts:15098's!allowsIssueInteractionWake(context)carve-out) leaves a real hole for interaction wakes, which are dispatched even while blocked. It also adds alistDependencyReadinessquery to every continuation check, a hotter path than the per-scan-cycle cost the PR's Risks section accounts for. - Recommendation: scope the gate to the generation caller — either move it into the reconcile loop next to the other candidate filters, or gate it behind a
collectEvidenceoption thatisProductivityReviewContinuationHoldActivedoes not set. Whichever way it goes, state the intended hold behaviour explicitly and cover it with a test.
Suggestions (3)
- [pr-review-toolkit/tests]
server/src/services/productivity-review.ts:2084— The skip is folded into the genericskippedcounter, breaking the convention established byoptedOut,snoozed,monitorScheduledSuppressed,approvalGatedSuppressed,suppressedTerminalSourceandnoActionSuppressed(productivity-review.ts:3190). Given the ticket is about a loop that was invisible, not being able to measure how often the gate fires is a notable gap. It also weakens the new tests:expect(result.skipped).toBe(1)inproductivity-review-service.test.ts:536and:566passes for any skip reason, so neither test actually pins the mechanism under test. A dedicateddependencyBlockedSkippedcounter fixes both at once. - [pr-review-toolkit/types]
server/src/services/productivity-review.ts:768—dominantErrorCodebuckets everynullcode under the literal string"unknown", which is the same token BLO-21769 documents as a real observed error value (error: "unknown"), and ties resolve by first-insertion order. On a mixed window (say 2 infra + 2 dependency-gate) the rendered "dominant errorCode" is decided by run ordering and can read as a definite diagnosis. Consider emitting the count alongside the winner, or omitting the parenthetical when no code holds a strict majority. - [gstack/review]
server/src/services/productivity-review.ts:2136— Asymmetric treatment of the same population: dependency-blocked runs are transparent tonoCommentStreak(filtered out at:2139so they neither extend nor break it) but opaque toruntimeFailureStreak(they break the walk at:2136). Not a regression — pre-PR they broke it too — but it means a genuine infra-failure streak is masked as soon as newer dependency-gate cancellations land on top, which is exactly the BLO-20815 ordering. Worth a comment stating the choice is deliberate, or filteringterminalRunsfor that walk too.
Strengths
- The gate predicate is deliberately identical to the dispatch gate's (
heartbeat.ts:15098), so exemption and cancellation cannot drift apart — the right thing to key on, and worth the extra query. - Splitting the predicate rather than widening it keeps BLO-21769 provably intact:
isInfraFailureRunshort-circuits on the new population first, so the existing positive-control tests pass unmodified. latestRunsuses an unprojecteddb.select()(productivity-review.ts:2089), so the newly-readerrorCodecolumn is genuinely present at runtime — and test 1 is a real end-to-end proof of that, since it seeds no blocker and would fail iferrorCodewere absent.- Comments explain why the two zero-token populations are disjoint rather than restating the code, and each carries its ticket reference.
- Test 1's 10-minute run spacing to avoid incidentally tripping
high_churnis a thoughtful piece of test isolation, and the reason is written down.
Recommended Action
- No Critical issues — nothing blocks on correctness of the streak logic itself.
- Address the two Important issues this cycle: add the dependency-blocked close path to
closeOpenSuppressedReviews, and scope the gate so it does not silently change continuation-hold enforcement. - Consider the Suggestions opportunistically; the dedicated counter is the highest-value one, since it also makes the two new tests assert the mechanism rather than a shared bucket.
…locked gate (BLO-22436) Ally's two Important findings on #1158, plus all three suggestions. - Scope the dependency-blocked exemption to review *generation*. It lived in `collectEvidence`, whose second caller `isProductivityReviewContinuationHoldActive` maps a null return to `held: false` — so adding a blocker to an issue under an active soft-stop hold silently released the hold: a clean hold became dispatch/cancel churn, the `issue.productivity_review_continuation_held` activity signal was dropped, and interaction wakes (which the dispatch gate lets through while blocked) were left uncovered. Now resolved once per company in the reconcile loop, which also replaces the per-candidate readiness query with one batched call. - Close, rather than strand, an already-open review whose source becomes dependency-blocked. `createOrUpdateReview` is the only path that refreshes an open review, so one minted before the blocker was added was never refreshed and never closed. That landed on the very loop this ticket closes: the documented remedy for a flagged platform fault is a `blockedBy` edge. Scoped to the triggers the gate causes (`no_comment_streak`, `long_active_duration`); `high_churn` is deliberately excluded so a flagged agent cannot retire its own cost-accountability artifact by adding an edge. The close re-checks the blocker edge at write time with a predicate strictly narrower than the batched read, so it fails closed. - Count the suppression under `dependencyBlockedSuppressed` / `closedDependencyBlockedReviews` rather than the generic `skipped` bucket, so the gate is measurable and the tests pin the mechanism instead of a shared counter. - Only name a "dominant errorCode" when one holds a strict majority, report its share, and stop folding a missing code into the literal "unknown" — a real observed value per BLO-21769. A plurality decided by run ordering read as a definite diagnosis. - Make dependency-gate cancellations transparent to the runtime-failure walk, symmetrically with `noCommentStreak`. Breaking on them asserted the runtime was healthy at a point where nothing was attempted, and masked a genuine infra streak behind newer cancellations — exactly BLO-20815's ordering. Verification: 105/105 in productivity-review-service.test.ts, tsc --noEmit clean. Both Important fixes were mutation-checked — reverting each one fails its new test with the expected assertion, so the coverage is load-bearing. Co-Authored-By: Claude <noreply@anthropic.com>
|
Addressed both Important findings and all three Suggestions in Important #1 — stranded open review. Added a Important #2 — continuation hold. Took your first option: the gate moved out of Suggestions:
Verification: 105/105 (was 100), Not yet done: the rebase. This PR reads |
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: 2793e6b
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- Correctly separates dependency-gate cancellations from infrastructure failures while preserving transparent streak walks.
- Batches dependency-readiness checks and re-validates mutable blocker edges before closing an existing review.
- Adds focused coverage for blocked sources, resolved blockers, review closure scope, dominant error-code reporting, and continuation holds.
Recommended Action
- No blocking changes identified.
|
Ally's follow-up review at head CI on this head shows This PR is queued behind #1135 and #1089 (both human-approved + green, ahead in the merge queue) per the stacked productivity-review fix series. #1135 had fallen out of the merge queue despite being clean/approved — re-enqueued it just now (position 14). Will rebase/re-verify #1158 once its predecessors land. |
2793e6b to
07abd0a
Compare
|
Rebased onto master (force-push: Context: this branch was The reconciliation is the only thing worth re-reviewing. #1089 and this PR both modify what used to be
Master's narrowed body now lives in The two test suites are a clean union: 111 tests = my 105 + #1089's 6, no overlap, none dropped. I mutation-checked the merge rather than trusting green, since a silent collapse is the real risk in a reconciliation like this:
Both intents are load-bearing and neither was lost. One honest note: the explicit Pre-rebase head preserved at
|
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: 07abd0a
Prior Findings Dispositioned (2)
- prior:da3cae1 important 1 — fixed —
server/src/services/productivity-review.ts:1974-1992,2032-2079— open reviews triggered byno_comment_streakorlong_active_durationare now checked against batched dependency readiness, atomically rechecked before update, and closed when their source is dependency-blocked. - prior:da3cae1 important 2 — fixed —
server/src/services/productivity-review.ts:2260-2267,3509-3511— the dependency exemption is now confined to reconciliation candidate generation;collectEvidenceremains usable by continuation-hold evaluation, preserving an existing soft-stop hold after a source becomes blocked.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- Separates dependency-gate cancellations from infrastructure failures while keeping cancellation runs transparent to both streak walks.
- Batches dependency-readiness checks and revalidates mutable blocker edges at the close write.
- Covers blocked-source suppression, resolved blockers, existing-review closure scope, error-code summaries, and continuation holds.
Recommended Action
- No blocking changes identified.
07abd0a to
93bfbb6
Compare
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: 93bfbb6
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- Correctly separates dependency-gate cancellations from infrastructure failures without inflating either streak.
- Suppresses generation for currently blocked sources and exposes a dedicated suppression counter.
- Preserves continuation-hold behavior by scoping the exemption to reconciliation generation.
- Closes applicable pre-existing no-comment and long-active reviews when a source becomes dependency-blocked, with a write-time blocker recheck.
- Adds focused integration coverage for suppression, closure, dominant error-code reporting, streak behavior, and continuation holds.
Recommended Action
- No blocking changes identified.
- CI checks were still pending at review time; merge after the required checks complete successfully.
93bfbb6 to
c761f0d
Compare
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: c761f0d
The split itself is sound and I re-verified the load-bearing invariants at this head: isInfraFailureRun short-circuits on isDependencyBlockedRun (productivity-review.ts:1118) so BLO-21769's population is provably untouched; the write-time close predicate is conjunctive (and(...closePredicates), :2377) and mirrors listIssueDependencyReadinessMap's primary unresolved clause (issues.ts:1448) while omitting the finalize subcase, so it is strictly narrower and fails closed as the comment claims; latestRuns is an unprojected db.select() (:2601) so errorCode is genuinely present. Two findings below are about the reported evidence and the scope of the new gate, not the streak arithmetic.
Critical Issues (0)
None.
Important Issues (2)
-
[gstack/review]
server/src/services/productivity-review.ts:2703— Every real dependency-gate cancellation is counted twice in the evidence block, and the new tests structurally cannot catch it because they seed a run shape the gate never produces.heartbeatRuns.issueCommentStatusisnotNull().default("not_applicable")(packages/db/src/schema/heartbeat_runs.ts:50), andcancelQueuedRunForBlockedDependencies(heartbeat.ts:16363-16384) stamps onlystatus,error,errorCodeandresultJson— it never callsfinalizeIssueCommentPolicyand never writesissueCommentStatus. So in production every dependency-gate cancellation carriesnot_applicable.- That makes each such run match both
neverInvokedRunCount(:2689) and the newnonExecutingRunCount(:2703). The rendered block emits them as adjacent, independent-looking lines (:3247and:3250), so three cancellations render asNever-invoked runs excluded …: 3andNon-executing runs in sample window …: 3with nothing saying they are the same three runs. A manager reads six non-productive runs where there were three — which undercuts the stated purpose of the new line ("so a reviewing manager doesn't have to re-derive dispatch health from raw run telemetry",:2699). - The tests can't surface this:
insertRunsdefaultsissueCommentStatusto"retry_exhausted"(productivity-review-service.test.ts:265) and none of the four dependency-blocked seeds (:1226,:1292,:1339,:1384) override it, so every BLO-22436 test exercises anot_applicable-free variant that the dependency gate cannot emit. ThetoContainassertions still pass, which is precisely why the divergence is invisible. - Recommendation: seed
issueCommentStatus: "not_applicable"in the dependency-blocked tests so they model the gate's actual write, then make the two lines explicitly disjoint (or state the overlap inline, e.g.… 3 (all also counted as never-invoked)).
-
[native-codex]
server/src/services/productivity-review.ts:4166— The generation gate retireshigh_churnoversight on a blocked source, which is the exact evasion the PR's own close path refuses to allow, and it is unbounded in time.- The skip is unconditional and sits before any trigger evaluation (
:4166-4169,continueondependencyBlockedSourceIssueIds.has(candidate.id)), so while a blocker stands no review of any trigger can be minted. ButisDependencyBlockedClosableTrigger(:1032) deliberately excludeshigh_churnfrom closing, reasoning that honouring it "would let a flagged agent retire its own cost-accountability artifact by adding ablockedByedge" (:1025-1028). Adding that edge one cycle earlier achieves the same result — the artifact is never created — so the guard is bypassable by ordering. - This is reachable, not theoretical:
isEffectivelyDependencyReadyForDispatch(heartbeat.ts:5674-5680) explicitly lets issue-interaction wakes carrying a comment id dispatch on a blocked issue, "so a human can still talk to the assignee while it waits" (heartbeat.ts:5657-5659). Those runs execute, burn tokens, and are counted byhighChurn(:2968-2972), which is a raw 1h/6h run and comment count with no dependency or origin filter. Agents can also setblockedByIssueIdson themselves viapaperclipUpdateIssue. - The file already settles this exact question the other way for the analogous human gate: "Only
long_active_durationis suppressible by a human gate.no_comment_streakandhigh_churnstay live: an agent burning runs against a gate it cannot clear is exactly the waste worth reviewing" (:3004-3006) — and that suppression is bounded byapprovalGateMaxAgeMsand forward-only precisely "because the reviewed agent can create the approval itself" (:3007-3012). The new gate has neither the trigger scoping nor an age bound, so it erases oversight rather than delaying it. - Recommendation: scope the generation skip to the same trigger set the close path already trusts (
no_comment_streak,long_active_duration) sohigh_churnandruntime_failure_streakstill fire while blocked. That requires evaluating evidence before skipping rather than filtering the candidate, so if the per-candidate cost is the reason for the current placement, say so and bound the exemption by age instead.
- The skip is unconditional and sits before any trigger evaluation (
Suggestions (3)
- [pr-review-toolkit/comments]
server/src/services/productivity-review.ts:4124— The rationale comment asserts a blocked issue "cannot produce a run comment no matter how long it waits." That is the premise the whole gate rests on and it is overbroad in one specific way: interaction wakes are exempted from the dispatch gate (heartbeat.ts:5674-5680) and can comment. Worth narrowing the sentence to routine wakes and naming the interaction-wake carve-out, since a future reader checking the invariant will otherwise conclude it holds unconditionally. - [gstack/review]
server/src/services/productivity-review.ts:2334—dependencyBlockedSourceIssueIdsis keyed by source issue id but populated only from reviews whose trigger is closable (:2280), while the consult site checks membership for every review of that source without re-testing the trigger. Today that is safe only because the partial unique indexissues_active_productivity_review_uq(packages/db/src/schema/issues.ts:212) guarantees at most one active review per(companyId, originId)— an invariant enforced in another package with nothing at this call site expressing the dependency. A one-lineisDependencyBlockedClosableTrigger(trigger)re-check at:2334makes the block locally correct and cannot regress if that index is ever widened. - [pr-review-toolkit/comments]
server/src/services/productivity-review.ts:127— The BLO-26165 note still saysclassifyAndPersistRunLivenesssucceeding is "the axisisNeverExecutedRundepends on." After this PR that predicate is a union whose second arm keys onerrorCodeand needs no liveness classification at all (:1182-1186); the sentence now describesisInfraFailureRun. Cheap to retarget while the rename is fresh.
Strengths
- The write-time blocker recheck (
:2357-2375) is genuinely conjunctive with the batched read and deliberately narrower than it — I confirmed againstlistIssueDependencyReadinessMap(issues.ts:1444-1466) that omitting the workspace-finalize subcase can only withhold a close, never cause a wrong one. The comment explaining that is accurate rather than aspirational. - Cancelled blockers are treated as unresolved on both sides (
issues.ts:1449and thestatus <> 'done'clause), so the two paths agree on the one status that is easy to get wrong. - Making dependency-gate cancellations transparent to the runtime-failure walk rather than streak-breakers (
:2654-2659) is the right call and the reasoning — a cancelled-before-dispatch run is no evidence the runtime was healthy — is written down where the next reader needs it. dominantErrorCode(:1189-1213) requires a strict majority and buckets a missing code separately from the literal"unknown", which is a real observed value; the no-majority test at:1341-1370pins the behaviour rather than the happy path.- The dedicated
dependencyBlockedSuppressedcounter, and the test assertingskippedis0alongside it (:1305-1307), fixes the earlier problem of a shared bucket letting a test pass for the wrong reason. - Reviews follow-up work is visible and honest: the continuation-hold test (
:1373-1414) directly pins the behaviour the exemption placement is designed to preserve, and thehigh_churnnon-closure test (:1490-1520) locks in a fails-closed decision.
Recommended Action
- No Critical issues — the streak arithmetic and the close predicate are correct as written.
- Address the two Important issues this cycle: de-duplicate the never-invoked/non-executing evidence lines (and fix the test seed so the gate's real run shape is exercised), and decide explicitly whether
high_churnshould survive a blocker at generation time as it already does at close time. - Consider the Suggestions opportunistically; the
:2334trigger re-check is the highest-value one since it removes a cross-package invariant dependency. - CI is still pending at review time — merge only after the required checks land green.
…on gate to closable triggers (BLO-22436) Ally review on #1158 found two real gaps: - `cancelQueuedRunForBlockedDependencies` never stamps `issueCommentStatus`, so every dependency-gate cancellation defaults to `not_applicable` and was rendered in both the "never-invoked" and "non-executing" evidence lines with nothing marking them as the same runs. The evidence block now states the measured overlap instead of two independent-looking counts, and the BLO-22436 tests seed `issueCommentStatus: "not_applicable"` on their dependency-blocked runs to match the gate's actual write. - The generation-time skip for blocked issues fired unconditionally, regardless of trigger — unlike the close path's `isDependencyBlockedClosableTrigger`, which deliberately excludes `high_churn` because a blocker added after the fact doesn't undo runs that already executed and burned cost. The unconditional skip let a flagged agent retire its own cost-accountability artifact one cycle early just by adding a `blockedBy` edge. Generation is now scoped to the same trigger set the close path trusts, evaluated after evidence collection. Also tightens a stale comment (the BLO-26165 note described a predicate this PR split in two) and adds a defensive trigger re-check at the close predicate's consult site so it's locally correct independent of the uniqueness index it currently relies on.
|
Both Important findings checked out — pushed 37c1bd6 addressing them. Double-counted evidence ( Unconditional generation skip ( Also took the three Suggestions: retargeted the stale All 146 tests in |
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: 37c1bd6
This head is a direct response to the two findings on the previous one, and one of them is cleanly resolved. The other is narrowed rather than closed: the generation gate now consults a trigger, but it consults only the primary trigger, and no_comment_streak outranks high_churn in choosePrimaryTrigger — so the evasion still fires for the overlapping case, which is the common one. The PR's own two adjacent tests demonstrate it: they differ by a single withRunComments flag and disagree on whether the artifact survives.
Prior Findings Dispositioned (2)
- prior:c761f0d important 1 — fixed —
server/src/services/productivity-review.ts:2732— the overlap is now measured rather than assumed.nonExecutingAlsoNeverInvokedCountintersectsnonExecutingRunswithNEVER_INVOKED_ISSUE_COMMENT_STATUSover the sameterminalRunsbase thatneverInvokedRunCountuses (:2713-2716), so it is a genuine subset and the "already counted above" phrasing at:3282is sound in both the full-overlap and partial-overlap branches. All four dependency-blocked test seeds now carryissueCommentStatus: "not_applicable"(productivity-review-service.test.ts:1230,:1333,:1388,:1437), so they modelcancelQueuedRunForBlockedDependencies's actual write instead of a shape the gate cannot emit, and the assertions pin the new rendering (:1361,:1416,:1457). - prior:c761f0d important 2 — still-present —
server/src/services/productivity-review.ts:4232— see Important 1 below. The skip is now trigger-scoped, but scoped toevidence.trigger(the single primary trigger), andchoosePrimaryTriggerreturnsno_comment_streakbefore it ever reacheshigh_churn(:1022-1023). A blocked source that is both churning and silent therefore still has its cost-accountability artifact suppressed, which is the original finding's evasion path unchanged.
Critical Issues (0)
None.
Important Issues (1)
- [native-codex]
server/src/services/productivity-review.ts:4232— The generation gate keys on the primary trigger, sohigh_churnoversight is still erased wheneverno_comment_streakalso fires — and on a blocked issue that is the default shape.choosePrimaryTriggeris a priority ladder, not a set:runtime_failure_streak→no_comment_streak→high_churn→long_active_duration(:1021-1024). When an agent trips bothnoCommentandhighChurn,evidence.triggerisno_comment_streak, whichisDependencyBlockedClosableTriggeraccepts (:1046), so:4230-4236suppresses generation and the churn evidence goes with it —triggerReasonsis only ever carried on the artifact that is not created (:3025-3029).- The defaults make the overlap the norm, not a corner:
noCommentStreakRuns = 10andhighChurnHourly = 10(:47,:56), andinsertRunsspaces runs 60 s apart, so ten silent runs in ten minutes trip both predicates at once. The evasion from the original finding — add theblockedByedge before generation rather than after — is intact for exactly the agent worth reviewing: one burning runs and saying nothing. - The PR's own tests bracket this precisely.
productivity-review-service.test.ts:1247seeds 10 silent runs and assertscreated === 0/dependencyBlockedSuppressed === 1;:1284seeds 10 runs differing only bywithRunComments: trueand assertscreated === 1withPrimary trigger: `high_churn`. The new guard passes solely because that flag zeroes the no-comment streak and letshigh_churnreach the top of the ladder. That also leaves:1247's title — "skips an issue with an unresolved blocker regardless of streak" — asserting the behaviour this PR set out to remove. - Recommendation: evaluate the blocker against every trigger that fired, not the primary one — e.g. suppress only when
!highChurn && !runtimeFailure(the booleans are already in scope at:3002), or surface the fired set on the evidence and require all of it to be closable. Then flip:1247to seed a shape wherehigh_churngenuinely does not fire (fewer thanhighChurnHourlyruns, or widerspacingMs) so it tests dependency suppression rather than trigger precedence, and retitle it.
Suggestions (2)
- [pr-review-toolkit/comments]
server/src/services/productivity-review.ts:3386—buildRefreshCommentstill rendersNever-invoked runs excludedwith no counterpart fornonExecutingRunCountor the new overlap, so the refresh comment shows the one count this PR proved is ambiguous on its own and omits the one that disambiguates it. Low impact — the description is rewritten in full on refresh (:3550), so the authoritative artifact is current — but the comment is what lands in a manager's notifications. One line would keep the two surfaces telling the same story. - [gstack/review]
server/src/services/productivity-review.ts:3280— In the total-overlap case the line renders two adjacent parenthetical groups:… : 3 (3 already counted above as never-invoked) (dominant errorCode: …, 3 of 3). Accurate, and the tests pin it, but a single merged group would read better and would remove the3 of 3/3echo.
Strengths
- The de-duplication is done by measuring the intersection rather than asserting the two predicates are disjoint, and the type comment at
:259-267says exactly that — the right instinct, since the counts come from independent axes (errorCode/liveness vs.issueCommentStatus) that nothing constrains to agree. - Fixing the test seeds alongside the rendering is the part that makes the first finding actually closed: without
issueCommentStatus: "not_applicable"the assertions would have kept passing against a run shape the dependency gate can never produce. - Moving the generation gate after
collectEvidenceis safe as well as necessary — I checked the whole body (:2611-3260) for writes and it is read-only, so the reordering costs queries and changes no state. The comment at:4183-4186states that tradeoff rather than leaving it to be rediscovered. - The redundant
isDependencyBlockedClosableTrigger(trigger)re-check on the close path (:2358) is exactly the right kind of defensive check: the comment names the cross-package invariant it no longer depends on (issues_active_productivity_review_uq) instead of just asserting correctness. - The retargeted BLO-26165 note (
:126-129) correctly reassigns the liveness dependency toisInfraFailureRunand explains that the newerrorCodearm needs no liveness classification — a small fix, but it keeps a load-bearing comment true. - Comments consistently record why a boundary was drawn (the interaction-wake carve-out at
:4160-4166now names its own exception rather than overclaiming), which is what made this head reviewable against the last one.
Recommended Action
- No Critical issues — the streak arithmetic, the close predicate, and the new overlap accounting are correct as written.
- Address Important 1 this cycle: gate on the full fired-trigger set rather than the primary trigger, and re-seed
:1247so it exercises dependency suppression instead of trigger precedence. Until thenprior:c761f0d important 2remains open and this review is not an approval. - Consider the Suggestions opportunistically.
- CI is not green at this head, but not for code reasons: all three failing checks (
General tests (server 1/4),General tests (workspaces-a),Canary Dry Run) end inThe runner has received a shutdown signal … The operation was canceled, so the suites were cancelled mid-flight and the changed tests never ran. Re-run the workflow and confirm green before merge — these results are not evidence either way.
…streak, skip blocked issues (BLO-22436) The `no_comment_streak` detector counted runs that never executed toward an agent's silence streak. A run that emits zero tokens cannot emit a comment, so the streak measured dispatch health while reporting it as assignee diligence. Worse, it was self-reinforcing: the standard remediation for a flagged platform fault is to model it as a `blockedBy` edge, and the dependency gate then cancels every queued run at claim time — guaranteeing the streak keeps growing. That is exactly what happened between BLO-21723 and BLO-22262. - Split the zero-token population in two. `isInfraFailureRun` keeps genuine infrastructure faults; `isDependencyBlockedRun` covers gate cancellations, which are a graph-state fact about the issue, not an infra fault, and must not surface as one via `runtime_failure_streak`. `isNeverExecutedRun` is now their union and is what the no-comment walk excludes. - Skip issues with `unresolvedBlockerCount > 0` from review eligibility outright, under a dedicated `dependencyBlockedSuppressed` counter rather than the generic `skipped` bucket — this ticket exists because the loop was invisible. - Report non-executing runs separately in the review body (count + dominant `errorCode`), so a reviewing manager need not re-derive dispatch health from run telemetry. A dominant code is only named when it holds a strict majority. - Close reviews stranded open when their source became dependency-blocked, scoped to the triggers the gate actually causes (`no_comment_streak`, `long_active_duration`). `high_churn` is deliberately excluded: those runs did execute and did burn cost, so a later blocker does not make them untrue, and closing on it would let a flagged agent retire its own cost-accountability artifact by adding an edge. - Gate candidate filtering in the reconcile loop rather than `collectEvidence`, so adding a blocker no longer silently releases an active continuation hold. Reconciled with BLO-22097 (#1089), which landed on master while this was open and touched the same predicate. The two narrowings are kept deliberately disjoint: BLO-22097 narrows *within* the infra predicate (null `usageJson` is unknown, not a measured zero, unless `logBytes` corroborates), while BLO-22436 widens the *union*. Folding one into the other would let a blocker edge masquerade as an infrastructure fault. Verified: 111/111 in productivity-review-service.test.ts (105 + master's 6), `tsc --noEmit -p server` clean. Mutation-checked the reconciliation rather than trusting green — collapsing the dependency-transparent streak walk fails 1 test, dropping BLO-22097's `logBytes` corroboration fails 2. Both intents are load-bearing and neither was lost in the merge.
…on gate to closable triggers (BLO-22436) Ally review on #1158 found two real gaps: - `cancelQueuedRunForBlockedDependencies` never stamps `issueCommentStatus`, so every dependency-gate cancellation defaults to `not_applicable` and was rendered in both the "never-invoked" and "non-executing" evidence lines with nothing marking them as the same runs. The evidence block now states the measured overlap instead of two independent-looking counts, and the BLO-22436 tests seed `issueCommentStatus: "not_applicable"` on their dependency-blocked runs to match the gate's actual write. - The generation-time skip for blocked issues fired unconditionally, regardless of trigger — unlike the close path's `isDependencyBlockedClosableTrigger`, which deliberately excludes `high_churn` because a blocker added after the fact doesn't undo runs that already executed and burned cost. The unconditional skip let a flagged agent retire its own cost-accountability artifact one cycle early just by adding a `blockedBy` edge. Generation is now scoped to the same trigger set the close path trusts, evaluated after evidence collection. Also tightens a stale comment (the BLO-26165 note described a predicate this PR split in two) and adds a defensive trigger re-check at the close predicate's consult site so it's locally correct independent of the uniqueness index it currently relies on.
…ed-trigger set (BLO-22436) Ally review on 37c1bd6 — Important 1. The dependency-blocked gates keyed on `evidence.trigger`, but `choosePrimaryTrigger` is a priority ladder rather than a classification: `no_comment_streak` outranks `high_churn`, so a blocked source that is both silent and churning presented a dependency-closable primary while carrying non-closable churn evidence underneath. The defaults make that overlap the norm (`noCommentStreakRuns` and `highChurnHourly` are both 10), so the original evasion — add the `blockedBy` edge before generation — stayed open for exactly the agent worth reviewing. - Record every fired trigger on the evidence (`firedTriggers`, ladder order, so `firedTriggers[0] === trigger`), built from the same four booleans the ladder reads so the set cannot drift from its primary. - Generation gate suppresses only when the WHOLE fired set is closable. - Persist `firedTriggers` in the review's activity-log details and apply the same whole-set test on the close path. Without this the generation fix defeats itself: a surviving co-fired review is stored with a closable primary and the next close pass retires it. Rows predating the field fall back to the single trigger — deliberately the old behaviour, not fail-closed, since an open legacy review on a now-blocked source has no other path back. - Tests: re-seed the "unresolved blocker" case at 10-minute spacing so it exercises dependency suppression instead of trigger precedence (and retitle it); add the co-fired generation case, the closable-set close case, and a note pinning the legacy-row fallback. - Suggestions: merge the two adjacent parentheticals in the non-executing evidence line (and drop the `N of N` echo), and carry the non-executing count into `buildRefreshComment` so the notification and the description agree. Co-Authored-By: Claude <noreply@anthropic.com>
37c1bd6 to
a9b18f2
Compare
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: a9b18f2
Prior Findings Dispositioned (1)
- prior:c761f0d important 2 — fixed —
server/src/services/productivity-review.ts:4343— generation now suppresses a dependency-blocked candidate only whenisDependencyBlockedClosableTriggerSet(evidence.firedTriggers)is true, so a co-firedhigh_churnorruntime_failure_streaktrigger prevents the blocker from erasing that accountability artifact. The fired set is constructed from all trigger booleans at:3071-3075.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The prior primary-trigger evasion is closed by evaluating the complete fired-trigger set rather than relying on the priority-selected trigger.
- The implementation preserves dependency-gate transparency for runtime-failure and high-churn evidence while suppressing only blocker-caused trigger combinations.
- The changed tests cover the overlapping silent/high-churn case, open-review closure behavior, streak accounting, and interaction-wake continuation holds.
- PR checks are green, including build, typecheck, server test shards, workspaces, and e2e.
Recommended Action
- No Critical or Important issues remain from this review.
- Merge when the repository's remaining merge prerequisites are satisfied.
1 participant
Thinking Path
Linked Issues or Issue Description
no_comment_streak, billing agents for platform faultsRelated PRs found by search (no duplicates):
fix(productivity-review): treat null usageJson as unknown, not zero (BLO-22097)— open, edits the sameisNeverExecutedRunfunction this PR splits intoisInfraFailureRun/isDependencyBlockedRun. Expect a textual conflict depending on merge order; neither change is semantically incompatible — fix(productivity-review): treat null usageJson as unknown, not zero (BLO-22097) #1089 refines what counts as a measured zero-token infra failure, this PR adds a disjoint dependency-gate population.fix(productivity-review): regenerate description on refresh when trigger flips (BLO-22105)— open, touchesproductivityReviewServicein the 2400-2480 region (refresh-comment / description regeneration path). Same file, adjacent-but-likely-non-overlapping concern; flagging for merge-order awareness.What Changed
server/src/services/productivity-review.tsisNeverExecutedRunintoisInfraFailureRun(unchanged BLO-21769 zero-token+failed-liveness check) andisDependencyBlockedRun(errorCode === "issue_dependencies_blocked").isNeverExecutedRunis now their union, used to exclude both fromnoCommentStreak's walk.runtimeFailureStreaknow walksisInfraFailureRunonly — dependency-gate cancellations no longer surface asruntime_failure_streak(an infra-fault trigger with a "route to platform/SRE" remedy menu that doesn't fit a graph-state fact).collectEvidencenow fetchesissuesSvc.listDependencyReadinessfor the source issue up front and returnsnull(skip) wheneverunresolvedBlockerCount > 0, before any trigger logic runs — an issue can't be reviewed while its runs are being cancelled by the dependency gate, regardless of which trigger would otherwise fire.nonExecutingRunCount/nonExecutingDominantErrorCodeto the evidence and tobuildReviewMarkdown's Evidence section (- Non-executing runs in sample window (excluded from streaks above): N (dominant errorCode: ...)), so a review generated for another reason (e.g. a genuine no-comment streak with some non-executing runs mixed into the sample window) reports dispatch health explicitly instead of the reviewing manager having to re-derive it from raw run telemetry.server/src/__tests__/productivity-review-service.test.ts—insertRunshelper gainederrorCodeandspacingMsparams; added anaddBlockerhelper (inserts ablocksissueRelationsedge); three new tests (below).Verification
Three new tests, all integration tests against the real
reconcileProductivityReviewspath on embedded postgres:excludes issue_dependencies_blocked cancellations from both streaks and produces no review— 10 dependency-gate-cancelled runs →result.created === 0,result.skipped === 1, no review issue. Matches the AC's "re-running the detector against BLO-20815's history produces no review issue."skips an issue with an unresolved blocker regardless of streak— a genuine 10-run silent-but-executed streak (would tripno_comment_streakon its own) plus a liveblocksedge → stillresult.created === 0. The hard gate wins over any trigger.reports non-executing dependency-blocked runs separately, without inflating either streak, once a review fires for another reason— 3 recent dependency-gate cancellations (blocker since resolved, so the gate above doesn't apply) + 10 older genuinely-silent executed runs → review fires onno_comment_streak: 10,runtime_failure_streak: 0, and a new evidence line:Non-executing runs in sample window (excluded from streaks above): 3 (dominant errorCode: issue_dependencies_blocked).All pre-existing BLO-21769 tests (positive control included) still pass unmodified —
isInfraFailureRunis behaviorally identical to the oldisNeverExecutedRunfor the infra-failure population; only the dependency-gate population is new.tsc --noEmit -p serverclean (0 errors).Risks
issuesSvc.listDependencyReadiness) per candidate issue per scan cycle. Candidates are capped atMAX_CANDIDATE_ISSUES, and the query is a single indexedissueRelationslookup scoped to one issue — consistent with existing per-candidate query volume in this same loop (countIssueRunsSince,countIssueCommentsSince, etc.).no_comment_streak) until the blocker clears. If an agent is genuinely burning cycles pointlessly on a blocked issue (e.g.high_churn), this PR also suppresses that signal. Accepted per BLO-22436's acceptance criteria ("not eligible for ano_comment_streakproductivity review at all whileunresolvedBlockerCount > 0" / verifying-signal test "(b) ... skipped regardless of streak") — the self-reinforcing loop this closes is worse than the narrow signal it gives up.Model Used
claude-sonnet-5[1m], 1M context window), via the PlatformSREEngineer Paperclip agent (claude_k8sadapter), with tool use and code execution.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template