smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(coding-agent): read the settled turn from agent_end for error notifications

Browse Source 
Classifier-refusal failures end a turn with stopReason === "error" but
get pruned from the active context (agent-session.ts's
#removeAssistantMessageFromActiveContext) before agent_end fires.
sendErrorNotification() read viewSession.getLastAssistantMessage(),
which reflects that mutated context and silently missed the
notification for exactly the turns it should fire on.

Thread the agent_end event through #handleAgentEnd -> #finishAgentEnd
so sendErrorNotification reads the turn's own outcome from
agent_end.messages instead.
This commit is contained in:
Mathews-Tom
2026-07-06 04:02:40 +05:30
parent b95155423a
commit 90527a5ae4
2 changed files with 106 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/coding-agent/src/modes/controllers/event-controller.ts
+13 -7
View File
@@ -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<AgentSessionEvent, { type: "agent_end" }>): Promise<void> {
async #handleAgentEnd(event: Extract<AgentSessionEvent, { type: "agent_end" }>): Promise<void> {
// 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<void> {
async #finishAgentEnd(event: Extract<AgentSessionEvent, { type: "agent_end" }>): Promise<void> {
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<AgentSessionEvent, { type: "agent_end" }>): 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();
packages/coding-agent/test/modes/controllers/event-controller-abort-guard.test.ts
+93 -8
View File
@@ -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<AgentSessionEvent, { type: "agent_end" }> {
return { type: "agent_end", messages } as Extract<AgentSessionEvent, { type: "agent_end" }>;
}
/** 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<string, unknown>(),
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();
});
});