fix: resolve two rc-channel Sentry crashes (MAESTRO-Q8, MAESTRO-QA)#1041
fix: resolve two rc-channel Sentry crashes (MAESTRO-Q8, MAESTRO-QA)#1041pedramamini wants to merge 2 commits into
Conversation
Node throws "spawn EINVAL" when launching a .cmd/.bat file without a shell (CVE-2024-27980 hardening). npm-installed agent CLIs resolve to shims like claude.cmd/codex.cmd on Windows, so tab naming — which spawns agents through child_process — failed for every Windows user. Auto-enable shell for .cmd/.bat commands (mirroring the existing .exe and shebang-script branches) and quote command paths containing spaces under the default cmd.exe shell so npm shims under "C:\Users\First Last\..." resolve correctly. Fixes MAESTRO-Q8 (channel: rc).
HistoryManager.getEntries already degrades to an empty history when a session file is truncated/corrupt, but it also captured the JSON SyntaxError to Sentry. A history write interrupted by a crash or power loss is an expected, recoverable on-disk condition — not a code bug — so this only buried the dashboard in non-actionable noise. Treat SyntaxError as handled (log + return []); keep reporting genuinely unexpected read failures (permissions, I/O, etc.). Fixes MAESTRO-QA (channel: rc).
📝 WalkthroughWalkthroughThis PR introduces two independent error-handling improvements: History Manager now treats truncated/corrupt JSON history files as recoverable conditions without reporting to Sentry, while ChildProcessSpawner auto-enables shell mode for Windows batch files and defensively quotes command paths containing spaces to prevent cmd.exe path splitting. ChangesHistory Manager Corrupt JSON Handling
Windows Batch File Shell Spawning
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR fixes two field crashes on the rc channel:
Confidence Score: 5/5Both changes are narrow, well-tested bug fixes with no impact on non-Windows paths and no changes to data persistence logic. The ChildProcessSpawner changes add two tightly-scoped Windows-only guards: one that mirrors an already-working pattern (auto-shell for .exe → .cmd/.bat), and one that defensively quotes paths containing spaces only when spawning through the default cmd.exe shell. The history-manager change is a single instanceof check that redirects one error type away from Sentry without altering the return value. Both fixes have dedicated unit tests, the existing beforeEach vi.resetAllMocks() ensures mock state is clean between tests, and parseHistoryFileData re-throws the original SyntaxError on unrecoverable input so the new branch is reliably reachable. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[ChildProcessSpawner.spawn] --> B{isWindows?}
B -- No --> Z[spawn as-is]
B -- Yes --> C{commandExt}
C -- .exe, no path --> D[useShell = true]
C -- .cmd / .bat --> E[useShell = true - NEW branch]
C -- no ext + path + shebang --> F[useShell = true]
C -- other --> G{config.runInShell?}
G -- Yes --> D
G -- No --> Z
D --> H{spawnShell === true AND path has spaces?}
E --> H
F --> H
H -- Yes --> I[Quote spawnCommand - NEW quoting guard]
H -- No --> J[spawn with shell=true]
I --> J
subgraph getEntries
direction TD
R[readFile] --> S{parse error?}
S -- ENOENT --> T[return empty]
S -- SyntaxError - NEW --> U[logger.warn / return empty / no Sentry]
S -- other error --> V[logger.warn / captureException / return empty]
S -- OK --> W[return entries]
end
Reviews (1): Last reviewed commit: "fix(history): stop reporting corrupt his..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/main/process-manager/spawners/ChildProcessSpawner.ts`:
- Around line 329-343: The current quote-guard only runs when spawnShell ===
true and misses cases where spawnShell is a concrete shell string that points to
cmd.exe; update the condition in ChildProcessSpawner around spawnCommand quoting
to also detect explicit cmd.exe shell strings (e.g., check spawnShell === true
OR basename/spawnShell endsWith 'cmd.exe' case-insensitively) so that when
running on Windows and the resolved shell is cmd.exe you still wrap spawnCommand
with quotes if it contains whitespace and isn't already quoted; refer to symbols
spawnShell, spawnCommand and isWindows() when making this change.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 895543a7-112a-44e6-8b1a-e9b019af9676
📒 Files selected for processing (4)
src/__tests__/main/history-manager.test.tssrc/__tests__/main/process-manager/spawners/ChildProcessSpawner.test.tssrc/main/history-manager.tssrc/main/process-manager/spawners/ChildProcessSpawner.ts
| // When spawning through the default Windows shell (cmd.exe via ComSpec), | ||
| // Node concatenates the command and args into a single command line without | ||
| // quoting the command itself. A command path that contains spaces — e.g. an | ||
| // npm shim under "C:\Users\First Last\AppData\Roaming\npm\claude.cmd" — would | ||
| // be split by cmd.exe and fail. Quote it defensively. We only do this for the | ||
| // boolean (cmd.exe) shell path; an explicit shell string carries its own | ||
| // quoting rules and is the caller's responsibility. | ||
| if ( | ||
| isWindows() && | ||
| spawnShell === true && | ||
| /\s/.test(spawnCommand) && | ||
| !spawnCommand.startsWith('"') | ||
| ) { | ||
| spawnCommand = `"${spawnCommand}"`; | ||
| } |
There was a problem hiding this comment.
Handle explicit cmd.exe shell paths here too.
This quote guard only runs when spawnShell === true, but src/main/ipc/handlers/process.ts:715-731 and 887-904 already thread a concrete shell string through config.shell. If that resolves to cmd.exe, this branch is skipped and a batch shim under a path like C:\Users\First Last\... still gets split by cmd.exe.
Suggested fix
+ const shellBaseName =
+ typeof spawnShell === 'string'
+ ? spawnShell.split(/[\\/]/).pop()?.toLowerCase()
+ : undefined;
+
if (
isWindows() &&
- spawnShell === true &&
+ (spawnShell === true || shellBaseName === 'cmd.exe' || shellBaseName === 'cmd') &&
/\s/.test(spawnCommand) &&
!spawnCommand.startsWith('"')
) {
spawnCommand = `"${spawnCommand}"`;
}📝 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.
| // When spawning through the default Windows shell (cmd.exe via ComSpec), | |
| // Node concatenates the command and args into a single command line without | |
| // quoting the command itself. A command path that contains spaces — e.g. an | |
| // npm shim under "C:\Users\First Last\AppData\Roaming\npm\claude.cmd" — would | |
| // be split by cmd.exe and fail. Quote it defensively. We only do this for the | |
| // boolean (cmd.exe) shell path; an explicit shell string carries its own | |
| // quoting rules and is the caller's responsibility. | |
| if ( | |
| isWindows() && | |
| spawnShell === true && | |
| /\s/.test(spawnCommand) && | |
| !spawnCommand.startsWith('"') | |
| ) { | |
| spawnCommand = `"${spawnCommand}"`; | |
| } | |
| // When spawning through the default Windows shell (cmd.exe via ComSpec), | |
| // Node concatenates the command and args into a single command line without | |
| // quoting the command itself. A command path that contains spaces — e.g. an | |
| // npm shim under "C:\Users\First Last\AppData\Roaming\npm\claude.cmd" — would | |
| // be split by cmd.exe and fail. Quote it defensively. We only do this for the | |
| // boolean (cmd.exe) shell path; an explicit shell string carries its own | |
| // quoting rules and is the caller's responsibility. | |
| const shellBaseName = | |
| typeof spawnShell === 'string' | |
| ? spawnShell.split(/[\\/]/).pop()?.toLowerCase() | |
| : undefined; | |
| if ( | |
| isWindows() && | |
| (spawnShell === true || shellBaseName === 'cmd.exe' || shellBaseName === 'cmd') && | |
| /\s/.test(spawnCommand) && | |
| !spawnCommand.startsWith('"') | |
| ) { | |
| spawnCommand = `"${spawnCommand}"`; | |
| } |
🤖 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/main/process-manager/spawners/ChildProcessSpawner.ts` around lines 329 -
343, The current quote-guard only runs when spawnShell === true and misses cases
where spawnShell is a concrete shell string that points to cmd.exe; update the
condition in ChildProcessSpawner around spawnCommand quoting to also detect
explicit cmd.exe shell strings (e.g., check spawnShell === true OR
basename/spawnShell endsWith 'cmd.exe' case-insensitively) so that when running
on Windows and the resolved shell is cmd.exe you still wrap spawnCommand with
quotes if it contains whitespace and isn't already quoted; refer to symbols
spawnShell, spawnCommand and isWindows() when making this change.
1 participant
Addresses two field crashes reported on the rc channel (validated via the
channel: rc/*-RCrelease tags in Sentry and by tracing the affected code).MAESTRO-Q8 —
Error: spawn EINVAL(133 events, 100% Windows)ChildProcessSpawnerauto-enablesshellon Windows for bare.execommands and shebang scripts, but not for.cmd/.bat. Since Node's CVE-2024-27980 hardening,child_process.spawnthrowsspawn EINVALwhen launching a batch file withoutshell: true. npm-installed agent CLIs resolve to shims likeclaude.cmd/codex.cmd/opencode.cmd, so tab naming (which spawns agents throughchild_process) failed for every affected Windows user.Fix: add a
.cmd/.batauto-shell branch (mirroring the existing.exe/shebang branches), and quote command paths containing spaces under the default cmd.exe shell so npm shims underC:\Users\First Last\...resolve correctly.MAESTRO-QA —
SyntaxError: Unterminated string in JSON(25 events)HistoryManager.getEntriesalready degrades gracefully to an empty history when a session file is truncated/corrupt — but it also calledcaptureExceptionon the JSONSyntaxError. A history write interrupted by a crash or power loss is an expected, recoverable on-disk condition, not a code bug, so this only buried the Sentry dashboard in non-actionable noise.Fix: treat
SyntaxErroras handled (log + return[]); genuinely unexpected read failures (permissions, I/O, etc.) are still reported.Excluded (investigated, not actionable here)
Logger.addLog): already fixed — the console write is wrapped in try/catch ("Fixes MAESTRO-5C"); the lone event is from a stale0.15.4-RCbuild.2e369c927on rc.Testing
ChildProcessSpawner.test.ts(Windows.cmd/.batshell + path quoting) andhistory-manager.test.ts(corrupt JSON not reported; real read errors still reported).npm run lint(all 3 tsconfigs) ✅ —vitestfor both affected suites: 116 passed ✅Summary by CodeRabbit
Release Notes