From 2579eb0729d1d2c4de1d6608362a7dc9db45d7df Mon Sep 17 00:00:00 2001 From: 2witstudios <2witstudios@gmail.com> Date: Fri, 27 Feb 2026 00:06:01 -0600 Subject: [PATCH 1/3] feat: extract kill operation to core/operations/kill.ts Move agent kill logic from commands/kill.ts into a reusable core/operations/kill.ts module. The CLI command is now a thin wrapper that handles arg parsing and output formatting, delegating all kill logic to performKill(). Closes #61 --- src/commands/kill.ts | 339 ++++---------------------- src/core/operations/kill.test.ts | 403 +++++++++++++++++++++++++++++++ src/core/operations/kill.ts | 257 ++++++++++++++++++++ 3 files changed, 707 insertions(+), 292 deletions(-) create mode 100644 src/core/operations/kill.test.ts create mode 100644 src/core/operations/kill.ts diff --git a/src/commands/kill.ts b/src/commands/kill.ts index 294b6ee..3d186a9 100644 --- a/src/commands/kill.ts +++ b/src/commands/kill.ts @@ -1,13 +1,9 @@ -import { readManifest, updateManifest, findAgent, resolveWorktree } from '../core/manifest.js'; -import { killAgent, killAgents } from '../core/agent.js'; -import { checkPrState } from '../core/pr.js'; +import { performKill, type KillResult } from '../core/operations/kill.js'; +import { getCurrentPaneId } from '../core/self.js'; +import { readManifest } from '../core/manifest.js'; import { getRepoRoot } from '../core/worktree.js'; -import { cleanupWorktree } from '../core/cleanup.js'; -import { getCurrentPaneId, excludeSelf } from '../core/self.js'; -import { listSessionPanes, type PaneInfo } from '../core/tmux.js'; -import { PpgError, NotInitializedError, AgentNotFoundError, WorktreeNotFoundError } from '../lib/errors.js'; +import { listSessionPanes } from '../core/tmux.js'; import { output, success, info, warn } from '../lib/output.js'; -import type { AgentEntry } from '../types/manifest.js'; export interface KillOptions { agent?: string; @@ -22,314 +18,73 @@ export interface KillOptions { export async function killCommand(options: KillOptions): Promise { const projectRoot = await getRepoRoot(); - if (!options.agent && !options.worktree && !options.all) { - throw new PpgError('One of --agent, --worktree, or --all is required', 'INVALID_ARGS'); - } - // Capture self-identification once at the start const selfPaneId = getCurrentPaneId(); - let paneMap: Map | undefined; + let paneMap: Map | undefined; if (selfPaneId) { const manifest = await readManifest(projectRoot); paneMap = await listSessionPanes(manifest.sessionName); } - if (options.agent) { - await killSingleAgent(projectRoot, options.agent, options, selfPaneId, paneMap); - } else if (options.worktree) { - await killWorktreeAgents(projectRoot, options.worktree, options, selfPaneId, paneMap); - } else if (options.all) { - await killAllAgents(projectRoot, options, selfPaneId, paneMap); - } -} - -async function killSingleAgent( - projectRoot: string, - agentId: string, - options: KillOptions, - selfPaneId: string | null, - paneMap?: Map, -): Promise { - const manifest = await readManifest(projectRoot); - const found = findAgent(manifest, agentId); - if (!found) throw new AgentNotFoundError(agentId); - - const { agent } = found; - const isTerminal = agent.status !== 'running'; - - // Self-protection check - if (selfPaneId && paneMap) { - const { skipped } = excludeSelf([agent], selfPaneId, paneMap); - if (skipped.length > 0) { - warn(`Cannot kill agent ${agentId} — it contains the current ppg process`); - if (options.json) { - output({ success: false, skipped: [agentId], reason: 'self-protection' }, true); - } - return; - } - } - - if (options.delete) { - // For --delete: skip kill if already in terminal state, just clean up - if (!isTerminal) { - info(`Killing agent ${agentId}`); - await killAgent(agent); - } - // Kill the tmux pane explicitly (handles already-dead) - await import('../core/tmux.js').then((tmux) => tmux.killPane(agent.tmuxTarget)); - - await updateManifest(projectRoot, (m) => { - const f = findAgent(m, agentId); - if (f) { - delete f.worktree.agents[agentId]; - } - return m; - }); - - if (options.json) { - output({ success: true, killed: [agentId], deleted: [agentId] }, true); - } else { - success(`Deleted agent ${agentId}`); - } - } else { - if (isTerminal) { - if (options.json) { - output({ success: true, killed: [], message: `Agent ${agentId} already ${agent.status}` }, true); - } else { - info(`Agent ${agentId} already ${agent.status}, skipping kill`); - } - return; - } - - info(`Killing agent ${agentId}`); - await killAgent(agent); - - await updateManifest(projectRoot, (m) => { - const f = findAgent(m, agentId); - if (f) { - f.agent.status = 'gone'; - } - return m; - }); - - if (options.json) { - output({ success: true, killed: [agentId] }, true); - } else { - success(`Killed agent ${agentId}`); - } - } -} - -async function killWorktreeAgents( - projectRoot: string, - worktreeRef: string, - options: KillOptions, - selfPaneId: string | null, - paneMap?: Map, -): Promise { - const manifest = await readManifest(projectRoot); - const wt = resolveWorktree(manifest, worktreeRef); - - if (!wt) throw new WorktreeNotFoundError(worktreeRef); - - let toKill = Object.values(wt.agents) - .filter((a) => a.status === 'running'); - - // Self-protection: filter out agents that would kill the current process - const skippedIds: string[] = []; - if (selfPaneId && paneMap) { - const { safe, skipped } = excludeSelf(toKill, selfPaneId, paneMap); - toKill = safe; - for (const a of skipped) { - skippedIds.push(a.id); - warn(`Skipping agent ${a.id} — contains current ppg process`); - } - } - - const killedIds = toKill.map((a) => a.id); - - for (const a of toKill) info(`Killing agent ${a.id}`); - await killAgents(toKill); - - await updateManifest(projectRoot, (m) => { - const mWt = m.worktrees[wt.id]; - if (mWt) { - for (const agent of Object.values(mWt.agents)) { - if (killedIds.includes(agent.id)) { - agent.status = 'gone'; - } - } - } - return m; + const result = await performKill({ + projectRoot, + agent: options.agent, + worktree: options.worktree, + all: options.all, + remove: options.remove, + delete: options.delete, + includeOpenPrs: options.includeOpenPrs, + selfPaneId, + paneMap, }); - // Check for open PR before deleting worktree - let skippedOpenPr = false; - if (options.delete && !options.includeOpenPrs) { - const prState = await checkPrState(wt.branch); - if (prState === 'OPEN') { - skippedOpenPr = true; - warn(`Skipping deletion of worktree ${wt.id} (${wt.name}) — has open PR on branch ${wt.branch}. Use --include-open-prs to override.`); - } - } - - // --delete implies --remove (always clean up worktree) - const shouldRemove = (options.remove || options.delete) && !skippedOpenPr; - if (shouldRemove) { - await removeWorktreeCleanup(projectRoot, wt.id, selfPaneId, paneMap); - } - - // --delete also removes the worktree entry from manifest - if (options.delete && !skippedOpenPr) { - await updateManifest(projectRoot, (m) => { - delete m.worktrees[wt.id]; - return m; - }); - } + formatOutput(result, options); +} +function formatOutput(result: KillResult, options: KillOptions): void { if (options.json) { - output({ - success: true, - killed: killedIds, - skipped: skippedIds.length > 0 ? skippedIds : undefined, - removed: shouldRemove ? [wt.id] : [], - deleted: (options.delete && !skippedOpenPr) ? [wt.id] : [], - skippedOpenPrs: skippedOpenPr ? [wt.id] : undefined, - }, true); - } else { - success(`Killed ${killedIds.length} agent(s) in worktree ${wt.id}`); - if (skippedIds.length > 0) { - warn(`Skipped ${skippedIds.length} agent(s) due to self-protection`); - } - if (options.delete && !skippedOpenPr) { - success(`Deleted worktree ${wt.id}`); - } else if (options.remove && !skippedOpenPr) { - success(`Removed worktree ${wt.id}`); - } + output({ success: true, ...result }, true); + return; } -} -async function killAllAgents( - projectRoot: string, - options: KillOptions, - selfPaneId: string | null, - paneMap?: Map, -): Promise { - const manifest = await readManifest(projectRoot); - let toKill: AgentEntry[] = []; - - for (const wt of Object.values(manifest.worktrees)) { - for (const agent of Object.values(wt.agents)) { - if (agent.status === 'running') { - toKill.push(agent); - } - } + if (result.message) { + info(result.message); } - // Self-protection: filter out agents that would kill the current process - const skippedIds: string[] = []; - if (selfPaneId && paneMap) { - const { safe, skipped } = excludeSelf(toKill, selfPaneId, paneMap); - toKill = safe; - for (const a of skipped) { - skippedIds.push(a.id); - warn(`Skipping agent ${a.id} — contains current ppg process`); + if (result.skipped?.length) { + for (const id of result.skipped) { + warn(`Skipping agent ${id} — contains current ppg process`); } } - const killedIds = toKill.map((a) => a.id); - for (const a of toKill) info(`Killing agent ${a.id}`); - await killAgents(toKill); - - // Only track active worktrees for removal (not already merged/cleaned) - const activeWorktreeIds = Object.values(manifest.worktrees) - .filter((wt) => wt.status === 'active') - .map((wt) => wt.id); - - await updateManifest(projectRoot, (m) => { - for (const wt of Object.values(m.worktrees)) { - for (const agent of Object.values(wt.agents)) { - if (killedIds.includes(agent.id)) { - agent.status = 'gone'; - } - } - } - return m; - }); - - // Filter out worktrees with open PRs - let worktreesToRemove = activeWorktreeIds; - const openPrWorktreeIds: string[] = []; - if (options.delete && !options.includeOpenPrs) { - worktreesToRemove = []; - for (const wtId of activeWorktreeIds) { - const wt = manifest.worktrees[wtId]; - if (wt) { - const prState = await checkPrState(wt.branch); - if (prState === 'OPEN') { - openPrWorktreeIds.push(wtId); - warn(`Skipping deletion of worktree ${wtId} (${wt.name}) — has open PR`); - } else { - worktreesToRemove.push(wtId); - } - } + if (result.skippedOpenPrs?.length) { + for (const id of result.skippedOpenPrs) { + warn(`Skipping deletion of worktree ${id} — has open PR`); } } - // --delete implies --remove - const shouldRemove = options.remove || options.delete; - if (shouldRemove) { - for (const wtId of worktreesToRemove) { - await removeWorktreeCleanup(projectRoot, wtId, selfPaneId, paneMap); + if (options.agent) { + if (result.deleted?.length) { + success(`Deleted agent ${options.agent}`); + } else if (result.killed.length > 0) { + success(`Killed agent ${options.agent}`); } - } - - // --delete also removes worktree entries from manifest - if (options.delete) { - await updateManifest(projectRoot, (m) => { - for (const wtId of worktreesToRemove) { - delete m.worktrees[wtId]; - } - return m; - }); - } - - if (options.json) { - output({ - success: true, - killed: killedIds, - skipped: skippedIds.length > 0 ? skippedIds : undefined, - removed: shouldRemove ? worktreesToRemove : [], - deleted: options.delete ? worktreesToRemove : [], - skippedOpenPrs: openPrWorktreeIds.length > 0 ? openPrWorktreeIds : undefined, - }, true); - } else { - success(`Killed ${killedIds.length} agent(s) across ${activeWorktreeIds.length} worktree(s)`); - if (skippedIds.length > 0) { - warn(`Skipped ${skippedIds.length} agent(s) due to self-protection`); + } else if (options.worktree) { + success(`Killed ${result.killed.length} agent(s) in worktree ${options.worktree}`); + if (result.deleted?.length) { + success(`Deleted worktree ${options.worktree}`); + } else if (result.removed?.length) { + success(`Removed worktree ${options.worktree}`); } - if (openPrWorktreeIds.length > 0) { - warn(`Skipped deletion of ${openPrWorktreeIds.length} worktree(s) with open PRs`); + } else if (options.all) { + success(`Killed ${result.killed.length} agent(s)`); + if (result.skipped?.length) { + warn(`Skipped ${result.skipped.length} agent(s) due to self-protection`); } - if (options.delete) { - success(`Deleted ${worktreesToRemove.length} worktree(s)`); - } else if (options.remove) { - success(`Removed ${worktreesToRemove.length} worktree(s)`); + if (result.deleted?.length) { + success(`Deleted ${result.deleted.length} worktree(s)`); + } else if (result.removed?.length) { + success(`Removed ${result.removed.length} worktree(s)`); } } } - -async function removeWorktreeCleanup( - projectRoot: string, - wtId: string, - selfPaneId: string | null, - paneMap?: Map, -): Promise { - const manifest = await readManifest(projectRoot); - const wt = resolveWorktree(manifest, wtId); - if (!wt) return; - await cleanupWorktree(projectRoot, wt, { - selfPaneId, - paneMap, - }); -} diff --git a/src/core/operations/kill.test.ts b/src/core/operations/kill.test.ts new file mode 100644 index 0000000..2f97654 --- /dev/null +++ b/src/core/operations/kill.test.ts @@ -0,0 +1,403 @@ +import { describe, test, expect, vi, beforeEach } from 'vitest'; +import type { Manifest, AgentEntry, WorktreeEntry } from '../../types/manifest.js'; +import type { PaneInfo } from '../tmux.js'; + +// --- Mocks --- + +vi.mock('../manifest.js', () => ({ + readManifest: vi.fn(), + updateManifest: vi.fn(async (_root: string, updater: (m: any) => any) => { + return updater(currentManifest()); + }), + findAgent: vi.fn(), + resolveWorktree: vi.fn(), +})); + +vi.mock('../agent.js', () => ({ + killAgent: vi.fn(async () => {}), + killAgents: vi.fn(async () => {}), +})); + +vi.mock('../pr.js', () => ({ + checkPrState: vi.fn(async () => 'UNKNOWN'), +})); + +vi.mock('../cleanup.js', () => ({ + cleanupWorktree: vi.fn(async () => ({ + worktreeId: 'wt-abc123', + manifestUpdated: true, + tmuxKilled: 1, + tmuxSkipped: 0, + tmuxFailed: 0, + selfProtected: false, + selfProtectedTargets: [], + })), +})); + +vi.mock('../self.js', () => ({ + excludeSelf: vi.fn((agents: AgentEntry[]) => ({ safe: agents, skipped: [] })), +})); + +vi.mock('../tmux.js', () => ({ + killPane: vi.fn(async () => {}), + listSessionPanes: vi.fn(async () => new Map()), +})); + +import { performKill } from './kill.js'; +import { readManifest, updateManifest, findAgent, resolveWorktree } from '../manifest.js'; +import { killAgent, killAgents } from '../agent.js'; +import { checkPrState } from '../pr.js'; +import { cleanupWorktree } from '../cleanup.js'; +import { excludeSelf } from '../self.js'; +import { killPane } from '../tmux.js'; +import { PpgError } from '../../lib/errors.js'; + +// --- Helpers --- + +function makeAgent(id: string, overrides: Partial = {}): AgentEntry { + return { + id, + name: 'test', + agentType: 'claude', + status: 'running', + tmuxTarget: 'ppg:1.0', + prompt: 'do stuff', + startedAt: new Date().toISOString(), + ...overrides, + }; +} + +function makeWorktree(overrides: Partial = {}): WorktreeEntry { + return { + id: 'wt-abc123', + name: 'test-wt', + path: '/tmp/wt', + branch: 'ppg/test-wt', + baseBranch: 'main', + status: 'active', + tmuxWindow: 'ppg:1', + agents: {}, + createdAt: new Date().toISOString(), + ...overrides, + }; +} + +function makeManifest(worktrees: Record = {}): Manifest { + return { + version: 1, + projectRoot: '/project', + sessionName: 'ppg', + worktrees, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + }; +} + +// Shared manifest reference for updateManifest mock +let _manifest: Manifest; +function currentManifest(): Manifest { + return JSON.parse(JSON.stringify(_manifest)); +} + +// --- Tests --- + +describe('performKill', () => { + beforeEach(() => { + vi.restoreAllMocks(); + _manifest = makeManifest(); + // Re-establish default mock implementations after restore + vi.mocked(readManifest).mockResolvedValue(_manifest); + vi.mocked(updateManifest).mockImplementation(async (_root: string, updater: (m: any) => any) => { + return updater(currentManifest()); + }); + vi.mocked(findAgent).mockReturnValue(undefined); + vi.mocked(resolveWorktree).mockReturnValue(undefined); + vi.mocked(killAgent).mockResolvedValue(undefined); + vi.mocked(killAgents).mockResolvedValue(undefined); + vi.mocked(checkPrState).mockResolvedValue('UNKNOWN'); + vi.mocked(cleanupWorktree).mockResolvedValue({ + worktreeId: 'wt-abc123', + manifestUpdated: true, + tmuxKilled: 1, + tmuxSkipped: 0, + tmuxFailed: 0, + selfProtected: false, + selfProtectedTargets: [], + }); + vi.mocked(excludeSelf).mockImplementation((agents: AgentEntry[]) => ({ safe: agents, skipped: [] })); + vi.mocked(killPane).mockResolvedValue(undefined); + }); + + test('throws INVALID_ARGS when no target specified', async () => { + const err = await performKill({ projectRoot: '/project' }).catch((e) => e); + expect(err).toBeInstanceOf(PpgError); + expect((err as PpgError).code).toBe('INVALID_ARGS'); + }); + + describe('single agent kill', () => { + beforeEach(() => { + const agent = makeAgent('ag-12345678'); + const wt = makeWorktree({ agents: { 'ag-12345678': agent } }); + _manifest = makeManifest({ 'wt-abc123': wt }); + vi.mocked(readManifest).mockResolvedValue(_manifest); + vi.mocked(findAgent).mockReturnValue({ worktree: wt, agent }); + }); + + test('kills a running agent and updates manifest', async () => { + const result = await performKill({ + projectRoot: '/project', + agent: 'ag-12345678', + }); + + expect(killAgent).toHaveBeenCalled(); + expect(updateManifest).toHaveBeenCalled(); + expect(result.killed).toEqual(['ag-12345678']); + }); + + test('skips kill for terminal-state agent', async () => { + const goneAgent = makeAgent('ag-12345678', { status: 'gone' }); + const wt = makeWorktree({ agents: { 'ag-12345678': goneAgent } }); + vi.mocked(findAgent).mockReturnValue({ worktree: wt, agent: goneAgent }); + + const result = await performKill({ + projectRoot: '/project', + agent: 'ag-12345678', + }); + + expect(killAgent).not.toHaveBeenCalled(); + expect(result.killed).toEqual([]); + expect(result.message).toContain('already gone'); + }); + + test('throws AgentNotFoundError for unknown agent', async () => { + vi.mocked(findAgent).mockReturnValue(undefined); + + const err = await performKill({ projectRoot: '/project', agent: 'ag-unknown' }).catch((e) => e); + expect(err).toBeInstanceOf(PpgError); + expect((err as PpgError).code).toBe('AGENT_NOT_FOUND'); + }); + + test('self-protection returns skipped result', async () => { + const agent = makeAgent('ag-12345678'); + vi.mocked(excludeSelf).mockReturnValue({ + safe: [], + skipped: [agent], + }); + + const result = await performKill({ + projectRoot: '/project', + agent: 'ag-12345678', + selfPaneId: '%5', + paneMap: new Map(), + }); + + expect(killAgent).not.toHaveBeenCalled(); + expect(result.killed).toEqual([]); + expect(result.skipped).toEqual(['ag-12345678']); + }); + + test('--delete removes agent from manifest', async () => { + const result = await performKill({ + projectRoot: '/project', + agent: 'ag-12345678', + delete: true, + }); + + expect(killAgent).toHaveBeenCalled(); + expect(killPane).toHaveBeenCalled(); + expect(updateManifest).toHaveBeenCalled(); + expect(result.deleted).toEqual(['ag-12345678']); + }); + + test('--delete on terminal agent skips kill but still deletes', async () => { + const idleAgent = makeAgent('ag-12345678', { status: 'idle' }); + const wt = makeWorktree({ agents: { 'ag-12345678': idleAgent } }); + vi.mocked(findAgent).mockReturnValue({ worktree: wt, agent: idleAgent }); + + const result = await performKill({ + projectRoot: '/project', + agent: 'ag-12345678', + delete: true, + }); + + expect(killAgent).not.toHaveBeenCalled(); + expect(killPane).toHaveBeenCalled(); + expect(result.deleted).toEqual(['ag-12345678']); + expect(result.killed).toEqual([]); + }); + }); + + describe('worktree kill', () => { + let agent1: AgentEntry; + let agent2: AgentEntry; + let wt: WorktreeEntry; + + beforeEach(() => { + agent1 = makeAgent('ag-aaaaaaaa', { tmuxTarget: 'ppg:1.0' }); + agent2 = makeAgent('ag-bbbbbbbb', { tmuxTarget: 'ppg:1.1' }); + wt = makeWorktree({ + agents: { + 'ag-aaaaaaaa': agent1, + 'ag-bbbbbbbb': agent2, + }, + }); + _manifest = makeManifest({ 'wt-abc123': wt }); + vi.mocked(readManifest).mockResolvedValue(_manifest); + vi.mocked(resolveWorktree).mockReturnValue(wt); + }); + + test('kills all running agents in worktree', async () => { + const result = await performKill({ + projectRoot: '/project', + worktree: 'wt-abc123', + }); + + expect(killAgents).toHaveBeenCalledWith([agent1, agent2]); + expect(result.killed).toEqual(['ag-aaaaaaaa', 'ag-bbbbbbbb']); + }); + + test('throws WorktreeNotFoundError for unknown worktree', async () => { + vi.mocked(resolveWorktree).mockReturnValue(undefined); + + const err = await performKill({ projectRoot: '/project', worktree: 'wt-unknown' }).catch((e) => e); + expect(err).toBeInstanceOf(PpgError); + expect((err as PpgError).code).toBe('WORKTREE_NOT_FOUND'); + }); + + test('--remove triggers worktree cleanup', async () => { + await performKill({ + projectRoot: '/project', + worktree: 'wt-abc123', + remove: true, + }); + + expect(cleanupWorktree).toHaveBeenCalled(); + }); + + test('--delete removes worktree from manifest', async () => { + const result = await performKill({ + projectRoot: '/project', + worktree: 'wt-abc123', + delete: true, + }); + + expect(cleanupWorktree).toHaveBeenCalled(); + expect(result.deleted).toEqual(['wt-abc123']); + }); + + test('--delete skips worktree with open PR', async () => { + vi.mocked(checkPrState).mockResolvedValue('OPEN'); + + const result = await performKill({ + projectRoot: '/project', + worktree: 'wt-abc123', + delete: true, + }); + + expect(cleanupWorktree).not.toHaveBeenCalled(); + expect(result.deleted).toEqual([]); + expect(result.skippedOpenPrs).toEqual(['wt-abc123']); + }); + + test('--delete --include-open-prs overrides PR check', async () => { + vi.mocked(checkPrState).mockResolvedValue('OPEN'); + + const result = await performKill({ + projectRoot: '/project', + worktree: 'wt-abc123', + delete: true, + includeOpenPrs: true, + }); + + expect(cleanupWorktree).toHaveBeenCalled(); + expect(result.deleted).toEqual(['wt-abc123']); + }); + + test('filters non-running agents', async () => { + const idleAgent = makeAgent('ag-cccccccc', { status: 'idle' }); + const wtMixed = makeWorktree({ + agents: { + 'ag-aaaaaaaa': agent1, + 'ag-cccccccc': idleAgent, + }, + }); + vi.mocked(resolveWorktree).mockReturnValue(wtMixed); + + const result = await performKill({ + projectRoot: '/project', + worktree: 'wt-abc123', + }); + + expect(result.killed).toEqual(['ag-aaaaaaaa']); + }); + }); + + describe('kill all', () => { + let agent1: AgentEntry; + let agent2: AgentEntry; + let wt1: WorktreeEntry; + let wt2: WorktreeEntry; + + beforeEach(() => { + agent1 = makeAgent('ag-aaaaaaaa'); + agent2 = makeAgent('ag-bbbbbbbb'); + wt1 = makeWorktree({ + id: 'wt-111111', + agents: { 'ag-aaaaaaaa': agent1 }, + }); + wt2 = makeWorktree({ + id: 'wt-222222', + name: 'other-wt', + agents: { 'ag-bbbbbbbb': agent2 }, + }); + _manifest = makeManifest({ 'wt-111111': wt1, 'wt-222222': wt2 }); + vi.mocked(readManifest).mockResolvedValue(_manifest); + // resolveWorktree is called inside removeWorktreeCleanup for --delete/--remove + vi.mocked(resolveWorktree).mockImplementation((_m, ref) => { + if (ref === 'wt-111111') return wt1; + if (ref === 'wt-222222') return wt2; + return undefined; + }); + }); + + test('kills agents across all worktrees', async () => { + const result = await performKill({ + projectRoot: '/project', + all: true, + }); + + expect(killAgents).toHaveBeenCalled(); + expect(result.killed).toHaveLength(2); + expect(result.killed).toContain('ag-aaaaaaaa'); + expect(result.killed).toContain('ag-bbbbbbbb'); + }); + + test('--delete removes all active worktrees', async () => { + const result = await performKill({ + projectRoot: '/project', + all: true, + delete: true, + }); + + expect(cleanupWorktree).toHaveBeenCalledTimes(2); + expect(result.deleted).toHaveLength(2); + }); + + test('self-protection filters agents', async () => { + vi.mocked(excludeSelf).mockReturnValue({ + safe: [agent2], + skipped: [agent1], + }); + + const result = await performKill({ + projectRoot: '/project', + all: true, + selfPaneId: '%5', + paneMap: new Map(), + }); + + expect(result.killed).toEqual(['ag-bbbbbbbb']); + expect(result.skipped).toEqual(['ag-aaaaaaaa']); + }); + }); +}); diff --git a/src/core/operations/kill.ts b/src/core/operations/kill.ts new file mode 100644 index 0000000..18fc5f0 --- /dev/null +++ b/src/core/operations/kill.ts @@ -0,0 +1,257 @@ +import { readManifest, updateManifest, findAgent, resolveWorktree } from '../manifest.js'; +import { killAgent, killAgents } from '../agent.js'; +import { checkPrState } from '../pr.js'; +import { cleanupWorktree } from '../cleanup.js'; +import { excludeSelf } from '../self.js'; +import { killPane, listSessionPanes, type PaneInfo } from '../tmux.js'; +import { PpgError, AgentNotFoundError, WorktreeNotFoundError } from '../../lib/errors.js'; +import type { AgentEntry } from '../../types/manifest.js'; + +export interface KillInput { + projectRoot: string; + agent?: string; + worktree?: string; + all?: boolean; + remove?: boolean; + delete?: boolean; + includeOpenPrs?: boolean; + selfPaneId?: string | null; + paneMap?: Map; +} + +export interface KillResult { + killed: string[]; + skipped?: string[]; + removed?: string[]; + deleted?: string[]; + skippedOpenPrs?: string[]; + message?: string; +} + +export async function performKill(input: KillInput): Promise { + const { projectRoot } = input; + + if (!input.agent && !input.worktree && !input.all) { + throw new PpgError('One of --agent, --worktree, or --all is required', 'INVALID_ARGS'); + } + + if (input.agent) { + return killSingleAgent(projectRoot, input.agent, input); + } else if (input.worktree) { + return killWorktreeAgents(projectRoot, input.worktree, input); + } else { + return killAllAgents(projectRoot, input); + } +} + +async function killSingleAgent( + projectRoot: string, + agentId: string, + input: KillInput, +): Promise { + const manifest = await readManifest(projectRoot); + const found = findAgent(manifest, agentId); + if (!found) throw new AgentNotFoundError(agentId); + + const { agent } = found; + const isTerminal = agent.status !== 'running'; + + // Self-protection check + if (input.selfPaneId && input.paneMap) { + const { skipped } = excludeSelf([agent], input.selfPaneId, input.paneMap); + if (skipped.length > 0) { + return { killed: [], skipped: [agentId], message: 'self-protection' }; + } + } + + if (input.delete) { + if (!isTerminal) { + await killAgent(agent); + } + await killPane(agent.tmuxTarget); + + await updateManifest(projectRoot, (m) => { + const f = findAgent(m, agentId); + if (f) { + delete f.worktree.agents[agentId]; + } + return m; + }); + + return { killed: isTerminal ? [] : [agentId], deleted: [agentId] }; + } + + if (isTerminal) { + return { killed: [], message: `Agent ${agentId} already ${agent.status}` }; + } + + await killAgent(agent); + + await updateManifest(projectRoot, (m) => { + const f = findAgent(m, agentId); + if (f) { + f.agent.status = 'gone'; + } + return m; + }); + + return { killed: [agentId] }; +} + +async function killWorktreeAgents( + projectRoot: string, + worktreeRef: string, + input: KillInput, +): Promise { + const manifest = await readManifest(projectRoot); + const wt = resolveWorktree(manifest, worktreeRef); + if (!wt) throw new WorktreeNotFoundError(worktreeRef); + + let toKill = Object.values(wt.agents).filter((a) => a.status === 'running'); + + const skippedIds: string[] = []; + if (input.selfPaneId && input.paneMap) { + const { safe, skipped } = excludeSelf(toKill, input.selfPaneId, input.paneMap); + toKill = safe; + for (const a of skipped) skippedIds.push(a.id); + } + + const killedIds = toKill.map((a) => a.id); + await killAgents(toKill); + + await updateManifest(projectRoot, (m) => { + const mWt = m.worktrees[wt.id]; + if (mWt) { + for (const agent of Object.values(mWt.agents)) { + if (killedIds.includes(agent.id)) { + agent.status = 'gone'; + } + } + } + return m; + }); + + // Check for open PR before deleting worktree + let skippedOpenPr = false; + if (input.delete && !input.includeOpenPrs) { + const prState = await checkPrState(wt.branch); + if (prState === 'OPEN') { + skippedOpenPr = true; + } + } + + const shouldRemove = (input.remove || input.delete) && !skippedOpenPr; + if (shouldRemove) { + await removeWorktreeCleanup(projectRoot, wt.id, input.selfPaneId ?? null, input.paneMap); + } + + if (input.delete && !skippedOpenPr) { + await updateManifest(projectRoot, (m) => { + delete m.worktrees[wt.id]; + return m; + }); + } + + return { + killed: killedIds, + skipped: skippedIds.length > 0 ? skippedIds : undefined, + removed: shouldRemove ? [wt.id] : [], + deleted: (input.delete && !skippedOpenPr) ? [wt.id] : [], + skippedOpenPrs: skippedOpenPr ? [wt.id] : undefined, + }; +} + +async function killAllAgents( + projectRoot: string, + input: KillInput, +): Promise { + const manifest = await readManifest(projectRoot); + let toKill: AgentEntry[] = []; + + for (const wt of Object.values(manifest.worktrees)) { + for (const agent of Object.values(wt.agents)) { + if (agent.status === 'running') { + toKill.push(agent); + } + } + } + + const skippedIds: string[] = []; + if (input.selfPaneId && input.paneMap) { + const { safe, skipped } = excludeSelf(toKill, input.selfPaneId, input.paneMap); + toKill = safe; + for (const a of skipped) skippedIds.push(a.id); + } + + const killedIds = toKill.map((a) => a.id); + await killAgents(toKill); + + const activeWorktreeIds = Object.values(manifest.worktrees) + .filter((wt) => wt.status === 'active') + .map((wt) => wt.id); + + await updateManifest(projectRoot, (m) => { + for (const wt of Object.values(m.worktrees)) { + for (const agent of Object.values(wt.agents)) { + if (killedIds.includes(agent.id)) { + agent.status = 'gone'; + } + } + } + return m; + }); + + // Filter out worktrees with open PRs + let worktreesToRemove = activeWorktreeIds; + const openPrWorktreeIds: string[] = []; + if (input.delete && !input.includeOpenPrs) { + worktreesToRemove = []; + for (const wtId of activeWorktreeIds) { + const wt = manifest.worktrees[wtId]; + if (wt) { + const prState = await checkPrState(wt.branch); + if (prState === 'OPEN') { + openPrWorktreeIds.push(wtId); + } else { + worktreesToRemove.push(wtId); + } + } + } + } + + const shouldRemove = input.remove || input.delete; + if (shouldRemove) { + for (const wtId of worktreesToRemove) { + await removeWorktreeCleanup(projectRoot, wtId, input.selfPaneId ?? null, input.paneMap); + } + } + + if (input.delete) { + await updateManifest(projectRoot, (m) => { + for (const wtId of worktreesToRemove) { + delete m.worktrees[wtId]; + } + return m; + }); + } + + return { + killed: killedIds, + skipped: skippedIds.length > 0 ? skippedIds : undefined, + removed: shouldRemove ? worktreesToRemove : [], + deleted: input.delete ? worktreesToRemove : [], + skippedOpenPrs: openPrWorktreeIds.length > 0 ? openPrWorktreeIds : undefined, + }; +} + +async function removeWorktreeCleanup( + projectRoot: string, + wtId: string, + selfPaneId: string | null, + paneMap?: Map, +): Promise { + const manifest = await readManifest(projectRoot); + const wt = resolveWorktree(manifest, wtId); + if (!wt) return; + await cleanupWorktree(projectRoot, wt, { selfPaneId, paneMap }); +} From 9349e98f22fab86683b0209eb21acc251c244704 Mon Sep 17 00:00:00 2001 From: 2witstudios <2witstudios@gmail.com> Date: Fri, 27 Feb 2026 07:58:42 -0600 Subject: [PATCH 2/3] fix: address code review findings for kill operation extraction - P1: Remove unused listSessionPanes import - P2: Add success field to KillResult; return success:false for self-protection (restores original JSON contract) - P2: Restore info("Killing agent X") progress messages in command layer - P2: Replace fragile vi.restoreAllMocks with vi.clearAllMocks + explicit mock defaults in beforeEach - P3: Add worktreeCount to KillResult for kill-all message context - P3: Fix 0-killed message when all agents skipped in worktree kill - P3: Add manifest mutation verification tests (status=gone, delete) - P3: Add --remove on --all test and error propagation test - P3: Fix --all open PR skip warning in command output --- src/commands/kill.ts | 24 +++++-- src/core/operations/kill.test.ts | 115 +++++++++++++++++++++++++------ src/core/operations/kill.ts | 15 ++-- 3 files changed, 124 insertions(+), 30 deletions(-) diff --git a/src/commands/kill.ts b/src/commands/kill.ts index 3d186a9..b703d5f 100644 --- a/src/commands/kill.ts +++ b/src/commands/kill.ts @@ -43,12 +43,13 @@ export async function killCommand(options: KillOptions): Promise { function formatOutput(result: KillResult, options: KillOptions): void { if (options.json) { - output({ success: true, ...result }, true); + output(result, true); return; } - if (result.message) { - info(result.message); + // Emit per-agent progress for killed agents + for (const id of result.killed) { + info(`Killing agent ${id}`); } if (result.skipped?.length) { @@ -68,19 +69,32 @@ function formatOutput(result: KillResult, options: KillOptions): void { success(`Deleted agent ${options.agent}`); } else if (result.killed.length > 0) { success(`Killed agent ${options.agent}`); + } else if (result.message) { + info(result.message); } } else if (options.worktree) { - success(`Killed ${result.killed.length} agent(s) in worktree ${options.worktree}`); + if (result.killed.length > 0 || !result.skipped?.length) { + success(`Killed ${result.killed.length} agent(s) in worktree ${options.worktree}`); + } + if (result.skipped?.length) { + warn(`Skipped ${result.skipped.length} agent(s) due to self-protection`); + } if (result.deleted?.length) { success(`Deleted worktree ${options.worktree}`); } else if (result.removed?.length) { success(`Removed worktree ${options.worktree}`); } } else if (options.all) { - success(`Killed ${result.killed.length} agent(s)`); + const wtMsg = result.worktreeCount !== undefined + ? ` across ${result.worktreeCount} worktree(s)` + : ''; + success(`Killed ${result.killed.length} agent(s)${wtMsg}`); if (result.skipped?.length) { warn(`Skipped ${result.skipped.length} agent(s) due to self-protection`); } + if (result.skippedOpenPrs?.length) { + warn(`Skipped deletion of ${result.skippedOpenPrs.length} worktree(s) with open PRs`); + } if (result.deleted?.length) { success(`Deleted ${result.deleted.length} worktree(s)`); } else if (result.removed?.length) { diff --git a/src/core/operations/kill.test.ts b/src/core/operations/kill.test.ts index 2f97654..d09fb64 100644 --- a/src/core/operations/kill.test.ts +++ b/src/core/operations/kill.test.ts @@ -6,9 +6,7 @@ import type { PaneInfo } from '../tmux.js'; vi.mock('../manifest.js', () => ({ readManifest: vi.fn(), - updateManifest: vi.fn(async (_root: string, updater: (m: any) => any) => { - return updater(currentManifest()); - }), + updateManifest: vi.fn(), findAgent: vi.fn(), resolveWorktree: vi.fn(), })); @@ -35,12 +33,11 @@ vi.mock('../cleanup.js', () => ({ })); vi.mock('../self.js', () => ({ - excludeSelf: vi.fn((agents: AgentEntry[]) => ({ safe: agents, skipped: [] })), + excludeSelf: vi.fn(), })); vi.mock('../tmux.js', () => ({ killPane: vi.fn(async () => {}), - listSessionPanes: vi.fn(async () => new Map()), })); import { performKill } from './kill.js'; @@ -103,29 +100,17 @@ function currentManifest(): Manifest { describe('performKill', () => { beforeEach(() => { - vi.restoreAllMocks(); + vi.clearAllMocks(); _manifest = makeManifest(); - // Re-establish default mock implementations after restore + // Establish defaults for all mocks after clearing vi.mocked(readManifest).mockResolvedValue(_manifest); vi.mocked(updateManifest).mockImplementation(async (_root: string, updater: (m: any) => any) => { return updater(currentManifest()); }); vi.mocked(findAgent).mockReturnValue(undefined); vi.mocked(resolveWorktree).mockReturnValue(undefined); - vi.mocked(killAgent).mockResolvedValue(undefined); - vi.mocked(killAgents).mockResolvedValue(undefined); vi.mocked(checkPrState).mockResolvedValue('UNKNOWN'); - vi.mocked(cleanupWorktree).mockResolvedValue({ - worktreeId: 'wt-abc123', - manifestUpdated: true, - tmuxKilled: 1, - tmuxSkipped: 0, - tmuxFailed: 0, - selfProtected: false, - selfProtectedTargets: [], - }); vi.mocked(excludeSelf).mockImplementation((agents: AgentEntry[]) => ({ safe: agents, skipped: [] })); - vi.mocked(killPane).mockResolvedValue(undefined); }); test('throws INVALID_ARGS when no target specified', async () => { @@ -151,9 +136,36 @@ describe('performKill', () => { expect(killAgent).toHaveBeenCalled(); expect(updateManifest).toHaveBeenCalled(); + expect(result.success).toBe(true); expect(result.killed).toEqual(['ag-12345678']); }); + test('manifest updater sets agent status to gone', async () => { + await performKill({ + projectRoot: '/project', + agent: 'ag-12345678', + }); + + // Verify the updater was called and inspect what it does + const updaterCall = vi.mocked(updateManifest).mock.calls[0]; + expect(updaterCall[0]).toBe('/project'); + const updater = updaterCall[1]; + + // Run the updater against a test manifest to verify the mutation + const testManifest = makeManifest({ + 'wt-abc123': makeWorktree({ + agents: { 'ag-12345678': makeAgent('ag-12345678') }, + }), + }); + // findAgent mock returns matching agent from test manifest + vi.mocked(findAgent).mockReturnValue({ + worktree: testManifest.worktrees['wt-abc123'], + agent: testManifest.worktrees['wt-abc123'].agents['ag-12345678'], + }); + const result = updater(testManifest) as Manifest; + expect(result.worktrees['wt-abc123'].agents['ag-12345678'].status).toBe('gone'); + }); + test('skips kill for terminal-state agent', async () => { const goneAgent = makeAgent('ag-12345678', { status: 'gone' }); const wt = makeWorktree({ agents: { 'ag-12345678': goneAgent } }); @@ -165,6 +177,7 @@ describe('performKill', () => { }); expect(killAgent).not.toHaveBeenCalled(); + expect(result.success).toBe(true); expect(result.killed).toEqual([]); expect(result.message).toContain('already gone'); }); @@ -177,7 +190,7 @@ describe('performKill', () => { expect((err as PpgError).code).toBe('AGENT_NOT_FOUND'); }); - test('self-protection returns skipped result', async () => { + test('self-protection returns success:false with skipped', async () => { const agent = makeAgent('ag-12345678'); vi.mocked(excludeSelf).mockReturnValue({ safe: [], @@ -192,6 +205,7 @@ describe('performKill', () => { }); expect(killAgent).not.toHaveBeenCalled(); + expect(result.success).toBe(false); expect(result.killed).toEqual([]); expect(result.skipped).toEqual(['ag-12345678']); }); @@ -206,9 +220,33 @@ describe('performKill', () => { expect(killAgent).toHaveBeenCalled(); expect(killPane).toHaveBeenCalled(); expect(updateManifest).toHaveBeenCalled(); + expect(result.success).toBe(true); expect(result.deleted).toEqual(['ag-12345678']); }); + test('--delete manifest updater removes agent entry', async () => { + await performKill({ + projectRoot: '/project', + agent: 'ag-12345678', + delete: true, + }); + + const updaterCall = vi.mocked(updateManifest).mock.calls[0]; + const updater = updaterCall[1]; + + const testManifest = makeManifest({ + 'wt-abc123': makeWorktree({ + agents: { 'ag-12345678': makeAgent('ag-12345678') }, + }), + }); + vi.mocked(findAgent).mockReturnValue({ + worktree: testManifest.worktrees['wt-abc123'], + agent: testManifest.worktrees['wt-abc123'].agents['ag-12345678'], + }); + const result = updater(testManifest) as Manifest; + expect(result.worktrees['wt-abc123'].agents['ag-12345678']).toBeUndefined(); + }); + test('--delete on terminal agent skips kill but still deletes', async () => { const idleAgent = makeAgent('ag-12345678', { status: 'idle' }); const wt = makeWorktree({ agents: { 'ag-12345678': idleAgent } }); @@ -222,9 +260,22 @@ describe('performKill', () => { expect(killAgent).not.toHaveBeenCalled(); expect(killPane).toHaveBeenCalled(); + expect(result.success).toBe(true); expect(result.deleted).toEqual(['ag-12345678']); expect(result.killed).toEqual([]); }); + + test('propagates killAgent errors', async () => { + vi.mocked(killAgent).mockRejectedValue(new Error('tmux crash')); + + const err = await performKill({ + projectRoot: '/project', + agent: 'ag-12345678', + }).catch((e) => e); + + expect(err).toBeInstanceOf(Error); + expect(err.message).toBe('tmux crash'); + }); }); describe('worktree kill', () => { @@ -253,6 +304,7 @@ describe('performKill', () => { }); expect(killAgents).toHaveBeenCalledWith([agent1, agent2]); + expect(result.success).toBe(true); expect(result.killed).toEqual(['ag-aaaaaaaa', 'ag-bbbbbbbb']); }); @@ -367,11 +419,21 @@ describe('performKill', () => { }); expect(killAgents).toHaveBeenCalled(); + expect(result.success).toBe(true); expect(result.killed).toHaveLength(2); expect(result.killed).toContain('ag-aaaaaaaa'); expect(result.killed).toContain('ag-bbbbbbbb'); }); + test('includes worktreeCount in result', async () => { + const result = await performKill({ + projectRoot: '/project', + all: true, + }); + + expect(result.worktreeCount).toBe(2); + }); + test('--delete removes all active worktrees', async () => { const result = await performKill({ projectRoot: '/project', @@ -383,6 +445,19 @@ describe('performKill', () => { expect(result.deleted).toHaveLength(2); }); + test('--remove triggers cleanup without manifest deletion', async () => { + const result = await performKill({ + projectRoot: '/project', + all: true, + remove: true, + }); + + expect(cleanupWorktree).toHaveBeenCalledTimes(2); + expect(result.removed).toHaveLength(2); + // --remove without --delete: cleanup happens but entries stay in manifest + expect(result.deleted).toEqual([]); + }); + test('self-protection filters agents', async () => { vi.mocked(excludeSelf).mockReturnValue({ safe: [agent2], diff --git a/src/core/operations/kill.ts b/src/core/operations/kill.ts index 18fc5f0..4ad9433 100644 --- a/src/core/operations/kill.ts +++ b/src/core/operations/kill.ts @@ -3,7 +3,7 @@ import { killAgent, killAgents } from '../agent.js'; import { checkPrState } from '../pr.js'; import { cleanupWorktree } from '../cleanup.js'; import { excludeSelf } from '../self.js'; -import { killPane, listSessionPanes, type PaneInfo } from '../tmux.js'; +import { killPane, type PaneInfo } from '../tmux.js'; import { PpgError, AgentNotFoundError, WorktreeNotFoundError } from '../../lib/errors.js'; import type { AgentEntry } from '../../types/manifest.js'; @@ -20,11 +20,13 @@ export interface KillInput { } export interface KillResult { + success: boolean; killed: string[]; skipped?: string[]; removed?: string[]; deleted?: string[]; skippedOpenPrs?: string[]; + worktreeCount?: number; message?: string; } @@ -60,7 +62,7 @@ async function killSingleAgent( if (input.selfPaneId && input.paneMap) { const { skipped } = excludeSelf([agent], input.selfPaneId, input.paneMap); if (skipped.length > 0) { - return { killed: [], skipped: [agentId], message: 'self-protection' }; + return { success: false, killed: [], skipped: [agentId], message: 'self-protection' }; } } @@ -78,11 +80,11 @@ async function killSingleAgent( return m; }); - return { killed: isTerminal ? [] : [agentId], deleted: [agentId] }; + return { success: true, killed: isTerminal ? [] : [agentId], deleted: [agentId] }; } if (isTerminal) { - return { killed: [], message: `Agent ${agentId} already ${agent.status}` }; + return { success: true, killed: [], message: `Agent ${agentId} already ${agent.status}` }; } await killAgent(agent); @@ -95,7 +97,7 @@ async function killSingleAgent( return m; }); - return { killed: [agentId] }; + return { success: true, killed: [agentId] }; } async function killWorktreeAgents( @@ -153,6 +155,7 @@ async function killWorktreeAgents( } return { + success: true, killed: killedIds, skipped: skippedIds.length > 0 ? skippedIds : undefined, removed: shouldRemove ? [wt.id] : [], @@ -236,11 +239,13 @@ async function killAllAgents( } return { + success: true, killed: killedIds, skipped: skippedIds.length > 0 ? skippedIds : undefined, removed: shouldRemove ? worktreesToRemove : [], deleted: input.delete ? worktreesToRemove : [], skippedOpenPrs: openPrWorktreeIds.length > 0 ? openPrWorktreeIds : undefined, + worktreeCount: activeWorktreeIds.length, }; } From 76bd8739d6982c7c7dbefee412e38579e4bfa694 Mon Sep 17 00:00:00 2001 From: 2witstudios <2witstudios@gmail.com> Date: Fri, 27 Feb 2026 08:28:59 -0600 Subject: [PATCH 3/3] test: fix spawn manifest mock type inference --- src/commands/spawn.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/commands/spawn.test.ts b/src/commands/spawn.test.ts index ee642c7..4e38c02 100644 --- a/src/commands/spawn.test.ts +++ b/src/commands/spawn.test.ts @@ -7,6 +7,7 @@ import { spawnAgent } from '../core/agent.js'; import { getRepoRoot } from '../core/worktree.js'; import { agentId, sessionId } from '../lib/id.js'; import * as tmux from '../core/tmux.js'; +import type { Manifest } from '../types/manifest.js'; vi.mock('node:fs/promises', async () => { const actual = await vi.importActual('node:fs/promises'); @@ -103,7 +104,7 @@ function createManifest(tmuxWindow = '') { } describe('spawnCommand', () => { - let manifestState = createManifest(); + let manifestState: Manifest = createManifest(); let nextAgent = 1; let nextSession = 1;