From bc8c462d289d702e565cea0108edcfe024b6af8d Mon Sep 17 00:00:00 2001 From: YeonGyu-Kim Date: Fri, 22 May 2026 20:39:33 +0900 Subject: [PATCH] fix(session-notification-sender): guard ctx.$ with execFile fallback for Desktop sidecar (#4128, #4061) OpenCode Desktop's Electron sidecar runtime can omit Bun's ctx.$ helper. The sender previously called ctx.$ unconditionally, throwing TypeError: ctx.$ is not a function as unhandledRejection and crashing the sidecar with exit code 1. Add a runtime guard at every call site, falling back to Node.js child_process.execFile (with windowsHide: true) when ctx.$ is missing. The Bun ctx.$ path remains preferred when available. Every notification path is wrapped in try/catch so no failure escapes as unhandledRejection. Fixes #4128 Fixes #4061 --- src/hooks/session-notification-sender.test.ts | 78 +++-- src/hooks/session-notification-sender.ts | 278 +++++++++++++----- 2 files changed, 256 insertions(+), 100 deletions(-) diff --git a/src/hooks/session-notification-sender.test.ts b/src/hooks/session-notification-sender.test.ts index 015b66915..4961b2cb6 100644 --- a/src/hooks/session-notification-sender.test.ts +++ b/src/hooks/session-notification-sender.test.ts @@ -1,4 +1,7 @@ +/// + import { afterEach, beforeEach, describe, expect, jest, spyOn, test } from "bun:test" +import * as childProcess from "node:child_process" import * as sender from "./session-notification-sender" import * as utils from "./session-notification-utils" import type { PluginInput } from "@opencode-ai/plugin" @@ -6,6 +9,9 @@ import { unsafeTestValue } from "../../test-support/unsafe-test-value" +type TestShellResult = ReturnType> +type TestShellFactory = (cmd: TemplateStringsArray, ...values: unknown[]) => TestShellResult + function createShellPromise(handler: (cmdStr: string) => void) { return (cmd: TemplateStringsArray, ...values: unknown[]) => { const cmdStr = cmd.reduce((acc, part, i) => acc + part + (values[i] ?? ""), "") @@ -64,6 +70,29 @@ function createThrowingShellPromise(shouldThrow: (cmdStr: string) => boolean) { } } +type ExecFileCall = { + readonly file: string + readonly args: readonly string[] + readonly options: { readonly windowsHide?: boolean } +} + +function mockExecFile(calls: ExecFileCall[], error: Error | null = null): ReturnType { + return spyOn(childProcess, "execFile").mockImplementation( + unsafeTestValue( + ( + file: string, + args: readonly string[], + options: { readonly windowsHide?: boolean }, + callback: (execError: Error | null, stdout: string, stderr: string) => void + ) => { + calls.push({ file, args: [...args], options }) + callback(error, "", "") + return unsafeTestValue>({}) + } + ) + ) +} + describe("session-notification-sender", () => { beforeEach(() => { jest.restoreAllMocks() @@ -77,34 +106,33 @@ describe("session-notification-sender", () => { spyOn(utils, "getAplayPath").mockResolvedValue("/usr/bin/aplay") }) + afterEach(() => { + jest.restoreAllMocks() + }) + describe("#given sendSessionNotification", () => { describe("#when ctx.$ is unavailable", () => { - test("#then it returns early without throwing when ctx has no $", async () => { - const cmuxSpy = spyOn(utils, "getCmuxPath") + test("#then it falls back to execFile without throwing", async () => { + const execFileCalls: ExecFileCall[] = [] + mockExecFile(execFileCalls) const mockCtx = unsafeTestValue({}) - await expect(sender.sendSessionNotification(mockCtx, "darwin", "Test", "Message")).resolves.toBeUndefined() - expect(cmuxSpy).not.toHaveBeenCalled() + await sender.sendSessionNotification(mockCtx, "win32", "Test", "Message") + + expect(execFileCalls.length).toBe(1) + expect(execFileCalls[0]?.file).toBe("powershell") + expect(execFileCalls[0]?.args[0]).toBe("-Command") + expect(execFileCalls[0]?.options.windowsHide).toBe(true) }) - test("#then it returns early without throwing when ctx.$ is not a function", async () => { - const cmuxSpy = spyOn(utils, "getCmuxPath") - const mockCtx = unsafeTestValue({ - $: "not-a-function", - }) - - await expect(sender.sendSessionNotification(mockCtx, "darwin", "Test", "Message")).resolves.toBeUndefined() - expect(cmuxSpy).not.toHaveBeenCalled() - }) - - test("#then it remains non-throwing across sender APIs", async () => { - const afplaySpy = spyOn(utils, "getAfplayPath") + test("#then it swallows execFile rejection without throwing", async () => { + const execFileCalls: ExecFileCall[] = [] + mockExecFile(execFileCalls, new Error("execFile failed")) const mockCtx = unsafeTestValue({}) - await expect(sender.sendSessionNotification(mockCtx, "darwin", "Test", "Message")).resolves.toBeUndefined() - await expect(sender.playSessionNotificationSound(mockCtx, "darwin", "/sound.aiff")).resolves.toBeUndefined() + await sender.sendSessionNotification(mockCtx, "win32", "Test", "Message") - expect(afplaySpy).not.toHaveBeenCalled() + expect(execFileCalls.length).toBe(1) }) }) @@ -192,13 +220,13 @@ describe("session-notification-sender", () => { $: createThrowingShellPromise((cmdStr) => cmdStr.includes("cmux notify")), }) - const originalFactory = mockCtx.$ + const originalFactory = unsafeTestValue(mockCtx.$) const trackingCalls: string[] = [] - mockCtx.$ = ((cmd: TemplateStringsArray, ...values: unknown[]) => { + mockCtx.$ = unsafeTestValue((cmd: TemplateStringsArray, ...values: unknown[]) => { const cmdStr = cmd.reduce((acc: string, part: string, i: number) => acc + part + (values[i] ?? ""), "") trackingCalls.push(cmdStr) return originalFactory(cmd, ...values) - }) as typeof mockCtx.$ + }) await sender.sendSessionNotification(mockCtx, "darwin", "Test", "Message") @@ -215,12 +243,12 @@ describe("session-notification-sender", () => { $: createThrowingShellPromise((cmdStr) => cmdStr.includes("cmux notify") || cmdStr.includes("terminal-notifier")), }) - const originalFactory = mockCtx.$ - mockCtx.$ = ((cmd: TemplateStringsArray, ...values: unknown[]) => { + const originalFactory = unsafeTestValue(mockCtx.$) + mockCtx.$ = unsafeTestValue((cmd: TemplateStringsArray, ...values: unknown[]) => { const cmdStr = cmd.reduce((acc: string, part: string, i: number) => acc + part + (values[i] ?? ""), "") trackingCalls.push(cmdStr) return originalFactory(cmd, ...values) - }) as typeof mockCtx.$ + }) await sender.sendSessionNotification(mockCtx, "darwin", "Test", "Message") diff --git a/src/hooks/session-notification-sender.ts b/src/hooks/session-notification-sender.ts index fb374afe5..63d6c3ce5 100644 --- a/src/hooks/session-notification-sender.ts +++ b/src/hooks/session-notification-sender.ts @@ -1,4 +1,6 @@ import type { PluginInput } from "@opencode-ai/plugin" +import { execFile } from "node:child_process" +import { promisify } from "node:util" import { platform } from "os" import { log } from "../shared" import { @@ -39,17 +41,45 @@ type ShellCommand = Promise & { nothrow?: () => ShellCommand } +type ShellRunner = NonNullable + +type ShellFailureMode = "throw" | "nothrow" + let hasLoggedUnavailableShellHelper = false -function canRunNotificationCommand(ctx: PluginInput): boolean { - if (typeof ctx?.$ === "function") return true +function getShellRunner(ctx: PluginInput): ShellRunner | undefined { + // Guard for #4128 + #4061: OpenCode Desktop's Electron sidecar can omit Bun's ctx.$ helper. + if (typeof ctx.$ === "function") return ctx.$ if (!hasLoggedUnavailableShellHelper) { hasLoggedUnavailableShellHelper = true - log("[session-notification] ctx.$ unavailable; skipping notification command execution") + log("[session-notification] ctx.$ unavailable; falling back to child_process.execFile") } - return false + return undefined +} + +function logCommandFailure(commandName: string, error: Error | string): void { + log("[session-notification] notification command failed", { + commandName, + error: typeof error === "string" ? error : error.message, + }) +} + +function logOperationFailure(operation: string, error: Error | string): void { + log("[session-notification] notification operation failed", { + operation, + error: typeof error === "string" ? error : error.message, + }) +} + +async function runQuiet(command: ShellCommand): Promise { + if (typeof command.quiet === "function") { + await command.quiet() + return + } + + await command } async function runQuietNothrow(command: ShellCommand): Promise { @@ -62,64 +92,135 @@ async function runQuietNothrow(command: ShellCommand): Promise { await safeCommand } +async function runExecFile(commandPath: string, args: readonly string[]): Promise { + const execFileAsync = promisify(execFile) + await execFileAsync(commandPath, [...args], { windowsHide: true }) +} + +async function runNotificationCommand( + ctx: PluginInput, + commandPath: string, + args: readonly string[], + shellCommand: (shell: ShellRunner) => ShellCommand, + shellFailureMode: ShellFailureMode = "nothrow" +): Promise { + const shell = getShellRunner(ctx) + if (shell) { + if (shellFailureMode === "throw") { + await runQuiet(shellCommand(shell)) + return + } + + await runQuietNothrow(shellCommand(shell)) + return + } + + await runExecFile(commandPath, args) +} + export async function sendSessionNotification( ctx: PluginInput, platform: Platform, title: string, message: string ): Promise { - if (!canRunNotificationCommand(ctx)) return - - switch (platform) { - case "darwin": { - // Try cmux first - native UNUserNotificationCenter, properly attributed - const cmuxPath = await getCmuxPath() - if (cmuxPath) { - try { - await ctx.$`${cmuxPath} notify --title ${title} --body ${message}`.quiet() - break - } catch { - } - } - - // Try terminal-notifier - deterministic click-to-focus - const terminalNotifierPath = await getTerminalNotifierPath() - if (terminalNotifierPath) { - const bundleId = process.env.__CFBundleIdentifier - try { - if (bundleId) { - await ctx.$`${terminalNotifierPath} -title ${title} -message ${message} -activate ${bundleId}`.quiet() - } else { - await ctx.$`${terminalNotifierPath} -title ${title} -message ${message}`.quiet() + try { + switch (platform) { + case "darwin": { + // Try cmux first - native UNUserNotificationCenter, properly attributed + const cmuxPath = await getCmuxPath() + if (cmuxPath) { + try { + await runNotificationCommand( + ctx, + cmuxPath, + ["notify", "--title", title, "--body", message], + (shell) => shell`${cmuxPath} notify --title ${title} --body ${message}`, + "throw" + ) + break + } catch (error) { + if (error instanceof Error) { + logCommandFailure("cmux", error) + } else { + logCommandFailure("cmux", String(error)) + } } - break - } catch { } + + // Try terminal-notifier - deterministic click-to-focus + const terminalNotifierPath = await getTerminalNotifierPath() + if (terminalNotifierPath) { + const bundleId = process.env.__CFBundleIdentifier + const args = bundleId + ? ["-title", title, "-message", message, "-activate", bundleId] + : ["-title", title, "-message", message] + try { + await runNotificationCommand( + ctx, + terminalNotifierPath, + args, + (shell) => bundleId + ? shell`${terminalNotifierPath} -title ${title} -message ${message} -activate ${bundleId}` + : shell`${terminalNotifierPath} -title ${title} -message ${message}`, + "throw" + ) + break + } catch (error) { + if (error instanceof Error) { + logCommandFailure("terminal-notifier", error) + } else { + logCommandFailure("terminal-notifier", String(error)) + } + } + } + + // Fallback: osascript (click may open Finder instead of terminal) + const osascriptPath = await getOsascriptPath() + if (!osascriptPath) return + + const escapedTitle = escapeAppleScriptText(title) + const escapedMessage = escapeAppleScriptText(message) + const appleScript = "display notification \"" + escapedMessage + "\" with title \"" + escapedTitle + "\"" + await runNotificationCommand( + ctx, + osascriptPath, + ["-e", appleScript], + (shell) => shell`${osascriptPath} -e ${appleScript}` + ) + break } + case "linux": { + const notifySendPath = await getNotifySendPath() + if (!notifySendPath) return - // Fallback: osascript (click may open Finder instead of terminal) - const osascriptPath = await getOsascriptPath() - if (!osascriptPath) return + await runNotificationCommand( + ctx, + notifySendPath, + [title, message], + (shell) => shell`${notifySendPath} ${title} ${message} 2>/dev/null` + ) + break + } + case "win32": { + const powershellPath = await getPowershellPath() + if (!powershellPath) return - const escapedTitle = escapeAppleScriptText(title) - const escapedMessage = escapeAppleScriptText(message) - await runQuietNothrow(ctx.$`${osascriptPath} -e ${"display notification \"" + escapedMessage + "\" with title \"" + escapedTitle + "\""}`) - break + const toastScript = buildWindowsToastScript(title, message) + await runNotificationCommand( + ctx, + powershellPath, + ["-Command", toastScript], + (shell) => shell`${powershellPath} -Command ${toastScript}` + ) + break + } } - case "linux": { - const notifySendPath = await getNotifySendPath() - if (!notifySendPath) return - - await runQuietNothrow(ctx.$`${notifySendPath} ${title} ${message} 2>/dev/null`) - break - } - case "win32": { - const powershellPath = await getPowershellPath() - if (!powershellPath) return - - const toastScript = buildWindowsToastScript(title, message) - await runQuietNothrow(ctx.$`${powershellPath} -Command ${toastScript}`) - break + } catch (error) { + if (error instanceof Error) { + logOperationFailure("send", error) + } else { + logOperationFailure("send", String(error)) } } } @@ -129,33 +230,60 @@ export async function playSessionNotificationSound( platform: Platform, soundPath: string ): Promise { - if (!canRunNotificationCommand(ctx)) return - - switch (platform) { - case "darwin": { - const afplayPath = await getAfplayPath() - if (!afplayPath) return - await runQuietNothrow(ctx.$`${afplayPath} ${soundPath}`) - break - } - case "linux": { - const paplayPath = await getPaplayPath() - if (paplayPath) { - await runQuietNothrow(ctx.$`${paplayPath} ${soundPath} 2>/dev/null`) - } else { - const aplayPath = await getAplayPath() - if (aplayPath) { - await runQuietNothrow(ctx.$`${aplayPath} ${soundPath} 2>/dev/null`) - } + try { + switch (platform) { + case "darwin": { + const afplayPath = await getAfplayPath() + if (!afplayPath) return + await runNotificationCommand( + ctx, + afplayPath, + [soundPath], + (shell) => shell`${afplayPath} ${soundPath}` + ) + break + } + case "linux": { + const paplayPath = await getPaplayPath() + if (paplayPath) { + await runNotificationCommand( + ctx, + paplayPath, + [soundPath], + (shell) => shell`${paplayPath} ${soundPath} 2>/dev/null` + ) + } else { + const aplayPath = await getAplayPath() + if (aplayPath) { + await runNotificationCommand( + ctx, + aplayPath, + [soundPath], + (shell) => shell`${aplayPath} ${soundPath} 2>/dev/null` + ) + } + } + break + } + case "win32": { + const powershellPath = await getPowershellPath() + if (!powershellPath) return + const escaped = escapePowerShellSingleQuotedText(soundPath) + const soundScript = "(New-Object Media.SoundPlayer '" + escaped + "').PlaySync()" + await runNotificationCommand( + ctx, + powershellPath, + ["-Command", soundScript], + (shell) => shell`${powershellPath} -Command ${soundScript}` + ) + break } - break } - case "win32": { - const powershellPath = await getPowershellPath() - if (!powershellPath) return - const escaped = escapePowerShellSingleQuotedText(soundPath) - await runQuietNothrow(ctx.$`${powershellPath} -Command ${"(New-Object Media.SoundPlayer '" + escaped + "').PlaySync()"}`) - break + } catch (error) { + if (error instanceof Error) { + logOperationFailure("sound", error) + } else { + logOperationFailure("sound", String(error)) } } }