[Bugfix #389] Fix: Kill shellper processes during af cleanup

packages/codev/src/agent-farm/__tests__/cleanup-shellper-kill.test.ts

@@ -0,0 +1,166 @@
+/**
+ * Tests for cleanup command — shellper process kill (Bugfix #389)
+ *
+ * When `af cleanup` runs, it must kill shellper processes associated with the
+ * builder's worktree. Previously, cleanup relied solely on the Tower API which
+ * silently fails when Tower is not running or the terminal was already removed.
+ *
+ * The fix adds a direct `ps`-based search for shellper-main.js processes whose
+ * JSON config contains the worktree path, killing them via process group signal.
+ */
+
+import { describe, it, expect, vi, afterEach } from 'vitest';
+import { killShellperProcesses } from '../commands/cleanup.js';
+import { execFile } from 'node:child_process';
+
+// Mock execFile to simulate ps output
+vi.mock('node:child_process', async () => {
+ const actual = await vi.importActual<typeof import('node:child_process')>('node:child_process');
+ return {
+ ...actual,
+ execFile: vi.fn(),
+ };
+});
+
+const mockExecFile = vi.mocked(execFile);
+
+// Mock process.kill to track kill signals
+const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => true);
+
+afterEach(() => {
+ vi.clearAllMocks();
+});
+
+describe('killShellperProcesses (Bugfix #389)', () => {
+ const worktree = '/workspace/.builders/bugfix-42-login-fails';
+
+ function simulatePsOutput(stdout: string): void {
+ mockExecFile.mockImplementation((_cmd, _args, callback) => {
+ (callback as (err: Error | null, stdout: string) => void)(null, stdout);
+ return {} as ReturnType<typeof execFile>;
+ });
+ }
+
+ function simulatePsError(): void {
+ mockExecFile.mockImplementation((_cmd, _args, callback) => {
+ (callback as (err: Error | null, stdout: string) => void)(new Error('ps failed'), '');
+ return {} as ReturnType<typeof execFile>;
+ });
+ }
+
+ it('kills shellper processes matching the worktree cwd', async () => {
+ simulatePsOutput(
+ ` PID ARGS\n` +
+ `12345 node /path/to/shellper-main.js {"command":"/bin/bash","args":[],"cwd":"${worktree}","socketPath":"/tmp/shellper-abc.sock"}\n` +
+ `99999 node /usr/bin/some-other-process\n`
+ );
+
+ const killed = await killShellperProcesses(worktree);
+
+ expect(killed).toBe(1);
+ // Should attempt process group kill first (-pid)
+ expect(killSpy).toHaveBeenCalledWith(-12345, 'SIGTERM');
+ });
+
+ it('does not kill shellper processes for different worktrees', async () => {
+ simulatePsOutput(
+ ` PID ARGS\n` +
+ `12345 node /path/to/shellper-main.js {"command":"/bin/bash","args":[],"cwd":"/workspace/.builders/bugfix-99-other","socketPath":"/tmp/shellper-abc.sock"}\n`
+ );
+
+ const killed = await killShellperProcesses(worktree);
+
+ expect(killed).toBe(0);
+ expect(killSpy).not.toHaveBeenCalled();
+ });
+
+ it('does not match partial worktree path prefixes', async () => {
+ // bugfix-42 should NOT match bugfix-42-login-fails-continued
+ const shortWorktree = '/workspace/.builders/bugfix-42';
+ simulatePsOutput(
+ ` PID ARGS\n` +
+ `12345 node /path/to/shellper-main.js {"command":"/bin/bash","args":[],"cwd":"${shortWorktree}-login-fails-continued","socketPath":"/tmp/shellper-abc.sock"}\n`
+ );
+
+ const killed = await killShellperProcesses(shortWorktree);
+
+ expect(killed).toBe(0);
+ expect(killSpy).not.toHaveBeenCalled();
+ });
+
+ it('kills multiple shellper processes for the same worktree', async () => {
+ simulatePsOutput(
+ ` PID ARGS\n` +
+ `12345 node /path/to/shellper-main.js {"command":"/bin/bash","args":[],"cwd":"${worktree}","socketPath":"/tmp/shellper-abc.sock"}\n` +
+ `12346 node /path/to/shellper-main.js {"command":"/bin/bash","args":[],"cwd":"${worktree}","socketPath":"/tmp/shellper-def.sock"}\n`
+ );
+
+ const killed = await killShellperProcesses(worktree);
+
+ expect(killed).toBe(2);
+ expect(killSpy).toHaveBeenCalledWith(-12345, 'SIGTERM');
+ expect(killSpy).toHaveBeenCalledWith(-12346, 'SIGTERM');
+ });
+
+ it('falls back to individual PID kill when process group kill fails', async () => {
+ simulatePsOutput(
+ ` PID ARGS\n` +
+ `12345 node /path/to/shellper-main.js {"command":"/bin/bash","args":[],"cwd":"${worktree}","socketPath":"/tmp/shellper-abc.sock"}\n`
+ );
+
+ // First call (-pid group kill) fails, second call (individual pid) succeeds
+ killSpy
+ .mockImplementationOnce(() => { throw new Error('ESRCH'); })
+ .mockImplementationOnce(() => true);
+
+ const killed = await killShellperProcesses(worktree);
+
+ expect(killed).toBe(1);
+ expect(killSpy).toHaveBeenCalledWith(-12345, 'SIGTERM');
+ expect(killSpy).toHaveBeenCalledWith(12345, 'SIGTERM');
+ });
+
+ it('returns 0 when no shellper processes found', async () => {
+ simulatePsOutput(
+ ` PID ARGS\n` +
+ `99999 node /usr/bin/some-other-process\n`
+ );
+
+ const killed = await killShellperProcesses(worktree);
+
+ expect(killed).toBe(0);
+ expect(killSpy).not.toHaveBeenCalled();
+ });
+
+ it('returns 0 gracefully when ps fails', async () => {
+ simulatePsError();
+
+ const killed = await killShellperProcesses(worktree);
+
+ expect(killed).toBe(0);
+ });
+
+ it('skips non-shellper processes even if they mention the worktree', async () => {
+ simulatePsOutput(
+ ` PID ARGS\n` +
+ `12345 claude --cwd ${worktree}\n`
+ );
+
+ const killed = await killShellperProcesses(worktree);
+
+ expect(killed).toBe(0);
+ expect(killSpy).not.toHaveBeenCalled();
+ });
+
+ it('does not kill its own process', async () => {
+ simulatePsOutput(
+ ` PID ARGS\n` +
+ `${process.pid} node /path/to/shellper-main.js {"command":"/bin/bash","args":[],"cwd":"${worktree}","socketPath":"/tmp/shellper-abc.sock"}\n`
+ );
+
+ const killed = await killShellperProcesses(worktree);
+
+ expect(killed).toBe(0);
+ expect(killSpy).not.toHaveBeenCalled();
+ });
+});

packages/codev/src/agent-farm/commands/cleanup.ts

@@ -5,6 +5,7 @@
 import { existsSync, readdirSync } from 'node:fs';
 import { rm } from 'node:fs/promises';
 import { join } from 'node:path';
+import { execFile } from 'node:child_process';
 import type { Builder, Config } from '../types.js';
 import { getConfig } from '../utils/index.js';
 import { logger, fatal } from '../utils/logger.js';
@@ -37,6 +38,60 @@ async function cleanupPorchState(projectId: string, config: Config): Promise<voi
  }
 }
 
+/**
+ * Find and kill shellper processes associated with a worktree path (Bugfix #389).
+ *
+ * When Tower is not running (or the terminal was already removed from Tower's
+ * registry), the Tower API kill path silently fails, leaving shellper processes
+ * orphaned. This function searches `ps` output for shellper-main.js processes
+ * whose JSON config contains the worktree path as `cwd`, and kills them directly.
+ *
+ * Uses process group kill (-pid) to also terminate PTY children (Claude, bash).
+ */
+export async function killShellperProcesses(worktreePath: string): Promise<number> {
+ let killed = 0;
+ try {
+ const stdout = await new Promise<string>((resolve, reject) => {
+ // -ww prevents arg truncation on macOS/Linux
+ execFile('ps', ['-ww', '-eo', 'pid,args'], (err, out) => {
+ if (err) { reject(err); return; }
+ resolve(out);
+ });
+ });
+
+ // Match shellper-main.js processes whose JSON config cwd is this worktree.
+ // The shellper is spawned with JSON as argv[2]: {"cwd":"/path/to/worktree",...}
+ const cwdPattern = `"cwd":"${worktreePath}"`;
+
+ for (const line of stdout.split('\n')) {
+ if (!line.includes('shellper-main.js')) continue;
+ if (!line.includes(cwdPattern)) continue;
+
+ const pid = parseInt(line.trim(), 10);
+ if (isNaN(pid) || pid <= 0 || pid === process.pid) continue;
+
+ try {
+ // Kill process group (shellper + its PTY child) to prevent orphaned
+ // PTY processes. Shellper is spawned with detached:true, so it's a
+ // process group leader.
+ process.kill(-pid, 'SIGTERM');
+ killed++;
+ } catch {
+ // Process group kill failed — try individual PID
+ try {
+ process.kill(pid, 'SIGTERM');
+ killed++;
+ } catch {
+ // Process already dead
+ }
+ }
+ }
+ } catch {
+ // ps not available or failed — non-fatal
+ }
+ return killed;
+}
+
 export interface CleanupOptions {
  project?: string;
  issue?: number;
@@ -202,6 +257,16 @@ async function cleanupBuilder(builder: Builder, force?: boolean, issueNumber?: n
  }
  }
 
+ // Bugfix #389: Kill shellper processes directly by worktree path.
+ // The Tower API kill may fail if Tower isn't running, the terminal was already
+ // removed, or Tower was restarted. This catches any surviving shellper processes.
+ if (!isShellMode && builder.worktree) {
+ const shellpersKilled = await killShellperProcesses(builder.worktree);
+ if (shellpersKilled > 0) {
+ logger.info(`Killed ${shellpersKilled} shellper process(es)`);
+ }
+ }
+
  // For ephemeral builders (bugfix, task): actually remove worktree and delete branches
  if (isEphemeral && !isShellMode) {
  // Remove worktree