diff --git a/PLAN.md b/PLAN.md index 5f41131..d1c7234 100644 --- a/PLAN.md +++ b/PLAN.md @@ -188,7 +188,7 @@ extensions/ index.ts config.ts i18n.ts - safe-send-message.ts + notification-sender.ts tools/ index.ts schema.ts @@ -670,7 +670,7 @@ High-level decisions: Phase 2B implementation slices: 1. Add manager end-cause metadata. See `docs/process-notifications-plan.md#manager-update-end-cause-metadata`. -2. Add notification message infrastructure: constants, safe send wrapper, XML content builder, custom message renderer. +2. Add notification message infrastructure: constants, normal notification sender, XML content builder, custom message renderer. 3. Add notification registry/service: classify process ends, compile/evaluate log matchers from `appendedText`, track intentional stops, and send custom messages. 4. Add `notify` to the `process start` tool schema with attention-only values. 5. Update `process stop` to mark intentional stops before calling `manager.kill()`. @@ -1373,7 +1373,7 @@ The skill file and prompt guidelines do not need persistence-specific behavior. 1. **Listener leaks**: Every `pi.events.on()` and `manager.onEvent()` returns an unsubscribe function. These MUST be stored and called on session_shutdown. The EventBus is never cleared by Pi. -2. **Stale Pi context**: After `/new`/`/fork`/`/resume`, the old `pi` proxy throws on any method call. The `safeSendMessage` wrapper handles this for async manager events that fire between shutdown and cleanup. All pi.events listeners from the old instance must be unsubscribed. +2. **Stale Pi context**: After `/new`/`/fork`/`/resume`, the old `pi` proxy throws on any method call. Do not solve this by swallowing stale-proxy errors. Prevent stale calls through lifecycle ownership: notification services must be extension-instance scoped, expose `dispose()`, set a disposed flag before cleanup, unsubscribe manager/pi event listeners, and never send after disposal. 3. **Exit hook duplication**: The `process.once("exit", ...)` handler in `hooks/exit.ts` must be guarded by a globalThis flag. Without this, each extension reload would add another exit handler. The guarded handler must iterate a global manager set, not close over one manager from the first extension load. diff --git a/docs/process-notifications-plan.md b/docs/process-notifications-plan.md index c978991..dddc83e 100644 --- a/docs/process-notifications-plan.md +++ b/docs/process-notifications-plan.md @@ -530,7 +530,7 @@ Guidelines: ```txt extensions/processes/ constants.ts - safe-send-message.ts + notification-sender.ts message-renderer.ts notifications/ types.ts @@ -599,9 +599,13 @@ Initial minimum: Builds XML-like `content` strings and JSON `details`. +#### `notification-sender.ts` + +Small wrapper around `pi.sendMessage()` that builds the custom message payload. It must not catch and swallow stale proxy errors. Stale sends are prevented by disposing the notification service before manager cleanup and by unsubscribing all event listeners. + #### `service.ts` -Subscribes to manager events and sends messages. +Subscribes to manager events and sends messages. The service is extension-instance scoped. It may close over the current extension instance's `pi`, but it must not store it globally or use it after disposal. Inputs: @@ -614,6 +618,14 @@ interface NotificationServiceDeps { } ``` +Lifecycle requirements: + +- The service owns every `manager.onEvent()` / `pi.events.on()` disposer it creates. +- `dispose()` sets `disposed = true` before unsubscribing. +- The send path checks `disposed` and returns without sending after disposal. +- Session shutdown must dispose the notification service before calling `manager.killAll()` or `manager.cleanup()`. +- Do not catch stale `pi` / revoked-proxy errors as normal control flow. If such an error occurs, the lifecycle cleanup is wrong and should be fixed. + Behavior: 1. On `process_ended`: @@ -665,15 +677,15 @@ Update `extensions/processes/index.ts`: export default function processesExtension(pi: ExtensionAPI): void { const manager = getManager(); const notifications = createNotificationRegistry(); + const notificationService = registerNotificationService(pi, manager, notifications); registerMessageRenderer(pi); - registerNotificationService(pi, manager, notifications); registerProcessTool(pi, manager, notifications); - registerCleanupHook(pi, manager, notifications); + registerCleanupHook(pi, manager, notifications, notificationService); } ``` -All listeners must return disposers and be cleaned up on `session_shutdown`. +All listeners must return disposers and be cleaned up on `session_shutdown`. Cleanup order matters: dispose notification services/listeners before killing or cleaning up the manager, because manager cleanup can emit process events. ## Phased implementation @@ -699,7 +711,7 @@ pnpm lint Scope: - Add constants. -- Add `safe-send-message.ts`. +- Add `notification-sender.ts`. - Add XML content renderer. - Add TUI message renderer. - Add notification details types. @@ -723,7 +735,7 @@ Validation: - Unit-test classifier. - Unit-test log matching. -- Unit-test service with mocked `pi.sendMessage()` and fake manager events. +- Unit-test service with mocked `pi.sendMessage()` and fake manager events. Include disposal tests that prove no send happens after `dispose()`. ### Phase 4: tool schema and action integration diff --git a/extensions/processes/notification-sender.test.ts b/extensions/processes/notification-sender.test.ts new file mode 100644 index 0000000..2077539 --- /dev/null +++ b/extensions/processes/notification-sender.test.ts @@ -0,0 +1,70 @@ +import type { ExtensionAPI } from "@earendil-works/pi-coding-agent"; +import { describe, expect, it, vi } from "vitest"; + +import { MESSAGE_TYPE_PROCESS_NOTIFICATION } from "./constants"; +import { sendProcessNotificationMessage } from "./notification-sender"; +import type { ProcessNotificationDetails } from "./notifications/types"; + +const details: ProcessNotificationDetails = { + kind: "crash", + processId: "proc_1", + processName: "test", + command: "pnpm test", + timestamp: 123, + summary: "Process failed.", + status: "exited", + exitCode: 1, + endReason: "exit", + signal: null, + attention: "turn", +}; + +const options = { + triggerTurn: true, + deliverAs: "steer" as const, +}; + +function piWithSendMessage( + sendMessage: ExtensionAPI["sendMessage"], +): ExtensionAPI { + return { sendMessage } as ExtensionAPI; +} + +describe("sendProcessNotificationMessage", () => { + it("sends a displayed process notification custom message", () => { + const sendMessage = vi.fn(); + + sendProcessNotificationMessage( + piWithSendMessage(sendMessage), + details, + options, + ); + + expect(sendMessage).toHaveBeenCalledWith( + { + customType: MESSAGE_TYPE_PROCESS_NOTIFICATION, + content: expect.stringContaining( + '', + ), + display: true, + details, + }, + options, + ); + }); + + it("does not catch sendMessage errors", () => { + const error = new Error("send failed"); + const sendMessage = vi.fn(() => { + throw error; + }); + + expect(() => + sendProcessNotificationMessage( + piWithSendMessage(sendMessage), + details, + options, + ), + ).toThrow(error); + }); +}); diff --git a/extensions/processes/notification-sender.ts b/extensions/processes/notification-sender.ts new file mode 100644 index 0000000..8aae4fa --- /dev/null +++ b/extensions/processes/notification-sender.ts @@ -0,0 +1,26 @@ +import type { ExtensionAPI } from "@earendil-works/pi-coding-agent"; + +import { MESSAGE_TYPE_PROCESS_NOTIFICATION } from "./constants"; +import { buildProcessNotificationContent } from "./notifications/render-content"; +import type { ProcessNotificationDetails } from "./notifications/types"; + +export interface ProcessNotificationSendOptions { + triggerTurn: boolean; + deliverAs: "steer" | "followUp" | "nextTurn"; +} + +export function sendProcessNotificationMessage( + pi: ExtensionAPI, + details: ProcessNotificationDetails, + options: ProcessNotificationSendOptions, +): void { + pi.sendMessage( + { + customType: MESSAGE_TYPE_PROCESS_NOTIFICATION, + content: buildProcessNotificationContent(details), + display: true, + details, + }, + options, + ); +} diff --git a/extensions/processes/safe-send-message.ts b/extensions/processes/safe-send-message.ts deleted file mode 100644 index c186962..0000000 --- a/extensions/processes/safe-send-message.ts +++ /dev/null @@ -1,33 +0,0 @@ -import type { ExtensionAPI } from "@earendil-works/pi-coding-agent"; - -import { MESSAGE_TYPE_PROCESS_NOTIFICATION } from "./constants"; -import type { ProcessNotificationDetails } from "./notifications/types"; - -export interface ProcessNotificationMessage { - content: string; - details: ProcessNotificationDetails; -} - -export function safeSendProcessNotificationMessage( - pi: ExtensionAPI, - message: ProcessNotificationMessage, - options: { - triggerTurn: boolean; - deliverAs: "steer" | "followUp" | "nextTurn"; - }, -): boolean { - try { - pi.sendMessage( - { - customType: MESSAGE_TYPE_PROCESS_NOTIFICATION, - content: message.content, - display: true, - details: message.details, - }, - options, - ); - return true; - } catch { - return false; - } -}