fix(cli): Bun Windows compatibility — shell:true child_process workaround and iii-exec config#917
Open
JackieJK wants to merge 5 commits into
Open
fix(cli): Bun Windows compatibility — shell:true child_process workaround and iii-exec config#917JackieJK wants to merge 5 commits into
JackieJK wants to merge 5 commits into
Conversation
…ound and iii-exec config On Bun for Windows, child_process cannot spawn native Rust-compiled executables (oven-sh/bun#32011). execFileSync and spawn hang, causing iii version check and engine startup to fail silently. - Add IS_BUN constant for runtime detection - Use shell:true in execFileSync/spawn on Windows+Bun - Generate Bun-compatible iii-config with \�un run src/index.ts\ replacing \ ode dist/index.mjs\ in the iii-exec worker
|
@JackieJK is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds comprehensive Bun runtime support via detection flags, iii config rewriting for Bun launcher commands, subprocess compatibility for Windows+Bun, hook execution through a cross-runtime runner, DNS/ONNX/dependency hints for service compatibility, and package config updates. Also refactors AgentSDKProvider's recursion guard from Node-only AsyncLocalStorage to a cross-runtime callId/Map approach. ChangesBun Runtime Support
SDK Provider Recursion Guard
Sequence DiagramssequenceDiagram
participant User as User/CLI
participant CLI as startEngine()
participant Config as writeBunCompatibleConfig()
participant FS as File System
participant Engine as iii Engine
User->>CLI: start engine
CLI->>Config: IS_BUN detected
Config->>FS: read original iii-config.yaml
Config->>FS: rewrite node→bun, write to ~/.agentmemory/iii-config.bun.yaml
Config-->>CLI: return Bun config path
CLI->>Engine: spawn with Bun config path
Engine-->>User: engine running
sequenceDiagram
participant Plugin as Copilot/Claude Plugin
participant HookConfig as hooks.json / hooks.copilot.json
participant Runner as hook-runner.mjs
participant Runtime as Bun/Node Runtime
participant HookScript as Hook Script (e.g., session-start.mjs)
Plugin->>HookConfig: event triggered
HookConfig->>Runner: shell exec hook-runner.mjs [script-path]
Runner->>Runtime: detect environment (IS_BUN)
Runtime-->>Runner: runtime type
Runner->>HookScript: spawn script via detected runtime
HookScript->>Plugin: completion
stateDiagram-v2
[*] --> Idle: AgentSDKProvider created
Idle --> TopLevel: query() called (no parent)
TopLevel --> AllocateCallId: nextCallId++ and add to sdkActiveCalls
AllocateCallId --> ExecuteSDK: sdkContext.run(callId, ...)
ExecuteSDK --> MayReenter: nested query() call
MayReenter --> CheckCallId: sdkContext.getStore() == currentCallId?
CheckCallId --> ShortCircuit: yes, already active
CheckCallId --> AllocateNested: no, allocate new callId
ShortCircuit --> ReturnEmpty: return ''
AllocateNested --> ExecuteSDK: execute nested query
ExecuteSDK --> CleanupInner: inner finally
CleanupInner --> DecrementRefCount: sdkActiveCount--, restore AGENTMEMORY_SDK_CHILD
DecrementRefCount --> CleanupOuter: outer finally
CleanupOuter --> RemoveCallId: sdkActiveCalls.delete(callId)
RemoveCallId --> [*]
ReturnEmpty --> RemoveCallId
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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 unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/cli.ts`:
- Around line 394-396: After reading and replacing the Bun config (the
readFileSync -> content and writeFileSync to bunPath using the regex /- node
dist\/index\.mjs/ -> `- ${runtimeCmd} src/index.ts`), verify that the
replacement actually changed the file by comparing the original read string
(from originalPath) to the new content; if they are equal (no match applied),
throw or exit with a clear error mentioning bunPath and the expected pattern so
the process fails fast instead of silently writing an unchanged config. Use the
existing symbols content, originalPath, bunPath, runtimeCmd and the
regex/pattern to perform the check before calling writeFileSync.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Address CodeRabbit review: verify the config replacement actually changed the file. If upstream iii-config.yaml drifts, throw instead of silently writing an unchanged config.
…ation Replace three Node.js-only APIs with cross-runtime equivalents so the project runs under both Node.js >=20 and Bun >=1.1: - AsyncLocalStorage → per-call Map recursion guard (agent-sdk.ts) - node:dns/promises → Bun.dns.lookup() shim (mesh.ts) - Force ONNX WASM backend under Bun (local.ts) - Bun-aware CJK hint in cjk-segmenter.ts Add cross-runtime hook launcher (scripts/hook-runner.mjs) that detects the current runtime and spawns hook scripts with the correct binary. Update all 22 hook commands in hooks.json + hooks.copilot.json to use it. Switch package.json scripts from hardcoded node to tsx (already a dev dependency, works on both runtimes). Add bun >=1.1.0 to engines.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/functions/mesh.ts (1)
18-20: ⚡ Quick winTypeScript guideline drift: WHAT-style explanatory comments were added in runtime helper blocks. Replace these comments with clearer identifier names and keep intent encoded in code structure.
src/functions/mesh.ts#L18-L20: remove the descriptive DNS helper comments and rely on the helper/function naming.src/providers/embedding/local.ts#L3-L10: remove the Bun/ONNX explanatory comments and keep the runtime intent in naming and flow.As per coding guidelines, for
src/**/*.ts: “In TypeScript source code, avoid code comments explaining WHAT — use clear naming instead.”🤖 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/functions/mesh.ts` around lines 18 - 20, The code contains WHAT-style explanatory comments that violate the TypeScript coding guideline of using clear naming instead of comments to convey intent. Fix this at two locations: (1) in src/functions/mesh.ts lines 18-20, remove the comments describing the DNS lookup behavior and cross-runtime handling, relying instead on the variable name IS_BUN and function/method naming to convey the intent of the runtime detection logic; (2) in src/providers/embedding/local.ts lines 3-10, remove the explanatory comments about Bun and ONNX runtime handling and similarly encode the intent through clear identifier names and code structure. Ensure the remaining code (variable declarations, function names, and control flow) clearly communicates what is being detected or handled without the descriptive comments.Source: Coding guidelines
🤖 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 `@plugin/hooks/hooks.copilot.json`:
- Around line 7-68: The command entries in this hooks configuration file use
unquoted ${COPILOT_PLUGIN_ROOT} paths which will fail on systems where the
plugin root contains spaces. Fix this by adding double quotes around both the
hook-runner.mjs path and the individual script paths in all command values
throughout the file. Apply this consistently to all hooks including
userPromptSubmitted, preToolUse, postToolUse, postToolUseFailure, preCompact,
agentStop, sessionEnd, subagentStart, subagentStop, and notification hooks,
following the same quoting pattern already used in the plugin/hooks/hooks.json
file.
In `@src/providers/agent-sdk.ts`:
- Around line 25-26: The recursion guard is not working correctly because the
active call id is allocated fresh on every call (around line 68) and only
inserted into sdkActiveCalls after the recursion check (around line 69-81), so
re-entrant calls are never detected. Fix this by using async-local context or
storage (such as AsyncLocalStorage) to track the active call id per async
execution context. For root calls, allocate a new id and store it in both the
async context and sdkActiveCalls. For nested/re-entrant calls, first check if an
active call id already exists in the async context, and if so, verify it exists
in sdkActiveCalls to detect recursion before proceeding with the SDK query. This
ensures that when provider.summarize() is called recursively, the inner call
will detect the outer call's id in the active calls map and return an empty
string instead of making a new SDK query.
---
Nitpick comments:
In `@src/functions/mesh.ts`:
- Around line 18-20: The code contains WHAT-style explanatory comments that
violate the TypeScript coding guideline of using clear naming instead of
comments to convey intent. Fix this at two locations: (1) in
src/functions/mesh.ts lines 18-20, remove the comments describing the DNS lookup
behavior and cross-runtime handling, relying instead on the variable name IS_BUN
and function/method naming to convey the intent of the runtime detection logic;
(2) in src/providers/embedding/local.ts lines 3-10, remove the explanatory
comments about Bun and ONNX runtime handling and similarly encode the intent
through clear identifier names and code structure. Ensure the remaining code
(variable declarations, function names, and control flow) clearly communicates
what is being detected or handled without the descriptive comments.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4f75ab9-1be0-43dc-8971-15de0a4178c9
📒 Files selected for processing (8)
package.jsonplugin/hooks/hooks.copilot.jsonplugin/hooks/hooks.jsonscripts/hook-runner.mjssrc/functions/mesh.tssrc/providers/agent-sdk.tssrc/providers/embedding/local.tssrc/state/cjk-segmenter.ts
✅ Files skipped from review due to trivial changes (1)
- src/state/cjk-segmenter.ts
…ng, comment cleanup - agent-sdk.ts: replace broken callId check with AsyncLocalStorage for per-async-context recursion detection (concurrent siblings get separate contexts, recursive re-entry is blocked) - hooks.copilot.json: quote COPILOT_PLUGIN_ROOT paths to handle spaces, matching hooks.json pattern - mesh.ts, local.ts: remove WHAT-style comments per coding guidelines
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
1 participant
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.
What
Fixes agentmemory CLI startup under Bun on Windows, where child_process cannot spawn native Rust-compiled executables (oven-sh/bun#32011).
Three changes, all gated behind an IS_BUN runtime detection:
No impact on Node.js — all three changes are IS_WINDOWS && IS_BUN conditional.
Why
Bun's uv_spawn-based child_process implementation on Windows cannot spawn PE binaries with certain subsystem characteristics (iii.exe and other Rust-compiled executables). execFileSync and spawnSync both hang; spawn silently fails. The documented workaround is shell: true, which routes through cmd.exe.
Root cause is upstream in Bun (oven-sh/bun#32011). This PR is a defensive workaround so agentmemory works on Bun without waiting for the upstream fix.
How to verify
Summary by CodeRabbit
Summary
New Features
Bug Fixes
Chores