fix(windows): cross-platform detached worker spawn + backslash-safe hook dedup#221
Merged
Merged
Conversation
…ook dedup
On Windows the capture/summary hooks crashed with exit 1. The wiki and
skillify workers were spawned via `spawn("nohup", ["node", ...])`, but
nohup doesn't exist on Windows, so spawn emits an async ENOENT 'error'
event that — with no listener — went unhandled and killed the hook. The
summary worker fires only past a periodic message threshold, which is why
capture.js failed intermittently rather than every tool call.
Extract a shared src/utils/spawn-detached.ts that invokes the node binary
directly via process.execPath (dropping nohup), installs an 'error'
listener before unref, and relies on detached + ignored stdio to outlive
the parent on both POSIX and Windows. Route all five worker spawns
(claude/codex/cursor/hermes wiki + skillify) through it, so the workers
now actually run on Windows instead of crashing the hook.
Also fix duplicate hooks on Windows re-install: hook-dedup matched
forward-slash paths (`/bundle/`) while Windows writes backslash commands
via join(), so isHivemindHookEntry (codex) and isHivemindEntry (cursor)
never matched the prior install and every re-install appended a duplicate
PostToolUse hook. Normalize separators before matching.
Tests: real-process repro of the crash (old shape exits 1, fixed exits 0)
plus a real detached-worker E2E on Linux; backslash-string dedup cases and
a "5 re-installs => 1 entry" guard; and a windows-latest CI leg that runs
both suites on real win32.
Collaborator
Author
|
@coderabbitai review |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (18)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughConsolidates Windows path handling for hook detection, adds a cross-platform detached-worker helper, refactors multiple spawners to use it, centralizes project-name derivation, and adds Windows CI and tests. ChangesWindows Compatibility and Worker Spawning
Sequence Diagram(s)sequenceDiagram
participant Caller as Hook/Skillify Code
participant Helper as spawnDetachedNodeWorker
participant ChildProc as node:child_process.spawn
participant Worker as Detached Worker
Caller->>Helper: workerPath, [configFile], deps?
Helper->>ChildProc: execPath, [workerPath, ...args], {detached:true, stdio:['ignore','ignore','ignore'], windowsHide:true}
ChildProc-->>Helper: childProcess
Helper->>ChildProc: .on('error', swallow)
Helper->>ChildProc: .unref()
Note over Caller: returns immediately (non-blocking)
Note over Worker: continues independently
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Actions performedReview triggered.
|
Contributor
Coverage ReportScope: files changed in this PR. Enforced threshold: 90% per metric (per file via
File Coverage — 18 files changed
Generated for commit 4bbe51e. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/spawn-wiki-worker.ts (1)
93-93:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
cwd.split("/").pop()won't extract the basename on Windows.This PR targets Windows compatibility, but
projectNamestill splits on/only. A Windowscwdsuch asC:\work\repohas no forward slashes, sopop()returns the entire path instead ofrepo, polluting theprojectfield threaded into the worker config and summary rows. Usepath.basename(already importing fromnode:path) for cross-platform correctness.🐛 Proposed fix
-import { dirname, join } from "node:path"; +import { basename, dirname, join } from "node:path";- const projectName = cwd.split("/").pop() || "unknown"; + const projectName = basename(cwd) || "unknown";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/spawn-wiki-worker.ts` at line 93, The projectName extraction uses cwd.split("/") which fails on Windows; replace that logic by calling path.basename(cwd) where projectName is defined (the const projectName = ... line in spawn-wiki-worker.ts) so it returns the cross-platform directory name; ensure the existing import from node:path is used and remove the split("/").pop() approach so project field passed into the worker config and summary rows contains the correct basename on all platforms.
🧹 Nitpick comments (1)
src/utils/spawn-detached.ts (1)
55-58: ⚡ Quick winConsider adding
windowsHide: trueto reduce Windows console-window flashes
On Windows,child_process.spawn’swindowsHide: trueis intended to suppress the console window created for console apps; withdetached: truethe outcome can still vary by launch/OS flags, but settingwindowsHideimproves the chances and is effectively a no-op on POSIX.♻️ Proposed tweak
const child = spawn(execPath, [workerPath, ...args], { detached: true, stdio: ["ignore", "ignore", "ignore"], + windowsHide: true, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/spawn-detached.ts` around lines 55 - 58, The spawn call in spawnDetached (where spawn(execPath, [workerPath, ...args], { detached: true, stdio: [...] })) can cause Windows console flashes; add windowsHide: true to the options object so the options become { detached: true, stdio: ["ignore","ignore","ignore"], windowsHide: true } to suppress console windows on Windows while remaining a no-op on POSIX.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yaml:
- Line 68: The checkout step using actions/checkout@v6.0.2 must explicitly
disable credential persistence; update the checkout step that references
actions/checkout@v6.0.2 to add the key persist-credentials: false under that
step so tokens are not persisted to subsequent steps or artifacts.
---
Outside diff comments:
In `@src/hooks/spawn-wiki-worker.ts`:
- Line 93: The projectName extraction uses cwd.split("/") which fails on
Windows; replace that logic by calling path.basename(cwd) where projectName is
defined (the const projectName = ... line in spawn-wiki-worker.ts) so it returns
the cross-platform directory name; ensure the existing import from node:path is
used and remove the split("/").pop() approach so project field passed into the
worker config and summary rows contains the correct basename on all platforms.
---
Nitpick comments:
In `@src/utils/spawn-detached.ts`:
- Around line 55-58: The spawn call in spawnDetached (where spawn(execPath,
[workerPath, ...args], { detached: true, stdio: [...] })) can cause Windows
console flashes; add windowsHide: true to the options object so the options
become { detached: true, stdio: ["ignore","ignore","ignore"], windowsHide: true
} to suppress console windows on Windows while remaining a no-op on POSIX.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c625e01-e931-474d-aaf0-a473781fef92
📒 Files selected for processing (12)
.github/workflows/ci.yamlsrc/cli/install-codex.tssrc/cli/install-cursor.tssrc/hooks/codex/spawn-wiki-worker.tssrc/hooks/cursor/spawn-wiki-worker.tssrc/hooks/hermes/spawn-wiki-worker.tssrc/hooks/spawn-wiki-worker.tssrc/skillify/spawn-skillify-worker.tssrc/utils/spawn-detached.tstests/claude-code/spawn-wiki-worker.test.tstests/cli/install-helpers.test.tstests/shared/spawn-detached.test.ts
…rabbit) CodeRabbit/zizmor flagged the new windows-smoke checkout for persisting the checkout token into .git/config (artipacked). The job only reads + tests, so disable credential persistence per the repo workflow-hardening guideline.
Suppress the transient console window Windows pops for the detached worker; no-op on POSIX. Asserted in the spawn-detached unit test.
cwd.split("/").pop() only splits on forward slashes, so on Windows a cwd like
C:\work\repo returned the whole path instead of `repo`, polluting the project
field threaded into capture rows, session rows, and worker summaries.
Extract projectNameFromCwd (path.basename, platform-aware) and route all 13
hook call-sites through it: capture / session-start / stop and the four
wiki-worker spawns across claude/codex/cursor/hermes. New unit test asserts
win32 vs posix basename semantics and guards the old split("/") negative
pattern.
Collaborator
Author
|
Addressed the two out-of-diff / nitpick findings:
|
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
khustup2
approved these changes
May 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Reported by a Windows/PowerShell Codex user: the Hivemind PostToolUse capture hook (
capture.js) intermittently exited 1, andhooks.jsonhad a duplicated PostToolUse entry (capture ran twice). Two distinct Windows bugs:Bug 1 — the crash (exit 1)
The wiki-summary and skillify workers were spawned as:
nohupdoesn't exist on Windows →spawnemits an asyncENOENT'error' event, not a sync throw. With noerrorlistener, that event is unhandled and crashes the parent hook with exit 1. The summary worker only fires past a periodic message threshold (first at N captured, then every M), which is why it failed sometimes, not every tool call. The pattern existed in 5 helpers — codex/claude/cursor/hermes wiki workers + skillify — onlyspawn-pull-worker.tshad previously been fixed.Bug 2 — the duplicate PostToolUse hook
isHivemindHookEntry(codex) /isHivemindEntry(cursor) matched with forward-slash literals (/bundle/), but on Windows the hook command is written viajoin()with backslashes (node "C:\Users\...\hivemind\bundle\capture.js"). So dedup never matched the prior install → every re-install appended a duplicate.Fix
src/utils/spawn-detached.ts— invokes the node binary directly viaprocess.execPath(dropsnohup), installs anerrorlistener beforeunref(), and relies ondetached+ ignored stdio to outlive the parent on both POSIX and Windows. Net effect: the workers now actually run on Windows instead of crashing the hook. All 5 spawn sites routed through it.install-codex.ts(isHivemindHookEntry+isForeignHivemindHookEntry) andinstall-cursor.ts(isHivemindEntry) so backslash commands match. Pi uses marker blocks — unaffected.Testing — 3 layers
Neither bug actually requires Windows to test; both are mechanisms Linux reproduces on demand:
nohupmissing on Windows" ≡ "any missing binary on Linux."process.execPath+ a tiny worker that writes a sentinel; asserts it ran with the right arg and the call returned <1s (fire-and-forget). Plus an in-process test that calls the real helper with a bogus execPath and survives.windows-smokejob (windows-latest) runs the spawn + install-dedup suites on real win32 — scoped, since the rest of the suite assumes POSIX paths/chmod/symlinks.Bug 2 is pure string logic, so tests feed the exact backslash command
join()produces, including a "5 re-installs → still 1 PostToolUse entry" repro of the reported symptom.133/133 passing across all touched suites (9 in the new spawn-detached suite, 4 of them real-process).
Not covered
windows-latestrun (executes when this PR's CI runs).capture.jsbundle — blocked locally by the missing nativetree-sitteroptional dep (unmodifiedmainfailstscidentically; environmental, not this change).Summary by CodeRabbit
Bug Fixes
Improvements
Tests