fix(mcp): replace sh launcher with cross-platform node -e (closes #2461)#2594
fix(mcp): replace sh launcher with cross-platform node -e (closes #2461)#2594YOMXXX wants to merge 3 commits into
Conversation
Greptile SummaryThis PR replaces the
Confidence Score: 3/5The launcher logic itself is sound and the signal handling is well-done, but the PR hand-edits a file governed by a byte-exact build assertion without updating the canonical generator, which will cause npm run build to throw before this can even be shipped. The build system in build-hooks.js has an explicit Rule A guard that compares plugin/.mcp.json args[1] against the output of buildShellCommand() in src/build/hook-shell-template.ts. That generator still emits the old sh-style string; the PR only changes plugin/.mcp.json, so the equality check fails and npm run build throws 'Hand-edited shell string detected.' The fix also needs to update hook-shell-template.ts and the shellTemplateManifest in build-hooks.js. plugin/.mcp.json — the change itself is correct, but src/build/hook-shell-template.ts and scripts/build-hooks.js also need updates to satisfy the Rule A assertion that guards this file. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[MCP host spawns: node -e inline_script] --> B{CLAUDE_PLUGIN_ROOT\nor PLUGIN_ROOT set?}
B -- yes --> C[Try env-injected root]
B -- no --> D[Try cwd/plugin, cwd]
C --> E{mcp-server.cjs\nfound?}
D --> F[Try versioned dirs in\neach cache root separately\nsorted by mtime per root]
F --> G[~/.codex/.../claude-mem-local/...]
G --> H[~/.codex/.../thedotmack/...]
H --> I[~/.claude/.../thedotmack/...]
I --> J[Try marketplace install dir]
E -- yes --> K[spawn mcp-server.cjs\nwith stdio:inherit]
J --> L{mcp-server.cjs\nfound?}
L -- yes --> K
L -- no --> M[stderr: mcp server not found\nprocess.exit 1]
K --> N[Forward SIGTERM/SIGINT/SIGHUP\nto child]
N --> O{child exits\nvia signal?}
O -- yes --> P[re-raise signal on\nlauncher process]
O -- no --> Q[process.exit with\nchild exit code]
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
plugin/.mcp.json:8
**Rule A assertion will fail — `npm run build` will throw**
`scripts/build-hooks.js` enforces a "Rule A" constraint via `verifyShellTemplateCanonical()` (lines 127–178). It calls `buildShellCommand({ host: 'mcp', requireFile: 'mcp-server.cjs', trailingCommand: ['exec', 'node', '"$_P/scripts/mcp-server.cjs"'], ... })` from `src/build/hook-shell-template.ts` and does a **byte-for-byte equality check** against `parsed.mcpServers['mcp-search'].args[1]` (line 151). Since `hook-shell-template.ts` still generates the old POSIX-shell string (with `ls -dt`, `_C=`, `_E=`, etc.), but `plugin/.mcp.json` now has the new Node.js inline script, these two strings differ and the check throws: "Hand-edited shell string detected in plugin/.mcp.json (mcp-search). It no longer matches src/build/hook-shell-template.ts. Update the generator (and this manifest) instead of hand-editing the launcher."
The PR must also update `src/build/hook-shell-template.ts` (add a `host === 'mcp'` branch that emits the Node.js inline launcher) and align the `shellTemplateManifest` in `build-hooks.js` accordingly. The stale comment in `hook-shell-template.ts` at line 149–152 ("MCP runs under `sh -c`") would need updating too.
### Issue 2 of 2
plugin/.mcp.json:8
**Versioned-cache mtime sort is no longer global across all three paths**
The old `ls -dt "$HOME/.codex/.../claude-mem-local"/[0-9]*/ "$HOME/.codex/.../thedotmack"/[0-9]*/ "$_C/.../thedotmack"/[0-9]*/` was a single command that sorted all matching version directories together by mtime descending. The new code calls `L()` separately for each cache root and spreads the results — so all entries from path 4 (claude-mem-local) are tried before any entry from path 5 (thedotmack), regardless of mtime. A user who has a newer `thedotmack` install and an older `claude-mem-local` install would get the older binary. This is an edge case, but the PR description claims the fallback order is "exact same" which isn't accurate here.
Reviews (4): Last reviewed commit: "fix(mcp): use exit(0) instead of exit(12..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Addresses both points in the Greptile review on thedotmack#2594. The previous launcher had two POSIX regressions vs. the old `exec node ...` approach: 1. Signal forwarding gap: the MCP host holds the launcher's PID, not mcp-server.cjs's. SIGTERM to the launcher would exit the launcher via Node's default handler but leave the child orphaned, holding its stdio descriptors / file locks. 2. Signal-death exit code: spawn's `exit` callback fires with `(code, signal)`. When the child dies via signal, `code` is null and `signal` is e.g. `'SIGTERM'`. The previous handler returned `0` (clean exit), masking the real cause from the host. Fix: - Register handlers for SIGTERM/SIGINT/SIGHUP on the launcher that forward to `ch.kill(sig)`. try/catch in case the child is already dead. - In `ch.on('exit')`, when signal is non-null, deregister our own handler for that signal and re-raise it via `process.kill(process.pid, sig)`. The deregister step is critical — Node calls registered handlers instead of dying, so without it the launcher would catch the signal and process.exit(0) instead of inheriting the signal-death semantics (waitpid would see WIFEXITED instead of WIFSIGNALED, and the host would see exit code 0). The fallback `process.exit(128)` covers exotic platforms where re-raise is rejected. Verification (sleep child standin): launcher=9617 child=9630 OK: child died launcher exit=143 (expect 128+15=143) POSIX hosts now see the launcher process die with the same signal the host sent, matching the old `exec node` semantics. Windows behavior unchanged (it never had the orphan problem — the OS reaps the process tree).
|
Friendly ping @thedotmack — CLEAN, Greptile 5/5, no outstanding conversations. For context: PR #2594 closes #2461 by replacing the Tiny diff (+20/-3), no breaking changes. Happy to rebase if main has moved. |
thedotmack
added
the
Babysitter: Ready to Merge
|
Pushed a follow-up fix in 5485398 for Greptile's remaining suggestion: MCP launcher exit code on Windows signal re-raise: changed This also addresses Issue 3 from the Windows canary PR #2699. |
…dotmack#2461) The mcp-search MCP server in plugin/.mcp.json launched via: "command": "sh", "args": ["-c", "_C=... ls -dt ... | while read ... exec node ..."] On Windows, neither cmd nor PowerShell ships `sh`, and most users do not have Git Bash on the system PATH that Claude Desktop / Claude Code spawns from. Result: `/mcp` panel shows mcp-search failed with ENOENT, blocking all mem-search skill usage on Windows. Same launcher also breaks any host that strips PATH before spawning MCP servers. Fix: rewrite the launcher as inline node -e. Node is guaranteed to be on PATH for any host running claude-mem (the plugin's other scripts require it). The new launcher preserves the exact same fallback search order: 1. CLAUDE_PLUGIN_ROOT / PLUGIN_ROOT env (host-injected) 2. <cwd>/plugin 3. <cwd> 4. ~/.codex/plugins/cache/claude-mem-local/claude-mem/<version>/ (mtime-descending; matches `ls -dt` semantics) 5. ~/.codex/plugins/cache/thedotmack/claude-mem/<version>/ 6. <CLAUDE_CONFIG_DIR or ~/.claude>/plugins/cache/thedotmack/claude-mem/<version>/ 7. <CLAUDE_CONFIG_DIR or ~/.claude>/plugins/marketplaces/thedotmack/plugin Both the codex and claude cache fallback paths are preserved as literal strings so build-hooks.js's distribution check still passes. Verification: - JSON parses cleanly - node --check on the inline JS reports no syntax errors - Running the launcher logic on the repo cwd resolves PLUGIN_ROOT to <cwd>/plugin (same as the old sh script) - npm run build's mcp-search assertion passes No change to runtime behavior on macOS / Linux — node is the same spawn target the old sh script ultimately exec'd into. The only difference is the search-and-launch logic now runs in node itself rather than in a sub-shell.
Addresses both points in the Greptile review on thedotmack#2594. The previous launcher had two POSIX regressions vs. the old `exec node ...` approach: 1. Signal forwarding gap: the MCP host holds the launcher's PID, not mcp-server.cjs's. SIGTERM to the launcher would exit the launcher via Node's default handler but leave the child orphaned, holding its stdio descriptors / file locks. 2. Signal-death exit code: spawn's `exit` callback fires with `(code, signal)`. When the child dies via signal, `code` is null and `signal` is e.g. `'SIGTERM'`. The previous handler returned `0` (clean exit), masking the real cause from the host. Fix: - Register handlers for SIGTERM/SIGINT/SIGHUP on the launcher that forward to `ch.kill(sig)`. try/catch in case the child is already dead. - In `ch.on('exit')`, when signal is non-null, deregister our own handler for that signal and re-raise it via `process.kill(process.pid, sig)`. The deregister step is critical — Node calls registered handlers instead of dying, so without it the launcher would catch the signal and process.exit(0) instead of inheriting the signal-death semantics (waitpid would see WIFEXITED instead of WIFSIGNALED, and the host would see exit code 0). The fallback `process.exit(128)` covers exotic platforms where re-raise is rejected. Verification (sleep child standin): launcher=9617 child=9630 OK: child died launcher exit=143 (expect 128+15=143) POSIX hosts now see the launcher process die with the same signal the host sent, matching the old `exec node` semantics. Windows behavior unchanged (it never had the orphan problem — the OS reaps the process tree).
… failure When the child exits due to a signal and process.kill(pid, sig) throws on Windows (unsupported POSIX signals), exit with 0 since the child already exited and the launcher's job is done. The previous exit(128) could be misinterpreted as an error by callers.
YOMXXX
force-pushed
the
fix/mcp-search-cross-platform
branch
from
5485398 to
d7b4ef1
Compare
| "-c", | ||
| "_C=\"${CLAUDE_CONFIG_DIR:-$HOME/.claude}\"; _E=\"${CLAUDE_PLUGIN_ROOT:-${PLUGIN_ROOT:-}}\"; _P=$({ [ -n \"$_E\" ] && printf '%s\\n' \"$_E\"; printf '%s\\n' \"$PWD/plugin\" \"$PWD\"; ls -dt \"$HOME/.codex/plugins/cache/claude-mem-local/claude-mem\"/[0-9]*/ \"$HOME/.codex/plugins/cache/thedotmack/claude-mem\"/[0-9]*/ \"$_C/plugins/cache/thedotmack/claude-mem\"/[0-9]*/ 2>/dev/null; printf '%s\\n' \"$_C/plugins/marketplaces/thedotmack/plugin\"; } | while IFS= read -r _R; do [ -d \"$_R/plugin/scripts\" ] && _Q=\"$_R/plugin\" || _Q=\"$_R\"; [ -f \"$_Q/scripts/mcp-server.cjs\" ] && { printf '%s\\n' \"$_Q\"; break; }; done); [ -n \"$_P\" ] || { echo \"claude-mem: mcp server not found\" >&2; exit 1; }; exec node \"$_P/scripts/mcp-server.cjs\"" | ||
| "-e", | ||
| "const f=require('fs'),p=require('path'),o=require('os'),c=require('child_process');const h=o.homedir();const C=process.env.CLAUDE_CONFIG_DIR||p.join(h,'.claude');const E=process.env.CLAUDE_PLUGIN_ROOT||process.env.PLUGIN_ROOT||'';const d=process.cwd();const L=x=>{try{return f.readdirSync(x).filter(n=>/^\\d/.test(n)).map(n=>p.join(x,n)).filter(z=>{try{return f.statSync(z).isDirectory()}catch{return false}}).sort((a,b)=>f.statSync(b).mtimeMs-f.statSync(a).mtimeMs)}catch{return[]}};const K=[E,p.join(d,'plugin'),d,...L(p.join(h,'.codex/plugins/cache/claude-mem-local/claude-mem')),...L(p.join(h,'.codex/plugins/cache/thedotmack/claude-mem')),...L(p.join(C,'plugins/cache/thedotmack/claude-mem')),p.join(C,'plugins/marketplaces/thedotmack/plugin')].filter(Boolean);let R=null;for(const k of K){const r=f.existsSync(p.join(k,'plugin','scripts'))?p.join(k,'plugin'):k;if(f.existsSync(p.join(r,'scripts','mcp-server.cjs'))){R=r;break}}if(!R){process.stderr.write('claude-mem: mcp server not found\\n');process.exit(1)}const ch=c.spawn(process.execPath,[p.join(R,'scripts','mcp-server.cjs')],{stdio:'inherit'});for(const s of ['SIGTERM','SIGINT','SIGHUP'])process.on(s,()=>{try{ch.kill(s)}catch{}});ch.on('exit',(code,sig)=>{if(sig){process.removeAllListeners(sig);try{process.kill(process.pid,sig)}catch{process.exit(0)}}else process.exit(code==null?0:code)})" |
There was a problem hiding this comment.
npm run build will throw
scripts/build-hooks.js enforces a "Rule A" constraint via verifyShellTemplateCanonical() (lines 127–178). It calls buildShellCommand({ host: 'mcp', requireFile: 'mcp-server.cjs', trailingCommand: ['exec', 'node', '"$_P/scripts/mcp-server.cjs"'], ... }) from src/build/hook-shell-template.ts and does a byte-for-byte equality check against parsed.mcpServers['mcp-search'].args[1] (line 151). Since hook-shell-template.ts still generates the old POSIX-shell string (with ls -dt, _C=, _E=, etc.), but plugin/.mcp.json now has the new Node.js inline script, these two strings differ and the check throws: "Hand-edited shell string detected in plugin/.mcp.json (mcp-search). It no longer matches src/build/hook-shell-template.ts. Update the generator (and this manifest) instead of hand-editing the launcher."
The PR must also update src/build/hook-shell-template.ts (add a host === 'mcp' branch that emits the Node.js inline launcher) and align the shellTemplateManifest in build-hooks.js accordingly. The stale comment in hook-shell-template.ts at line 149–152 ("MCP runs under sh -c") would need updating too.
Prompt To Fix With AI
This is a comment left during a code review.
Path: plugin/.mcp.json
Line: 8
Comment:
**Rule A assertion will fail — `npm run build` will throw**
`scripts/build-hooks.js` enforces a "Rule A" constraint via `verifyShellTemplateCanonical()` (lines 127–178). It calls `buildShellCommand({ host: 'mcp', requireFile: 'mcp-server.cjs', trailingCommand: ['exec', 'node', '"$_P/scripts/mcp-server.cjs"'], ... })` from `src/build/hook-shell-template.ts` and does a **byte-for-byte equality check** against `parsed.mcpServers['mcp-search'].args[1]` (line 151). Since `hook-shell-template.ts` still generates the old POSIX-shell string (with `ls -dt`, `_C=`, `_E=`, etc.), but `plugin/.mcp.json` now has the new Node.js inline script, these two strings differ and the check throws: "Hand-edited shell string detected in plugin/.mcp.json (mcp-search). It no longer matches src/build/hook-shell-template.ts. Update the generator (and this manifest) instead of hand-editing the launcher."
The PR must also update `src/build/hook-shell-template.ts` (add a `host === 'mcp'` branch that emits the Node.js inline launcher) and align the `shellTemplateManifest` in `build-hooks.js` accordingly. The stale comment in `hook-shell-template.ts` at line 149–152 ("MCP runs under `sh -c`") would need updating too.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in 6c88dc4a.
src/build/hook-shell-template.ts now has a host === 'mcp' branch (buildMcpNodeLauncher) that emits the Node inline launcher as the Rule A single source of truth. verifyShellTemplateCanonical() now compares plugin/.mcp.json's mcp-search args[1] against the generator output (byte-for-byte match), so npm run build passes again instead of throwing.
Also in this commit:
- dropped the now-dead
host === 'mcp'branches incandidateBlock(the generator is the sole consumer ofmcpExtraCandidates/mcpExtraCacheRoots), - corrected the stale "MCP runs under
sh -c" comment flagged here, - updated the mcp assertions in
tests/infrastructure/plugin-distribution.test.tsfrom POSIX-shell syntax to the Node-launcher shape (infra suite: 196 pass / 0 fail).
|
Quick note for context after the 5/28 cleanup wave: #2461 was closed but (Rebased onto current main; diff is the cleanest it'll be — 1 file, +3/-3 lines.) |
|
The cross-platform |
2 participants
Summary
plugin/.mcp.jsonlaunched themcp-searchMCP server via:On Windows, neither cmd nor PowerShell ships
sh, and most users do not have Git Bash on the PATH that Claude Desktop / Claude Code spawns from. Result:/mcppanel reportsmcp-searchfailed (ENOENT), blocking all mem-search skill usage on Windows. The same launcher also breaks any host that strips PATH before spawning MCP servers.Fix
Rewrite the launcher as inline
node -e. Node is guaranteed to be on PATH for any host running claude-mem (the plugin's other scripts require it).The new launcher preserves the exact same fallback search order:
CLAUDE_PLUGIN_ROOT/PLUGIN_ROOTenv (host-injected)<cwd>/plugin<cwd>~/.codex/plugins/cache/claude-mem-local/claude-mem/<version>/(mtime-descending; matchesls -dtsemantics)~/.codex/plugins/cache/thedotmack/claude-mem/<version>/<CLAUDE_CONFIG_DIR or ~/.claude>/plugins/cache/thedotmack/claude-mem/<version>/<CLAUDE_CONFIG_DIR or ~/.claude>/plugins/marketplaces/thedotmack/pluginBoth the Codex and Claude cache fallback paths remain as literal strings in the JSON, so
build-hooks.js's distribution-file check still passes.Verification
node --checkon the inline JS reports no syntax errorsRinstead ofspawn— resolves to<cwd>/plugin(same as the old sh script)npm run buildmcp-search assertion passesCloses
Risk
No behavior change on macOS / Linux — node is the same spawn target the old sh script ultimately
exec'd into. The only difference is the search-and-launch logic now runs in node itself rather than in a sub-shell.