refactor(cli): consolidate duplicated agent-spawn logic across watch capabilities

.changeset/consolidate-watch-agent-spawn.md

@@ -0,0 +1,5 @@
+---
+"@bradygaster/squad-cli": patch
+---
+
+Consolidate the duplicated `buildAgentCommand()`/spawn-with-timeout logic in the `execute` and `wave-dispatch` watch capabilities into the shared `agent-spawn.ts` module (#994). As a side effect, `execute`'s Copilot session spawn now goes through the same Windows `cmd.exe` argument-escaping path (DEP0190) as the rest of the watch capabilities.

packages/squad-cli/src/cli/commands/watch/agent-spawn.ts

@@ -11,6 +11,7 @@
 
 import { execFile, execFileSync } from 'node:child_process';
 import type { WatchContext } from './types.js';
+import { withAdditionalMcpConfig } from '../../core/copilot-invocation.js';
 
 /** True when running on Windows — used to gate `shell: true`. */
 export const IS_WINDOWS = process.platform === 'win32';
@@ -116,6 +117,29 @@ export function buildAgentCommand(
   return { cmd, args };
 }
 
+/**
+ * Build the command + args array for a Copilot session, with the
+ * `--additional-mcp-config`/`--yolo` workaround injected so `squad_state_*`
+ * MCP tools register (see {@link withAdditionalMcpConfig}).
+ *
+ * Unlike {@link buildAgentCommand}, this always defaults to the bare
+ * `copilot` binary rather than probing for a `gh copilot` fallback — used
+ * by capabilities that spawn a full agent session against the repo
+ * (execute, wave-dispatch).
+ */
+export function buildCopilotCommand(
+  prompt: string,
+  context: WatchContext,
+): { cmd: string; args: string[] } {
+  if (context.agentCmd) {
+    const parts = context.agentCmd.trim().split(/\s+/);
+    return { cmd: parts[0]!, args: [...parts.slice(1), '-p', prompt] };
+  }
+  const args = ['-p', prompt];
+  if (context.copilotFlags) args.push(...context.copilotFlags.trim().split(/\s+/));
+  return { cmd: 'copilot', args: withAdditionalMcpConfig('copilot', args, context.teamRoot) };
+}
+
 /**
  * Spawn an agent command with a timeout.
  *
@@ -155,16 +179,21 @@ export function spawnWithTimeout(
  * Spawn an agent command with a timeout, resolving with success/error
  * instead of rejecting.  Used by execute and wave-dispatch where the
  * caller wants to handle failure without try/catch.
+ *
+ * Pass `pidTracking` when the caller wants the child process registered
+ * with a {@link WatchContext.pidTracker} for cleanup on exit/crash (e.g.
+ * the `execute` capability, which spawns long-running sessions).
  */
 export function spawnAgent(
   cmd: string,
   args: string[],
   cwd: string,
   timeoutMs: number,
+  pidTracking?: { tracker: NonNullable<WatchContext['pidTracker']>; label: string },
 ): Promise<{ success: boolean; error?: string }> {
   const safeArgs = escapeArgs(args);
   return new Promise<{ success: boolean; error?: string }>((resolve) => {
-    execFile(
+    const cp = execFile(
       cmd,
       safeArgs,
       {
@@ -183,5 +212,10 @@ export function spawnAgent(
         }
       },
     );
+
+    if (pidTracking && cp.pid) {
+      pidTracking.tracker.track(cp.pid, pidTracking.label);
+      cp.on('exit', () => pidTracking.tracker.untrack(cp.pid!));
+    }
   });
 }

packages/squad-cli/src/cli/commands/watch/capabilities/execute.ts

@@ -2,14 +2,14 @@
  * Execute capability — spawns Copilot sessions for eligible issues.
  */
 
-import { execFile, type ChildProcess } from 'node:child_process';
+import { execFile } from 'node:child_process';
 import { existsSync } from 'node:fs';
 import path from 'node:path';
 import type { WatchCapability, WatchContext, PreflightResult, CapabilityResult } from '../types.js';
 import type { MachineCapabilities } from '@bradygaster/squad-sdk/ralph/capabilities';
 import { createVerboseLogger } from '../verbose.js';
 import { loadAgentCharter } from '../../../shell/spawn.js';
-import { withAdditionalMcpConfig } from '../../../core/copilot-invocation.js';
+import { buildCopilotCommand, spawnAgent } from '../agent-spawn.js';
 
 /** Normalized work item for execution. */
 export interface ExecutableWorkItem {
@@ -47,24 +47,6 @@ export function classifyIssue(title: string): 'read' | 'write' {
   return 'write'; // default to write (safer — gets full agent session)
 }
 
-/** Build agent command for a prompt. */
-function buildAgentCommand(
-  prompt: string,
-  context: WatchContext,
-): { cmd: string; args: string[] } {
-  if (context.agentCmd) {
-    const parts = context.agentCmd.trim().split(/\s+/);
-    const cmd = parts[0]!;
-    const args = [...parts.slice(1), '-p', prompt];
-    return { cmd, args };
-  }
-  const args = ['-p', prompt];
-  if (context.copilotFlags) {
-    args.push(...context.copilotFlags.trim().split(/\s+/));
-  }
-  return { cmd: 'copilot', args: withAdditionalMcpConfig('copilot', args, context.teamRoot) };
-}
-
 /** Labels that indicate an issue should not be auto-executed. */
 const BLOCKING_LABELS = ['status:blocked', 'status:wontfix', 'status:on-hold', 'blocked'];
 
@@ -162,36 +144,15 @@ async function executeAll(
   }
 
   const fullPrompt = charterPrefix + prompt;
-  const { cmd, args } = buildAgentCommand(fullPrompt, context);
+  const { cmd, args } = buildCopilotCommand(fullPrompt, context);
 
-  return new Promise<{ success: boolean; error?: string }>((resolve) => {
-    const cp: ChildProcess = execFile(
-      cmd,
-      args,
-      { cwd: context.teamRoot, timeout: timeoutMs, maxBuffer: 50 * 1024 * 1024 },
-      (err) => {
-        if (err) {
-          const execErr = err as Error & { killed?: boolean };
-          const msg = execErr.killed ? `Timed out` : execErr.message;
-          resolve({ success: false, error: msg });
-        } else {
-          resolve({ success: true });
-        }
-      },
-    );
+  // Track child PID for cleanup on exit/crash
+  const issueNums = issues.map(i => `#${i.number}`).join(',');
+  const pidTracking = context.pidTracker
+    ? { tracker: context.pidTracker, label: `copilot-session-${issueNums}` }
+    : undefined;
 
-    // Track child PID for cleanup on exit/crash
-    if (context.pidTracker && cp.pid) {
-      const issueNums = issues.map(i => `#${i.number}`).join(',');
-      context.pidTracker.track(cp.pid, `copilot-session-${issueNums}`);
-    }
-
-    cp.on('exit', () => {
-      if (context.pidTracker && cp.pid) {
-        context.pidTracker.untrack(cp.pid);
-      }
-    });
-  });
+  return spawnAgent(cmd, args, context.teamRoot, timeoutMs, pidTracking);
 }
 
 export class ExecuteCapability implements WatchCapability {

packages/squad-cli/src/cli/commands/watch/capabilities/wave-dispatch.ts

@@ -2,9 +2,8 @@
  * WaveDispatch capability — parallel sub-task execution within issues.
  */
 
-import { execFile, type ChildProcess } from 'node:child_process';
 import type { WatchCapability, WatchContext, PreflightResult, CapabilityResult } from '../types.js';
-import { withAdditionalMcpConfig } from '../../../core/copilot-invocation.js';
+import { buildCopilotCommand, spawnAgent } from '../agent-spawn.js';
 
 interface SubTask {
   description: string;
@@ -35,36 +34,13 @@ function parseSubTasks(body: string | undefined): SubTask[] {
   return tasks;
 }
 
-function buildAgentCommand(prompt: string, context: WatchContext): { cmd: string; args: string[] } {
-  if (context.agentCmd) {
-    const parts = context.agentCmd.trim().split(/\s+/);
-    return { cmd: parts[0]!, args: [...parts.slice(1), '-p', prompt] };
-  }
-  const args = ['-p', prompt];
-  if (context.copilotFlags) args.push(...context.copilotFlags.trim().split(/\s+/));
-  return { cmd: 'copilot', args: withAdditionalMcpConfig('copilot', args, context.teamRoot) };
-}
-
 function executeSubTask(
   prompt: string,
   context: WatchContext,
   timeoutMs: number,
 ): Promise<{ success: boolean; error?: string }> {
-  const { cmd, args } = buildAgentCommand(prompt, context);
-  return new Promise((resolve) => {
-    const _cp: ChildProcess = execFile(
-      cmd, args,
-      { cwd: context.teamRoot, timeout: timeoutMs, maxBuffer: 50 * 1024 * 1024 },
-      (err) => {
-        if (err) {
-          const execErr = err as Error & { killed?: boolean };
-          resolve({ success: false, error: execErr.killed ? 'Timed out' : execErr.message });
-        } else {
-          resolve({ success: true });
-        }
-      },
-    );
-  });
+  const { cmd, args } = buildCopilotCommand(prompt, context);
+  return spawnAgent(cmd, args, context.teamRoot, timeoutMs);
 }
 
 export class WaveDispatchCapability implements WatchCapability {

packages/squad-cli/src/cli/commands/watch/index.ts

@@ -712,7 +712,7 @@ export async function runWatch(dest: string, options: WatchOptions | WatchConfig
     verbose: config.verbose ?? false,
     interval: `${config.interval}m`,
     execute: config.execute ?? false,
-    agentCmd: config.agentCmd ?? '(default: gh copilot)',
+    agentCmd: config.agentCmd ?? '(default: copilot)',
     dispatchMode: config.capabilities['wave-dispatch'] ? 'wave' : 'task',
     maxConcurrent: config.maxConcurrent ?? 1,
   });

test/cli/watch-agent-spawn.test.ts

@@ -0,0 +1,113 @@
+/**
+ * Tests for the shared agent-spawn utilities (issue #994).
+ *
+ * `buildCopilotCommand()` and the pid-tracking branch of `spawnAgent()`
+ * replace logic that used to be duplicated across the `execute` and
+ * `wave-dispatch` capabilities — cover them directly here.
+ */
+
+import { describe, it, expect, vi, beforeEach } from 'vitest';
+import type { WatchContext } from '../../packages/squad-cli/src/cli/commands/watch/types.js';
+
+const { mockExecFile, mockFsExistsSync } = vi.hoisted(() => ({
+  mockExecFile: vi.fn((...args: unknown[]) => {
+    const cb = args.find(a => typeof a === 'function') as
+      | ((...cbArgs: unknown[]) => void)
+      | undefined;
+    if (cb) cb(null, '', '');
+    return { pid: 1234, on: vi.fn() };
+  }),
+  mockFsExistsSync: vi.fn((): boolean => false),
+}));
+
+vi.mock('node:child_process', () => ({
+  execFile: mockExecFile,
+  execFileSync: vi.fn(),
+}));
+
+vi.mock('node:fs', () => ({
+  existsSync: mockFsExistsSync,
+}));
+
+import { buildCopilotCommand, spawnAgent } from '../../packages/squad-cli/src/cli/commands/watch/agent-spawn.js';
+
+function makeContext(overrides: Partial<WatchContext> = {}): WatchContext {
+  return {
+    teamRoot: '/fake/team',
+    adapter: {} as WatchContext['adapter'],
+    round: 1,
+    roster: [],
+    config: {},
+    ...overrides,
+  };
+}
+
+describe('agent-spawn: buildCopilotCommand', () => {
+  beforeEach(() => {
+    vi.clearAllMocks();
+    mockFsExistsSync.mockReturnValue(false);
+  });
+
+  it('uses agentCmd override and skips MCP injection', () => {
+    const ctx = makeContext({ agentCmd: 'my-agent --flag' });
+    const { cmd, args } = buildCopilotCommand('hello', ctx);
+    expect(cmd).toBe('my-agent');
+    expect(args).toEqual(['--flag', '-p', 'hello']);
+  });
+
+  it('defaults to the bare copilot binary and appends copilotFlags', () => {
+    const ctx = makeContext({ copilotFlags: '--foo' });
+    const { cmd, args } = buildCopilotCommand('hello', ctx);
+    expect(cmd).toBe('copilot');
+    expect(args).toEqual(['-p', 'hello', '--foo']);
+  });
+
+  it('injects --additional-mcp-config/--yolo when .mcp.json exists at teamRoot', () => {
+    mockFsExistsSync.mockReturnValue(true);
+    const ctx = makeContext({ teamRoot: '/repo' });
+    const { cmd, args } = buildCopilotCommand('hello', ctx);
+    expect(cmd).toBe('copilot');
+    expect(args[0]).toBe('--yolo');
+    expect(args).toContain('--additional-mcp-config');
+    expect(args).toContain('-p');
+    expect(args).toContain('hello');
+  });
+});
+
+describe('agent-spawn: spawnAgent pid tracking', () => {
+  beforeEach(() => vi.clearAllMocks());
+
+  it('tracks and untracks the child pid when pidTracking is provided', async () => {
+    let exitHandler: (() => void) | undefined;
+    mockExecFile.mockImplementation((...args: unknown[]) => {
+      const cb = args.find(a => typeof a === 'function') as
+        | ((...cbArgs: unknown[]) => void)
+        | undefined;
+      if (cb) cb(null, '', '');
+      return {
+        pid: 4242,
+        on: (event: string, handler: () => void) => {
+          if (event === 'exit') exitHandler = handler;
+        },
+      };
+    });
+    const track = vi.fn();
+    const untrack = vi.fn();
+
+    const result = await spawnAgent('copilot', ['-p', 'x'], '/repo', 1000, {
+      tracker: { track, untrack },
+      label: 'copilot-session-#1',
+    });
+
+    expect(result.success).toBe(true);
+    expect(track).toHaveBeenCalledWith(4242, 'copilot-session-#1');
+    expect(untrack).not.toHaveBeenCalled();
+    exitHandler?.();
+    expect(untrack).toHaveBeenCalledWith(4242);
+  });
+
+  it('does not touch a tracker when pidTracking is omitted', async () => {
+    const result = await spawnAgent('copilot', ['-p', 'x'], '/repo', 1000);
+    expect(result.success).toBe(true);
+  });
+});

test/cli/watch-capabilities.test.ts

@@ -361,6 +361,34 @@ describe('Watch Capabilities', () => {
           expect.any(Function),
         );
       });
+
+      it('tracks and untracks the spawned pid via context.pidTracker', async () => {
+        let exitHandler: (() => void) | undefined;
+        mockExecFile.mockImplementation((...args: unknown[]) => {
+          const cb = findCallback(args);
+          if (cb) cb(null, '', '');
+          return {
+            pid: 4242,
+            on: (event: string, handler: () => void) => {
+              if (event === 'exit') exitHandler = handler;
+            },
+          };
+        });
+        const track = vi.fn();
+        const untrack = vi.fn();
+        const cap = new ExecuteCapability();
+        const ctx = makeContext({
+          pidTracker: { track, untrack },
+          adapter: mockAdapter([{ id: 1, title: 'Fix', tags: ['squad'] }]),
+        });
+
+        await cap.execute(ctx);
+
+        expect(track).toHaveBeenCalledWith(4242, expect.stringContaining('#1'));
+        expect(untrack).not.toHaveBeenCalled();
+        exitHandler?.();
+        expect(untrack).toHaveBeenCalledWith(4242);
+      });
     });
   });