From 4028e5b77db889597e751f5172b0545c1674b6ff Mon Sep 17 00:00:00 2001 From: Prateek Date: Sat, 14 Feb 2026 04:07:38 +0530 Subject: [PATCH] fix: suppress immediate notification when send-to-agent reaction handles event MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a send-to-agent reaction was configured (e.g. for ci-failed), the reactionHandledNotify flag stayed false, causing checkSession() to also call notifyHuman() immediately — bypassing the retry/escalation workflow. Co-Authored-By: Claude Opus 4.6 --- .../src/__tests__/lifecycle-manager.test.ts | 73 +++++++++++++++++++ packages/core/src/lifecycle-manager.ts | 9 ++- 2 files changed, 78 insertions(+), 4 deletions(-) diff --git a/packages/core/src/__tests__/lifecycle-manager.test.ts b/packages/core/src/__tests__/lifecycle-manager.test.ts index 4d0b43318..b8b67e158 100644 --- a/packages/core/src/__tests__/lifecycle-manager.test.ts +++ b/packages/core/src/__tests__/lifecycle-manager.test.ts @@ -601,6 +601,79 @@ describe("reactions", () => { expect(mockSessionManager.send).not.toHaveBeenCalled(); }); + it("suppresses immediate notification when send-to-agent reaction handles the event", async () => { + const mockNotifier: Notifier = { + name: "mock-notifier", + notify: vi.fn().mockResolvedValue(undefined), + }; + + const mockSCM: SCM = { + name: "mock-scm", + detectPR: vi.fn(), + getPRState: vi.fn().mockResolvedValue("open"), + mergePR: vi.fn(), + closePR: vi.fn(), + getCIChecks: vi.fn(), + getCISummary: vi.fn().mockResolvedValue("failing"), + getReviews: vi.fn(), + getReviewDecision: vi.fn(), + getPendingComments: vi.fn(), + getAutomatedComments: vi.fn(), + getMergeability: vi.fn(), + }; + + const registryWithNotifier: PluginRegistry = { + ...mockRegistry, + get: vi.fn().mockImplementation((slot: string, name: string) => { + if (slot === "runtime") return mockRuntime; + if (slot === "agent") return mockAgent; + if (slot === "scm") return mockSCM; + if (slot === "notifier" && name === "desktop") return mockNotifier; + return null; + }), + }; + + // Session transitions from pr_open → ci_failed, which maps to ci-failed reaction + const session = makeSession({ status: "pr_open", pr: makePR() }); + vi.mocked(mockSessionManager.get).mockResolvedValue(session); + vi.mocked(mockSessionManager.send).mockResolvedValue(undefined); + + writeMetadata(dataDir, "app-1", { + worktree: "/tmp", + branch: "main", + status: "pr_open", + project: "my-app", + }); + + // Configure send-to-agent reaction for ci-failed with retries + const configWithReaction = { + ...config, + reactions: { + "ci-failed": { + auto: true, + action: "send-to-agent" as const, + message: "Fix CI", + retries: 3, + escalateAfter: 3, + }, + }, + }; + + const lm = createLifecycleManager({ + config: configWithReaction, + registry: registryWithNotifier, + sessionManager: mockSessionManager, + }); + + await lm.check("app-1"); + + expect(lm.getStates().get("app-1")).toBe("ci_failed"); + // send-to-agent reaction should have been executed + expect(mockSessionManager.send).toHaveBeenCalledWith("app-1", "Fix CI"); + // Notifier should NOT have been called — the reaction is handling it + expect(mockNotifier.notify).not.toHaveBeenCalled(); + }); + it("notifies humans on significant transitions without reaction config", async () => { const mockNotifier: Notifier = { name: "mock-notifier", diff --git a/packages/core/src/lifecycle-manager.ts b/packages/core/src/lifecycle-manager.ts index 28aa90fb4..ca3094a0d 100644 --- a/packages/core/src/lifecycle-manager.ts +++ b/packages/core/src/lifecycle-manager.ts @@ -444,10 +444,11 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan reactionKey, reactionConfig as ReactionConfig, ); - // "notify" and "auto-merge" reactions already call notifyHuman - if (reactionConfig.action === "notify" || reactionConfig.action === "auto-merge") { - reactionHandledNotify = true; - } + // Reaction is handling this event — suppress immediate human notification. + // "send-to-agent" retries + escalates on its own; "notify"/"auto-merge" + // already call notifyHuman internally. Notifying here would bypass the + // delayed escalation behaviour configured via retries/escalateAfter. + reactionHandledNotify = true; } } }