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 <noreply@anthropic.com>
This commit is contained in:
parent
95fd47ef2a
commit
9e7a767730
|
|
@ -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", () => {
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
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,10 +444,28 @@ 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
|
||||
states.set(session.id, newStatus);
|
||||
|
|
|
|||
Loading…
Reference in New Issue