diff --git a/.changeset/fix-review-enrichment-double-billing.md b/.changeset/fix-review-enrichment-double-billing.md new file mode 100644 index 000000000..06cf75272 --- /dev/null +++ b/.changeset/fix-review-enrichment-double-billing.md @@ -0,0 +1,13 @@ +--- +"@aoagents/ao-core": patch +--- + +fix(core): prevent double-billing reaction attempts on changes_requested transition + +The enriched review dispatch in `maybeDispatchReviewBacklog` now sends directly via +`sessionManager.send` when the transition handler already called `executeReaction` for +the same reaction key. This prevents the attempt counter from incrementing twice in a +single poll cycle, which would cause premature escalation for projects with `retries: 1`. + +Also moves the review backlog throttle timestamp after the SCM fetch so a failed +`getReviewThreads` call doesn't block retries for 2 minutes. diff --git a/packages/core/src/__tests__/lifecycle-manager.test.ts b/packages/core/src/__tests__/lifecycle-manager.test.ts index bd5c82d66..1c4caad04 100644 --- a/packages/core/src/__tests__/lifecycle-manager.test.ts +++ b/packages/core/src/__tests__/lifecycle-manager.test.ts @@ -1998,7 +1998,7 @@ describe("reactions", () => { expect(metadata?.["lastPendingReviewDispatchHash"]).toBe("c1"); }); - it("does not double-send when changes_requested transition already triggered the reaction", async () => { + it("sends enriched review content on changes_requested transition alongside the generic message", async () => { config.reactions = { "changes-requested": { auto: true, @@ -2052,12 +2052,171 @@ describe("reactions", () => { }); await lm.check("app-1"); + + // First call is the transition reaction (generic message), second is + // the backlog dispatch with actual review comment content. + expect(mockSessionManager.send).toHaveBeenCalledTimes(2); + const enrichedMessage = vi.mocked(mockSessionManager.send).mock.calls[1]![1] as string; + expect(enrichedMessage).toContain("src/route.ts:44"); + expect(enrichedMessage).toContain("@reviewer"); + expect(enrichedMessage).toContain("Please add validation"); + + // Second check: throttled (within REVIEW_BACKLOG_THROTTLE_MS window) and + // fingerprint already matches dispatch hash — neither path re-sends. + vi.mocked(mockSessionManager.send).mockClear(); + await lm.check("app-1"); + expect(mockSessionManager.send).not.toHaveBeenCalled(); + }); + + it("does not double-bill reaction attempts on changes_requested transition with retries:1", async () => { + const notifier = createMockNotifier(); + + config.reactions = { + "changes-requested": { + auto: true, + action: "send-to-agent", + message: "Handle requested changes.", + retries: 1, + }, + }; + + const mockSCM = createMockSCM({ + getReviewDecision: vi.fn().mockResolvedValue("changes_requested"), + enrichSessionsPRBatch: vi.fn().mockImplementation(async (prs: PRInfo[]) => { + const result = new Map(); + for (const pr of prs) { + result.set(`${pr.owner}/${pr.repo}#${pr.number}`, { + state: "open", + ciStatus: "passing", + reviewDecision: "changes_requested", + mergeable: false, + }); + } + return result; + }), + getReviewThreads: vi.fn().mockResolvedValue({ + threads: [ + { + id: "c1", + author: "reviewer", + body: "Needs validation", + path: "src/handler.ts", + line: 10, + isResolved: false, + createdAt: new Date(), + url: "https://example.com/comment/retries", + isBot: false, + }, + ], + reviews: [], + }), + }); + + const registry: PluginRegistry = { + ...mockRegistry, + get: vi.fn().mockImplementation((slot: string, name: string) => { + if (slot === "runtime") return plugins.runtime; + if (slot === "agent") return plugins.agent; + if (slot === "scm") return mockSCM; + if (slot === "notifier" && name === "desktop") return notifier; + return null; + }), + }; + + vi.mocked(mockSessionManager.send).mockResolvedValue(undefined); + + const lm = setupCheck("app-1", { + session: makeSession({ status: "pr_open", pr: makePR() }), + registry, + }); + await lm.check("app-1"); - // First call is the transition reaction (static message), second would be - // the review backlog dispatch. But the changes_requested transition guard - // prevents double-send, so only 1 call total. - expect(mockSessionManager.send).toHaveBeenCalledTimes(1); + // Transition handler sends the generic message (attempt 1), and the backlog + // dispatch sends the enriched message directly (no attempt increment). + // Total sends = 2 but reaction attempts = 1, so no escalation. + expect(mockSessionManager.send).toHaveBeenCalledTimes(2); + expect(notifier.notify).not.toHaveBeenCalledWith( + expect.objectContaining({ type: "reaction.escalated" }), + ); + + // The enriched message should contain the actual review content + const enrichedMessage = vi.mocked(mockSessionManager.send).mock.calls[1]![1] as string; + expect(enrichedMessage).toContain("src/handler.ts:10"); + expect(enrichedMessage).toContain("Needs validation"); + }); + + it("routes enriched review dispatch through executeReaction when action is notify (not send-to-agent)", async () => { + const notifier = createMockNotifier(); + + config.reactions = { + "changes-requested": { + auto: true, + action: "notify", + message: "Review changes requested.", + }, + }; + config.notificationRouting = { + ...config.notificationRouting, + info: ["desktop"], + }; + + const mockSCM = createMockSCM({ + getReviewDecision: vi.fn().mockResolvedValue("changes_requested"), + enrichSessionsPRBatch: vi.fn().mockImplementation(async (prs: PRInfo[]) => { + const result = new Map(); + for (const pr of prs) { + result.set(`${pr.owner}/${pr.repo}#${pr.number}`, { + state: "open", + ciStatus: "passing", + reviewDecision: "changes_requested", + mergeable: false, + }); + } + return result; + }), + getReviewThreads: vi.fn().mockResolvedValue({ + threads: [ + { + id: "c1", + author: "reviewer", + body: "Fix the type", + path: "src/api.ts", + line: 5, + isResolved: false, + createdAt: new Date(), + url: "https://example.com/comment/notify", + isBot: false, + }, + ], + reviews: [], + }), + }); + + const registry: PluginRegistry = { + ...mockRegistry, + get: vi.fn().mockImplementation((slot: string, name: string) => { + if (slot === "runtime") return plugins.runtime; + if (slot === "agent") return plugins.agent; + if (slot === "scm") return mockSCM; + if (slot === "notifier" && name === "desktop") return notifier; + return null; + }), + }; + + vi.mocked(mockSessionManager.send).mockResolvedValue(undefined); + + const lm = setupCheck("app-1", { + session: makeSession({ status: "pr_open", pr: makePR() }), + registry, + }); + + await lm.check("app-1"); + + // action: "notify" should NOT send to the agent — it routes through + // executeReaction → notifyHuman. The bypass branch must not fire. + expect(mockSessionManager.send).not.toHaveBeenCalled(); + expect(notifier.notify).toHaveBeenCalled(); }); it("dispatches detailed automated review comments when using the default sentinel message", async () => { diff --git a/packages/core/src/config.ts b/packages/core/src/config.ts index 7d03ac4ea..57a236bf5 100644 --- a/packages/core/src/config.ts +++ b/packages/core/src/config.ts @@ -676,7 +676,7 @@ function applyDefaultReactions(config: OrchestratorConfig): OrchestratorConfig { auto: true, action: "send-to-agent", message: - "There are review comments on your PR. Details will follow shortly.", + "There are new review comments on your PR requesting changes.", escalateAfter: "30m", }, "bugbot-comments": { diff --git a/packages/core/src/lifecycle-manager.ts b/packages/core/src/lifecycle-manager.ts index d6c3f6a26..fdb8a3918 100644 --- a/packages/core/src/lifecycle-manager.ts +++ b/packages/core/src/lifecycle-manager.ts @@ -1439,7 +1439,7 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan async function maybeDispatchReviewBacklog( session: Session, - oldStatus: SessionStatus, + _oldStatus: SessionStatus, newStatus: SessionStatus, transitionReaction?: { key: string; result: ReactionResult | null }, ): Promise { @@ -1472,11 +1472,10 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan // (getReviewThreads) consumes API quota on every poll. // // Exception: bypass throttle when a transition reaction just fired for a - // review reaction key. The transitionReaction branch records - // lastPendingReviewDispatchHash, which requires the current fingerprint from - // the API. If we throttle here, that metadata never gets written and the - // next unthrottled poll sees a "new" fingerprint, clears the reaction tracker, - // and fires a duplicate dispatch. + // review reaction key. The enriched dispatch needs the current fingerprint + // from the API so it can fire and record the hash in the same cycle. If we + // throttle here, the next unthrottled poll sees a "new" fingerprint, clears + // the reaction tracker, and fires a duplicate dispatch. const hasRelevantTransition = transitionReaction?.key === humanReactionKey || transitionReaction?.key === automatedReactionKey; @@ -1486,11 +1485,9 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan return; } } - lastReviewBacklogCheckAt.set(session.id, Date.now()); - // Single GraphQL call for all review threads (human + bot) + review summaries. // Split locally by isBot for separate reaction pipelines. - let allThreads: ReviewComment[] | null = null; + let allThreads: ReviewComment[]; let reviewSummaries: ReviewSummary[] = []; try { if (scm.getReviewThreads) { @@ -1502,11 +1499,18 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan allThreads = await scm.getPendingComments(session.pr); } } catch { - // Failed to fetch — preserve existing metadata + // Failed to fetch — preserve existing metadata. + // Don't update the throttle timestamp so the next poll retries immediately + // instead of being blocked for 2 minutes with the agent left on a bare notification. + return; } + // Only stamp the throttle after a successful SCM fetch. If the fetch failed, + // we returned above so the next poll can retry without waiting 2 minutes. + lastReviewBacklogCheckAt.set(session.id, Date.now()); + // Persist review comments + summaries to metadata for dashboard consumption - if (allThreads !== null) { + { const unresolved = allThreads.filter((c) => !c.isBot); const reviewBlob = JSON.stringify({ unresolvedThreads: unresolved.length, @@ -1528,12 +1532,11 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan } } - const pendingComments = allThreads ? allThreads.filter((c) => !c.isBot) : null; - const automatedComments = allThreads ? allThreads.filter((c) => c.isBot) : null; + const pendingComments = allThreads.filter((c) => !c.isBot); + const automatedComments = allThreads.filter((c) => c.isBot); // --- Pending (human) review comments --- - // null = SCM fetch failed; skip processing to preserve existing metadata. - if (pendingComments !== null) { + { const pendingFingerprint = makeFingerprint(pendingComments.map((comment) => comment.id)); const lastPendingFingerprint = session.metadata["lastPendingReviewFingerprint"] ?? ""; const lastPendingDispatchHash = session.metadata["lastPendingReviewDispatchHash"] ?? ""; @@ -1557,32 +1560,42 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan lastPendingReviewDispatchHash: "", lastPendingReviewDispatchAt: "", }); - } else if ( - transitionReaction?.key === humanReactionKey && - transitionReaction.result?.success - ) { - if (lastPendingDispatchHash !== pendingFingerprint) { - updateSessionMetadata(session, { - lastPendingReviewDispatchHash: pendingFingerprint, - lastPendingReviewDispatchAt: new Date().toISOString(), - }); - } - } else if ( - !(oldStatus !== newStatus && newStatus === "changes_requested") && - pendingFingerprint !== lastPendingDispatchHash - ) { + } else if (pendingFingerprint !== lastPendingDispatchHash) { const reactionConfig = getReactionConfigForSession(session, humanReactionKey); if ( reactionConfig && reactionConfig.action && (reactionConfig.auto !== false || reactionConfig.action === "notify") ) { - const enrichedConfig = { - ...reactionConfig, - message: formatReviewCommentsMessage(pendingComments, "reviewer", reviewSummaries), - }; - const result = await executeReaction(session, humanReactionKey, enrichedConfig); - if (result.success) { + const enrichedMessage = formatReviewCommentsMessage( + pendingComments, + "reviewer", + reviewSummaries, + ); + + // When the transition handler already called executeReaction for this + // key, send the enriched payload directly to avoid double-billing the + // reaction attempt budget. A project with retries:1 would otherwise + // escalate on the very first transition poll. + // Only bypass for "send-to-agent" — "notify" actions must go through + // executeReaction so they route to notifyHuman instead of the agent. + let success = false; + if ( + transitionReaction?.key === humanReactionKey && + reactionConfig.action === "send-to-agent" + ) { + try { + await sessionManager.send(session.id, enrichedMessage); + success = true; + } catch { + // Send failed — will retry on next unthrottled poll + } + } else { + const enrichedConfig = { ...reactionConfig, message: enrichedMessage }; + const result = await executeReaction(session, humanReactionKey, enrichedConfig); + success = result.success; + } + if (success) { updateSessionMetadata(session, { lastPendingReviewDispatchHash: pendingFingerprint, lastPendingReviewDispatchAt: new Date().toISOString(), @@ -1593,7 +1606,7 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan } // --- Automated (bot) review comments --- - if (automatedComments !== null) { + { const automatedFingerprint = makeFingerprint(automatedComments.map((comment) => comment.id)); const lastAutomatedFingerprint = session.metadata["lastAutomatedReviewFingerprint"] ?? ""; const lastAutomatedDispatchHash = session.metadata["lastAutomatedReviewDispatchHash"] ?? ""; @@ -1619,14 +1632,25 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan reactionConfig.action && (reactionConfig.auto !== false || reactionConfig.action === "notify") ) { - // Always enrich with formatted comment listing so the agent has inline - // data and doesn't need to re-fetch via gh api. - const enrichedConfig = { - ...reactionConfig, - message: formatReviewCommentsMessage(automatedComments, "bot"), - }; - const result = await executeReaction(session, automatedReactionKey, enrichedConfig); - if (result.success) { + const enrichedMessage = formatReviewCommentsMessage(automatedComments, "bot"); + + let success = false; + if ( + transitionReaction?.key === automatedReactionKey && + reactionConfig.action === "send-to-agent" + ) { + try { + await sessionManager.send(session.id, enrichedMessage); + success = true; + } catch { + // Send failed — will retry on next unthrottled poll + } + } else { + const enrichedConfig = { ...reactionConfig, message: enrichedMessage }; + const result = await executeReaction(session, automatedReactionKey, enrichedConfig); + success = result.success; + } + if (success) { updateSessionMetadata(session, { lastAutomatedReviewDispatchHash: automatedFingerprint, lastAutomatedReviewDispatchAt: new Date().toISOString(),