fix: address PR review feedback

- Remove duplicate killPtyProcess method in claude-usage-service.ts
- Import and use spawnSync correctly instead of spawn.sync
- Fix misleading comment about shell option and signal handling
This commit is contained in:
Scott
2026-01-18 13:06:13 -07:00
parent 0c452a3ebc
commit 8b5da3195b
2 changed files with 5 additions and 21 deletions

View File

@@ -46,22 +46,6 @@ export class ClaudeUsageService {
}
}
/**
* Kill a PTY process with platform-specific handling.
* Windows doesn't support Unix signals like SIGTERM, so we call kill() without arguments.
* On Unix-like systems (macOS, Linux), we can specify the signal.
*
* @param ptyProcess - The PTY process to kill
* @param signal - The signal to send on Unix-like systems (default: 'SIGTERM')
*/
private killPtyProcess(ptyProcess: pty.IPty, signal: string = 'SIGTERM'): void {
if (this.isWindows) {
ptyProcess.kill();
} else {
ptyProcess.kill(signal);
}
}
/**
* Check if Claude CLI is available on the system
*/

View File

@@ -4,7 +4,7 @@
* Works on Windows (CMD, PowerShell, Git Bash) and Unix (macOS, Linux)
*/
import { spawn } from 'child_process';
import { spawn, spawnSync } from 'child_process';
import { existsSync } from 'fs';
import { platform } from 'os';
import { fileURLToPath } from 'url';
@@ -38,12 +38,12 @@ function findBashOnWindows() {
if (bashPath === 'bash.exe') {
// Check if bash is in PATH
try {
const result = spawn.sync?.('where', ['bash.exe'], { stdio: 'pipe' });
const result = spawnSync('where', ['bash.exe'], { stdio: 'pipe' });
if (result?.status === 0) {
return 'bash.exe';
}
} catch {
// where command failed, continue checking
} catch (err) {
// where command failed, continue checking other paths
}
} else if (existsSync(bashPath)) {
return bashPath;
@@ -101,7 +101,7 @@ function runBashScript() {
// Ensure proper terminal handling
TERM: process.env.TERM || 'xterm-256color',
},
// On Windows, we need to use shell for proper signal handling
// shell: false ensures signals are forwarded directly to the child process
shell: false,
});