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, merged) 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
• Ally's review on the first version of this PR found two real gaps in that first pass: the comment-count cap silently blocked the description fix forever once hit, and the overwrite could clobber a concurrently-edited description since the advisory lock doesn't protect against a plain issue edit. Both are fixed here.

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 (now merged) — this PR was rebased onto master and now targets it directly.
• 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: the hard-floor interval check still gates everything (comment + description), so a trigger flip can't force more writes than a normal refresh allows. Past that gate, on an actual, parseable trigger flip, the description is regenerated via buildReviewMarkdown. An unparseable/legacy description is left alone rather than guessed at.
  • Comment cap no longer blocks the description fix (Ally finding test(plugin-linear): requestId fixtures + getLinkByLinear mock-leak fix; scripts: ensure-build-deps freshness check #1). maxRefreshComments now only suppresses the extra refresh comment; the description rewrite still runs. Previously, once a review hit the comment cap, a later trigger flip could never correct the Manager Decision block again — exactly the staleness this PR exists to close.
  • Concurrent-edit safety (Ally finding fix(test): restore upstream agent-permissions expectations dropped during v513 merge #2). existing.description is read before the transaction opens, and the advisory lock only serializes this refresh path against itself — it does nothing to protect a plain concurrent edit to the review issue's description (e.g. a manager note added directly via the issue API). The UPDATE is now guarded with WHERE description = <the value read>, so a concurrent edit loses the race cleanly (0 rows matched, nothing clobbered) instead of being silently discarded.
  • Logged descriptionRegenerated on the issue.productivity_review_updated activity entry for observability.

Verification

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

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

Five 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).
4. New: exhausts DEFAULT_PRODUCTIVITY_REVIEW_MAX_REFRESH_COMMENTS, confirms a same-trigger refresh past the cap stays a no-op, then flips the trigger and asserts the description is corrected while the refresh-comment count stays capped.
5. New: injects a concurrent plain UPDATE issues SET description = ... (via a db.transaction spy, same idiom as execution-workspaces-service.test.ts) between the outer read and the guarded write, and asserts the concurrently-written description survives untouched while the refresh comment still gets appended.

Risks

• Low. The description rewrite is still gated by the hard-floor interval (PRODUCTIVITY_REVIEW_MIN_REFRESH_INTERVAL_MS), so a trigger flip is not a new way to force extra writes within that window. maxRefreshComments now bounds comment churn only, by design (see "What Changed") — this is a deliberate narrowing of what the cap protects, not a bypass of it.
• 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."
• The optimistic-concurrency guard on the description UPDATE means a genuine race (concurrent manual edit) can leave the Manager Decision block stale until the next refresh cycle notices the flip again — an intentional trade-off favoring not clobbering human edits over always winning the race.
• No schema/migration change. Existing reviews are only touched the next time they actually refresh past the throttle window with a genuinely different trigger.
• Rebased onto master now that fix(productivity-review): exclude never-executed runs from no_comment_streak (BLO-21769) #1041 merged; no longer stacked on its branch.

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 on this push (rebased onto master + addressed review findings)
• 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.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Strengths

• Refreshes preserve the hard interval and comment cap while allowing stale trigger-specific Manager Decision guidance to self-correct.
• The guarded description update avoids clobbering a concurrent direct edit; targeted tests cover both trigger directions, cap behavior, unchanged triggers, and that race.

Recommended Action

1. Merge when the remaining repository requirements are satisfied.

[allyblockcast]

@ally please review at head 095db9ebd — this addresses both Important findings from the prior review (comment-cap no longer blocks the description regeneration; the overwrite is now guarded with optimistic concurrency against a concurrent edit) and rebases the PR onto master now that #1041 has merged.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The description rewrite is limited to a recognized, changed trigger and remains behind the existing refresh interval.
• The conditional update protects a concurrent direct description edit, while the comment cap still permits stale durable guidance to self-correct.
• Tests cover both trigger directions, unchanged triggers, the capped-comment path, and the concurrent-edit race.

Recommended Action

1. No blocking findings. Continue with the normal merge sequence.

[kkroo]

Approved at exact head 1268fdc. Ally's exact-head review has no blocking findings and the required head checks completed successfully.

[kkroo]

Revalidated exact head d95980a after conflict rebuild onto current master: original two human-authored commits preserved; server typecheck and commit-attribution gate pass; focused productivity-review suite loads (DB-backed cases skip without local test database).

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• Regenerates the durable Manager Decision guidance only for a recognized trigger flip, while retaining the existing refresh interval guard.
• Separates the refresh-comment cap from correction of stale guidance and protects a concurrent direct description edit with a conditional update.
• Adds coverage for both trigger directions, unchanged triggers, the capped-comment path, and the direct-edit race.

Recommended Action

1. No blocking findings. Continue with the normal merge sequence.