bkinnightskytw
/
agent-orchestrator
mirror of https://github.com/ComposioHQ/agent-orchestrator.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

fix(core): deliver enriched review content on changes_requested transition (#1578) 

* fix(core): deliver enriched review content on changes_requested transition

The transition reaction for changes_requested sent a generic message
("Details will follow shortly") but the backlog dispatch that carries
the actual review comment bodies was blocked by its own deduplication
logic — the transition handler recorded the fingerprint hash as
"dispatched" without ever sending the enriched content.

Remove the premature hash recording and the transition guard so the
backlog dispatch fires in the same poll cycle, delivering actual review
comment details (file paths, line numbers, authors, bodies) to the
agent immediately.

Closes #1558

* fix(core): update stale comments from review feedback

- Update throttle-bypass comment to reflect removed Branch B
- Fix test comment: second check is throttled, not just fingerprint match

* fix(core): prevent double-billing reaction attempts on changes_requested transition

When a changes_requested transition fires, the transition handler calls
executeReaction (attempt 1) and then maybeDispatchReviewBacklog calls it
again for the enriched message (attempt 2). With retries:1, this caused
premature escalation on the very first transition poll.

Fix: when the transition handler already fired executeReaction for the
same reaction key, send the enriched payload directly via
sessionManager.send — bypassing the reaction tracker entirely.

Also moves lastReviewBacklogCheckAt after the SCM fetch so a failed
getReviewThreads call doesn't block retries for 2 minutes.

Fixes #1578

* fix(core): gate review bypass on send-to-agent action type

The direct sessionManager.send bypass (introduced to prevent double-billing
reaction attempts) fired unconditionally, ignoring reactionConfig.action.
With action: "notify", the enriched review content was pushed to the agent's
stdin instead of routing through notifyHuman.

Gate the bypass on action === "send-to-agent" so notify configs fall through
to executeReaction which routes correctly. Applied to both human and
automated review comment paths.

Adds test verifying action: "notify" does not call sessionManager.send
and does fire the notifier.

Fixes #1578
This commit is contained in:
Adil Shaikh 2026-05-03 18:30:47 +05:30 committed by GitHub
parent cf5a418a48
commit 703d5844f0
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 246 additions and 50 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
.changeset/fix-review-enrichment-double-billing.md Normal file
View File

@ -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.

169
packages/core/src/__tests__/lifecycle-manager.test.ts
View File

@ -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 () => {

2
packages/core/src/config.ts
View File

@ -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": {

112
packages/core/src/lifecycle-manager.ts
View File

@ -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<void> {
@ -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(),