fix(session-notification): revert PR #543 and add proper notification plugin conflict detection (#575)
* revert: undo PR #543 changes (bun shell GC crash was misdiagnosed) This reverts commit4a38e70(PR #543) and2064568(follow-up fix). ## Why This Revert The original diagnosis was incorrect. PR #543 assumed Bun's ShellInterpreter GC bug was causing Windows crashes, but further investigation revealed the actual root cause: **The crash occurs when oh-my-opencode's session-notification runs alongside external notification plugins (e.g., @mohak34/opencode-notifier).** Evidence: - User removed opencode-notifier plugin → crashes stopped - Release version (with original ctx.$ code) works fine when used alone - No widespread crash reports from users without external notifiers - Both plugins listen to session.idle and send concurrent notifications The real issue is a conflict between two notification systems: 1. oh-my-opencode: ctx.$ → PowerShell → Windows.UI.Notifications 2. opencode-notifier: node-notifier → SnoreToast.exe A proper fix will detect and handle this conflict gracefully. Refs: #543, oven-sh/bun#23177, oven-sh/bun#24368 See: docs/CRASH_INVESTIGATION_TIMELINE.md (in follow-up commit) * fix(session-notification): detect and avoid conflict with external notification plugins When oh-my-opencode's session-notification runs alongside external notification plugins like opencode-notifier, both listen to session.idle and send concurrent notifications. This can cause crashes on Windows due to resource contention between different notification mechanisms: - oh-my-opencode: ctx.$ → PowerShell → Windows.UI.Notifications - opencode-notifier: node-notifier → SnoreToast.exe This commit adds: 1. External plugin detection (checks opencode.json for known notifiers) 2. Auto-disable of session-notification when conflict detected 3. Console warning explaining the situation 4. Config option 'notification.force_enable' to override Known notification plugins detected: - opencode-notifier - @mohak34/opencode-notifier - mohak34/opencode-notifier This is the actual fix for the Windows crash issue previously misdiagnosed as a Bun.spawn GC bug (PR #543). Refs: #543 * docs: add crash investigation timeline explaining the real root cause Documents the investigation journey from initial misdiagnosis (Bun GC bug) to discovering the actual root cause (notification plugin conflict). Key findings: - PR #543 was based on incorrect assumption - The real issue is concurrent notification plugins - oh-my-opencode + opencode-notifier = crash on Windows - Either plugin alone works fine * fix: address review feedback - add PowerShell escaping and use existing JSONC parser - Add back single-quote escaping for PowerShell soundPath to prevent command failures - Replace custom stripJsonComments with existing parseJsoncSafe from jsonc-parser - All 655 tests pass --------- Co-authored-by: sisyphus-dev-ai <sisyphus-dev-ai@users.noreply.github.com>
This commit is contained in:
committed by
GitHub
parent
b7588ee60e
commit
4902983398
@@ -1,6 +1,5 @@
|
||||
import type { PluginInput } from "@opencode-ai/plugin"
|
||||
import { platform } from "os"
|
||||
import { spawn } from "node:child_process"
|
||||
import { subagentSessions, getMainSessionID } from "../features/claude-code-session-state"
|
||||
import {
|
||||
getOsascriptPath,
|
||||
@@ -12,21 +11,6 @@ import {
|
||||
startBackgroundCheck,
|
||||
} from "./session-notification-utils"
|
||||
|
||||
/**
|
||||
* Execute a command using node:child_process instead of Bun shell.
|
||||
* This avoids Bun's ShellInterpreter GC bug on Windows (oven-sh/bun#23177, #24368).
|
||||
*/
|
||||
function execCommand(command: string, args: string[]): Promise<void> {
|
||||
return new Promise((resolve) => {
|
||||
const proc = spawn(command, args, {
|
||||
stdio: "ignore",
|
||||
detached: false,
|
||||
})
|
||||
proc.on("close", () => resolve())
|
||||
proc.on("error", () => resolve())
|
||||
})
|
||||
}
|
||||
|
||||
interface Todo {
|
||||
content: string
|
||||
status: string
|
||||
@@ -81,17 +65,14 @@ async function sendNotification(
|
||||
|
||||
const esTitle = title.replace(/\\/g, "\\\\").replace(/"/g, '\\"')
|
||||
const esMessage = message.replace(/\\/g, "\\\\").replace(/"/g, '\\"')
|
||||
const script = `display notification "${esMessage}" with title "${esTitle}"`
|
||||
// Use node:child_process instead of Bun shell to avoid potential GC issues
|
||||
await execCommand(osascriptPath, ["-e", script]).catch(() => {})
|
||||
await ctx.$`${osascriptPath} -e ${"display notification \"" + esMessage + "\" with title \"" + esTitle + "\""}`.catch(() => {})
|
||||
break
|
||||
}
|
||||
case "linux": {
|
||||
const notifySendPath = await getNotifySendPath()
|
||||
if (!notifySendPath) return
|
||||
|
||||
// Use node:child_process instead of Bun shell to avoid potential GC issues
|
||||
await execCommand(notifySendPath, [title, message]).catch(() => {})
|
||||
await ctx.$`${notifySendPath} ${title} ${message} 2>/dev/null`.catch(() => {})
|
||||
break
|
||||
}
|
||||
case "win32": {
|
||||
@@ -112,8 +93,7 @@ $Toast = [Windows.UI.Notifications.ToastNotification]::new($SerializedXml)
|
||||
$Notifier = [Windows.UI.Notifications.ToastNotificationManager]::CreateToastNotifier('OpenCode')
|
||||
$Notifier.Show($Toast)
|
||||
`.trim().replace(/\n/g, "; ")
|
||||
// Use node:child_process instead of Bun shell to avoid GC crash (oven-sh/bun#23177)
|
||||
await execCommand(powershellPath, ["-Command", toastScript]).catch(() => {})
|
||||
await ctx.$`${powershellPath} -Command ${toastScript}`.catch(() => {})
|
||||
break
|
||||
}
|
||||
}
|
||||
@@ -124,19 +104,17 @@ async function playSound(ctx: PluginInput, p: Platform, soundPath: string): Prom
|
||||
case "darwin": {
|
||||
const afplayPath = await getAfplayPath()
|
||||
if (!afplayPath) return
|
||||
// Use node:child_process instead of Bun shell to avoid potential GC issues
|
||||
execCommand(afplayPath, [soundPath]).catch(() => {})
|
||||
ctx.$`${afplayPath} ${soundPath}`.catch(() => {})
|
||||
break
|
||||
}
|
||||
case "linux": {
|
||||
const paplayPath = await getPaplayPath()
|
||||
if (paplayPath) {
|
||||
// Use node:child_process instead of Bun shell to avoid potential GC issues
|
||||
execCommand(paplayPath, [soundPath]).catch(() => {})
|
||||
ctx.$`${paplayPath} ${soundPath} 2>/dev/null`.catch(() => {})
|
||||
} else {
|
||||
const aplayPath = await getAplayPath()
|
||||
if (aplayPath) {
|
||||
execCommand(aplayPath, [soundPath]).catch(() => {})
|
||||
ctx.$`${aplayPath} ${soundPath} 2>/dev/null`.catch(() => {})
|
||||
}
|
||||
}
|
||||
break
|
||||
@@ -144,9 +122,7 @@ async function playSound(ctx: PluginInput, p: Platform, soundPath: string): Prom
|
||||
case "win32": {
|
||||
const powershellPath = await getPowershellPath()
|
||||
if (!powershellPath) return
|
||||
// Use node:child_process instead of Bun shell to avoid GC crash (oven-sh/bun#23177)
|
||||
const soundScript = `(New-Object Media.SoundPlayer '${soundPath.replace(/'/g, "''")}').PlaySync()`
|
||||
execCommand(powershellPath, ["-Command", soundScript]).catch(() => {})
|
||||
ctx.$`${powershellPath} -Command ${"(New-Object Media.SoundPlayer '" + soundPath.replace(/'/g, "''") + "').PlaySync()"}`.catch(() => {})
|
||||
break
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user