fix: spawn Claude CLI binary directly instead of /bin/bash#7
Open
jonathan-chamberlin wants to merge 5 commits intograhama1970:mainfrom
Open
fix: spawn Claude CLI binary directly instead of /bin/bash#7jonathan-chamberlin wants to merge 5 commits intograhama1970:mainfrom
jonathan-chamberlin wants to merge 5 commits intograhama1970:mainfrom
Conversation
On macOS, the Claude CLI is a Mach-O binary, not a shell script. Running it via `/bin/bash <binary>` fails with "cannot execute binary file". This spawns the CLI directly and passes args without the binary path prefix.
📝 WalkthroughWalkthroughReplaces in-file tool registration and orchestration with modular pieces: centralized config, CLI discovery, spawn utilities, task store, RooModes manager, explicit tool definitions, and per-tool handlers; server routes MCP tool calls via TOOL_DEFINITIONS to dedicated handlers and uses standardized shutdown/cleanup hooks. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Router as "Tool Router"
participant Handler as "Tool Handler"
participant TaskStore
participant Spawn as "spawnAsync"
participant Claude as "Claude CLI"
Client->>Server: MCP request (tool, args)
Server->>Router: lookup tool in TOOL_DEFINITIONS
Router->>Handler: invoke specific handler (e.g., claude_code)
Handler->>TaskStore: createTask / registerProcess
Handler->>Spawn: spawnAsync(cliPath, args, {cwd, timeout})
Spawn->>Claude: execute binary, stream stdout/stderr
Claude-->>Spawn: stdout/stderr + exit code
Spawn-->>Handler: result or error
Handler->>TaskStore: completeTask or failTask (appendOutput)
Handler-->>Server: return ServerResult (taskId/status or final result)
Server-->>Client: MCP response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
…bility - Extract config.ts (env vars, constants, types), cli.ts (CLI discovery), spawn.ts (async exec with heartbeat), roomodes.ts (cache/watcher) - Extract tool handlers: tools/health.ts, tools/convert-task.ts, tools/claude-code.ts - Extract tool-definitions.ts (schemas and descriptions) - Rewrite server.ts as thin shell with switch dispatcher - Add withRequestTracking helper (replaces 6 duplicate cleanup calls) - Type loadRooModes return as RooModesConfig instead of any - Remove dual fs/path imports and stale comments - Add Known Issues section to CLAUDE.md (stale MCP session fix) - Zero behavioral changes; all tools verified via MCP protocol test
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
src/config.ts (1)
3-9: Consider adding validation for parsed numeric environment variables.
parseIntreturnsNaNwhen given non-numeric input (e.g.,MCP_MAX_RETRIES="abc"), and the fallback (|| 'default') only applies when the env var is undefined or empty.NaNvalues can cause silent failures—for example, retry loops that never execute or timeout comparisons that always fail.🛡️ Proposed helper to safely parse numeric env vars
+function parseIntEnv(envVar: string | undefined, defaultValue: number): number { + if (!envVar) return defaultValue; + const parsed = parseInt(envVar, 10); + return Number.isNaN(parsed) || parsed < 0 ? defaultValue : parsed; +} + export const DEBUG_MODE = process.env.MCP_CLAUDE_DEBUG === 'true'; -export const HEARTBEAT_INTERVAL_MS = parseInt(process.env.MCP_HEARTBEAT_INTERVAL_MS || '15000', 10); -export const EXECUTION_TIMEOUT_MS = parseInt(process.env.MCP_EXECUTION_TIMEOUT_MS || '1800000', 10); +export const HEARTBEAT_INTERVAL_MS = parseIntEnv(process.env.MCP_HEARTBEAT_INTERVAL_MS, 15000); +export const EXECUTION_TIMEOUT_MS = parseIntEnv(process.env.MCP_EXECUTION_TIMEOUT_MS, 1800000); export const USE_ROO_MODES = process.env.MCP_USE_ROOMODES === 'true'; -export const MAX_RETRIES = parseInt(process.env.MCP_MAX_RETRIES || '3', 10); -export const RETRY_DELAY_MS = parseInt(process.env.MCP_RETRY_DELAY_MS || '1000', 10); +export const MAX_RETRIES = parseIntEnv(process.env.MCP_MAX_RETRIES, 3); +export const RETRY_DELAY_MS = parseIntEnv(process.env.MCP_RETRY_DELAY_MS, 1000); export const WATCH_ROO_MODES = process.env.MCP_WATCH_ROOMODES === 'true';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.ts` around lines 3 - 9, The numeric environment constants (HEARTBEAT_INTERVAL_MS, EXECUTION_TIMEOUT_MS, MAX_RETRIES, RETRY_DELAY_MS) currently use parseInt which can yield NaN for invalid inputs; add a small validated parser helper (e.g., safeParseInt or parseEnvInt) and use it to parse each env var, returning the provided default when the value is undefined, empty, or NaN; update references to HEARTBEAT_INTERVAL_MS, EXECUTION_TIMEOUT_MS, MAX_RETRIES, and RETRY_DELAY_MS to call this helper so the constants are always a valid number.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/roomodes.ts`:
- Around line 25-30: The file watcher callback for watcher watching roomodesPath
currently only invalidates roomodesCache on eventType === 'change', which misses
atomic saves that emit 'rename'; update the watcher callback in the watcher for
roomodesPath to treat both 'change' and 'rename' as triggers to set
roomodesCache = null and log the invalidation (i.e., check if eventType ===
'change' || eventType === 'rename' and then clear roomodesCache and log),
ensuring both event types are handled.
In `@src/server.ts`:
- Around line 49-53: The server handshake hard-codes version '1.0.0' while
handleHealth() returns packageJson.version; update the Server instantiation to
use the same source by replacing the literal '1.0.0' with the packageVersion
value (e.g., this.packageVersion or packageJson.version) so the first-argument
object ({ name: 'claude_code', version: ... }) and handleHealth() return the
same version consistently.
In `@src/spawn.ts`:
- Around line 15-16: The debug logging in src/spawn.ts currently emits raw
command arguments and full stdout/stderr (see the debugLog call that
interpolates command and args and any handlers that log
child.stdout/child.stderr), which can leak prompts, repo contents and secrets;
update these debugLog calls to redact sensitive content by either removing args
from logs or replacing them with a redacted placeholder (e.g., log only the
command name and a note like "[REDACTED ARGS]"), and change any stdout/stderr
debug prints to sanitize or truncate output before logging (or log only exit
code/status and a size/summary rather than full I/O). Apply the same redaction
approach to the other logging sites in this file (the child stdout/stderr
handlers around lines 40-42 and 58-60) and to the analogous logger call in
src/tools/claude-code.ts (around line 100) so no raw prompt/repo/secret text is
emitted.
- Around line 55-69: In the child process 'close' handler in spawn.ts (the
child.on('close', (code, signal) => { ... }) block), preserve the signal when
creating the rejection error: when code !== 0 (including code === null on kill),
construct the Error object and attach the signal property (e.g., err.signal =
signal) before calling reject(err) so downstream checks like err.signal ===
'SIGTERM' work; ensure the debug logs still include code and signal for
diagnostics and that resolve({ stdout, stderr }) remains unchanged for code ===
0.
In `@src/tools/claude-code.ts`:
- Around line 49-57: The current logic silently falls back to homedir when a
caller supplies an invalid toolArguments.workFolder; change this to reject
invalid values: when typeof toolArguments.workFolder === 'string' resolve the
path with pathResolve, use existsSync and fs.statSync (or lstatSync) to verify
it exists and is a directory, and if the check fails call a clear error path
(throw an Error or return a failure) instead of setting effectiveCwd =
homedir(); keep debugLog for success or failure but do not permit defaulting to
HOME when an explicit workFolder was provided (refer to effectiveCwd,
toolArguments.workFolder, pathResolve, existsSync, debugLog).
In `@src/tools/convert-task.ts`:
- Around line 23-35: Relative paths for markdownPath and outputPath are being
resolved against different bases (markdownPath is passed raw to spawnAsync with
cwd: homedir(), while outputPath is written relative to the server cwd), causing
mismatched input/output locations; fix by normalizing both paths against the
same base before use — e.g., call pathResolve(homedir(), markdownPath) (or
pathResolve(process.cwd(), ...) if you prefer server-root behavior) and
similarly resolve outputPath with the same base, then pass the resolved
markdownPath to spawnAsync and use the resolved outputPath when writing results;
update references in this file for markdownPath, outputPath and the spawnAsync
invocation (which sets cwd: homedir()) and ensure any subsequent logic (lines
~81-83) uses the same resolved values.
- Around line 32-33: The converterPath currently uses pathResolve(__dirname,
'../docs/task_converter.py') which points to src/docs; update the converterPath
assignment so pathResolve goes up two directories from __dirname (to target the
repo-level docs) before 'task_converter.py' so spawnAsync('python3',
[converterPath, ...]) points to the actual repo docs script; modify the
pathResolve call in convert_task_markdown (where converterPath is defined)
accordingly.
In `@src/tools/health.ts`:
- Around line 27-33: The top-level health status is always set to 'ok' even when
the Claude CLI is unavailable; update healthInfo (the object constructed around
packageVersion and claudeCliStatus) so its status reflects claudeCliStatus—e.g.,
compute overallStatus = claudeCliStatus === 'ok' ? 'ok' : 'degraded' and set
healthInfo.status to that value (or set healthInfo.status to 'degraded' when
claudeCliStatus !== 'ok') so callers see degraded health when the CLI is
unavailable.
---
Nitpick comments:
In `@src/config.ts`:
- Around line 3-9: The numeric environment constants (HEARTBEAT_INTERVAL_MS,
EXECUTION_TIMEOUT_MS, MAX_RETRIES, RETRY_DELAY_MS) currently use parseInt which
can yield NaN for invalid inputs; add a small validated parser helper (e.g.,
safeParseInt or parseEnvInt) and use it to parse each env var, returning the
provided default when the value is undefined, empty, or NaN; update references
to HEARTBEAT_INTERVAL_MS, EXECUTION_TIMEOUT_MS, MAX_RETRIES, and RETRY_DELAY_MS
to call this helper so the constants are always a valid number.
🪄 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: 3563bdca-8a32-462d-acb8-f295d27bb24d
⛔ Files ignored due to path filters (9)
dist/cli.jsis excluded by!**/dist/**dist/config.jsis excluded by!**/dist/**dist/roomodes.jsis excluded by!**/dist/**dist/server.jsis excluded by!**/dist/**dist/spawn.jsis excluded by!**/dist/**dist/tool-definitions.jsis excluded by!**/dist/**dist/tools/claude-code.jsis excluded by!**/dist/**dist/tools/convert-task.jsis excluded by!**/dist/**dist/tools/health.jsis excluded by!**/dist/**
📒 Files selected for processing (10)
CLAUDE.mdsrc/cli.tssrc/config.tssrc/roomodes.tssrc/server.tssrc/spawn.tssrc/tool-definitions.tssrc/tools/claude-code.tssrc/tools/convert-task.tssrc/tools/health.ts
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
Comment on lines
+25
to
+30
| watcher = watch(roomodesPath, (eventType) => { | ||
| if (eventType === 'change') { | ||
| roomodesCache = null; | ||
| console.error(`[Info] .roomodes file changed, cache invalidated`); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js fs.watch, what events are emitted when a watched file is atomically saved or replaced by an editor, and does a watcher on the file continue tracking the replacement after a rename?
💡 Result:
In Node.js fs.watch on a file, when atomically saved or replaced by an editor (typically via writing to a temp file and renaming it over the original), the watcher emits 'rename' events: usually two—one for the disappearance of the old file and one for the appearance of the new file. A 'change' event may follow if the content modification is detected separately, but this varies by platform and editor. Behavior is not 100% consistent across platforms (Linux, macOS, Windows) due to underlying OS APIs (inotify, kqueue, ReadDirectoryChangesW). The watcher on the original filename continues tracking the replacement file after the rename because fs.watch monitors the path (directory entry), not the inode. When the directory entry is updated to point to the new inode, events for that path continue firing for subsequent changes to the new file. This is standard behavior, though some older issues noted potential detection problems with certain editors. Note: fs.watch has platform inconsistencies and may miss events or emit extras; consider chokidar for reliability. Use fs.stat or debounce for deduplication.
Citations:
- 1: fs.watch fails to detect changes to file replaced by rename event nodejs/node-v0.x-archive#2062
- 2: https://stackoverflow.com/questions/20153244/nodejs-atomic-file-replace-operation-trigger-only-one-time-some-watchers
- 3: https://oneuptime.com/blog/post/2026-01-22-nodejs-watch-file-changes/view
- 4: watch always fires rename event nodejs/node#9082
- 5: https://nodejs.org/api/fs.html
- 6: https://stackoverflow.com/questions/10762630/nodejs-fs-watch-on-directory-only-fires-when-changed-by-editor-but-not-shell-or
- 7:
fs.watchdoesn't pass the correct filename of the changed file? nodejs/node#3086
Handle rename events in the .roomodes watcher.
When editors atomically save files by replacing them, fs.watch() emits rename events that are not currently handled. Since the code only checks for change events, cache invalidation is skipped for atomic saves, leaving stale config in memory until the TTL expires. Add rename to the event checks to catch these cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/roomodes.ts` around lines 25 - 30, The file watcher callback for watcher
watching roomodesPath currently only invalidates roomodesCache on eventType ===
'change', which misses atomic saves that emit 'rename'; update the watcher
callback in the watcher for roomodesPath to treat both 'change' and 'rename' as
triggers to set roomodesCache = null and log the invalidation (i.e., check if
eventType === 'change' || eventType === 'rename' and then clear roomodesCache
and log), ensuring both event types are handled.
Comment on lines
+49
to
+53
| this.packageVersion = packageJson.version; | ||
|
|
||
| this.server = new Server( | ||
| { | ||
| name: 'claude_code', | ||
| version: '1.0.0', | ||
| }, | ||
| { | ||
| capabilities: { | ||
| tools: {}, | ||
| }, | ||
| } | ||
| { name: 'claude_code', version: '1.0.0' }, | ||
| { capabilities: { tools: {} } }, |
There was a problem hiding this comment.
Keep the MCP server version in sync with package.json.
handleHealth() returns packageJson.version, but the server handshake still hard-codes 1.0.0, so the two version surfaces will drift on the next release.
Suggested fix
this.server = new Server(
- { name: 'claude_code', version: '1.0.0' },
+ { name: 'claude_code', version: this.packageVersion },
{ capabilities: { tools: {} } },
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| this.packageVersion = packageJson.version; | |
| this.server = new Server( | |
| { | |
| name: 'claude_code', | |
| version: '1.0.0', | |
| }, | |
| { | |
| capabilities: { | |
| tools: {}, | |
| }, | |
| } | |
| { name: 'claude_code', version: '1.0.0' }, | |
| { capabilities: { tools: {} } }, | |
| this.packageVersion = packageJson.version; | |
| this.server = new Server( | |
| { name: 'claude_code', version: this.packageVersion }, | |
| { capabilities: { tools: {} } }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server.ts` around lines 49 - 53, The server handshake hard-codes version
'1.0.0' while handleHealth() returns packageJson.version; update the Server
instantiation to use the same source by replacing the literal '1.0.0' with the
packageVersion value (e.g., this.packageVersion or packageJson.version) so the
first-argument object ({ name: 'claude_code', version: ... }) and handleHealth()
return the same version consistently.
Comment on lines
+55
to
+69
| child.on('close', (code) => { | ||
| clearInterval(progressReporter); | ||
| const executionTimeMs = Date.now() - executionStartTime; | ||
| debugLog(`[Spawn Close] Exit code: ${code}, Execution time: ${executionTimeMs}ms`); | ||
| debugLog(`[Spawn Stderr Full] ${stderr.trim()}`); | ||
| debugLog(`[Spawn Stdout Full] ${stdout.trim()}`); | ||
| if (code === 0) { | ||
| resolve({ stdout, stderr }); | ||
| } else { | ||
| reject( | ||
| new Error( | ||
| `Command failed with exit code ${code}\nStderr: ${stderr.trim()}\nStdout: ${stdout.trim()}` | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js child_process.spawn, when the timeout option is exceeded, what values are provided to the close event for code and signal, and is an error event emitted in that case?
💡 Result:
When the timeout option in child_process.spawn is exceeded, Node.js sends the killSignal (default 'SIGTERM') to the child process. Once the child process terminates due to the signal, the 'close' event is emitted with code = null and signal = the killSignal value (e.g., 'SIGTERM'). No 'error' event is emitted in this case; the termination is treated as a normal signaled exit.
Citations:
- 1: https://nodejs.org/api/child_process.html
- 2: Know when a process ended due to the
child_process.spawn()timeoutoption nodejs/node#51561 - 3: https://nodejs.org/docs/latest-v18.x/api/child_process.html
- 4: http://nodejs.org/docs/latest/api/child_process.html
- 5: child_process: add timeout to spawn and fork nodejs/node#37256
🏁 Script executed:
# Read the spawn.ts file around lines 55-69 to verify the current implementation
cat -n src/spawn.ts | sed -n '50,75p'Repository: grahama1970/claude-code-mcp-enhanced
Length of output: 1015
🏁 Script executed:
# Search for timeout handling and signal usage in src/tools/claude-code.ts
rg -n "timeout|signal" src/tools/claude-code.ts -A 3 -B 3Repository: grahama1970/claude-code-mcp-enhanced
Length of output: 978
🏁 Script executed:
# Search for how the spawnAsync error is being caught/handled in the codebase
rg -n "spawnAsync" src/ -A 5 -B 1 | head -80Repository: grahama1970/claude-code-mcp-enhanced
Length of output: 2936
🏁 Script executed:
# Get more context around the timeout detection in claude-code.ts
cat -n src/tools/claude-code.ts | sed -n '155,175p'Repository: grahama1970/claude-code-mcp-enhanced
Length of output: 972
🏁 Script executed:
# Check if there are any other places where signal might be set on the error
rg -n "\.signal\s*=" src/ --type tsRepository: grahama1970/claude-code-mcp-enhanced
Length of output: 142
Preserve the child signal in rejected errors.
When the timeout option kills the process, close arrives with code === null plus a signal parameter (e.g., 'SIGTERM'). The current handler drops the signal, preventing src/tools/claude-code.ts from recognizing timeouts. The downstream code explicitly checks err.signal === 'SIGTERM' to distinguish timeout failures from other errors; without this property, timeout errors fall back to the generic failure path instead of returning a proper timeout response.
Suggested fix
- child.on('close', (code) => {
+ child.on('close', (code, signal) => {
clearInterval(progressReporter);
const executionTimeMs = Date.now() - executionStartTime;
debugLog(`[Spawn Close] Exit code: ${code}, Execution time: ${executionTimeMs}ms`);
debugLog(`[Spawn Stderr Full] ${stderr.trim()}`);
debugLog(`[Spawn Stdout Full] ${stdout.trim()}`);
if (code === 0) {
resolve({ stdout, stderr });
} else {
- reject(
- new Error(
- `Command failed with exit code ${code}\nStderr: ${stderr.trim()}\nStdout: ${stdout.trim()}`
- )
- );
+ const error = new Error(
+ `Command failed with ${signal ? `signal ${signal}` : `exit code ${code}`}\nStderr: ${stderr.trim()}\nStdout: ${stdout.trim()}`
+ ) as Error & { signal?: string };
+ error.signal = signal ?? undefined;
+ reject(error);
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| child.on('close', (code) => { | |
| clearInterval(progressReporter); | |
| const executionTimeMs = Date.now() - executionStartTime; | |
| debugLog(`[Spawn Close] Exit code: ${code}, Execution time: ${executionTimeMs}ms`); | |
| debugLog(`[Spawn Stderr Full] ${stderr.trim()}`); | |
| debugLog(`[Spawn Stdout Full] ${stdout.trim()}`); | |
| if (code === 0) { | |
| resolve({ stdout, stderr }); | |
| } else { | |
| reject( | |
| new Error( | |
| `Command failed with exit code ${code}\nStderr: ${stderr.trim()}\nStdout: ${stdout.trim()}` | |
| ) | |
| ); | |
| } | |
| child.on('close', (code, signal) => { | |
| clearInterval(progressReporter); | |
| const executionTimeMs = Date.now() - executionStartTime; | |
| debugLog(`[Spawn Close] Exit code: ${code}, Execution time: ${executionTimeMs}ms`); | |
| debugLog(`[Spawn Stderr Full] ${stderr.trim()}`); | |
| debugLog(`[Spawn Stdout Full] ${stdout.trim()}`); | |
| if (code === 0) { | |
| resolve({ stdout, stderr }); | |
| } else { | |
| const error = new Error( | |
| `Command failed with ${signal ? `signal ${signal}` : `exit code ${code}`}\nStderr: ${stderr.trim()}\nStdout: ${stdout.trim()}` | |
| ) as Error & { signal?: string }; | |
| error.signal = signal ?? undefined; | |
| reject(error); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/spawn.ts` around lines 55 - 69, In the child process 'close' handler in
spawn.ts (the child.on('close', (code, signal) => { ... }) block), preserve the
signal when creating the rejection error: when code !== 0 (including code ===
null on kill), construct the Error object and attach the signal property (e.g.,
err.signal = signal) before calling reject(err) so downstream checks like
err.signal === 'SIGTERM' work; ensure the debug logs still include code and
signal for diagnostics and that resolve({ stdout, stderr }) remains unchanged
for code === 0.
Comment on lines
+49
to
+57
| let effectiveCwd = homedir(); | ||
| if (typeof toolArguments.workFolder === 'string') { | ||
| const resolved = pathResolve(toolArguments.workFolder); | ||
| if (existsSync(resolved)) { | ||
| effectiveCwd = resolved; | ||
| debugLog(`[Debug] Using workFolder as CWD: ${effectiveCwd}`); | ||
| } else { | ||
| debugLog(`[Warning] Specified workFolder does not exist: ${resolved}. Using default.`); | ||
| } |
There was a problem hiding this comment.
Reject invalid workFolder values instead of falling back to $HOME.
If a caller passes a typo or a file path here, the tool quietly switches to the home directory and then runs with --dangerously-skip-permissions. That's a bad-parameter bug that can turn into edits in the wrong tree.
Suggested fix
-import { existsSync } from 'node:fs';
+import { existsSync, statSync } from 'node:fs';
@@
let effectiveCwd = homedir();
if (typeof toolArguments.workFolder === 'string') {
const resolved = pathResolve(toolArguments.workFolder);
- if (existsSync(resolved)) {
- effectiveCwd = resolved;
- debugLog(`[Debug] Using workFolder as CWD: ${effectiveCwd}`);
- } else {
- debugLog(`[Warning] Specified workFolder does not exist: ${resolved}. Using default.`);
- }
+ if (!existsSync(resolved) || !statSync(resolved).isDirectory()) {
+ throw new McpError(ErrorCode.InvalidParams, `Invalid workFolder: ${resolved}`);
+ }
+ effectiveCwd = resolved;
+ debugLog(`[Debug] Using workFolder as CWD: ${effectiveCwd}`);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/claude-code.ts` around lines 49 - 57, The current logic silently
falls back to homedir when a caller supplies an invalid
toolArguments.workFolder; change this to reject invalid values: when typeof
toolArguments.workFolder === 'string' resolve the path with pathResolve, use
existsSync and fs.statSync (or lstatSync) to verify it exists and is a
directory, and if the check fails call a clear error path (throw an Error or
return a failure) instead of setting effectiveCwd = homedir(); keep debugLog for
success or failure but do not permit defaulting to HOME when an explicit
workFolder was provided (refer to effectiveCwd, toolArguments.workFolder,
pathResolve, existsSync, debugLog).
Comment on lines
+23
to
+35
| const markdownPath = toolArguments.markdownPath; | ||
| const outputPath = | ||
| typeof toolArguments.outputPath === 'string' ? toolArguments.outputPath : undefined; | ||
|
|
||
| debugLog(`[Debug] Converting markdown task file: ${markdownPath}`); | ||
|
|
||
| let stderr = ''; | ||
|
|
||
| try { | ||
| const converterPath = pathResolve(__dirname, '../docs/task_converter.py'); | ||
| const result = await spawnAsync('python3', [converterPath, '--json-output', markdownPath], { | ||
| cwd: homedir(), | ||
| timeout: CONVERTER_TIMEOUT_MS, |
There was a problem hiding this comment.
Resolve markdownPath and outputPath against the same base.
markdownPath is passed through verbatim while the Python child runs with cwd: homedir(), so relative inputs are interpreted from $HOME. outputPath is then written by Node relative to the server cwd, which means input and output use different base directories.
Suggested fix
- const markdownPath = toolArguments.markdownPath;
- const outputPath =
- typeof toolArguments.outputPath === 'string' ? toolArguments.outputPath : undefined;
+ const markdownPath = pathResolve(toolArguments.markdownPath);
+ const outputPath =
+ typeof toolArguments.outputPath === 'string' ? pathResolve(toolArguments.outputPath) : undefined;
@@
- const result = await spawnAsync('python3', [converterPath, '--json-output', markdownPath], {
- cwd: homedir(),
- timeout: CONVERTER_TIMEOUT_MS,
- });
+ const result = await spawnAsync('python3', [converterPath, '--json-output', markdownPath], {
+ timeout: CONVERTER_TIMEOUT_MS,
+ });Also applies to: 81-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/convert-task.ts` around lines 23 - 35, Relative paths for
markdownPath and outputPath are being resolved against different bases
(markdownPath is passed raw to spawnAsync with cwd: homedir(), while outputPath
is written relative to the server cwd), causing mismatched input/output
locations; fix by normalizing both paths against the same base before use —
e.g., call pathResolve(homedir(), markdownPath) (or pathResolve(process.cwd(),
...) if you prefer server-root behavior) and similarly resolve outputPath with
the same base, then pass the resolved markdownPath to spawnAsync and use the
resolved outputPath when writing results; update references in this file for
markdownPath, outputPath and the spawnAsync invocation (which sets cwd:
homedir()) and ensure any subsequent logic (lines ~81-83) uses the same resolved
values.
Comment on lines
+32
to
+33
| const converterPath = pathResolve(__dirname, '../docs/task_converter.py'); | ||
| const result = await spawnAsync('python3', [converterPath, '--json-output', markdownPath], { |
There was a problem hiding this comment.
The converter script path is off by one directory.
From src/tools/convert-task.ts, ../docs/task_converter.py resolves under src/docs/, not the repo-level docs/ directory. Unless the build copies the Python script there, every convert_task_markdown call will fail before Python starts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/convert-task.ts` around lines 32 - 33, The converterPath currently
uses pathResolve(__dirname, '../docs/task_converter.py') which points to
src/docs; update the converterPath assignment so pathResolve goes up two
directories from __dirname (to target the repo-level docs) before
'task_converter.py' so spawnAsync('python3', [converterPath, ...]) points to the
actual repo docs script; modify the pathResolve call in convert_task_markdown
(where converterPath is defined) accordingly.
Comment on lines
+27
to
+33
| const healthInfo = { | ||
| status: 'ok', | ||
| version: packageVersion, | ||
| claudeCli: { | ||
| path: claudeCliPath, | ||
| status: claudeCliStatus, | ||
| }, |
There was a problem hiding this comment.
Report degraded health when the CLI is unavailable.
The payload always returns status: 'ok', so callers that key off the top-level field will treat a broken Claude CLI install as healthy.
Suggested fix
const healthInfo = {
- status: 'ok',
+ status: claudeCliStatus === 'available' ? 'ok' : 'degraded',
version: packageVersion,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const healthInfo = { | |
| status: 'ok', | |
| version: packageVersion, | |
| claudeCli: { | |
| path: claudeCliPath, | |
| status: claudeCliStatus, | |
| }, | |
| const healthInfo = { | |
| status: claudeCliStatus === 'available' ? 'ok' : 'degraded', | |
| version: packageVersion, | |
| claudeCli: { | |
| path: claudeCliPath, | |
| status: claudeCliStatus, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/health.ts` around lines 27 - 33, The top-level health status is
always set to 'ok' even when the Claude CLI is unavailable; update healthInfo
(the object constructed around packageVersion and claudeCliStatus) so its status
reflects claudeCliStatus—e.g., compute overallStatus = claudeCliStatus === 'ok'
? 'ok' : 'degraded' and set healthInfo.status to that value (or set
healthInfo.status to 'degraded' when claudeCliStatus !== 'ok') so callers see
degraded health when the CLI is unavailable.
…n bump Sync lock file with package.json rename to @grahama1970/claude-code-mcp-enhanced v1.13.0
…52 tests - Extract 3 helpers from 178-line handleClaudeCode god function - Fix __dirname ESM bug in convert-task.ts (used import.meta.url) - Encapsulate roomodes mutable state in factory function - Extract shared requireStringParam validation utility - Move CLAUDE_CODE_DESCRIPTION to separate file - Remove unused deps (zod, server-perplexity-ask, server-brave-search) - Move @eslint/js to devDependencies - Add Vitest with 52 tests across 8 test files (all passing)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/tools/claude-code.ts (1)
18-29:⚠️ Potential issue | 🟠 MajorReject invalid
workFoldervalues instead of silently falling back to$HOME.When a caller explicitly provides a
workFolderthat doesn't exist, falling back tohomedir()can cause the CLI (running with--dangerously-skip-permissions) to operate on the wrong directory. This could result in unintended file modifications.🛡️ Proposed fix to reject invalid workFolder
+import { existsSync, statSync } from 'node:fs'; -import { existsSync } from 'node:fs'; // ... function resolveWorkingDirectory(workFolder?: string): string { if (typeof workFolder !== 'string') { return homedir(); } const resolved = pathResolve(workFolder); - if (existsSync(resolved)) { - debugLog(`[Debug] Using workFolder as CWD: ${resolved}`); - return resolved; + if (!existsSync(resolved) || !statSync(resolved).isDirectory()) { + throw new McpError( + ErrorCode.InvalidParams, + `Invalid workFolder: "${resolved}" does not exist or is not a directory`, + ); } - debugLog(`[Warning] Specified workFolder does not exist: ${resolved}. Using default.`); - return homedir(); + debugLog(`[Debug] Using workFolder as CWD: ${resolved}`); + return resolved; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/claude-code.ts` around lines 18 - 29, The resolveWorkingDirectory function currently falls back to homedir() when a caller passes a non-existent workFolder, which can silently cause the CLI to operate in the wrong directory; change resolveWorkingDirectory to only return homedir() when workFolder is undefined or not provided (typeof workFolder !== 'string'), but when workFolder is a string and pathResolve(workFolder) does not exist (existsSync(resolved) === false) throw a descriptive Error (include the resolved path and that it does not exist) instead of returning homedir(); keep the debugLog messages but log the error before throwing to aid debugging.
🧹 Nitpick comments (6)
tests/validation.test.ts (1)
26-40: Consider usingtoThrowwith message matcher for consistency.While these tests work because
requireStringParam({}, ...)deterministically throws, usingtoThrowwith a regex matcher is more idiomatic and self-documenting.♻️ Suggested improvement
- it('error message includes param name', () => { - try { - requireStringParam({}, 'prompt', 'my_tool'); - } catch (e) { - expect((e as Error).message).toContain('prompt'); - } - }); - - it('error message includes tool name', () => { - try { - requireStringParam({}, 'prompt', 'my_tool'); - } catch (e) { - expect((e as Error).message).toContain('my_tool'); - } - }); + it('error message includes param name', () => { + expect(() => requireStringParam({}, 'prompt', 'my_tool')).toThrow(/prompt/); + }); + + it('error message includes tool name', () => { + expect(() => requireStringParam({}, 'prompt', 'my_tool')).toThrow(/my_tool/); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/validation.test.ts` around lines 26 - 40, Replace the current try/catch assertions in the two tests with Jest's toThrow matcher for clarity and idiomatic style: call requireStringParam inside a function passed to expect and use .toThrow with a RegExp matching 'prompt' for the "error message includes param name" test and a RegExp matching 'my_tool' for the "error message includes tool name" test (referencing the requireStringParam function and the two it(...) test cases).tests/tools/health.test.ts (1)
10-13: Consider addingvi.clearAllMocks()inbeforeEach.The
beforeEachsets up the mock return value but doesn't clear previous call history. While the current tests don't check call counts, addingvi.clearAllMocks()would prevent subtle issues if such assertions are added later.♻️ Suggested improvement
beforeEach(() => { + vi.clearAllMocks(); vi.mocked(spawnAsync).mockResolvedValue({ stdout: 'claude 1.0.0', stderr: '' }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tools/health.test.ts` around lines 10 - 13, In the describe('handleHealth') test suite, ensure mock state is reset before each test by calling vi.clearAllMocks() at the start of the beforeEach block, then reapply vi.mocked(spawnAsync).mockResolvedValue(...) so spawnAsync's mock return is set but previous call history and mock state are cleared; this will prevent leaked call counts or state from other tests affecting assertions on spawnAsync or future added tests.src/tool-definitions.ts (2)
4-14: Consider usingTOOL_NAMESconstants to avoid potential mismatches.The tool names are hardcoded as string literals here but also defined as constants in
src/config.ts. If either is updated without the other, the server's switch statement insrc/server.tswill silently fail to route requests (falling through toMethodNotFound).♻️ Suggested refactor to use constants
+import { TOOL_NAMES } from './config.js'; import { CLAUDE_CODE_DESCRIPTION } from './descriptions.js'; export { CLAUDE_CODE_DESCRIPTION } from './descriptions.js'; export const TOOL_DEFINITIONS = [ { - name: 'health', + name: TOOL_NAMES.HEALTH, description: 'Returns health status, version information, and current configuration of the Claude Code MCP server.', // ... }, { - name: 'convert_task_markdown', + name: TOOL_NAMES.CONVERT_TASK, // ... }, { - name: 'claude_code', + name: TOOL_NAMES.CLAUDE_CODE, // ... }, ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tool-definitions.ts` around lines 4 - 14, TOOL_DEFINITIONS currently uses hardcoded string literals for tool names which can drift from the TOOL_NAMES constants in config.ts; update TOOL_DEFINITIONS to reference the TOOL_NAMES constant (import TOOL_NAMES from src/config) instead of literal strings (e.g., replace 'health' with TOOL_NAMES.health) and ensure any other entries in TOOL_DEFINITIONS use the corresponding TOOL_NAMES keys so the server routing switch in server.ts (which matches TOOL_NAMES values) will remain consistent.
45-49: Clarify schema:workFolderdescription says "Mandatory" but field is optional.The description states "Mandatory when using file operations" but
workFolderis not in therequiredarray. This could confuse API consumers. Consider either:
- Adding it to
requiredif it's truly mandatory, or- Rewording to "Required when using file operations" to clarify it's conditionally required.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tool-definitions.ts` around lines 45 - 49, The workFolder schema entry (property name: workFolder) has a misleading description that says "Mandatory when using file operations" while not being listed in the schema's required array; either add "workFolder" to the required array if it truly must always be provided, or change the description to a conditional phrasing such as "Required when using file operations" to avoid confusion for API consumers—update the workFolder property description in src/tool-definitions.ts accordingly and ensure the change aligns with how the code validates/uses workFolder.src/tools/claude-code.ts (2)
125-130: Transient error detection relies on string matching—consider using error codes.Matching substrings like
'429'or'500'inerror.messageis fragile and could match unintended messages (e.g., "processed 429 items"). If the error object has acodeproperty or structured data, prefer checking that.♻️ More robust transient check
+ const errWithCode = error as Error & { code?: string; statusCode?: number }; const isTransient = - error.message.includes('ECONNRESET') || - error.message.includes('ETIMEDOUT') || - error.message.includes('ECONNREFUSED') || - error.message.includes('429') || - error.message.includes('500'); + errWithCode.code === 'ECONNRESET' || + errWithCode.code === 'ETIMEDOUT' || + errWithCode.code === 'ECONNREFUSED' || + errWithCode.statusCode === 429 || + errWithCode.statusCode === 500 || + // Fallback to message matching if structured codes unavailable + /\b(ECONNRESET|ETIMEDOUT|ECONNREFUSED)\b/.test(error.message);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/claude-code.ts` around lines 125 - 130, The transient detection using substring matches on error.message (the isTransient variable) is fragile; change it to check canonical error properties first (e.g., error.code === 'ECONNRESET' || error.code === 'ECONNREFUSED' or numeric HTTP/status checks like error.status === 429 or response?.status === 500) and only fall back to message includes as a last resort; update the logic around isTransient to prefer error.code, error.status, or response?.status and remove loose matches for '429'/'500' in error.message to avoid false positives.
134-136: Dead code afterbail()call.The
bail()function throws immediately perasync-retrysemantics, making line 135 unreachable. While harmless, it may confuse readers.♻️ Suggested cleanup
if (!isTransient) { debugLog(`[Retry] Non-retryable error. Bailing out.`); bail(error); - return { stdout: '', stderr: '' }; + // bail() throws; this line is unreachable but satisfies TypeScript + throw error; }Using
throw errorafterbail()is a common pattern that's semantically clearer and satisfies TypeScript's control flow analysis without the misleading return.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/claude-code.ts` around lines 134 - 136, Summary: Dead unreachable return after calling bail(error) — remove or replace it so control flow is clear. Replace the sequence "bail(error); return { stdout: '', stderr: '' };" with a single thrown error (e.g., "throw error;") or simply remove the return and let bail(error) (which throws) stand alone; update the block around the bail call in src/tools/claude-code.ts (the lines with bail(error) and the returned object) so there is no unreachable return statement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/spawn.test.ts`:
- Around line 27-33: The test for spawnAsync currently silently passes when no
error is thrown; update the test in tests/spawn.test.ts so that after calling
spawnAsync('sh', ['-c', 'echo "error output" >&2; exit 1']) you explicitly fail
the test if it resolves (e.g., call a test fail/assertion immediately after the
await), and keep the existing catch block that asserts the thrown Error.message
contains 'error output'; ensure references to spawnAsync are used so the test
will fail when no exception is raised.
- Around line 19-25: The test currently uses try/catch so it will silently pass
if spawnAsync('sh', ['-c', 'exit 42']) resolves; update the test to assert the
rejection explicitly by replacing the try/catch with either await
expect(spawnAsync('sh', ['-c', 'exit 42'])).rejects.toThrow(/42/) or add
expect.assertions(1) and call await spawnAsync(...) inside expect(...).rejects
to ensure the test fails when spawnAsync unexpectedly resolves; target the test
block that references spawnAsync to make the change.
---
Duplicate comments:
In `@src/tools/claude-code.ts`:
- Around line 18-29: The resolveWorkingDirectory function currently falls back
to homedir() when a caller passes a non-existent workFolder, which can silently
cause the CLI to operate in the wrong directory; change resolveWorkingDirectory
to only return homedir() when workFolder is undefined or not provided (typeof
workFolder !== 'string'), but when workFolder is a string and
pathResolve(workFolder) does not exist (existsSync(resolved) === false) throw a
descriptive Error (include the resolved path and that it does not exist) instead
of returning homedir(); keep the debugLog messages but log the error before
throwing to aid debugging.
---
Nitpick comments:
In `@src/tool-definitions.ts`:
- Around line 4-14: TOOL_DEFINITIONS currently uses hardcoded string literals
for tool names which can drift from the TOOL_NAMES constants in config.ts;
update TOOL_DEFINITIONS to reference the TOOL_NAMES constant (import TOOL_NAMES
from src/config) instead of literal strings (e.g., replace 'health' with
TOOL_NAMES.health) and ensure any other entries in TOOL_DEFINITIONS use the
corresponding TOOL_NAMES keys so the server routing switch in server.ts (which
matches TOOL_NAMES values) will remain consistent.
- Around line 45-49: The workFolder schema entry (property name: workFolder) has
a misleading description that says "Mandatory when using file operations" while
not being listed in the schema's required array; either add "workFolder" to the
required array if it truly must always be provided, or change the description to
a conditional phrasing such as "Required when using file operations" to avoid
confusion for API consumers—update the workFolder property description in
src/tool-definitions.ts accordingly and ensure the change aligns with how the
code validates/uses workFolder.
In `@src/tools/claude-code.ts`:
- Around line 125-130: The transient detection using substring matches on
error.message (the isTransient variable) is fragile; change it to check
canonical error properties first (e.g., error.code === 'ECONNRESET' ||
error.code === 'ECONNREFUSED' or numeric HTTP/status checks like error.status
=== 429 or response?.status === 500) and only fall back to message includes as a
last resort; update the logic around isTransient to prefer error.code,
error.status, or response?.status and remove loose matches for '429'/'500' in
error.message to avoid false positives.
- Around line 134-136: Summary: Dead unreachable return after calling
bail(error) — remove or replace it so control flow is clear. Replace the
sequence "bail(error); return { stdout: '', stderr: '' };" with a single thrown
error (e.g., "throw error;") or simply remove the return and let bail(error)
(which throws) stand alone; update the block around the bail call in
src/tools/claude-code.ts (the lines with bail(error) and the returned object) so
there is no unreachable return statement.
In `@tests/tools/health.test.ts`:
- Around line 10-13: In the describe('handleHealth') test suite, ensure mock
state is reset before each test by calling vi.clearAllMocks() at the start of
the beforeEach block, then reapply vi.mocked(spawnAsync).mockResolvedValue(...)
so spawnAsync's mock return is set but previous call history and mock state are
cleared; this will prevent leaked call counts or state from other tests
affecting assertions on spawnAsync or future added tests.
In `@tests/validation.test.ts`:
- Around line 26-40: Replace the current try/catch assertions in the two tests
with Jest's toThrow matcher for clarity and idiomatic style: call
requireStringParam inside a function passed to expect and use .toThrow with a
RegExp matching 'prompt' for the "error message includes param name" test and a
RegExp matching 'my_tool' for the "error message includes tool name" test
(referencing the requireStringParam function and the two it(...) test cases).
🪄 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: 05bfe82a-53cb-428c-b517-7e18d8c14bb2
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
package.jsonsrc/descriptions.tssrc/roomodes.tssrc/tool-definitions.tssrc/tools/claude-code.tssrc/tools/convert-task.tssrc/validation.tstests/cli.test.tstests/config.test.tstests/roomodes.test.tstests/spawn.test.tstests/tool-definitions.test.tstests/tools/claude-code.test.tstests/tools/health.test.tstests/validation.test.tsvitest.config.ts
✅ Files skipped from review due to trivial changes (2)
- tests/tool-definitions.test.ts
- src/descriptions.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/roomodes.ts
- src/tools/convert-task.ts
Comment on lines
+19
to
+25
| it('error message contains exit code info on failure', async () => { | ||
| try { | ||
| await spawnAsync('sh', ['-c', 'exit 42']); | ||
| } catch (e) { | ||
| expect((e as Error).message).toContain('42'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Test silently passes if no error is thrown.
The try/catch pattern doesn't fail the test if spawnAsync unexpectedly resolves successfully. If the promise resolves, the catch block is never reached, and no assertion runs.
🐛 Proposed fix using expect.assertions or expect().rejects
it('error message contains exit code info on failure', async () => {
- try {
- await spawnAsync('sh', ['-c', 'exit 42']);
- } catch (e) {
- expect((e as Error).message).toContain('42');
- }
+ await expect(spawnAsync('sh', ['-c', 'exit 42'])).rejects.toThrow(/42/);
});Or use expect.assertions(1):
it('error message contains exit code info on failure', async () => {
+ expect.assertions(1);
try {
await spawnAsync('sh', ['-c', 'exit 42']);
} catch (e) {
expect((e as Error).message).toContain('42');
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('error message contains exit code info on failure', async () => { | |
| try { | |
| await spawnAsync('sh', ['-c', 'exit 42']); | |
| } catch (e) { | |
| expect((e as Error).message).toContain('42'); | |
| } | |
| }); | |
| it('error message contains exit code info on failure', async () => { | |
| await expect(spawnAsync('sh', ['-c', 'exit 42'])).rejects.toThrow(/42/); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/spawn.test.ts` around lines 19 - 25, The test currently uses try/catch
so it will silently pass if spawnAsync('sh', ['-c', 'exit 42']) resolves; update
the test to assert the rejection explicitly by replacing the try/catch with
either await expect(spawnAsync('sh', ['-c', 'exit 42'])).rejects.toThrow(/42/)
or add expect.assertions(1) and call await spawnAsync(...) inside
expect(...).rejects to ensure the test fails when spawnAsync unexpectedly
resolves; target the test block that references spawnAsync to make the change.
Comment on lines
+27
to
+33
| it('rejects and error message contains stderr content', async () => { | ||
| try { | ||
| await spawnAsync('sh', ['-c', 'echo "error output" >&2; exit 1']); | ||
| } catch (e) { | ||
| expect((e as Error).message).toContain('error output'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Same issue: test silently passes if no error is thrown.
🐛 Proposed fix
it('rejects and error message contains stderr content', async () => {
- try {
- await spawnAsync('sh', ['-c', 'echo "error output" >&2; exit 1']);
- } catch (e) {
- expect((e as Error).message).toContain('error output');
- }
+ await expect(
+ spawnAsync('sh', ['-c', 'echo "error output" >&2; exit 1'])
+ ).rejects.toThrow(/error output/);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('rejects and error message contains stderr content', async () => { | |
| try { | |
| await spawnAsync('sh', ['-c', 'echo "error output" >&2; exit 1']); | |
| } catch (e) { | |
| expect((e as Error).message).toContain('error output'); | |
| } | |
| }); | |
| it('rejects and error message contains stderr content', async () => { | |
| await expect( | |
| spawnAsync('sh', ['-c', 'echo "error output" >&2; exit 1']) | |
| ).rejects.toThrow(/error output/); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/spawn.test.ts` around lines 27 - 33, The test for spawnAsync currently
silently passes when no error is thrown; update the test in tests/spawn.test.ts
so that after calling spawnAsync('sh', ['-c', 'echo "error output" >&2; exit
1']) you explicitly fail the test if it resolves (e.g., call a test
fail/assertion immediately after the await), and keep the existing catch block
that asserts the thrown Error.message contains 'error output'; ensure references
to spawnAsync are used so the test will fail when no exception is raised.
Claude.ai's Anthropic proxy drops MCP connections after ~2-3 min of
silence (error -32000, hardcoded, non-configurable). This makes
claude_code return immediately with a taskId and runs the CLI in the
background. New get_task_result tool polls for results.
- claude_code now returns {taskId, status: "running"} immediately
- New get_task_result tool: polls task store for running/completed/failed
- In-memory task store with 30-min TTL cleanup timer
- spawnWithHandle() exposes ChildProcess + partial stdout for polling
- Graceful shutdown kills background tasks on SIGTERM
- Tool description rewritten to teach LLM the polling workflow
- 65 tests passing (updated existing + added task-store, get-task-result)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/config.ts (1)
3-9: Consider validating parsed environment variables for NaN.If an environment variable contains a non-numeric string (e.g.,
MCP_HEARTBEAT_INTERVAL_MS='abc'),parseIntreturnsNaN, which could cause unexpected behavior. This is an edge case but could be protected with validation orNumber.isNaNchecks.Example validation helper
function parseIntEnv(envVar: string | undefined, defaultValue: number): number { if (!envVar) return defaultValue; const parsed = parseInt(envVar, 10); return Number.isNaN(parsed) ? defaultValue : parsed; }Also applies to: 17-17
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.ts` around lines 3 - 9, The numeric env vars (HEARTBEAT_INTERVAL_MS, EXECUTION_TIMEOUT_MS, MAX_RETRIES, RETRY_DELAY_MS) are parsed with parseInt and can become NaN for invalid input; add a small validation helper (e.g., parseIntEnv or similar) that takes the env string and default number, uses parseInt(...,10) and returns the default when Number.isNaN(parsed), then replace the direct parseInt calls in those constants with the validated helper so they always yield a numeric fallback.tests/task-store.test.ts (1)
73-80: Test forcleanupExpiredTasksdoesn't verify actual cleanup of expired tasks.The test only verifies that recent tasks aren't removed, but doesn't test that old tasks are actually cleaned up. The comments acknowledge this limitation. Consider adding a test that manipulates the internal state or uses a shorter TTL for testing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/task-store.test.ts` around lines 73 - 80, Add a new test that actually verifies cleanupExpiredTasks removes expired tasks by creating a task via createTask('t_exp', ...) then forcing it to be expired (either by mocking Date.now or by directly adjusting the stored task's timestamp retrieved with getTask('t_exp') or by configuring a short TTL used by cleanupExpiredTasks), call cleanupExpiredTasks() and assert it returns 1 and that getTask('t_exp') is now undefined; reference createTask, getTask and cleanupExpiredTasks (or the internal task store/timestamps) to locate where to adjust the timestamp or inject a mocked clock.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/task-store.ts`:
- Around line 90-104: cleanupExpiredTasks currently deletes entries for expired
tasks without terminating their running child processes, risking orphaned
processes; modify cleanupExpiredTasks so that for each expired task it checks
processes.get(taskId), calls child.kill() (or child.kill('SIGTERM') and fallback
to 'SIGKILL' on failure/timeouts) before removing from the processes and tasks
maps, handle possible exceptions from child.kill() (log via debugLog) and only
increment cleaned after the process termination attempt, mirroring the approach
used in killAllRunningTasks to ensure no orphaned children remain.
---
Nitpick comments:
In `@src/config.ts`:
- Around line 3-9: The numeric env vars (HEARTBEAT_INTERVAL_MS,
EXECUTION_TIMEOUT_MS, MAX_RETRIES, RETRY_DELAY_MS) are parsed with parseInt and
can become NaN for invalid input; add a small validation helper (e.g.,
parseIntEnv or similar) that takes the env string and default number, uses
parseInt(...,10) and returns the default when Number.isNaN(parsed), then replace
the direct parseInt calls in those constants with the validated helper so they
always yield a numeric fallback.
In `@tests/task-store.test.ts`:
- Around line 73-80: Add a new test that actually verifies cleanupExpiredTasks
removes expired tasks by creating a task via createTask('t_exp', ...) then
forcing it to be expired (either by mocking Date.now or by directly adjusting
the stored task's timestamp retrieved with getTask('t_exp') or by configuring a
short TTL used by cleanupExpiredTasks), call cleanupExpiredTasks() and assert it
returns 1 and that getTask('t_exp') is now undefined; reference createTask,
getTask and cleanupExpiredTasks (or the internal task store/timestamps) to
locate where to adjust the timestamp or inject a mocked clock.
🪄 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: a37435f0-cd1a-4e83-9162-e0fdd0a09d22
⛔ Files ignored due to path filters (11)
dist/config.jsis excluded by!**/dist/**dist/descriptions.jsis excluded by!**/dist/**dist/roomodes.jsis excluded by!**/dist/**dist/server.jsis excluded by!**/dist/**dist/spawn.jsis excluded by!**/dist/**dist/task-store.jsis excluded by!**/dist/**dist/tool-definitions.jsis excluded by!**/dist/**dist/tools/claude-code.jsis excluded by!**/dist/**dist/tools/convert-task.jsis excluded by!**/dist/**dist/tools/get-task-result.jsis excluded by!**/dist/**dist/validation.jsis excluded by!**/dist/**
📒 Files selected for processing (12)
src/config.tssrc/descriptions.tssrc/server.tssrc/spawn.tssrc/task-store.tssrc/tool-definitions.tssrc/tools/claude-code.tssrc/tools/get-task-result.tstests/task-store.test.tstests/tool-definitions.test.tstests/tools/claude-code.test.tstests/tools/get-task-result.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/tools/get-task-result.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/tools/claude-code.test.ts
- src/descriptions.ts
- tests/tool-definitions.test.ts
Comment on lines
+90
to
+104
| export function cleanupExpiredTasks(): number { | ||
| const now = Date.now(); | ||
| let cleaned = 0; | ||
| for (const [taskId, task] of tasks) { | ||
| if (now - task.createdAt > TASK_TTL_MS) { | ||
| tasks.delete(taskId); | ||
| processes.delete(taskId); | ||
| cleaned++; | ||
| } | ||
| } | ||
| if (cleaned > 0) { | ||
| debugLog(`[TaskStore] Cleaned up ${cleaned} expired task(s)`); | ||
| } | ||
| return cleaned; | ||
| } |
There was a problem hiding this comment.
cleanupExpiredTasks may leave orphaned child processes for expired running tasks.
If a task's TTL expires while it's still running, the function deletes the process reference without terminating it first. Consider calling child.kill() before deletion, similar to killAllRunningTasks.
Suggested fix
export function cleanupExpiredTasks(): number {
const now = Date.now();
let cleaned = 0;
for (const [taskId, task] of tasks) {
if (now - task.createdAt > TASK_TTL_MS) {
+ if (task.status === 'running') {
+ const child = processes.get(taskId);
+ if (child) {
+ try { child.kill('SIGTERM'); } catch { /* ignore */ }
+ }
+ }
tasks.delete(taskId);
processes.delete(taskId);
cleaned++;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function cleanupExpiredTasks(): number { | |
| const now = Date.now(); | |
| let cleaned = 0; | |
| for (const [taskId, task] of tasks) { | |
| if (now - task.createdAt > TASK_TTL_MS) { | |
| tasks.delete(taskId); | |
| processes.delete(taskId); | |
| cleaned++; | |
| } | |
| } | |
| if (cleaned > 0) { | |
| debugLog(`[TaskStore] Cleaned up ${cleaned} expired task(s)`); | |
| } | |
| return cleaned; | |
| } | |
| export function cleanupExpiredTasks(): number { | |
| const now = Date.now(); | |
| let cleaned = 0; | |
| for (const [taskId, task] of tasks) { | |
| if (now - task.createdAt > TASK_TTL_MS) { | |
| if (task.status === 'running') { | |
| const child = processes.get(taskId); | |
| if (child) { | |
| try { child.kill('SIGTERM'); } catch { /* ignore */ } | |
| } | |
| } | |
| tasks.delete(taskId); | |
| processes.delete(taskId); | |
| cleaned++; | |
| } | |
| } | |
| if (cleaned > 0) { | |
| debugLog(`[TaskStore] Cleaned up ${cleaned} expired task(s)`); | |
| } | |
| return cleaned; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/task-store.ts` around lines 90 - 104, cleanupExpiredTasks currently
deletes entries for expired tasks without terminating their running child
processes, risking orphaned processes; modify cleanupExpiredTasks so that for
each expired task it checks processes.get(taskId), calls child.kill() (or
child.kill('SIGTERM') and fallback to 'SIGKILL' on failure/timeouts) before
removing from the processes and tasks maps, handle possible exceptions from
child.kill() (log via debugLog) and only increment cleaned after the process
termination attempt, mirroring the approach used in killAllRunningTasks to
ensure no orphaned children remain.
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.
Summary
/bin/bash <binary>fails withcannot execute binary filespawnAsync('/bin/bash', [this.claudeCliPath, ...args])→spawnAsync(this.claudeCliPath, args)in both the health check and the main CLI execution pathspawnhandles bothTest plan
availableclaude_codetool executes prompts and returns results end-to-endSummary by CodeRabbit
New Features
Refactor
Documentation
Tests
Chores