From 9e7a7677309ede5bf83431d0b49b3366b376fc65 Mon Sep 17 00:00:00 2001 From: Prateek Date: Sat, 14 Feb 2026 03:55:19 +0530 Subject: [PATCH] fix: notify on significant transitions, preserve stuck state on probe failure Two bugs fixed: 1. Transitions with non-info priority (e.g. merge.completed, review.approved, session.errored) now call notifyHuman() directly when no reaction config handles notification. Before, these transitions updated state silently with no human notification. 2. When agent.detectActivity() throws, sessions in stuck/needs_input state now preserve that state instead of being coerced to "working" by the fallback at the bottom of determineStatus(). Co-Authored-By: Claude Opus 4.6 --- .../src/__tests__/lifecycle-manager.test.ts | 109 ++++++++++++++++++ packages/core/src/lifecycle-manager.ts | 38 +++++- 2 files changed, 142 insertions(+), 5 deletions(-) diff --git a/packages/core/src/__tests__/lifecycle-manager.test.ts b/packages/core/src/__tests__/lifecycle-manager.test.ts index 65d1e9b99..4d0b43318 100644 --- a/packages/core/src/__tests__/lifecycle-manager.test.ts +++ b/packages/core/src/__tests__/lifecycle-manager.test.ts @@ -13,6 +13,7 @@ import type { Runtime, Agent, SCM, + Notifier, ActivityState, PRInfo, } from "../types.js"; @@ -253,6 +254,56 @@ describe("check (single session)", () => { expect(lm.getStates().get("app-1")).toBe("needs_input"); }); + it("preserves stuck state when detectActivity throws", async () => { + vi.mocked(mockAgent.detectActivity).mockRejectedValue(new Error("probe failed")); + + const session = makeSession({ status: "stuck" }); + vi.mocked(mockSessionManager.get).mockResolvedValue(session); + + writeMetadata(dataDir, "app-1", { + worktree: "/tmp", + branch: "main", + status: "stuck", + project: "my-app", + }); + + const lm = createLifecycleManager({ + config, + registry: mockRegistry, + sessionManager: mockSessionManager, + }); + + await lm.check("app-1"); + + // Should preserve "stuck" — NOT coerce to "working" + expect(lm.getStates().get("app-1")).toBe("stuck"); + }); + + it("preserves needs_input state when detectActivity throws", async () => { + vi.mocked(mockAgent.detectActivity).mockRejectedValue(new Error("probe failed")); + + const session = makeSession({ status: "needs_input" }); + vi.mocked(mockSessionManager.get).mockResolvedValue(session); + + writeMetadata(dataDir, "app-1", { + worktree: "/tmp", + branch: "main", + status: "needs_input", + project: "my-app", + }); + + const lm = createLifecycleManager({ + config, + registry: mockRegistry, + sessionManager: mockSessionManager, + }); + + await lm.check("app-1"); + + // Should preserve "needs_input" — NOT coerce to "working" + expect(lm.getStates().get("app-1")).toBe("needs_input"); + }); + it("detects PR states from SCM", async () => { const mockSCM: SCM = { name: "mock-scm", @@ -550,6 +601,64 @@ describe("reactions", () => { expect(mockSessionManager.send).not.toHaveBeenCalled(); }); + it("notifies humans on significant transitions without reaction config", 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("merged"), + mergePR: vi.fn(), + closePR: vi.fn(), + getCIChecks: vi.fn(), + getCISummary: vi.fn(), + 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; + }), + }; + + // merge.completed has "action" priority but NO reaction key mapping, + // so it must reach notifyHuman directly + const session = makeSession({ status: "approved", pr: makePR() }); + vi.mocked(mockSessionManager.get).mockResolvedValue(session); + + writeMetadata(dataDir, "app-1", { + worktree: "/tmp", + branch: "main", + status: "approved", + project: "my-app", + }); + + const lm = createLifecycleManager({ + config, + registry: registryWithNotifier, + sessionManager: mockSessionManager, + }); + + await lm.check("app-1"); + + expect(lm.getStates().get("app-1")).toBe("merged"); + expect(mockNotifier.notify).toHaveBeenCalled(); + expect(mockNotifier.notify).toHaveBeenCalledWith( + expect.objectContaining({ type: "merge.completed" }), + ); + }); }); describe("getStates", () => { diff --git a/packages/core/src/lifecycle-manager.ts b/packages/core/src/lifecycle-manager.ts index 5808da50b..28aa90fb4 100644 --- a/packages/core/src/lifecycle-manager.ts +++ b/packages/core/src/lifecycle-manager.ts @@ -192,10 +192,18 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan // 2. Check agent activity if (agent) { - const activity = await agent.detectActivity(session).catch(() => session.activity); - if (activity === "exited") return "killed"; - if (activity === "blocked") return "stuck"; - if (activity === "waiting_input") return "needs_input"; + try { + const activity = await agent.detectActivity(session); + if (activity === "exited") return "killed"; + if (activity === "blocked") return "stuck"; + if (activity === "waiting_input") return "needs_input"; + } catch { + // On probe failure, preserve current stuck/needs_input state rather + // than letting the fallback at the bottom coerce them to "working" + if (session.status === "stuck" || session.status === "needs_input") { + return session.status; + } + } } // 3. Check PR state if PR exists @@ -412,10 +420,12 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan } } - // Check if there's a reaction for this transition + // Handle transition: notify humans and/or trigger reactions const eventType = statusToEventType(oldStatus, newStatus); if (eventType) { + let reactionHandledNotify = false; const reactionKey = eventToReactionKey(eventType); + if (reactionKey) { // Merge project-specific overrides with global defaults const project = config.projects[session.projectId]; @@ -434,9 +444,27 @@ 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; + } } } } + + // For significant transitions not already notified by a reaction, notify humans + if (!reactionHandledNotify) { + const priority = inferPriority(eventType); + if (priority !== "info") { + const event = createEvent(eventType, { + sessionId: session.id, + projectId: session.projectId, + message: `${session.id}: ${oldStatus} → ${newStatus}`, + data: { oldStatus, newStatus }, + }); + await notifyHuman(event, priority); + } + } } } else { // No transition but track current state