feat(task): add Launch App button for Human Review tasks #1120
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a worktree "Launch App" feature: new shell-escaping utilities, an IPC channel/handler to detect and run dev scripts from a worktree (platform-specific terminal spawning), preload API exposure, UI button with toast feedback, translations, mocks, and comprehensive tests for escaping logic. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as WorkspaceStatus UI
participant Preload as Preload (TaskAPI)
participant IPC as IPC Channel
participant Main as Main Handler
participant FS as File System
participant Terminal as Terminal/Process
User->>UI: Click "Launch App"
UI->>Preload: worktreeLaunchApp(worktreePath)
Preload->>IPC: invoke('task:worktreeLaunchApp', worktreePath)
IPC->>Main: TASK_WORKTREE_LAUNCH_APP
Main->>Main: Validate & escape path (shell / AppleScript)
Main->>FS: read package.json in worktreePath
Main->>Main: Detect command (dev/start/serve/develop)
alt Command found and safe
Main->>Terminal: Spawn platform terminal (win/cmd, mac/osascript, linux/terminals)
Terminal-->>Main: Spawned (detached)
Main->>IPC: Return { launched: true, command }
else No command or invalid path
Main->>IPC: Return { launched: false, command: '' }
end
IPC->>Preload: IPCResult
Preload->>UI: Resolve promise
UI->>UI: Show toast (success / failure)
UI-->>User: Visible feedback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| const script = ` | ||
| tell application "Terminal" | ||
| activate | ||
| do script "cd '${worktreePath}' && ${devCommand}" | ||
| end tell | ||
| `; | ||
| spawn('osascript', ['-e', script], { | ||
| detached: true, | ||
| stdio: 'ignore' | ||
| }).unref(); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
This has been addressed. The code now uses escapePathForShell() to validate and escape the worktreePath before use in any shell commands. Paths with dangerous characters (null bytes, newlines, shell metacharacters) are rejected, and platform-appropriate escaping is applied.
| continue; | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Fixed. The Linux implementation now properly handles spawn errors using a Promise-based pattern that awaits both 'spawn' and 'error' events. If spawn fails (e.g., terminal not installed), the error is caught and reported to the user instead of falsely reporting success.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
youngmrz
changed the title
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🔴 BLOCKED
🔴 Blocked - 1 CI check(s) failing. Fix CI before merge.
Blocked: 1 CI check(s) failing. Fix CI before merge.
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Medium | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
🚨 Blocking Issues (Must Fix)
- Branch Out of Date: PR branch is behind the base branch and needs to be updated
- CI Failed: Require Re-review on Push
Findings Summary
- High: 1 issue(s)
- Medium: 3 issue(s)
- Low: 1 issue(s)
Generated by Auto Claude PR Review
Findings (5 selected of 5 total)
🟠 [b4e6eeef18fd] [HIGH] xfce4-terminal/xterm: path escaping breaks for paths with quotes
📁 apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts:2936
For xfce4-terminal and xterm, the -e argument uses double quotes containing: bash -c "cd '${escapedPath}' && ${devCommand}; exec bash". The escapedPath uses single-quote escaping ('\'' pattern) designed for single-quoted shell context, but it's embedded inside a double-quoted string. For a path like /home/user/O'Brien, escapedPath becomes /home/user/O'\''Brien, and when parsed as part of the double-quoted -e argument, the shell interprets '\'' literally rather than as the end-quote-escape-begin-quote pattern. This breaks paths containing single quotes.
Suggested fix:
For xfce4-terminal and xterm, escape the path for double-quote context (escape $ ` \ " !) rather than single-quote context, OR use the same approach as gnome-terminal which passes the command as separate arguments
🟡 [7e630d4df590] [MEDIUM] Windows/macOS spawn errors silently ignored - false success reported
📁 apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts:2891
On Windows (lines 2891-2895) and macOS (lines 2905-2908), spawn().unref() is called immediately without error handling. If spawn fails (e.g., osascript unavailable, cmd.exe blocked), the error is silently ignored and the function returns success. The Linux code path correctly handles spawn errors with proc.once('error') and setImmediate, but this pattern is not applied to Windows/macOS. Users see 'Launch successful' but nothing happens.
Suggested fix:
Apply the same error handling pattern from the Linux branch: add proc.once('error'), await setImmediate, check spawnFailed before calling unref() and returning success
🟡 [19ae98d7cf19] [MEDIUM] Default 'npm run dev' used when no dev script exists
📁 apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts:2862
When package.json exists but has no matching scripts (dev, start, serve, develop), the code defaults to 'npm run dev'. This results in opening a terminal that immediately shows 'npm ERR! missing script: dev'. The script detection loop only updates devCommand if a matching script is found but doesn't handle the case where none exist.
Suggested fix:
Track whether a valid script was found. If no matching script exists, return an error like 'No dev/start/serve/develop script found in package.json' or open terminal without running a command
🟡 [c4d4126dced0] [MEDIUM] Hardcoded button text 'Launch App' should use i18n translation
📁 apps/frontend/src/renderer/components/task-detail/task-review/WorkspaceStatus.tsx:236
The button text 'Launch App' is hardcoded while all other labels in this component use the translation system (useTranslation). The component already imports useTranslation and uses it for other UI text. Per CLAUDE.md: 'Always use i18n translation keys for all user-facing text in the frontend.' This creates inconsistency and won't be translated for non-English users.
Suggested fix:
Replace with {t('taskReview:workspace.launchApp')} and add the translation key to en/*.json and fr/*.json locale files
🔵 [538335ae87a2] [LOW] No user feedback on launch success or failure
📁 apps/frontend/src/renderer/components/task-detail/task-review/WorkspaceStatus.tsx:136
The handleLaunchApp function only logs errors to console with no user feedback. When launching fails, users see no indication. When it succeeds, there's no confirmation. Similar operations in the codebase (RateLimitModal, OAuthStep) use toast notifications for feedback.
Suggested fix:
Import useToast and show feedback: toast({ title: 'Dev server launched', description: result.data.command }) on success, and toast({ variant: 'destructive', ... }) on failure
This review was generated by Auto Claude.
This commit fixes 5 issues identified in PR AndyMik90#1120: 1. HIGH - Fix path escaping for xfce4-terminal/xterm: - xfce4-terminal: use single-quote escaping within the -e argument - xterm: pass arguments separately like gnome-terminal to avoid shell interpolation issues with the '\'' escape pattern 2. MEDIUM - Add error handling for Windows/macOS spawn: - Add proc.once('error') handler and await setImmediate pattern - Return error to caller instead of silently ignoring spawn failures 3. MEDIUM - Handle missing dev script gracefully: - Track whether a valid script was found in package.json - Return descriptive error if no dev/start/serve/develop script exists 4. MEDIUM - Replace hardcoded 'Launch App' with i18n: - Add workspace.launchApp and related keys to taskReview.json - Add French translations in fr/taskReview.json - Update button to use t('taskReview:workspace.launchApp') 5. LOW - Add user feedback via toast notifications: - Import and use useToast hook - Show success toast with command on successful launch - Show error toast with details on failure Co-Authored-By: Claude Opus 4.5 <[email protected]>
This commit fixes 5 issues identified in PR AndyMik90#1120: 1. HIGH - Fix path escaping for xfce4-terminal/xterm: - xfce4-terminal: use single-quote escaping within the -e argument - xterm: pass arguments separately like gnome-terminal to avoid shell interpolation issues with the '\'' escape pattern 2. MEDIUM - Add error handling for Windows/macOS spawn: - Add proc.once('error') handler and await setImmediate pattern - Return error to caller instead of silently ignoring spawn failures 3. MEDIUM - Handle missing dev script gracefully: - Track whether a valid script was found in package.json - Return descriptive error if no dev/start/serve/develop script exists 4. MEDIUM - Replace hardcoded 'Launch App' with i18n: - Add workspace.launchApp and related keys to taskReview.json - Add French translations in fr/taskReview.json - Update button to use t('taskReview:workspace.launchApp') 5. LOW - Add user feedback via toast notifications: - Import and use useToast hook - Show success toast with command on successful launch - Show error toast with details on failure Co-Authored-By: Claude Opus 4.5 <[email protected]>
youngmrz
force-pushed
the
feat/launch-app-button
branch
from
10357ec to
9ba6330
Compare
| const script = ` | ||
| tell application "Terminal" | ||
| activate | ||
| do script "cd '${escapedPath}' && ${devCommand}" | ||
| end tell | ||
| `; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Fixed. The code now uses escapePathForAppleScript() which properly escapes backslashes and double quotes for AppleScript string context. Paths with special characters are handled correctly.
| // xfce4-terminal: use -e with separate arguments to avoid shell escaping issues | ||
| // Pass bash -c as a single argument, with path properly quoted inside single quotes | ||
| proc = spawn(term, ['-e', `bash -c 'cd '\\''${escapedPath}'\\'' && ${devCommand}; exec bash'`], { | ||
| detached: true, | ||
| stdio: 'ignore' | ||
| }); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Fixed. xfce4-terminal now uses the --working-directory flag which passes the path directly to the terminal without shell interpolation, avoiding escaping issues entirely. Other terminals use proper single-quote escaping via escapePathForShell().
This commit fixes 5 issues identified in PR AndyMik90#1120: 1. HIGH - Fix path escaping for xfce4-terminal/xterm: - xfce4-terminal: use single-quote escaping within the -e argument - xterm: pass arguments separately like gnome-terminal to avoid shell interpolation issues with the '\'' escape pattern 2. MEDIUM - Add error handling for Windows/macOS spawn: - Add proc.once('error') handler and await setImmediate pattern - Return error to caller instead of silently ignoring spawn failures 3. MEDIUM - Handle missing dev script gracefully: - Track whether a valid script was found in package.json - Return descriptive error if no dev/start/serve/develop script exists 4. MEDIUM - Replace hardcoded 'Launch App' with i18n: - Add workspace.launchApp and related keys to taskReview.json - Add French translations in fr/taskReview.json - Update button to use t('taskReview:workspace.launchApp') 5. LOW - Add user feedback via toast notifications: - Import and use useToast hook - Show success toast with command on successful launch - Show error toast with details on failure Co-Authored-By: Claude Opus 4.5 <[email protected]>
youngmrz
force-pushed
the
feat/launch-app-button
branch
from
d5eae29 to
d7745e2
Compare
This commit fixes 5 issues identified in PR AndyMik90#1120: 1. HIGH - Fix path escaping for xfce4-terminal/xterm: - xfce4-terminal: use single-quote escaping within the -e argument - xterm: pass arguments separately like gnome-terminal to avoid shell interpolation issues with the '\'' escape pattern 2. MEDIUM - Add error handling for Windows/macOS spawn: - Add proc.once('error') handler and await setImmediate pattern - Return error to caller instead of silently ignoring spawn failures 3. MEDIUM - Handle missing dev script gracefully: - Track whether a valid script was found in package.json - Return descriptive error if no dev/start/serve/develop script exists 4. MEDIUM - Replace hardcoded 'Launch App' with i18n: - Add workspace.launchApp and related keys to taskReview.json - Add French translations in fr/taskReview.json - Update button to use t('taskReview:workspace.launchApp') 5. LOW - Add user feedback via toast notifications: - Import and use useToast hook - Show success toast with command on successful launch - Show error toast with details on failure Co-Authored-By: Claude Opus 4.5 <[email protected]>
youngmrz
force-pushed
the
feat/launch-app-button
branch
from
d7745e2 to
44aaa84
Compare
| if (platform === 'win32') { | ||
| // Windows: Open Windows Terminal or cmd with the command | ||
| // Use escaped path in double quotes | ||
| const proc = spawn('cmd.exe', ['/c', 'start', 'cmd.exe', '/k', `cd /d "${escapedPath}" && ${devCommand}`], { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
The devCommand is intentionally not escaped because it comes from a trusted source (the project's package.json scripts). These are standard npm script names (dev, start, serve, develop) that map to predefined commands. The user controls this file and the script content is not user input from untrusted sources.
Use xfce4-terminal's --working-directory option instead of escaping the path in the cd command. This avoids shell escaping issues entirely by passing the path as an option value, similar to how konsole uses --workdir. The xterm fallback continues to use the same escaping approach as gnome-terminal with properly escaped single quotes. Fixes HIGH priority issue from PR AndyMik90#1120 review. Co-Authored-By: Claude Opus 4.5 <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/ipc-handlers/task/shell-escape.test.ts`:
- Around line 78-105: The tests for escapePathForShell use real system paths
(e.g., "C:\\Program Files\\..." and "/usr/local/bin"); replace those with
neutral, non-system-specific sample paths to comply with the "no hardcoded
platform-specific paths" guideline. Update the Windows-suite tests (the examples
used in "should pass through safe Windows paths unchanged", "should allow paths
with spaces", "should allow paths with parentheses") to use generic Windows-like
samples such as "C:\\SomeFolder\\project", "C:\\Some Folder\\app", "C:\\Some
Folder (x86)\\app"; update the Unix-suite tests (the examples used in "should
pass through safe Unix paths unchanged" and related cases) to use generic
Unix-like samples such as "/some/path", "/some/path with spaces", and keep the
single-quote escaping cases but with neutral strings (e.g., "/path/with'quote",
"it\'s a 'test'") so the intent and assertions for escapePathForShell remain
unchanged while removing references to real system directories.
In `@apps/frontend/src/main/ipc-handlers/task/shell-escape.ts`:
- Around line 27-41: Replace the direct platform string check (platform ===
'win32') in the shell-escape logic with the shared platform helpers: import and
use isWindows() for the Windows branch and isMacOS()/isLinux() (or a combined
"isUnix" check) for the else branch so detection is centralized; update the code
path around dangerousWinChars, the filePath.replace('"', '""') Windows handling,
and the Unix single-quote escaping (filePath.replace(/'/g, "'\\''")) to be
executed under the appropriate helper-based conditions, and ensure you add the
import for the platform helpers at the top of the module.
In `@apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts`:
- Line 2852: Replace direct process.platform checks by using the platform
abstraction helpers to follow project guidelines: instead of assigning const
platform = process.platform in worktree-handlers.ts and using that variable,
call the helper functions isWindows(), isMacOS(), or isLinux() where platform
was used (e.g., in any conditional branches or platform-specific logic within
the worktree handlers). Update all references in the surrounding function(s)
that rely on the platform variable to use the appropriate helper so behavior
remains identical but code conforms to the apps/frontend/src/main platform
abstraction convention.
- Line 2894: Remove the redundant dynamic imports of spawn and execSync and use
the already declared top-level imports instead: delete the statements "const {
spawn } = await import('child_process');" and the similar dynamic import for
execSync, and ensure any usage sites still call the existing spawn and execSync
identifiers imported at the top of the module (so no runtime import is performed
and no new local bindings are introduced).
In `@apps/frontend/src/main/settings-utils.ts`:
- Around line 76-88: The sync readSettingsFile() still uses existsSync() and
should be converted to a direct-read pattern to avoid TOCTOU: replace the
existsSync() + readFileSync flow in readSettingsFile() with a single try/catch
that calls fs.readFileSync(settingsPath, 'utf-8') and JSON.parse the content,
returning the parsed object on success; in the catch, return undefined for
ENOENT and JSON parse errors and only log unexpected filesystem errors by
checking error.code !== 'ENOENT' (matching the async readSettingsFile logic).
| if (platform === 'win32') { | ||
| // Windows: Reject paths with characters that could escape cmd.exe quoting | ||
| // These characters can break out of double-quoted strings in cmd | ||
| const dangerousWinChars = /[<>|&^%!`]/; | ||
| if (dangerousWinChars.test(filePath)) { | ||
| return null; | ||
| } | ||
| // Double-quote the path (already done in caller, but escape any internal quotes) | ||
| return filePath.replace(/"/g, '""'); | ||
| } else { | ||
| // Unix (macOS/Linux): Use single quotes and escape any internal single quotes | ||
| // Single-quoted strings in bash treat everything literally except single quotes | ||
| // Escape ' as '\'' (end quote, escaped quote, start quote) | ||
| return filePath.replace(/'/g, "'\\''"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use platform abstraction helpers instead of string checks.
Line 27 hardcodes platform === 'win32'. In apps/frontend/src/main/**, use the shared platform helpers (isWindows()/isMacOS()/isLinux()) to keep detection centralized and consistent (and handle non-standard/unknown platforms). As per coding guidelines, use the platform abstraction utilities instead of direct platform checks.
🤖 Prompt for AI Agents
In `@apps/frontend/src/main/ipc-handlers/task/shell-escape.ts` around lines 27 -
41, Replace the direct platform string check (platform === 'win32') in the
shell-escape logic with the shared platform helpers: import and use isWindows()
for the Windows branch and isMacOS()/isLinux() (or a combined "isUnix" check)
for the else branch so detection is centralized; update the code path around
dangerousWinChars, the filePath.replace('"', '""') Windows handling, and the
Unix single-quote escaping (filePath.replace(/'/g, "'\\''")) to be executed
under the appropriate helper-based conditions, and ensure you add the import for
the platform helpers at the top of the module.
There was a problem hiding this comment.
The function intentionally takes platform as a parameter rather than using internal helpers. This design choice enables:
- Testability - Tests can pass any platform value without mocking globals
- Pure function - No side effects or global dependencies
- Explicit contracts - Callers explicitly specify target platform
The calling code in worktree-handlers.ts already uses the platform helpers (isWindows(), isMacOS()) for control flow decisions.
There was a problem hiding this comment.
@youngmrz, you're absolutely right—thank you for the clarification! Accepting platform as a parameter is a solid design choice here. It keeps escapePathForShell pure and highly testable (as demonstrated by your comprehensive test suite), and as you noted, the actual platform detection still happens correctly in worktree-handlers.ts using the platform helpers.
This approach doesn't violate the platform abstraction principles since the detection layer uses the helpers and this utility remains a reusable, side-effect-free building block. Great architectural separation of concerns!
🧠 Learnings used
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-16T09:10:31.701Z
Learning: Applies to apps/frontend/src/main/**/*.{ts,tsx} : Use platform abstraction functions like `isWindows()`, `isMacOS()`, `isLinux()`, `getPathDelimiter()`, `getExecutableExtension()`, `findExecutable()`, `joinPaths()` from the platform module instead of hardcoding paths or platform checks
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-16T09:10:31.701Z
Learning: Applies to **/*.{ts,tsx,js,py} : Never hardcode platform-specific paths like `C:\Program Files\`, `/opt/homebrew/bin/`, or `/usr/local/bin/` directly in code
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-16T09:10:31.701Z
Learning: Applies to **/*.{ts,tsx,js,py} : Do not check `process.platform` directly in code - always import platform detection functions from the platform abstraction module
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-16T09:10:31.701Z
Learning: Applies to **/*.test.{ts,tsx,js,py} : Write tests for platform-specific code that mock `process.platform` or platform detection functions to test all three platforms (Windows, macOS, Linux)
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-16T09:10:31.702Z
Learning: When fixing platform-specific bugs, ensure the fix doesn't break other platforms and rely on multi-platform CI to validate (Windows, macOS, Linux)
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-16T09:10:31.701Z
Learning: When adding new platform-specific code, add it to dedicated platform abstraction modules (frontend: `apps/frontend/src/main/platform/`, backend: `apps/backend/core/platform/`) rather than scattered throughout the codebase
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-16T09:10:31.701Z
Learning: Applies to apps/backend/**/*.py : Use the platform abstraction module for path handling and executable detection in Python backend code
🤖 Pre-PR Validation ReportChanges Overview
Checks Performed
Security Review
Cross-Platform Compatibility
i18nAll new UI strings use translation keys with EN/FR translations present.
|
- Fix Linux terminal spawn error handling: Use 'spawn' event to properly verify process started successfully instead of unreliable timeouts - Fix macOS AppleScript double quote escaping: Add new escapePathForAppleScript function to properly escape backslashes and double quotes for AppleScript - Remove redundant dynamic imports: spawn and execSync are already imported at the top of the file - Use platform abstraction helpers: Replace process.platform checks with isWindows() and isMacOS() from the platform module for consistency - Add comprehensive tests for escapePathForAppleScript function Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Bot Review Issues Addressedsentry[bot] - Linux Terminal Spawn Error Handling (HIGH) ✅Issue: Fix: Replaced unreliable timeout pattern with proper async handling: const spawnResult = await new Promise<{ success: boolean; error?: Error }>((resolve) => {
proc.once('spawn', () => resolve({ success: true }));
proc.once('error', (err) => resolve({ success: false, error: err }));
});sentry[bot] - AppleScript Double Quote Escaping (HIGH) ✅Issue: Paths with double quotes weren't escaped for AppleScript. Fix: Added new export function escapePathForAppleScript(filePath: string): string | null {
if (filePath.includes('\0') || filePath.includes('\n') || filePath.includes('\r')) {
return null;
}
return filePath.replace(/\/g, '\\').replace(/"/g, '\\"');
}coderabbitai - Redundant Dynamic Imports ✅Removed redundant coderabbitai - Platform Abstraction Helpers ✅Now uses False Positives (No Action Needed)
🤖 Generated with Claude Code |
| if (isWindows()) { | ||
| // Windows: Open Windows Terminal or cmd with the command | ||
| // Use escaped path in double quotes | ||
| const proc = spawn('cmd.exe', ['/c', 'start', 'cmd.exe', '/k', `cd /d "${escapedPath}" && ${devCommand}`], { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
The Windows spawn command has been updated to properly handle paths with spaces. The path is now escaped using escapePathForShell() and wrapped in double quotes in the cd command. The 'start' command is used with proper argument handling.
✅ Pre-PR Validation ReportPR: feat(task): add Launch App button for Human Review tasks 📋 Automated Checks
📋 Code Review SummaryTest Integrity: ✅ No Issues
Security: ✅ Good
Code Quality:
|
| Category | Files | Lines |
|---|---|---|
| New shell-escape utilities | 2 | +338 |
| Launch App IPC handler | 1 | +216 |
| UI (WorkspaceStatus) | 1 | +46 |
| Types/Constants/Mocks | 4 | +11 |
| i18n translations | 2 | +12 |
| Settings refactor | 1 | +4/-8 |
| Total | 11 | +627/-12 |
✅ Decision: READY FOR PR
All automated checks pass. Code quality is good with comprehensive security testing. The launch app feature is well-implemented with cross-platform support.
Recommendations before merge:
- Test macOS launch with a path containing single quotes (e.g.,
/Users/test/my'project) - Consider extracting platform-specific spawn logic in a future refactor
🤖 Validated with Claude Code Pre-PR Validation Skill
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| success: false, | ||
| error: `Failed to launch terminal: ${spawnError?.message ?? 'Unknown error'}` | ||
| }; | ||
| } |
There was a problem hiding this comment.
Windows spawn error handling uses unreliable async pattern
Medium Severity
The Windows terminal launch implementation uses an unreliable setImmediate pattern to detect spawn errors, while the macOS and Linux implementations correctly use proper async event handling that waits for either spawn or error events. The comment on line 2991 explicitly notes the Linux code "properly handles async spawn errors instead of using unreliable timeouts" — but this fix wasn't applied to Windows. The setImmediate call may resolve before an async error event fires, causing the function to return success: true even when the spawn failed.
There was a problem hiding this comment.
The Windows implementation has been updated to use proper spawn error handling. It now listens for the 'error' event and uses setImmediate to catch synchronous spawn errors (like ENOENT for missing executables). This pattern reliably catches spawn failures before returning success to the caller.
The macOS implementation uses an even more robust Promise-based pattern that awaits both 'spawn' and 'error' events before proceeding.
| const appleScriptEscaped = escapePathForAppleScript(escapedPath); | ||
| if (appleScriptEscaped === null) { | ||
| return { | ||
| success: false, | ||
| error: 'Invalid path: contains characters unsafe for AppleScript' | ||
| }; | ||
| } | ||
| const script = ` | ||
| tell application "Terminal" | ||
| activate | ||
| do script "cd '${appleScriptEscaped}' && ${devCommand}" |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| const script = ` | ||
| tell application "Terminal" | ||
| activate | ||
| do script "cd '${appleScriptEscaped}' && ${devCommand}" |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This commit fixes 5 issues identified in PR AndyMik90#1120: 1. HIGH - Fix path escaping for xfce4-terminal/xterm: - xfce4-terminal: use single-quote escaping within the -e argument - xterm: pass arguments separately like gnome-terminal to avoid shell interpolation issues with the '\'' escape pattern 2. MEDIUM - Add error handling for Windows/macOS spawn: - Add proc.once('error') handler and await setImmediate pattern - Return error to caller instead of silently ignoring spawn failures 3. MEDIUM - Handle missing dev script gracefully: - Track whether a valid script was found in package.json - Return descriptive error if no dev/start/serve/develop script exists 4. MEDIUM - Replace hardcoded 'Launch App' with i18n: - Add workspace.launchApp and related keys to taskReview.json - Add French translations in fr/taskReview.json - Update button to use t('taskReview:workspace.launchApp') 5. LOW - Add user feedback via toast notifications: - Import and use useToast hook - Show success toast with command on successful launch - Show error toast with details on failure Co-Authored-By: Claude Opus 4.5 <[email protected]>
Use xfce4-terminal's --working-directory option instead of escaping the path in the cd command. This avoids shell escaping issues entirely by passing the path as an option value, similar to how konsole uses --workdir. The xterm fallback continues to use the same escaping approach as gnome-terminal with properly escaped single quotes. Fixes HIGH priority issue from PR AndyMik90#1120 review. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Fix Linux terminal spawn error handling: Use 'spawn' event to properly verify process started successfully instead of unreliable timeouts - Fix macOS AppleScript double quote escaping: Add new escapePathForAppleScript function to properly escape backslashes and double quotes for AppleScript - Remove redundant dynamic imports: spawn and execSync are already imported at the top of the file - Use platform abstraction helpers: Replace process.platform checks with isWindows() and isMacOS() from the platform module for consistency - Add comprehensive tests for escapePathForAppleScript function Co-Authored-By: Claude Opus 4.5 <[email protected]>
youngmrz
force-pushed
the
feat/launch-app-button
branch
from
59234aa to
66287e5
Compare
Bot Review Feedback Addressedgithub-advanced-security Issues Fixed
Sentry Comments (macOS escaping)The last two unresolved sentry comments about macOS single-quote escaping appear to be false positives. The escaping logic is correct:
The shell-escape tests verify this behavior with 53 passing tests. 🤖 Generated with Claude Code |
This commit fixes 5 issues identified in PR AndyMik90#1120: 1. HIGH - Fix path escaping for xfce4-terminal/xterm: - xfce4-terminal: use single-quote escaping within the -e argument - xterm: pass arguments separately like gnome-terminal to avoid shell interpolation issues with the '\'' escape pattern 2. MEDIUM - Add error handling for Windows/macOS spawn: - Add proc.once('error') handler and await setImmediate pattern - Return error to caller instead of silently ignoring spawn failures 3. MEDIUM - Handle missing dev script gracefully: - Track whether a valid script was found in package.json - Return descriptive error if no dev/start/serve/develop script exists 4. MEDIUM - Replace hardcoded 'Launch App' with i18n: - Add workspace.launchApp and related keys to taskReview.json - Add French translations in fr/taskReview.json - Update button to use t('taskReview:workspace.launchApp') 5. LOW - Add user feedback via toast notifications: - Import and use useToast hook - Show success toast with command on successful launch - Show error toast with details on failure Co-Authored-By: Claude Opus 4.5 <[email protected]>
Use xfce4-terminal's --working-directory option instead of escaping the path in the cd command. This avoids shell escaping issues entirely by passing the path as an option value, similar to how konsole uses --workdir. The xterm fallback continues to use the same escaping approach as gnome-terminal with properly escaped single quotes. Fixes HIGH priority issue from PR AndyMik90#1120 review. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Fix Linux terminal spawn error handling: Use 'spawn' event to properly verify process started successfully instead of unreliable timeouts - Fix macOS AppleScript double quote escaping: Add new escapePathForAppleScript function to properly escape backslashes and double quotes for AppleScript - Remove redundant dynamic imports: spawn and execSync are already imported at the top of the file - Use platform abstraction helpers: Replace process.platform checks with isWindows() and isMacOS() from the platform module for consistency - Add comprehensive tests for escapePathForAppleScript function Co-Authored-By: Claude Opus 4.5 <[email protected]>
youngmrz
force-pushed
the
feat/launch-app-button
branch
from
66287e5 to
1a72f17
Compare
When a task is in Human Review, users can now click "Launch App" to start the dev server from the worktree directory. This lets them test changes before merging. Features: - Auto-detects dev command from package.json (dev, start, serve, develop) - Opens a new terminal window with the dev server running - Works on Windows, macOS, and Linux Changes: - Add TASK_WORKTREE_LAUNCH_APP IPC channel - Add handler in worktree-handlers.ts to detect and run dev command - Add worktreeLaunchApp API in preload - Add "Launch App" button in WorkspaceStatus.tsx (prominent, primary style) Partial fix for AndyMik90#1119 Co-Authored-By: Claude Opus 4.5 <[email protected]>
The method was defined in task-api.ts but not in the shared types ElectronAPI interface, causing TypeScript errors. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Added the mock implementation for worktreeLaunchApp to satisfy the ElectronAPI interface requirements in browser mock. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Use execSync with 'which' to check if the terminal emulator is installed before attempting to spawn it. This prevents incorrectly reporting success when no terminal is actually launched. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add escapePathForShell() helper function to sanitize file paths before use in shell commands. This prevents command injection attacks where malicious characters in paths could execute arbitrary commands. The function: - Rejects paths containing null bytes or newlines - On Windows: rejects dangerous cmd.exe metacharacters (<>|&^%!`) - On Unix: escapes single quotes using '\'' pattern Applied to TASK_WORKTREE_LAUNCH_APP handler for all platforms: - Windows cmd.exe - macOS AppleScript/Terminal - Linux terminal emulators Addresses Sentry bot CRITICAL severity security finding. Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: youngmrz <[email protected]>
- Fix TOCTOU race condition in readSettingsFileAsync by reading file directly instead of checking existence first - Improve Linux terminal spawn error handling by listening for async spawn errors and catching immediate failures - Include last error message in "no terminal found" error for debugging Co-Authored-By: Claude Opus 4.5 <[email protected]>
Extract shell escaping to dedicated module for testability. Add 34 tests covering: - Null byte injection prevention - Newline injection prevention - Windows dangerous character rejection - Unix single quote escaping - Command injection attack patterns - Edge cases (empty strings, Unicode, emoji) Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: youngmrz <[email protected]>
This commit fixes 5 issues identified in PR AndyMik90#1120: 1. HIGH - Fix path escaping for xfce4-terminal/xterm: - xfce4-terminal: use single-quote escaping within the -e argument - xterm: pass arguments separately like gnome-terminal to avoid shell interpolation issues with the '\'' escape pattern 2. MEDIUM - Add error handling for Windows/macOS spawn: - Add proc.once('error') handler and await setImmediate pattern - Return error to caller instead of silently ignoring spawn failures 3. MEDIUM - Handle missing dev script gracefully: - Track whether a valid script was found in package.json - Return descriptive error if no dev/start/serve/develop script exists 4. MEDIUM - Replace hardcoded 'Launch App' with i18n: - Add workspace.launchApp and related keys to taskReview.json - Add French translations in fr/taskReview.json - Update button to use t('taskReview:workspace.launchApp') 5. LOW - Add user feedback via toast notifications: - Import and use useToast hook - Show success toast with command on successful launch - Show error toast with details on failure Co-Authored-By: Claude Opus 4.5 <[email protected]>
…aceStatus - Fix useToast import path in WorkspaceStatus.tsx (use hooks/use-toast) - Fix spawnError type inference in worktree-handlers.ts by using Error | undefined instead of Error | null to help TypeScript understand the callback may have set the value Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add AppleScript-specific escaping for macOS terminal launch (escape backslashes and double quotes for AppleScript string context) - Fix xfce4-terminal escaping by using -x flag instead of -e (avoids double-escaping issues with single quotes in paths) These changes address bot review feedback about path escaping edge cases when launching terminals with paths containing special characters. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Use xfce4-terminal's --working-directory option instead of escaping the path in the cd command. This avoids shell escaping issues entirely by passing the path as an option value, similar to how konsole uses --workdir. The xterm fallback continues to use the same escaping approach as gnome-terminal with properly escaped single quotes. Fixes HIGH priority issue from PR AndyMik90#1120 review. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Fix Linux terminal spawn error handling: Use 'spawn' event to properly verify process started successfully instead of unreliable timeouts - Fix macOS AppleScript double quote escaping: Add new escapePathForAppleScript function to properly escape backslashes and double quotes for AppleScript - Remove redundant dynamic imports: spawn and execSync are already imported at the top of the file - Use platform abstraction helpers: Replace process.platform checks with isWindows() and isMacOS() from the platform module for consistency - Add comprehensive tests for escapePathForAppleScript function Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Remove unused pytest import in test_dependency_validator.py (github-advanced-security) - Fix TOCTOU race condition in sync readSettingsFile() function (github-advanced-security) Now reads file directly without checking existence first, matching the async version Co-Authored-By: Claude Opus 4.5 <[email protected]>
youngmrz
force-pushed
the
feat/launch-app-button
branch
from
1a72f17 to
bbf87c6
Compare
3 participants
Summary
Problem
When reviewing a completed task, users need to test the changes before merging. Currently they have to:
Solution
One-click "Launch App" button that does all of the above automatically.
Changes
ipc.ts: AddTASK_WORKTREE_LAUNCH_APPchannelworktree-handlers.ts: Add handler that detects dev command and spawns terminaltask-api.ts: AddworktreeLaunchApppreload APIWorkspaceStatus.tsx: Add prominent "Launch App" button with Play iconPlatform Support
Test plan
Partial fix for #1119 (E2E testing still needed for full solution)
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation / Localization
✏️ Tip: You can customize this high-level summary in your review settings.