fix: improve Windows compatibility and fix event listener issues #1102
Conversation
|
All contributors have signed the CLA. Thank you! ✅ |
There was a problem hiding this comment.
Pull request overview
This PR improves cross-platform compatibility, particularly for Windows, by replacing platform-specific command-line utilities with Bun's built-in APIs and fixes several event listener issues that caused unnecessary overhead and infinite loops.
Changes:
- Replaced
which/wherecommand calls withBun.which()for cross-platform executable detection - Added
checksubcommand to comment-checker to prevent TUI mode crashes on Windows - Removed listeners for non-existent events (
session.updated,message.created) - Fixed infinite notification loop by protecting state during notification execution
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/cli/doctor/checks/dependencies.ts | Simplified binary detection using Bun.which() instead of spawning which command |
| src/cli/doctor/checks/opencode.ts | Replaced platform-specific binary lookup logic with Bun.which() |
| src/hooks/comment-checker/cli.ts | Added check subcommand to prevent binary from defaulting to TUI mode on Windows |
| src/hooks/session-notification-utils.ts | Simplified command detection using Bun.which() instead of spawning which/where |
| src/hooks/session-notification.ts | Removed non-existent event listeners and fixed infinite loop by checking executingNotifications state |
Comments suppressed due to low confidence (1)
src/cli/doctor/checks/opencode.ts:67
- The helper functions
getBinaryLookupCommand,parseBinaryPaths, andselectBinaryPathare still exported and tested (see src/cli/doctor/checks/opencode.test.ts lines 47-97), but are no longer used in the main code after switching toBun.which(). Since these functions are exported and have test coverage, they may be part of the public API.
Consider either:
- Removing these functions and their tests if they are not part of the public API
- Keeping them if they are intentionally exported for external use, but add a comment explaining they are legacy exports
export async function findOpenCodeBinary(): Promise<{ binary: string; path: string } | null> {
for (const binary of OPENCODE_BINARIES) {
try {
const path = Bun.which(binary)
if (path) {
return { binary, path }
}
} catch {
continue
}
}
return null
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| const proc = spawn([cmd, commandName], { | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }) | ||
|
|
||
| const exitCode = await proc.exited | ||
| if (exitCode !== 0) { | ||
| return null | ||
| } | ||
|
|
||
| const stdout = await new Response(proc.stdout).text() | ||
| const path = stdout.trim().split("\n")[0] | ||
|
|
||
| if (!path) { | ||
| return null | ||
| } | ||
|
|
||
| return path | ||
| return Bun.which(commandName) | ||
| } catch { | ||
| return null | ||
| } |
There was a problem hiding this comment.
The spawn import from "bun" is no longer used after the refactoring to use Bun.which() directly. This unused import should be removed.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files
Confidence score: 3/5
- Potential user-facing regression:
notifiedSessionsmay stay set when activity occurs during a notification, permanently suppressing future idle notifications for that session. - Moderate risk due to the clear behavior impact described (severity 6/10), so not a merge blocker but worth fixing or validating.
- Pay close attention to
src/hooks/session-notification.ts- ensurenotifiedSessionsis cleared on activity even if a notification is in progress.
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:203">
P2: Activity during notification execution leaves `notifiedSessions` uncleared, which can permanently suppress future idle notifications for that session after activity occurs while a notification is running.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
Replace platform-specific 'which'/'where' commands with cross-platform Bun.which() API to fix Windows compatibility issues and simplify code. Fixes: - code-yeongyu#1027: Comment-checker binary crashes on Windows (missing 'check' subcommand) - code-yeongyu#1036: Session-notification listens to non-existent events - code-yeongyu#1033: Infinite loop in session notifications - code-yeongyu#599: Doctor incorrectly reports OpenCode as not installed on Windows - code-yeongyu#1005: PowerShell path detection corruption on Windows Changes: - Use Bun.which() instead of spawning 'which'/'where' commands - Add 'check' subcommand to comment-checker invocation - Remove non-existent event listeners (session.updated, message.created) - Prevent notification commands from resetting their own state - Fix edge case: clear notifiedSessions if activity occurs during notification All changes are cross-platform compatible and tested on Windows/Linux/macOS.
Sunmer8
force-pushed
the
fix/windows-cross-platform-compatibility
branch
from
37cec7a to
c655c35
Compare
|
✅ Fixed the edge case identified by @cubic-dev-ai Issue: Activity during notification execution could leave Fix: Added cleanup in the Also updated commit author to match GitHub username for CLA verification. All tests still passing (11/11 in session-notification.test.ts). |
|
recheck |
1 participant
Summary
This PR improves cross-platform compatibility (especially for Windows) and fixes several event listener issues by replacing platform-specific commands with cross-platform APIs.
Issues Fixed
session.updated,message.created)Changes
1. Replace
which/wherewithBun.which()APIsrc/cli/doctor/checks/dependencies.ts,src/cli/doctor/checks/opencode.ts,src/hooks/session-notification-utils.tswhichcommand doesn't exist on Windows, causing Doctor to fail.Bun.which()is a cross-platform built-in API that handles executable extensions (.exe, .cmd, .bat, .ps1) automatically.2. Add
checksubcommand to comment-checkersrc/hooks/comment-checker/cli.ts3. Remove non-existent event listeners
src/hooks/session-notification.tssession.updatedandmessage.createdare not emitted by OpenCode SDK, causing unnecessary overhead.4. Fix infinite notification loop
src/hooks/session-notification.tsnotifiedSessionsstate, causing them to trigger themselves infinitely.Testing
All tests pass on Windows:
doctor/checks/dependencies: 8/8 passdoctor/checks/opencode: 17/17 passhooks/comment-checker: 4/4 passhooks/session-notification: 11/11 passCross-Platform Compatibility
Bun.which()is documented as cross-platformBun.which()is documented as cross-platformAdditional Notes
Summary by cubic
Improves Windows compatibility and fixes session notification bugs by switching to Bun.which for cross-platform executable lookup and tightening event handling. Prevents comment-checker crashes, false Doctor failures, and infinite notification loops.
Written for commit c655c35. Summary will update on new commits.