From 3c22430c7a14dc0b49a6f65c173219764694fe08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20Can=20G=C3=BCm=C3=BC=C5=9F?= Date: Thu, 2 Jul 2026 10:43:24 +0300 Subject: [PATCH] refactor(cli): consolidate duplicated agent-spawn logic across watch capabilities execute.ts and wave-dispatch.ts each carried their own copy of buildAgentCommand() and a spawn-with-timeout helper, diverging from the shared agent-spawn.ts module added for #920/#923 because they needed the withAdditionalMcpConfig wiring. Extract a buildCopilotCommand() helper and extend spawnAgent() with optional pid-tracking so both capabilities import from the shared module instead of keeping local copies. As a side effect, execute's Copilot spawn now goes through the same Windows cmd.exe argument-escaping path as the rest of watch. Fixes #994 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .changeset/consolidate-watch-agent-spawn.md | 5 + .../src/cli/commands/watch/agent-spawn.ts | 36 +++++- .../commands/watch/capabilities/execute.ts | 57 ++------- .../watch/capabilities/wave-dispatch.ts | 30 +---- .../squad-cli/src/cli/commands/watch/index.ts | 2 +- test/cli/watch-agent-spawn.test.ts | 113 ++++++++++++++++++ test/cli/watch-capabilities.test.ts | 28 +++++ 7 files changed, 194 insertions(+), 77 deletions(-) create mode 100644 .changeset/consolidate-watch-agent-spawn.md create mode 100644 test/cli/watch-agent-spawn.test.ts diff --git a/.changeset/consolidate-watch-agent-spawn.md b/.changeset/consolidate-watch-agent-spawn.md new file mode 100644 index 000000000..8e17a3923 --- /dev/null +++ b/.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. diff --git a/packages/squad-cli/src/cli/commands/watch/agent-spawn.ts b/packages/squad-cli/src/cli/commands/watch/agent-spawn.ts index 1843e4f57..f0f217f2b 100644 --- a/packages/squad-cli/src/cli/commands/watch/agent-spawn.ts +++ b/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; 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!)); + } }); } diff --git a/packages/squad-cli/src/cli/commands/watch/capabilities/execute.ts b/packages/squad-cli/src/cli/commands/watch/capabilities/execute.ts index 4aa367523..a3b0bdb5a 100644 --- a/packages/squad-cli/src/cli/commands/watch/capabilities/execute.ts +++ b/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 { diff --git a/packages/squad-cli/src/cli/commands/watch/capabilities/wave-dispatch.ts b/packages/squad-cli/src/cli/commands/watch/capabilities/wave-dispatch.ts index 0befa23f0..db36250c9 100644 --- a/packages/squad-cli/src/cli/commands/watch/capabilities/wave-dispatch.ts +++ b/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 { diff --git a/packages/squad-cli/src/cli/commands/watch/index.ts b/packages/squad-cli/src/cli/commands/watch/index.ts index d07026935..c1276cd15 100644 --- a/packages/squad-cli/src/cli/commands/watch/index.ts +++ b/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, }); diff --git a/test/cli/watch-agent-spawn.test.ts b/test/cli/watch-agent-spawn.test.ts new file mode 100644 index 000000000..1174f3181 --- /dev/null +++ b/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 { + 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); + }); +}); diff --git a/test/cli/watch-capabilities.test.ts b/test/cli/watch-capabilities.test.ts index 568bd493e..3d2689a5d 100644 --- a/test/cli/watch-capabilities.test.ts +++ b/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); + }); }); });