fix(terminal): PowerShell command execution in Agent Terminals

[VDT-91]

Summary

• Fix PowerShell command execution in Agent Terminals on Windows
• Add screen clearing before Claude starts for cleaner UX
• Fix escapeShellArg() backslash escape regression

Problem

When using "Invoke Claude All" in Agent Terminals on Windows with PowerShell, Claude fails to start. The command was being generated with bash-style syntax that PowerShell doesn't understand.

Error: The token '&&' is not a valid statement separator in this version

Changes

1. Added escapeCommandForShell() function (shell-escape.ts)

• PowerShell requires the & (call) operator to execute quoted paths: & 'path.bat'
• Without it, PowerShell just prints the path instead of executing

2. Added screen clearing before Claude starts (claude-integration-handler.ts)

• PowerShell: cls;
• cmd.exe: cls &&
• bash/zsh: clear &&

3. Fixed escapeShellArg() regression

• Backslash escape sequence was corrupted ('\'' → ''')
• Restored proper POSIX single-quote escaping

4. Fixed test cross-platform compatibility

• Added explicit shellType to test mocks
• Fixed Windows path separator handling in assertions

Before

cd "D:\Project" && PATH='...' 'claude-wrapper.bat'
# Error: && not valid in PowerShell

After

cls; Set-Location 'D:\Project'; $env:PATH='...'; & 'claude-wrapper.bat'
# Works correctly

Test plan

• All 24 claude-integration-handler tests pass
• TypeScript compiles without errors
• Manually tested on Windows with PowerShell

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Terminals now detect and store shell type; command execution adapts per shell (clear, cd, escaping, PATH handling).

• Cross-platform Compatibility

  • Improved Windows path handling and consistent command prefixes across bash, zsh, PowerShell, pwsh, and cmd.

• Tests

  • Expanded test coverage for shell detection, multi-shell escaping, command construction, and case-insensitive log parsing.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

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 (3)

apps/frontend/src/main/agent/agent-events.ts (1)

48-61: Consider i18n for progress messages (optional, pre-existing).

The hardcoded message strings (e.g., 'Discovering project context...', 'Gathering requirements...') are potentially user-facing. Per coding guidelines, these should use translation keys. This is pre-existing and out of scope for this PR, but worth noting for future improvement.

apps/frontend/src/main/terminal/claude-integration-handler.ts (2)

430-474: Consider user notification for profile feature limitation.

When a non-bash shell is detected and the profile has a token or configDir, the code silently falls back to the default method. This means OAuth token injection won't work for PowerShell/cmd users.

The console.warn helps with debugging, but users won't know why their profile switch isn't working as expected. Consider:

1. Sending an IPC notification to the renderer to show a user-facing warning
2. Or documenting this limitation in the UI when users select non-default profiles on Windows

---

595-678: Code duplication between sync and async invoke methods.

The shell-aware logic in invokeClaudeAsync is nearly identical to invokeClaude. Consider extracting the shared logic (shell type resolution, command building, profile handling) into a helper function that both methods can call, differing only in their async/sync I/O operations.

This would reduce maintenance burden when updating shell handling logic.

🤖 Fix all issues with AI agents

In @apps/frontend/src/main/terminal/__tests__/pty-manager.test.ts:
- Around line 127-147: The tests directly mutate process.platform using
Object.defineProperty in the "should return unknown for unrecognized Windows
shells" and "should fallback to bash for unrecognized Unix shells" blocks;
switch to using vitest's vi.stubGlobal to stub process.platform (e.g.,
vi.stubGlobal('process', { ...process, platform: 'win32' }) and similarly for
'darwin') and remove manual restores, or ensure you call the returned restore
function from vi.stubGlobal; update the two tests that call detectShellType(...)
to use vi.stubGlobal instead of Object.defineProperty for safer, cleaner
platform mocking while keeping the same assertions.

In @apps/frontend/src/main/terminal/claude-integration-handler.ts:
- Around line 406-414: The fallback shell on Windows currently defaults to 'cmd'
when terminal.shellType is undefined, which may cause issues for users on modern
Windows systems where PowerShell is standard. Update the fallback logic in the
section using shellType (around the terminal.shellType assignment) to use
'powershell' instead of 'cmd' for Windows platforms. Additionally, add a console
warning logging which fallback shell was chosen when terminal.shellType is not
detected to help with troubleshooting shell detection failures.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9427301 and 9514c2c.

⛔ Files ignored due to path filters (1)

• package-lock.json is excluded by !**/package-lock.json, !**/package-lock.json

📒 Files selected for processing (10)

• apps/frontend/src/main/__tests__/agent-events.test.ts
• apps/frontend/src/main/agent/agent-events.ts
• apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts
• apps/frontend/src/main/terminal/__tests__/pty-manager.test.ts
• apps/frontend/src/main/terminal/claude-integration-handler.ts
• apps/frontend/src/main/terminal/pty-manager.ts
• apps/frontend/src/main/terminal/terminal-lifecycle.ts
• apps/frontend/src/main/terminal/types.ts
• apps/frontend/src/shared/utils/__tests__/shell-escape.test.ts
• apps/frontend/src/shared/utils/shell-escape.ts

🧰 Additional context used

📓 Path-based instructions (2)

apps/frontend/src/**/*.{tsx,ts}

📄 CodeRabbit inference engine (CLAUDE.md)

apps/frontend/src/**/*.{tsx,ts}: Use i18n translation keys for all user-facing text in the frontend. All labels, buttons, messages must use translation keys from react-i18next with namespace:section.key format (e.g., 'navigation:items.githubPRs').
Never use hardcoded strings for UI text in JSX/TSX files. Always use translation keys via useTranslation() hook.

Files:

• apps/frontend/src/main/__tests__/agent-events.test.ts
• apps/frontend/src/main/terminal/types.ts
• apps/frontend/src/main/terminal/__tests__/pty-manager.test.ts
• apps/frontend/src/shared/utils/__tests__/shell-escape.test.ts
• apps/frontend/src/main/terminal/terminal-lifecycle.ts
• apps/frontend/src/main/terminal/pty-manager.ts
• apps/frontend/src/main/agent/agent-events.ts
• apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts
• apps/frontend/src/main/terminal/claude-integration-handler.ts
• apps/frontend/src/shared/utils/shell-escape.ts

apps/frontend/**/*.{ts,tsx}

⚙️ CodeRabbit configuration file

apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.

Files:

• apps/frontend/src/main/__tests__/agent-events.test.ts
• apps/frontend/src/main/terminal/types.ts
• apps/frontend/src/main/terminal/__tests__/pty-manager.test.ts
• apps/frontend/src/shared/utils/__tests__/shell-escape.test.ts
• apps/frontend/src/main/terminal/terminal-lifecycle.ts
• apps/frontend/src/main/terminal/pty-manager.ts
• apps/frontend/src/main/agent/agent-events.ts
• apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts
• apps/frontend/src/main/terminal/claude-integration-handler.ts
• apps/frontend/src/shared/utils/shell-escape.ts

🧬 Code graph analysis (5)

apps/frontend/src/main/terminal/__tests__/pty-manager.test.ts (1)

apps/frontend/src/main/terminal/pty-manager.ts (1)

• detectShellType (32-75)

apps/frontend/src/shared/utils/__tests__/shell-escape.test.ts (1)

apps/frontend/src/shared/utils/shell-escape.ts (10)

• escapeShellArg (26-31)
• escapeShellArgPowerShell (47-52)
• escapeShellArgWindows (66-78)
• escapeShellArgForShell (87-99)
• escapeCommandForShell (111-124)
• escapeShellPath (132-134)
• buildCdCommand (144-158)
• buildCdCommandForShell (167-188)
• buildPathPrefixForShell (197-221)
• isPathSafe (231-248)

apps/frontend/src/main/terminal/pty-manager.ts (1)

apps/frontend/src/main/terminal/types.ts (1)

• ShellType (9-9)

apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts (1)

apps/frontend/src/main/terminal/claude-integration-handler.ts (2)

• buildClaudeShellCommand (98-120)
• isBashCompatibleShell (63-65)

apps/frontend/src/shared/utils/shell-escape.ts (1)

apps/frontend/src/main/terminal/types.ts (1)

• ShellType (9-9)

🔇 Additional comments (28)

apps/frontend/src/main/agent/agent-events.ts (1)

43-43: LGTM! Case-insensitive matching implementation is clean and consistent.

The approach of creating lowerLog once and reusing it for all comparisons is efficient. The implementation correctly preserves the original log for structured markers like __TASK_LOG_ (line 32) while using lowerLog for pattern matching.

Also applies to: 146-147, 182-183

apps/frontend/src/main/__tests__/agent-events.test.ts (1)

531-584: LGTM! Good test coverage for case-insensitivity.

The new tests comprehensively verify case-insensitive matching for both parseRoadmapProgress and parseIdeationProgress. Combined with existing uppercase tests (e.g., 'PROJECT ANALYSIS starting'), the test suite now covers the full case spectrum (uppercase, mixed-case, lowercase).

apps/frontend/src/main/terminal/types.ts (2)

5-9: LGTM!

The ShellType union is well-defined, covering the major shells (bash, zsh, PowerShell variants, cmd) with an unknown fallback. This provides a solid foundation for shell-aware command generation.

---

28-29: LGTM!

Adding shellType as an optional property to TerminalProcess is appropriate since shell detection may not always succeed. The JSDoc comment clearly explains its purpose.

apps/frontend/src/main/terminal/terminal-lifecycle.ts (2)

69-75: LGTM!

The destructuring of spawnPtyProcess result correctly captures both the PTY process and the detected shell type. This aligns with the updated SpawnResult interface from pty-manager.ts.

---

87-88: LGTM!

The shellType is correctly propagated to the TerminalProcess object, enabling shell-aware command generation downstream.

apps/frontend/src/main/terminal/__tests__/pty-manager.test.ts (1)

76-84: LGTM!

Good test coverage for preventing false positives when paths contain "cmd" as a substring. This validates the precise matching logic that avoids misidentifying paths like mycmdtool\bash.exe as cmd.exe.

apps/frontend/src/shared/utils/__tests__/shell-escape.test.ts (3)

1-21: LGTM!

Comprehensive test file covering shell escaping utilities across POSIX, PowerShell, and cmd.exe. The imports are complete and the test structure is well-organized.

---

42-49: LGTM!

Excellent security-focused tests validating that command injection attempts are safely wrapped in single quotes. This confirms the escaping logic prevents shell metacharacter exploitation.

---

111-115: LGTM!

Good test for the defensive behavior that throws on double quotes in cmd.exe paths. This is the correct approach since double quotes cannot be reliably escaped inside double-quoted strings in cmd.exe.

apps/frontend/src/main/terminal/pty-manager.ts (3)

32-75: LGTM!

The detectShellType function implements precise shell detection with well-ordered checks (more specific patterns first). Key strengths:

• pwsh checked before powershell to avoid false positives
• cmd.exe uses exact matching to prevent substring false positives
• Sensible Unix fallback to 'bash'

---

15-21: LGTM!

The SpawnResult interface cleanly encapsulates the PTY process and detected shell type, enabling downstream consumers to access both values from a single spawn operation.

---

161-174: LGTM!

The spawn logic correctly computes shellType before spawning and returns the composite result. The variable naming is clear (ptyProcess vs pty in the result) to avoid confusion.

apps/frontend/src/shared/utils/shell-escape.ts (5)

26-31: LGTM!

The POSIX shell escaping correctly uses the standard '\'' pattern to escape single quotes within single-quoted strings. This is the canonical and secure approach for POSIX shells.

---

66-78: LGTM!

The defensive approach of throwing on double quotes in cmd.exe paths is correct. Double quotes cannot be reliably escaped inside double-quoted strings in cmd.exe, and this prevents potential command injection. The comment correctly notes this is rare in practice.

---

111-124: LGTM!

The PowerShell call operator (&) is correctly applied when escaping commands. This is necessary because PowerShell requires the call operator to execute a quoted string as a command path.

---

167-188: LGTM!

Shell-aware cd command construction is correctly implemented:

• PowerShell uses Set-Location with semicolon separator
• cmd.exe uses cd /d for drive switching with && separator
• POSIX shells use cd with && separator

The /d flag for cmd.exe is important for changing drives on Windows.

---

197-221: LGTM!

The PATH prefix construction correctly handles shell-specific syntax:

• PowerShell: $env:PATH= with semicolons preserved
• cmd.exe: set "PATH=" with % escaped to %%
• POSIX: Windows semicolons converted to colons for compatibility

apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts (5)

43-45: LGTM!

Adding explicit shellType: 'bash' to the mock terminal ensures deterministic test behavior. The comment clearly explains the purpose.

---

239-240: LGTM!

Using regex patterns with [/\\] to match both forward and back slashes handles Windows path normalization correctly. This makes the tests cross-platform compatible.

Also applies to: 282-283

---

433-445: LGTM!

Tests correctly verify shell-specific clear command prefixes:

• bash → clear &&
• powershell → cls;

This validates the getClearCommand helper behavior.

---

626-656: LGTM!

Comprehensive tests for isBashCompatibleShell covering all ShellType variants. Correctly returns true only for bash and zsh.

---

658-686: LGTM!

Shell type variation tests correctly verify:

• cmd → cls && (uses && like bash)
• pwsh → cls; (uses ; like Windows PowerShell)
• zsh → clear &&
• unknown → defaults to clear && (POSIX fallback)

apps/frontend/src/main/terminal/claude-integration-handler.ts (5)

16-24: LGTM!

The imports and type updates correctly support the new shell-aware command construction. The discriminated union update to ClaudeCommandConfig enforces that callers provide shellType when using the default method.

Also applies to: 34-37

---

39-65: LGTM!

The shell-specific clear commands are correctly implemented:

• PowerShell uses ; as statement separator (not &&)
• cmd.exe and bash/zsh use && for command chaining

The isBashCompatibleShell helper properly gates bash-specific features (source, export, bash -c).

---

98-120: LGTM!

The function correctly:

1. Preserves bash-specific syntax for temp-file and config-dir methods with clear documentation
2. Uses shell-aware clear command for the default method
3. Wraps the default case in braces to satisfy ESLint's no-case-declarations rule

---

511-534: LGTM!

The resume function correctly applies shell-aware escaping and path prefix without the clear screen prefix (appropriate for resume operations where preserving terminal context is desirable).

---

694-717: LGTM!

The async resume function correctly mirrors the sync version with shell-aware command construction.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

rg -n "shellType" --type=ts -C3 -g '!*.test.ts' -g '!*.spec.ts'

Repository: AndyMik90/Auto-Claude

Length of output: 23858

---

🏁 Script executed:

rg -n "function detectShellType|export.*detectShellType" --type=ts -C5

Repository: AndyMik90/Auto-Claude

Length of output: 1085

---

🏁 Script executed:

rg -n "export function detectShellType" --type=ts -A30

Repository: AndyMik90/Auto-Claude

Length of output: 2763

---

🏁 Script executed:

rg -n "export function detectShellType" --type=ts -A40

Repository: AndyMik90/Auto-Claude

Length of output: 3507

---

🏁 Script executed:

rg -n "export function detectShellType" --type=ts -A45

Repository: AndyMik90/Auto-Claude

Length of output: 3785

---

Consider using 'powershell' as the Windows fallback instead of 'cmd'.

The platform fallback logic (line 408) defaults to 'cmd' on Windows when terminal.shellType is undefined. However, PowerShell is the standard default shell on modern Windows systems (Windows Server 2016+, Windows 11). If shell detection fails, falling back to 'cmd' syntax may not work correctly for users running PowerShell.

The codebase already has full PowerShell support with dedicated escaping logic (buildCdCommandForShell, escapeCommandForShell, buildPathPrefixForShell all handle 'powershell' and 'pwsh'). Consider changing the fallback to 'powershell' on Windows to align with modern defaults, or add a console warning that explicitly logs which fallback is being used to help diagnose detection failures.

Note: This fallback only applies if terminal.shellType is undefined, which should be rare since shell detection runs during PTY creation. However, detection failures on unusual shell configurations would benefit from explicit logging.

🤖 Prompt for AI Agents

In @apps/frontend/src/main/terminal/claude-integration-handler.ts around lines
406 - 414, The fallback shell on Windows currently defaults to 'cmd' when
terminal.shellType is undefined, which may cause issues for users on modern
Windows systems where PowerShell is standard. Update the fallback logic in the
section using shellType (around the terminal.shellType assignment) to use
'powershell' instead of 'cmd' for Windows platforms. Additionally, add a console
warning logging which fallback shell was chosen when terminal.shellType is not
detected to help with troubleshooting shell detection failures.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In @apps/frontend/src/main/terminal/pty-manager.ts:
- Around line 33-70: The detectShellType function uses imprecise includes()
checks that cause false positives; change the path matching to use endsWith()
(or a boundary-aware regex) instead of includes() for the pwsh, powershell,
bash, and zsh checks so only true file-name or final path-segment matches are
accepted; update the pwsh branch to remove normalized.includes('/pwsh') and rely
on normalized.endsWith('/pwsh') or endsWith('pwsh'), do the same for powershell
(drop includes('/powershell')), bash (drop includes('/bash')), and zsh (drop
includes('/zsh')), ensuring the logic around
normalized.endsWith('\\cmd')/endsWith('/cmd') and normalized === 'cmd' for the
cmd branch remains unchanged.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9514c2c and 608c779.

📒 Files selected for processing (2)

• apps/frontend/src/main/terminal/claude-integration-handler.ts
• apps/frontend/src/main/terminal/pty-manager.ts

🧰 Additional context used

📓 Path-based instructions (2)

apps/frontend/src/**/*.{tsx,ts}

📄 CodeRabbit inference engine (CLAUDE.md)

apps/frontend/src/**/*.{tsx,ts}: Use i18n translation keys for all user-facing text in the frontend. All labels, buttons, messages must use translation keys from react-i18next with namespace:section.key format (e.g., 'navigation:items.githubPRs').
Never use hardcoded strings for UI text in JSX/TSX files. Always use translation keys via useTranslation() hook.

Files:

• apps/frontend/src/main/terminal/pty-manager.ts
• 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/pty-manager.ts
• apps/frontend/src/main/terminal/claude-integration-handler.ts

🧬 Code graph analysis (2)

apps/frontend/src/main/terminal/pty-manager.ts (1)

apps/frontend/src/main/terminal/types.ts (1)

• ShellType (9-9)

apps/frontend/src/main/terminal/claude-integration-handler.ts (3)

apps/frontend/src/main/terminal/types.ts (1)

• ShellType (9-9)

apps/frontend/src/shared/utils/shell-escape.ts (4)

• buildCdCommandForShell (167-188)
• escapeCommandForShell (111-124)
• buildPathPrefixForShell (197-221)
• escapeShellArg (26-31)

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 (python)
• GitHub Check: CodeQL (javascript-typescript)

🔇 Additional comments (10)

apps/frontend/src/main/terminal/pty-manager.ts (2)

16-22: LGTM!

The SpawnResult interface is well-structured, pairing the PTY handle with the detected shell type for downstream shell-aware command generation.

---

135-176: LGTM!

The changes correctly detect the shell type from the resolved shell path and return it alongside the PTY handle. Good addition of shellType to the debug log for troubleshooting shell detection issues.

apps/frontend/src/main/terminal/claude-integration-handler.ts (8)

16-24: LGTM!

Imports correctly updated to bring in shell-aware utilities and the ShellType type.

---

34-37: LGTM!

The discriminated union is correctly updated. The default method now carries shellType for shell-aware command generation, while temp-file and config-dir remain bash-specific (guarded by isBashCompatibleShell() checks at call sites).

---

39-54: LGTM!

Shell-specific clear commands are correct: PowerShell uses cls; (semicolon separator), cmd uses cls &&, and bash/zsh use clear &&. The default fallback to bash-style clear is appropriate.

---

56-65: LGTM!

The helper correctly identifies bash-compatible shells for guarding bash-specific OAuth token injection methods.

---

98-120: LGTM!

The shell-aware command construction is correct. The default method uses getClearCommand() for shell-specific clear commands, while temp-file and config-dir continue using bash-specific syntax (appropriately guarded by isBashCompatibleShell() at call sites).

---

406-499: LGTM!

The shell-aware command generation is well-implemented:

• Good documentation explaining the 'cmd' fallback rationale on Windows
• Consistent use of shell-aware utilities (buildCdCommandForShell, escapeCommandForShell, buildPathPrefixForShell)
• Proper guarding of bash-specific methods with isBashCompatibleShell() checks
• Clean fallback to the default method for non-bash shells

---

512-559: LGTM!

The resumeClaude function correctly applies shell-aware command escaping and path prefix. The omission of the clear command is appropriate for resume scenarios where preserving terminal context is desirable.

---

571-746: LGTM!

The async versions correctly mirror their sync counterparts with consistent shell-aware logic. Good use of fsPromises.writeFile for non-blocking file operations in the async path.

[coderabbitai]

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In @apps/frontend/src/main/terminal/__tests__/pty-manager.test.ts:
- Around line 127-147: The tests that override process.platform (the two it
blocks asserting detectShellType for Windows and Unix) must restore the original
platform inside a finally to guarantee restoration on failure; wrap the platform
mutation and expect call in a try/finally, capturing originalPlatform before the
change and resetting process.platform in the finally block so detectShellType
tests cannot leak a modified process.platform to other tests.

In @apps/frontend/src/main/terminal/pty-manager.ts:
- Around line 33-84: detectShellType has redundant path checks like
endsWith('/pwsh'), endsWith('/powershell'), endsWith('/bash'), and
endsWith('/zsh') because endsWith('pwsh'|'powershell'|'bash'|'zsh') already
covers those cases; simplify the conditionals in detectShellType by removing the
redundant endsWith('/...') checks (leave platform-specific/backslash checks for
Windows cmd as-is) and update the related comments to reflect the simplified
matching logic so behavior remains identical but with fewer evaluations.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 608c779 and 3e28f11.

📒 Files selected for processing (2)

• apps/frontend/src/main/terminal/__tests__/pty-manager.test.ts
• apps/frontend/src/main/terminal/pty-manager.ts

🧰 Additional context used

📓 Path-based instructions (2)

apps/frontend/src/**/*.{tsx,ts}

📄 CodeRabbit inference engine (CLAUDE.md)

apps/frontend/src/**/*.{tsx,ts}: Use i18n translation keys for all user-facing text in the frontend. All labels, buttons, messages must use translation keys from react-i18next with namespace:section.key format (e.g., 'navigation:items.githubPRs').
Never use hardcoded strings for UI text in JSX/TSX files. Always use translation keys via useTranslation() hook.

Files:

• apps/frontend/src/main/terminal/__tests__/pty-manager.test.ts
• apps/frontend/src/main/terminal/pty-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/terminal/__tests__/pty-manager.test.ts
• apps/frontend/src/main/terminal/pty-manager.ts

🧬 Code graph analysis (2)

apps/frontend/src/main/terminal/__tests__/pty-manager.test.ts (1)

apps/frontend/src/main/terminal/pty-manager.ts (1)

• detectShellType (33-84)

apps/frontend/src/main/terminal/pty-manager.ts (1)

apps/frontend/src/main/terminal/types.ts (1)

• ShellType (9-9)

⏰ 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/main/terminal/pty-manager.ts (2)

16-22: LGTM! Clean interface design.

The SpawnResult interface appropriately couples the PTY process with its detected shell type, enabling downstream code to make shell-aware command decisions.

---

143-183: LGTM! Clean refactor of return type.

The function correctly detects the shell type before spawning and returns both the PTY process and shell type. The debug log at line 160 is helpful for troubleshooting shell detection issues.

apps/frontend/src/main/terminal/__tests__/pty-manager.test.ts (3)

10-29: LGTM! Proper mocking setup.

The mocks correctly isolate the test from native modules and external dependencies, making the tests reliable and fast.

---

172-250: Excellent false-positive prevention test coverage.

These tests comprehensively validate the endsWith() matching strategy, ensuring shell names appearing as directory substrings (e.g., pwsh-tools, bash-scripts, cmd-admin) don't cause misdetection. The complex mixed scenarios at lines 237-248 are particularly valuable.

---

35-118: LGTM! Comprehensive shell detection test coverage.

The tests cover all supported shell types with multiple path variations (Windows .exe, Unix paths, WSL, MSYS2, Cygwin). The cmd false-positive test at lines 76-83 directly validates the bug fix mentioned in the PR.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Use try/finally to ensure platform restoration on test failure.

If an assertion fails before the restore line, process.platform remains modified, potentially affecting subsequent tests in the same run.

🧪 Suggested fix using try/finally

 it('should return unknown for unrecognized Windows shells', () => {
   // Save original platform
   const originalPlatform = process.platform;
   Object.defineProperty(process, 'platform', { value: 'win32' });
 
-  expect(detectShellType('C:\\some\\random\\shell.exe')).toBe('unknown');
-
-  // Restore platform
-  Object.defineProperty(process, 'platform', { value: originalPlatform });
+  try {
+    expect(detectShellType('C:\\some\\random\\shell.exe')).toBe('unknown');
+  } finally {
+    // Restore platform
+    Object.defineProperty(process, 'platform', { value: originalPlatform });
+  }
 });

 it('should fallback to bash for unrecognized Unix shells', () => {
   // Save original platform
   const originalPlatform = process.platform;
   Object.defineProperty(process, 'platform', { value: 'darwin' });
 
-  expect(detectShellType('/usr/local/bin/fish')).toBe('bash');
-
-  // Restore platform
-  Object.defineProperty(process, 'platform', { value: originalPlatform });
+  try {
+    expect(detectShellType('/usr/local/bin/fish')).toBe('bash');
+  } finally {
+    // Restore platform
+    Object.defineProperty(process, 'platform', { value: originalPlatform });
+  }
 });

📝 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.

Suggested change

	it('should return unknown for unrecognized Windows shells', () => {
	// Save original platform
	const originalPlatform = process.platform;
	Object.defineProperty(process, 'platform', { value: 'win32' });
	expect(detectShellType('C:\\some\\random\\shell.exe')).toBe('unknown');
	// Restore platform
	Object.defineProperty(process, 'platform', { value: originalPlatform });
	});
	it('should fallback to bash for unrecognized Unix shells', () => {
	// Save original platform
	const originalPlatform = process.platform;
	Object.defineProperty(process, 'platform', { value: 'darwin' });
	expect(detectShellType('/usr/local/bin/fish')).toBe('bash');
	// Restore platform
	Object.defineProperty(process, 'platform', { value: originalPlatform });
	});
	it('should return unknown for unrecognized Windows shells', () => {
	// Save original platform
	const originalPlatform = process.platform;
	Object.defineProperty(process, 'platform', { value: 'win32' });
	try {
	expect(detectShellType('C:\\some\\random\\shell.exe')).toBe('unknown');
	} finally {
	// Restore platform
	Object.defineProperty(process, 'platform', { value: originalPlatform });
	}
	});
	it('should fallback to bash for unrecognized Unix shells', () => {
	// Save original platform
	const originalPlatform = process.platform;
	Object.defineProperty(process, 'platform', { value: 'darwin' });
	try {
	expect(detectShellType('/usr/local/bin/fish')).toBe('bash');
	} finally {
	// Restore platform
	Object.defineProperty(process, 'platform', { value: originalPlatform });
	}
	});

🤖 Prompt for AI Agents

In @apps/frontend/src/main/terminal/__tests__/pty-manager.test.ts around lines
127 - 147, The tests that override process.platform (the two it blocks asserting
detectShellType for Windows and Unix) must restore the original platform inside
a finally to guarantee restoration on failure; wrap the platform mutation and
expect call in a try/finally, capturing originalPlatform before the change and
resetting process.platform in the finally block so detectShellType tests cannot
leak a modified process.platform to other tests.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Solid shell detection logic with minor redundancy.

The implementation correctly uses endsWith() for precise path-boundary matching. One minor observation: the checks like endsWith('/bash') on line 68 are redundant since endsWith('bash') already covers any path ending with /bash. The same applies to zsh (line 76), pwsh (line 40), and powershell (line 48).

This doesn't affect correctness—just introduces a few unnecessary evaluations. The extra clarity may justify keeping them as-is.

🤖 Prompt for AI Agents

In @apps/frontend/src/main/terminal/pty-manager.ts around lines 33 - 84,
detectShellType has redundant path checks like endsWith('/pwsh'),
endsWith('/powershell'), endsWith('/bash'), and endsWith('/zsh') because
endsWith('pwsh'|'powershell'|'bash'|'zsh') already covers those cases; simplify
the conditionals in detectShellType by removing the redundant endsWith('/...')
checks (leave platform-specific/backslash checks for Windows cmd as-is) and
update the related comments to reflect the simplified matching logic so behavior
remains identical but with fewer evaluations.

[mferraznw]

can we get this in pls?