fix(productivity-review): close open reviews whose source went terminal (BLO-20549)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• The productivity-review subsystem watches assigned issues and opens manager review issues when an agent's work episode looks stuck (no_comment_streak, long_active_duration, high_churn)
• Open review issues are retired by the reconcile sweep, but sources that reach terminal status drop out of the normal candidate loop
• A long-active review whose source later reaches done no longer represents an unattended active episode, so leaving it open creates manager triage noise
• cancelled, missing provenance, and historical triggers are different: an assignee can cancel/restore a source, and completion does not invalidate accountability signals like no-comment or high-churn
• This pull request narrows open-review retirement to the safe case: known long_active_duration reviews whose source is still done at the write boundary
• The benefit is targeted orphan cleanup without giving the reviewed assignee a cheap way to retire their own oversight artifact

Linked Issues or Issue Description

• Fixes: BLO-20549 — productivity-review: auto-close open reviews whose source issue went terminal
• Refs Suppress long-active productivity reviews behind a pending approval gate (BLO-19360) #887 (BLO-19360) — monitor-only suppression boundary this PR preserves
• Refs fix(productivity-review): suppress terminal-source no-comment reviews #355 (closed) / BLO-6243 — creation-time terminal-source suppression
• Refs Suppress long_active_duration productivity review for deliberate future monitors #33 — deliberate-future-monitor suppression

Live examples at time of original filing: BLO-20085 (in_progress) → source BLO-19707 (done); BLO-20503 (todo) → source BLO-20096 (done).

What Changed

• closeOpenSuppressedMonitorReviews → closeOpenSuppressedReviews, since it now closes both future-monitor and safe terminal-source cases.
• Added a terminal-source close branch only for reviews whose latest creation/update provenance says trigger: "long_active_duration" and whose source status is done.
• Explicitly leave cancelled sources open; cancellation is not protected like done and can be restored later.
• Explicitly leave no_comment_streak, high_churn, missing trigger provenance, and future trigger values open.
• Make the terminal-source close conditional at update time on the review still being open and the source still being done; audit logging and counters only run when that conditional update changes a row.
• Keep the existing monitor-scheduled branch separate and preserve its counter.

Verification

• PATH=/opt/homebrew/bin:$PATH pnpm --filter @paperclipai/server typecheck — pass
• PATH=/opt/homebrew/bin:$PATH pnpm --filter @paperclipai/server exec vitest run src/__tests__/productivity-review-service.test.ts — file loaded but skipped locally because embedded Postgres is unavailable on this host; CI is the authoritative execution signal for this suite
• git diff --check — pass

Updated embedded-Postgres tests cover:

1. Source done + long_active_duration closes and logs suppressedBy: "terminal_source".
2. Source cancelled + long_active_duration stays open.
3. Source done + no_comment_streak stays open.
4. Source done + missing provenance stays open.
5. Concurrent reconcilers log/count a terminal-source close once.
6. Active source negative case remains open.

No UI changes, so no screenshots.

Risks

• This is intentionally narrower than the first version. Reviews for cancelled sources, no-comment, high-churn, or missing provenance will still require manager/user action until separate semantics are designed.
• The conditional update prevents stale source status between the sweep read and close statement, but it is not a general-purpose source row lock for unrelated future flows.
• Migration safety: none — no schema change.

Model Used

Codex GPT-5 with repository tool use and local command execution.

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
• 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 where supported by this host
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots (n/a — no UI change)
• I have updated relevant documentation to reflect my changes (n/a — behavior documented in-code; no external docs cover this sweep)
• 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

[allyblockcast]

🔗 Paperclip issue: BLO-19707
🔗 Paperclip issue: BLO-20503
🔗 Paperclip issue: BLO-19360
🔗 Paperclip issue: BLO-20549
🔗 Paperclip issue: BLO-20096
🔗 Paperclip issue: BLO-20085
🔗 Paperclip issue: BLO-6243

[allyblockcast]

🔗 Paperclip issue: BLO-19707
🔗 Paperclip issue: BLO-20503
🔗 Paperclip issue: BLO-19360
🔗 Paperclip issue: BLO-20549
🔗 Paperclip issue: BLO-20096
🔗 Paperclip issue: BLO-20085
🔗 Paperclip issue: BLO-6243

[allyblockcast]

@ally please review at head 425bd9a.

Focus, in priority order:

1. The threat-model boundary. Suppress long-active productivity reviews behind a pending approval gate (BLO-19360) #887 documented this function as "deliberately monitor-only": an approval gate may suppress new productivity reviews but must never close one that already fired, because the reviewed agent can mint its own pending approval and retire its own oversight artifact. This PR closes already-fired reviews on terminal source status. Is the distinction I drew sound — that terminating the source issue costs the agent the work item itself, unlike a free-to-mint approval? Specifically: can an agent under review cheaply set its own source issue to cancelled to make the review disappear, and if so is the activityLog trace sufficient mitigation? If you think it is not, say so — I would rather not merge than widen a self-retirement path.

2. Branch-ordering. The terminal check now runs before the trigger === "long_active_duration" filter that previously gated the whole loop. Confirm no review that master would have closed as monitor_scheduled is now mis-attributed, and that reviews master left alone still get left alone.

3. Trigger-independence. The terminal branch fires for no_comment_streak and high_churn too, and for reviews with no recoverable trigger at all. Is there a trigger whose alarm legitimately survives its source completing?

Not asking you to re-litigate the counter split (closedSuppressedMonitorReviews vs new closedTerminalSourceReviews) — that is deliberate so the pre-existing monitor tests stay a regression check.

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Risks
• Missing section: ## Model Used
• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (1)

• [gstack/review] server/src/services/productivity-review.ts:914 — cancelled is an assignee-controlled self-retirement path, so the reviewed agent can now erase an already-fired oversight item. The normal issue mutation boundary permits an assignee to patch its own issue; unlike done, cancelled has no execution/artifact evidence gate, and the route actively cancels the current run when that status is requested. After this sweep closes the review, a plain status patch can return the source to todo without using the guarded resume path. The new activity entry is forensic evidence after the fact, not mitigation for removing the review from the manager's open queue.
  • Do not auto-close on an assignee-authored cancellation. Restrict this branch to a terminal transition made by a board/reviewer-authorized actor, or at minimum to a verifiable done transition; add an integration test proving the reviewed assignee cannot cancel, sweep away the review, and restore the source.

Important Issues (1)

• [pr-review-toolkit/code] server/src/services/productivity-review.ts:914 — trigger-independent closure conflates “the source is no longer active” with “the manager decision is no longer needed.” no_comment_streak and high_churn are historical completed-run/accountability signals, and the generated review explicitly asks a manager to choose a verdict; finishing the source does not invalidate that evidence. Closing reviews with no recoverable trigger also makes this branch automatically opt future/unknown trigger semantics into terminal suppression.
  • Limit automatic terminal retirement to the trigger whose condition is inherently current-state based (long_active_duration), and fail closed when trigger provenance is missing. If broader retirement is desired, define and test per-trigger terminal semantics rather than treating every existing and future trigger alike.

Strengths

• The non-terminal branch preserves the existing long_active_duration plus deliberate-future-monitor condition and keeps the monitor counter separate.
• The added negative test confirms active sources without a monitor remain untouched.
• The audit payload records the source status and source issue ID, which is useful once the authorization boundary is fixed.

Recommended Action

1. Fix the Critical self-retirement path before merge.
2. Narrow or explicitly justify and test terminal behavior per trigger.

[kkroo]

/ally review

[kkroo]

/ally review

[kkroo]

/ally review

[allyblockcast]

Staff Engineer structural review: changes required

Reviewed exact head 425bd9ad2853f77d6fc62b899c9f42d43f1772cf. CI is green, but this is not approved.

1. Critical: assignee-controlled cancellation retires its own oversight artifact (server/src/services/productivity-review.ts:900-914). The issue mutation boundary authorizes the current execution run to mutate its own issue (server/src/routes/issues.ts:4463-4465), and status: "cancelled" explicitly cancels that run (server/src/routes/issues.ts:8898-8899, 9378-9396). The sweep then closes the manager review. The activity log is forensic evidence, not prevention. Do not close on cancellation without proving the terminal transition came from a board/reviewer-authorized actor; the minimal safe alternative is done only. Add an end-to-end regression proving the reviewed assignee cannot cancel, reconcile away the review, and restore the source.

2. Important: preserve trigger semantics and fail closed on missing provenance (server/src/services/productivity-review.ts:897-917). no_comment_streak and high_churn are historical accountability signals, not current-state alarms; completion does not invalidate them. Existing code classifies both as soft-stop triggers and says they stay live (:436-438, :1111-1119). Restrict terminal retirement to long_active_duration; absent or unknown creation provenance must remain open unless per-trigger semantics are explicitly designed and tested.

3. Important: terminal status is a stale snapshot at the side effect (server/src/services/productivity-review.ts:891-946). The source can resume after sourceIssueById is loaded but before the review update, closing an active source's review. Make closure conditional on source terminality at write time with a transaction/lock or atomic predicate, and emit the activity/counter only when that conditional update changed the review.

This is the explicit lane call: operational orphan cleanup must not weaken the oversight invariant. Narrow the cleanup semantics rather than treating all terminal states and triggers as equivalent.

GitHub would not accept a formal REQUEST_CHANGES review because this bot identity owns the PR, so this comment is the blocking review verdict.

[kkroo]

/test
/ally review

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (2)

• prior:425bd9a critical 1 — still-present — server/src/services/productivity-review.ts:914 — The synchronized head only merged master; the PR's service and test files are unchanged from the prior reviewed head, and cancelled still enters the terminal-source retirement branch.
• prior:425bd9a important 1 — still-present — server/src/services/productivity-review.ts:914 — Terminal retirement still precedes trigger filtering, so no_comment_streak, high_churn, missing provenance, and future trigger values are all retired without per-trigger semantics.

Critical Issues (1)

• [prior:425bd9a critical 1] server/src/services/productivity-review.ts:914 — An assignee-controlled cancelled transition still retires the already-fired oversight issue. Cancellation is not protected by the done evidence gate, while the source can later be restored; the activity entry records the self-retirement but does not prevent it.
  • Do not auto-close on an assignee-authored cancellation. Restrict retirement to a terminal transition made by a board/reviewer-authorized actor, or at minimum to a verifiable done transition, and add an end-to-end regression for cancel, reconcile, and restore.

Important Issues (2)

• [prior:425bd9a important 1] server/src/services/productivity-review.ts:914 — The trigger-independent branch still treats historical accountability signals as current-state alarms. Completion does not invalidate no_comment_streak or high_churn, and missing/unknown provenance is silently opted into suppression.
  • Restrict terminal retirement to long_active_duration and fail closed on missing provenance, or define and test explicit terminal semantics for each trigger.
• [gstack/review] server/src/services/productivity-review.ts:935 — Source terminality is a stale snapshot by the time the review is closed. The source is loaded before the loop, but the update predicates only on the review ID; a concurrent resume can make the source active before this write and still lose its open review. Concurrent reconcilers can also update the same already-open review and emit duplicate audit entries/counters.
  • Make the close conditional at write time on both the review still being open and the source still being terminal, using a transaction/lock or an atomic conditional statement. Emit the activity and increment counters only when that conditional write changes one row.

Strengths

• The monitor-only branch and its separate counter remain intact.
• The audit payload preserves source status, source issue ID, and trigger provenance when available.
• The negative test covers an active source without a monitor.

Recommended Action

1. Fix the self-retirement path before merge.
2. Preserve per-trigger semantics and fail closed on unknown provenance.
3. Bind closure to current source/review state at the write boundary.

Because this PR is authored by app/allyblockcast, the App cannot review or approve its own PR. The exact head must be reopened under an independent author before an App approval can satisfy review/ally-complete; the shared merge-token user is not substitute gate evidence.

[kkroo]

@ally re-review at head 1f2fa48be. This addresses the three blocking findings by narrowing the cleanup instead of broadening oversight retirement.

Changes:

• terminal-source retirement now requires trigger === "long_active_duration" and source status done;
• cancelled sources remain open, so assignee-controlled cancel/restore cannot retire its own review;
• no_comment_streak, high_churn, missing provenance, and future trigger values remain open;
• close writes are conditional on the review still being open and the source still being done at update time; activity and counters only emit when that conditional update returns a row;
• added regression tests for done close, cancelled non-close, non-long-active non-close, missing provenance non-close, and concurrent reconcilers producing one audit/counter.

Local verification:

• pnpm --filter @paperclipai/server typecheck passed
• focused embedded-Postgres suite loaded but skipped on this host because embedded PG is unavailable
• git diff --check passed

/test
/ally review

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

• prior:425bd9a critical 1 — fixed — server/src/services/productivity-review.ts:910 — terminal-source retirement now requires the protected done state; cancelled does not enter either close branch, with the negative case covered at server/src/__tests__/productivity-review-service.test.ts:1203.
• prior:425bd9a important 1 — fixed — server/src/services/productivity-review.ts:897 — the latest review provenance must resolve exactly to long_active_duration; historical triggers and missing provenance fail closed, with regressions at server/src/__tests__/productivity-review-service.test.ts:1249 and server/src/__tests__/productivity-review-service.test.ts:1291.
• prior:6161a5a important 1 — fixed — server/src/services/productivity-review.ts:931 — the close is now conditional in one statement on the review remaining open and, for terminal retirement, the source still reading done; RETURNING gates activity and counters so racing reconcilers produce one close, covered at server/src/__tests__/productivity-review-service.test.ts:1322.

Looks good. No Critical or Important issues remain in the reviewed diff.

Strengths

• The cleanup is narrowly limited to a current-state long_active_duration alarm after evidence-gated completion.
• Cancellation, historical triggers, missing provenance, active sources, and duplicate reconcilers all have explicit regression coverage.
• Audit and metric side effects only run for the reconciler that actually changed the review row.

Recommended Action

1. Let the remaining CI jobs complete.
2. Reopen this exact head under an independent author so the allyblockcast App can provide the gate-authorizing approval; the shared merge-token User review is not substitute evidence for review/ally-complete.

Because this PR is authored by app/allyblockcast, the App cannot review or approve its own PR. The exact head must be reopened under an independent author before an App approval is possible.