fix(windows): improve argument escaping and error detection#90
fix(windows): improve argument escaping and error detection#90moffermann wants to merge 2 commits intotuannvm:mainfrom
Conversation
- Extract escapeArgForWindowsShell to separate module for testability - Handle newlines in arguments (replace with spaces for cmd.exe safety) - Use caret (^) escaping for quotes when no spaces present - Fix error detection: don't treat stderr-only output as success - Improve comments to accurately describe shell usage purpose - Add comprehensive unit tests (14 cases) for Windows escaping 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
WalkthroughAdds a Windows cmd.exe argument-escaping utility, refactors command execution to use it, introduces streaming execution with onProgress callbacks and 10MB buffer limits, and updates exit-code and error-handling semantics for streaming and non-streaming paths. Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant executeCommandStreaming
participant ChildProcess
participant ProgressCallback
Caller->>executeCommandStreaming: executeCommandStreaming(cmd, args, { onProgress })
executeCommandStreaming->>executeCommandStreaming: Escape Windows args if needed
executeCommandStreaming->>ChildProcess: spawn(cmd, args, { shell: true? })
rect rgb(200,220,240)
Note over ChildProcess,ProgressCallback: Streaming phase
ChildProcess-->>executeCommandStreaming: stdout / stderr chunks
executeCommandStreaming->>executeCommandStreaming: Accumulate & buffer (max 10MB)
executeCommandStreaming->>ProgressCallback: onProgress (debounced) with current output
end
ChildProcess-->>executeCommandStreaming: exit event (exitCode)
rect rgb(220,200,240)
Note over executeCommandStreaming,ProgressCallback: Completion & validation
executeCommandStreaming->>executeCommandStreaming: Validate exit code (0 or has stdout)
executeCommandStreaming->>ProgressCallback: onProgress (final summary)
executeCommandStreaming-->>Caller: return { stdout, stderr, exitCode }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/utils/escape.ts (1)
15-35: Empty string argument may be lost by cmd.exe.When an empty string is passed to cmd.exe without quotes, it may be silently dropped as an argument. Consider wrapping empty strings in quotes to preserve them:
🔎 Proposed fix
export function escapeArgForWindowsShell(arg: string): string { + // Empty strings need quotes to be preserved as arguments + if (arg === '') { + return '""'; + } + // Replace newlines with spaces - cmd.exe interprets \n as command separator let escaped = arg.replace(/[\r\n]+/g, ' ');src/utils/command.ts (1)
186-194: Final progress message may mislead for short outputs.The
...suffix is always appended regardless of output length, which could be misleading when the full output is less than 200 characters.🔎 Proposed fix
if (options.onProgress && (stdout || stderr)) { const finalOutput = stdout || stderr; const lastChunk = finalOutput.slice(-500); // Last 500 chars if (lastChunk.trim()) { + const trimmed = lastChunk.trim(); + const display = trimmed.length > 200 + ? `${trimmed.slice(0, 200)}...` + : trimmed; options.onProgress( - `[Completed] ${lastChunk.trim().slice(0, 200)}...` + `[Completed] ${display}` ); } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/__tests__/windows-escape.test.tssrc/utils/command.tssrc/utils/escape.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/__tests__/windows-escape.test.ts (1)
src/utils/escape.ts (1)
escapeArgForWindowsShell(15-35)
src/utils/command.ts (2)
src/utils/escape.ts (1)
escapeArgForWindowsShell(15-35)src/types.ts (1)
CommandResult(95-98)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: code-review
🔇 Additional comments (7)
src/utils/escape.ts (1)
1-35: Well-structured escape utility with good documentation.The implementation correctly handles the key cmd.exe escaping concerns:
- Newline normalization to prevent command injection
- Percent sign doubling to prevent environment variable expansion
- Proper quoting strategy with quote-doubling vs caret escaping
The separation into a dedicated module improves testability as intended.
src/__tests__/windows-escape.test.ts (2)
100-102: Empty string test expectation may need adjustment.If the escape function is updated to wrap empty strings in quotes (to preserve them as arguments in cmd.exe), this test expectation would need to change to
'""'.
1-119: Comprehensive test coverage for Windows escaping.The test suite covers the key scenarios well:
- Basic arguments and flags
- Percent sign escaping for env var protection
- Newline sanitization
- Quote handling (caret vs doubling strategies)
- Shell metacharacters
- Real-world complex cases
Good use of descriptive test names and logical grouping.
src/utils/command.ts (4)
6-6: Correct integration of the escape utility.Clean import of the centralized escaping function, maintaining single responsibility.
24-26: Appropriate Windows argument escaping.The comment correctly notes that only arguments are escaped (not the command name), which aligns with the escape function's documented scope.
68-91: Exit code handling logic is sound.The approach of accepting non-zero exit codes when stdout is present (with a warning) while rejecting non-zero with only stderr appropriately balances tolerance for quirky tools with proper error detection. The comment at lines 73-75 clearly documents this intentional behavior.
196-214: Consistent error handling between streaming and non-streaming paths.The streaming variant now mirrors the non-streaming exit code logic, with appropriate comments explaining why only stdout (not stderr) counts for non-zero exit codes. This correctly prevents masking ENOENT errors when
shell: trueis used.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
The handleError function now traverses the error cause chain to display all error details instead of only the top-level message. This helps users understand why commands failed (e.g., authentication errors, command not found, network issues). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/errors.ts:
- Around line 37-50: The cause-chain traversal starting at error.cause (variable
current) must guard against cycles and unbounded depth: add a Set (e.g., seen)
to record visited cause objects and check seen.has(current) at the top of the
while loop to break if a cycle is detected, and also enforce a sensible maxDepth
counter (e.g., 50–100) to prevent runaway traversal; when breaking for a cycle
or max depth, append a short message to messages (e.g., "[circular cause]" or
"[truncated cause chain]") and otherwise proceed as before pushing
current.message and advancing current = current.cause.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/errors.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: code-review
🔇 Additional comments (1)
src/errors.ts (1)
52-57: Clean implementation of consecutive deduplication.The deduplication logic correctly removes only consecutive identical messages, which is appropriate for error cause chains where the same message at different levels might provide useful context.
| // Traverse the cause chain to get root cause details | ||
| let current: unknown = error.cause; | ||
| while (current) { | ||
| if (current instanceof Error) { | ||
| messages.push(current.message); | ||
| current = current.cause; | ||
| } else if (typeof current === 'string') { | ||
| messages.push(current); | ||
| break; | ||
| } else { | ||
| messages.push(String(current)); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add safeguards against circular references and unbounded traversal.
The cause chain traversal lacks protection against circular references (e.g., err1.cause = err2, err2.cause = err1), which would cause an infinite loop. While uncommon in typical usage, error-handling code should be defensive against edge cases.
🔒 Proposed fix with cycle detection
// Traverse the cause chain to get root cause details
let current: unknown = error.cause;
+ const seen = new Set<Error>();
+ const MAX_DEPTH = 10;
+ let depth = 0;
+
- while (current) {
+ while (current && depth < MAX_DEPTH) {
if (current instanceof Error) {
+ if (seen.has(current)) break; // Circular reference detected
+ seen.add(current);
messages.push(current.message);
current = current.cause;
+ depth++;
} else if (typeof current === 'string') {
messages.push(current);
break;
} else {
messages.push(String(current));
break;
}
}📝 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.
| // Traverse the cause chain to get root cause details | |
| let current: unknown = error.cause; | |
| while (current) { | |
| if (current instanceof Error) { | |
| messages.push(current.message); | |
| current = current.cause; | |
| } else if (typeof current === 'string') { | |
| messages.push(current); | |
| break; | |
| } else { | |
| messages.push(String(current)); | |
| break; | |
| } | |
| } | |
| // Traverse the cause chain to get root cause details | |
| let current: unknown = error.cause; | |
| const seen = new Set<Error>(); | |
| const MAX_DEPTH = 10; | |
| let depth = 0; | |
| while (current && depth < MAX_DEPTH) { | |
| if (current instanceof Error) { | |
| if (seen.has(current)) break; // Circular reference detected | |
| seen.add(current); | |
| messages.push(current.message); | |
| current = current.cause; | |
| depth++; | |
| } else if (typeof current === 'string') { | |
| messages.push(current); | |
| break; | |
| } else { | |
| messages.push(String(current)); | |
| break; | |
| } | |
| } |
🤖 Prompt for AI Agents
In @src/errors.ts around lines 37 - 50, The cause-chain traversal starting at
error.cause (variable current) must guard against cycles and unbounded depth:
add a Set (e.g., seen) to record visited cause objects and check
seen.has(current) at the top of the while loop to break if a cycle is detected,
and also enforce a sensible maxDepth counter (e.g., 50–100) to prevent runaway
traversal; when breaking for a cycle or max depth, append a short message to
messages (e.g., "[circular cause]" or "[truncated cause chain]") and otherwise
proceed as before pushing current.message and advancing current = current.cause.
1 participant
🤖 Generated with Claude Code
Summary
Changes
Testing
Related Issues
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.