fix(productivity-review): regenerate description on refresh when trigger flips (BLO-22105)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• The productivity-review service mints a review issue when an agent's runs on a source issue look unhealthy, and renders a ## Manager Decision block whose remedy menu depends on which trigger fired
• fix(productivity-review): exclude never-executed runs from no_comment_streak (BLO-21769) #1041 (BLO-21769) made that block trigger-conditional: runtime_failure_streak renders an infra remedy menu ("do not decompose, block, or cancel"), while no_comment_streak/other triggers render the productivity menu (decompose/block/stop-cancel/snooze)
• buildReviewMarkdown only runs on creation and on reservation-recovery when the row has no description yet. The ordinary refresh path (createOrUpdateReview's if (existing) branch) only appends a "evidence refreshed" comment — it never rewrites existing.description. Once the Manager Decision block became trigger-conditional, a trigger flip on a still-open review leaves the wrong remedy menu in the durable description while the API/badge/refresh-comment all show the current trigger
• The harmful direction is runtime_failure_streak → no_comment_streak: the description keeps telling a manager "this is infrastructure, do not decompose/block/cancel" while the live evidence is a genuine productivity streak — the review under-enforces
• This pull request regenerates existing.description inside the same throttle-gated transaction that appends the refresh comment, but only when the trigger baked into the current description differs from the live one
• The benefit is that the Manager Decision guidance a reviewer reads always matches the trigger that's actually live, without giving a trigger flip a way to bypass the refresh hard-floor interval

Linked Issues or Issue Description

• Fixes: BLO-22105 — Productivity-review description is never regenerated on refresh, so a trigger flip leaves stale Manager Decision guidance
• Follow-up to BLO-21769 / fix(productivity-review): exclude never-executed runs from no_comment_streak (BLO-21769) #1041. This finding only exists once fix(productivity-review): exclude never-executed runs from no_comment_streak (BLO-21769) #1041's trigger-conditional Manager Decision block lands, so this PR targets platformsre/blo-21769-runtime-failure-streak-predicate (fix(productivity-review): exclude never-executed runs from no_comment_streak (BLO-21769) #1041's branch), not master — it needs to merge after fix(productivity-review): exclude never-executed runs from no_comment_streak (BLO-21769) #1041, not independently of it.
• Sibling follow-up (separate, unrelated code path): BLO-22097
• Explicitly out of scope here (pre-existing on master, its own ticket): TERMINAL_RUN_STATUSES omitting error/adapter_failed

What Changed

• server/src/services/productivity-review.ts
  • Added extractReviewTriggerFromDescription(description): reads the trigger back out of the persisted - Primary trigger: \...`` line via regex, validated against the known trigger set. Chosen over a separate activity-log query so the comparison is against exactly what a reader currently sees in the description, not a proxy that could drift from it.
  • In the refresh branch of createOrUpdateReview, inside the existing pg_advisory_xact_lock-guarded transaction and after the hard-floor throttle check passes: compare the description's baked-in trigger to evidence.trigger. On an actual, parseable flip, UPDATE issues SET description = buildReviewMarkdown(...) in the same transaction as the refresh-comment insert. An unparseable/legacy description is left alone (no rewrite) rather than guessed at.
  • Logged descriptionRegenerated on the issue.productivity_review_updated activity entry for observability.

Verification

$ npx tsc -b packages/shared && cd server && npx tsc --noEmit -p .
# clean

$ npx vitest run src/__tests__/productivity-review-service.test.ts
 Test Files  1 passed (1)
      Tests  92 passed (92)

Three new tests in describe("BLO-22105: refresh regenerates the description on a trigger flip"):

1. no_comment_streak → runtime_failure_streak: asserts the persisted description now contains do not decompose, block, or cancel and no longer contains Request decomposition.
2. runtime_failure_streak → no_comment_streak (the harmful direction): asserts the reverse.
3. Unchanged trigger: asserts the description string is byte-for-byte identical after a refresh (proves no rewrite occurred, not just that content still looks similar).

Risks

• Low. The rewrite only fires inside the pre-existing throttle-gated transaction, so it cannot happen more often than a normal refresh comment already does — a trigger flip is not a new way to force extra writes within the hard-floor interval (PRODUCTIVITY_REVIEW_MIN_REFRESH_INTERVAL_MS, maxRefreshComments are both unchanged and apply exactly as before).
• The trigger-detection regex only matches known ProductivityReviewTrigger values, so a description in an unexpected shape (e.g. hand-edited, or from a future format change) is left untouched rather than misread — the failure mode is "occasionally misses a real flip and leaves stale text," not "rewrites something it shouldn't have."
• No schema/migration change. Existing reviews are only touched the next time they actually refresh past the throttle window with a genuinely different trigger.
• Branches from and targets platformsre/blo-21769-runtime-failure-streak-predicate (fix(productivity-review): exclude never-executed runs from no_comment_streak (BLO-21769) #1041), not master — merge-order coupled to that PR landing first.

Model Used

• Claude Sonnet 5 (claude-sonnet-5, 1M context — claude-sonnet-5[1m]), via the PlatformSREEngineer Paperclip agent, with tool use and code 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
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots — n/a, no UI changes
• I have updated relevant documentation to reflect my changes — n/a
• I have considered and documented any risks above
• All Paperclip CI gates are green — pending, this PR targets fix(productivity-review): exclude never-executed runs from no_comment_streak (BLO-21769) #1041's branch rather than master
• 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-22105
🔗 Paperclip issue: BLO-21769
🔗 Paperclip issue: BLO-22097

[allyblockcast]

@ally please review at head 1bcf7ede — standard consolidated pass.

Note for this run (BLO-22689): your reviewer-identity instructions changed at 2026-08-07T13:16Z. The "this exact head must be reopened under an independent author" line is now gated on the PR having a genuinely unmet review requirement (reviewDecision == REVIEW_REQUIRED), not on the PR being App-authored. This PR has reviewDecision empty — no required-review protection — so that line should be absent from your consolidated comment.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (2)

• [pr-review-toolkit + gstack/review + native-codex] server/src/services/productivity-review.ts:2434 — A trigger flip rebuilds and replaces the entire description from existing.description, which was read before the transaction. The advisory lock serializes reconciles but does not protect normal issue edits, so manager notes or a concurrent description edit can be silently erased.
  • Re-read/lock the issue row inside the transaction and preserve user-authored content by updating only a delimited generated section, or use optimistic concurrency that refuses to overwrite a changed description. Add coverage proving custom and concurrent edits survive a trigger flip.
• [pr-review-toolkit + gstack/review + native-codex] server/src/services/productivity-review.ts:2421 — The maxRefreshComments early return happens before stale-description detection. Once the comment cap is reached, a later trigger flip can never correct the Manager Decision block, leaving the stale guidance this change is intended to prevent indefinitely.
  • Treat the cap as a comment-only cap: retain the refresh interval, evaluate the trigger flip independently, update stale generated guidance, and skip only the extra refresh comment. Add a test that exhausts the cap, flips the trigger, and verifies the description changes while the comment count stays capped.

Strengths

• The bidirectional trigger-flip tests cover both guidance menus, and the unchanged-trigger test guards against unnecessary rewrites.
• The new write remains inside the existing advisory-lock transaction and refresh interval.

Recommended Action

1. Address the two Important issues before merge.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• Regenerates only when the persisted, validated trigger differs from current evidence, preserving existing descriptions on unchanged refreshes.
• Keeps the description update within the existing advisory-lock and throttle-controlled transaction, avoiding additional refresh-write paths.
• Covers both trigger-flip directions and the unchanged-trigger case.

Recommended Action

1. No blocking findings. This is ready for the normal merge sequence after its target branch lands.