fix(terminal): use Windows cmd.exe compatible shell escaping for Claude invocation #1044
Conversation
…de invocation On Windows, the terminal uses cmd.exe which has different syntax than bash: - Single quotes don't work as string delimiters (cmd.exe treats them literally) - PATH=value command syntax is bash-specific This caused the Claude button click to fail immediately on Windows because commands like `PATH='...' 'claude.exe'` are invalid in cmd.exe. Changes: - Add escapeForPlatform() function that uses double quotes on Windows - Add isWindows constant for platform detection - Update buildClaudeShellCommand() with Windows-specific branches: - Default: Skip PATH= prefix (PTY already has correct PATH) - Temp-file: Use 'call' and 'del' instead of 'source' and 'rm' - Config-dir: Use 'set' instead of bash inline env syntax - Update temp file handling to use .bat extension and batch syntax on Windows - Replace all escapeShellArg(claudeCmd) calls with escapeForPlatform() Before (broken on Windows): cd "D:\..." && PATH='...' 'C:\...\claude.exe' After (Windows-compatible): cd "D:\..." && "C:\...\claude.exe" Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds Windows-aware platform handling to Claude command construction: centralizes platform escaping, branches command/temp-file/config-dir behaviors by OS, adapts temp-file extension/content and invocation, and exposes Changes
Sequence Diagram(s)sequenceDiagram
participant App as Frontend App
participant Builder as ClaudeCommandBuilder
participant FS as FileSystem
participant Shell as OS Shell
participant Claude as Claude CLI
App->>Builder: request command (method, token, configDir)
Builder->>Builder: detect platform (isWindows)
alt temp-file method
Builder->>FS: write temp file (.bat on Windows / shell script on Unix)
Builder->>Shell: construct platform-specific invocation (batch vs shell)
Shell->>Claude: execute constructed command
Shell->>FS: remove temp file
else config-dir method
Builder->>Builder: use rawConfigDir and escapedConfigDir to set CLAUDE_CONFIG_DIR
Builder->>Shell: construct env-setting invocation (Windows `set` vs Unix `export`)
Shell->>Claude: invoke Claude CLI with escaped args
end
Claude-->>App: return output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Summary of ChangesHello @kirollosatef, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug preventing the Claude CLI from launching correctly on Windows terminals. The core problem stemmed from the application's attempt to execute bash-style commands, including single-quoted paths and Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🎉 Thanks for your first PR!
A maintainer will review it soon. Please make sure:
- Your branch is synced with
develop - CI checks pass
- You've followed our contribution guide
Welcome to the Auto Claude community!
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue with shell command escaping on Windows by introducing platform-specific logic for building and escaping commands. The use of isWindows and escapeForPlatform centralizes the platform detection and makes the code cleaner.
However, I've found a critical security vulnerability in the way temporary batch files are created on Windows to handle OAuth tokens. The token is written to the file without being escaped, which can lead to command injection if the token contains special characters. I've left two critical comments pointing out the vulnerable locations and suggesting a fix using the existing escapeShellArgWindows utility.
Apart from this critical issue, the changes are well-implemented and improve cross-platform compatibility.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/terminal/claude-integration-handler.ts:
- Around line 730-732: The PATH prefix in resumeClaudeAsync is currently built
the same way as in resumeClaude and therefore on Windows incorrectly produces a
bash-style "PATH=" assignment; update resumeClaudeAsync to mirror resumeClaude's
logic: build pathPrefix using claudeEnv.PATH and
normalizePathForBash/escapeShellArg but skip prepending "PATH=" when
process.platform === 'win32' (i.e., only use the "PATH=" prefix on non-Windows),
referencing the pathPrefix variable and functions normalizePathForBash and
escapeShellArg in resumeClaudeAsync so behavior matches resumeClaude.
- Around line 552-554: The PATH prefix in resumeClaude is built using bash
syntax and will break on Windows; update resumeClaude so it mirrors
buildClaudeShellCommand's behavior by not prepending a bash-style PATH on
Windows (use process.platform === 'win32' or the existing isWindows helper),
i.e. only construct pathPrefix = `PATH=... ` using
escapeShellArg(normalizePathForBash(claudeEnv.PATH)) when not on Windows, and
otherwise omit the prefix entirely; ensure the same fix is applied to the other
occurrence around line 569 so resumeClaude produces a valid command on Windows.
- Around line 488-491: The Windows branch writing tempFileContent injects token
raw into the batch file (in the isWindows branch) and must escape batch-special
characters to prevent command injection; implement or reuse an escaping helper
(e.g., escapeBatchArg or similar) and replace direct ${token} usage in the
isWindows branch with the escaped value so the line becomes set
CLAUDE_CODE_OAUTH_TOKEN=<escapedToken> in the Windows batch content
construction; ensure the helper handles %, ^, &, |, <, > and surrounding
whitespace consistently and mirror the Unix approach that uses
escapeShellArg(token).
- Around line 670-672: The tempFileContent construction in the async Claude
invocation is not escaping the token for the Windows branch; update the code
that builds tempFileContent (the variable using isWindows) to mirror the
synchronous invokeClaude fix by wrapping/escaping the token with
escapeShellArg(token) (and ensuring the Windows batch line sets
CLAUDE_CODE_OAUTH_TOKEN using the escaped value, preserving the existing CRLF
for Windows and newline for Unix) so both branches securely escape the token.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/frontend/src/main/terminal/claude-integration-handler.ts
🧰 Additional context used
📓 Path-based instructions (2)
apps/frontend/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{tsx,ts}: Use i18n translation keys for all user-facing text in the frontend. All labels, buttons, messages must use translation keys from react-i18next with namespace:section.key format (e.g., 'navigation:items.githubPRs').
Never use hardcoded strings for UI text in JSX/TSX files. Always use translation keys via useTranslation() hook.
Files:
apps/frontend/src/main/terminal/claude-integration-handler.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/terminal/claude-integration-handler.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). (3)
- GitHub Check: Seer Code Review
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (3)
apps/frontend/src/main/terminal/claude-integration-handler.ts (3)
104-118: LGTM!The Windows cmd.exe syntax is appropriate:
callfor batch file execution,delfor cleanup, andsetfor environment variables. Correctly omits the PATH prefix for the default case since the PTY environment already has the correct PATH.
460-460: LGTM!The consistent use of
escapeForPlatformforclaudeCmdandconfigDirpaths ensures proper escaping across both Windows and Unix platforms.Also applies to: 502-502
44-50: No action needed—the implementation is correct.The
escapeShellArgWindowsfunction only escapes cmd.exe special characters using the^escape character and returns an unquoted string. The wrapping in double quotes at line 47 inescapeForPlatformis correct and necessary, not a double-quoting issue. This pattern is used consistently throughout the codebase (e.g., inbuildCdCommandandterminal-handlers.ts).
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Addresses security and compatibility issues identified in PR review: 1. **Security: Escape token in Windows batch files** - Use `escapeShellArgWindows(token)` to prevent command injection - Tokens with special chars like `&`, `|` could execute arbitrary commands 2. **Fix resumeClaude/resumeClaudeAsync PATH prefix** - Skip bash-style `PATH=...` prefix on Windows - The PTY already has the correct PATH in its environment 3. **Fix CLAUDE_CONFIG_DIR quoting** - Use `set "VAR=value"` syntax instead of `set VAR="value"` - Prevents quotes from being included in the variable value - Added rawConfigDir to ClaudeCommandConfig type for proper escaping 4. **Update test file** - Add rawConfigDir to config-dir test case Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts (1)
114-144: Test logic contradicts implementation - causes pipeline failure.This test sets
process.platformto'win32'but expects bash-stylePATH='...'output. However, the implementation at lines 553-556 ofclaude-integration-handler.tsexplicitly skips the PATH prefix on Windows:const pathPrefix = !isWindows && claudeEnv.PATH ? `PATH=${escapeShellArg(...)} ` : '';The test expectation at line 137 will never match because Windows commands won't contain
PATH=at all.Options:
- Remove this test if PATH conversion is no longer relevant on Windows
- Change the test to verify Unix behavior (don't mock platform as
win32)- Update expectations to match actual Windows output format
Proposed fix: Test Unix PATH normalization instead
- it('converts Windows PATH separators to colons for bash invocations', async () => { - const originalPlatform = Object.getOwnPropertyDescriptor(process, 'platform'); - Object.defineProperty(process, 'platform', { value: 'win32' }); - - try { - mockGetClaudeCliInvocation.mockReturnValue({ - command: 'C:\\Tools\\claude\\claude.exe', - env: { PATH: 'C:\\Tools\\claude;C:\\Windows' }, - }); + it('converts Windows-style PATH separators to colons for bash on Unix', async () => { + // Test that normalizePathForBash converts semicolons to colons + // This is relevant when PATH contains Windows-style separators on Unix + mockGetClaudeCliInvocation.mockReturnValue({ + command: '/opt/claude/bin/claude', + env: { PATH: '/opt/claude/bin;/usr/bin' }, // Unusual but tests the conversion + });apps/frontend/src/main/terminal/claude-integration-handler.ts (1)
102-131: Windows command construction looks correct, but verify edge cases.The Windows branching logic correctly:
- Uses
clsinstead ofclear- Uses
callanddelfor temp-file method- Uses
set "VAR=value"syntax which is the correct Windows pattern for paths with spacesOne minor observation: Line 108 uses
delwithout/Q(quiet) flag, which may prompt for confirmation on read-only files. Consider usingdel /Qfor silent deletion.Optional: Add /Q flag to del command
case 'temp-file': // On Windows, use 'call' to execute the batch file that sets env var, then run claude - return `cls && ${cwdCommand}call ${config.escapedTempFile} && del ${config.escapedTempFile} && ${fullCmd}\r`; + return `cls && ${cwdCommand}call ${config.escapedTempFile} && del /Q ${config.escapedTempFile} && ${fullCmd}\r`;
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/terminal/claude-integration-handler.ts:
- Around line 665-676: The temp token file creation logic is duplicated between
invokeClaudeAsync and invokeClaude and also lacks Windows-specific permission
handling; extract the shared logic into a helper (e.g.,
createTempTokenFileContent) that generates nonce, tempFile, escapedTempFile and
platform-specific content using existing symbols isWindows, escapeForPlatform,
escapeShellArgWindows, escapeShellArg, and then replace the duplicated code in
invokeClaudeAsync and invokeClaude to call this helper and write the file via
fsPromises.writeFile(tempFile, content, { mode: 0o600 }); also ensure you
address the Windows security concern by documenting or implementing an
appropriate permission/secure-write strategy for .bat files on Windows within
the helper.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.tsapps/frontend/src/main/terminal/claude-integration-handler.ts
🧰 Additional context used
📓 Path-based instructions (2)
apps/frontend/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{tsx,ts}: Use i18n translation keys for all user-facing text in the frontend. All labels, buttons, messages must use translation keys from react-i18next with namespace:section.key format (e.g., 'navigation:items.githubPRs').
Never use hardcoded strings for UI text in JSX/TSX files. Always use translation keys via useTranslation() hook.
Files:
apps/frontend/src/main/terminal/claude-integration-handler.tsapps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/terminal/claude-integration-handler.tsapps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts
🪛 GitHub Actions: CI
apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts
[error] 137-137: converts Windows PATH separators to colons for bash invocations - Assertion failed: expected PATH to contain PATH='C:\Tools\claude:C:\Windows' but received: 'cd "/tmp/project" && "C:\Tools\claude\claude.exe"...'. The test likely relies on Windows PATH conversion logic not being applied.
⏰ 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). (3)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Seer Code Review
🔇 Additional comments (7)
apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts (1)
477-477: LGTM - rawConfigDir added to match updated type signature.The addition of
rawConfigDiraligns with theClaudeCommandConfigtype change in the implementation, which now requires bothescapedConfigDirandrawConfigDirfor the'config-dir'method.apps/frontend/src/main/terminal/claude-integration-handler.ts (6)
552-556: LGTM - PATH prefix correctly skipped on Windows.The logic to skip the
PATH=prefix on Windows is correct since the PTY already has the correct PATH environment. The comment clearly explains the reasoning. This is the root cause of the failing test (already flagged above).
60-63: LGTM - Type updated correctly for Windows config-dir handling.The addition of
rawConfigDirto the'config-dir'variant is necessary because Windowsset "VAR=value"syntax requires the raw path (escaped differently than shell arguments), while Unix still uses the shell-escaped version.
461-464: LGTM - Platform-aware escaping applied correctly.The escaping logic correctly uses
escapeForPlatformfor the Claude command and maintains the bash-style PATH escaping (since it's only used on Unix per lines 553-556).
503-504: LGTM - Config-dir method correctly passes both escaped and raw paths.Both
escapedConfigDir(for Unix) andrawConfigDir(for Windowssetcommand) are correctly provided, matching the updatedClaudeCommandConfigtype.
731-735: LGTM - Async resumeClaude mirrors sync version correctly.The PATH prefix logic correctly mirrors the sync version, skipping the bash-style prefix on Windows.
35-50: LGTM - Platform detection and escaping helper are well-structured.The
isWindowsconstant andescapeForPlatformhelper provide a clean abstraction for platform-specific escaping. The function correctly delegates toescapeShellArgWindowsfor Windows andescapeShellArgfor Unix. TheescapeShellArgWindowsimplementation properly handles all cmd.exe special characters (^, &, |, <, >, ", %) with correct escape sequences and critical ordering (carets escaped first to prevent double-escaping). The Unix variant uses standard POSIX-safe single quote escaping.
- Updated test 'converts Windows PATH separators' to verify correct Windows behavior: PATH prefix is skipped on Windows (PTY already has correct PATH) - Added /Q flag to del command for quiet deletion (no confirmation prompt) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts (1)
455-503: Consider adding Windows-specific tests for temp-file and config-dir methods.The current tests for
buildClaudeShellCommandonly verify Unix/bash syntax. Given the PR adds significant Windows-specific logic (.batextension,call/del /Qcommands,set "VAR=value"syntax), consider adding tests that mockprocess.platformto'win32'and verify:
- Temp-file method produces:
cls && ... call "tempfile.bat" && del /Q "tempfile.bat" && ...- Config-dir method produces:
cls && ... set "CLAUDE_CONFIG_DIR=..." && ...This would help catch regressions in Windows-specific command generation.
Do you want me to generate the Windows-specific test cases for these methods?
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/terminal/claude-integration-handler.ts:
- Around line 489-494: Update the Windows batch assignment to quote the token
value for consistency and safety: in the tempFileContent construction inside
claude-integration-handler.ts (the branch that sets isWindows), change the set
CLAUDE_CODE_OAUTH_TOKEN line to use set "CLAUDE_CODE_OAUTH_TOKEN=..." and wrap
the result of escapeShellArgWindows(token) in quotes; do the same analogous
change in the invokeClaudeAsync implementation where the same Windows batch
pattern is produced, ensuring you use escapeShellArgWindows(token) inside the
quoted assignment.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.tsapps/frontend/src/main/terminal/claude-integration-handler.ts
🧰 Additional context used
📓 Path-based instructions (2)
apps/frontend/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{tsx,ts}: Use i18n translation keys for all user-facing text in the frontend. All labels, buttons, messages must use translation keys from react-i18next with namespace:section.key format (e.g., 'navigation:items.githubPRs').
Never use hardcoded strings for UI text in JSX/TSX files. Always use translation keys via useTranslation() hook.
Files:
apps/frontend/src/main/terminal/claude-integration-handler.tsapps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/terminal/claude-integration-handler.tsapps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.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: Seer Code Review
🔇 Additional comments (7)
apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts (2)
114-146: LGTM! Windows platform test properly validates cmd.exe syntax.The test correctly:
- Mocks
process.platformto'win32'- Verifies PATH prefix is omitted on Windows (PTY provides correct PATH)
- Validates double-quote escaping for Windows cmd.exe
- Properly restores the original platform in the
finallyblock
473-488: LGTM! Test updated to match new config-dir signature.The addition of
rawConfigDirparameter aligns with the updatedClaudeCommandConfigtype that now requires both escaped and raw paths for proper Windowssetcommand handling.apps/frontend/src/main/terminal/claude-integration-handler.ts (5)
102-120: LGTM! Windows command syntax is correct.The Windows-specific command generation:
- Uses
clsinstead ofclear- Uses
callfor batch file execution anddel /Qfor quiet deletion- Uses
set "VAR=value"syntax which correctly handles paths with spaces- Omits PATH prefix on Windows as the PTY already has the correct PATH
The implementation follows cmd.exe conventions correctly.
553-557: LGTM! Consistent Windows PATH handling in resumeClaude.The conditional
!isWindows && claudeEnv.PATHcorrectly skips the PATH prefix on Windows, consistent with thebuildClaudeShellCommandbehavior and PR objectives.
732-736: LGTM! Consistent Windows PATH handling in async resume.The async version maintains the same Windows PATH handling logic as the sync version.
60-63: LGTM! Well-designed discriminated union update.Adding
rawConfigDirto theconfig-dirvariant is necessary because:
escapedConfigDirhas platform-specific quoting (single quotes on Unix)- Windows
set "VAR=value"needs the raw path with only special character escapingThe discriminated union ensures type safety by requiring both fields together.
44-50: VerifyescapeShellArgWindowsimplementation to confirm it doesn't add quotes.The
escapeForPlatformfunction wraps the result ofescapeShellArgWindows(arg)in double quotes on Windows. IfescapeShellArgWindowsalready returns a quoted string, this would produce malformed output like""value"". Inspect the implementation ofescapeShellArgWindowsto confirm it only escapes characters without adding outer quotes.
Use `set "VAR=value"` syntax for token assignment to match CLAUDE_CONFIG_DIR handling and follow Windows batch conventions. Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/terminal/claude-integration-handler.ts:
- Around line 664-678: The temp-token file creation logic is duplicated between
invokeClaudeAsync and invokeClaude; extract it into a shared async helper (e.g.,
writeTempTokenFile(token): Promise<TempTokenFileResult>) that uses crypto,
isWindows, os/path, escapeForPlatform, escapeShellArgWindows/escapeShellArg and
fsPromises to build tempFile/tempFileContent, write the file with mode 0o600 and
return { tempFile, escapedTempFile }; then replace the duplicated blocks in both
invokeClaudeAsync and invokeClaude with calls to writeTempTokenFile (or a sync
variant if needed) and update call sites to use the returned
tempFile/escapedTempFile.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/frontend/src/main/terminal/claude-integration-handler.ts
🧰 Additional context used
📓 Path-based instructions (2)
apps/frontend/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{tsx,ts}: Use i18n translation keys for all user-facing text in the frontend. All labels, buttons, messages must use translation keys from react-i18next with namespace:section.key format (e.g., 'navigation:items.githubPRs').
Never use hardcoded strings for UI text in JSX/TSX files. Always use translation keys via useTranslation() hook.
Files:
apps/frontend/src/main/terminal/claude-integration-handler.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/terminal/claude-integration-handler.ts
🧬 Code graph analysis (1)
apps/frontend/src/main/terminal/claude-integration-handler.ts (1)
apps/frontend/scripts/download-python.cjs (2)
isWindows(358-358)os(22-22)
⏰ 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). (3)
- GitHub Check: Seer Code Review
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (4)
apps/frontend/src/main/terminal/claude-integration-handler.ts (4)
60-63: LGTM!Good type design. Adding
rawConfigDirto the discriminated union allows proper Windows-specific handling while maintaining the escaped version for Unix compatibility.
553-557: LGTM!The conditional PATH prefix correctly skips bash-style
PATH=syntax on Windows where the PTY already inherits the correct PATH. Good inline documentation.
732-736: LGTM!Consistent with the sync
resumeClaude()implementation. The Windows PATH handling is correctly applied.
106-109:callcommand with double-quoted batch file paths is standard cmd.exe syntax and works correctly.Windows cmd.exe fully supports
call "path with spaces\file.bat"syntax per Microsoft documentation. TheescapeForPlatform()implementation properly wraps paths in double quotes and usesescapeShellArgWindows()to escape special characters (^, ", &, |, <, >, %). The code at line 109 is correct.
The set "VAR=VALUE" syntax in cmd.exe treats content inside quotes literally, including caret characters. Using escapeShellArgWindows() which adds carets would corrupt values containing special characters. Added escapeForWindowsSet() that only escapes % (as %%) since that's the only character that still gets expanded inside quotes. This fixes potential path/token corruption when values contain characters like & that would get escaped as ^& incorrectly. Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/terminal/claude-integration-handler.ts`:
- Around line 463-465: The code computes pathPrefix from claudeEnv.PATH using
normalizePathForBash and escapeShellArg even on Windows where it is unused;
replicate resumeClaude's pattern by only computing pathPrefix when not on
Windows (e.g., guard with !isWindows or the same platform check used in
resumeClaude) and apply the same change to the async variant that computes
pathPrefix later; update the blocks that reference pathPrefix so they still pass
it into buildClaudeShellCommand unchanged, but avoid constructing the string on
Windows to keep behavior consistent.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
apps/frontend/src/main/terminal/claude-integration-handler.tsapps/frontend/src/shared/utils/shell-escape.ts
🧰 Additional context used
📓 Path-based instructions (2)
apps/frontend/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{tsx,ts}: Use i18n translation keys for all user-facing text in the frontend. All labels, buttons, messages must use translation keys from react-i18next with namespace:section.key format (e.g., 'navigation:items.githubPRs').
Never use hardcoded strings for UI text in JSX/TSX files. Always use translation keys via useTranslation() hook.
Files:
apps/frontend/src/main/terminal/claude-integration-handler.tsapps/frontend/src/shared/utils/shell-escape.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/terminal/claude-integration-handler.tsapps/frontend/src/shared/utils/shell-escape.ts
🧬 Code graph analysis (1)
apps/frontend/src/main/terminal/claude-integration-handler.ts (1)
apps/frontend/src/shared/utils/shell-escape.ts (3)
escapeShellArgWindows(74-89)escapeShellArg(24-29)escapeForWindowsSet(104-108)
⏰ 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). (3)
- GitHub Check: Seer Code Review
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (9)
apps/frontend/src/shared/utils/shell-escape.ts (1)
104-108: LGTM! Correct escaping for Windowssetcommand context.The differentiation from
escapeShellArgWindowsis important and well-documented. One minor observation: double quotes are silently removed rather than rejected. For OAuth tokens and config paths this should be fine, but consider logging a warning if the value contains quotes to help debug unexpected behavior.apps/frontend/src/main/terminal/claude-integration-handler.ts (8)
16-16: LGTM!Import correctly added for the new Windows-specific escaping function.
35-50: LGTM! Clean platform abstraction.The
isWindowsconstant andescapeForPlatformhelper provide a clean abstraction for platform-specific quoting. The approach of escaping metacharacters before wrapping in double quotes on Windows is correct for cmd.exe.
60-63: LGTM!Adding
rawConfigDirto the discriminated union is the right approach—Windowsset "VAR=VALUE"syntax needs the unescaped path with onlyescapeForWindowsSetapplied, while Unix still uses the pre-escaped version.
102-132: Well-implemented Windows command construction.The Windows branching correctly addresses the previously flagged issues:
- Uses
escapeForWindowsSet(config.rawConfigDir)forsetcommand (fixes caret-escaping bug)- Omits bash-style
PATH=prefix on Windows- Uses appropriate Windows commands (
cls,call,del /Q,set)
489-494: Security fix confirmed: Token is now properly escaped.The previously flagged command injection vulnerability is fixed. Using
escapeForWindowsSet(token)correctly handles theset "VAR=VALUE"context where only%needs escaping (as%%) and quotes must be stripped.
553-557: PATH prefix fix confirmed for Windows.The
!isWindows &&guard correctly prevents bash-stylePATH=value commandsyntax from being emitted on Windows, where it would cause cmd.exe to fail.
666-677: Async version correctly mirrors sync implementation.The token escaping fix is consistently applied in both sync and async versions. The code duplication with the sync version was previously flagged and acknowledged as a future refactoring opportunity.
732-736: LGTM!PATH prefix fix consistently applied in async variant.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| const pathPrefix = claudeEnv.PATH | ||
| ? `PATH=${escapeShellArg(normalizePathForBash(claudeEnv.PATH))} ` | ||
| : ''; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor inconsistency: pathPrefix computed but unused on Windows.
Unlike resumeClaude (line 555), this still computes pathPrefix unconditionally. While not a bug (it's ignored by buildClaudeShellCommand on Windows), it's a slight inconsistency. Consider aligning with resumeClaude's pattern for consistency:
- const pathPrefix = claudeEnv.PATH
+ const pathPrefix = !isWindows && claudeEnv.PATH
? `PATH=${escapeShellArg(normalizePathForBash(claudeEnv.PATH))} `
: '';Same applies to the async version at lines 646-648.
🤖 Prompt for AI Agents
In `@apps/frontend/src/main/terminal/claude-integration-handler.ts` around lines
463 - 465, The code computes pathPrefix from claudeEnv.PATH using
normalizePathForBash and escapeShellArg even on Windows where it is unused;
replicate resumeClaude's pattern by only computing pathPrefix when not on
Windows (e.g., guard with !isWindows or the same platform check used in
resumeClaude) and apply the same change to the async variant that computes
pathPrefix later; update the blocks that reference pathPrefix so they still pass
it into buildClaudeShellCommand unchanged, but avoid constructing the string on
Windows to keep behavior consistent.
3 participants
Summary
PATH=value command)Problem
On Windows, clicking the "Claude" button in the Agent Terminal caused:
Generating terminal name for command: laude...(first char stripped)Root cause: The code was using bash/Unix syntax for all platforms:
Windows cmd.exe treats single quotes as literal characters, not string delimiters.
Solution
Added Windows-specific handling:
escapeForPlatform()- Uses double quotes on Windows, single quotes on UnixisWindowsconstant - Platform detection helperbuildClaudeShellCommand()- Windows branches:PATH=prefix (PTY has correct PATH)call/delinstead ofsource/rmsetinstead of bash inline env syntax.batextension and batch syntax on WindowsAfter fix:
Test plan
Changes
apps/frontend/src/main/terminal/claude-integration-handler.ts(+68/-22 lines)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.