fix(productivity-review): suppress reviews on dead assignees, exclude never-started runs - #8
Open
PraeSynBH wants to merge 2 commits into
Open
fix(productivity-review): suppress reviews on dead assignees, exclude never-started runs#8PraeSynBH wants to merge 2 commits into
PraeSynBH wants to merge 2 commits into
Conversation
… never-started runs (RBR-983) RBR-978 fired a no_comment_streak review on RBR-920 while the assignee (CTO) was in `error` with no live process, and 9 of the 12 "completed" runs never actually executed -- they cancelled each other in a 43s reassignment race. The monitor measured the absence of output from a dead process and attributed it to the agent's work habits. - AC1: suppress review generation entirely when the assignee cannot act (`error`/`paused`/`terminated`/`pending_approval`); log an `issue.productivity_review_suppressed_infra` activity entry instead of minting a performance finding. - AC2: exclude pre-start-cancelled runs (`issue_assignee_changed`, `lock_released_on_reassignment`, and other pre-start cancel codes, plus the structural `startedAt == null` signal) from the no-comment streak -- only executed runs are admissible evidence. - AC3: classify a streak dominated by wall-clock timeouts as a `capacity` signal, not a `productivity` one, and say so explicitly in the review body. - AC4: do not fire `long_active_duration` when the assignee cannot act -- elapsed time there measures outage, not effort. - AC5: if the most recent dispositioned (`done`) review on the same source issue already closed with an infra/capacity signal class and the pattern recurs, extend that review with a recurrence comment instead of minting a fresh one. - AC6: behavioral test -- an agent in `error` with 9 no-comment cancelled (never-started) runs on an assigned `in_progress` issue (which would also trip long_active_duration) produces zero productivity reviews and exactly one suppression log entry. Verified: `vitest run src/__tests__/productivity-review-service.test.ts` against a local embedded-postgres dev database -- 12/12 passing, including the new AC6 test and the updated pre-existing streak-label assertion. Per RBR-939, did not run repo-wide typecheck/pnpm test.
… beforeAll Same embedded-postgres teardown cost as startup; afterAll was flaking past vitest's 10s default under load while beforeAll already carried 120_000. No behavioral change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thinking Path
Linked Issues or Issue Description
No public GitHub issue exists for this. Following the bug report template:
reconcileProductivityReviewscounts every terminal run (including runs cancelled before they ever executed) toward the no-comment streak, and fireslong_active_durationpurely on elapsed wall-clock time — regardless of whether the assignee agent has a live process. An agent that goes intostatus: error(e.g. "Process lost — server may have restarted") produces a false-positive productivity review: the monitor reads "no comments" and "long elapsed time" as evidence of poor diligence when the agent had no process to act with.error/paused/terminated/pending_approval), exclude runs that never started executing (pre-start cancels from reassignment races) from the no-comment streak, label a streak dominated by wall-clock timeouts as a capacity signal rather than a productivity one, and avoid firinglong_active_durationon an assignee that cannot act. If a prior review on the same source issue already closed with an infra/capacity disposition and the pattern recurs, it should extend that review rather than mint a duplicate.in_progressissue assigned to an agent withstatus: error, insert 9+cancelledheartbeat runs witherrorCode: issue_assignee_changed(nostartedAt) and a 7h-elapsed active episode, runreconcileProductivityReviews. Before this change: fires bothno_comment_streakandlong_active_duration, minting a review that blames the agent. After:result.created === 0, zero review issues created, oneissue.productivity_review_suppressed_infraactivity log entry.What Changed
resolveAssigneeActability: before evaluating any trigger, checks whether the assignee agent's lifecycle status (error/paused/terminated/pending_approval) means it currently has no live process. If so, the review is suppressed entirely and anissue.productivity_review_suppressed_infraactivity log entry is written instead (rate-limited like refresh comments), routing the signal to the operator as an infra fault rather than a performance finding.isPreStartCancelledRun/isExecutedRun: a run that terminated before the adapter ever started (identified by known pre-start-cancel error codes, astopReasonin the result payload, or the structuralstartedAt == nullsignal on acancelledrun) is excluded from the no-comment streak and from the executed-run counts used for evidence — only runs that actually executed are admissible evidence about the assignee's output.classifySignal: labels the fired trigger'ssignalClassasproductivity,capacity(wall-clock timeouts dominate the streak), orinfra(assignee cannot act), and includes the classification and its reason in both the review body and the activity log so the review reads correctly.long_active_durationno longer fires when the assignee cannot act — elapsed time against a dead/paused process measures an outage, not effort.findUnresolvedDispositionedReviewCause: if the most recentdonereview on the same source issue was created with aninfraorcapacitysignal class and the pattern recurs, extends that review with a recurrence comment (issue.productivity_review_recurrence_held) instead of minting a fresh review that re-derives the same conclusion.executedRunCount,preStartCancelledRunCount,wallClockTimeoutRunCount, "No-comment executed-run streak") to reflect the corrected accounting.errorwith 9 no-comment pre-start-cancelled runs on an assignedin_progressissue (which would also independently triplong_active_duration) produces zero productivity reviews and exactly one suppression log entry.Verification
Ran the full productivity-review service test file against a local embedded-postgres dev database:
Result: 12/12 passing, including the new AC6 behavioral test and an updated pre-existing assertion (
No-comment completed-run streak→No-comment executed-run streak, matching the corrected copy). Per repo guidance on embedded-Postgres suite cost, did not run the repo-widepnpm testorpnpm -r typecheck; ran a scopednpx tsc --noEmitpass over the server package and confirmed no new errors were introduced by this file (all pre-existing errors are unrelated plugin-sdk/plugin-host-services module-resolution issues, verified present before this change).Risks
error/paused/terminated/pending_approvalwill no longer get productivity reviews at all while in that state — this is intentional (the whole point of the fix) but means a genuinely unproductive agent that later flips intoerrorright before review time would also be suppressed until it recovers. Mitigated by theissue.productivity_review_suppressed_infraactivity log entry, which still surfaces the situation to operators.PRODUCTIVITY_REVIEW_PRE_START_CANCEL_ERROR_CODES) is a fixed set; a future cancel path with a new error code that reuses the pattern won't be recognized by name, but the structuralstartedAt == nullfallback still catches it for anycancelledrun.Model Used
Claude (Anthropic), model
claude-sonnet-5, via Claude Code CLI. Standard reasoning mode, with tool use (file read/search/edit, shell execution) to inspect the existing implementation, run the test suite, and fix a regressed assertion.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template#NNN/github.com/paperclipai/paperclipURLs)docs/...,fix/...) and contains no internal Paperclip ticket id or instance-derived details