From a6ee5a7553d5fa2292ff8d4167aa1f32d36f8d03 Mon Sep 17 00:00:00 2001 From: YeonGyu-Kim Date: Thu, 25 Dec 2025 06:58:03 +0900 Subject: [PATCH] fix: Notification hook works weirdly for subagent sessions (#189) * fix: Notification hook works weirdly for subagent sessions - Added mainSessionID check to prevent notifications in subagent sessions - Only trigger notifications for main session when waiting for user input - Added comprehensive tests to validate the fix Issue: https://github.com/code-yeongyu/oh-my-opencode/issues/92 * chore: changes by sisyphus-dev-ai --------- Co-authored-by: codingsh Co-authored-by: sisyphus-dev-ai --- = | 0 src/hooks/session-notification.test.ts | 349 +++++++++++++++++++++++++ src/hooks/session-notification.ts | 6 +- 3 files changed, 354 insertions(+), 1 deletion(-) create mode 100644 = create mode 100644 src/hooks/session-notification.test.ts diff --git a/= b/= new file mode 100644 index 0000000..e69de29 diff --git a/src/hooks/session-notification.test.ts b/src/hooks/session-notification.test.ts new file mode 100644 index 0000000..934e44c --- /dev/null +++ b/src/hooks/session-notification.test.ts @@ -0,0 +1,349 @@ +import { describe, expect, test, beforeEach, afterEach } from "bun:test" + +import { createSessionNotification } from "./session-notification" +import { setMainSession, subagentSessions } from "../features/claude-code-session-state" + +describe("session-notification", () => { + let notificationCalls: string[] + + function createMockPluginInput() { + return { + $: async (cmd: TemplateStringsArray | string) => { + // #given - track notification commands (osascript, notify-send, powershell) + const cmdStr = typeof cmd === "string" ? cmd : cmd.join("") + if (cmdStr.includes("osascript") || cmdStr.includes("notify-send") || cmdStr.includes("powershell")) { + notificationCalls.push(cmdStr) + } + return { stdout: "", stderr: "", exitCode: 0 } + }, + client: { + session: { + todo: async () => ({ data: [] }), + }, + }, + directory: "/tmp/test", + } as any + } + + beforeEach(() => { + // #given - reset state before each test + notificationCalls = [] + }) + + afterEach(() => { + // #given - cleanup after each test + subagentSessions.clear() + setMainSession(undefined) + }) + + test("should not trigger notification for subagent session", async () => { + // #given - a subagent session exists + const subagentSessionID = "subagent-123" + subagentSessions.add(subagentSessionID) + + const hook = createSessionNotification(createMockPluginInput(), { + idleConfirmationDelay: 0, + }) + + // #when - subagent session goes idle + await hook({ + event: { + type: "session.idle", + properties: { sessionID: subagentSessionID }, + }, + }) + + // Wait for any pending timers + await new Promise((resolve) => setTimeout(resolve, 50)) + + // #then - notification should NOT be sent + expect(notificationCalls).toHaveLength(0) + }) + + test("should not trigger notification when mainSessionID is set and session is not main", async () => { + // #given - main session is set, but a different session goes idle + const mainSessionID = "main-123" + const otherSessionID = "other-456" + setMainSession(mainSessionID) + + const hook = createSessionNotification(createMockPluginInput(), { + idleConfirmationDelay: 0, + }) + + // #when - non-main session goes idle + await hook({ + event: { + type: "session.idle", + properties: { sessionID: otherSessionID }, + }, + }) + + // Wait for any pending timers + await new Promise((resolve) => setTimeout(resolve, 50)) + + // #then - notification should NOT be sent + expect(notificationCalls).toHaveLength(0) + }) + + test("should trigger notification for main session when idle", async () => { + // #given - main session is set + const mainSessionID = "main-789" + setMainSession(mainSessionID) + + const hook = createSessionNotification(createMockPluginInput(), { + idleConfirmationDelay: 10, + skipIfIncompleteTodos: false, + }) + + // #when - main session goes idle + await hook({ + event: { + type: "session.idle", + properties: { sessionID: mainSessionID }, + }, + }) + + // Wait for idle confirmation delay + buffer + await new Promise((resolve) => setTimeout(resolve, 100)) + + // #then - notification should be sent + expect(notificationCalls.length).toBeGreaterThanOrEqual(1) + }) + + test("should skip notification for subagent even when mainSessionID is set", async () => { + // #given - both mainSessionID and subagent session exist + const mainSessionID = "main-999" + const subagentSessionID = "subagent-888" + setMainSession(mainSessionID) + subagentSessions.add(subagentSessionID) + + const hook = createSessionNotification(createMockPluginInput(), { + idleConfirmationDelay: 0, + }) + + // #when - subagent session goes idle + await hook({ + event: { + type: "session.idle", + properties: { sessionID: subagentSessionID }, + }, + }) + + // Wait for any pending timers + await new Promise((resolve) => setTimeout(resolve, 50)) + + // #then - notification should NOT be sent (subagent check takes priority) + expect(notificationCalls).toHaveLength(0) + }) + + test("should handle subagentSessions and mainSessionID checks in correct order", async () => { + // #given - main session and subagent session exist + const mainSessionID = "main-111" + const subagentSessionID = "subagent-222" + const unknownSessionID = "unknown-333" + setMainSession(mainSessionID) + subagentSessions.add(subagentSessionID) + + const hook = createSessionNotification(createMockPluginInput(), { + idleConfirmationDelay: 0, + }) + + // #when - subagent session goes idle + await hook({ + event: { + type: "session.idle", + properties: { sessionID: subagentSessionID }, + }, + }) + + // #when - unknown session goes idle (not main, not in subagentSessions) + await hook({ + event: { + type: "session.idle", + properties: { sessionID: unknownSessionID }, + }, + }) + + // Wait for any pending timers + await new Promise((resolve) => setTimeout(resolve, 50)) + + // #then - no notifications (subagent blocked by subagentSessions, unknown blocked by mainSessionID check) + expect(notificationCalls).toHaveLength(0) + }) + + test("should cancel pending notification on session activity", async () => { + // #given - main session is set + const mainSessionID = "main-cancel" + setMainSession(mainSessionID) + + const hook = createSessionNotification(createMockPluginInput(), { + idleConfirmationDelay: 100, // Long delay + skipIfIncompleteTodos: false, + }) + + // #when - session goes idle + await hook({ + event: { + type: "session.idle", + properties: { sessionID: mainSessionID }, + }, + }) + + // #when - activity happens before delay completes + await hook({ + event: { + type: "tool.execute.before", + properties: { sessionID: mainSessionID }, + }, + }) + + // Wait for original delay to pass + await new Promise((resolve) => setTimeout(resolve, 150)) + + // #then - notification should NOT be sent (cancelled by activity) + expect(notificationCalls).toHaveLength(0) + }) + + test("should handle session.created event without notification", async () => { + // #given - a new session is created + const hook = createSessionNotification(createMockPluginInput(), {}) + + // #when - session.created event fires + await hook({ + event: { + type: "session.created", + properties: { + info: { id: "new-session", title: "Test Session" }, + }, + }, + }) + + // Wait for any pending timers + await new Promise((resolve) => setTimeout(resolve, 50)) + + // #then - no notification should be triggered + expect(notificationCalls).toHaveLength(0) + }) + + test("should handle session.deleted event and cleanup state", async () => { + // #given - a session exists + const hook = createSessionNotification(createMockPluginInput(), {}) + + // #when - session.deleted event fires + await hook({ + event: { + type: "session.deleted", + properties: { + info: { id: "deleted-session" }, + }, + }, + }) + + // Wait for any pending timers + await new Promise((resolve) => setTimeout(resolve, 50)) + + // #then - no notification should be triggered + expect(notificationCalls).toHaveLength(0) + }) + + test("should mark session activity on message.updated event", async () => { + // #given - main session is set + const mainSessionID = "main-message" + setMainSession(mainSessionID) + + const hook = createSessionNotification(createMockPluginInput(), { + idleConfirmationDelay: 50, + skipIfIncompleteTodos: false, + }) + + // #when - session goes idle, then message.updated fires + await hook({ + event: { + type: "session.idle", + properties: { sessionID: mainSessionID }, + }, + }) + + await hook({ + event: { + type: "message.updated", + properties: { + info: { sessionID: mainSessionID, role: "user", finish: false }, + }, + }, + }) + + // Wait for idle delay to pass + await new Promise((resolve) => setTimeout(resolve, 100)) + + // #then - notification should NOT be sent (activity cancelled it) + expect(notificationCalls).toHaveLength(0) + }) + + test("should mark session activity on tool.execute.before event", async () => { + // #given - main session is set + const mainSessionID = "main-tool" + setMainSession(mainSessionID) + + const hook = createSessionNotification(createMockPluginInput(), { + idleConfirmationDelay: 50, + skipIfIncompleteTodos: false, + }) + + // #when - session goes idle, then tool.execute.before fires + await hook({ + event: { + type: "session.idle", + properties: { sessionID: mainSessionID }, + }, + }) + + await hook({ + event: { + type: "tool.execute.before", + properties: { sessionID: mainSessionID }, + }, + }) + + // Wait for idle delay to pass + await new Promise((resolve) => setTimeout(resolve, 100)) + + // #then - notification should NOT be sent (activity cancelled it) + expect(notificationCalls).toHaveLength(0) + }) + + test("should not send duplicate notification for same session", async () => { + // #given - main session is set + const mainSessionID = "main-dup" + setMainSession(mainSessionID) + + const hook = createSessionNotification(createMockPluginInput(), { + idleConfirmationDelay: 10, + skipIfIncompleteTodos: false, + }) + + // #when - session goes idle twice + await hook({ + event: { + type: "session.idle", + properties: { sessionID: mainSessionID }, + }, + }) + + // Wait for first notification + await new Promise((resolve) => setTimeout(resolve, 50)) + + await hook({ + event: { + type: "session.idle", + properties: { sessionID: mainSessionID }, + }, + }) + + // Wait for second potential notification + await new Promise((resolve) => setTimeout(resolve, 50)) + + // #then - only one notification should be sent + expect(notificationCalls).toHaveLength(1) + }) +}) diff --git a/src/hooks/session-notification.ts b/src/hooks/session-notification.ts index 8034058..56dc1d2 100644 --- a/src/hooks/session-notification.ts +++ b/src/hooks/session-notification.ts @@ -1,6 +1,6 @@ import type { PluginInput } from "@opencode-ai/plugin" import { platform } from "os" -import { subagentSessions } from "../features/claude-code-session-state" +import { subagentSessions, getMainSessionID } from "../features/claude-code-session-state" interface Todo { content: string @@ -243,6 +243,10 @@ export function createSessionNotification( if (subagentSessions.has(sessionID)) return + // Only trigger notifications for the main session (not subagent sessions) + const mainSessionID = getMainSessionID() + if (mainSessionID && sessionID !== mainSessionID) return + if (notifiedSessions.has(sessionID)) return if (pendingTimers.has(sessionID)) return if (executingNotifications.has(sessionID)) return