feat(productivity-review): report a dependency-blocked accounting bucket (BLO-22887)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• The productivity-review detector watches assigned issues and mints a review issue — a real, dispatchable work item costing a manager run — when an episode looks neglected
• BLO-22436 already stops it firing on a dependency-blocked source whose entire fired-trigger set is dependency-closable (no_comment_streak, long_active_duration); triggers a blocker does not excuse (high_churn, runtime_failure_streak) deliberately still fire
• But those surviving reviews said nothing about the blocker. A manager read - Elapsed accounting: 18h 52m monitor-gated, 13h 8m unattended with no indication that the control plane had independently classified that same wall-clock as dependency_blocked — the two subsystems disagreeing in the reviewer's face, which is the exact finding BLO-22887 was filed on
• That disagreement is what makes a reviewer close a genuine high_churn review as a false positive, or spend the run re-deriving blocker state the platform already had
• This pull request adds a third accounting bucket — dependency-blocked — reported next to the monitor-gated/unattended split rather than folded into it, in both the review description and the refresh comment
• The benefit is that a review still warranted on other grounds carries the blocker context that explains its own existence, without weakening the trigger it fired on

Linked Issues or Issue Description

Refs BLO-22887 — "Productivity detector counts dependency-blocked time as unattended, generating false long_active_duration reviews".

That issue's ACs 1, 3 and 4 (suppression on both the generation and close paths, monitor-lapse independence, true-positive preservation) already landed on master via BLO-22436. AC2 is the only one outstanding and is what this PR closes:

Elapsed accounting distinguishes a third bucket — dependency-blocked — from monitor-gated and unattended, and the review body reports it separately when a review is still warranted on other grounds.

Supersedes #1361, which was closed as superseded (it re-implemented ACs 1/3/4 by a worse mechanism and was written against capacityGating, deleted by BLO-23624). Nothing from that branch is reused.

What Changed

• DependencyGating type + evidence.dependencyGating (server/src/services/productivity-review.ts) — unresolved blocker count, the done-but-awaiting-workspace-finalize subset, and the fired triggers a blocker does not excuse. null whenever the source has no unresolved blocker, so the line's presence is itself the signal.
• formatDependencyGating — renders the bucket beside the existing formatMonitorGating / formatNoExecutableTurnGating helpers.
• Two render sites — - Dependency accounting: in buildReviewMarkdown (the durable evidence block) and in buildRefreshComment (what lands in the manager's notifications), matching how both existing accounting lines are wired.
• reconcileProductivityReviews — dependencyBlockedSourceIssueIds widened from Set<string> to a Map carrying the counts. No new query: the listDependencyReadiness call it already makes for BLO-22436's suppression gate is the sole source. The .has() membership semantics of the suppression check are unchanged.
• collectEvidence sets dependencyGating: null and fetches nothing. Its other caller — isProductivityReviewContinuationHoldActive — must not see dependency state (existing header note), and would otherwise pay a second readiness round-trip for a field it never renders.
• Three test cells in server/src/__tests__/productivity-review-service.test.ts.

Deliberate scope limit: state, not a span

The line reports blocker state at the evidence pass, and says so in its own text:

- Dependency accounting: 1 unresolved `blockedBy` blocker at this evidence pass; reviewed
  anyway because `high_churn` fired, which an unresolved blocker does not excuse — blocker
  state at this pass, not a measured span: the elapsed figures above are wall-clock and are
  NOT reduced by this, so read their unattended portion as covering dependency-blocked time
  of unrecorded length

The readiness map carries ids and counts, never the edge's own age, so there is no honest "blocked for N hours" figure derivable from it. Manufacturing one is precisely what sank #1361 — an unbounded latestRuns.find(run => run.status === "scheduled_retry" …) against MAX_RUNS_FOR_STREAK = 100, so an older park silently read as absent. Subtracting an unmeasured span from the wall-clock buckets would replace a known-wrong attribution with an invented one. Naming the limit is the deliverable.

This also respects master:3470-3481 (BLO-25877), which explicitly rejects folding a suppression subtraction into the longActive predicate — an accounting line does not touch that predicate, and this one does not.

Verification

Red before green. Both new assertion cells fail on master (commit 2ebf8009) with the tests present and the implementation absent:

$ pnpm exec vitest run server/src/__tests__/productivity-review-service.test.ts -t "BLO-22887"

 FAIL  src/__tests__/productivity-review-service.test.ts > reports a dependency-blocked bucket
       alongside the elapsed split when a review still fires on a non-closable trigger (BLO-22887)
 ❯ src/__tests__/productivity-review-service.test.ts:1549:33
    1549|     expect(review?.description).toContain("- Dependency accounting: 1 …

 FAIL  src/__tests__/productivity-review-service.test.ts > carries the dependency accounting line
       into the refresh comment (BLO-22887)
 AssertionError: expected 0 to be greater than 0
 ❯ src/__tests__/productivity-review-service.test.ts:1621:36

 Test Files  1 failed (1)
      Tests  2 failed | 1 passed | 163 skipped (166)

The third cell — the over-reporting guard — passes on master by construction and is a regression guard, not a red case.

Green after the change:

$ pnpm exec vitest run server/src/__tests__/productivity-review-service.test.ts -t "BLO-22887"
 Test Files  1 passed (1)
      Tests  3 passed | 163 skipped (166)

$ pnpm exec vitest run server/src/__tests__/productivity-review-service.test.ts
 Test Files  1 passed (1)
      Tests  166 passed (166)

$ pnpm --filter @paperclipai/server exec tsc --noEmit     # clean
$ git diff --check                                        # clean

The three cells, and what each is load-bearing for:

1. reports a dependency-blocked bucket alongside the elapsed split when a review still fires on a non-closable trigger — 10 commenting runs + an unresolved blocker → high_churn review created, description contains the line with the correct count and names high_churn as the reason it survived.
2. omits the dependency accounting line when the source issue's only blocker is resolved — the over-reporting guard, and the counterpart to BLO-22436's cell 3. A line that rendered unconditionally would pass cell 1 while telling every reviewer in the fleet that an unblocked issue is dependency-blocked. Uses a done blocker rather than no edge at all, so it proves the key is unresolved readiness and not mere edge existence. Asserts not.toContain("Dependency accounting") while the review is still created.
3. carries the dependency accounting line into the refresh comment — the refresh comment is what reaches a manager's notifications and already mirrors both existing accounting lines; a bucket present only in the description would leave the summary telling a different story from the artifact it summarises.

The pre-existing BLO-22436 suppression cells (skips an issue with an unresolved blocker… → dependencyBlockedSuppressed === 1, and both "still generates a review for a dependency-blocked issue" cells) all still pass unchanged, which is the AC1/AC3/AC4 regression surface.

Risks

Low. Additive reporting only — no predicate, no suppression decision, and no counter changes.

• No behavioural change to what gets reviewed. The suppression gate's .has() test is identical after the Set → Map widening; only the value type changed. result.dependencyBlockedSuppressed is untouched.
• No new database work. The readiness map was already fetched on this path for every candidate. No issueRelations query and no latestRuns scan were added — the two mechanisms that made fix(productivity): account for dependency-blocked elapsed time #1361 unsafe.
• Over-reporting is the realistic failure mode (a line on an unblocked issue), and cell 2 is the guard against exactly it.
• The continuation-hold path is unaffected: collectEvidence hard-codes dependencyGating: null and fetches nothing, so isProductivityReviewContinuationHoldActive sees no dependency state and pays no extra round-trip — preserving the invariant its header comment documents.
• Review bodies get one line longer on blocked sources. No migration, no contract change, no UI change.

Model Used

Claude — claude-opus-5[1m], 1M context, extended thinking, tool use / code execution via Claude Code running as a Paperclip heartbeat 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 (fix(productivity): account for dependency-blocked elapsed time #1361, superseded and closed; fix(productivity-review): suppress dependency-blocked reviews #1230)
• 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-side markdown only
• I have updated relevant documentation to reflect my changes — n/a; the behaviour is documented in-code next to the two existing accounting buckets
• I have considered and documented any risks above
• All Paperclip CI gates are green
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups
• I will address all Greptile and reviewer comments before requesting merge

---

Paperclip issue: https://paperclip.blockcast.net/BLO/issues/BLO-22887

[allyblockcast]

🔗 Paperclip issue: BLO-23624
🔗 Paperclip issue: BLO-25877
🔗 Paperclip issue: BLO-22887
🔗 Paperclip issue: BLO-22436

[github-actions]

@ally head 22ca24a has been awaiting review for 2.0h with no review on either surface (pulls/1722/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head 22ca24a.

[github-actions]

@ally head 22ca24a has been awaiting review for 4.4h with no review on either surface (pulls/1722/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head 22ca24a.

[github-actions]

@ally head 22ca24a has been awaiting review for 7.0h with no review on either surface (pulls/1722/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head 22ca24a.

[allyblockcast]

Ally — Consolidated PR Review

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

The design call is the right one — reporting blocker state next to the elapsed split instead of inventing a span from a readiness map that carries no edge timestamps, and threading the counts through the Set→Map change so the bucket costs no second listDependencyReadiness round-trip. Two things about the rendered line itself do not hold up.

Critical Issues (0)

Important Issues (2)

• [code / gstack-review] server/src/services/productivity-review.ts:1149 — the closing caveat asserts the existence of figures that are frequently absent from the artifact it is printed in.

  • The line ends …the elapsed figures above are wall-clock and are NOT reduced by this, so read their unattended portion as covering dependency-blocked time of unrecorded length. The "unattended portion" exists only inside the Elapsed accounting line, which both builders render conditionally on evidence.monitorGating (productivity-review.ts:3894, :4002).
  • monitorGating is null whenever elapsedMs is null (monitorGatingBreakdown, productivity-review.ts:1026), and elapsedMs is null unless the source is in_progress (productivity-review.ts:3463). Candidates are status IN ('todo','in_progress') (productivity-review.ts:4769) — so every todo candidate renders the dependency line with no elapsed split above it. A todo issue carrying an unresolved blockedBy edge is the single most ordinary dependency-blocked shape in this system, not a corner.
  • It is worse in buildRefreshComment (productivity-review.ts:3963): unlike the description, that body has no unconditional elapsed figure at all, so with monitorGating null the sentence points at nothing whatsoever. The description at least prints Current active elapsed time: unknown (msToHuman(null), :534).
  • This is the one clause whose whole job is stopping a manager misreading a number; pointing it at a number that is not on the page inverts that. Recommend making the tail conditional — pass Boolean(evidence.monitorGating) (or the gating itself) into formatDependencyGating and fall back to something like no elapsed split was computed for this episode, so there is no wall-clock figure this reduces.

• [tests] server/src/__tests__/productivity-review-service.test.ts:1549 — everything formatDependencyGating renders after the count prefix is uncovered; deleting it leaves the suite green.

  • Both positive tests assert only the prefix - Dependency accounting: 1 unresolved \blockedBy` blocker (:1549, :1630). The pendingFinalizeBlockerCount parenthetical (productivity-review.ts:1139), the nonClosableTriggers "reviewed anyway because … fired" clause (:1146), and the "not a measured span" caveat (:1149`) have no assertion pinning them.
  • The assertion intended to cover the trigger clause — expect(review?.description).toContain("\high_churn`")at:1552, whose comment says it verifies the line "names the trigger that kept the review alive" — is satisfied by the pre-existing Primary trigger: `high_churn`line that the *next* test asserts at:1586. It passes with the survived` ternary deleted entirely.
  • Given how much of this PR's value is in the prose (the counts alone are the part a reviewer could get from the source issue), recommend asserting the full rendered line, or at minimum a scoped substring such as reviewed anyway because \high_churn` fired, plus one case with a done-but-pending-finalize blocker to exercise pendingFinalizeBlockerCount`.

Suggestions (2)

• [comments / tests] server/src/__tests__/productivity-review-service.test.ts:1553 — // The bucket is reported next to the elapsed split, never folded into it. sits above not.toContain("Dependency accounting: 0 "), which checks neither adjacency nor non-folding, and cannot fail: formatDependencyGating is only reached when dependencyGating is non-null, which requires unresolvedBlockerCount > 0. The never-folded invariant is the design centrepiece and is directly assertable — pin the Elapsed accounting unattended figure across a with-blocker / without-blocker pair and require it identical.

• [code] server/src/services/productivity-review.ts:400 — the durable description only picks up dependencyGating at creation or on a trigger flip (descriptionStale, :4149), while the refresh comment always carries the live value. A blocker added or resolved mid-episode therefore leaves the description's dependency line permanently disagreeing with the newest refresh comment. That is pre-existing behaviour shared with Elapsed accounting, but dependency edges flip far more often than monitor gating does, so it will be visible sooner here. at this evidence pass partly inoculates the wording; worth a note in the type comment either way.

Strengths

• The refusal to synthesise a duration is correct and the reasoning is recorded where the next editor will hit it (productivity-review.ts:233-241), including the concrete prior failure (PR #1361's unbounded latestRuns.find against a 100-run cap).
• Set → Map keeps the .has() membership test at :4868 byte-identical while carrying the counts, so BLO-22436's suppression semantics are provably unchanged and no second readiness query is added.
• The attach point is right: after the approval/monitor suppression branches, where the union has narrowed and both non-rendering paths have already continued. nonClosableTriggers really is non-empty by construction there — collectEvidence returns null when no trigger fires (:3544), and isDependencyBlockedClosableTriggerSet fails closed on an empty set (:1399) — so the invariant the comment claims holds.
• pendingFinalizeBlockerCount is a genuine subset of unresolvedBlockerCount (issues.ts:1655-1667), so the parenthetical is arithmetically honest rather than a second, overlapping total.
• Leaving dependencyGating: null on the collectEvidence path is the right call and the note explains why the continuation-hold caller must not see dependency state at all.

Recommended Action

1. Fix Critical issues before merge. — none.
2. Address Important issues this cycle: gate the "elapsed figures above" caveat on monitorGating, and extend the assertions past the count prefix.
3. Consider Suggestions opportunistically.