fix(frontend): quote Claude CLI paths containing spaces on Windows #841
Conversation
📝 WalkthroughWalkthroughAdds multi-shell command construction and escaping, async environment/executable helpers, per-terminal shell detection and tracking, and rewrites Claude CLI invocation and IPC flows to be shell-type aware; extensive test updates and string-quoting normalization across the frontend terminal and tooling code. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TerminalMgr as Terminal Manager
participant PTYMgr as PTY Manager
participant ShellUtils as Shell Utilities
participant ClaudeHandler as Claude Integration
participant OS as OS Process
User->>TerminalMgr: create terminal (preferredTerminal)
TerminalMgr->>PTYMgr: getShellPath(preferredTerminal)
PTYMgr->>OS: probe candidate shells (existsAsync)
PTYMgr-->>TerminalMgr: return shellPath, shellType
TerminalMgr->>TerminalMgr: store TerminalProcess { shellType }
User->>ClaudeHandler: invokeClaude(profile, cwd)
ClaudeHandler->>ShellUtils: buildPathAssignment / buildEnvVarAssignment / buildCommandInvocation (shellType)
ShellUtils-->>ClaudeHandler: shell-aware command string
ClaudeHandler->>OS: execute command (uses needsShell from prepareShellCommand)
OS-->>ClaudeHandler: output/status
ClaudeHandler->>TerminalMgr: finalizeClaudeInvoke(...)
TerminalMgr-->>User: session active
sequenceDiagram
participant CLI as CLI Detection
participant EnvUtils as Env Utils
participant Analyzer as Path Analyzer
participant PyRunner as Python Runner
CLI->>EnvUtils: findExecutableAsync("claude"/"python")
EnvUtils->>Analyzer: analyzePathQuoting(executablePath)
Analyzer-->>EnvUtils: { commandPath, needsShell }
EnvUtils-->>CLI: resolved executable and shell hint
CLI->>EnvUtils: preparePythonSubprocessCommand(pythonPath)
EnvUtils->>Analyzer: analyze quoting/paths
alt needsShell
EnvUtils->>PyRunner: return { commandPath, needsShell: true }
else
EnvUtils->>PyRunner: return { commandPath, needsShell: false }
end
CLI->>PyRunner: run accordingly (shell vs direct args)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 @StillKnotKnown, 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 resolves a critical bug on Windows where the Claude CLI tool could not be properly detected or executed if its installation path included spaces. The issue arose because 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
|
![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 fixes an issue where the Claude CLI detection would fail on Windows if the path to claude.cmd contained spaces. The fix correctly wraps the command in double quotes when shell: true is used. The implementation looks correct and addresses the described problem effectively.
I've also identified a similar pre-existing bug in validatePython and validatePythonAsync where paths with spaces are not handled correctly due to pythonCmd.split(' '). I've left specific comments with suggestions to make the Python validation more robust, following a similar principle of handling paths with spaces correctly.
| const parts = pythonCmd.split(" "); | ||
| const cmd = parts[0]; | ||
| const args = [...parts.slice(1), '--version']; | ||
| const args = [...parts.slice(1), "--version"]; |
There was a problem hiding this comment.
This logic for parsing the python command is brittle and will fail for paths containing spaces. For example, if pythonCmd is C:\Program Files\Python311\python.exe, split(' ') will incorrectly break the path. This is the same class of bug that this PR fixes for the Claude CLI.
Since pythonCmd can be either a command with arguments (like py -3) or a full file path, you should distinguish between these cases instead of unconditionally splitting by space. A simple heuristic is to check for path separators.
| const parts = pythonCmd.split(" "); | |
| const cmd = parts[0]; | |
| const args = [...parts.slice(1), '--version']; | |
| const args = [...parts.slice(1), "--version"]; | |
| const parts = pythonCmd.includes(path.sep) ? [pythonCmd] : pythonCmd.split(' '); | |
| const cmd = parts[0]; | |
| const args = [...parts.slice(1), '--version']; |
| const parts = pythonCmd.split(" "); | ||
| const cmd = parts[0]; | ||
| const args = [...parts.slice(1), '--version']; | ||
| const args = [...parts.slice(1), "--version"]; |
There was a problem hiding this comment.
This logic for parsing the python command is brittle and will fail for paths containing spaces. For example, if pythonCmd is C:\Program Files\Python311\python.exe, split(' ') will incorrectly break the path. This is the same class of bug that this PR fixes for the Claude CLI.
Since pythonCmd can be either a command with arguments (like py -3) or a full file path, you should distinguish between these cases instead of unconditionally splitting by space. A simple heuristic is to check for path separators.
| const parts = pythonCmd.split(" "); | |
| const cmd = parts[0]; | |
| const args = [...parts.slice(1), '--version']; | |
| const args = [...parts.slice(1), "--version"]; | |
| const parts = pythonCmd.includes(path.sep) ? [pythonCmd] : pythonCmd.split(' '); | |
| const cmd = parts[0]; | |
| const args = [...parts.slice(1), '--version']; |
![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
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/cli-tool-manager.ts (1)
1-1835: AI summary is inconsistent with actual code changes.The AI-generated summary claims this PR only contains formatting changes with "no behavioral or control flow changes." However, the code includes a critical bug fix at lines 940-945 and 1045-1050 that adds new logic to quote paths containing spaces on Windows. This IS a behavioral change that directly addresses the reported issue #831.
Additionally, mixing a critical bug fix with extensive formatting changes (converting single quotes to double quotes throughout the entire file) makes the PR harder to review and violates the principle of separating refactoring from functional changes. The formatting changes should ideally be in a separate PR.
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/cli-tool-manager.ts:
- Around line 940-946: The code wraps claudeCmd in double quotes when needsShell
and claudeCmd includes a space, but that can double-quote an already-quoted
path; update the logic in cli-tool-manager.ts around needsShell/command to first
detect if claudeCmd already starts and ends with a double-quote (e.g.,
claudeCmd.startsWith('"') && claudeCmd.endsWith('"')) and only add quotes when
it is not already quoted; keep using the existing needsShell and claudeCmd
symbols and ensure the final command variable preserves existing quoting and
only wraps unquoted paths containing spaces.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/frontend/src/main/cli-tool-manager.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/main/cli-tool-manager.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/main/cli-tool-manager.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/cli-tool-manager.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). (2)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (2)
apps/frontend/src/main/cli-tool-manager.ts (2)
1045-1051: Async implementation correctly mirrors the sync bug fix.The same quoting logic is properly applied to the async validation path, ensuring consistency between
validateClaudeandvalidateClaudeAsync. This is good.The same edge case about pre-quoted paths (mentioned in the previous comment on lines 940-946) applies here as well.
940-951: Add test coverage for the validateClaude methods on Windows with paths containing spaces.The fix logic is correct, but there are no tests for the
validateClaude()andvalidateClaudeAsync()methods in the test suite. Without automated tests, the fix for Windows paths with spaces (e.g.,C:\Users\Jane Smith\AppData\Roaming\npm\claude.cmd) cannot be verified. Add tests covering:
- Windows platform detection with
.cmdand.batfiles- Path quoting when claudeCmd contains spaces
- Version extraction from the quoted command
These tests should verify the fix resolves the exact scenario where npm installs Claude CLI in a user directory with spaces in the username.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
StillKnotKnown
added
bug
There was a problem hiding this comment.
Actionable comments posted: 4
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/changelog/version-suggester.ts (1)
1-256: Consider separating stylistic changes from functional fixes.This PR includes extensive quote style changes (single → double quotes) throughout the file alongside the critical Windows path handling fix. These stylistic changes:
- Are functionally equivalent (TypeScript treats both identically)
- Significantly increase the diff size
- Make it harder to review the actual bug fix
- Don't align with any specified coding guidelines
For future PRs, consider:
- Separating style-only changes into a dedicated PR
- Using a linter/formatter (like Prettier) to enforce consistent quote style across the codebase automatically
- Focusing each PR on a single concern (bug fix vs. style normalization)
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/changelog/formatter.ts:
- Around line 326-336: The path quoting logic defines quotedPath but never uses
it, and the shell command string is built with escapedPath without surrounding
quotes, so paths with spaces break on Windows; update the command construction
that uses escapedPath (the variable named command) to wrap the executable path
in escaped quotes (or replace usage with quotedPath appropriately) so the final
shell command includes \"<escapedPath>\" before the arguments, ensuring cmd.exe
receives the full quoted path; keep the existing escapedPath transformations for
backslashes/inner quotes and only add the surrounding escaped double quotes when
needsShell is true.
- Around line 349-368: The shell command construction uses ${escapedPath} which
doesn't add quotes and will break on Windows paths with spaces; change it to use
${quotedPath} instead so the command string becomes command = "${quotedPath} -p
\\"{prompt}\\" --output-format text --model haiku", i.e., in the needsShell
branch replace escapedPath with quotedPath to ensure the executable path is
properly quoted when building the command string for subprocess.run.
In @apps/frontend/src/main/changelog/version-suggester.ts:
- Line 154: The generated shell command doesn't quote escapedClaudePath, so
paths with spaces break; update the command assignment where variable command is
constructed to wrap escapedClaudePath in escaped quotes (e.g., set command =
"\"${escapedClaudePath}\" chat --model haiku --prompt \\\"{prompt}\\\"") so the
resulting Python string yields a quoted path in the shell; locate the
construction using the escapedClaudePath and prompt variables in
version-suggester.ts and replace it with the quoted-path version while
preserving existing escaping for the inner prompt quotes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
apps/frontend/src/main/changelog/formatter.tsapps/frontend/src/main/changelog/version-suggester.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/main/changelog/version-suggester.tsapps/frontend/src/main/changelog/formatter.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/main/changelog/version-suggester.tsapps/frontend/src/main/changelog/formatter.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/changelog/version-suggester.tsapps/frontend/src/main/changelog/formatter.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). (2)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (2)
apps/frontend/src/main/changelog/formatter.ts (1)
1-65: LGTM: String quote normalization.The standardization from single quotes to double quotes improves consistency across the file.
apps/frontend/src/main/changelog/version-suggester.ts (1)
132-141: Well-designed Windows batch file detection and escaping strategy.The logic correctly identifies Windows batch files (
.cmd/.bat) that requireshell=Truefor execution, and applies appropriate escaping strategies for each mode:
- Shell mode: Escapes backslashes and double quotes (since the path will be in a double-quoted shell command)
- List mode: Escapes backslashes and single quotes (since the path will be in a single-quoted Python list)
This differentiation is correct for the two execution contexts.
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/changelog/formatter.ts (1)
322-387: Good use of base64 encoding for prompt, but shell command still has injection risk.The use of base64 encoding on line 324 (
Buffer.from(prompt, "utf-8").toString("base64")) is a safer approach than string escaping for passing the prompt to Python.However, when
needsShellis true (line 356), the command is still built via string concatenation:command = ${commandPath} + ' -p "' + prompt + '" --output-format text --model haiku'The
promptvariable here is the decoded base64 content, which could contain shell metacharacters. Unlike version-suggester.ts where the prompt is escaped before embedding, here the base64 is decoded and used directly.Consider either:
- Keeping the prompt base64-encoded and decoding it after passing to subprocess, or
- Escaping shell metacharacters for the prompt in the command string
🛡️ Suggested fix using stdin for prompt
${ needsShell ? `# On Windows with .cmd/.bat files, use shell=True with quoted path - command = ${commandPath} + ' -p "' + prompt + '" --output-format text --model haiku' + command = ${commandPath} + ' -p - --output-format text --model haiku' result = subprocess.run( command, capture_output=True, text=True, - stdin=subprocess.DEVNULL, + input=prompt, timeout=300, shell=True )`apps/frontend/src/main/changelog/version-suggester.ts (1)
128-181: Command injection risk via prompt in shell mode on Windows batch files.When
needsShellis true (i.e., claudePath ends with .cmd or .bat), the generated Python code constructs a command string on line 159 via string concatenation:command = ${commandPath} + ' chat --model haiku --prompt "' + prompt + '"'The prompt escaping on line 130 replaces double quotes with
\", but in the Python string context (line 153), this becomes literal backslash-quote characters. When this shell command is executed via cmd.exe, backslashes do not escape double quotes—the quote immediately closes the string context, allowing an attacker to break out and inject arbitrary shell commands.While the prompt originates from internal git commit messages (limiting exposure), an attacker with commit access could craft a malicious commit message to achieve code execution. Consider using safer alternatives such as passing the prompt via stdin or a temporary file instead of shell command concatenation.
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/changelog/version-suggester.ts:
- Around line 144-146: The Windows path quoting logic is duplicated; extract it
into a shared utility like preparePythonSubprocessCommand(executablePath:
string) (or escapePathForPythonSubprocess) that returns { commandPath,
needsShell } and implements the existing rules (detect .cmd/.bat, decide
needsShell, add surrounding quotes when needed, escape backslashes and internal
quotes/apostrophes differently for shell vs non-shell). Replace the inline logic
in version-suggester (the const commandPath calculation), formatter.ts, and
cli-tool-manager.ts (all occurrences around the Windows quoting blocks) to call
this new utility and use its commandPath and needsShell results so behavior
stays consistent across files.
In @apps/frontend/src/main/cli-tool-manager.ts:
- Around line 54-62: The type import ToolDetectionResult and other imports are
placed after the existsAsync function; move all imports (including
ToolDetectionResult, findHomebrewPythonUtil, getWindowsExecutablePaths,
getWindowsExecutablePathsAsync, WINDOWS_GIT_PATHS,
findWindowsExecutableViaWhere, findWindowsExecutableViaWhereAsync) to the top of
the file above the existsAsync declaration so all imports are grouped together
and the function definitions follow them.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
apps/frontend/src/main/changelog/formatter.tsapps/frontend/src/main/changelog/version-suggester.tsapps/frontend/src/main/cli-tool-manager.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/main/changelog/formatter.tsapps/frontend/src/main/changelog/version-suggester.tsapps/frontend/src/main/cli-tool-manager.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/main/changelog/formatter.tsapps/frontend/src/main/changelog/version-suggester.tsapps/frontend/src/main/cli-tool-manager.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/changelog/formatter.tsapps/frontend/src/main/changelog/version-suggester.tsapps/frontend/src/main/cli-tool-manager.ts
🧬 Code graph analysis (1)
apps/frontend/src/main/cli-tool-manager.ts (3)
apps/frontend/src/shared/types/cli.ts (1)
ToolDetectionResult(12-25)apps/frontend/src/main/env-utils.ts (1)
findExecutable(252-273)apps/frontend/src/main/utils/windows-paths.ts (2)
findWindowsExecutableViaWhere(134-176)getWindowsExecutablePaths(86-119)
⏰ 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). (2)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (8)
apps/frontend/src/main/changelog/version-suggester.ts (1)
1-11: LGTM - Style normalization.The import and type literal changes from single to double quotes are consistent with the project's formatting standards. The
bumpTypeunion type using double-quoted literals is semantically equivalent and aligns with the rest of the codebase.apps/frontend/src/main/cli-tool-manager.ts (4)
940-957: LGTM - Windows path quoting for shell execution.The logic correctly handles paths with spaces when executing
.cmd/.batfiles:
- Detects batch files requiring shell execution
- Only adds quotes if the path contains spaces and isn't already quoted
- Uses
execFileSyncwithshell: needsShellThe security note in the comments (lines 935-939) appropriately documents that
claudeCmdcomes from internal path detection, makingshell: trueacceptable here.
1050-1067: Async implementation mirrors sync correctly.The path quoting logic in
validateClaudeAsyncis identical tovalidateClaude, ensuring consistent behavior between sync and async code paths. This duplication is acceptable given the structural difference between the two methods.
67-67: LGTM - Type literal style normalization.The
CLITooltype definition using double-quoted string literals is consistent with the project's formatting standards and TypeScript conventions.
1845-1848: LGTM - Async cache pre-warming.The
preWarmToolCachefunction correctly usesPromise.allfor parallel tool detection and logs progress appropriately.apps/frontend/src/main/changelog/formatter.ts (3)
7-65: LGTM - Template and constant style normalization.The
FORMAT_TEMPLATESandAUDIENCE_INSTRUCTIONSconstants now use double-quoted keys and strings, consistent with the project's formatting standards. The template content and functionality remain unchanged.
138-201: LGTM - Changelog prompt builder.The refactored
buildChangelogPromptfunction uses consistent arrow function syntax and double-quoted strings. The functional behavior remains unchanged with clear task summary construction.
206-317: LGTM - Git prompt builder.The
buildGitPromptfunction updates are stylistic only. The commit line formatting and switch-case handling for history types remain functionally equivalent.
There was a problem hiding this comment.
Actionable comments posted: 4
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/cli-tool-manager.ts (2)
23-1830: Large stylistic change mixed with functional changes.The entire file has been converted from single quotes to double quotes (~200+ lines), which makes the diff significantly larger and harder to review. While consistency is valuable, it's generally better practice to separate purely stylistic changes into a dedicated commit or PR to improve reviewability and make the functional changes (Windows path quoting fix) more prominent.
That said, if double quotes are the project standard, this change is acceptable.
934-966: Correct fix for Windows .cmd file execution with spaces in paths, but update validation messages to use i18n translation keys.The use of
prepareShellCommandproperly addresses the Windows .cmd/.bat execution issue. The implementation correctly:
- Detects .cmd/.bat files that require
shell: true- Quotes paths containing spaces when shell mode is enabled
- Includes clear security documentation explaining why
shell: trueis safeHowever, the hardcoded user-facing messages in the validation methods violate the frontend i18n guideline. Lines 950 and 955 contain strings that should be externalized to translation files:
- "Claude CLI ${versionStr} is available"
- "Failed to validate Claude CLI: ..."
This applies to all
validate*methods in the file (validateClaude, validatePython, validateGit, validateGitHubCLI, and their async variants). These messages should use i18n translation keys with dynamic version/error values injected at runtime, similar to the established pattern inapps/frontend/src/shared/i18n/.
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/changelog/formatter.ts:
- Around line 341-360: When needsShell is true the code builds a single command
string using commandPath and the decoded prompt which allows shell injection via
unescaped quotes; change the generation to safely escape the prompt before
concatenation (e.g., call shlex.quote(prompt) in the Python snippet) or, better,
avoid shell mode entirely by passing arguments as a list to subprocess.run;
update the template that emits the shell-True branch (the string using command =
${commandPath} + ' -p "' + prompt + '" ...' and the subsequent subprocess.run
call with shell=True) to instead escape prompt with shlex.quote or emit a
non-shell list form, ensuring subprocess.run is invoked with properly
quoted/escaped prompt and no raw user input is injected into a shell.
In @apps/frontend/src/main/changelog/version-suggester.ts:
- Around line 128-134: The createAnalysisScript method currently only escapes
Python string literals (escapedPrompt) but does not neutralize cmd.exe
metacharacters when preparePythonSubprocessCommand returns needsShell=true,
allowing shell injection; update createAnalysisScript to avoid inserting raw
prompt into a shell command by encoding the prompt (e.g., base64 as used in
formatter.ts) and passing the encoded payload to the Python subprocess, or if
shell invocation cannot be avoided, explicitly escape cmd.exe-specific
metacharacters (`%`, `^`, `&`, `|`, `<`, `>`, etc.) before concatenation; ensure
you reference createAnalysisScript, escapedPrompt, and the
preparePythonSubprocessCommand / needsShell result when making the change.
In @apps/frontend/src/main/env-utils.ts:
- Around line 475-497: The isWindowsBatchFile check in
preparePythonSubprocessCommand uses case-sensitive endsWith(".cmd")/".bat" so
uppercase extensions like ".CMD" or ".Bat" won't be detected; update that check
to perform a case-insensitive test (e.g., use executablePath.toLowerCase() or a
case-insensitive regex) when computing isWindowsBatchFile (and/or normalize the
extension first) so shell mode is enabled for any case variation of .cmd/.bat.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
apps/frontend/src/main/changelog/formatter.tsapps/frontend/src/main/changelog/version-suggester.tsapps/frontend/src/main/cli-tool-manager.tsapps/frontend/src/main/env-utils.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/main/cli-tool-manager.tsapps/frontend/src/main/env-utils.tsapps/frontend/src/main/changelog/version-suggester.tsapps/frontend/src/main/changelog/formatter.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/main/cli-tool-manager.tsapps/frontend/src/main/env-utils.tsapps/frontend/src/main/changelog/version-suggester.tsapps/frontend/src/main/changelog/formatter.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/cli-tool-manager.tsapps/frontend/src/main/env-utils.tsapps/frontend/src/main/changelog/version-suggester.tsapps/frontend/src/main/changelog/formatter.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
🧬 Code graph analysis (2)
apps/frontend/src/main/changelog/version-suggester.ts (1)
apps/frontend/src/main/env-utils.ts (1)
preparePythonSubprocessCommand(475-497)
apps/frontend/src/main/changelog/formatter.ts (1)
apps/frontend/src/main/env-utils.ts (1)
preparePythonSubprocessCommand(475-497)
⏰ 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). (2)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (5)
apps/frontend/src/main/cli-tool-manager.ts (1)
1039-1066: Consistent fix applied to async validation.Good consistency—the async version mirrors the sync version's fix at lines 941-948, ensuring Windows path spacing issues are handled correctly in both code paths.
apps/frontend/src/main/env-utils.ts (2)
518-535: LGTM - Clean implementation of shell command preparation.The
prepareShellCommandfunction correctly handles the Windows batch file detection and quoting logic for Node.js child_process usage. The separation frompreparePythonSubprocessCommandkeeps concerns clear.
29-36: LGTM - Non-blocking filesystem existence check.The
existsAsynchelper correctly usesfs.promises.accessfor async path existence checking, which is appropriate for the Electron main process to avoid blocking the event loop.apps/frontend/src/main/changelog/formatter.ts (1)
1-6: LGTM - Import organization and utility integration.The import of
preparePythonSubprocessCommandfromenv-utilscentralizes the Windows path handling logic appropriately, reducing code duplication across the codebase.apps/frontend/src/main/changelog/version-suggester.ts (1)
1-12: LGTM - Import organization and type definition updates.The integration of
preparePythonSubprocessCommandfromenv-utilscentralizes the Windows path handling logic. The quote style normalization is consistent with the rest of the changes.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🟠 NEEDS REVISION
🟠 Needs revision - 2 issue(s) require attention.
2 issue(s) must be addressed (0 required, 2 recommended), 3 suggestions
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | High | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
Findings Summary
- Medium: 2 issue(s)
- Low: 3 issue(s)
Generated by Auto Claude PR Review
Findings (5 selected of 5 total)
🟡 [9264ea3630b5] [MEDIUM] Paths with shell metacharacters (like &) but no spaces are not quoted
📁 apps/frontend/src/main/env-utils.ts:487
The preparePythonSubprocessCommand function only adds quotes when the path contains spaces. However, Windows allows & in file names (e.g., C:\Dev&Test\claude.cmd). When passed to shell execution, unquoted & is interpreted as a command separator by cmd.exe, causing the command to fail or execute partial commands.
Suggested fix:
Either always quote paths in shell mode (safest), or add shell metacharacter detection: `const needsQuoting = needsShell && (executablePath.includes(' ') || /[&|<>^%()]/.test(executablePath)) && !isAlreadyQuoted;`
🟡 [77cd54876f28] [MEDIUM] prepareShellCommand has same shell metacharacter quoting gap
📁 apps/frontend/src/main/env-utils.ts:531
The prepareShellCommand function uses identical quoting logic - only quoting paths that contain spaces. Paths with shell metacharacters like & but no spaces will not be quoted, leading to command parsing issues when passed to execFileSync with shell: true.
Suggested fix:
Apply same fix as preparePythonSubprocessCommand: detect shell metacharacters in addition to spaces when deciding whether to quote.
🔵 [2d38eb759f20] [LOW] Missing input validation for empty/undefined paths
📁 apps/frontend/src/main/env-utils.ts:475
The preparePythonSubprocessCommand function does not validate input. If executablePath is an empty string, it returns { commandPath: '', needsShell: false } which could lead to confusing errors downstream. A null/undefined input would throw a cryptic error at executablePath.toLowerCase() without a helpful message.
Suggested fix:
Add input validation: `if (!executablePath || typeof executablePath !== 'string') { throw new Error('preparePythonSubprocessCommand: executablePath is required'); }`
🔵 [99ec10d5b49a] [LOW] prepareShellCommand also lacks input validation
📁 apps/frontend/src/main/env-utils.ts:519
The prepareShellCommand function has the same validation issue - no check for empty string or null/undefined input. Will return invalid results or throw cryptic errors.
Suggested fix:
Add same input validation as suggested for preparePythonSubprocessCommand.
🔵 [f56c304b8c92] [LOW] Missing unit tests for new path utility functions
📁 apps/frontend/src/main/__tests__/cli-tool-manager.test.ts:0
The new utility functions preparePythonSubprocessCommand and prepareShellCommand have no dedicated unit tests. The test file only mocks these functions. These functions handle security-sensitive Windows path quoting logic that should be tested with various patterns: paths with spaces, already-quoted paths, .cmd/.bat extensions (case variations), backslashes, etc.
Suggested fix:
Add a new test file `apps/frontend/src/main/__tests__/env-utils.test.ts` with test cases for path handling edge cases including spaces, shell metacharacters, already-quoted paths, and case-insensitive extension matching.
This review was generated by Auto Claude.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/__tests__/cli-tool-manager.test.ts:
- Around line 35-41: Remove the unused test helper function mockDirent (the
const mockDirent = (name: string, isDir: boolean): { name: string; isDirectory:
() => boolean } => ({ name, isDirectory: () => isDir }) declaration) from the
test file since it is not referenced; alternatively, if you intend to keep it
for future use, rename it to _mockDirent to avoid static-analysis unused
warnings.
In @apps/frontend/src/main/changelog/formatter.ts:
- Line 334: Move the unconditional import of shlex into the branch where it's
used: only import shlex when needsShell is true before calling shlex.quote();
update the code around the needsShell conditional in formatter.ts so shlex is
imported locally in that branch (referencing shlex and the needsShell check) to
avoid the unnecessary import in list mode.
In @apps/frontend/src/main/env-utils.ts:
- Around line 475-498: The current escaping logic in
preparePythonSubprocessCommand is correct but needs clearer inline
documentation; update the JSDoc for preparePythonSubprocessCommand to include a
concrete example showing input -> intermediate steps (add quotes for spaces,
escape backslashes and inner quotes for Python string) and the final commandPath
and needsShell values (use the Windows example "C:\Users\Jane
Smith\AppData\Roaming\npm\claude.cmd"), showing the exact transformed string
value for commandPath (the Python-escaped quoted path) and a short note how it
will be used in the generated Python subprocess call; keep the example
consistent with the existing comment style and ensure the shown commandPath
matches the current escaping logic in the function.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
apps/frontend/src/main/__tests__/cli-tool-manager.test.tsapps/frontend/src/main/changelog/formatter.tsapps/frontend/src/main/changelog/version-suggester.tsapps/frontend/src/main/env-utils.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/main/__tests__/cli-tool-manager.test.tsapps/frontend/src/main/changelog/version-suggester.tsapps/frontend/src/main/env-utils.tsapps/frontend/src/main/changelog/formatter.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/main/__tests__/cli-tool-manager.test.tsapps/frontend/src/main/changelog/version-suggester.tsapps/frontend/src/main/env-utils.tsapps/frontend/src/main/changelog/formatter.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/__tests__/cli-tool-manager.test.tsapps/frontend/src/main/changelog/version-suggester.tsapps/frontend/src/main/env-utils.tsapps/frontend/src/main/changelog/formatter.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
🧬 Code graph analysis (3)
apps/frontend/src/main/__tests__/cli-tool-manager.test.ts (1)
apps/frontend/src/main/cli-tool-manager.ts (3)
getClaudeDetectionPaths(169-189)sortNvmVersionDirs(208-229)buildClaudeDetectionResult(252-268)
apps/frontend/src/main/changelog/version-suggester.ts (1)
apps/frontend/src/main/env-utils.ts (1)
preparePythonSubprocessCommand(475-498)
apps/frontend/src/main/changelog/formatter.ts (1)
apps/frontend/src/main/env-utils.ts (1)
preparePythonSubprocessCommand(475-498)
🪛 GitHub Check: test-frontend
apps/frontend/src/main/__tests__/cli-tool-manager.test.ts
[warning] 35-35:
'mockDirent' is assigned a value but never used. Allowed unused vars must match /^_/u
[warning] 318-318:
Unexpected any. Specify a different type
[warning] 266-266:
Unexpected any. Specify a different type
[warning] 195-195:
Unexpected any. Specify a different type
[warning] 193-193:
Unexpected any. Specify a different type
[warning] 159-159:
Unexpected any. Specify a different type
[warning] 157-157:
Unexpected any. Specify a different type
[warning] 121-121:
Unexpected any. Specify a different type
[warning] 119-119:
Unexpected any. Specify a different type
⏰ 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). (2)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (5)
apps/frontend/src/main/__tests__/cli-tool-manager.test.ts (1)
56-70: LGTM: Mock setup supports test scenarios correctly.The mocks for
prepareShellCommandandpreparePythonSubprocessCommandappropriately returnneedsShell: falsefor these NVM detection tests, which don't require exercising Windows-specific shell behavior. This allows the tests to focus on path detection logic without Windows quoting complexity.apps/frontend/src/main/env-utils.ts (2)
519-537: LGTM: Simpler shell command preparation for Node.js.The
prepareShellCommand()function correctly handles path quoting for Node.jschild_processfunctions. The logic is appropriately simpler thanpreparePythonSubprocessCommand()since Node.js handles shell escaping internally whenshell: trueis used.
476-481: Security: Proper case-insensitive extension checks for Windows.The case-insensitive extension checks (
.toLowerCase()) correctly handle Windows filesystem semantics whereclaude.CMD,claude.cmd, andclaude.Cmdare all equivalent. This prevents bypasses where mixed-case extensions might not trigger shell mode when needed.apps/frontend/src/main/changelog/formatter.ts (1)
325-326: LGTM: Strong shell injection protections.The implementation uses multiple layers of protection against shell injection:
- Base64 encoding (line 325): Prevents special character issues during script generation
shlex.quote()(line 346): Safely escapes the prompt for shell execution in Windows.cmd/.batmode- List mode (line 357): Uses array syntax with
subprocess.run()which bypasses shell entirely for.exefilesThese defenses align with the PR's security hardening objectives.
Also applies to: 338-338, 346-346
apps/frontend/src/main/changelog/version-suggester.ts (1)
128-173: LGTM: Correct and secure Claude CLI invocation.The
createAnalysisScript()implementation correctly:
- Base64 encodes the prompt (line 130) to avoid escaping issues
- Uses
preparePythonSubprocessCommand()(line 133) for proper Windows path handling- Conditionally imports
shlex(line 148) only when needed for shell mode - more efficient than the approach informatter.ts- Applies
shlex.quote()(line 149) to prevent shell injection in Windows.cmd/.batexecution- Uses list syntax (line 159) for non-shell mode to bypass shell entirely
The security measures align with the PR's hardening objectives.
| import subprocess | ||
| import sys | ||
| import base64 | ||
| import shlex |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: shlex imported unconditionally but only used in shell mode.
The import shlex statement at line 334 is in the base Python script, but shlex.quote() is only used when needsShell is true (line 346). This doesn't cause errors but adds a small unnecessary import in list mode.
⚡ Optional optimization
Move the import shlex into the shell mode branch:
import subprocess
import sys
import base64
-import shlex
try:
# Decode the base64 prompt to avoid string escaping issues
prompt = base64.b64decode('${base64Prompt}').decode('utf-8')
# Use Claude Code CLI to generate
# stdin=DEVNULL prevents hanging when claude checks for interactive input
${
needsShell
- ? `# On Windows with .cmd/.bat files, use shell=True with properly escaped prompt
+ ? `# On Windows with .cmd/.bat files, use shell=True with properly escaped prompt
+ import shlex
# Use shlex.quote to safely escape the prompt for shell
escaped_prompt = shlex.quote(prompt)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @apps/frontend/src/main/changelog/formatter.ts at line 334, Move the
unconditional import of shlex into the branch where it's used: only import shlex
when needsShell is true before calling shlex.quote(); update the code around the
needsShell conditional in formatter.ts so shlex is imported locally in that
branch (referencing shlex and the needsShell check) to avoid the unnecessary
import in list mode.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/env-utils.ts:
- Around line 531-559: Update the function documentation for prepareShellCommand
to explicitly state that needsShell is true only when shell mode is required for
Windows batch files (.cmd/.bat), that the function only adds quotes when it
determines needsShell is true (so .exe or other paths with spaces will not be
quoted), and that callers who set shell: true for other reasons must perform
their own quoting/escaping of executablePath; reference the prepareShellCommand
signature and the returned { command, needsShell } values so readers know which
symbols to rely on.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
apps/frontend/src/main/__tests__/env-utils.test.tsapps/frontend/src/main/env-utils.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/main/env-utils.tsapps/frontend/src/main/__tests__/env-utils.test.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/main/env-utils.tsapps/frontend/src/main/__tests__/env-utils.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/env-utils.tsapps/frontend/src/main/__tests__/env-utils.test.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
🧬 Code graph analysis (1)
apps/frontend/src/main/__tests__/env-utils.test.ts (1)
apps/frontend/src/main/env-utils.ts (2)
preparePythonSubprocessCommand(475-510)prepareShellCommand(531-559)
⏰ 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). (2)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (8)
apps/frontend/src/main/env-utils.ts (3)
12-36: LGTM - Clean async utilities.The additions of
fsPromises,execFile, and theexistsAsynchelper follow Node.js best practices for non-blocking I/O operations.
56-65: LGTM - Correct Windows npm handling.The use of
npm.cmdwithshell: trueon Windows is correct and necessary for proper npm command resolution. The async version (lines 305-316) follows the same pattern appropriately.Also applies to: 305-316
505-507: The escaping logic in lines 505-507 is correct for the intended use cases and does not require changes.In non-shell mode (line 507), the
commandPathis inserted directly into a Python list literal with single quotes (e.g.,['${commandPath}', '-p', prompt, ...]). Single-quote escaping is necessary to prevent syntax errors when paths contain apostrophes (e.g.,C:\O'Reilly\claude.exemust becomeC:\\O\'Reilly\\claude.exe).In shell mode (line 506), the multi-layered quoting correctly handles paths that need both internal quotes (for spaces/metacharacters) and external wrapping (for Python string representation).
Both escaping strategies align with how the generated Python code actually uses
commandPath.Likely an incorrect or invalid review comment.
apps/frontend/src/main/__tests__/env-utils.test.ts (5)
26-48: LGTM - Thorough input validation tests.The input validation tests comprehensively cover null, undefined, empty string, and non-string inputs for both functions. Good defensive testing.
Also applies to: 245-267
50-87: LGTM - Comprehensive batch file detection tests.Case-insensitive extension checking for
.cmdand.batfiles is properly tested across lowercase, uppercase, and mixed case. This correctly validates Windows file extension behavior.Also applies to: 269-296
312-316: Test documents edge case behavior correctly.This test validates that paths with spaces are not quoted when
needsShell=false(e.g.,.exefiles on Windows). While correct, this represents an edge case where the caller would need to handle quoting separately if they choose to use shell mode for non-batch files.The test accurately documents the current behavior: quoting is only applied when shell mode is required (for
.cmd/.batfiles). This aligns with the implementation but may benefit from a comment explaining that this is expected behavior for the "shell required" use case.
141-231: LGTM - Excellent security and escaping coverage.The shell metacharacter tests comprehensively cover all Windows
cmd.exespecial characters (&|<>^%()), which is crucial for preventing command injection. The Python string escaping tests verify the complex transformation logic for backslashes and quotes. Well done.
89-106: LGTM - Platform-specific behavior correctly tested.The tests properly validate that shell mode requirements and
.cmd/.bathandling are Windows-specific. The Linux tests confirm that these files are treated as regular executables on non-Windows platforms, which is correct.Also applies to: 357-376
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/frontend/src/main/env-utils.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/main/env-utils.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/main/env-utils.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/env-utils.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). (2)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (3)
apps/frontend/src/main/env-utils.ts (3)
12-434: LGTM: Well-structured environment utilities with comprehensive PATH handling.The environment augmentation logic is thorough and properly handles:
- Platform-specific binary paths with home directory expansion
- Essential system paths for macOS Keychain access
- npm global prefix detection with appropriate caching
- Async alternatives to avoid blocking the event loop
- Correct use of
npm.cmdwithshell: trueon WindowsThe foundation is solid for the new subprocess command preparation utilities.
578-606: LGTM: Clear and well-documented shell command preparation.The implementation correctly:
- Sets
needsShell=trueonly for Windows.cmd/.batfiles (case-insensitive)- Quotes paths only when
needsShell=trueAND the path contains spaces or shell metacharacters- Leaves
.exefiles unquoted (documented in JSDoc examples)The JSDoc documentation (lines 532-577) is excellent, with three concrete examples that clarify when quoting occurs and when callers must handle their own quoting. This prevents misuse of the function.
475-510: Escaping logic correctly handles both shell and non-shell command execution.The implementation properly handles:
- Windows batch file detection (case-insensitive)
- Shell metacharacter detection (all relevant cmd.exe special characters)
- Conditional quoting based on spaces and metacharacters
- Correct escaping for Python string representation in both execution modes
Shell mode verification (version-suggester.ts):
commandPathis concatenated into a command string:command = ${commandPath} + ' chat --model ... '- Escaping produces
"\"C:\\Users\\Jane Smith\\npm\\claude.cmd\""which evaluates to"C:\Users\Jane Smith\npm\claude.cmd"in Python- When passed to
subprocess.run(command, shell=True), cmd.exe correctly interprets the quoted path with spacesNon-shell mode verification (formatter.ts):
commandPathis used in a subprocess argument list:['${commandPath}', '-p', prompt, ...]- Escaping produces
C:\\Users\\Jane Smith\\npm\\claude.exewith single quotes escaped- The single quotes in the template are Python syntax; the string value correctly contains escaped backslashes and quotes
- Subprocess list-mode execution correctly handles the unquoted path with proper escaping
Minor edge case: Line 491 checks if a path is already quoted by testing
startsWith('"') && endsWith('"'). This would incorrectly identify a malformed path like"C:\path"internal.cmd"as already quoted, but this scenario is unlikely for CLI tool paths and does not affect the actual usage.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/env-utils.ts:
- Around line 523-543: The metacharacter detection in analyzePathQuoting is
missing the double-quote, so update its regex (the one currently matching
/[&|<>^%()]/) to also include the " character so paths containing embedded
quotes are treated as needing quoting; then add a unit test covering a path with
an embedded double-quote (e.g., a path like C:\Dev"Test\file.cmd) to verify
preparePythonSubprocessCommand returns needsShell/commandPath that properly
quotes/escapes the quote edge case.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/frontend/src/main/env-utils.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/main/env-utils.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/main/env-utils.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/env-utils.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). (2)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (14)
apps/frontend/src/main/env-utils.ts (14)
12-17: LGTM: Import consolidation and style consistency.The import statements now consistently use double quotes and include the necessary Node.js built-in modules for async filesystem operations and subprocess execution.
29-36: LGTM: Non-blocking path existence check.The
existsAsyncfunction properly usesfs.promises.accessto avoid blocking the Electron main process event loop.
53-82: LGTM: Windows .cmd handling for npm global prefix detection.The function correctly uses
npm.cmdwithshell: trueon Windows, which is essential for executing.cmdfiles. The--location=globalflag avoids workspace context errors in monorepos.Note: This is a synchronous operation that may block briefly. Ensure it's only called during initialization or use the async version (
getNpmGlobalPrefixAsync) for performance-critical paths.
88-117: LGTM: Comprehensive platform-specific PATH entries.The constants cover common tool installation locations across platforms and include essential system paths for macOS Keychain access. String literal style is now consistent.
128-180: LGTM: Clean path expansion and filtering logic.The helper functions properly expand home directory references and filter paths based on existence and current PATH contents. The use of Sets prevents duplicates efficiently.
192-237: LGTM: Robust PATH augmentation logic.The function correctly handles PATH augmentation across platforms, ensuring essential system paths are present and new paths take priority. The synchronous approach is acceptable for the use case.
248-277: LGTM: Executable search with correct Windows extension priority.The function properly prioritizes Windows-native extensions (
.exe,.cmd,.bat) before checking extensionless files, which is important for avoiding Git Bash/Cygwin script confusion.
291-424: LGTM: Non-blocking async alternatives with proper caching.The async versions properly use non-blocking operations and implement caching to avoid repeated subprocess calls. The dual-cache approach (value + promise) correctly prevents concurrent duplicate calls.
The caching logic is complex but correct. Consider adding unit tests to verify cache invalidation scenarios if not already present.
545-624: LGTM: Clear shell command preparation with important usage caveat.The function correctly prepares paths for Node.js
child_processfunctions, distinguishing between Windows batch files (.cmd/.bat) that require shell mode and other executables that don't.Critical documentation note (lines 558-560): Callers who enable
shell: truefor reasons other than.cmd/.batfiles (e.g., environment variable expansion, custom shell scripts) MUST perform their own quoting. The function returnsneedsShell=falsefor non-batch files, so the command will not be quoted even if it contains spaces.The examples clearly illustrate this distinction. Good defensive documentation.
525-529: LGTM: Proper input validation.Both preparation functions validate that
executablePathis a non-empty string and throw descriptive errors. This prevents cryptic failures downstream.
445-452: LGTM: Well-defined TypeScript interfaces.The interfaces are properly structured with clear documentation:
QuotingDecision(internal): encapsulates quoting analysisPythonSubprocessCommand(public): for Python subprocess preparationShellCommandResult(public): for Node.js child_process preparationGood type safety and API clarity.
Also applies to: 488-496, 545-563
469-471: LGTM: Case-insensitive extension check for Windows.The lowercase conversion ensures that
.CMD,.Cmd,.cmd, etc., are all correctly identified as batch files requiring shell mode. This is a security hardening measure mentioned in the PR objectives.
1-624: Overall assessment: Well-structured utility module with comprehensive Windows path handling.This file successfully addresses the PR objectives by:
✅ Implementing Windows path quoting for
.cmd/.batfiles
✅ Providing both sync and async versions for Electron compatibility
✅ Extracting shared quoting logic to reduce duplication
✅ Using case-insensitive extension checks for security hardening
✅ Including comprehensive documentation with examples
✅ Proper TypeScript type safetyStrengths:
- Clear separation between Python subprocess and Node.js child_process preparation
- Non-blocking async alternatives with proper caching
- Defensive input validation
- Comprehensive JSDoc with examples
Recommendations:
- Ensure thorough test coverage for edge cases (especially paths with literal quotes or multiple special characters)
- Consider documenting known limitations if any exist for unusual path characters
- Verify shell metacharacter regex covers all necessary cases for your usage patterns
The code quality is high and the implementation is production-ready.
466-486: The metacharacter coverage is correct as-is; the suggested characters are not special in Windows cmd.exe.The current regex
[&|<>^%()]correctly identifies all Windows cmd.exe special characters. The suggested additions (backtick,!,=, comma, semicolon) are not special metacharacters in cmd.exe:
`(backtick) — special only in PowerShell/Unix shells, not cmd.exe!— only special with delayed expansion (non-default cmd.exe behavior)=— not special in cmd.exe paths,— not special in cmd.exe paths;— not a command separator in cmd.exe (only in Unix shells)The test suite (lines 149–195 of env-utils.test.ts) comprehensively covers all eight current metacharacters individually, with no gaps in coverage. The implementation is correct.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/__tests__/env-utils.test.ts:
- Around line 227-237: The test for already-quoted paths should assert the full
transformed string, not just backslashes; update the "should escape quotes for
Python string in shell mode" test that calls
preparePythonSubprocessCommand(quotedPath) (where quotedPath =
'"C:\\Test\\claude.cmd"') to add an assertion that result.commandPath equals the
exact expected escaped form (i.e., quotes preserved and escaped and backslashes
doubled for Python subprocess usage) in addition to the existing toMatch check
so the test clearly documents the precise output format produced by
preparePythonSubprocessCommand.
In @apps/frontend/src/main/env-utils.ts:
- Around line 523-543: The function preparePythonSubprocessCommand currently
returns a fully quoted Python string literal for shell mode but an unquoted
escaped path for list mode; make the behavior consistent by returning complete
Python string literals in both modes: for needsShell true keep the existing
double-quoted literal behavior, and for needsShell false wrap the escaped path
in single quotes (escaping any single quotes/backslashes appropriately) so
callers can embed the returned commandPath directly into generated Python code;
update the implementation that builds commandPath (and adjust any comments) and
keep analyzePathQuoting, needsShell and commandPath identifiers intact so
callers see a uniform returned Python string literal.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
apps/frontend/src/main/__tests__/env-utils.test.tsapps/frontend/src/main/env-utils.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/main/env-utils.tsapps/frontend/src/main/__tests__/env-utils.test.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/main/env-utils.tsapps/frontend/src/main/__tests__/env-utils.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/env-utils.tsapps/frontend/src/main/__tests__/env-utils.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). (2)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (7)
apps/frontend/src/main/env-utils.ts (3)
12-17: LGTM! Clean imports for the new utilities.The imports are appropriate for the environment utilities and subprocess handling functionality.
466-486: Shell metacharacter detection looks robust.The regex at line 479 correctly includes
"(double-quote) as a shell metacharacter, which is important for Windows cmd.exe. The case-insensitive extension check viatoLowerCase()properly handles.CMD,.Cmd,.bat, etc.
611-624: Excellent documentation and clear implementation.The
prepareShellCommandfunction is well-documented with multiple examples covering different scenarios. The logic correctly handles Windows batch files while avoiding unnecessary quoting for other file types.apps/frontend/src/main/__tests__/env-utils.test.ts (4)
1-24: LGTM! Well-structured test setup.The test file follows best practices:
- Saves and restores
process.platformin afterEach to prevent test pollution- Uses vitest's describe/it structure effectively
- Imports all necessary types and functions
50-87: Excellent coverage of case-insensitive extension detection.The tests properly verify that
.cmd,.CMD,.Cmd,.bat, and.BATall trigger shell mode on Windows, while.exefiles do not. This aligns with the Windows file system's case-insensitive nature.
141-211: Comprehensive metacharacter testing, including embedded quotes.The test suite covers all Windows cmd.exe metacharacters (
&,|,<,>,^,%,(,),"). The embedded double-quote test at line 197-202 is particularly important for preventing shell injection vulnerabilities.
371-390: Good cross-platform test coverage.The Unix/Linux tests correctly verify that:
- Regular files don't require shell mode
- Paths with spaces remain unquoted when
needsShell=falseThis ensures the Windows-specific quoting logic doesn't affect other platforms.
StillKnotKnown
added
size/XL
- Remove duplicate mock from beforeEach block (was using setImmediate with type issues) - Keep global mock using setTimeout which has better type compatibility - All useXterm tests (20) and cli-tool-manager tests (28) pass
apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts
Fixed
Show fixed
Hide fixed
StillKnotKnown
left a comment
StillKnotKnown
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 3 blocking issue(s) require fixes.
Resolution Status
- ✅ Resolved: 1 previous findings addressed
- ❌ Unresolved: 0 previous findings remain
- 🆕 New Issues: 5 new findings in recent changes
🚨 Blocking Issues
- security: Base64-encoded prompt embedded in Python script without claudePath validation
- security: Base64 prompt embedding vulnerability in version suggester
- security: preparePythonSubprocessCommand escapes paths but doesn't validate source security
Verdict
All 16 CI checks passing (✅), but 3 CONFIRMED security findings block merge:
HIGH Severity (2 findings):
- NEW-001, NEW-002: Base64-encoded prompts embedded in Python scripts without claudePath validation. The changelog generation code (formatter.ts:338, version-suggester.ts:141) uses user-configured claudePath from Settings in shell commands with shell=True, but doesn't validate the path with isSecurePath() before embedding it. While shlex.quote() protects the prompt argument, the claudePath itself could contain shell metacharacters if a malicious path is configured, creating a command injection vector.
MEDIUM Severity (1 finding):
- NEW-003: preparePythonSubprocessCommand() (env-utils.ts:538) lacks security validation. This function is now used in the changelog code paths but doesn't validate the input path is secure, creating a gap in defense-in-depth protection.
Why this matters: The CLI tool manager validates user-configured paths with isSecurePath() before using them, but the changelog generation code doesn't perform this validation. User-configured paths from Settings flow directly to subprocess commands with shell=True without adequate validation.
LOW Severity (2 findings) - Not blocking:
- NEW-004, NEW-005: Quality issues in test architecture and metacharacter detection.
Fix: Add isSecurePath() validation in changelog generation code paths before using claudePath, or add validation in preparePythonSubprocessCommand() itself.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (5 selected of 5 total)
🟠 [NEW-001] [HIGH] Base64-encoded prompt embedded in Python script without claudePath validation
📁 apps/frontend/src/main/changelog/formatter.ts:338
In createGenerationScript(), the base64-encoded prompt is embedded into a Python script that constructs shell commands. When needsShell=true, the command uses shell=True with commandPath derived from claudePath. While shlex.quote() is used on the decoded prompt, the claudePath itself (which can come from user Settings) is not validated with isSecurePath() before being embedded. This creates a potential command injection vector if a malicious path is configured.
Suggested fix:
Add isSecurePath() validation for claudePath before creating the generation script. The VersionSuggester and changelog generators should validate user-configured paths just like cli-tool-manager.ts does.
🟠 [NEW-002] [HIGH] Base64 prompt embedding vulnerability in version suggester
📁 apps/frontend/src/main/changelog/version-suggester.ts:141
The createAnalysisScript() method has the same vulnerability pattern as createGenerationScript() - embedding base64-encoded prompt and using claudePath with shell=True without validating the path is secure. The claudePath comes from the constructor which can be user-configured in Settings.
Suggested fix:
Add isSecurePath() validation for claudePath in VersionSuggester constructor or before creating the analysis script.
🟡 [NEW-003] [MEDIUM] preparePythonSubprocessCommand escapes paths but doesn't validate source security
📁 apps/frontend/src/main/env-utils.ts:538
The preparePythonSubprocessCommand() function properly escapes paths for Python string representation, but it doesn't perform security validation on the input path. This function is now used in changelog generation code paths where claudePath from user Settings flows directly to shell commands. Without validation here or at call sites, user-configured paths containing shell metacharacters could lead to command injection.
Suggested fix:
Add isSecurePath() validation at the start of preparePythonSubprocessCommand() and throw an error if the path contains dangerous patterns. This provides defense-in-depth even if callers should validate.
🔵 [NEW-004] [LOW] Test mock architecture fragility with shared execSync/execFileSync mock
📁 apps/frontend/src/main/__tests__/cli-tool-manager.test.ts:46
The mock architecture where execSync and execFileSync share the same mock function (sharedSyncMock) creates test fragility. Tests that set one will affect both because validateClaude() uses execSync for .cmd files and execFileSync for others. While the comment documents this design necessity, it's a code smell that could cause confusing test failures.
Suggested fix:
Consider documenting more clearly why this sharing is necessary. Could add a helper function like vi.mocked(execSyncOrExecFileSync) to make the intent clearer. Refactor tests if possible to avoid this coupling.
🔵 [NEW-005] [LOW] analyzePathQuoting has incomplete metacharacter detection for Windows cmd.exe
📁 apps/frontend/src/main/env-utils.ts:466
The analyzePathQuoting() function checks for shell metacharacters with /[&|<>^%()]/ but doesn't cover all possible cmd.exe metacharacters. Notably missing is ! (delayed expansion character) which can be problematic in batch files. The current set covers most common cases but is not exhaustive.
Suggested fix:
Consider adding ! and other edge case metacharacters. Document that the check is not exhaustive and covers the most common dangerous characters. Consider whether exhaustive coverage is practical or if code should just avoid shell=True entirely.
This review was generated by Auto Claude.
Fixes 3 HIGH/MEDIUM severity security issues: - NEW-001 (HIGH): Add validation in formatter.ts createGenerationScript() - NEW-002 (HIGH): Add validation in version-suggester.ts constructor - NEW-003 (MEDIUM): Add validation in preparePythonSubprocessCommand() Also fixes: - Remove unused renderHook import from useXterm.test.ts The changelog generation code now validates claudePath before embedding it into shell commands, preventing command injection via malicious user-configured paths. All 1373 tests pass.
There was a problem hiding this comment.
Actionable comments posted: 5
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/renderer/components/terminal/__tests__/useXterm.test.ts (1)
74-152: AvoidkeyEventHandler!; fail fast with a clear error if the hook stops attaching the handler.
Non-null assertions here turn “handler not registered” into a less actionable runtime failure later.Proposed fix (explicit guard)
// After rendering, keyEventHandler is guaranteed to be set by attachCustomKeyEventHandler - // Use non-null assertion since we know the hook will set it + // Fail fast with a clear error if the hook stops attaching the handler + if (!keyEventHandler) { + throw new Error("useXterm did not attach a custom key event handler"); + } return { - keyEventHandler: keyEventHandler!, + keyEventHandler, mockInstance: { hasSelection: overrides.hasSelection, getSelection: overrides.getSelection, paste: overrides.paste, input: overrides.input, }, }; }
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/env-utils.ts:
- Around line 619-632: Add the same security validation used in
preparePythonSubprocessCommand: call isSecurePath(executablePath) after the
existing type check in prepareShellCommand and throw an Error (or return a clear
failure) if it returns false; keep the analyzePathQuoting logic and subsequent
quoting behavior unchanged. Ensure you reference prepareShellCommand and
isSecurePath so reviewers can find the change, and preserve the returned
ShellCommandResult shape ({ command, needsShell }) while enforcing the
secure-path check before building the command.
- Around line 546-548: The code double-quotes paths that are already quoted
because preparePythonSubprocessCommand destructures needsShell and needsQuoting
but ignores isAlreadyQuoted returned by analyzePathQuoting; update
preparePythonSubprocessCommand to also read isAlreadyQuoted and, when needsShell
is true, avoid adding outer quotes if isAlreadyQuoted is true (or alternatively
strip surrounding quotes from quotedPath before applying your escape/re-quoting
logic), ensuring quotedPath is not wrapped twice when building commandPath.
In @apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts:
- Around line 190-197: The tests override navigator.platform by calling
Object.defineProperty directly which can create a non-configurable property and
poison the worker; capture the original property descriptor once (e.g., const
originalNavigatorPlatformDescriptor = Object.getOwnPropertyDescriptor(navigator,
"platform")), replace direct defineProperty calls with a small helper (e.g.,
setNavigatorPlatform(value, { configurable: true, writable: true }) ) that
preserves configurability, and in afterEach restore the full original descriptor
via Object.defineProperty(navigator, "platform",
originalNavigatorPlatformDescriptor) so all occurrences (tests around lines
referenced) use the helper and restore the original descriptor instead of only
restoring value.
- Around line 64-69: Save the original global shims
(global.requestAnimationFrame, global.cancelAnimationFrame, and
global.ResizeObserver) before mocking them in the test file, replace them with
the vi.fn mocks as shown, and restore the originals in an afterAll hook to avoid
cross-test leakage; specifically, capture originals into locals (e.g., const
origRaf = global.requestAnimationFrame, origCancel =
global.cancelAnimationFrame, origRO = global.ResizeObserver), set your mocks
(using vi.fn(...) and the existing setTimeout/clearTimeout wrappers), and in
afterAll restore globals from those locals and clear any vi mocks.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
apps/frontend/src/main/changelog/formatter.tsapps/frontend/src/main/changelog/version-suggester.tsapps/frontend/src/main/env-utils.tsapps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/main/changelog/version-suggester.tsapps/frontend/src/main/changelog/formatter.tsapps/frontend/src/main/env-utils.tsapps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/main/changelog/version-suggester.tsapps/frontend/src/main/changelog/formatter.tsapps/frontend/src/main/env-utils.tsapps/frontend/src/renderer/components/terminal/__tests__/useXterm.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/changelog/version-suggester.tsapps/frontend/src/main/changelog/formatter.tsapps/frontend/src/main/env-utils.tsapps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts
🧬 Code graph analysis (3)
apps/frontend/src/main/changelog/version-suggester.ts (2)
apps/frontend/src/main/utils/windows-paths.ts (1)
isSecurePath(40-56)apps/frontend/src/main/env-utils.ts (1)
preparePythonSubprocessCommand(524-551)
apps/frontend/src/main/env-utils.ts (1)
apps/frontend/src/main/utils/windows-paths.ts (1)
isSecurePath(40-56)
apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts (1)
apps/frontend/src/renderer/components/terminal/useXterm.ts (1)
useXterm(34-389)
⏰ 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). (2)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (12)
apps/frontend/src/main/changelog/formatter.ts (2)
1-4: LGTM!The imports are correctly organized, bringing in the new security utilities (
isSecurePath) and subprocess preparation helper (preparePythonSubprocessCommand) needed for the security hardening in this file.
324-336: LGTM on security validation and command preparation.The early validation with
isSecurePathbefore any command construction is a solid defense-in-depth approach. Combined withpreparePythonSubprocessCommandfor proper quoting, this addresses the command injection risks when paths contain spaces or special characters.apps/frontend/src/main/changelog/version-suggester.ts (4)
28-33: LGTM on constructor validation.Early validation of
claudePathin the constructor ensures that invalid paths are rejected before any operations are attempted. This is consistent with the pattern used informatter.ts.
136-140: LGTM on base64 encoding for prompt safety.Base64 encoding the prompt eliminates string escaping issues and shell injection risks when passing complex text through the subprocess boundary.
109-129: LGTM on prompt construction.The prompt is well-structured with clear instructions and a defined JSON output format. The commit summary formatting is clean and maintainable.
151-171: Addressshlex.quote()Windows compatibility concern in bothversion-suggester.tsandformatter.ts.Both files use
shlex.quote()for shell escaping on Windows (lines 354/156 respectively), butshlex.quote()is designed for POSIX shells and may not correctly escape arguments for Windowscmd.exe. Additionally, verify CLI argument consistency:
version-suggester.ts:chat --model haiku --prompt [arg]formatter.ts:-p [arg] --output-format text --model haikuConfirm both invocation styles are valid for your target Claude CLI version. If these represent different command variations, clarify which should be used where.
apps/frontend/src/main/env-utils.ts (5)
30-37: LGTM!Clean async existence check using
fs.promises.access. This is the correct non-blocking approach for the Electron main process.
467-487: LGTM on path quoting analysis.The case-insensitive extension check using
toLowerCase()correctly handles Windows file system behavior. The metacharacter detection for& | < > ^ % ( )covers the keycmd.exespecial characters.
524-551: LGTM onpreparePythonSubprocessCommand.The function correctly:
- Validates input type
- Performs security validation via
isSecurePath- Analyzes quoting requirements
- Escapes backslashes and quotes for Python string representation
The differentiated escaping for shell mode (double-quote escaping) vs list mode (single-quote escaping) is appropriate for the respective Python string contexts.
652-655: LGTM!Simple delegation to
prepareShellCommandkeeps the logic centralized.
673-695: LGTM ongetSpawnOptions.The function correctly merges user-provided options with the computed
shellrequirement. The spread operator ensures user options are preserved while the shell setting is determined by path analysis.apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts (1)
9-59: Mocks/imports look consistent; behavior-preserving edits are fine.
The conversion to double-quoted strings and the mock return tweaks (e.g.,getSelection: "",serialize: "") read as no-op behavior changes and should be stable.
| ${ | ||
| needsShell | ||
| ? `# On Windows with .cmd/.bat files, use shell=True with properly escaped prompt | ||
| # Use shlex.quote to safely escape the prompt for shell | ||
| escaped_prompt = shlex.quote(prompt) | ||
| command = ${commandPath} + ' -p ' + escaped_prompt + ' --output-format text --model haiku' | ||
| result = subprocess.run( | ||
| ['${escapedClaudePath}', '-p', prompt, '--output-format', 'text', '--model', 'haiku'], | ||
| command, | ||
| capture_output=True, | ||
| text=True, | ||
| stdin=subprocess.DEVNULL, | ||
| timeout=300, | ||
| shell=True | ||
| )` | ||
| : `result = subprocess.run( | ||
| ['${commandPath}', '-p', prompt, '--output-format', 'text', '--model', 'haiku'], | ||
| capture_output=True, | ||
| text=True, | ||
| stdin=subprocess.DEVNULL, | ||
| timeout=300 | ||
| ) | ||
| )` | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
shlex.quote Python Windows cmd.exe behavior single quotes
💡 Result:
Short answer: shlex.quote() produces POSIX-style single-quoted tokens (e.g. "'a b'") and is intended for Unix shells. cmd.exe does not treat single quotes as quoting characters (only double quotes are standard), so using shlex.quote() for commands executed by cmd.exe will leave the single-quote characters literal rather than grouping/escaping the argument. Use subprocess with a list (shell=False) or apply Windows/cmd quoting (double quotes and the cmd-specific escaping rules) instead. [1][2][3]
Sources:
- Python shlex.quote docs (warning about non-POSIX shells). [1]
- shlex.quote / tutorial notes (POSIX single-quote behavior; not for cmd/PowerShell). [2]
- Explanation of cmd.exe single-quote behavior (single quotes not used by cmd.exe; double quotes are). [3]
Replace shlex.quote() with Windows-aware escaping for shell=True on cmd.exe.
On Windows with shell=True, the command executes via cmd.exe, which does not recognize single quotes as escape characters (only double quotes). The shlex.quote() function produces POSIX-style single-quoted strings, leaving the quotes literal in cmd.exe rather than escaping the argument. This can cause incorrect command execution or potential command injection vulnerabilities.
Use platform-aware escaping (e.g., double quotes with cmd-specific escaping rules) or avoid shell=True entirely by passing arguments as a list to subprocess.run().
apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts
Show resolved
Hide resolved
…ble-quoting Test improvements: - Replace non-null assertion with explicit guard in useXterm.test.ts (fail fast with clear error if hook doesn't attach handler) - Add explicit return type annotation to setupMockXterm for TypeScript - Fix navigator.platform property descriptor handling (capture full descriptor, preserve configurability, restore properly) - Save and restore original global shims (RAF, cancelRAF, ResizeObserver) to avoid cross-test leakage Security fixes: - Add isSecurePath() validation to prepareShellCommand in env-utils.ts - Fix double-quoting issue in preparePythonSubprocessCommand (use isAlreadyQuoted to avoid double-wrapping quoted paths) All useXterm (20), cli-tool-manager (33), and env-handlers (3) tests pass.
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 (1)
apps/frontend/src/main/env-utils.ts (1)
668-711: Critical validation bug blocks legitimate paths with shell metacharacters.These helper functions correctly delegate to
prepareShellCommand(), which properly quotes paths when needed. However,prepareShellCommand()validates paths usingisSecurePath()(line 635), which rejects any path containing shell metacharacters like&, even though these characters are safely handled through quoting.This causes legitimate paths like
D:\Path&Name\tool.cmdto throw validation errors instead of being safely quoted and executed. The test case "should handle paths with spaces and special characters" currently fails becauseisSecurePath()blocks the&character despite the quoting logic correctly wrapping the path.Fix
isSecurePath()inapps/frontend/src/main/utils/windows-paths.tsto allow metacharacters that are safe within quoted paths, or adjust the validation to occur after quoting is applied.
🤖 Fix all issues with AI agents
In @apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts:
- Around line 172-176: The test contains a redundant non-null assertion on
keyEventHandler in the return statement of the useXterm test; since the
preceding guard throws if keyEventHandler is falsy, remove the unnecessary `!`
and return `keyEventHandler` directly (update the return in the function that
checks `if (!keyEventHandler) { throw ... }` so it returns `keyEventHandler`
without the non-null assertion).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
apps/frontend/src/main/env-utils.tsapps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.tsapps/frontend/src/main/env-utils.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.tsapps/frontend/src/main/env-utils.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/renderer/components/terminal/__tests__/useXterm.test.tsapps/frontend/src/main/env-utils.ts
🧬 Code graph analysis (2)
apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts (1)
apps/frontend/src/renderer/components/terminal/useXterm.ts (1)
useXterm(34-389)
apps/frontend/src/main/env-utils.ts (1)
apps/frontend/src/main/utils/windows-paths.ts (1)
isSecurePath(40-56)
🪛 GitHub Actions: CI
apps/frontend/src/main/env-utils.ts
[error] 636-636: prepareShellCommand: executablePath contains potentially dangerous characters. Path: D:\Path&Name\tool.cmd
🪛 GitHub Check: test-frontend
apps/frontend/src/main/env-utils.ts
[failure] 636-636: src/main/tests/env-utils.test.ts > shouldUseShell > Windows platform > should handle paths with spaces and special characters
Error: prepareShellCommand: executablePath contains potentially dangerous characters. Path: D:\Path&Name\tool.cmd
❯ prepareShellCommand src/main/env-utils.ts:636:11
❯ shouldUseShell src/main/env-utils.ts:669:26
❯ src/main/tests/env-utils.test.ts:52:14
⏰ 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). (2)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (11)
apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts (4)
9-10: LGTM: Proper test utilities imported.The addition of
afterAllenables proper cleanup after all tests complete, and theMocktype import improves type safety for mock assertions.
62-83: Excellent: Descriptor-preserving platform mock.This approach properly captures and restores the full property descriptor for
navigator.platform, preventing test pollution and ensuring each test can override the platform value without affecting subsequent tests.
234-239: LGTM: Proper global cleanup in afterAll.The
afterAllhook correctly restores the original global shims (requestAnimationFrame,cancelAnimationFrame,ResizeObserver) that were saved at module load time (lines 87-89), ensuring the test environment is fully cleaned up after all tests complete.
242-806: LGTM: Comprehensive test coverage.The test suite provides excellent coverage of:
- Platform-specific keyboard shortcuts (Windows/Linux/macOS)
- Smart copy/paste behavior (CTRL+C with/without selection)
- Linux-specific shortcuts (CTRL+SHIFT+C/V)
- Clipboard error handling
- Preservation of existing shortcuts
- Event type filtering (keydown vs. keyup)
All tests properly use
act()for React updates, handle async operations correctly, and validate both positive and negative cases. The consistent use ofsetNavigatorPlatformensures proper test isolation for platform-specific behavior.apps/frontend/src/main/env-utils.ts (7)
30-37: LGTM: Clean async existence check.The implementation correctly uses
fs.promises.accessfor non-blocking path checks and handles errors gracefully.
54-83: LGTM: Correct shell usage for Windows npm.cmd.The function correctly uses
shell: trueon Windows to resolvenpm.cmd, which requires shell execution. The caching and error handling are appropriate.
292-341: LGTM: Well-implemented async version with proper cache coordination.The async implementation correctly prevents concurrent npm calls by tracking the in-flight promise. The shell configuration matches the sync version.
352-398: LGTM: Efficient parallel path checking.The async version improves performance by checking path existence in parallel using
Promise.all, which is a good optimization over the sync version's sequential checks.
249-268: LGTM: Correct Windows extension priority.The extension search order (
.exe,.cmd,.bat,.ps1, then extensionless) correctly prioritizes native Windows executables before bash/Git Bash scripts.Also applies to: 408-425
467-487: LGTM: Well-designed quoting analysis logic.The
analyzePathQuotinghelper correctly:
- Uses case-insensitive extension checks for Windows (lines 470-472)
- Detects already-quoted paths to avoid double-quoting (line 476)
- Identifies shell metacharacters and spaces that require quoting (lines 480-484)
This is the core logic that makes Windows batch file invocation safe.
524-560: The Python string escaping logic is correct for all reachable code paths.Pre-quoted paths (like
"C:\path\file.cmd") fail theisSecurePath()security validation and cannot reach the escaping logic—this is intentional security design preventing injection via pre-quoted input. For all paths that pass security validation:
- Unquoted paths with spaces: Correctly quoted and escaped for Python
- Unquoted paths without spaces: Correctly escaped (backslashes only)
- Escaping (backslash and quote handling): Correct for Python subprocess.run()
No issues found.
| if (!isSecurePath(executablePath)) { | ||
| throw new Error( | ||
| `preparePythonSubprocessCommand: executablePath contains potentially dangerous characters. Path: ${executablePath}` | ||
| ); | ||
| } |
There was a problem hiding this comment.
Critical: isSecurePath validation breaks the primary use case.
The pipeline test failure reveals a fundamental conflict: this PR aims to safely handle Windows paths containing special characters (like &) through proper quoting, but isSecurePath() rejects those paths outright, preventing the quoting logic from running.
Test failure:
Error: prepareShellCommand: executablePath contains potentially dangerous characters. Path: D:\Path&Name\tool.cmd
Root cause: The isSecurePath() pattern /[;&|${}[]<>!"^]/rejects&, but legitimate Windows directory names can contain &(e.g.,C:\Users\Rock & Roll...`). The quoting/escaping logic added in this PR was specifically designed to handle such cases safely.
Impact: This validation blocks the core functionality introduced by this PR (handling spaces and special characters in Windows paths).
💡 Proposed solution: Make validation context-aware
Option 1 (Recommended): Remove the isSecurePath check here since the quoting logic already handles these characters safely when needsShell=true:
- // Security validation: ensure path doesn't contain dangerous characters
- if (!isSecurePath(executablePath)) {
- throw new Error(
- `preparePythonSubprocessCommand: executablePath contains potentially dangerous characters. Path: ${executablePath}`
- );
- }
-Option 2: Modify isSecurePath in windows-paths.ts to allow shell metacharacters in paths (they're safe when properly quoted):
// In windows-paths.ts
export function isSecurePath(pathStr: string): boolean {
const dangerousPatterns = [
/%[^%]+%/, // Environment variable expansion
/\.\.\//, // Directory traversal (Unix)
/\.\.\\/, // Directory traversal (Windows)
/[\r\n]/, // Newlines (command injection)
];
// Note: Removed shell metacharacters - they're safe when quoted
for (const pattern of dangerousPatterns) {
if (pattern.test(pathStr)) {
return false;
}
}
return true;
}Rationale: Shell metacharacters (&, |, <, >, etc.) in path names are safe when the path is properly quoted (which this code does). The real injection risks are:
- Unquoted variable expansion (
%PATH%) - Path traversal (
../) - Newlines (command chaining)
Committable suggestion skipped: line range outside the PR's diff.
apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts
Outdated
Show resolved
Hide resolved
The & and | characters are valid in Windows filenames and are safe when properly quoted by prepareShellCommand. The isSecurePath() validation was too strict - it rejected these characters even though the quoting function safely handles them. Removed & and | from the dangerous characters list in isSecurePath() since they are safe when paths are properly quoted. Fixes failing test: "should handle paths with spaces and special characters"
StillKnotKnown
left a comment
StillKnotKnown
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟡 Follow-up Review: Merge With Changes
🟡 Can merge - Minor suggestions noted, no blockers.
Resolution Status
- ✅ Resolved: 5 previous findings addressed
- ❌ Unresolved: 0 previous findings remain
- 🆕 New Issues: 4 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 1 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 0 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
🚨 Blocking Issues
- security: Command arguments not escaped in buildCommandInvocation
Verdict
All 5 previous findings have been properly addressed: 4 resolved through security improvements (isSecurePath validation, better documentation), and 1 dismissed as false positive after validation confirmed defense-in-depth is complete. The new code review identified 4 additional findings (1 medium security, 3 low quality). The medium security finding (NEW-001: argument escaping in buildCommandInvocation) is mitigated by current codebase usage (all arguments are hardcoded strings like '--continue' and 'setup-token'), and represents a future-proofing improvement rather than an immediate vulnerability. All CI checks (16/16) are passing. The remaining issues are low-severity quality improvements that can be addressed post-merge. The author has successfully implemented the security fixes requested in the previous review.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (4 selected of 4 total)
🟡 [NEW-001] [MEDIUM] Command arguments not escaped in buildCommandInvocation
📁 apps/frontend/src/shared/utils/shell-escape.ts:353
The buildCommandInvocation function does not escape individual arguments. While current usages only pass hardcoded arguments like ['--continue'] and ['setup-token'], the function itself is vulnerable if user-controlled data is ever passed. The argsStr is created by joining arguments without any escaping: const argsStr = hasArgs ? ' ' + args.join(' ') : '';. This represents a potential command injection vulnerability if the function is ever used with user-controlled arguments.
Suggested fix:
Escape each argument based on shell type:
const argsStr = hasArgs ? " " + args.map(arg => {
switch (shellType) {
case "powershell": return escapePowerShellArg(arg);
case "cmd": return escapeShellArgWindows(arg);
default: return escapeShellArg(arg);
}
}).join(" ") : "";
🔵 [NEW-002] [LOW] isSecurePath validation only applied to Windows user-configured paths
📁 apps/frontend/src/main/cli-tool-manager.ts:699
User-configured Claude paths are only validated with isSecurePath() on Windows (line 699: if (process.platform === 'win32' && !isSecurePath(this.userConfig.claudePath))). On Unix systems, paths are not validated for shell metacharacters. However, the risk is low because: 1) User-configured paths come from the user's own settings file, 2) On Unix, shell: true is NOT used for execution (only for Windows .cmd/.bat files), 3) execFileSync without shell: true doesn't interpret shell metacharacters.
Suggested fix:
For defense-in-depth, add Unix path validation with a Unix-friendly path validator that checks for shell metacharacters that would be dangerous if the path were ever used in a shell context.
🔵 [NEW-003] [LOW] Bash availability check blocks main process
📁 apps/frontend/src/main/terminal/claude-integration-handler.ts:39
The isBashAvailable() function uses execSync which can block the main process. This is called during invokeClaude which could cause UI jank. The execSync call has no timeout specified: require('child_process').execSync('bash --version', { stdio: 'ignore' }).
Suggested fix:
Add a timeout: require('child_process').execSync('bash --version', { stdio: 'ignore', timeout: 1000 }); or use an async version or cache the result at module initialization.
🔵 [NEW-004] [LOW] No caching of bash availability check
📁 apps/frontend/src/main/terminal/claude-integration-handler.ts:39
The isBashAvailable() function performs filesystem and process checks every time it's called. The result of this check doesn't change during the application's lifetime, so it should be cached to avoid redundant execSync calls.
Suggested fix:
Cache the result at module load time:
let bashAvailableCache: boolean | undefined;
function isBashAvailable(): boolean {
if (bashAvailableCache !== undefined) {
return bashAvailableCache;
}
// ... perform checks ...
bashAvailableCache = result;
return result;
}
This review was generated by Auto Claude.
Addresses NEW-003 and NEW-004 from code review: - Add module-level cache for isBashAvailable() result (result doesn't change during app lifetime) - Add 1000ms timeout to execSync call to prevent indefinite blocking - Cache all code paths (Unix, Windows with bash, Windows without bash) This improves performance by avoiding redundant filesystem and process checks on every Claude invocation.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/terminal/claude-integration-handler.ts:
- Around line 71-77: The catch block in the bash availability check redundantly
calls require("fs"); remove that inline require and use the already-imported fs
module instead — update the catch branch that currently does const fs =
require("fs"); to reference the top-level imported fs when evaluating
commonBashPaths.some(...) and assigning bashAvailableCache, keeping the existing
variable names (commonBashPaths, bashAvailableCache) and return value unchanged.
- Around line 735-739: The async write to tempFile uses fsPromises.writeFile
with a hardcoded { mode: 0o600 } which is problematic on Windows; change the
call to mirror the sync invokeClaude behavior by only including the mode option
when running on non-Windows (process.platform !== 'win32'), i.e., build the
options object conditionally before calling fsPromises.writeFile for the export
CLAUDE_CODE_OAUTH_TOKEN write (referencing tempFile, escapeShellArg(token), and
fsPromises.writeFile).
- Around line 925-930: The SHELL_PROMPT_PATTERNS constant is missing a
PowerShell-specific regex; update SHELL_PROMPT_PATTERNS to include a pattern
that matches PowerShell prompts like "PS C:\path>" (for example add a regex such
as /^PS\s+[^\r\n>]+>\s*$/m) so the prompt detection reliably recognizes Windows
PowerShell sessions alongside the existing Unix/cmd patterns.
- Around line 486-504: The temp file written earlier (tempFilePath) is never
removed when falling back to native Windows shells; update the non-bash branches
in invokeClaude to delete that temp file after setting the
CLAUDE_CODE_OAUTH_TOKEN: for the powershell branch (where escapePowerShellArg
and claudeCmd are used) append a Remove-Item -Force <escaped tempFilePath> (use
escapePowerShellArg(tempFilePath) or equivalent) to the command string, and for
the cmd.exe branch append a del /f "<escaped tempFilePath>" (use
escapeShellArgWindows(tempFilePath) or equivalent); ensure both branches still
set the env var and invoke claude before deleting and that the temp path is
properly escaped.
- Around line 741-764: The async temp-file branch assumes bash exists on
Windows; add an isBashAvailable() check (same helper used by the sync
invokeClaude) and, if bash is not available, construct native PowerShell and cmd
commands instead of invoking bash: in claude-integration-handler.ts adjust the
block that builds command for shellType === "powershell" || shellType === "cmd"
to call await isBashAvailable(), and when false build a PowerShell command that
sources the temp file, deletes it, and executes claudeCmd using PowerShell
escaping/quoting, and a cmd.exe equivalent for cmd, mirroring the fallback logic
used in the sync invokeClaude function (use escapedTempFile, escapedClaudeCmd /
escapedClaudeBash variables appropriately).
📜 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 (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/main/terminal/claude-integration-handler.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
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
🧠 Learnings (2)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
📚 Learning: 2026-01-04T23:59:48.743Z
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
Applied to files:
apps/frontend/src/main/terminal/claude-integration-handler.ts
🧬 Code graph analysis (1)
apps/frontend/src/main/terminal/claude-integration-handler.ts (3)
apps/frontend/src/main/claude-profile-manager.ts (1)
getClaudeProfileManager(560-565)apps/frontend/src/shared/utils/shell-escape.ts (6)
buildCdCommandForShell(417-441)buildPathAssignment(215-239)escapeShellArg(33-38)escapePowerShellArg(153-158)escapeShellArgWindows(83-98)buildCommandInvocation(353-391)apps/frontend/src/main/claude-cli-utils.ts (2)
getClaudeCliInvocation(44-52)getClaudeCliInvocationAsync(66-77)
⏰ 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). (2)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (8)
apps/frontend/src/main/terminal/claude-integration-handler.ts (8)
115-131: LGTM!The discriminated union pattern for
ClaudeCommandConfigprovides good type safety, and the command construction for each method is correct for Unix-style shells.
172-200: LGTM!Good extraction of the finalization logic into a reusable helper. The function correctly handles terminal title updates, IPC notifications, and session persistence.
205-261: LGTM!Rate limit detection and auto-switching logic is well-structured with appropriate guard checks and error handling.
266-365: LGTM!OAuth token handling correctly distinguishes between profile-specific terminals and active profile flows, with proper success/failure notifications.
938-1008: LGTM!The polling-based approach with configurable timeout is appropriate for monitoring terminal output. The shell prompt patterns cover common shell environments.
1013-1149: LGTM!Profile switching logic correctly handles the complex flow of exiting the current Claude session and invoking with a new profile. The timeout handling provides reasonable fallback behavior with appropriate warnings.
391-596: LGTM!The sync
invokeClaudefunction correctly handles shell-type-aware command construction with proper escaping for different shell environments and invocation methods.
609-897: LGTM!The resume functions correctly use
--continuewith shell-aware command construction, and appropriately deprecate the sessionId parameter while maintaining backward compatibility.
- Remove inline require('fs') in bash availability check, use top-level imported fs module
- Fix async writeFile to only set mode 0o600 on non-Windows platforms
- Add PowerShell prompt regex pattern to SHELL_PROMPT_PATTERNS
- Add temp file removal to native PowerShell/cmd fallback branches
- Add isBashAvailable() check to async temp-file branch with native fallback
Mirrors the sync invokeClaude behavior in invokeClaudeAsync, ensuring Windows
users without bash can use OAuth token authentication with native shell syntax.
There was a problem hiding this comment.
Actionable comments posted: 4
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/claude-integration-handler.ts (1)
666-876: Significant code duplication between sync and async versions.The
invokeClaudeandinvokeClaudeAsyncfunctions share ~200 lines of nearly identical shell-specific command building logic. Consider extracting the command-building logic into a shared helper function that both can use.♻️ Suggested approach
Extract a helper function that takes the profile, shellType, claudeCmd, etc. and returns the appropriate command string:
interface BuildClaudeCommandParams { shellType: ShellType | "unknown"; cwdCommand: string; pathPrefix: string; claudeCmd: string; activeProfile: ProfileInfo & { configDir?: string }; token: string | undefined; } function buildProfileSpecificCommand(params: BuildClaudeCommandParams): string | null { const { shellType, cwdCommand, pathPrefix, claudeCmd, activeProfile, token } = params; if (token) { // Token-based command building logic... return command; } else if (activeProfile.configDir) { // ConfigDir-based command building logic... return command; } return null; // Use default method }Then both
invokeClaudeandinvokeClaudeAsynccan call this helper, reducing duplication.
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/terminal/claude-integration-handler.ts:
- Around line 771-783: The shell invocation always prefixes commands with
"clear" which is wrong for Windows shells; update the command construction in
the shellType === "powershell" branch to use "Clear-Host" (or the PowerShell
alias) instead of "clear", and in the cmd.exe branch to use "cls" instead of
"clear" while preserving the existing use of cwdCommand, pathPrefix, and the
escaped variables (escapePowerShellArg(token), escapePowerShellArg(claudeCmd),
escapePowerShellArg(tempFile) in the PowerShell branch and
escapeShellArgWindows(token), escapeShellArgWindows(claudeCmd),
escapeShellArgWindows(tempFile) in the cmd branch) so the final assignments to
the command variable call the correct clear/cls utility and still set
CLAUDE_CODE_OAUTH_TOKEN, run the claude command, and remove the temp file.
- Around line 56-76: isBashAvailable currently returns true if a bash executable
exists in commonBashPaths even when execSync("bash --version") failed (bash not
in PATH), causing callers that run "bash -c ..." to fail; modify isBashAvailable
to either (a) return the full absolute path to the discovered bash (from
commonBashPaths) instead of a boolean and update callers to use that path when
invoking bash, or (b) drop the commonBashPaths fallback so isBashAvailable only
returns true when bash is in PATH; update references to bashAvailableCache and
all callers that call bash -c to accept and use the returned path (e.g., use
returnedBashPath + " -c ..." when not null) so commands never assume bash is on
PATH.
- Around line 479-484: The PowerShell and cmd.exe branches in the shellType
conditional are identical; remove the duplicate branches and assign command once
using the shared template instead of repeating the same string in both the if
and else. Replace the if (shellType === "powershell") { ... } else { ... } with
a single assignment that builds command from cwdCommand, pathPrefix,
escapedTempFileBash and escapedClaudeBash (and appends "\r" as before), or keep
a minimal conditional only for true differences if any are introduced later.
- Around line 498-504: The shell-clear commands are incorrect for Windows: in
the cmd.exe branch replace the leading "clear &&" with "cls &&" (update the
constructed command where command is assigned using cwdCommand, pathPrefix,
escapedToken, escapedClaudeCmdWin, escapedTempFileWin) and in the PowerShell
branch replace "clear &&" with a PowerShell-appropriate clear such as
"Clear-Host; " (or "cls; ") and use ";" as the statement separator instead of
"&&" so the PowerShell command construction (the branch that builds command
using escapeShellArgWindows / escapedToken / escapedClaudeCmdWin etc.) works
across older PowerShell versions.
📜 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 (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/main/terminal/claude-integration-handler.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
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
🧠 Learnings (1)
📚 Learning: 2026-01-04T23:59:48.743Z
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
Applied to files:
apps/frontend/src/main/terminal/claude-integration-handler.ts
🧬 Code graph analysis (1)
apps/frontend/src/main/terminal/claude-integration-handler.ts (5)
apps/frontend/src/main/claude-profile-manager.ts (2)
getClaudeProfileManager(560-565)initializeClaudeProfileManager(572-590)apps/frontend/src/main/terminal/types.ts (2)
RateLimitEvent(34-42)OAuthTokenEvent(47-54)apps/frontend/src/shared/constants/ipc.ts (1)
IPC_CHANNELS(6-517)apps/frontend/src/shared/utils/shell-escape.ts (6)
buildCdCommandForShell(417-441)buildPathAssignment(215-239)escapeShellArg(33-38)escapePowerShellArg(153-158)escapeShellArgWindows(83-98)buildCommandInvocation(353-391)apps/frontend/src/main/claude-cli-utils.ts (2)
getClaudeCliInvocation(44-52)getClaudeCliInvocationAsync(66-77)
⏰ 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). (2)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (10)
apps/frontend/src/main/terminal/claude-integration-handler.ts (10)
6-26: LGTM!Imports are well-organized and all appear to be used throughout the file. The shell-escape utilities are appropriately imported for the multi-shell support being added.
114-130: LGTM!The discriminated union provides good type safety, and the function correctly generates bash-specific commands with history-safe prefixes. The JSDoc documentation is thorough.
171-199: LGTM!Good refactoring to consolidate the post-invocation logic. The function handles null checks appropriately and correctly differentiates terminal titles for default vs. named profiles.
204-260: LGTM!Rate limit handling is well-implemented with proper deduplication via the
lastNotifiedRateLimitResetmap. The auto-switch flow is correctly async with proper error handling.
265-364: LGTM!OAuth token handling is comprehensive with proper fallback from profile-specific terminals to the active profile. Good defensive null check added for
activeProfileat line 312.
533-567: LGTM!The configDir method correctly handles shell-specific environment variable assignment for PowerShell, cmd.exe, and Unix shells. The bash-c wrapper with exec is appropriate for replacing the shell process.
610-654: LGTM!Good use of shell-type aware command building. The deprecation warning for the
sessionIdparameter is helpful for callers.
884-926: LGTM!Clean async implementation of
resumeClaudewith proper shell-type awareness.
954-960: LGTM!Good addition of the PowerShell prompt pattern (
/^PS\s+[^\r\n>]+>\s*$/m). The regex patterns now cover common shell prompts across bash, zsh, fish, and PowerShell.
1043-1179: LGTM!Comprehensive profile switching implementation with robust exit handling. The use of
waitForClaudeExitwith timeout handling ensures the function doesn't hang indefinitely. Good tracing throughout for debugging.
- Extract shared command-building logic into buildProfileCommandString() helper - Refactor isBashAvailable() to getBashPath() returning absolute path - Fix clear commands for Windows shells (PowerShell: Clear-Host, cmd: cls) - Remove duplicate PowerShell/cmd.exe branches in bash invocation - Add temp file removal to native Windows shell fallback Changes: 1. New buildProfileCommandString() helper function centralizes shell-specific command building logic for both token-based and configDir-based methods, reducing ~100 lines of duplication between sync and async versions. 2. getBashPath() now returns the absolute path to bash (or null if not found) instead of a boolean. This fixes the issue where bash exists in common installation locations but is not in PATH, causing "bash -c ..." to fail. 3. Fixed clear commands for Windows: Clear-Host for PowerShell, cls for cmd.exe instead of the Unix-only "clear" command. 4. Removed duplicate PowerShell/cmd.exe conditional branches that had identical command strings. 5. Native Windows shell fallback (when bash unavailable) now properly uses Clear-Host/cls for screen clearing and removes temp files after use.
- Re-apply requestAnimationFrame mock in afterEach hook after vi.restoreAllMocks() to prevent "requestAnimationFrame is not defined" errors from pending timeouts - Add requestAnimationFrame mock setup to terminal-copy-paste.test.ts - Add afterAll to vitest imports in terminal-copy-paste.test.ts The issue was that vi.restoreAllMocks() was removing the global requestAnimationFrame mock while async setTimeout callbacks were still pending, causing them to fail when trying to call the removed mock.
There was a problem hiding this comment.
Actionable comments posted: 9
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/__tests__/integration/terminal-copy-paste.test.ts (1)
144-180: Consider extracting repeated mock setup into a helper function.The XTerm and addon mock implementations are duplicated across all tests in this file. Extracting this to a helper would reduce boilerplate and make tests easier to maintain.
Example helper approach
// Helper to set up XTerm mocks with optional overrides function setupXtermMocks(overrides: { hasSelection?: () => boolean; getSelection?: () => string; onKeyHandler?: (handler: (event: KeyboardEvent) => boolean) => void; // ... other overrides } = {}) { const keyEventHandler: { current: ((event: KeyboardEvent) => boolean) | null } = { current: null }; (XTerm as unknown as Mock).mockImplementation(function () { return { open: vi.fn(), loadAddon: vi.fn(), attachCustomKeyEventHandler: vi.fn((handler) => { keyEventHandler.current = handler; overrides.onKeyHandler?.(handler); }), hasSelection: overrides.hasSelection ?? vi.fn(() => false), getSelection: overrides.getSelection ?? vi.fn(() => ""), // ... rest of implementation }; }); // Setup addon mocks... return { keyEventHandler }; }
🤖 Fix all issues with AI agents
In @apps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts:
- Around line 115-122: afterEach currently calls vi.restoreAllMocks and then
re-applies requestAnimationFrame/cancelAnimationFrame but not the ResizeObserver
mock; update afterEach to also re-apply the global.ResizeObserver mock (the same
mock created in beforeEach) after vi.restoreAllMocks so tests that run after
restoreAllMocks still have a working ResizeObserver; locate the ResizeObserver
setup in beforeEach and mirror that re-assignment in afterEach alongside the
existing global.requestAnimationFrame and global.cancelAnimationFrame
re-applications.
In @apps/frontend/src/main/terminal/claude-integration-handler.ts:
- Around line 820-831: The inline comment about file permissions is vague;
update the comment above the writeOptions block (near variables tempFile,
writeOptions, and the call to fsPromises.writeFile) to explicitly state that
mode: 0o600 is a Unix-only POSIX file permission flag and is ignored or
unsupported on Windows (process.platform === "win32"), so we only set it on
non-Windows platforms to avoid errors; keep the existing conditional logic but
replace the comment with this clearer explanation referencing mode 0o600,
process.platform, writeOptions, and the subsequent fsPromises.writeFile call.
- Around line 580-590: Update the inline comment above the writeOptions logic to
explicitly state that Unix file permission bits (mode 0o600) are POSIX-specific
and not applicable on Windows—Node on Windows ignores POSIX mode flags—so we
only set writeOptions.mode when process.platform !== "win32"; reference the
variables tempFile, writeOptions, fs.writeFileSync and the conditional on
process.platform to locate the comment to clarify why Windows is skipped.
- Around line 159-171: The current implementation in the claude invocation
builder embeds the OAuth token directly into the generated PowerShell and cmd
command strings (see variables claudeCmd, tempFile, escapePowerShellArg,
escapeShellArgWindows, cwdCommand, pathPrefix), which exposes the token on the
process command line; instead generate a temporary script file (PowerShell .ps1
for PowerShell flows and a .bat for cmd flows) that contains the token
assignment and the claude invocation (using properly escaped claudeCmd and
cwdCommand inside the script), write that temp script securely, invoke it via
powershell -File "temp-script.ps1" or cmd /c "temp-script.bat" (so the token
never appears in the parent process command line), then remove the temp script
with Remove-Item/del as part of the invoked script or immediately after
execution; update the code paths that currently return inline commands to
instead create, call, and clean up these temp script files and ensure you keep
using the existing escape helpers when writing script contents.
In @apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts:
- Around line 448-468: Remove the redundant null-check guards around
keyEventHandler in the tests: setupMockXterm already throws if the handler isn’t
attached, so delete the surrounding "if (keyEventHandler) { ... }" and directly
invoke keyEventHandler!(event) (or call keyEventHandler(event) with a non-null
assertion) in the blocks that create KeyboardEvent objects; apply the same
removal for the other occurrences noted (around the blocks at ~490-496,
~599-601, ~715-717, ~736-738, ~756-758) to improve readability.
- Around line 170-183: The test redundantly uses a non-null assertion on
keyEventHandler after a fail-fast check; remove the `!` on the returned
keyEventHandler and simply return `keyEventHandler` (it is guaranteed non-null
by the preceding if). Also simplify or fix the returned mockInstance: either
construct and return the actual mock terminal instance used in tests instead of
passing through possibly-undefined override properties, or remove mockInstance
entirely if tests don't use it; update references to `overrides.hasSelection`,
`overrides.getSelection`, `overrides.paste`, and `overrides.input` accordingly
so the returned object contains only defined functions or the real mock methods
used by useXterm.
- Around line 228-237: The afterEach currently calls vi.restoreAllMocks() and
then redundantly re-applies the requestAnimationFrame/cancelAnimationFrame
mocks; remove the two re-assignments of global.requestAnimationFrame and
global.cancelAnimationFrame from the afterEach block while keeping
vi.restoreAllMocks() and the restoration of navigator.platform, relying on the
existing beforeEach to re-establish those global mocks as needed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
apps/frontend/src/__tests__/integration/terminal-copy-paste.test.tsapps/frontend/src/main/terminal/claude-integration-handler.tsapps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.tsapps/frontend/src/main/terminal/claude-integration-handler.tsapps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.tsapps/frontend/src/main/terminal/claude-integration-handler.tsapps/frontend/src/__tests__/integration/terminal-copy-paste.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/renderer/components/terminal/__tests__/useXterm.test.tsapps/frontend/src/main/terminal/claude-integration-handler.tsapps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts
🧠 Learnings (1)
📚 Learning: 2026-01-04T23:59:48.743Z
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
Applied to files:
apps/frontend/src/main/terminal/claude-integration-handler.ts
🧬 Code graph analysis (2)
apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts (1)
apps/frontend/src/renderer/components/terminal/useXterm.ts (1)
useXterm(34-389)
apps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts (1)
apps/frontend/src/renderer/components/terminal/useXterm.ts (1)
useXterm(34-389)
⏰ 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). (2)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (20)
apps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts (5)
68-72: LGTM! Good practice saving original globals for restoration.Capturing the original global shims before tests run ensures clean restoration in
afterAll, preventing test pollution in other test files.
124-129: LGTM! Proper cleanup of global shims.The
afterAllhook correctly restores all three original globals, ensuring this test file doesn't pollute the test environment for other files.
294-308: LGTM! Good pattern for platform-specific test isolation.Saving and restoring
navigator.platformin nestedbeforeEach/afterEachhooks ensures platform-specific tests don't affect each other.
474-582: LGTM! Comprehensive keyboard event propagation testing.The tests properly verify that copy/paste shortcuts don't interfere with other terminal shortcuts like SHIFT+Enter, and that event handling order is maintained correctly.
676-786: LGTM! Important error resilience test.This test verifies that clipboard permission errors are handled gracefully and don't break terminal functionality - an important UX consideration for environments with restricted clipboard access.
apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts (5)
62-83: Well-designed navigator.platform handling.Capturing the full property descriptor and providing a helper function to set
navigator.platformwithconfigurable: trueis a clean approach that prevents descriptor pollution across tests. This pattern ensures proper restoration inafterEach.
86-89: Good global mock lifecycle management.Saving originals for
requestAnimationFrame,cancelAnimationFrame, andResizeObserverand restoring them inafterAllprevents test pollution and ensures subsequent test suites have clean globals.Also applies to: 239-244
246-370: Comprehensive platform detection tests.The platform detection tests properly cover Windows, Linux, and macOS behaviors for paste/copy shortcuts including the Linux-specific
CTRL+SHIFT+C/Vvariants. Good coverage of edge cases.
636-698: Solid clipboard error handling tests.Good coverage of both write and read errors with proper console.error verification and mock restoration. The graceful error handling pattern (log but don't throw) aligns with the implementation in
useXterm.ts.
791-811: Good defensive test for event type filtering.Testing that only
keydownevents are handled (notkeyup) prevents accidental double-handling of shortcuts. This aligns with the implementation'sevent.type === 'keydown'checks.apps/frontend/src/main/terminal/claude-integration-handler.ts (10)
6-26: LGTM: Imports are well-organized and necessary.The imports support the new shell-aware command construction and both sync/async file operations for token handling.
203-254: LGTM: Type-safe command building for Unix shells.The discriminated union pattern ensures correct options are provided for each method. The history-safe prefixes and
execusage prevent token leakage in shell history and ensure clean process replacement.Note: This function handles only Unix-style shells (bash/zsh/fish). Windows-specific logic is in
buildProfileCommandString.
295-323: LGTM: Good refactoring that consolidates post-invocation logic.This function eliminates code duplication between sync and async variants while ensuring consistent terminal title updates, IPC notifications, and session persistence.
328-384: LGTM: Robust rate limit detection and auto-switching.Proper deduplication, comprehensive IPC events, and graceful error handling for the auto-switch flow.
389-488: LGTM with minor i18n consideration: OAuth token detection and auto-save.The logic properly handles profile-specific terminals, active profile fallback, and comprehensive error cases. IPC notifications provide good feedback to the renderer.
Note on i18n: Lines 445 and 482 send hardcoded English error messages via IPC (
"No active profile found","Failed to save token to active profile"). If these are displayed to users in the renderer, they should use translation keys. However, per project learnings, pre-existing i18n issues can be deferred to future cleanup passes.
514-683: LGTM: Shell-aware Claude invocation with proper profile handling.The function correctly implements shell-type awareness for command construction, handles token-based and configDir-based authentication, and consolidates finalization logic.
696-740: LGTM: Shell-aware resume with clear deprecation handling.The function correctly uses shell-aware helpers and provides clear documentation about why
--continueis preferred over--resumewith session IDs.
752-924: LGTM: Async version properly implements non-blocking invocation.All async operations are properly awaited, preventing main process blocking. The code duplication with the sync version is intentional to support both APIs.
998-1086: LGTM: Robust Claude exit detection with PowerShell support.The prompt-based detection is much more reliable than fixed delays, and the addition of PowerShell patterns (line 1007) aligns with the PR's cross-shell objectives. The fallback to
isClaudeModeflag provides additional safety.
1091-1228: LGTM: Robust profile switching with comprehensive error handling.The function properly:
- Detects and exits Claude mode before switching
- Uses prompt-based exit detection instead of fixed delays
- Handles timeouts gracefully (proceeds with warning rather than blocking users)
- Provides extensive tracing for debugging
- Clears rate limit state before re-invoking
The updated callback signature (lines 1095-1099) improves readability.
| if (shellType === "powershell" || shellType === "cmd") { | ||
| // On Windows, check if bash is available | ||
| const bashPath = getBashPath(); | ||
| if (bashPath) { | ||
| // Use bash-style temp file with bash -c wrapper | ||
| const tempFileBash = tempFile.replace(/\\/g, "/"); | ||
| const bashClaudeCmd = claudeCmd.replace(/\\/g, "/"); | ||
| const escapedClaudeBash = escapeShellArg(bashClaudeCmd); | ||
| const escapedTempFileBash = escapeShellArg(tempFileBash); | ||
| // Use the discovered bash path (may be absolute path on Windows) | ||
| const bashExe = bashPath === "bash" ? "bash" : bashPath.replace(/\\/g, "/"); | ||
|
|
||
| // Both PowerShell and cmd.exe use the same bash command | ||
| return `${cwdCommand}${pathPrefix}${bashExe} -c "source ${escapedTempFileBash} && rm -f ${escapedTempFileBash} && exec ${escapedClaudeBash}"\r`; |
There was a problem hiding this comment.
Critical: Bash executable path not quoted, will fail when path contains spaces.
Lines 151-154 construct a bash command using the discovered bash path, but fail to quote bashExe when it contains spaces. This directly contradicts the PR objective of fixing "Claude Code detection fails when username contains spaces."
If bashPath is C:\Program Files\Git\bin\bash.exe, the conversion to forward slashes yields C:/Program Files/Git/bin/bash.exe, but line 154 uses it unquoted:
bash -c "source ..." // if bashPath is "bash"
C:/Program Files/Git/bin/bash.exe -c "source ..." // FAILS - splits on space
🐛 Proposed fix: Quote the bash executable path
const tempFileBash = tempFile.replace(/\\/g, "/");
const bashClaudeCmd = claudeCmd.replace(/\\/g, "/");
const escapedClaudeBash = escapeShellArg(bashClaudeCmd);
const escapedTempFileBash = escapeShellArg(tempFileBash);
- // Use the discovered bash path (may be absolute path on Windows)
- const bashExe = bashPath === "bash" ? "bash" : bashPath.replace(/\\/g, "/");
+ // Use the discovered bash path (may be absolute path on Windows) and quote it
+ const bashExe = bashPath === "bash"
+ ? "bash"
+ : `"${bashPath.replace(/\\/g, "/")}"`; // Quote if absolute path
- // Both PowerShell and cmd.exe use the same bash command
- return `${cwdCommand}${pathPrefix}${bashExe} -c "source ${escapedTempFileBash} && rm -f ${escapedTempFileBash} && exec ${escapedClaudeBash}"\r`;
+ // For PowerShell, need call operator if quoted
+ if (shellType === "powershell" && bashExe.startsWith('"')) {
+ return `${cwdCommand}${pathPrefix}& ${bashExe} -c "source ${escapedTempFileBash} && rm -f ${escapedTempFileBash} && exec ${escapedClaudeBash}"\r`;
+ } else {
+ return `${cwdCommand}${pathPrefix}${bashExe} -c "source ${escapedTempFileBash} && rm -f ${escapedTempFileBash} && exec ${escapedClaudeBash}"\r`;
+ }Note: PowerShell requires the call operator & when invoking a quoted executable.
📝 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.
| if (shellType === "powershell" || shellType === "cmd") { | |
| // On Windows, check if bash is available | |
| const bashPath = getBashPath(); | |
| if (bashPath) { | |
| // Use bash-style temp file with bash -c wrapper | |
| const tempFileBash = tempFile.replace(/\\/g, "/"); | |
| const bashClaudeCmd = claudeCmd.replace(/\\/g, "/"); | |
| const escapedClaudeBash = escapeShellArg(bashClaudeCmd); | |
| const escapedTempFileBash = escapeShellArg(tempFileBash); | |
| // Use the discovered bash path (may be absolute path on Windows) | |
| const bashExe = bashPath === "bash" ? "bash" : bashPath.replace(/\\/g, "/"); | |
| // Both PowerShell and cmd.exe use the same bash command | |
| return `${cwdCommand}${pathPrefix}${bashExe} -c "source ${escapedTempFileBash} && rm -f ${escapedTempFileBash} && exec ${escapedClaudeBash}"\r`; | |
| if (shellType === "powershell" || shellType === "cmd") { | |
| // On Windows, check if bash is available | |
| const bashPath = getBashPath(); | |
| if (bashPath) { | |
| // Use bash-style temp file with bash -c wrapper | |
| const tempFileBash = tempFile.replace(/\\/g, "/"); | |
| const bashClaudeCmd = claudeCmd.replace(/\\/g, "/"); | |
| const escapedClaudeBash = escapeShellArg(bashClaudeCmd); | |
| const escapedTempFileBash = escapeShellArg(tempFileBash); | |
| // Use the discovered bash path (may be absolute path on Windows) and quote it | |
| const bashExe = bashPath === "bash" | |
| ? "bash" | |
| : `"${bashPath.replace(/\\/g, "/")}"`; // Quote if absolute path | |
| // For PowerShell, need call operator if quoted | |
| if (shellType === "powershell" && bashExe.startsWith('"')) { | |
| return `${cwdCommand}${pathPrefix}& ${bashExe} -c "source ${escapedTempFileBash} && rm -f ${escapedTempFileBash} && exec ${escapedClaudeBash}"\r`; | |
| } else { | |
| return `${cwdCommand}${pathPrefix}${bashExe} -c "source ${escapedTempFileBash} && rm -f ${escapedTempFileBash} && exec ${escapedClaudeBash}"\r`; | |
| } |
| if (shellType === "powershell") { | ||
| // PowerShell: set environment variable, invoke claude, then remove temp file | ||
| const escapedToken = escapePowerShellArg(token); | ||
| const escapedClaude = escapePowerShellArg(claudeCmd); | ||
| const escapedTempFile = escapePowerShellArg(tempFile); | ||
| return `Clear-Host; ${cwdCommand}$env:CLAUDE_CODE_OAUTH_TOKEN=${escapedToken}; ${pathPrefix}${escapedClaude}; Remove-Item -Force ${escapedTempFile}\r`; | ||
| } else { | ||
| // cmd.exe: set environment variable, invoke claude, then remove temp file | ||
| const escapedToken = escapeShellArgWindows(token); | ||
| const escapedClaudeCmdWin = escapeShellArgWindows(claudeCmd); | ||
| const escapedTempFileWin = escapeShellArgWindows(tempFile); | ||
| return `cls && ${cwdCommand}set "CLAUDE_CODE_OAUTH_TOKEN=${escapedToken}" && ${pathPrefix}"${escapedClaudeCmdWin}" && del /f "${escapedTempFileWin}"\r`; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd apps/frontend/src/main/terminal && cat -n claude-integration-handler.ts | head -180 | tail -40Repository: AndyMik90/Auto-Claude
Length of output: 2720
🏁 Script executed:
cd apps/frontend/src/main/terminal && grep -n "buildProfileCommandString" claude-integration-handler.tsRepository: AndyMik90/Auto-Claude
Length of output: 406
🏁 Script executed:
cd apps/frontend/src/main/terminal && wc -l claude-integration-handler.tsRepository: AndyMik90/Auto-Claude
Length of output: 99
🏁 Script executed:
cd apps/frontend/src/main/terminal && sed -n '127,180p' claude-integration-handler.tsRepository: AndyMik90/Auto-Claude
Length of output: 2905
🏁 Script executed:
cd apps/frontend/src/main/terminal && grep -B5 -A2 "token exposure\|security\|OAuth" claude-integration-handler.ts | head -50Repository: AndyMik90/Auto-Claude
Length of output: 1887
🏁 Script executed:
cd apps/frontend && find . -name "*.md" -o -name "*.txt" | xargs grep -l "Windows\|PowerShell\|token" 2>/dev/null | head -10Repository: AndyMik90/Auto-Claude
Length of output: 510
🏁 Script executed:
cd apps/frontend && cat CONTRIBUTING.md | grep -A10 -B10 -i "windows\|powershell\|token" | head -80Repository: AndyMik90/Auto-Claude
Length of output: 541
🏁 Script executed:
cd apps/frontend && cat README.md | grep -A10 -B10 -i "windows\|powershell\|token\|security" | head -80Repository: AndyMik90/Auto-Claude
Length of output: 1817
🏁 Script executed:
cd apps/frontend/src && find . -name "*escape*" -o -name "*shell*" | grep -E "\.(ts|tsx|js)$" | head -10Repository: AndyMik90/Auto-Claude
Length of output: 130
🏁 Script executed:
cd apps/frontend/src && rg "escapePowerShellArg|escapeShellArgWindows" -A5 | head -60Repository: AndyMik90/Auto-Claude
Length of output: 3715
🏁 Script executed:
cd apps/frontend/src/main/terminal && grep -B10 -A10 "temp file\|tempFile" claude-integration-handler.ts | head -60Repository: AndyMik90/Auto-Claude
Length of output: 2619
🏁 Script executed:
cd apps/frontend/src/main/terminal && sed -n '156,172p' claude-integration-handler.tsRepository: AndyMik90/Auto-Claude
Length of output: 1245
Remove Get-Content suggestion; confirm token exposure concern is valid, but propose better alternative using temp script files.
The core security concern is valid: on Windows without bash, the OAuth token is embedded directly in PowerShell (line 164) and cmd.exe (line 170) command strings, exposing it to:
- Process command-line arguments (visible in Task Manager/Process Explorer)
- Potentially PowerShell/cmd history
However, the Get-Content suggestion does not solve this—the token still appears in the command line during variable assignment. A better approach would use temporary PowerShell/batch script files:
For PowerShell:
powershell -File "temp-script.ps1"where temp-script.ps1 contains the token assignment and claude invocation.
For cmd.exe:
Create a temporary batch file containing the token and invoke it, keeping the token out of the command line entirely.
This approach keeps tokens confined to temp files (like the bash method) rather than exposing them in command-line arguments, while maintaining the same cleanup pattern.
🤖 Prompt for AI Agents
In @apps/frontend/src/main/terminal/claude-integration-handler.ts around lines
159 - 171, The current implementation in the claude invocation builder embeds
the OAuth token directly into the generated PowerShell and cmd command strings
(see variables claudeCmd, tempFile, escapePowerShellArg, escapeShellArgWindows,
cwdCommand, pathPrefix), which exposes the token on the process command line;
instead generate a temporary script file (PowerShell .ps1 for PowerShell flows
and a .bat for cmd flows) that contains the token assignment and the claude
invocation (using properly escaped claudeCmd and cwdCommand inside the script),
write that temp script securely, invoke it via powershell -File
"temp-script.ps1" or cmd /c "temp-script.bat" (so the token never appears in the
parent process command line), then remove the temp script with Remove-Item/del
as part of the invoked script or immediately after execution; update the code
paths that currently return inline commands to instead create, call, and clean
up these temp script files and ensure you keep using the existing escape helpers
when writing script contents.
apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts
Show resolved
Hide resolved
apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts
Show resolved
Hide resolved
apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts
Outdated
Show resolved
Hide resolved
- Extract repeated XTerm/addon mock setup into setupMockXTermWithHandler() helper in terminal-copy-paste.test.ts, reducing ~100 lines of duplicated code - Update file permissions comments in claude-integration-handler.ts to explicitly state that mode 0o600 is a Unix-only POSIX file permission flag - Remove redundant null-check guards around keyEventHandler in useXterm.test.ts (the handler is guaranteed non-null by the check above) - Remove non-null assertion after fail-fast check in useXterm.test.ts - Fix return type annotation in setupMockXterm() to match actual return value
There was a problem hiding this comment.
Actionable comments posted: 10
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/renderer/components/terminal/__tests__/useXterm.test.ts (1)
174-215: Add cleanup() call to afterEach blocks or global setup.This test uses
@vitest-environment jsdomandrender()from @testing-library/react, but never callscleanup(). Without it, DOM nodes and event listeners will accumulate across tests. Addcleanup()to theafterEachblock in this test or to the global setup file (src/__tests__/setup.ts) to prevent DOM leaks.apps/frontend/src/main/terminal/claude-integration-handler.ts (1)
514-685: Significant code duplication between sync and async invoke methods.The
invokeClaude(lines 514-685) andinvokeClaudeAsync(lines 754-928) functions share ~95% identical logic, differing only in:
- Sync vs async initialization:
getClaudeCliInvocation()vsawait getClaudeCliInvocationAsync()- Sync vs async file I/O:
fs.writeFileSync()vsawait fsPromises.writeFile()- Function signature: synchronous vs
asyncThis duplication makes maintenance harder—any bug fix or enhancement must be applied to both functions.
♻️ Consider extracting shared logic into a helper
Extract the common command-building and invocation logic into a shared helper:
/** * Shared implementation for invoking Claude with profile support */ async function invokeClaude_Internal( terminal: TerminalProcess, cwd: string | undefined, profileId: string | undefined, getWindow: WindowGetter, onSessionCapture: (terminalId: string, projectPath: string, startTime: number) => void, // Dependency injection for sync vs async operations deps: { initProfileManager: () => Promise<ClaudeProfileManager> | ClaudeProfileManager; getCliInvocation: () => Promise<ClaudeCliInvocation> | ClaudeCliInvocation; writeTokenFile: (path: string, content: string, options: fs.WriteFileOptions) => Promise<void> | void; } ): Promise<void> { // Common implementation here // ... } // Sync version export function invokeClaude(...args) { return invokeClaude_Internal(...args, { initProfileManager: () => getClaudeProfileManager(), getCliInvocation: () => getClaudeCliInvocation(), writeTokenFile: (path, content, opts) => fs.writeFileSync(path, content, opts), }); } // Async version export async function invokeClaudeAsync(...args) { return invokeClaude_Internal(...args, { initProfileManager: () => initializeClaudeProfileManager(), getCliInvocation: () => getClaudeCliInvocationAsync(), writeTokenFile: (path, content, opts) => fsPromises.writeFile(path, content, opts), }); }This reduces duplication while preserving both sync and async APIs.
Alternatively, if the async version is preferred for all use cases (non-blocking), consider deprecating the sync version and migrating all callers to the async API.
Also applies to: 754-928
🤖 Fix all issues with AI agents
In @apps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts:
- Around line 201-215: Remove the redundant re-application of globals in the
afterEach block: after calling vi.restoreAllMocks(), do not reassign
global.requestAnimationFrame, global.cancelAnimationFrame, or
global.ResizeObserver (these are already set in beforeEach). Delete the lines
that reassign those globals in the afterEach so afterEach only calls
vi.restoreAllMocks(); if any test truly needs globals reset mid-test, handle
that explicitly in that test or adjust beforeEach instead.
In @apps/frontend/src/main/terminal/claude-integration-handler.ts:
- Around line 70-78: The synchronous execSync in getBashPath blocks the main
thread; replace it with an async child_process.exec call (or perform detection
during async initialization) so bash detection does not block UI, return a
Promise from getBashPath (or populate bashPathCache during startup) and
propagate async changes to all callers such as buildProfileCommandString so they
await the result; keep the existing caching behavior (bashPathCache = "bash")
and preserve a timeout/abort mechanism for the subprocess.
- Around line 576-623: The temp file holding the OAuth token in invokeClaude is
not guaranteed to be removed on errors or if buildProfileCommandString returns
null; wrap the tempFile creation/use in a try/finally so you always unlink the
file (fs.unlinkSync) in the finally block, catch and surface errors from
fs.writeFileSync and terminal.pty.write, and ensure that if
buildProfileCommandString(...) returns null you cleanup before falling through;
apply the same try/finally pattern in invokeClaudeAsync and optionally register
a process exit handler to remove orphaned files matching the `.claude-token-*`
pattern.
- Line 72: The inline require call require("child_process").execSync used on
line with execSync("bash --version") should be replaced with a module-level
import for consistency; add an import like execSync from 'child_process' at the
top of claude-integration-handler.ts and update the inline call to use
execSync(...) instead of require(...). Ensure you remove the inline require and
keep the existing options object passed to execSync unchanged so behavior
remains identical.
In @apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts:
- Around line 85-90: The test setup assigns globals (origRequestAnimationFrame,
origCancelAnimationFrame, origResizeObserver) but then calls
vi.restoreAllMocks(), which won't revert direct global assignments; change the
lifecycle to set the shims in beforeAll and restore the originals in afterAll by
reassigning global.requestAnimationFrame = origRequestAnimationFrame etc., and
use vi.clearAllMocks() in afterEach to reset spies/mocks per test; remove or
avoid relying on vi.restoreAllMocks() for undoing the global shims.
- Around line 589-614: The test for "Linux CTRL+SHIFT+V paste shortcut" is
brittle due to key casing; update the KeyboardEvent creation in that test to use
key: "v" (lowercase) with ctrlKey: true and shiftKey: true so it matches
implementations that lowercase event.key, and keep assertions around
keyEventHandler, mockClipboard.readText and mockPaste unchanged (or add a second
assertion/event using uppercase "V" if you need to cover both behaviors).
- Around line 9-14: Replace unsafe "unknown as Mock" casts with typed helpers
from Vitest: use vi.mocked(XTerm) (and vi.mocked(FitAddon) where used) so mocks
are properly typed; locate places in the test file where XTerm or FitAddon are
cast to Mock (e.g., "XTerm as unknown as Mock") and change them to
vi.mocked(XTerm) or vi.mocked(FitAddon), removing the cast and adjusting any
call/instance assertions to rely on the typed mock returned by vi.mocked.
- Around line 616-679: The test assertions assert the exact console error prefix
which is brittle; update the two expectations in the "Clipboard error handling"
tests to match the log prefix with a partial matcher instead of exact equality —
replace the toHaveBeenCalledWith checks that expect "[useXterm] Failed to copy
selection:" and "[useXterm] Failed to read clipboard:" with checks using
expect.stringContaining(...) as the first argument while keeping
expect.any(Error) as the second, so the tests assert the prefix text is present
but tolerate non-critical log changes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
apps/frontend/src/__tests__/integration/terminal-copy-paste.test.tsapps/frontend/src/main/terminal/claude-integration-handler.tsapps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/__tests__/integration/terminal-copy-paste.test.tsapps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.tsapps/frontend/src/main/terminal/claude-integration-handler.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/__tests__/integration/terminal-copy-paste.test.tsapps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.tsapps/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/__tests__/integration/terminal-copy-paste.test.tsapps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.tsapps/frontend/src/main/terminal/claude-integration-handler.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
📚 Learning: 2026-01-04T23:59:48.743Z
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
Applied to files:
apps/frontend/src/main/terminal/claude-integration-handler.ts
🧬 Code graph analysis (2)
apps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts (1)
apps/frontend/src/renderer/components/terminal/useXterm.ts (1)
useXterm(34-389)
apps/frontend/src/main/terminal/claude-integration-handler.ts (4)
apps/frontend/src/shared/utils/shell-escape.ts (7)
ShellType(15-15)escapeShellArg(33-38)escapePowerShellArg(153-158)escapeShellArgWindows(83-98)buildCommandInvocation(353-391)buildCdCommandForShell(417-441)buildPathAssignment(215-239)apps/frontend/src/main/terminal/types.ts (3)
ShellType(8-8)RateLimitEvent(34-42)OAuthTokenEvent(47-54)apps/frontend/src/main/claude-profile-manager.ts (2)
getClaudeProfileManager(560-565)initializeClaudeProfileManager(572-590)apps/frontend/src/main/claude-cli-utils.ts (2)
getClaudeCliInvocation(44-52)getClaudeCliInvocationAsync(66-77)
⏰ 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). (2)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (18)
apps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts (7)
68-72: LGTM! Good practice saving original globals for restoration.Capturing the original global shims before tests run ensures proper cleanup in
afterAll, preventing cross-test pollution.
86-158: LGTM! Well-designed helper function for capturing key event handlers.The
setupMockXTermWithHandlerhelper centralizes mock setup and captures the xterm key event handler, enabling direct invocation in tests rather than relying on DOM event dispatch. This pattern improves test reliability and control.
217-222: LGTM! Proper cleanup of global shims.Restoring original globals in
afterAllensures no cross-suite pollution.
224-315: LGTM! Comprehensive copy integration tests.The tests properly verify both the positive path (selection exists → clipboard write) and negative path (no selection → no clipboard write). Using the captured key event handler directly improves test reliability over DOM event dispatch.
317-423: LGTM! Good coverage of platform-specific paste behavior.The tests properly mock
navigator.platformfor Windows/Linux scenarios and restore it after each test. Both the happy path (clipboard has content) and edge case (empty clipboard) are covered.
425-556: LGTM! Thorough keyboard event propagation tests.Good verification that copy/paste shortcuts don't interfere with existing shortcuts (Ctrl+1-9, Ctrl+Tab, Ctrl+T, Ctrl+W) and that they properly bubble up to window-level handlers.
558-635: LGTM! Important error resilience test.This test verifies that the terminal remains functional after a clipboard permission error. The pattern of using a console.error spy to verify error logging while confirming continued terminal operation is well-implemented. Proper spy restoration at line 633 prevents test pollution.
apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts (8)
17-59: Mocks are sufficiently complete for the tested behavior.
Good thatgetSelection()defaults to""androwsis present (avoids undefined access).
95-173:setupMockXterm()is a solid test helper; fail-fast handler assertion is especially good.
This reduces boilerplate and makes future behavior changes obvious.
234-358: Platform shortcut coverage looks good; consider making SHIFT+V/C tests resilient to browser key casing.
Some environments reportevent.keyas"v"even withshiftKey: true; ifuseXtermdoesn’t normalize via.toLowerCase(), these tests may be brittle.
360-457: Smart CTRL/C behavior tests are clear and correctly separate “copy selection” vs “send ^C”.
Good assertions on handler return values + clipboard side effects.
459-534: Paste behavior tests are consistent with platform expectations.
The explicit macOS “ctrlKey+V should not paste” case is a good regression guard.
536-587: Linux CTRL+SHIFT+C vs Windows behavior is well covered.
No issues.
681-763: Shortcut preservation tests are valuable; looped Ctrl+1-9 case is nicely compact.
No issues spotted.
765-785: Keyup ignored test is a good guardrail.
No issues.apps/frontend/src/main/terminal/claude-integration-handler.ts (3)
295-323: LGTM! Good consolidation of post-invocation logic.The
finalizeClaudeInvokefunction effectively centralizes terminal title updates, IPC notifications, session persistence, and callback invocations. This reduces code duplication between sync and async invoke methods and improves maintainability.The implementation includes proper null checks and type-safe parameter handling.
1006-1012: Good implementation of prompt-based exit detection.The
waitForClaudeExitfunction improves reliability by polling for shell prompt patterns instead of using fixed delays. The addition of PowerShell prompt detection (line 1011) aligns well with the cross-shell support goals of this PR.The implementation includes:
- Reasonable timeout (5000ms) and polling interval (100ms)
- Fallback to
isClaudeModeflag check- Clear error messages on timeout
💡 Minor enhancement: Add cmd.exe prompt pattern
Consider adding a pattern for Windows cmd.exe prompts, which often show as
C:\path>:const SHELL_PROMPT_PATTERNS = [ /[$%#>❯]\s*$/m, /\w+@[\w.-]+[:\s]/, /^\s*\S+\s*[$%#>❯]\s*$/m, /\(.*\)\s*[$%#>❯]\s*$/m, /^PS\s+[^\r\n>]+>\s*$/m, + /^[A-Z]:[\\\/].*>\s*$/m, // cmd.exe: C:\path> ];This would improve exit detection reliability on Windows systems using cmd.exe.
Also applies to: 1020-1090
1095-1232: Improved profile switching with better exit handling and tracing.The enhanced
switchClaudeProfileimplementation adds several reliability improvements:
- Defensive exit handling: Sends Ctrl+C followed by
/exitcommand with adaptive wait (lines 1166-1176)- Timeout handling: Proceeds with caution on timeout rather than aborting, preventing users from getting stuck (lines 1178-1188)
- Verbose tracing: Extensive
console.warnlogging aids debugging profile switching issues (throughout)- Type-safe callback: Updated signature with multiline formatting improves readability (lines 1099-1103)
💡 Optional: Consider structured logging
The extensive
console.warncalls (lines 1107-1230) could be consolidated using a structured logger with context:const logger = createLogger("[ClaudeIntegration:switchClaudeProfile]", { terminalId: terminal.id, profileId, }); logger.info("Called for terminal"); logger.debug("Terminal state:", { isClaudeMode: terminal.isClaudeMode }); // ... etcThis would reduce repetition and enable easier filtering/categorization of logs.
| // Check if bash is in PATH (returns "bash" if found via PATH) | ||
| try { | ||
| require("child_process").execSync("bash --version", { | ||
| stdio: "ignore", | ||
| timeout: 1000, // Add timeout to prevent blocking | ||
| }); | ||
| bashPathCache = "bash"; | ||
| return "bash"; | ||
| } catch { |
There was a problem hiding this comment.
Synchronous subprocess execution blocks the main thread.
The execSync call on line 72 can block Electron's main process for up to 1 second, potentially causing UI freezes. While module-level caching mitigates repeated calls, the first invocation will still block.
⚡ Consider async alternative using child_process.exec
Refactor to use async exec and make getBashPath return a Promise, or move bash detection to the async initialization phase:
+import { exec } from "child_process";
+import { promisify } from "util";
+
+const execAsync = promisify(exec);
+
-function getBashPath(): string | null {
+async function getBashPathAsync(): Promise<string | null> {
// Return cached result if available
if (bashPathCache !== undefined) {
return bashPathCache;
}
if (process.platform !== "win32") {
bashPathCache = "bash";
return "bash";
}
const commonBashPaths = [
"C:\\Program Files\\Git\\bin\\bash.exe",
"C:\\Program Files\\Git\\usr\\bin\\bash.exe",
"C:\\msys64\\usr\\bin\\bash.exe",
"C:\\cygwin64\\bin\\bash.exe",
];
try {
- require("child_process").execSync("bash --version", {
- stdio: "ignore",
- timeout: 1000,
- });
+ await execAsync("bash --version", {
+ timeout: 1000,
+ windowsHide: true,
+ });
bashPathCache = "bash";
return "bash";
} catch {
for (const bashPath of commonBashPaths) {
if (fs.existsSync(bashPath)) {
bashPathCache = bashPath;
return bashPath;
}
}
bashPathCache = null;
return null;
}
}Note: This would require making buildProfileCommandString async and updating all call sites.
🤖 Prompt for AI Agents
In @apps/frontend/src/main/terminal/claude-integration-handler.ts around lines
70 - 78, The synchronous execSync in getBashPath blocks the main thread; replace
it with an async child_process.exec call (or perform detection during async
initialization) so bash detection does not block UI, return a Promise from
getBashPath (or populate bashPathCache during startup) and propagate async
changes to all callers such as buildProfileCommandString so they await the
result; keep the existing caching behavior (bashPathCache = "bash") and preserve
a timeout/abort mechanism for the subprocess.
| } else { | ||
| // Bash not available - use native PowerShell/cmd syntax | ||
| // For native Windows shells, we need to create a different temp file format | ||
| // and set the token as an environment variable directly | ||
| if (shellType === "powershell") { | ||
| // PowerShell: set environment variable, invoke claude, then remove temp file | ||
| const escapedToken = escapePowerShellArg(token); | ||
| const escapedClaude = escapePowerShellArg(claudeCmd); | ||
| const escapedTempFile = escapePowerShellArg(tempFile); | ||
| return `Clear-Host; ${cwdCommand}$env:CLAUDE_CODE_OAUTH_TOKEN=${escapedToken}; ${pathPrefix}${escapedClaude}; Remove-Item -Force ${escapedTempFile}\r`; | ||
| } else { | ||
| // cmd.exe: set environment variable, invoke claude, then remove temp file | ||
| const escapedToken = escapeShellArgWindows(token); | ||
| const escapedClaudeCmdWin = escapeShellArgWindows(claudeCmd); | ||
| const escapedTempFileWin = escapeShellArgWindows(tempFile); | ||
| return `cls && ${cwdCommand}set "CLAUDE_CODE_OAUTH_TOKEN=${escapedToken}" && ${pathPrefix}"${escapedClaudeCmdWin}" && del /f "${escapedTempFileWin}"\r`; | ||
| } | ||
| } |
There was a problem hiding this comment.
Security concern: OAuth token exposure in native Windows shells.
When bash is unavailable on Windows (lines 159-171), the OAuth token is set as an environment variable in PowerShell or cmd.exe. This exposes the token in several ways:
- Process listings: Tools like
tasklistor Process Explorer can display environment variables of running processes - Child processes: All processes spawned by the shell inherit
CLAUDE_CODE_OAUTH_TOKEN - Shell history: PowerShell history may capture commands containing the token (unlike the
HISTFILE=controls used in bash)
The bash method (line 154) is more secure as it uses a temporary file with restricted permissions (mode 0o600) that's immediately deleted.
Recommendations:
-
Add a security warning in logs when falling back to native Windows shells:
console.warn("[ClaudeIntegration] SECURITY WARNING: Using native Windows shell for token authentication. Token may be visible in process listings. Install Git Bash for improved security.");
-
Document this limitation in user-facing documentation and recommend installing Git Bash for Windows users with security concerns.
-
Consider alternative approaches:
- Create a PowerShell script file with restricted ACLs instead of inline commands
- Use a named pipe for token passing (more complex but more secure)
- Prompt users to authenticate via browser OAuth flow instead of token passing
Based on learnings, security hardening was a stated goal of this PR (ACS-176), so this tradeoff should be explicitly documented or improved.
apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts
Outdated
Show resolved
Hide resolved
apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts
Show resolved
Hide resolved
apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts
Show resolved
Hide resolved
- Add cleanup() to afterEach in useXterm.test.ts to prevent DOM leaks
- Fix test setup lifecycle: use beforeAll/afterAll for global mocks, clearAllMocks in afterEach
- Remove redundant global mock re-application in terminal-copy-paste.test.ts afterEach
- Replace inline require('child_process') with module-level execSync import
- Fix brittle key casing test: use lowercase "v" for CTRL+SHIFT+V test
- Replace unsafe "unknown as Mock" casts with vi.mocked() for type safety
- Use expect.stringContaining() for console error assertions (less brittle)
- Add try/finally to ensure temp OAuth token files are always cleaned up
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/terminal/claude-integration-handler.ts:
- Around line 1209-1230: When waitForClaudeExit times out in
switchClaudeProfile, add retry-and-fallback handling instead of silently
proceeding: implement a limited retry loop in waitForClaudeExit that resends the
exit sequence (e.g., sendCtrlC + "/exit"), try an alternative Ctrl+D (\x04) if
available, and attempt a graceful suspend/force kill fallback by tracking the
Claude PID and calling process.kill as a last resort; on final timeout return a
structured result like { success: false, error: "timeout", requiresManualExit:
true } so the caller/UI can notify the user, and ensure you clear/normalize
terminal state (terminal.isClaudeMode = false, terminal.claudeSessionId =
undefined and reset claudeProfileId) when you determine the session is no longer
valid to avoid stale state.
- Around line 576-640: The temp-file cleanup in invokeClaude (the finally block
that unlinks tempFile after terminal.pty.write(command)) races with the shell
executing the command and can delete the token before the shell sources it;
remove that immediate cleanup and instead rely on the in-shell cleanup already
embedded in the command (rm/Remove-Item/del) and add a non-blocking fallback
deferred cleanup (e.g., setTimeout to check and unlink tempFile after a few
seconds) so the shell has time to execute; apply the same change to
invokeClaudeAsync where the same finally unlink pattern appears.
In @apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts:
- Around line 9-10: The import of the Mock type is unused; remove "Mock" from
the import list so the top import becomes just the runtime helpers from vitest
(i.e., delete the type-only import symbol "Mock" from the import declaration in
this test file), ensuring no other references to Mock remain in the file.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
apps/frontend/src/__tests__/integration/terminal-copy-paste.test.tsapps/frontend/src/main/terminal/claude-integration-handler.tsapps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.tsapps/frontend/src/__tests__/integration/terminal-copy-paste.test.tsapps/frontend/src/main/terminal/claude-integration-handler.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.tsapps/frontend/src/__tests__/integration/terminal-copy-paste.test.tsapps/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/renderer/components/terminal/__tests__/useXterm.test.tsapps/frontend/src/__tests__/integration/terminal-copy-paste.test.tsapps/frontend/src/main/terminal/claude-integration-handler.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
📚 Learning: 2026-01-04T23:59:48.743Z
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
Applied to files:
apps/frontend/src/main/terminal/claude-integration-handler.ts
🧬 Code graph analysis (3)
apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts (1)
apps/frontend/src/renderer/components/terminal/useXterm.ts (1)
useXterm(34-389)
apps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts (1)
apps/frontend/src/renderer/components/terminal/useXterm.ts (1)
useXterm(34-389)
apps/frontend/src/main/terminal/claude-integration-handler.ts (4)
apps/frontend/src/shared/utils/shell-escape.ts (7)
ShellType(15-15)escapeShellArg(33-38)escapePowerShellArg(153-158)escapeShellArgWindows(83-98)buildCommandInvocation(353-391)buildCdCommandForShell(417-441)buildPathAssignment(215-239)apps/frontend/src/main/terminal/types.ts (3)
ShellType(8-8)RateLimitEvent(34-42)OAuthTokenEvent(47-54)apps/frontend/src/main/claude-profile-manager.ts (2)
getClaudeProfileManager(560-565)initializeClaudeProfileManager(572-590)apps/frontend/src/main/claude-cli-utils.ts (2)
getClaudeCliInvocation(44-52)getClaudeCliInvocationAsync(66-77)
⏰ 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). (2)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (5)
apps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts (3)
68-210: Good test lifecycle management with proper cleanup.The pattern of saving original globals (lines 68-73) and restoring them in
afterAll(lines 205-210) is excellent practice for preventing test pollution. ThebeforeEach/afterEachpair properly isolates each test case.Minor note: The
as unknown as numbertype assertion on line 171 is a necessary workaround for jsdom environment compatibility.
74-158: Well-designed test helper for capturing key event handlers.The
setupMockXTermWithHandlerfunction elegantly solves the challenge of testing custom key event handlers by capturing them in a wrapper object. The pattern of overriding mocks before module import works correctly with Vitest's module system.The type assertions like
as unknown as Mock(line 104) are acceptable for test utilities where mock typing flexibility is needed.
212-624: Comprehensive test coverage with good edge case handling.The test suite covers key scenarios effectively:
- Selection API integration with clipboard operations (copy with/without selection)
- Platform-specific paste behavior (Windows/Linux)
- Keyboard event propagation and handler ordering
- Error recovery (clipboard denied doesn't break terminal)
The dynamic import pattern (
await import("../../renderer/components/terminal/useXterm")) after mock setup ensures fresh module evaluation with updated mocks—a correct approach for testing with mock overrides.The platform-specific tests (Win32/Linux) align well with the PR's objective of improving cross-platform shell handling.
apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts (2)
61-240: Excellent test infrastructure and lifecycle management!The test setup demonstrates several best practices:
- Comprehensive navigator.platform mocking with proper descriptor preservation to avoid test pollution (lines 61-83)
- Well-designed
setupMockXtermhelper that significantly reduces boilerplate while maintaining flexibility (lines 91-172)- Proper global shim management in
beforeAll/afterAll(lines 180-196, 235-240)- Thorough cleanup in
afterEachto ensure test isolation (lines 228-233)- Fail-fast guard check at line 166-168 that provides clear errors if the hook stops attaching handlers
This infrastructure provides a solid foundation for the comprehensive test coverage below.
242-793: Comprehensive test coverage for cross-platform keyboard handling!The test suite thoroughly validates:
- Platform-specific paste/copy shortcuts (Windows CTRL+V, Linux CTRL+SHIFT+C/V, macOS Cmd handling)
- Smart CTRL+C behavior (copy when text selected, interrupt when not)
- Clipboard error handling with proper console.error verification
- Preservation of existing shortcuts (SHIFT+Enter, Ctrl+Backspace, Ctrl+1-9, Ctrl+T/W)
- Event type filtering (keydown vs keyup)
This comprehensive coverage directly supports the PR's objective to fix Claude CLI detection failures on Windows when paths contain spaces, by ensuring the terminal keyboard handling remains robust across all platforms.
| function getBashPath(): string | null { | ||
| // Return cached result if available | ||
| if (bashPathCache !== undefined) { | ||
| return bashPathCache; | ||
| } | ||
|
|
||
| if (process.platform !== "win32") { | ||
| bashPathCache = "bash"; // Unix systems always have bash in PATH | ||
| return "bash"; | ||
| } | ||
|
|
||
| // On Windows, check if bash is in PATH by trying common locations | ||
| const commonBashPaths = [ | ||
| "C:\\Program Files\\Git\\bin\\bash.exe", | ||
| "C:\\Program Files\\Git\\usr\\bin\\bash.exe", | ||
| "C:\\msys64\\usr\\bin\\bash.exe", | ||
| "C:\\cygwin64\\bin\\bash.exe", | ||
| ]; | ||
|
|
||
| // Check if bash is in PATH (returns "bash" if found via PATH) | ||
| try { | ||
| execSync("bash --version", { | ||
| stdio: "ignore", | ||
| timeout: 1000, // Add timeout to prevent blocking | ||
| }); | ||
| bashPathCache = "bash"; | ||
| return "bash"; | ||
| } catch { | ||
| // bash not found in PATH, check common installation locations | ||
| for (const bashPath of commonBashPaths) { | ||
| if (fs.existsSync(bashPath)) { | ||
| bashPathCache = bashPath; | ||
| return bashPath; | ||
| } | ||
| } | ||
| // bash not found anywhere | ||
| bashPathCache = null; | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider async alternative for getBashPath to avoid blocking.
The execSync call (line 73) blocks the main thread for up to 1000ms during the first invocation. While caching mitigates repeated blocking, the initial call can still delay the event loop.
Consider an async version for non-blocking detection
If bash detection timing becomes an issue, consider:
- Pre-warming the cache during app initialization (async)
- Creating an async version
getBashPathAsync()that usesexecwith promises - Falling back to synchronous detection only when necessary
Current implementation is acceptable since:
- Caching ensures it only blocks once per app lifetime
- 1000ms timeout prevents indefinite blocking
- Windows users without bash are detected immediately via the catch block
| import { describe, it, expect, vi, beforeEach, afterEach, beforeAll, afterAll } from "vitest"; | ||
| import type { Mock } from "vitest"; |
There was a problem hiding this comment.
Remove unused import.
The Mock type is imported but never used in this file.
🧹 Proposed fix
-import { describe, it, expect, vi, beforeEach, afterEach, beforeAll, afterAll } from "vitest";
-import type { Mock } from "vitest";
+import { describe, it, expect, vi, beforeEach, afterEach, beforeAll, afterAll } from "vitest";📝 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.
| import { describe, it, expect, vi, beforeEach, afterEach, beforeAll, afterAll } from "vitest"; | |
| import type { Mock } from "vitest"; | |
| import { describe, it, expect, vi, beforeEach, afterEach, beforeAll, afterAll } from "vitest"; |
🤖 Prompt for AI Agents
In @apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts
around lines 9 - 10, The import of the Mock type is unused; remove "Mock" from
the import list so the top import becomes just the runtime helpers from vitest
(i.e., delete the type-only import symbol "Mock" from the import declaration in
this test file), ensuring no other references to Mock remain in the file.
…ling - Document security trade-off for OAuth token exposure in native Windows shells (PowerShell/cmd.exe when bash is unavailable, tokens visible in process listings) - Add retry-and-fallback handling for waitForClaudeExit timeout in switchClaudeProfile (retries up to 2 times with Ctrl+C + /exit before proceeding) - Fix test lifecycle: don't restore original requestAnimationFrame in afterAll (pending setTimeout callbacks may fire after tests complete)
| // Mock requestAnimationFrame for jsdom environment (not provided by default) | ||
| global.requestAnimationFrame = vi.fn((cb: FrameRequestCallback) => setTimeout(cb, 0) as unknown as number); | ||
| // Save originals to restore in afterAll hook | ||
| const origRequestAnimationFrame = global.requestAnimationFrame; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
| global.requestAnimationFrame = vi.fn((cb: FrameRequestCallback) => setTimeout(cb, 0) as unknown as number); | ||
| // Save originals to restore in afterAll hook | ||
| const origRequestAnimationFrame = global.requestAnimationFrame; | ||
| const origCancelAnimationFrame = global.cancelAnimationFrame; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
| // Save originals to restore in afterAll hook | ||
| const origRequestAnimationFrame = global.requestAnimationFrame; | ||
| const origCancelAnimationFrame = global.cancelAnimationFrame; | ||
| const origResizeObserver = global.ResizeObserver; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
There was a problem hiding this comment.
Actionable comments posted: 4
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/claude-integration-handler.ts (1)
783-970: Consider extracting shared logic between sync and async versions.
invokeClaudeAsyncduplicates most of the logic frominvokeClaude. While thebuildProfileCommandStringhelper already extracts some shared logic, the profile resolution, environment override checks, and command execution flow could be further consolidated.One approach: extract the common logic into a shared function that accepts the CLI invocation and file operations as parameters, allowing both sync and async versions to share the core flow.
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/terminal/claude-integration-handler.ts:
- Around line 96-111: The BuildProfileCommandParams interface redundantly unions
"unknown" with ShellType; update the interface declaration
(BuildProfileCommandParams) to use ShellType alone for the shellType property
(i.e., replace `shellType: ShellType | "unknown"` with `shellType: ShellType`)
and ensure any call sites constructing BuildProfileCommandParams still pass a
valid ShellType value; no other behavior changes needed.
- Around line 897-906: The finally block in invokeClaudeAsync is causing a race
by deleting tempFile as soon as terminal.pty.write() returns; move the cleanup
out of the finally and only delete the tempFile in error paths (e.g., inside the
catch handling for the overall try) so that successful command execution (which
handles its own cleanup) isn't interrupted. Specifically, remove the tempFile
unlink/access calls from the finally and add them to the catch that handles
failures in invokeClaudeAsync (or the surrounding try/catch), referencing
tempFile and terminal.pty.write() so the shell can source the file before any
deletion.
- Around line 637-650: The finally block in invokeClaude that checks
fs.existsSync(tempFile) and calls fs.unlinkSync(tempFile) races with the
asynchronous shell execution (terminal.pty.write(command)) and can delete the
tempFile before the shell sources it; remove the premature cleanup from the
finally block (the temp file is already removed by the shell commands that
include rm/Remove-Item/del) so delete the fs.existsSync/fs.unlinkSync cleanup
and related debugError/debugLog calls in the finally section of invokeClaude to
avoid the race.
In @apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts:
- Around line 607-611: The KeyboardEvent in the test creates an event with key:
"v" while shiftKey: true; change the key to the uppercase "V" so the synthetic
event matches browser behavior when Shift is pressed (update the KeyboardEvent
construction that assigns key, ctrlKey, shiftKey — the variable named event in
useXterm.test.ts).
📜 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/renderer/components/terminal/__tests__/useXterm.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/main/terminal/claude-integration-handler.tsapps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/main/terminal/claude-integration-handler.tsapps/frontend/src/renderer/components/terminal/__tests__/useXterm.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/renderer/components/terminal/__tests__/useXterm.test.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
📚 Learning: 2026-01-04T23:59:48.743Z
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
Applied to files:
apps/frontend/src/main/terminal/claude-integration-handler.ts
🧬 Code graph analysis (2)
apps/frontend/src/main/terminal/claude-integration-handler.ts (2)
apps/frontend/src/shared/utils/shell-escape.ts (7)
ShellType(15-15)escapeShellArg(33-38)escapePowerShellArg(153-158)escapeShellArgWindows(83-98)buildCommandInvocation(353-391)buildCdCommandForShell(417-441)buildPathAssignment(215-239)apps/frontend/src/main/terminal/types.ts (3)
ShellType(8-8)RateLimitEvent(34-42)OAuthTokenEvent(47-54)
apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts (1)
apps/frontend/src/renderer/components/terminal/useXterm.ts (1)
useXterm(34-389)
⏰ 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). (2)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (12)
apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts (4)
61-83: Excellent improvement to platform mocking.The full property descriptor capture (lines 63-71) and the
setNavigatorPlatformhelper prevent property descriptor poisoning across tests. Restoring the original descriptor inafterEach(line 232) ensures proper cleanup. This is a robust pattern for mocking non-writable navigator properties.Also applies to: 232-232
95-172: Well-designed helper function with fail-fast validation.The refactored
setupMockXtermhelper reduces test boilerplate and provides a clear contract: it returns a Promise with a guaranteed non-nullkeyEventHandler. The explicit null check at lines 166-168 ("useXterm did not attach a custom key event handler") provides excellent fail-fast behavior if the hook's internal behavior changes.
180-240: Appropriate lifecycle management with clear rationale.The
beforeAll/afterAllhooks properly manage global mocks. The comment at lines 236-239 clearly explains why the original global shims (saved at lines 87-89) are intentionally NOT restored: jsdom doesn't provide these APIs, and restoring them toundefinedwould break pendingsetTimeoutcallbacks from component lifecycle. Thecleanup()call inafterEach(line 230) properly prevents memory leaks.
242-793: Comprehensive test coverage for keyboard handling.The test suite thoroughly covers:
- Platform-specific copy/paste shortcuts across Windows, Linux, and macOS
- Smart CTRL+C behavior (copy vs. interrupt based on selection)
- Linux-specific CTRL+SHIFT+C/V shortcuts
- macOS system clipboard integration (Cmd+V)
- Clipboard error handling with graceful degradation
- Preservation of existing shortcuts (SHIFT+Enter, CTRL+Backspace, tab switching)
- Event type discrimination (keydown vs. keyup)
The test structure is well-organized with clear describe blocks and descriptive test names.
apps/frontend/src/main/terminal/claude-integration-handler.ts (8)
6-27: LGTM!The imports are well-organized, separating Node.js built-ins, shared utilities, and local modules. The inclusion of both synchronous and asynchronous file system operations (
fsandfsPromises) appropriately supports the sync/async invoke patterns.
52-91: LGTM!The bash path detection is well-implemented with:
- Module-level caching to avoid redundant subprocess calls
- A 1000ms timeout on
execSyncto prevent blocking- Fallback to common installation locations on Windows
- Proper error handling for missing bash
137-213: LGTM!The shell-specific command building logic is well-structured with:
- Clear separation between token-based and configDir-based methods
- Proper shell escaping for each shell type (PowerShell, cmd, bash/zsh/fish)
- Well-documented security trade-off for Windows systems without bash (lines 159-170)
- Appropriate fallback behavior returning
nullfor default method
340-396: LGTM!The rate limit handling is well-structured with proper deduplication via the
lastNotifiedRateLimitResetmap, appropriate error handling, and correctly gated auto-switch behavior based on user settings.
401-500: LGTM!OAuth token handling demonstrates good security practices:
- Token value is never logged (only length at line 411)
- Comprehensive error handling with defensive null checks
- Both success and failure paths properly notify the renderer
- Profile-specific and non-profile terminal paths are handled correctly
1062-1132: LGTM!The
waitForClaudeExitfunction is well-designed with:
- Configurable timeout and poll interval
- Multiple shell prompt patterns covering bash, zsh, fish, and PowerShell
- Structured return type enabling retry logic in callers
- Fallback detection via
isClaudeModeflag
1219-1264: LGTM!The retry logic for
waitForClaudeExitis a good addition for handling cases where Claude doesn't exit cleanly on the first attempt. The decision to proceed with the profile switch after all retries fail (with logging) is a reasonable trade-off—blocking indefinitely would be worse for user experience, and the comprehensive tracing enables debugging of such edge cases.
553-570: LGTM!The shell-type-aware command construction is well-implemented with consistent fallback to
"unknown"whenterminal.shellTypeis not set. The use ofbuildCdCommandForShell,buildPathAssignment, andbuildCommandInvocationutilities provides clean cross-platform compatibility.
| } finally { | ||
| // Always clean up the temp file, even if an error occurs | ||
| try { | ||
| await fsPromises.access(tempFile, fs.constants.F_OK); | ||
| await fsPromises.unlink(tempFile); | ||
| debugLog("[ClaudeIntegration:invokeClaudeAsync] Cleaned up temp file:", tempFile); | ||
| } catch { | ||
| // File doesn't exist or already cleaned up, ignore | ||
| } | ||
| } |
There was a problem hiding this comment.
Same race condition as the synchronous version.
The async finally block has the identical issue: it runs immediately after terminal.pty.write() returns, potentially deleting the temp file before the shell can source it. Apply the same fix—move cleanup to a catch block for error cases only, since successful command execution handles its own cleanup.
🐛 Proposed fix
if (command) {
debugLog(
"[ClaudeIntegration:invokeClaudeAsync] Executing command (temp file method, history-safe)"
);
terminal.pty.write(command);
profileManager.markProfileUsed(activeProfile.id);
finalizeClaudeInvoke(
terminal,
activeProfile,
projectPath,
startTime,
getWindow,
onSessionCapture
);
debugLog(
"[ClaudeIntegration:invokeClaudeAsync] ========== INVOKE CLAUDE COMPLETE (temp file) =========="
);
return;
}
- // If command is null, fall through to default method
- } finally {
- // Always clean up the temp file, even if an error occurs
- try {
- await fsPromises.access(tempFile, fs.constants.F_OK);
- await fsPromises.unlink(tempFile);
- debugLog("[ClaudeIntegration:invokeClaudeAsync] Cleaned up temp file:", tempFile);
- } catch {
- // File doesn't exist or already cleaned up, ignore
- }
+ } catch (writeError) {
+ // Only clean up if we failed to write/execute the command
+ try {
+ await fsPromises.access(tempFile, fs.constants.F_OK);
+ await fsPromises.unlink(tempFile);
+ debugLog("[ClaudeIntegration:invokeClaudeAsync] Cleaned up temp file after error:", tempFile);
+ } catch {
+ // File doesn't exist or already cleaned up, ignore
+ }
+ throw writeError;
}🤖 Prompt for AI Agents
In @apps/frontend/src/main/terminal/claude-integration-handler.ts around lines
897 - 906, The finally block in invokeClaudeAsync is causing a race by deleting
tempFile as soon as terminal.pty.write() returns; move the cleanup out of the
finally and only delete the tempFile in error paths (e.g., inside the catch
handling for the overall try) so that successful command execution (which
handles its own cleanup) isn't interrupted. Specifically, remove the tempFile
unlink/access calls from the finally and add them to the catch that handles
failures in invokeClaudeAsync (or the surrounding try/catch), referencing
tempFile and terminal.pty.write() so the shell can source the file before any
deletion.
apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts
Show resolved
Hide resolved
…ycle - Remove try/finally blocks from invokeClaude/invokeClaudeAsync that were racing with shell execution; shell commands already handle cleanup (rm/Remove-Item/del) - Update BuildProfileCommandParams to use ShellType alone (was redundant union) - Fix KeyboardEvent key casing from "v" to "V" when shiftKey is true - Add beforeAll hooks for global mocks in terminal-copy-paste.test.ts - Re-apply global mocks in afterEach to prevent uncaught requestAnimationFrame errors All code review feedback items addressed except extract shared logic (deferred).
| describe('Terminal copy/paste integration', () => { | ||
| // Mock requestAnimationFrame for jsdom environment (not provided by default) | ||
| // Save originals to restore in afterAll hook | ||
| const origRequestAnimationFrame = global.requestAnimationFrame; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
| // Mock requestAnimationFrame for jsdom environment (not provided by default) | ||
| // Save originals to restore in afterAll hook | ||
| const origRequestAnimationFrame = global.requestAnimationFrame; | ||
| const origCancelAnimationFrame = global.cancelAnimationFrame; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
| // Save originals to restore in afterAll hook | ||
| const origRequestAnimationFrame = global.requestAnimationFrame; | ||
| const origCancelAnimationFrame = global.cancelAnimationFrame; | ||
| const origResizeObserver = global.ResizeObserver; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
There was a problem hiding this comment.
Actionable comments posted: 12
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/claude-integration-handler.ts (1)
307-335: LGTM: Excellent refactoring to consolidate post-invocation logic.This function eliminates duplication between sync and async invocation paths by centralizing terminal state updates, IPC notifications, session persistence, and callback invocation.
🤖 Fix all issues with AI agents
In @apps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts:
- Around line 68-72: The three saved globals origRequestAnimationFrame,
origCancelAnimationFrame, and origResizeObserver are declared but never used;
either remove these declarations or prefix them with an underscore (e.g.,
_origRequestAnimationFrame) to satisfy the linter, and also delete the empty
afterAll block that only contains a comment (it serves no runtime/test purpose).
Ensure you update any related comments so there are no leftover references to
those variables or the no-op afterAll hook.
- Around line 166-237: Duplicate global mocks for requestAnimationFrame,
cancelAnimationFrame, and ResizeObserver are repeated in beforeAll, beforeEach,
and afterEach; extract that repeated code into a single helper (e.g.,
setupGlobalMocks) that assigns global.requestAnimationFrame,
global.cancelAnimationFrame, and global.ResizeObserver, then call
setupGlobalMocks() from beforeAll, beforeEach (before other per-test mocks like
mockClipboard and window.electronAPI), and afterEach (to re-apply mocks for
pending async callbacks) and remove the duplicated blocks; keep per-test setup
for mockClipboard and window.electronAPI in beforeEach only.
In @apps/frontend/src/main/terminal/claude-integration-handler.ts:
- Around line 145-155: The constructed command uses bashExe directly which can
break when bashPath contains spaces; wrap or escape bashExe before interpolating
into the returned command (e.g., apply the existing escapeShellArg to bashExe or
quote it appropriately) so the final string built in the return (`${bashExe} -c
"..."`) uses a safely quoted/escaped executable name; update the logic around
bashExe (set via bashPath === "bash" ? "bash" : bashPath.replace(/\\/g, "/"))
and reuse escapeShellArg(bashExe) (or conditionally quote based on shellType)
when building the command.
In @apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts:
- Around line 61-83: The module currently captures navigator.platform at import
time via originalNavigatorPlatformDescriptor which can throw if navigator is
undefined; move the descriptor capture into a safe lifecycle hook (e.g.,
beforeAll) or guard with globalThis.navigator?. to avoid errors at module-load
time, and keep the helper setNavigatorPlatform as-is but ensure any restore
logic uses the descriptor captured in beforeAll; update tests to rely on the
runtime-captured descriptor rather than the top-level
originalNavigatorPlatformDescriptor.
- Around line 198-220: The test setup replaces global.navigator with an empty
object which can make useXterm crash when it calls
navigator.platform.toLowerCase(); update the beforeEach navigator stub(s) to
include a safe default platform string (e.g., platform: "unknown" or "") instead
of leaving platform undefined (and apply the same change for the other
navigator-replacement block around lines 228-233); this ensures calls like
navigator.platform.toLowerCase() in useXterm succeed without changing production
code.
- Around line 689-727: Rename the misleading test titles that say "pass through"
to explicitly state interception and the expected output: change "should let
SHIFT+Enter pass through" to something like "should intercept SHIFT+Enter and
send ESC+newline" and change "should let Ctrl+Backspace pass through" to "should
intercept Ctrl+Backspace and send Ctrl+U"; update the test strings (the it(...)
descriptions) in the suite that uses setupMockXterm and keyEventHandler so they
accurately reflect behavior and keep existing assertions/comments intact.
- Around line 95-172: The test helper setupMockXterm currently reassigns
global.ResizeObserver inside the function which conflicts with the global stub
installed in beforeAll and makes per-test state harder to reason about; remove
the ResizeObserver override from setupMockXterm and instead use vi.stubGlobal in
your test setup (and vi.unstubAllGlobals in teardown) to provide the shared
ResizeObserver implementation so setupMockXterm only mocks XTerm and addons
(refer to the setupMockXterm function and the test suite beforeAll/afterAll
hooks), ensuring you also drop any local expectations that relied on the
per-call reassignment.
- Around line 17-59: The Terminal mock for @xterm/xterm lacks the write method
used by useXterm when buffered output exists; update the vi.mock for
"@xterm/xterm" so the mocked Terminal implementation includes a write method
(e.g., write: vi.fn()) so tests no longer rely on terminalBufferManager.get()
returning an empty string to avoid calling write; keep other mocked methods
(open, loadAddon, attachCustomKeyEventHandler, hasSelection, getSelection,
paste, input, onData, onResize, dispose, cols, rows) intact.
- Around line 9-14: The test file imports the unused Mock type and declares
unused variables origRequestAnimationFrame, origCancelAnimationFrame, and
origResizeObserver; remove the unused import "Mock" from the vitest import list
and delete the unused variable declarations (origRequestAnimationFrame,
origCancelAnimationFrame, origResizeObserver) in useXterm.test.ts so there are
no unused symbols left (verify no references to these names remain in the file
before committing).
- Around line 85-90: Replace the manual saving/restoration of globals in the
test file (the unused locals origRequestAnimationFrame,
origCancelAnimationFrame, origResizeObserver) with Vitest's vi.stubGlobal() to
stub requestAnimationFrame, cancelAnimationFrame and ResizeObserver at test
setup and call vi.unstubAllGlobals() in afterAll; remove the unused orig*
variables and any manual global assignments, and apply the same change to the
later block around the other occurrence (lines referenced in the comment) so
tests use vi.stubGlobal()/vi.unstubAllGlobals() idiomatically.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
apps/frontend/src/__tests__/integration/terminal-copy-paste.test.tsapps/frontend/src/main/terminal/claude-integration-handler.tsapps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/__tests__/integration/terminal-copy-paste.test.tsapps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.tsapps/frontend/src/main/terminal/claude-integration-handler.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/__tests__/integration/terminal-copy-paste.test.tsapps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.tsapps/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/__tests__/integration/terminal-copy-paste.test.tsapps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.tsapps/frontend/src/main/terminal/claude-integration-handler.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
📚 Learning: 2026-01-04T23:59:48.743Z
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
Applied to files:
apps/frontend/src/main/terminal/claude-integration-handler.ts
🧬 Code graph analysis (3)
apps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts (1)
apps/frontend/src/renderer/components/terminal/useXterm.ts (1)
useXterm(34-389)
apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts (1)
apps/frontend/src/renderer/components/terminal/useXterm.ts (1)
useXterm(34-389)
apps/frontend/src/main/terminal/claude-integration-handler.ts (3)
apps/frontend/src/shared/utils/shell-escape.ts (7)
ShellType(15-15)escapeShellArg(33-38)escapePowerShellArg(153-158)escapeShellArgWindows(83-98)buildCommandInvocation(353-391)buildCdCommandForShell(417-441)buildPathAssignment(215-239)apps/frontend/src/main/terminal/types.ts (3)
ShellType(8-8)RateLimitEvent(34-42)OAuthTokenEvent(47-54)apps/frontend/src/main/claude-cli-utils.ts (2)
getClaudeCliInvocation(44-52)getClaudeCliInvocationAsync(66-77)
🪛 GitHub Check: test-frontend
apps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts
[warning] 72-72:
'origResizeObserver' is assigned a value but never used. Allowed unused vars must match /^_/u
[warning] 71-71:
'origCancelAnimationFrame' is assigned a value but never used. Allowed unused vars must match /^_/u
[warning] 70-70:
'origRequestAnimationFrame' is assigned a value but never used. Allowed unused vars must match /^_/u
⏰ 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). (2)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (16)
apps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts (5)
9-16: LGTM!Imports are appropriate for the integration test scope, including the xterm types needed for mock type assertions.
74-158: LGTM!The
setupMockXTermWithHandlerhelper is well-documented and provides a clean way to capture the key event handler and customize mock behavior per test. The mutable wrapper pattern effectively solves the timing issue of accessing the handler after component render.
246-337: LGTM!Good integration tests covering both the happy path (selection exists → copy to clipboard) and the guard path (no selection → no clipboard write). The test structure with captured key event handlers accurately simulates user interactions.
339-578: LGTM!Comprehensive coverage of clipboard paste integration across platforms (Win32, Linux) and keyboard event propagation. The tests effectively verify that copy/paste operations don't interfere with other terminal shortcuts like SHIFT+Enter and Ctrl+number keys.
580-658: LGTM!Excellent resilience test that verifies clipboard errors are properly caught and logged without breaking terminal functionality. The test correctly validates that users can continue sending input even after a clipboard operation fails.
apps/frontend/src/main/terminal/claude-integration-handler.ts (11)
6-27: LGTM: Imports are well-organized and appropriate.The new imports support shell-aware command construction, async file operations, token handling, and crypto operations. The shell-escape utilities are correctly imported from the shared module.
52-91: LGTM: Bash detection is well-implemented with appropriate caching.The function correctly:
- Uses module-level caching to avoid redundant checks
- Adds a 1-second timeout to prevent indefinite blocking
- Falls back to common Windows installation paths
- Returns null when bash is unavailable
Note that the first invocation may block for up to 1 second, but this is acceptable since the result is cached and the function is called during command construction rather than on a hot path.
219-222: LGTM: Type safety improvement with discriminated union.The updated
ClaudeCommandConfigtype uses string literal keys and provides excellent type safety for the different invocation methods.
250-266: LGTM: Shell command builder is well-structured.The function correctly uses the discriminated union to ensure type-safe command construction for each method. The escaping is appropriate for each case.
340-500: LGTM: Handler functions updated with consistent logging style.The logging has been standardized to use double-quoted strings throughout. The core logic for rate limit detection, OAuth token handling, and profile switching remains correct.
712-756: LGTM: Resume function correctly updated for shell-aware command construction.The function properly uses
buildCommandInvocationwith the--continueflag and includes appropriate deprecation warnings for the unusedsessionIdparameter.
952-994: LGTM: Async resume function correctly implements non-blocking CLI detection.The async version properly awaits CLI invocation details and uses shell-aware command construction.
1022-1028: LGTM: PowerShell prompt pattern correctly added.The new pattern
/^PS\s+[^\r\n>]+>\s*$/mproperly matches PowerShell prompts likePS C:\path>orPS>, improving cross-shell compatibility.
1036-1106: LGTM: Wait-for-exit logic is robust and well-designed.The function correctly:
- Polls terminal output for shell prompt patterns
- Includes timeout protection (5 seconds default)
- Falls back to checking
isClaudeModeflag- Provides comprehensive debug logging
1193-1238: LGTM: Robust retry logic for profile switching.The enhanced exit sequence with retry logic handles the edge case where Claude doesn't exit promptly:
- Up to 2 retry attempts with explicit exit commands
- Comprehensive logging for debugging
- Graceful degradation (continues with profile switch even if exit times out)
This improves reliability when switching profiles during active Claude sessions.
1115-1132: LGTM: Enhanced tracing for profile switching.The always-on console.warn statements provide valuable diagnostic information for troubleshooting profile switching issues in production without requiring debug mode.
| // Mock requestAnimationFrame for jsdom environment (not provided by default) | ||
| // Save originals to restore in afterAll hook | ||
| const origRequestAnimationFrame = global.requestAnimationFrame; | ||
| const origCancelAnimationFrame = global.cancelAnimationFrame; | ||
| const origResizeObserver = global.ResizeObserver; |
There was a problem hiding this comment.
Remove or prefix unused variables to satisfy linting.
Static analysis flags origRequestAnimationFrame, origCancelAnimationFrame, and origResizeObserver as unused. Since the afterAll hook explicitly states these won't be restored, these variables serve no purpose.
🧹 Proposed fix
-// Mock requestAnimationFrame for jsdom environment (not provided by default)
-// Save originals to restore in afterAll hook
-const origRequestAnimationFrame = global.requestAnimationFrame;
-const origCancelAnimationFrame = global.cancelAnimationFrame;
-const origResizeObserver = global.ResizeObserver;
+// Note: jsdom doesn't provide requestAnimationFrame, cancelAnimationFrame, or ResizeObserver,
+// so we mock them in beforeAll/beforeEach without needing to capture/restore originals.Also remove the empty afterAll block at lines 239-244 since it only contains a comment explaining why restoration isn't done.
📝 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.
| // Mock requestAnimationFrame for jsdom environment (not provided by default) | |
| // Save originals to restore in afterAll hook | |
| const origRequestAnimationFrame = global.requestAnimationFrame; | |
| const origCancelAnimationFrame = global.cancelAnimationFrame; | |
| const origResizeObserver = global.ResizeObserver; | |
| // Note: jsdom doesn't provide requestAnimationFrame, cancelAnimationFrame, or ResizeObserver, | |
| // so we mock them in beforeAll/beforeEach without needing to capture/restore originals. |
🧰 Tools
🪛 GitHub Check: test-frontend
[warning] 72-72:
'origResizeObserver' is assigned a value but never used. Allowed unused vars must match /^_/u
[warning] 71-71:
'origCancelAnimationFrame' is assigned a value but never used. Allowed unused vars must match /^_/u
[warning] 70-70:
'origRequestAnimationFrame' is assigned a value but never used. Allowed unused vars must match /^_/u
🤖 Prompt for AI Agents
In @apps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts around
lines 68 - 72, The three saved globals origRequestAnimationFrame,
origCancelAnimationFrame, and origResizeObserver are declared but never used;
either remove these declarations or prefix them with an underscore (e.g.,
_origRequestAnimationFrame) to satisfy the linter, and also delete the empty
afterAll block that only contains a comment (it serves no runtime/test purpose).
Ensure you update any related comments so there are no leftover references to
those variables or the no-op afterAll hook.
| beforeAll(() => { | ||
| // Set up global mocks once for all tests | ||
| // These persist across tests but are not affected by vi.clearAllMocks() | ||
| global.requestAnimationFrame = vi.fn( | ||
| (cb: FrameRequestCallback) => setTimeout(cb, 0) as unknown as number | ||
| ); | ||
| global.cancelAnimationFrame = vi.fn((id: unknown) => clearTimeout(id as number)); | ||
|
|
||
| // Mock ResizeObserver | ||
| global.ResizeObserver = vi.fn().mockImplementation(function () { | ||
| return { | ||
| observe: vi.fn(), | ||
| unobserve: vi.fn(), | ||
| disconnect: vi.fn(), | ||
| }; | ||
| }); | ||
| }); | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
|
|
||
| // Mock requestAnimationFrame | ||
| global.requestAnimationFrame = vi.fn( | ||
| (cb: FrameRequestCallback) => setTimeout(cb, 0) as unknown as number | ||
| ); | ||
| global.cancelAnimationFrame = vi.fn((id: unknown) => clearTimeout(id as number)); | ||
|
|
||
| // Mock ResizeObserver | ||
| global.ResizeObserver = vi.fn().mockImplementation(function() { | ||
| global.ResizeObserver = vi.fn().mockImplementation(function () { | ||
| return { | ||
| observe: vi.fn(), | ||
| unobserve: vi.fn(), | ||
| disconnect: vi.fn() | ||
| disconnect: vi.fn(), | ||
| }; | ||
| }); | ||
|
|
||
| // Mock navigator.clipboard | ||
| mockClipboard = { | ||
| writeText: vi.fn().mockResolvedValue(undefined), | ||
| readText: vi.fn().mockResolvedValue('clipboard content') | ||
| readText: vi.fn().mockResolvedValue("clipboard content"), | ||
| }; | ||
|
|
||
| Object.defineProperty(global.navigator, 'clipboard', { | ||
| Object.defineProperty(global.navigator, "clipboard", { | ||
| value: mockClipboard, | ||
| writable: true | ||
| writable: true, | ||
| }); | ||
|
|
||
| // Mock window.electronAPI | ||
| (window as unknown as { electronAPI: unknown }).electronAPI = { | ||
| sendTerminalInput: vi.fn() | ||
| sendTerminalInput: vi.fn(), | ||
| }; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| vi.restoreAllMocks(); | ||
|
|
||
| // Re-apply global mocks for pending async callbacks (setTimeout, etc.) | ||
| // vi.restoreAllMocks() restores the original undefined values, but pending | ||
| // callbacks from the component (e.g., performInitialFit via setTimeout) may | ||
| // still need these mocks after the test completes. | ||
| global.requestAnimationFrame = vi.fn( | ||
| (cb: FrameRequestCallback) => setTimeout(cb, 0) as unknown as number | ||
| ); | ||
| global.cancelAnimationFrame = vi.fn((id: unknown) => clearTimeout(id as number)); | ||
| global.ResizeObserver = vi.fn().mockImplementation(function () { | ||
| return { | ||
| observe: vi.fn(), | ||
| unobserve: vi.fn(), | ||
| disconnect: vi.fn(), | ||
| }; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider consolidating duplicate mock setup.
The same requestAnimationFrame, cancelAnimationFrame, and ResizeObserver mocks are defined in beforeAll, beforeEach, and afterEach. While the current approach works, the duplication could be reduced by extracting the mock setup to a shared function.
♻️ Proposed refactor
+// Extract shared mock setup to avoid duplication
+function setupGlobalMocks() {
+ global.requestAnimationFrame = vi.fn(
+ (cb: FrameRequestCallback) => setTimeout(cb, 0) as unknown as number
+ );
+ global.cancelAnimationFrame = vi.fn((id: unknown) => clearTimeout(id as number));
+ global.ResizeObserver = vi.fn().mockImplementation(function () {
+ return {
+ observe: vi.fn(),
+ unobserve: vi.fn(),
+ disconnect: vi.fn(),
+ };
+ });
+}
describe("Terminal copy/paste integration", () => {
// ...
beforeAll(() => {
- global.requestAnimationFrame = vi.fn(
- (cb: FrameRequestCallback) => setTimeout(cb, 0) as unknown as number
- );
- global.cancelAnimationFrame = vi.fn((id: unknown) => clearTimeout(id as number));
-
- global.ResizeObserver = vi.fn().mockImplementation(function () {
- return {
- observe: vi.fn(),
- unobserve: vi.fn(),
- disconnect: vi.fn(),
- };
- });
+ setupGlobalMocks();
});
beforeEach(() => {
vi.clearAllMocks();
-
- global.requestAnimationFrame = vi.fn(
- (cb: FrameRequestCallback) => setTimeout(cb, 0) as unknown as number
- );
- global.cancelAnimationFrame = vi.fn((id: unknown) => clearTimeout(id as number));
-
- global.ResizeObserver = vi.fn().mockImplementation(function () {
- return {
- observe: vi.fn(),
- unobserve: vi.fn(),
- disconnect: vi.fn(),
- };
- });
+ setupGlobalMocks();
// ... rest of beforeEach
});
afterEach(() => {
vi.restoreAllMocks();
-
- global.requestAnimationFrame = vi.fn(
- (cb: FrameRequestCallback) => setTimeout(cb, 0) as unknown as number
- );
- global.cancelAnimationFrame = vi.fn((id: unknown) => clearTimeout(id as number));
- global.ResizeObserver = vi.fn().mockImplementation(function () {
- return {
- observe: vi.fn(),
- unobserve: vi.fn(),
- disconnect: vi.fn(),
- };
- });
+ // Re-apply for pending async callbacks
+ setupGlobalMocks();
});🤖 Prompt for AI Agents
In @apps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts around
lines 166 - 237, Duplicate global mocks for requestAnimationFrame,
cancelAnimationFrame, and ResizeObserver are repeated in beforeAll, beforeEach,
and afterEach; extract that repeated code into a single helper (e.g.,
setupGlobalMocks) that assigns global.requestAnimationFrame,
global.cancelAnimationFrame, and global.ResizeObserver, then call
setupGlobalMocks() from beforeAll, beforeEach (before other per-test mocks like
mockClipboard and window.electronAPI), and afterEach (to re-apply mocks for
pending async callbacks) and remove the duplicated blocks; keep per-test setup
for mockClipboard and window.electronAPI in beforeEach only.
| if (token && tempFile) { | ||
| if (shellType === "powershell" || shellType === "cmd") { | ||
| // On Windows, check if bash is available | ||
| const bashPath = getBashPath(); | ||
| if (bashPath) { | ||
| // Use bash-style temp file with bash -c wrapper | ||
| const tempFileBash = tempFile.replace(/\\/g, "/"); | ||
| const bashClaudeCmd = claudeCmd.replace(/\\/g, "/"); | ||
| const escapedClaudeBash = escapeShellArg(bashClaudeCmd); | ||
| const escapedTempFileBash = escapeShellArg(tempFileBash); | ||
| // Use the discovered bash path (may be absolute path on Windows) | ||
| const bashExe = bashPath === "bash" ? "bash" : bashPath.replace(/\\/g, "/"); | ||
|
|
||
| // Both PowerShell and cmd.exe use the same bash command | ||
| return `${cwdCommand}${pathPrefix}${bashExe} -c "source ${escapedTempFileBash} && rm -f ${escapedTempFileBash} && exec ${escapedClaudeBash}"\r`; | ||
| } else { | ||
| // Bash not available - use native PowerShell/cmd syntax | ||
| // | ||
| // SECURITY TRADE-OFF: When bash is unavailable, the OAuth token is embedded | ||
| // directly in the shell command string (e.g., `$env:CLAUDE_CODE_OAUTH_TOKEN=...` | ||
| // or `set "CLAUDE_CODE_OAUTH_TOKEN=..."`). This means the token may be visible | ||
| // in process listings (e.g., Task Manager, Process Explorer, ps.exe) during | ||
| // command execution. The token is NOT written to shell history files. | ||
| // | ||
| // This is an acceptable fallback for Windows systems without bash because: | ||
| // 1. Process listings are transient (cleared on process exit) | ||
| // 2. The exposure window is brief (command executes immediately) | ||
| // 3. Alternative approaches (e.g., temp script files) have their own trade-offs | ||
| // | ||
| // For better security, users should install Git for Windows or WSL to provide bash. | ||
| if (shellType === "powershell") { | ||
| // PowerShell: set environment variable, invoke claude, then remove temp file | ||
| const escapedToken = escapePowerShellArg(token); | ||
| const escapedClaude = escapePowerShellArg(claudeCmd); | ||
| const escapedTempFile = escapePowerShellArg(tempFile); | ||
| return `Clear-Host; ${cwdCommand}$env:CLAUDE_CODE_OAUTH_TOKEN=${escapedToken}; ${pathPrefix}${escapedClaude}; Remove-Item -Force ${escapedTempFile}\r`; | ||
| } else { | ||
| // cmd.exe: set environment variable, invoke claude, then remove temp file | ||
| const escapedToken = escapeShellArgWindows(token); | ||
| const escapedClaudeCmdWin = escapeShellArgWindows(claudeCmd); | ||
| const escapedTempFileWin = escapeShellArgWindows(tempFile); | ||
| return `cls && ${cwdCommand}set "CLAUDE_CODE_OAUTH_TOKEN=${escapedToken}" && ${pathPrefix}"${escapedClaudeCmdWin}" && del /f "${escapedTempFileWin}"\r`; | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Temp file cleanup is not guaranteed if command fails or is interrupted.
The OAuth token temp file is deleted only if the shell command executes successfully and completes. If the user presses Ctrl+C, the terminal is closed, or the command fails to execute, the temp file persists on disk with the sensitive OAuth token.
Recommended solution: Use a background cleanup mechanism (e.g., setTimeout to delete after a reasonable timeout, or track temp files and clean them up on app exit).
🔒 Add background cleanup for temp files
Consider adding a cleanup mechanism:
// At module level
const pendingTempFiles = new Set<string>();
// In buildProfileCommandString, after creating temp file:
pendingTempFiles.add(tempFile);
// Schedule cleanup after reasonable timeout (e.g., 30 seconds)
setTimeout(() => {
if (pendingTempFiles.has(tempFile)) {
fsPromises.unlink(tempFile).catch(() => {/* already deleted or error */});
pendingTempFiles.delete(tempFile);
}
}, 30000);
// After successful execution, remove from tracking:
// (in invokeClaude/invokeClaudeAsync after terminal.pty.write)
pendingTempFiles.delete(tempFile);This ensures tokens are cleaned up even if the command is interrupted.
| if (bashPath) { | ||
| // Use bash-style temp file with bash -c wrapper | ||
| const tempFileBash = tempFile.replace(/\\/g, "/"); | ||
| const bashClaudeCmd = claudeCmd.replace(/\\/g, "/"); | ||
| const escapedClaudeBash = escapeShellArg(bashClaudeCmd); | ||
| const escapedTempFileBash = escapeShellArg(tempFileBash); | ||
| // Use the discovered bash path (may be absolute path on Windows) | ||
| const bashExe = bashPath === "bash" ? "bash" : bashPath.replace(/\\/g, "/"); | ||
|
|
||
| // Both PowerShell and cmd.exe use the same bash command | ||
| return `${cwdCommand}${pathPrefix}${bashExe} -c "source ${escapedTempFileBash} && rm -f ${escapedTempFileBash} && exec ${escapedClaudeBash}"\r`; |
There was a problem hiding this comment.
Major: Bash executable path may not be properly quoted when it contains spaces.
Line 152 constructs bashExe from the absolute bash path but doesn't ensure it's quoted when used in the shell command at line 155. If the bash installation path contains spaces (e.g., C:\My Tools\Git\bin\bash.exe), the command will fail.
🛠️ Properly quote bash executable path
const bashExe = bashPath === "bash" ? "bash" : bashPath.replace(/\\/g, "/");
// Both PowerShell and cmd.exe use the same bash command
- return `${cwdCommand}${pathPrefix}${bashExe} -c "source ${escapedTempFileBash} && rm -f ${escapedTempFileBash} && exec ${escapedClaudeBash}"\r`;
+ const quotedBashExe = bashExe.includes(" ") ? `"${bashExe}"` : bashExe;
+ return `${cwdCommand}${pathPrefix}${quotedBashExe} -c "source ${escapedTempFileBash} && rm -f ${escapedTempFileBash} && exec ${escapedClaudeBash}"\r`;Or use the appropriate shell escaping function for PowerShell/cmd based on shellType.
📝 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.
| if (bashPath) { | |
| // Use bash-style temp file with bash -c wrapper | |
| const tempFileBash = tempFile.replace(/\\/g, "/"); | |
| const bashClaudeCmd = claudeCmd.replace(/\\/g, "/"); | |
| const escapedClaudeBash = escapeShellArg(bashClaudeCmd); | |
| const escapedTempFileBash = escapeShellArg(tempFileBash); | |
| // Use the discovered bash path (may be absolute path on Windows) | |
| const bashExe = bashPath === "bash" ? "bash" : bashPath.replace(/\\/g, "/"); | |
| // Both PowerShell and cmd.exe use the same bash command | |
| return `${cwdCommand}${pathPrefix}${bashExe} -c "source ${escapedTempFileBash} && rm -f ${escapedTempFileBash} && exec ${escapedClaudeBash}"\r`; | |
| if (bashPath) { | |
| // Use bash-style temp file with bash -c wrapper | |
| const tempFileBash = tempFile.replace(/\\/g, "/"); | |
| const bashClaudeCmd = claudeCmd.replace(/\\/g, "/"); | |
| const escapedClaudeBash = escapeShellArg(bashClaudeCmd); | |
| const escapedTempFileBash = escapeShellArg(tempFileBash); | |
| // Use the discovered bash path (may be absolute path on Windows) | |
| const bashExe = bashPath === "bash" ? "bash" : bashPath.replace(/\\/g, "/"); | |
| // Both PowerShell and cmd.exe use the same bash command | |
| const quotedBashExe = bashExe.includes(" ") ? `"${bashExe}"` : bashExe; | |
| return `${cwdCommand}${pathPrefix}${quotedBashExe} -c "source ${escapedTempFileBash} && rm -f ${escapedTempFileBash} && exec ${escapedClaudeBash}"\r`; | |
| } |
🤖 Prompt for AI Agents
In @apps/frontend/src/main/terminal/claude-integration-handler.ts around lines
145 - 155, The constructed command uses bashExe directly which can break when
bashPath contains spaces; wrap or escape bashExe before interpolating into the
returned command (e.g., apply the existing escapeShellArg to bashExe or quote it
appropriately) so the final string built in the return (`${bashExe} -c "..."`)
uses a safely quoted/escaped executable name; update the logic around bashExe
(set via bashPath === "bash" ? "bash" : bashPath.replace(/\\/g, "/")) and reuse
escapeShellArg(bashExe) (or conditionally quote based on shellType) when
building the command.
| const nonce = crypto.randomBytes(8).toString("hex"); | ||
| const tempFile = path.join(os.tmpdir(), `.claude-token-${Date.now()}-${nonce}`); | ||
| const escapedTempFile = escapeShellArg(tempFile); | ||
| debugLog('[ClaudeIntegration:invokeClaude] Writing token to temp file:', tempFile); | ||
|
|
||
| debugLog("[ClaudeIntegration:invokeClaude] Writing token to temp file:", tempFile); | ||
| // Write token file with Unix-only POSIX file permissions - mode 0o600 (owner read/write only) | ||
| // is a POSIX-specific permission bit that is not applicable on Windows; Node.js on Windows | ||
| // ignores POSIX mode flags, so we only set writeOptions.mode when process.platform !== "win32" | ||
| const writeOptions: fs.WriteFileOptions = {}; | ||
| if (process.platform !== "win32") { | ||
| writeOptions.mode = 0o600; | ||
| } | ||
| fs.writeFileSync( | ||
| tempFile, | ||
| `export CLAUDE_CODE_OAUTH_TOKEN=${escapeShellArg(token)}\n`, | ||
| { mode: 0o600 } | ||
| writeOptions | ||
| ); | ||
|
|
||
| const command = buildClaudeShellCommand(cwdCommand, pathPrefix, escapedClaudeCmd, { method: 'temp-file', escapedTempFile }); | ||
| debugLog('[ClaudeIntegration:invokeClaude] Executing command (temp file method, history-safe)'); | ||
| terminal.pty.write(command); | ||
| profileManager.markProfileUsed(activeProfile.id); | ||
| finalizeClaudeInvoke(terminal, activeProfile, projectPath, startTime, getWindow, onSessionCapture); | ||
| debugLog('[ClaudeIntegration:invokeClaude] ========== INVOKE CLAUDE COMPLETE (temp file) =========='); | ||
| return; | ||
| } else if (activeProfile.configDir) { | ||
| const escapedConfigDir = escapeShellArg(activeProfile.configDir); | ||
| const command = buildClaudeShellCommand(cwdCommand, pathPrefix, escapedClaudeCmd, { method: 'config-dir', escapedConfigDir }); | ||
| debugLog('[ClaudeIntegration:invokeClaude] Executing command (configDir method, history-safe)'); | ||
| terminal.pty.write(command); | ||
| profileManager.markProfileUsed(activeProfile.id); | ||
| finalizeClaudeInvoke(terminal, activeProfile, projectPath, startTime, getWindow, onSessionCapture); | ||
| debugLog('[ClaudeIntegration:invokeClaude] ========== INVOKE CLAUDE COMPLETE (configDir) =========='); | ||
| return; | ||
| // Build shell-specific command for temp file method using shared helper | ||
| const command = buildProfileCommandString({ | ||
| shellType, | ||
| cwdCommand, | ||
| pathPrefix, | ||
| claudeCmd, | ||
| token, | ||
| tempFile, | ||
| }); | ||
|
|
||
| if (command) { | ||
| debugLog( | ||
| "[ClaudeIntegration:invokeClaude] Executing command (temp file method, history-safe)" | ||
| ); | ||
| terminal.pty.write(command); | ||
| profileManager.markProfileUsed(activeProfile.id); | ||
| finalizeClaudeInvoke( | ||
| terminal, | ||
| activeProfile, | ||
| projectPath, | ||
| startTime, | ||
| getWindow, | ||
| onSessionCapture | ||
| ); | ||
| debugLog( | ||
| "[ClaudeIntegration:invokeClaude] ========== INVOKE CLAUDE COMPLETE (temp file) ==========" | ||
| ); | ||
| return; | ||
| } | ||
| // If command is null, fall through to default method | ||
| } |
There was a problem hiding this comment.
Critical: Temp file is not cleaned up if buildProfileCommandString returns null.
If buildProfileCommandString returns null at line 616 (indicating default method should be used), the function falls through to the default method (line 679), but the temp file created at lines 600-604 is never deleted. This leaves the OAuth token on disk.
🔒 Clean up temp file before falling through
if (command) {
debugLog(
"[ClaudeIntegration:invokeClaude] Executing command (temp file method, history-safe)"
);
terminal.pty.write(command);
profileManager.markProfileUsed(activeProfile.id);
finalizeClaudeInvoke(
terminal,
activeProfile,
projectPath,
startTime,
getWindow,
onSessionCapture
);
debugLog(
"[ClaudeIntegration:invokeClaude] ========== INVOKE CLAUDE COMPLETE (temp file) =========="
);
return;
+ } else {
+ // Clean up temp file if command building failed
+ try {
+ fs.unlinkSync(tempFile);
+ } catch (err) {
+ debugError("[ClaudeIntegration:invokeClaude] Failed to clean up temp file:", err);
+ }
}
// If command is null, fall through to default method
}Apply the same fix in the invokeClaudeAsync function around line 879.
| // Mock navigator.platform for platform detection | ||
| const originalNavigatorPlatform = navigator.platform; | ||
| // Capture the full property descriptor to properly restore it later | ||
| const originalNavigatorPlatformDescriptor = Object.getOwnPropertyDescriptor( | ||
| navigator, | ||
| "platform" | ||
| ) ?? { | ||
| value: navigator.platform, | ||
| writable: true, | ||
| configurable: true, | ||
| enumerable: true, | ||
| }; | ||
|
|
||
| /** | ||
| * Helper function to set navigator.platform with configurable:true | ||
| * to avoid poisoning the property descriptor across tests | ||
| */ | ||
| function setNavigatorPlatform(value: string): void { | ||
| Object.defineProperty(navigator, "platform", { | ||
| value, | ||
| configurable: true, | ||
| writable: true, | ||
| }); | ||
| } |
There was a problem hiding this comment.
navigator.platform descriptor capture is fragile at module-load time.
You capture navigator.platform’s descriptor at import time. If this test ever runs in a non-jsdom environment (or navigator is temporarily undefined), it will throw before beforeEach can patch anything. Consider computing this in beforeAll (or guarding via globalThis.navigator?.).
🤖 Prompt for AI Agents
In @apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts
around lines 61 - 83, The module currently captures navigator.platform at import
time via originalNavigatorPlatformDescriptor which can throw if navigator is
undefined; move the descriptor capture into a safe lifecycle hook (e.g.,
beforeAll) or guard with globalThis.navigator?. to avoid errors at module-load
time, and keep the helper setNavigatorPlatform as-is but ensure any restore
logic uses the descriptor captured in beforeAll; update tests to rely on the
runtime-captured descriptor rather than the top-level
originalNavigatorPlatformDescriptor.
| // Mock requestAnimationFrame for jsdom environment (not provided by default) | ||
| global.requestAnimationFrame = vi.fn((cb: FrameRequestCallback) => setTimeout(cb, 0) as unknown as number); | ||
| // Save originals to restore in afterAll hook | ||
| const origRequestAnimationFrame = global.requestAnimationFrame; | ||
| const origCancelAnimationFrame = global.cancelAnimationFrame; | ||
| const origResizeObserver = global.ResizeObserver; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the relevant lines
file_path="apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts"
if [ -f "$file_path" ]; then
echo "=== Lines 85-95 ==="
sed -n '85,95p' "$file_path"
echo ""
echo "=== Lines 235-245 ==="
sed -n '235,245p' "$file_path"
else
echo "File not found"
fiRepository: AndyMik90/Auto-Claude
Length of output: 1118
🏁 Script executed:
# Check the entire file structure to understand context
wc -l apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 142
🏁 Script executed:
# Search for vi.stubGlobal usage patterns in the codebase to see best practices
rg "vi\.stubGlobal|vi\.unstubAllGlobals" --type ts --type tsxRepository: AndyMik90/Auto-Claude
Length of output: 92
🏁 Script executed:
# Search for beforeAll/afterAll in this test file to understand the structure
rg "beforeAll|afterAll" "apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts" -A 5 -B 2Repository: AndyMik90/Auto-Claude
Length of output: 1439
🏁 Script executed:
# Verify Vitest API - search for vi.stubGlobal or vi.unstubAllGlobals usage in codebase
rg "stubGlobal|unstubAllGlobals" --type ts 2>/dev/null || echo "Pattern not found in codebase"Repository: AndyMik90/Auto-Claude
Length of output: 94
🌐 Web query:
Vitest vi.stubGlobal unstubAllGlobals API documentation
💡 Result:
vi.stubGlobal(name, value)
- Changes a global (on globalThis / global and window/top/self/parent in jsdom/happy-dom).
- Signature: stubGlobal(name: string | number | symbol, value: unknown) => Vitest.
- Restored by vi.unstubAllGlobals. (vitest.dev)
vi.unstubAllGlobals()
- Restores all globals that were changed via vi.stubGlobal.
- Signature: unstubAllGlobals(): Vitest (or () => Vitest in some versions). Vitest remembers original values on first call and restores them. (vitest.dev)
Official docs (primary reference): Vitest API — vi (see "vi.stubGlobal" and "vi.unstubAllGlobals"). (vitest.dev)
If you want direct examples or version-specific behavior (v0.x / v1 / v2+), tell me which Vitest version you use and I’ll paste the exact snippet.
Citations:
- 1: https://vitest.dev/api/vi?utm_source=openai
- 2: https://vitest.dev/api/vi?utm_source=openai
- 3: https://vitest.dev/api/vi?utm_source=openai
Use vi.stubGlobal() + vi.unstubAllGlobals() instead of manual global assignment.
The saved origRequestAnimationFrame, origCancelAnimationFrame, and origResizeObserver are unused locals and will trigger linting warnings. While the intentional non-restoration is documented (jsdom doesn't provide these APIs natively), using Vitest's built-in vi.stubGlobal() with vi.unstubAllGlobals() in the afterAll hook is the cleaner, more idiomatic pattern and eliminates the unused-locals issue entirely.
Suggested refactor
-// Mock requestAnimationFrame for jsdom environment (not provided by default)
-// Save originals to restore in afterAll hook
-const origRequestAnimationFrame = global.requestAnimationFrame;
-const origCancelAnimationFrame = global.cancelAnimationFrame;
-const origResizeObserver = global.ResizeObserver;
+// Mock requestAnimationFrame and ResizeObserver using Vitest's stubGlobal beforeAll(() => {
- global.requestAnimationFrame = vi.fn(
+ vi.stubGlobal("requestAnimationFrame", vi.fn(
(cb: FrameRequestCallback) => setTimeout(cb, 0) as unknown as number
- );
- global.cancelAnimationFrame = vi.fn((id: unknown) => clearTimeout(id as number));
+ ));
+ vi.stubGlobal("cancelAnimationFrame", vi.fn((id: unknown) => clearTimeout(id as number)));
- // Mock ResizeObserver
- global.ResizeObserver = vi.fn().mockImplementation(function () {
+ vi.stubGlobal("ResizeObserver", vi.fn().mockImplementation(function () {
return {
observe: vi.fn(),
unobserve: vi.fn(),
disconnect: vi.fn(),
};
- });
+ }));
});
afterAll(() => {
- // Note: We intentionally do NOT restore the original global shims ...
+ vi.unstubAllGlobals();
});Also applies to: 235-240
🤖 Prompt for AI Agents
In @apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts
around lines 85 - 90, Replace the manual saving/restoration of globals in the
test file (the unused locals origRequestAnimationFrame,
origCancelAnimationFrame, origResizeObserver) with Vitest's vi.stubGlobal() to
stub requestAnimationFrame, cancelAnimationFrame and ResizeObserver at test
setup and call vi.unstubAllGlobals() in afterAll; remove the unused orig*
variables and any manual global assignments, and apply the same change to the
later block around the other occurrence (lines referenced in the comment) so
tests use vi.stubGlobal()/vi.unstubAllGlobals() idiomatically.
| async function setupMockXterm( | ||
| overrides: { | ||
| hasSelection?: () => boolean; | ||
| getSelection?: () => string; | ||
| paste?: ReturnType<typeof vi.fn>; | ||
| input?: ReturnType<typeof vi.fn>; | ||
| } = {} | ||
| ): Promise<{ | ||
| keyEventHandler: (event: KeyboardEvent) => boolean; | ||
| }> { | ||
| let keyEventHandler: ((event: KeyboardEvent) => boolean) | null = null; | ||
|
|
||
| // Override XTerm mock to be constructable | ||
| (XTerm as unknown as Mock).mockImplementation(function() { | ||
| vi.mocked(XTerm).mockImplementation(function () { | ||
| return { | ||
| open: vi.fn(), | ||
| loadAddon: vi.fn(), | ||
| attachCustomKeyEventHandler: vi.fn((handler: (event: KeyboardEvent) => boolean) => { | ||
| keyEventHandler = handler; | ||
| }), | ||
| hasSelection: overrides.hasSelection ?? vi.fn(() => false), | ||
| getSelection: overrides.getSelection ?? vi.fn(() => ''), | ||
| getSelection: overrides.getSelection ?? vi.fn(() => ""), | ||
| paste: overrides.paste ?? vi.fn(), | ||
| input: overrides.input ?? vi.fn(), | ||
| onData: vi.fn(), | ||
| onResize: vi.fn(), | ||
| dispose: vi.fn(), | ||
| write: vi.fn(), | ||
| cols: 80, | ||
| rows: 24 | ||
| rows: 24, | ||
| }; | ||
| }); | ||
|
|
||
| // Setup addon mocks | ||
| const { FitAddon } = await import('@xterm/addon-fit'); | ||
| (FitAddon as unknown as Mock).mockImplementation(function() { | ||
| const { FitAddon } = await import("@xterm/addon-fit"); | ||
| vi.mocked(FitAddon).mockImplementation(function () { | ||
| return { fit: vi.fn() }; | ||
| }); | ||
|
|
||
| const { WebLinksAddon } = await import('@xterm/addon-web-links'); | ||
| (WebLinksAddon as unknown as Mock).mockImplementation(function() { | ||
| const { WebLinksAddon } = await import("@xterm/addon-web-links"); | ||
| vi.mocked(WebLinksAddon).mockImplementation(function () { | ||
| return {}; | ||
| }); | ||
|
|
||
| const { SerializeAddon } = await import('@xterm/addon-serialize'); | ||
| (SerializeAddon as unknown as Mock).mockImplementation(function() { | ||
| const { SerializeAddon } = await import("@xterm/addon-serialize"); | ||
| vi.mocked(SerializeAddon).mockImplementation(function () { | ||
| return { | ||
| serialize: vi.fn(() => ''), | ||
| dispose: vi.fn() | ||
| serialize: vi.fn(() => ""), | ||
| dispose: vi.fn(), | ||
| }; | ||
| }); | ||
|
|
||
| // Mock ResizeObserver | ||
| global.ResizeObserver = vi.fn().mockImplementation(function() { | ||
| global.ResizeObserver = vi.fn().mockImplementation(function () { | ||
| return { | ||
| observe: vi.fn(), | ||
| unobserve: vi.fn(), | ||
| disconnect: vi.fn() | ||
| disconnect: vi.fn(), | ||
| }; | ||
| }); | ||
|
|
||
| // Create and render test wrapper component | ||
| const TestWrapper = () => { | ||
| const { terminalRef } = useXterm({ terminalId: 'test-terminal' }); | ||
| return React.createElement('div', { ref: terminalRef }); | ||
| const { terminalRef } = useXterm({ terminalId: "test-terminal" }); | ||
| return React.createElement("div", { ref: terminalRef }); | ||
| }; | ||
|
|
||
| render(React.createElement(TestWrapper)); | ||
|
|
||
| // After rendering, keyEventHandler is guaranteed to be set by attachCustomKeyEventHandler | ||
| // Use non-null assertion since we know the hook will set it | ||
| // Fail fast with a clear error if the hook stops attaching the handler | ||
| if (!keyEventHandler) { | ||
| throw new Error("useXterm did not attach a custom key event handler"); | ||
| } | ||
| return { | ||
| keyEventHandler: keyEventHandler!, | ||
| mockInstance: { | ||
| hasSelection: overrides.hasSelection, | ||
| getSelection: overrides.getSelection, | ||
| paste: overrides.paste, | ||
| input: overrides.input | ||
| } | ||
| keyEventHandler, // No non-null assertion needed - guaranteed non-null by check above | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
setupMockXterm() is a good consolidation; consider scoping the ResizeObserver override.
The “fail fast if handler not attached” check is great and will prevent silent false-positives. The only concern: setupMockXterm() reassigns global.ResizeObserver even though you already install a global stub in beforeAll, which makes per-test state harder to reason about (and interacts with vi.clearAllMocks() behavior). If you adopt vi.stubGlobal/vi.unstubAllGlobals, you can drop the per-test reassignment and keep ResizeObserver’s implementation in one place.
🤖 Prompt for AI Agents
In @apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts
around lines 95 - 172, The test helper setupMockXterm currently reassigns
global.ResizeObserver inside the function which conflicts with the global stub
installed in beforeAll and makes per-test state harder to reason about; remove
the ResizeObserver override from setupMockXterm and instead use vi.stubGlobal in
your test setup (and vi.unstubAllGlobals in teardown) to provide the shared
ResizeObserver implementation so setupMockXterm only mocks XTerm and addons
(refer to the setupMockXterm function and the test suite beforeAll/afterAll
hooks), ensuring you also drop any local expectations that relied on the
per-call reassignment.
| beforeEach(() => { | ||
| // Clear all mocks before each test | ||
| vi.clearAllMocks(); | ||
|
|
||
| // Ensure window and navigator exist in test environment | ||
| if (typeof window === 'undefined') { | ||
| if (typeof window === "undefined") { | ||
| (global as { window: unknown }).window = {}; | ||
| } | ||
| if (typeof navigator === 'undefined') { | ||
| if (typeof navigator === "undefined") { | ||
| (global as { navigator: unknown }).navigator = {}; | ||
| } | ||
|
|
||
| // Mock navigator.clipboard | ||
| mockClipboard = { | ||
| writeText: vi.fn().mockResolvedValue(undefined), | ||
| readText: vi.fn().mockResolvedValue('test clipboard content') | ||
| readText: vi.fn().mockResolvedValue("test clipboard content"), | ||
| }; | ||
|
|
||
| Object.defineProperty(global.navigator, 'clipboard', { | ||
| Object.defineProperty(global.navigator, "clipboard", { | ||
| value: mockClipboard, | ||
| writable: true, | ||
| configurable: true | ||
| configurable: true, | ||
| }); |
There was a problem hiding this comment.
If you ever hit the “navigator undefined” branch, useXterm will crash on navigator.platform.toLowerCase().
beforeEach can set (global as { navigator: unknown }).navigator = {}. If that ever happens, navigator.platform becomes undefined, and useXterm’s fallback path will throw when calling .toLowerCase(). Consider ensuring a default platform value in that branch (or avoid replacing navigator entirely).
Also applies to: 228-233
🤖 Prompt for AI Agents
In @apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts
around lines 198 - 220, The test setup replaces global.navigator with an empty
object which can make useXterm crash when it calls
navigator.platform.toLowerCase(); update the beforeEach navigator stub(s) to
include a safe default platform string (e.g., platform: "unknown" or "") instead
of leaving platform undefined (and apply the same change for the other
navigator-replacement block around lines 228-233); this ensures calls like
navigator.platform.toLowerCase() in useXterm succeed without changing production
code.
| describe("Existing shortcuts preservation", () => { | ||
| it("should let SHIFT+Enter pass through", async () => { | ||
| const mockInput = vi.fn(); | ||
|
|
||
| const { keyEventHandler } = await setupMockXterm({ input: mockInput }); | ||
|
|
||
| await act(async () => { | ||
| const event = new KeyboardEvent('keydown', { | ||
| key: 'Enter', | ||
| const event = new KeyboardEvent("keydown", { | ||
| key: "Enter", | ||
| shiftKey: true, | ||
| ctrlKey: false, | ||
| metaKey: false | ||
| metaKey: false, | ||
| }); | ||
|
|
||
| if (keyEventHandler) { | ||
| keyEventHandler!(event); | ||
| } | ||
| keyEventHandler(event); | ||
| }); | ||
|
|
||
| // Should send ESC+newline for multi-line input | ||
| expect(mockInput).toHaveBeenCalledWith('\x1b\n'); | ||
| expect(mockInput).toHaveBeenCalledWith("\x1b\n"); | ||
| }); | ||
|
|
||
| it('should let Ctrl+Backspace pass through', async () => { | ||
| it("should let Ctrl+Backspace pass through", async () => { | ||
| const mockInput = vi.fn(); | ||
|
|
||
| const { keyEventHandler } = await setupMockXterm({ input: mockInput }); | ||
|
|
||
| await act(async () => { | ||
| const event = new KeyboardEvent('keydown', { | ||
| key: 'Backspace', | ||
| const event = new KeyboardEvent("keydown", { | ||
| key: "Backspace", | ||
| ctrlKey: true, | ||
| metaKey: false | ||
| metaKey: false, | ||
| }); | ||
|
|
||
| if (keyEventHandler) { | ||
| keyEventHandler!(event); | ||
| } | ||
| keyEventHandler(event); | ||
| }); | ||
|
|
||
| // Should send Ctrl+U for delete line | ||
| expect(mockInput).toHaveBeenCalledWith('\x15'); | ||
| expect(mockInput).toHaveBeenCalledWith("\x15"); | ||
| }); |
There was a problem hiding this comment.
Test names don’t match behavior (“pass through”).
SHIFT+Enter and Ctrl+Backspace are explicitly intercepted (sending \x1b\n and \x15 and returning false), so “pass through” is misleading. Rename to reflect the interception to reduce confusion when debugging failures.
🤖 Prompt for AI Agents
In @apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts
around lines 689 - 727, Rename the misleading test titles that say "pass
through" to explicitly state interception and the expected output: change
"should let SHIFT+Enter pass through" to something like "should intercept
SHIFT+Enter and send ESC+newline" and change "should let Ctrl+Backspace pass
through" to "should intercept Ctrl+Backspace and send Ctrl+U"; update the test
strings (the it(...) descriptions) in the suite that uses setupMockXterm and
keyEventHandler so they accurately reflect behavior and keep existing
assertions/comments intact.
|
Closing as requested - work will be handled differently |
StillKnotKnown
deleted the
stillknotknown/acs-176-claude-code-detection-fails-when-username-contains-spaces
branch
|
This PR has deviated too much from the original PR and changes as a result of merge conflicts and additional context that crept into scope. WIll start fresh cycle on this with new PR |
3 participants
Base Branch
developbranch (required for all feature/fix PRs)main(hotfix only - maintainers)Description
Fixes ACS-176. On Windows, when the username or path contains spaces (e.g.,
C:\Users\Jane Smith\AppData\Roaming\npm\claude.cmd), Claude CLI detection and execution failed in multiple places. This PR also addresses several security vulnerabilities and PowerShell compatibility issues.Initial Fixes (Windows Path Quoting)
cli-tool-manager.ts): Path wasn't quoted when executing withshell: truefor.cmdfiles during validationformatter.ts): Generated Python scripts didn't useshell=Truefor.cmd/.batfilesversion-suggester.ts): Same issue with generated Python scriptsThis fix wraps commands in quotes when shell mode is enabled AND the path contains spaces, and ensures generated Python scripts use
subprocess.run(shell=True)for Windows batch files.Code Refactoring
env-utils.ts:preparePythonSubprocessCommand(): For Python subprocess generation (formatter, version-suggester)prepareShellCommand(): For Node.js child_process execution (cli-tool-manager)Security Fixes
Case-insensitive extension check (
env-utils.ts):.CMD,.Bat,.BATetc.)preparePythonSubprocessCommand()andprepareShellCommand()now usetoLowerCase()before checking for.cmd/.batextensionsShell injection vulnerability fix (
formatter.ts):needsShell=true, now usesshlex.quote()to safely escape the promptShell injection vulnerability fix (
version-suggester.ts):needsShell=true, usesshlex.quote()to safely escape the prompt%,^,&,|,<,>, etc.)Path validation for user-configured paths (
formatter.ts,version-suggester.ts,env-utils.ts):isSecurePath()validation to prevent command injection via malicious Settings configuration; & | ${} [] <> ! " ^, env vars, directory traversal, newlines)PowerShell Compatibility Fixes (#865, #866, #867)
PowerShell terminals use different command syntax than bash/cmd.exe, causing failures in Claude CLI operations:
Shell Type Detection (
pty-manager.ts,terminal-lifecycle.ts):detectShellType()to identify PowerShell, cmd.exe, bash, zsh, fish from shell pathshellTypefield toTerminalProcessinterfaceShell-Aware Command Utilities (
shell-escape.ts):buildPathAssignment()- PATH syntax:$env:PATH = '...'(PowerShell) vsPATH='...'(bash) vsset "PATH=..."(cmd)buildEnvVarAssignment()- ENV syntax:$env:VAR = '...'vsVAR='...'vsset "VAR=..."buildEnvVarReference()- Variable reference:$env:VARvs$VARvs%VAR%buildCommandInvocation()- Adds&call operator for .cmd/.bat files in PowerShellbuildCdCommandForShell()- Shell-specific cd syntaxUpdated Command Generation:
claude-integration-handler.ts): Uses shell-type-aware PATH and command syntaxterminal-handlers.ts): Uses shell-type-aware environment variable syntaxBash Availability Check (
claude-integration-handler.ts):isBashAvailable()to detect bash on Windows (Git Bash, MSYS2, Cygwin)File Permissions Fix (
claude-integration-handler.ts):mode: 0o600is Unix-only - now only applied on non-Windows platformsEcho Command Fix (
terminal-handlers.ts):Test Fixes
Added missing mocks in
cli-tool-manager.test.ts:prepareShellCommandandpreparePythonSubprocessCommandto theenv-utilsmockcli-tool-manager.tsafter the refactoringFixed requestAnimationFrame mock in
useXterm.test.ts:Removed unused import (
useXterm.test.ts):renderHookwhich was not being usedRelated Issue
Closes #831 #865 #866 #867
Type of Change
Area
Commit Message Format
Follow conventional commits:
<type>: <subject>Types: feat, fix, docs, style, refactor, test, chore
Example:
feat: add user authentication systemChecklist
developbranchCI/Testing Requirements
Screenshots
N/A - Non-UI change
Feature Toggle
use_feature_nameBreaking Changes
Breaking: No
Details: None - this is a bug fix that adds proper path quoting for Windows paths containing spaces, security hardening for shell command execution, and PowerShell compatibility.
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.