fix(session-notification): revert PR #543 and add proper notification plugin conflict detection #575

JohnC0de · 2026-01-07T13:38:23Z

Summary

This PR reverts PR #543 and implements the actual fix for the Windows crash issue.

What Changed

Reverts PR fix(session-notification): avoid Bun shell GC crash on Windows #543: Restores original ctx.$ usage
Adds conflict detection: Automatically detects external notification plugins
Auto-disables on conflict: Skips session-notification when conflict detected
Adds documentation: Full investigation timeline in docs/CRASH_INVESTIGATION_TIMELINE.md

The Investigation Story

Initial Diagnosis (Wrong)

PR #543 assumed Bun's ShellInterpreter GC bug was causing crashes. We replaced ctx.$ with node:child_process.spawn to work around it.

What Actually Happened

After PR #543 merged, crashes continued. Further investigation revealed the real pattern:

User removed @mohak34/opencode-notifier plugin → crashes stopped immediately

The Real Root Cause

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 causes crashes on Windows due to resource contention:

	oh-my-opencode	opencode-notifier
Notification	`ctx.$` → PowerShell → Windows.UI.Notifications	`node-notifier` → SnoreToast.exe
Timing	1.5s delay	Immediate
Sound	PowerShell Media.SoundPlayer	PowerShell Media.SoundPlayer

Two different notification systems competing for the same Windows resources = crash.

Evidence

✅ Removing only the notifier plugin fixed crashes
✅ Release version (with original ctx.$) works fine alone
✅ No widespread complaints from users without external notifiers
❌ PR fix(session-notification): avoid Bun shell GC crash on Windows #543's assumption was based on superficial pattern matching with Bun issues

The Proper Fix

1. External Plugin Detection

New detectExternalNotificationPlugin() function that:

Reads opencode.json plugin list
Checks for known notification plugins
Returns detection result

2. Auto-Disable on Conflict

When external notifier detected:

[oh-my-opencode] External notification plugin detected: opencode-notifier

⚠️  Both oh-my-opencode and opencode-notifier listen to session.idle events.
   Running both simultaneously can cause crashes on Windows.

   oh-my-opencode's session-notification has been auto-disabled.

3. Config Override

Users can force-enable if needed:

{
  "notification": {
    "force_enable": true
  }
}

Testing

✅ TypeScript compiles cleanly
✅ Session-notification tests: 11 pass
✅ External-plugin-detector tests: 7 pass
✅ Full test suite: 649 pass, 6 fail (pre-existing unrelated failures)

Files Changed

File	Change
`src/hooks/session-notification.ts`	Reverted to `ctx.$`
`src/hooks/session-notification.test.ts`	Reverted test mocks
`src/shared/external-plugin-detector.ts`	NEW - conflict detection
`src/shared/external-plugin-detector.test.ts`	NEW - tests
`src/config/schema.ts`	Added `notification.force_enable`
`src/index.ts`	Added conflict check before creating hook
`docs/CRASH_INVESTIGATION_TIMELINE.md`	Full investigation documentation

Reverts: fix(session-notification): avoid Bun shell GC crash on Windows #543
Related: feat(debug): add comprehensive crash tracing system for Windows debugging #571, fix(lsp): add resilient handling for missing LSP server binaries #572 (will need updates)
External plugin: https://github.com/mohak34/opencode-notifier

Lessons Learned

Correlation ≠ Causation: Stack traces can mislead
Test exact user environment: The crash only happened with specific plugin combo
Challenge assumptions: "Bun.spawn is buggy" was accepted too quickly
Evidence-based debugging: User's discovery was the key evidence

Summary by cubic

Restores session-notification's use of ctx.$ and adds detection of external notifier plugins. On conflict, session-notification is auto-disabled to prevent Windows crashes.

Bug Fixes
- Restore ctx.$ for notification and sound commands on all platforms.
- Detect external notifier plugins and skip session-notification when found.
- Log a clear warning explaining the conflict and why notifications were disabled.
New Features
- Add notification.force_enable config to override and keep session-notification enabled.
- Introduce external plugin detector with tests and integrate it in plugin initialization.
- Add crash investigation timeline documentation.

^{Written for commit 6230287. Summary will update on new commits.}

…iagnosed) This reverts commit 4a38e70 (PR code-yeongyu#543) and 2064568 (follow-up fix). ## Why This Revert The original diagnosis was incorrect. PR code-yeongyu#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: code-yeongyu#543, oven-sh/bun#23177, oven-sh/bun#24368 See: docs/CRASH_INVESTIGATION_TIMELINE.md (in follow-up commit)

…tification 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 code-yeongyu#543). Refs: code-yeongyu#543

Documents the investigation journey from initial misdiagnosis (Bun GC bug) to discovering the actual root cause (notification plugin conflict). Key findings: - PR code-yeongyu#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

greptile-apps

Greptile Overview

Greptile Summary

This PR correctly reverts PR #543 and implements proper conflict detection between oh-my-opencode's session-notification and external notification plugins like opencode-notifier, which was the actual root cause of Windows crashes.

Key Changes:

Reverted session-notification.ts to use ctx.$ (Bun shell) instead of node:child_process.spawn - the original implementation was never the problem
Added external-plugin-detector.ts utility that scans opencode.json for conflicting notification plugins
Modified src/index.ts to auto-disable session-notification when external notifiers are detected (with console warning)
Added notification.force_enable config option for users who want to override the auto-disable behavior
Excellent documentation in CRASH_INVESTIGATION_TIMELINE.md explaining the investigation process and lessons learned

Minor Issue:

The new external-plugin-detector.ts reimplements JSONC parsing with a custom regex-based stripJsonComments function, but the codebase already has a robust jsonc-parser module that should be used instead

Confidence Score: 4/5

Safe to merge with one minor refactoring suggestion
The PR correctly addresses the root cause with evidence-based reasoning, includes comprehensive tests (18 passing tests across both new and reverted files), and follows TDD principles. Score is 4 instead of 5 due to the duplicated JSONC parsing logic that should use the existing jsonc-parser module for better maintainability
src/shared/external-plugin-detector.ts should be refactored to use the existing jsonc-parser module instead of custom regex-based comment stripping

Important Files Changed

File Analysis

Filename	Score	Overview
src/shared/external-plugin-detector.ts	4/5	New conflict detection utility with custom JSONC parser; should use existing `jsonc-parser` module instead
src/hooks/session-notification.ts	5/5	Reverted to use `ctx.$` - correctly restores original implementation that was working fine
src/index.ts	5/5	Added plugin conflict detection before creating session-notification hook with proper logging and config override support
src/config/schema.ts	5/5	Added `NotificationConfigSchema` with `force_enable` option - clean implementation following existing patterns

Sequence Diagram

sequenceDiagram
    participant Plugin as OhMyOpenCodePlugin
    participant Detector as detectExternalNotificationPlugin
    participant FS as FileSystem
    participant Config as opencode.json
    participant Hook as session-notification
    participant User as User Console

    Note over Plugin: Plugin initialization starts
    Plugin->>Plugin: Check if session-notification enabled
    
    alt session-notification enabled
        Plugin->>Plugin: Check notification.force_enable config
        
        alt force_enable = false (default)
            Plugin->>Detector: detectExternalNotificationPlugin(ctx.directory)
            Detector->>Detector: getConfigPaths(directory)
            
            loop For each config path
                Detector->>FS: Check if config file exists
                FS-->>Detector: exists/not exists
                
                alt Config exists
                    Detector->>Config: Read and parse JSONC
                    Config-->>Detector: { plugin: [...] }
                    Detector->>Detector: matchesNotificationPlugin(plugin)
                    
                    alt Match found (e.g., "opencode-notifier")
                        Detector-->>Plugin: { detected: true, pluginName: "opencode-notifier" }
                        Plugin->>User: console.warn(conflict warning)
                        Plugin->>Plugin: sessionNotification = null (skip creation)
                        Note over Plugin,Hook: session-notification disabled
                    end
                end
            end
            
            alt No conflict detected
                Detector-->>Plugin: { detected: false }
                Plugin->>Hook: createSessionNotification(ctx)
                Note over Hook: Uses ctx.$ for notifications
            end
        else force_enable = true
            Plugin->>Hook: createSessionNotification(ctx)
            Note over Hook: Force-enabled by user config
        end
    end

    Note over Plugin: Plugin continues initialization

    rect rgb(240, 240, 255)
        Note over Hook: When session.idle event occurs
        Hook->>Hook: Check if main session
        Hook->>Hook: Wait 1.5s idle confirmation
        Hook->>Hook: Send notification via ctx.$
        
        alt Windows
            Hook->>Hook: PowerShell + Windows.UI.Notifications
        else macOS
            Hook->>Hook: osascript display notification
        else Linux
            Hook->>Hook: notify-send
        end
    end

greptile-apps · 2026-01-07T13:42:16Z

Additional Comments (2)

src/shared/external-plugin-detector.ts
Custom JSONC parser duplicates existing functionality - use parseJsoncSafe from ./jsonc-parser instead

The codebase already has a proper JSONC parser using the jsonc-parser library in src/shared/jsonc-parser.ts. The regex-based stripJsonComments function is less robust and doesn't handle all edge cases (e.g., escaped quotes, block comments in strings).

// Remove this function - use existing parseJsoncSafe from ./jsonc-parser

src/shared/external-plugin-detector.ts
Replace custom JSONC parsing with existing utility

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<OpencodeConfig>(content)
      if (result.data) {
        return result.data.plugin ?? []
      }
    } catch {
      continue
    }
  }
  return []
}

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

cubic-dev-ai

2 issues found across 8 files

Confidence score: 3/5

Custom JSONC parsing in src/shared/external-plugin-detector.ts ignores the documented parseJsonc/readJsoncFile utilities, risking inconsistent behavior and future maintenance headaches.
src/hooks/session-notification.ts drops single-quote escaping for soundPath, so PowerShell commands can break or be exploited when paths contain ' characters.
Pay close attention to src/shared/external-plugin-detector.ts, src/hooks/session-notification.ts - ensure JSONC parsing follows project utilities and PowerShell sound paths are properly escaped.

Prompt for AI agents (all issues)


Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="src/hooks/session-notification.ts">

<violation number="1" location="src/hooks/session-notification.ts:125">
P2: Missing single quote escaping for `soundPath` in PowerShell command. If the path contains single quotes, the command will fail or could be exploited. The previous code escaped with `.replace(/'/g, "''")`.</violation>
</file>

<file name="src/shared/external-plugin-detector.ts">

<violation number="1" location="src/shared/external-plugin-detector.ts:61">
P2: Use the project's existing `parseJsonc` or `readJsoncFile` utility instead of custom `stripJsonComments` + `JSON.parse`. The project documentation explicitly lists this as an anti-pattern, and `src/shared/jsonc-parser.ts` already provides robust JSONC parsing via the `jsonc-parser` library.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

src/hooks/session-notification.ts

src/shared/external-plugin-detector.ts

code-yeongyu · 2026-01-07T14:39:06Z

@sisyphus-dev-ai fix reviews on there, commit, push

…ng 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

sisyphus-dev-ai · 2026-01-07T14:43:22Z

✅ Review Feedback Addressed

Fixed both issues identified by the AI reviewers:

Issue 1: PowerShell Single-Quote Escaping (session-notification.ts:125)

Problem: Missing single-quote escaping for soundPath in PowerShell command could cause failures or security issues when paths contain single quotes
Fix: Added back .replace(/'/g, "''") to properly escape single quotes in PowerShell strings
File: src/hooks/session-notification.ts

Issue 2: Custom JSONC Parsing (external-plugin-detector.ts:61)

Problem: Custom regex-based stripJsonComments function duplicates existing functionality and is less robust
Fix: Replaced custom implementation with existing parseJsoncSafe from src/shared/jsonc-parser.ts
Files: src/shared/external-plugin-detector.ts

Verification

✅ All 655 tests pass
✅ Changes committed and pushed

Ready for re-review! 🚀

JohnC0de added 3 commits January 7, 2026 10:29

This was referenced Jan 7, 2026

feat(debug): add comprehensive crash tracing system for Windows debugging #571

Closed

fix(lsp): add resilient handling for missing LSP server binaries #572

Open

greptile-apps bot reviewed Jan 7, 2026

View reviewed changes

cubic-dev-ai bot reviewed Jan 7, 2026

View reviewed changes

src/hooks/session-notification.ts Outdated Show resolved Hide resolved

src/shared/external-plugin-detector.ts Outdated Show resolved Hide resolved

sisyphus-dev-ai added the sisyphus: working Sisyphus is currently working on this label Jan 7, 2026

fix: address review feedback - add PowerShell escaping and use existi…

6230287

…ng 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

sisyphus-dev-ai removed the sisyphus: working Sisyphus is currently working on this label Jan 7, 2026

code-yeongyu merged commit 1570e29 into code-yeongyu:dev Jan 7, 2026
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(session-notification): revert PR #543 and add proper notification plugin conflict detection #575

fix(session-notification): revert PR #543 and add proper notification plugin conflict detection #575

Uh oh!

JohnC0de commented Jan 7, 2026 •

edited by cubic-dev-ai bot

Loading

Uh oh!

greptile-apps bot left a comment

Uh oh!

greptile-apps bot commented Jan 7, 2026

Uh oh!

cubic-dev-ai bot left a comment

Uh oh!

Uh oh!

Uh oh!

code-yeongyu commented Jan 7, 2026

Uh oh!

sisyphus-dev-ai commented Jan 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fix(session-notification): revert PR #543 and add proper notification plugin conflict detection #575

fix(session-notification): revert PR #543 and add proper notification plugin conflict detection #575

Uh oh!

Conversation

JohnC0de commented Jan 7, 2026 • edited by cubic-dev-ai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What Changed

The Investigation Story

Initial Diagnosis (Wrong)

What Actually Happened

The Real Root Cause

Evidence

The Proper Fix

1. External Plugin Detection

2. Auto-Disable on Conflict

3. Config Override

Testing

Files Changed

Related

Lessons Learned

Summary by cubic

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Greptile Overview

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Sequence Diagram

Uh oh!

greptile-apps bot commented Jan 7, 2026

Uh oh!

cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

code-yeongyu commented Jan 7, 2026

Uh oh!

sisyphus-dev-ai commented Jan 7, 2026

✅ Review Feedback Addressed

Issue 1: PowerShell Single-Quote Escaping (session-notification.ts:125)

Issue 2: Custom JSONC Parsing (external-plugin-detector.ts:61)

Verification

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JohnC0de commented Jan 7, 2026 •

edited by cubic-dev-ai bot

Loading