diff --git a/packages/coding-agent/src/modes/controllers/event-controller.ts b/packages/coding-agent/src/modes/controllers/event-controller.ts index 3366e527e..22c776068 100644 --- a/packages/coding-agent/src/modes/controllers/event-controller.ts +++ b/packages/coding-agent/src/modes/controllers/event-controller.ts @@ -1,4 +1,4 @@ -import type { ImageContent } from "@oh-my-pi/pi-ai"; +import type { AssistantMessage, ImageContent } from "@oh-my-pi/pi-ai"; import * as AIError from "@oh-my-pi/pi-ai/error"; import { getStreamingPartialJson } from "@oh-my-pi/pi-ai/utils/block-symbols"; import { type Component, Loader, TERMINAL } from "@oh-my-pi/pi-tui"; @@ -1060,7 +1060,7 @@ export class EventController { } } } - async #handleAgentEnd(_event: Extract): Promise { + async #handleAgentEnd(event: Extract): Promise { // A superseded agent_end: the agent is already streaming a fresh turn, so // this event belongs to a turn that has already been replaced. The session // dispatches to listeners fire-and-forget across an async extension-emit hop @@ -1072,10 +1072,10 @@ export class EventController { // then). Mirrors the collab guest's !isStreaming loader reconciler. if (this.ctx.session.isStreaming) return; - await this.#finishAgentEnd(); + await this.#finishAgentEnd(event); } - async #finishAgentEnd(): Promise { + async #finishAgentEnd(event: Extract): Promise { this.#setTerminalProgress(false); this.ctx.statusLine.markActivityEnd(); this.#streamingReveal.stop(); @@ -1120,7 +1120,7 @@ export class EventController { this.ctx.ui.requestRender(); this.#scheduleIdleCompaction(); this.#scheduleIdleRecap(); - this.sendErrorNotification(); + this.sendErrorNotification(event); this.sendCompletionNotification(); } @@ -1477,11 +1477,17 @@ export class EventController { return this.ctx.viewSession.getContextUsage()?.tokens ?? 0; } - sendErrorNotification(): void { + sendErrorNotification(event: Extract): void { const notify = settings.get("error.notify"); if (notify === "off") return; - const last = this.ctx.viewSession.getLastAssistantMessage?.(); + // Read the turn's own outcome from `agent_end.messages`, not the mutable + // active context: a classifier-refusal failure is final (stopReason === + // "error") but gets pruned from `viewSession`'s active context before this + // handler runs (see `#removeAssistantMessageFromActiveContext` in + // agent-session.ts), so `getLastAssistantMessage()` would see a stale or + // absent assistant and silently drop the notification. + const last = event.messages.findLast((message): message is AssistantMessage => message.role === "assistant"); if (last?.stopReason !== "error") return; const sessionName = this.ctx.sessionManager.getSessionName(); diff --git a/packages/coding-agent/test/modes/controllers/event-controller-abort-guard.test.ts b/packages/coding-agent/test/modes/controllers/event-controller-abort-guard.test.ts index 049d540a8..0be757fb5 100644 --- a/packages/coding-agent/test/modes/controllers/event-controller-abort-guard.test.ts +++ b/packages/coding-agent/test/modes/controllers/event-controller-abort-guard.test.ts @@ -19,6 +19,7 @@ import { SETTINGS_SCHEMA } from "@oh-my-pi/pi-coding-agent/config/settings-schem import { EventController } from "@oh-my-pi/pi-coding-agent/modes/controllers/event-controller"; import { initTheme } from "@oh-my-pi/pi-coding-agent/modes/theme/theme"; import type { InteractiveModeContext } from "@oh-my-pi/pi-coding-agent/modes/types"; +import type { AgentSessionEvent } from "@oh-my-pi/pi-coding-agent/session/agent-session"; import { TERMINAL } from "@oh-my-pi/pi-tui"; beforeAll(() => { @@ -61,6 +62,37 @@ function makeContext(lastMessage: AssistantMessage | undefined): InteractiveMode } as unknown as InteractiveModeContext; } +function makeAgentEndEvent(messages: AssistantMessage[]): Extract { + return { type: "agent_end", messages } as Extract; +} + +/** Full context needed to drive `#handleAgentEnd` -> `#finishAgentEnd` end to end. */ +function makeTurnEndContext(options: { lastAssistantMessage?: AssistantMessage } = {}): InteractiveModeContext { + const session = { + isStreaming: false, + isCompacting: false, + messages: [] as AssistantMessage[], + getLastAssistantMessage: () => options.lastAssistantMessage, + getContextUsage: () => undefined, + }; + return { + isInitialized: true, + loadingAnimation: undefined, + streamingComponent: undefined, + streamingMessage: undefined, + pendingTools: new Map(), + flushPendingModelSwitch: async () => {}, + ui: { requestRender: () => {} }, + chatContainer: { removeChild: () => {} }, + statusContainer: { clear: () => {} }, + statusLine: { markActivityEnd: () => {} }, + editor: { getText: () => "" }, + sessionManager: { getSessionName: () => "test-session" }, + session, + viewSession: session, + } as unknown as InteractiveModeContext; +} + describe("EventController.sendCompletionNotification — abort guard", () => { it("skips notification when the last assistant message stopReason === 'aborted'", () => { const spy = vi.spyOn(TERMINAL, "sendNotification").mockImplementation(() => {}); @@ -114,21 +146,45 @@ describe("EventController.sendErrorNotification", () => { it("fires an error notification when stopReason === 'error'", () => { const spy = vi.spyOn(TERMINAL, "sendNotification").mockImplementation(() => {}); settings.override("error.notify", "on"); - const controller = new EventController(makeContext(makeAssistantMessage("error"))); - controller.sendErrorNotification(); + const controller = new EventController(makeContext(undefined)); + controller.sendErrorNotification(makeAgentEndEvent([makeAssistantMessage("error")])); expect(spy).toHaveBeenCalledTimes(1); expect(spy).toHaveBeenCalledWith( expect.objectContaining({ body: "Stopped with error", type: "error", title: "test-session" }), ); }); + it("reads the terminal turn from agent_end.messages, not the mutable active context", () => { + // A classifier-refusal failure is pruned from `viewSession`'s active + // context before `agent_end` fires (`#removeAssistantMessageFromActiveContext` + // in agent-session.ts), so `viewSession.getLastAssistantMessage()` here + // reflects a stale, non-error turn. The notification must still fire off + // the event's own `messages`, not that stale snapshot. + const spy = vi.spyOn(TERMINAL, "sendNotification").mockImplementation(() => {}); + settings.override("error.notify", "on"); + const controller = new EventController(makeContext(makeAssistantMessage("stop"))); + controller.sendErrorNotification(makeAgentEndEvent([makeAssistantMessage("error")])); + expect(spy).toHaveBeenCalledTimes(1); + expect(spy).toHaveBeenCalledWith(expect.objectContaining({ body: "Stopped with error", type: "error" })); + }); + + it("uses the last assistant message when agent_end carries multiple messages", () => { + const spy = vi.spyOn(TERMINAL, "sendNotification").mockImplementation(() => {}); + settings.override("error.notify", "on"); + const controller = new EventController(makeContext(undefined)); + controller.sendErrorNotification( + makeAgentEndEvent([makeAssistantMessage("stop"), makeAssistantMessage("error")]), + ); + expect(spy).toHaveBeenCalledTimes(1); + }); + it("honors error.notify=off without changing completion notifications", () => { const spy = vi.spyOn(TERMINAL, "sendNotification").mockImplementation(() => {}); settings.override("error.notify", "off"); settings.override("completion.notify", "on"); - const errorController = new EventController(makeContext(makeAssistantMessage("error"))); - errorController.sendErrorNotification(); + const errorController = new EventController(makeContext(undefined)); + errorController.sendErrorNotification(makeAgentEndEvent([makeAssistantMessage("error")])); expect(spy).toHaveBeenCalledTimes(0); const completionController = new EventController(makeContext(makeAssistantMessage("stop"))); @@ -140,16 +196,45 @@ describe("EventController.sendErrorNotification", () => { it("skips user-aborted turns", () => { const spy = vi.spyOn(TERMINAL, "sendNotification").mockImplementation(() => {}); settings.override("error.notify", "on"); - const controller = new EventController(makeContext(makeAssistantMessage("aborted"))); - controller.sendErrorNotification(); + const controller = new EventController(makeContext(undefined)); + controller.sendErrorNotification(makeAgentEndEvent([makeAssistantMessage("aborted")])); expect(spy).toHaveBeenCalledTimes(0); }); it("skips normal completion turns", () => { const spy = vi.spyOn(TERMINAL, "sendNotification").mockImplementation(() => {}); settings.override("error.notify", "on"); - const controller = new EventController(makeContext(makeAssistantMessage("stop"))); - controller.sendErrorNotification(); + const controller = new EventController(makeContext(undefined)); + controller.sendErrorNotification(makeAgentEndEvent([makeAssistantMessage("stop")])); expect(spy).toHaveBeenCalledTimes(0); }); }); + +describe("EventController — error notification through the real turn-end path (#handleAgentEnd)", () => { + beforeEach(() => { + // Isolate the error-notification assertion from the pre-existing + // completion-notification side effect that also fires from + // `#finishAgentEnd` on every settled turn. + settings.override("completion.notify", "off"); + }); + + it("fires when the dispatched turn settles with stopReason === 'error', even with a stale active-context snapshot", async () => { + const spy = vi.spyOn(TERMINAL, "sendNotification").mockImplementation(() => {}); + settings.override("error.notify", "on"); + // viewSession (active context) reports no assistant at all — the shape a + // classifier-refusal prune leaves behind — while the terminal agent_end + // event still carries the failed turn. + const controller = new EventController(makeTurnEndContext({ lastAssistantMessage: undefined })); + await controller.handleEvent(makeAgentEndEvent([makeAssistantMessage("error")])); + expect(spy).toHaveBeenCalledTimes(1); + expect(spy).toHaveBeenCalledWith(expect.objectContaining({ body: "Stopped with error", type: "error" })); + }); + + it("skips notification when the dispatched turn settles with stopReason === 'aborted'", async () => { + const spy = vi.spyOn(TERMINAL, "sendNotification").mockImplementation(() => {}); + settings.override("error.notify", "on"); + const controller = new EventController(makeTurnEndContext()); + await controller.handleEvent(makeAgentEndEvent([makeAssistantMessage("aborted")])); + expect(spy).not.toHaveBeenCalled(); + }); +});