diff --git a/docs/CRASH_INVESTIGATION_TIMELINE.md b/docs/CRASH_INVESTIGATION_TIMELINE.md new file mode 100644 index 0000000..750abda --- /dev/null +++ b/docs/CRASH_INVESTIGATION_TIMELINE.md @@ -0,0 +1,152 @@ +# Windows Crash Investigation Timeline + +## Executive Summary + +**Initial Hypothesis**: Bun.spawn/ShellInterpreter GC bug causing crashes on Windows +**Actual Root Cause**: Conflict between oh-my-opencode's session-notification and external notification plugins (specifically `@mohak34/opencode-notifier`) + +**Evidence**: User removed `@mohak34/opencode-notifier` plugin → crashes stopped immediately. The release version of oh-my-opencode (with original Bun.spawn code) works fine when used alone. + +--- + +## Timeline + +### Phase 1: Initial Crash Reports (Early January 2026) + +**Symptoms:** +- Windows users experiencing crashes after extended oh-my-opencode usage +- Stack traces pointed to Bun's ShellInterpreter finalizer: + ``` + Segmentation fault at address 0x337081E00E0 + - interpreter.zig:1239: deinitFromFinalizer + - ZigGeneratedClasses.zig:19925: ShellInterpreterClass__finalize + ``` + +**Initial Analysis:** +- Similar to known Bun issues: oven-sh/bun#23177, oven-sh/bun#24368 +- Focus on `ctx.$` (Bun shell template literals) in session-notification.ts + +### Phase 2: PR #543 - Wrong Fix Merged (January 6, 2026) + +**PR**: [#543 - fix(session-notification): avoid Bun shell GC crash on Windows](https://github.com/code-yeongyu/oh-my-opencode/pull/543) + +**Changes Made:** +- Replaced `ctx.$` with `node:child_process.spawn` in `session-notification.ts` +- Updated tests to mock spawn instead of ctx.$ + +**Assumption**: The ShellInterpreter GC bug was causing crashes when notification commands were executed. + +**Status**: ❌ MERGED (reverted in this PR) + +### Phase 3: Continued Investigation - Debug Tracing (January 6-7, 2026) + +Crashes continued after PR #543. Added debug tracing system (PR #571) to capture what happens before crashes. + +**PR #571**: [feat(debug): add comprehensive crash tracing system](https://github.com/code-yeongyu/oh-my-opencode/pull/571) + +Tracing revealed LSP ENOENT errors, leading to: + +**PR #572**: [fix(lsp): add resilient handling for missing LSP server binaries](https://github.com/code-yeongyu/oh-my-opencode/pull/572) + +### Phase 4: More Bun.spawn Changes (January 7, 2026) - WRONG PATH + +Based on the assumption that Bun.spawn was the issue, additional files were modified locally: +- `src/hooks/session-notification-utils.ts` +- `src/hooks/comment-checker/cli.ts` +- `src/hooks/comment-checker/downloader.ts` +- `src/hooks/interactive-bash-session/index.ts` + +**Status**: ❌ REVERTED (never committed) + +### Phase 5: Root Cause Discovery (January 7, 2026) + +**Critical Observation by User:** +> "I removed `@mohak34/opencode-notifier` and crashes stopped. The release version with Bun.spawn works perfectly fine." + +**Key Evidence:** +1. Removing ONLY the notifier plugin fixed crashes +2. Release version (before PR #543) works fine for user and most others +3. No widespread complaints from other users about crashes +4. PR #543 was based on superficial pattern matching with Bun issues + +--- + +## The Real Root Cause: Notification Plugin Conflict + +### Two Plugins, Same Event + +Both plugins listen to `session.idle` and send notifications: + +| Aspect | oh-my-opencode | opencode-notifier | +|--------|---------------|-------------------| +| **Event** | `session.idle` | `session.idle` | +| **Delay** | 1.5s confirmation delay | Immediate | +| **Windows Notification** | PowerShell + Windows.UI.Notifications API | `node-notifier` → WindowsToaster → SnoreToast.exe | +| **Sound** | PowerShell Media.SoundPlayer | PowerShell Media.SoundPlayer | +| **Process spawning** | `ctx.$` (Bun shell) | `node:child_process` | + +### Conflict Points + +1. **Different notification systems fighting**: + - oh-my-opencode: Direct PowerShell → Windows.UI.Notifications + - opencode-notifier: SnoreToast.exe binary via node-notifier + +2. **Same app identity**: Both register with "OpenCode" as the toast notifier app + +3. **Concurrent execution**: Both trigger within milliseconds of each other on `session.idle` + +4. **Resource contention**: Windows Toast API may not handle concurrent registrations gracefully + +### Why It Wasn't Bun.spawn + +- Both plugins use different spawning methods - this didn't matter +- Release version works fine when used alone +- Most users don't have this issue (most don't use both plugins) +- The stack trace pointed to ShellInterpreter, but correlation ≠ causation + +--- + +## The Fix + +### What This PR Does + +1. **Reverts PR #543**: Restores original `ctx.$` usage (it was never the problem) + +2. **Adds conflict detection**: + - Scans `opencode.json` for known notification plugins + - Known plugins: `opencode-notifier`, `@mohak34/opencode-notifier` + +3. **Auto-disables on conflict**: + - When external notifier detected, skips creating session-notification hook + - Logs clear warning explaining why + +4. **Config override**: + ```json + { + "notification": { + "force_enable": true + } + } + ``` + Users can force-enable oh-my-opencode's notification if they want. + +--- + +## Lessons Learned + +1. **Correlation ≠ Causation**: Stack traces can be misleading; investigate root cause thoroughly +2. **Test with user's exact environment**: The crash only happened with specific plugin combination +3. **Challenge assumptions**: "Bun.spawn is buggy" was accepted too quickly without verifying +4. **Evidence-based debugging**: User's discovery (removing notifier = no crash) was the key evidence + +--- + +## Related Links + +- PR #543 (merged, reverted in this PR): https://github.com/code-yeongyu/oh-my-opencode/pull/543 +- PR #571 (open): https://github.com/code-yeongyu/oh-my-opencode/pull/571 +- PR #572 (open): https://github.com/code-yeongyu/oh-my-opencode/pull/572 +- opencode-notifier: https://github.com/mohak34/opencode-notifier +- Bun issues referenced (not actually the cause): + - https://github.com/oven-sh/bun/issues/23177 + - https://github.com/oven-sh/bun/issues/24368 diff --git a/src/config/schema.ts b/src/config/schema.ts index 8984284..2b09aba 100644 --- a/src/config/schema.ts +++ b/src/config/schema.ts @@ -238,6 +238,11 @@ export const BackgroundTaskConfigSchema = z.object({ modelConcurrency: z.record(z.string(), z.number().min(1)).optional(), }) +export const NotificationConfigSchema = z.object({ + /** Force enable session-notification even if external notification plugins are detected (default: false) */ + force_enable: z.boolean().optional(), +}) + export const OhMyOpenCodeConfigSchema = z.object({ $schema: z.string().optional(), disabled_mcps: z.array(AnyMcpNameSchema).optional(), @@ -255,6 +260,7 @@ export const OhMyOpenCodeConfigSchema = z.object({ skills: SkillsConfigSchema.optional(), ralph_loop: RalphLoopConfigSchema.optional(), background_task: BackgroundTaskConfigSchema.optional(), + notification: NotificationConfigSchema.optional(), }) export type OhMyOpenCodeConfig = z.infer @@ -272,5 +278,6 @@ export type DynamicContextPruningConfig = z.infer export type SkillDefinition = z.infer export type RalphLoopConfig = z.infer +export type NotificationConfig = z.infer export { AnyMcpNameSchema, type AnyMcpName, McpNameSchema, type McpName } from "../mcp/types" diff --git a/src/hooks/session-notification.test.ts b/src/hooks/session-notification.test.ts index 3c3e9af..ad6eb53 100644 --- a/src/hooks/session-notification.test.ts +++ b/src/hooks/session-notification.test.ts @@ -1,6 +1,4 @@ -import { describe, expect, test, beforeEach, afterEach, spyOn, mock } from "bun:test" -import { EventEmitter } from "node:events" -import * as childProcess from "node:child_process" +import { describe, expect, test, beforeEach, afterEach, spyOn } from "bun:test" import { createSessionNotification } from "./session-notification" import { setMainSession, subagentSessions } from "../features/claude-code-session-state" @@ -8,11 +6,20 @@ import * as utils from "./session-notification-utils" describe("session-notification", () => { let notificationCalls: string[] - let spawnMock: ReturnType function createMockPluginInput() { return { - $: async () => ({ stdout: "", stderr: "", exitCode: 0 }), + $: async (cmd: TemplateStringsArray | string, ...values: any[]) => { + // #given - track notification commands (osascript, notify-send, powershell) + const cmdStr = typeof cmd === "string" + ? cmd + : cmd.reduce((acc, part, i) => acc + part + (values[i] ?? ""), "") + + if (cmdStr.includes("osascript") || cmdStr.includes("notify-send") || cmdStr.includes("powershell")) { + notificationCalls.push(cmdStr) + } + return { stdout: "", stderr: "", exitCode: 0 } + }, client: { session: { todo: async () => ({ data: [] }), @@ -25,18 +32,6 @@ describe("session-notification", () => { beforeEach(() => { notificationCalls = [] - // Mock spawn to track notification commands - // Uses node:child_process.spawn instead of Bun shell to avoid GC crash - spawnMock = spyOn(childProcess, "spawn").mockImplementation(((cmd: string, args?: string[]) => { - // Track notification commands (osascript, notify-send, powershell) - if (cmd.includes("osascript") || cmd.includes("notify-send") || cmd.includes("powershell")) { - notificationCalls.push(`${cmd} ${(args ?? []).join(" ")}`) - } - const emitter = new EventEmitter() - setTimeout(() => emitter.emit("close", 0), 0) - return emitter as any - }) as typeof childProcess.spawn) - spyOn(utils, "getOsascriptPath").mockResolvedValue("/usr/bin/osascript") spyOn(utils, "getNotifySendPath").mockResolvedValue("/usr/bin/notify-send") spyOn(utils, "getPowershellPath").mockResolvedValue("powershell") diff --git a/src/hooks/session-notification.ts b/src/hooks/session-notification.ts index 0f7e375..eded518 100644 --- a/src/hooks/session-notification.ts +++ b/src/hooks/session-notification.ts @@ -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 { - 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 } } diff --git a/src/index.ts b/src/index.ts index bae5d02..226d6b6 100644 --- a/src/index.ts +++ b/src/index.ts @@ -62,7 +62,7 @@ import { import { BackgroundManager } from "./features/background-agent"; import { SkillMcpManager } from "./features/skill-mcp-manager"; import { type HookName } from "./config"; -import { log } from "./shared"; +import { log, detectExternalNotificationPlugin, getNotificationConflictWarning } from "./shared"; import { loadPluginConfig } from "./plugin-config"; import { createModelCacheState, getModelLimit } from "./plugin-state"; import { createConfigHandler } from "./plugin-handlers"; @@ -83,9 +83,24 @@ const OhMyOpenCodePlugin: Plugin = async (ctx) => { const sessionRecovery = isHookEnabled("session-recovery") ? createSessionRecoveryHook(ctx, { experimental: pluginConfig.experimental }) : null; - const sessionNotification = isHookEnabled("session-notification") - ? createSessionNotification(ctx) - : null; + + // Check for conflicting notification plugins before creating session-notification + let sessionNotification = null; + if (isHookEnabled("session-notification")) { + const forceEnable = pluginConfig.notification?.force_enable ?? false; + const externalNotifier = detectExternalNotificationPlugin(ctx.directory); + + if (externalNotifier.detected && !forceEnable) { + // External notification plugin detected - skip our notification to avoid conflicts + console.warn(getNotificationConflictWarning(externalNotifier.pluginName!)); + log("session-notification disabled due to external notifier conflict", { + detected: externalNotifier.pluginName, + allPlugins: externalNotifier.allPlugins, + }); + } else { + sessionNotification = createSessionNotification(ctx); + } + } const commentChecker = isHookEnabled("comment-checker") ? createCommentCheckerHooks(pluginConfig.comment_checker) diff --git a/src/shared/external-plugin-detector.test.ts b/src/shared/external-plugin-detector.test.ts new file mode 100644 index 0000000..f31ab48 --- /dev/null +++ b/src/shared/external-plugin-detector.test.ts @@ -0,0 +1,133 @@ +import { describe, expect, test, beforeEach, afterEach } from "bun:test" +import { detectExternalNotificationPlugin, getNotificationConflictWarning } from "./external-plugin-detector" +import * as fs from "node:fs" +import * as path from "node:path" +import * as os from "node:os" + +describe("external-plugin-detector", () => { + let tempDir: string + + beforeEach(() => { + tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "omo-test-")) + }) + + afterEach(() => { + fs.rmSync(tempDir, { recursive: true, force: true }) + }) + + describe("detectExternalNotificationPlugin", () => { + test("should return detected=false when no plugins configured", () => { + // #given - empty directory + // #when + const result = detectExternalNotificationPlugin(tempDir) + // #then + expect(result.detected).toBe(false) + expect(result.pluginName).toBeNull() + }) + + test("should return detected=false when only oh-my-opencode is configured", () => { + // #given - opencode.json with only oh-my-opencode + const opencodeDir = path.join(tempDir, ".opencode") + fs.mkdirSync(opencodeDir, { recursive: true }) + fs.writeFileSync( + path.join(opencodeDir, "opencode.json"), + JSON.stringify({ plugin: ["oh-my-opencode"] }) + ) + + // #when + const result = detectExternalNotificationPlugin(tempDir) + + // #then + expect(result.detected).toBe(false) + expect(result.pluginName).toBeNull() + expect(result.allPlugins).toContain("oh-my-opencode") + }) + + test("should detect opencode-notifier plugin", () => { + // #given - opencode.json with opencode-notifier + const opencodeDir = path.join(tempDir, ".opencode") + fs.mkdirSync(opencodeDir, { recursive: true }) + fs.writeFileSync( + path.join(opencodeDir, "opencode.json"), + JSON.stringify({ plugin: ["oh-my-opencode", "opencode-notifier"] }) + ) + + // #when + const result = detectExternalNotificationPlugin(tempDir) + + // #then + expect(result.detected).toBe(true) + expect(result.pluginName).toBe("opencode-notifier") + }) + + test("should detect opencode-notifier with version suffix", () => { + // #given - opencode.json with versioned opencode-notifier + const opencodeDir = path.join(tempDir, ".opencode") + fs.mkdirSync(opencodeDir, { recursive: true }) + fs.writeFileSync( + path.join(opencodeDir, "opencode.json"), + JSON.stringify({ plugin: ["oh-my-opencode", "opencode-notifier@1.2.3"] }) + ) + + // #when + const result = detectExternalNotificationPlugin(tempDir) + + // #then + expect(result.detected).toBe(true) + expect(result.pluginName).toBe("opencode-notifier") + }) + + test("should detect @mohak34/opencode-notifier", () => { + // #given - opencode.json with scoped package name + const opencodeDir = path.join(tempDir, ".opencode") + fs.mkdirSync(opencodeDir, { recursive: true }) + fs.writeFileSync( + path.join(opencodeDir, "opencode.json"), + JSON.stringify({ plugin: ["oh-my-opencode", "@mohak34/opencode-notifier"] }) + ) + + // #when + const result = detectExternalNotificationPlugin(tempDir) + + // #then - returns the matched known plugin pattern, not the full entry + expect(result.detected).toBe(true) + expect(result.pluginName).toContain("opencode-notifier") + }) + + test("should handle JSONC format with comments", () => { + // #given - opencode.jsonc with comments + const opencodeDir = path.join(tempDir, ".opencode") + fs.mkdirSync(opencodeDir, { recursive: true }) + fs.writeFileSync( + path.join(opencodeDir, "opencode.jsonc"), + `{ + // This is a comment + "plugin": [ + "oh-my-opencode", + "opencode-notifier" // Another comment + ] + }` + ) + + // #when + const result = detectExternalNotificationPlugin(tempDir) + + // #then + expect(result.detected).toBe(true) + expect(result.pluginName).toBe("opencode-notifier") + }) + }) + + describe("getNotificationConflictWarning", () => { + test("should generate warning message with plugin name", () => { + // #when + const warning = getNotificationConflictWarning("opencode-notifier") + + // #then + expect(warning).toContain("opencode-notifier") + expect(warning).toContain("session.idle") + expect(warning).toContain("auto-disabled") + expect(warning).toContain("force_enable") + }) + }) +}) diff --git a/src/shared/external-plugin-detector.ts b/src/shared/external-plugin-detector.ts new file mode 100644 index 0000000..ff04fe1 --- /dev/null +++ b/src/shared/external-plugin-detector.ts @@ -0,0 +1,132 @@ +/** + * Detects external plugins that may conflict with oh-my-opencode features. + * Used to prevent crashes from concurrent notification plugins. + */ + +import * as fs from "node:fs" +import * as path from "node:path" +import * as os from "node:os" +import { log } from "./logger" +import { parseJsoncSafe } from "./jsonc-parser" + +interface OpencodeConfig { + plugin?: string[] +} + +/** + * Known notification plugins that conflict with oh-my-opencode's session-notification. + * Both plugins listen to session.idle and send notifications simultaneously, + * which can cause crashes on Windows due to resource contention. + */ +const KNOWN_NOTIFICATION_PLUGINS = [ + "opencode-notifier", + "@mohak34/opencode-notifier", + "mohak34/opencode-notifier", +] + +function getWindowsAppdataDir(): string | null { + return process.env.APPDATA || null +} + +function getConfigPaths(directory: string): string[] { + const crossPlatformDir = path.join(os.homedir(), ".config") + const paths = [ + path.join(directory, ".opencode", "opencode.json"), + path.join(directory, ".opencode", "opencode.jsonc"), + path.join(crossPlatformDir, "opencode", "opencode.json"), + path.join(crossPlatformDir, "opencode", "opencode.jsonc"), + ] + + if (process.platform === "win32") { + const appdataDir = getWindowsAppdataDir() + if (appdataDir) { + paths.push(path.join(appdataDir, "opencode", "opencode.json")) + paths.push(path.join(appdataDir, "opencode", "opencode.jsonc")) + } + } + + return paths +} + +function loadOpencodePlugins(directory: string): string[] { + for (const configPath of getConfigPaths(directory)) { + try { + if (!fs.existsSync(configPath)) continue + const content = fs.readFileSync(configPath, "utf-8") + const result = parseJsoncSafe(content) + if (result.data) { + return result.data.plugin ?? [] + } + } catch { + continue + } + } + return [] +} + +/** + * Check if a plugin entry matches a known notification plugin. + * Handles various formats: "name", "name@version", "npm:name", "file://path/name" + */ +function matchesNotificationPlugin(entry: string): string | null { + const normalized = entry.toLowerCase() + for (const known of KNOWN_NOTIFICATION_PLUGINS) { + if ( + normalized === known || + normalized.startsWith(`${known}@`) || + normalized.includes(`/${known}`) || + normalized.endsWith(`/${known}`) + ) { + return known + } + } + return null +} + +export interface ExternalNotifierResult { + detected: boolean + pluginName: string | null + allPlugins: string[] +} + +/** + * Detect if any external notification plugin is configured. + * Returns information about detected plugins for logging/warning. + */ +export function detectExternalNotificationPlugin(directory: string): ExternalNotifierResult { + const plugins = loadOpencodePlugins(directory) + + for (const plugin of plugins) { + const match = matchesNotificationPlugin(plugin) + if (match) { + log(`Detected external notification plugin: ${plugin}`) + return { + detected: true, + pluginName: match, + allPlugins: plugins, + } + } + } + + return { + detected: false, + pluginName: null, + allPlugins: plugins, + } +} + +/** + * Generate a warning message for users with conflicting notification plugins. + */ +export function getNotificationConflictWarning(pluginName: string): string { + return `[oh-my-opencode] External notification plugin detected: ${pluginName} + +⚠️ Both oh-my-opencode and ${pluginName} listen to session.idle events. + Running both simultaneously can cause crashes on Windows. + + oh-my-opencode's session-notification has been auto-disabled. + + To use oh-my-opencode's notifications instead, either: + 1. Remove ${pluginName} from your opencode.json plugins + 2. Or set "notification": { "force_enable": true } in oh-my-opencode.json` +} diff --git a/src/shared/index.ts b/src/shared/index.ts index 3c3f25e..d3502df 100644 --- a/src/shared/index.ts +++ b/src/shared/index.ts @@ -19,3 +19,4 @@ export * from "./migration" export * from "./opencode-config-dir" export * from "./opencode-version" export * from "./permission-compat" +export * from "./external-plugin-detector"