diff --git a/server/src/__tests__/productivity-review-service.test.ts b/server/src/__tests__/productivity-review-service.test.ts index 3633d1163c45..a7ff33014422 100644 --- a/server/src/__tests__/productivity-review-service.test.ts +++ b/server/src/__tests__/productivity-review-service.test.ts @@ -542,6 +542,243 @@ describeEmbeddedPostgres("productivity review service", () => { expect(await listRefreshComments(review!.id)).toHaveLength(DEFAULT_PRODUCTIVITY_REVIEW_MAX_REFRESH_COMMENTS); }); + describe("BLO-22105: refresh regenerates the description on a trigger flip", () => { + it("regenerates the Manager Decision block when a no_comment_streak review flips to runtime_failure_streak", async () => { + const now = new Date("2026-04-28T12:00:00.000Z"); + const seeded = await seedAssignedIssue(); + await insertRuns({ + companyId: seeded.companyId, + agentId: seeded.coderId, + issueId: seeded.issueId, + count: DEFAULT_PRODUCTIVITY_REVIEW_NO_COMMENT_STREAK_RUNS, + now, + }); + + const service = productivityReviewService(db); + await service.reconcileProductivityReviews({ now, companyId: seeded.companyId }); + const [review] = await listProductivityReviews(seeded.companyId); + expect(review?.description).toContain("Primary trigger: `no_comment_streak`"); + expect(review?.description).toContain("Request decomposition"); + + const refreshAt = new Date(now.getTime() + DEFAULT_PRODUCTIVITY_REVIEW_REFRESH_INTERVAL_MS); + await insertRuns({ + companyId: seeded.companyId, + agentId: seeded.coderId, + issueId: seeded.issueId, + count: DEFAULT_PRODUCTIVITY_REVIEW_NO_COMMENT_STREAK_RUNS, + now: refreshAt, + status: "failed", + livenessState: "failed", + usageJson: { inputTokens: 0, outputTokens: 0 }, + }); + + const refresh = await service.reconcileProductivityReviews({ now: refreshAt, companyId: seeded.companyId }); + expect(refresh.updated).toBe(1); + + const [refreshedReview] = await listProductivityReviews(seeded.companyId); + expect(refreshedReview?.id).toBe(review!.id); + expect(refreshedReview?.description).toContain("Primary trigger: `runtime_failure_streak`"); + expect(refreshedReview?.description).toContain("do not decompose, block, or cancel"); + expect(refreshedReview?.description).not.toContain("Request decomposition"); + }); + + it("regenerates the Manager Decision block when a runtime_failure_streak review flips to no_comment_streak", async () => { + const now = new Date("2026-04-28T12:00:00.000Z"); + const seeded = await seedAssignedIssue(); + await insertRuns({ + companyId: seeded.companyId, + agentId: seeded.coderId, + issueId: seeded.issueId, + count: DEFAULT_PRODUCTIVITY_REVIEW_NO_COMMENT_STREAK_RUNS, + now, + status: "failed", + livenessState: "failed", + usageJson: { inputTokens: 0, outputTokens: 0 }, + }); + + const service = productivityReviewService(db); + await service.reconcileProductivityReviews({ now, companyId: seeded.companyId }); + const [review] = await listProductivityReviews(seeded.companyId); + expect(review?.description).toContain("Primary trigger: `runtime_failure_streak`"); + expect(review?.description).toContain("do not decompose, block, or cancel"); + + const refreshAt = new Date(now.getTime() + DEFAULT_PRODUCTIVITY_REVIEW_REFRESH_INTERVAL_MS); + await insertRuns({ + companyId: seeded.companyId, + agentId: seeded.coderId, + issueId: seeded.issueId, + count: DEFAULT_PRODUCTIVITY_REVIEW_NO_COMMENT_STREAK_RUNS, + now: refreshAt, + }); + + const refresh = await service.reconcileProductivityReviews({ now: refreshAt, companyId: seeded.companyId }); + expect(refresh.updated).toBe(1); + + const [refreshedReview] = await listProductivityReviews(seeded.companyId); + expect(refreshedReview?.id).toBe(review!.id); + expect(refreshedReview?.description).toContain("Primary trigger: `no_comment_streak`"); + expect(refreshedReview?.description).toContain("Request decomposition"); + expect(refreshedReview?.description).not.toContain("do not decompose, block, or cancel"); + }); + + it("does not rewrite the description when the refresh observes the same trigger", async () => { + const now = new Date("2026-04-28T12:00:00.000Z"); + const seeded = await seedAssignedIssue(); + await insertRuns({ + companyId: seeded.companyId, + agentId: seeded.coderId, + issueId: seeded.issueId, + count: DEFAULT_PRODUCTIVITY_REVIEW_NO_COMMENT_STREAK_RUNS, + now, + }); + + const service = productivityReviewService(db); + await service.reconcileProductivityReviews({ now, companyId: seeded.companyId }); + const [review] = await listProductivityReviews(seeded.companyId); + const originalDescription = review!.description; + + const refreshAt = new Date(now.getTime() + DEFAULT_PRODUCTIVITY_REVIEW_REFRESH_INTERVAL_MS); + await insertRuns({ + companyId: seeded.companyId, + agentId: seeded.coderId, + issueId: seeded.issueId, + count: DEFAULT_PRODUCTIVITY_REVIEW_NO_COMMENT_STREAK_RUNS, + now: refreshAt, + }); + + const refresh = await service.reconcileProductivityReviews({ now: refreshAt, companyId: seeded.companyId }); + expect(refresh.updated).toBe(1); + + const [refreshedReview] = await listProductivityReviews(seeded.companyId); + expect(refreshedReview?.description).toBe(originalDescription); + }); + + it("still regenerates a stale description after the refresh-comment cap is reached, without posting another comment", async () => { + const now = new Date("2026-04-28T12:00:00.000Z"); + const seeded = await seedAssignedIssue(); + await insertRuns({ + companyId: seeded.companyId, + agentId: seeded.coderId, + issueId: seeded.issueId, + count: DEFAULT_PRODUCTIVITY_REVIEW_NO_COMMENT_STREAK_RUNS, + now, + }); + + const service = productivityReviewService(db); + await service.reconcileProductivityReviews({ now, companyId: seeded.companyId }); + const [review] = await listProductivityReviews(seeded.companyId); + + // Exhaust the DEFAULT_PRODUCTIVITY_REVIEW_MAX_REFRESH_COMMENTS cap (3) + // with unchanged-trigger refreshes, exactly like the existing + // "caps refresh comments" coverage above. + let cursor = now; + for (let i = 0; i < DEFAULT_PRODUCTIVITY_REVIEW_MAX_REFRESH_COMMENTS; i += 1) { + cursor = new Date(cursor.getTime() + DEFAULT_PRODUCTIVITY_REVIEW_REFRESH_INTERVAL_MS); + await insertRuns({ + companyId: seeded.companyId, + agentId: seeded.coderId, + issueId: seeded.issueId, + count: DEFAULT_PRODUCTIVITY_REVIEW_NO_COMMENT_STREAK_RUNS, + now: cursor, + }); + await service.reconcileProductivityReviews({ now: cursor, companyId: seeded.companyId }); + } + expect(await listRefreshComments(review!.id)).toHaveLength(DEFAULT_PRODUCTIVITY_REVIEW_MAX_REFRESH_COMMENTS); + + // The cap is now hit. A further refresh that only repeats the same + // trigger should stay throttled... + const stillNoOpAt = new Date(cursor.getTime() + DEFAULT_PRODUCTIVITY_REVIEW_REFRESH_INTERVAL_MS); + const noOpRefresh = await service.reconcileProductivityReviews({ + now: stillNoOpAt, + companyId: seeded.companyId, + }); + expect(noOpRefresh.updated).toBe(0); + expect(noOpRefresh.existing).toBe(1); + + // ...but a trigger flip must still correct the Manager Decision block — + // the comment cap bounds comment churn, not correctness of the guidance. + const flipAt = new Date(stillNoOpAt.getTime() + DEFAULT_PRODUCTIVITY_REVIEW_REFRESH_INTERVAL_MS); + await insertRuns({ + companyId: seeded.companyId, + agentId: seeded.coderId, + issueId: seeded.issueId, + count: DEFAULT_PRODUCTIVITY_REVIEW_NO_COMMENT_STREAK_RUNS, + now: flipAt, + status: "failed", + livenessState: "failed", + usageJson: { inputTokens: 0, outputTokens: 0 }, + }); + const flipRefresh = await service.reconcileProductivityReviews({ now: flipAt, companyId: seeded.companyId }); + expect(flipRefresh.updated).toBe(1); + + const [afterFlip] = await listProductivityReviews(seeded.companyId); + expect(afterFlip?.description).toContain("Primary trigger: `runtime_failure_streak`"); + expect(afterFlip?.description).toContain("do not decompose, block, or cancel"); + // No new comment: the cap still bounds comment churn. + expect(await listRefreshComments(review!.id)).toHaveLength(DEFAULT_PRODUCTIVITY_REVIEW_MAX_REFRESH_COMMENTS); + }); + + it("preserves a description edited concurrently with a trigger-flip refresh instead of clobbering it", async () => { + const now = new Date("2026-04-28T12:00:00.000Z"); + const seeded = await seedAssignedIssue(); + await insertRuns({ + companyId: seeded.companyId, + agentId: seeded.coderId, + issueId: seeded.issueId, + count: DEFAULT_PRODUCTIVITY_REVIEW_NO_COMMENT_STREAK_RUNS, + now, + }); + + const service = productivityReviewService(db); + await service.reconcileProductivityReviews({ now, companyId: seeded.companyId }); + const [review] = await listProductivityReviews(seeded.companyId); + expect(review?.description).toContain("Primary trigger: `no_comment_streak`"); + + const refreshAt = new Date(now.getTime() + DEFAULT_PRODUCTIVITY_REVIEW_REFRESH_INTERVAL_MS); + await insertRuns({ + companyId: seeded.companyId, + agentId: seeded.coderId, + issueId: seeded.issueId, + count: DEFAULT_PRODUCTIVITY_REVIEW_NO_COMMENT_STREAK_RUNS, + now: refreshAt, + status: "failed", + livenessState: "failed", + usageJson: { inputTokens: 0, outputTokens: 0 }, + }); + + const concurrentlyEditedDescription = `${review!.description}\n\nManager note: escalating directly, do not overwrite.`; + + // Simulate a human editing the review issue's description directly + // (e.g. via the issues API) in the window between this refresh's outer + // read of `existing` and the transaction's guarded UPDATE. The + // advisory lock only serializes this refresh path against itself; it + // says nothing about a plain issue edit landing concurrently. + const originalTransaction = db.transaction.bind(db); + const transactionSpy = vi.spyOn(db, "transaction").mockImplementation( + (async (...args: Parameters) => { + await db + .update(issues) + .set({ description: concurrentlyEditedDescription }) + .where(eq(issues.id, review!.id)); + return originalTransaction(...args); + }) as typeof db.transaction, + ); + + try { + const refresh = await service.reconcileProductivityReviews({ now: refreshAt, companyId: seeded.companyId }); + // The refresh comment still gets appended — only the description + // overwrite lost the race. + expect(refresh.updated).toBe(1); + } finally { + transactionSpy.mockRestore(); + } + + const [afterRefresh] = await listProductivityReviews(seeded.companyId); + expect(afterRefresh?.description).toBe(concurrentlyEditedDescription); + expect(afterRefresh?.description).not.toContain("Primary trigger: `runtime_failure_streak`"); + }); + }); + it("allows only one productivity review per source issue in 24 hours", async () => { const now = new Date("2026-04-28T12:00:00.000Z"); const seeded = await seedAssignedIssue(); diff --git a/server/src/services/productivity-review.ts b/server/src/services/productivity-review.ts index e7f910d476ed..30092b65c00e 100644 --- a/server/src/services/productivity-review.ts +++ b/server/src/services/productivity-review.ts @@ -722,6 +722,25 @@ function formatTrigger(trigger: ProductivityReviewTrigger) { return "Long active duration"; } +const PRODUCTIVITY_REVIEW_TRIGGERS: readonly ProductivityReviewTrigger[] = [ + "no_comment_streak", + "long_active_duration", + "high_churn", + "runtime_failure_streak", +]; + +// BLO-22105: `buildReviewMarkdown` bakes the trigger that produced it into the +// `- Primary trigger:` line. Reading it back out of the persisted description +// (rather than, say, the last activity-log entry) means the comparison is +// against exactly what a reader currently sees, so a refresh regenerates +// precisely when the visible Manager Decision guidance is actually stale. +function extractReviewTriggerFromDescription(description: string | null): ProductivityReviewTrigger | null { + if (!description) return null; + const match = description.match(/^- Primary trigger: `([a-z_]+)`/m); + const candidate = match?.[1]; + return PRODUCTIVITY_REVIEW_TRIGGERS.find((trigger) => trigger === candidate) ?? null; +} + // True when a run's most recent classification is `failed` liveness AND it // burned zero input+output tokens. That combination means the agent never // got a model turn — the runtime crashed, the process was killed, or every @@ -2567,20 +2586,59 @@ export function productivityReviewService(db: Db, deps?: ProductivityReviewServi const refreshState = await getRefreshCommentState(evidence.sourceIssue.companyId, existing.id, tx); const lastRefreshAt = refreshState.latestCreatedAt ?? existing.createdAt; - if ( - refreshState.count >= opts.thresholds.maxRefreshComments || - evidence.generatedAt.getTime() - lastRefreshAt.getTime() < effectiveRefreshIntervalMs - ) { + // The hard-floor interval gates everything below, including the + // description rewrite — a trigger flip must not be usable to force + // more writes than a normal refresh already allows. + if (evidence.generatedAt.getTime() - lastRefreshAt.getTime() < effectiveRefreshIntervalMs) { return { throttled: true as const, lastRefreshAt }; } - await addRefreshComment( - existing.id, - buildRefreshComment(evidence, opts.prefix), - evidence.generatedAt, - tx, - ); - return { throttled: false as const, lastRefreshAt }; + // BLO-22105: the Manager Decision block is trigger-conditional (see + // buildReviewMarkdown), so a review whose live trigger has flipped since + // it was created/last regenerated is showing stale — potentially + // under-enforcing — remedy guidance. Regenerate only on an actual flip + // (never on unparseable/legacy descriptions) and only inside this same + // throttle-gated branch, so a trigger flip cannot be used to force a + // description write more often than the hard-floor interval allows. + const previousTrigger = extractReviewTriggerFromDescription(existing.description); + const descriptionStale = previousTrigger !== null && previousTrigger !== evidence.trigger; + let descriptionRegenerated = false; + if (descriptionStale) { + // `existing.description` was read outside this transaction. The + // advisory lock only serializes this refresh path against itself — + // it says nothing about a human editing the review issue's + // description directly in between. Guard the overwrite with the + // description we actually read so a concurrent edit loses the race + // cleanly (0 rows matched, nothing clobbered) instead of being + // silently discarded. + const [updatedRow] = await tx + .update(issues) + .set({ description: buildReviewMarkdown(evidence, opts.prefix), updatedAt: evidence.generatedAt }) + .where(and(eq(issues.id, existing.id), eq(issues.description, existing.description as string))) + .returning({ id: issues.id }); + descriptionRegenerated = updatedRow !== undefined; + } + + // `maxRefreshComments` bounds refresh-comment churn, not the + // correctness of the durable Manager Decision guidance. Gating the + // description rewrite on it too would mean a review that outlives the + // cap could never self-correct after a trigger flip — exactly the + // staleness this fix exists to close. Only the comment emission is + // capped; the interval check above still applies to both. + const commentCapped = refreshState.count >= opts.thresholds.maxRefreshComments; + if (!commentCapped) { + await addRefreshComment( + existing.id, + buildRefreshComment(evidence, opts.prefix), + evidence.generatedAt, + tx, + ); + } + + if (commentCapped && !descriptionRegenerated) { + return { throttled: true as const, lastRefreshAt }; + } + return { throttled: false as const, lastRefreshAt, descriptionRegenerated }; }); if (refreshOutcome.throttled) { @@ -2591,7 +2649,7 @@ export function productivityReviewService(db: Db, deps?: ProductivityReviewServi lastRefreshAt: refreshOutcome.lastRefreshAt.toISOString(), minIntervalMs: effectiveRefreshIntervalMs, }, - "productivity review refresh throttled: previous refresh within hard-floor window", + "productivity review refresh throttled: within hard-floor window or comment cap reached with no stale description to fix", ); return { kind: "existing" as const, reviewIssueId: existing.id }; } @@ -2610,6 +2668,7 @@ export function productivityReviewService(db: Db, deps?: ProductivityReviewServi noCommentStreak: evidence.noCommentStreak, runCountLastHour: evidence.runCountLastHour, commentCountLastHour: evidence.commentCountLastHour, + descriptionRegenerated: refreshOutcome.descriptionRegenerated, }, }); return { kind: "updated" as const, reviewIssueId: existing.id };