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
cccd159f7d
commit
1570e292fb
23
src/index.ts
23
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)
|
||||
|
||||
Reference in New Issue
Block a user