From f12bb90924b36ccb57d53947cbaef1a7ffd527e1 Mon Sep 17 00:00:00 2001 From: Affaan Mustafa Date: Mon, 16 Mar 2026 14:29:28 -0700 Subject: [PATCH] fix: refresh orchestration follow-up after #414 (#430) --- commands/orchestrate.md | 4 + commands/sessions.md | 14 +- scripts/lib/tmux-worktree-orchestrator.js | 200 +++++++++++++----- tests/lib/tmux-worktree-orchestrator.test.js | 190 +++++++++++++++++ .../scripts/orchestrate-codex-worker.test.js | 63 ++++++ 5 files changed, 416 insertions(+), 55 deletions(-) create mode 100644 tests/scripts/orchestrate-codex-worker.test.js diff --git a/commands/orchestrate.md b/commands/orchestrate.md index 2db80d66..3b36da93 100644 --- a/commands/orchestrate.md +++ b/commands/orchestrate.md @@ -1,3 +1,7 @@ +--- +description: Sequential and tmux/worktree orchestration guidance for multi-agent workflows. +--- + # Orchestrate Command Sequential agent workflow for complex tasks. diff --git a/commands/sessions.md b/commands/sessions.md index 3acfd418..4713b82f 100644 --- a/commands/sessions.md +++ b/commands/sessions.md @@ -1,3 +1,7 @@ +--- +description: Manage Claude Code session history, aliases, and session metadata. +--- + # Sessions Command Manage Claude Code session history - list, load, alias, and edit sessions stored in `~/.claude/sessions/`. @@ -255,11 +259,6 @@ Show all session aliases. /sessions aliases # List all aliases ``` -## Operator Notes - -- Session files persist `Project`, `Branch`, and `Worktree` in the header so `/sessions info` can disambiguate parallel tmux/worktree runs. -- For command-center style monitoring, combine `/sessions info`, `git diff --stat`, and the cost metrics emitted by `scripts/hooks/cost-tracker.js`. - **Script:** ```bash node -e " @@ -284,6 +283,11 @@ if (aliases.length === 0) { " ``` +## Operator Notes + +- Session files persist `Project`, `Branch`, and `Worktree` in the header so `/sessions info` can disambiguate parallel tmux/worktree runs. +- For command-center style monitoring, combine `/sessions info`, `git diff --stat`, and the cost metrics emitted by `scripts/hooks/cost-tracker.js`. + ## Arguments $ARGUMENTS: diff --git a/scripts/lib/tmux-worktree-orchestrator.js b/scripts/lib/tmux-worktree-orchestrator.js index 4cf8bb4d..8eb947c8 100644 --- a/scripts/lib/tmux-worktree-orchestrator.js +++ b/scripts/lib/tmux-worktree-orchestrator.js @@ -34,6 +34,22 @@ function formatCommand(program, args) { return [program, ...args.map(shellQuote)].join(' '); } +function buildTemplateVariables(values) { + return Object.entries(values).reduce((accumulator, [key, value]) => { + const stringValue = String(value); + const quotedValue = shellQuote(stringValue); + + accumulator[key] = stringValue; + accumulator[`${key}_raw`] = stringValue; + accumulator[`${key}_sh`] = quotedValue; + return accumulator; + }, {}); +} + +function buildSessionBannerCommand(sessionName, coordinationDir) { + return `printf '%s\\n' ${shellQuote(`Session: ${sessionName}`)} ${shellQuote(`Coordination: ${coordinationDir}`)}`; +} + function normalizeSeedPaths(seedPaths, repoRoot) { const resolvedRepoRoot = path.resolve(repoRoot); const entries = Array.isArray(seedPaths) ? seedPaths : []; @@ -239,7 +255,7 @@ function buildOrchestrationPlan(config = {}) { 'send-keys', '-t', sessionName, - `printf '%s\\n' 'Session: ${sessionName}' 'Coordination: ${coordinationDir}'`, + buildSessionBannerCommand(sessionName, coordinationDir), 'C-m' ], description: 'Print orchestrator session details' @@ -400,14 +416,82 @@ function cleanupExisting(plan) { } } -function executePlan(plan) { - runCommand('git', ['rev-parse', '--is-inside-work-tree'], { cwd: plan.repoRoot }); - runCommand('tmux', ['-V']); +function rollbackCreatedResources(plan, createdState, runtime = {}) { + const runCommandImpl = runtime.runCommand || runCommand; + const listWorktreesImpl = runtime.listWorktrees || listWorktrees; + const branchExistsImpl = runtime.branchExists || branchExists; + const errors = []; + + if (createdState.sessionCreated) { + try { + runCommandImpl('tmux', ['kill-session', '-t', plan.sessionName], { cwd: plan.repoRoot }); + } catch (error) { + errors.push(error.message); + } + } + + for (const workerPlan of [...createdState.workerPlans].reverse()) { + const expectedWorktreePath = canonicalizePath(workerPlan.worktreePath); + const existingWorktree = listWorktreesImpl(plan.repoRoot).find( + worktree => worktree.canonicalPath === expectedWorktreePath + ); + + if (existingWorktree) { + try { + runCommandImpl('git', ['worktree', 'remove', '--force', existingWorktree.listedPath], { + cwd: plan.repoRoot + }); + } catch (error) { + errors.push(error.message); + } + } else if (fs.existsSync(workerPlan.worktreePath)) { + fs.rmSync(workerPlan.worktreePath, { force: true, recursive: true }); + } + + try { + runCommandImpl('git', ['worktree', 'prune', '--expire', 'now'], { cwd: plan.repoRoot }); + } catch (error) { + errors.push(error.message); + } + + if (branchExistsImpl(plan.repoRoot, workerPlan.branchName)) { + try { + runCommandImpl('git', ['branch', '-D', workerPlan.branchName], { cwd: plan.repoRoot }); + } catch (error) { + errors.push(error.message); + } + } + } + + if (createdState.removeCoordinationDir && fs.existsSync(plan.coordinationDir)) { + fs.rmSync(plan.coordinationDir, { force: true, recursive: true }); + } + + if (errors.length > 0) { + throw new Error(`rollback failed: ${errors.join('; ')}`); + } +} + +function executePlan(plan, runtime = {}) { + const spawnSyncImpl = runtime.spawnSync || spawnSync; + const runCommandImpl = runtime.runCommand || runCommand; + const materializePlanImpl = runtime.materializePlan || materializePlan; + const overlaySeedPathsImpl = runtime.overlaySeedPaths || overlaySeedPaths; + const cleanupExistingImpl = runtime.cleanupExisting || cleanupExisting; + const rollbackCreatedResourcesImpl = runtime.rollbackCreatedResources || rollbackCreatedResources; + const createdState = { + workerPlans: [], + sessionCreated: false, + removeCoordinationDir: !fs.existsSync(plan.coordinationDir) + }; + + runCommandImpl('git', ['rev-parse', '--is-inside-work-tree'], { cwd: plan.repoRoot }); + runCommandImpl('tmux', ['-V']); if (plan.replaceExisting) { - cleanupExisting(plan); + cleanupExistingImpl(plan); } else { - const hasSession = spawnSync('tmux', ['has-session', '-t', plan.sessionName], { + const hasSession = spawnSyncImpl('tmux', ['has-session', '-t', plan.sessionName], { encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }); @@ -416,61 +500,76 @@ function executePlan(plan) { } } - materializePlan(plan); + try { + materializePlanImpl(plan); - for (const workerPlan of plan.workerPlans) { - runCommand('git', workerPlan.gitArgs, { cwd: plan.repoRoot }); - overlaySeedPaths({ - repoRoot: plan.repoRoot, - seedPaths: workerPlan.seedPaths, - worktreePath: workerPlan.worktreePath - }); - } - - runCommand( - 'tmux', - ['new-session', '-d', '-s', plan.sessionName, '-n', 'orchestrator', '-c', plan.repoRoot], - { cwd: plan.repoRoot } - ); - runCommand( - 'tmux', - [ - 'send-keys', - '-t', - plan.sessionName, - `printf '%s\\n' 'Session: ${plan.sessionName}' 'Coordination: ${plan.coordinationDir}'`, - 'C-m' - ], - { cwd: plan.repoRoot } - ); - - for (const workerPlan of plan.workerPlans) { - const splitResult = runCommand( - 'tmux', - ['split-window', '-d', '-P', '-F', '#{pane_id}', '-t', plan.sessionName, '-c', workerPlan.worktreePath], - { cwd: plan.repoRoot } - ); - const paneId = splitResult.stdout.trim(); - - if (!paneId) { - throw new Error(`tmux split-window did not return a pane id for ${workerPlan.workerName}`); + for (const workerPlan of plan.workerPlans) { + runCommandImpl('git', workerPlan.gitArgs, { cwd: plan.repoRoot }); + createdState.workerPlans.push(workerPlan); + overlaySeedPathsImpl({ + repoRoot: plan.repoRoot, + seedPaths: workerPlan.seedPaths, + worktreePath: workerPlan.worktreePath + }); } - runCommand('tmux', ['select-layout', '-t', plan.sessionName, 'tiled'], { cwd: plan.repoRoot }); - runCommand('tmux', ['select-pane', '-t', paneId, '-T', workerPlan.workerSlug], { - cwd: plan.repoRoot - }); - runCommand( + runCommandImpl( + 'tmux', + ['new-session', '-d', '-s', plan.sessionName, '-n', 'orchestrator', '-c', plan.repoRoot], + { cwd: plan.repoRoot } + ); + createdState.sessionCreated = true; + runCommandImpl( 'tmux', [ 'send-keys', '-t', - paneId, - `cd ${shellQuote(workerPlan.worktreePath)} && ${workerPlan.launchCommand}`, + plan.sessionName, + buildSessionBannerCommand(plan.sessionName, plan.coordinationDir), 'C-m' ], { cwd: plan.repoRoot } ); + + for (const workerPlan of plan.workerPlans) { + const splitResult = runCommandImpl( + 'tmux', + ['split-window', '-d', '-P', '-F', '#{pane_id}', '-t', plan.sessionName, '-c', workerPlan.worktreePath], + { cwd: plan.repoRoot } + ); + const paneId = splitResult.stdout.trim(); + + if (!paneId) { + throw new Error(`tmux split-window did not return a pane id for ${workerPlan.workerName}`); + } + + runCommandImpl('tmux', ['select-layout', '-t', plan.sessionName, 'tiled'], { cwd: plan.repoRoot }); + runCommandImpl('tmux', ['select-pane', '-t', paneId, '-T', workerPlan.workerSlug], { + cwd: plan.repoRoot + }); + runCommandImpl( + 'tmux', + [ + 'send-keys', + '-t', + paneId, + `cd ${shellQuote(workerPlan.worktreePath)} && ${workerPlan.launchCommand}`, + 'C-m' + ], + { cwd: plan.repoRoot } + ); + } + } catch (error) { + try { + rollbackCreatedResourcesImpl(plan, createdState, { + branchExists: runtime.branchExists, + listWorktrees: runtime.listWorktrees, + runCommand: runCommandImpl + }); + } catch (cleanupError) { + error.message = `${error.message}; cleanup failed: ${cleanupError.message}`; + } + throw error; } return { @@ -486,6 +585,7 @@ module.exports = { materializePlan, normalizeSeedPaths, overlaySeedPaths, + rollbackCreatedResources, renderTemplate, slugify }; diff --git a/tests/lib/tmux-worktree-orchestrator.test.js b/tests/lib/tmux-worktree-orchestrator.test.js index fcbe1b71..d93d2a63 100644 --- a/tests/lib/tmux-worktree-orchestrator.test.js +++ b/tests/lib/tmux-worktree-orchestrator.test.js @@ -9,6 +9,7 @@ const { slugify, renderTemplate, buildOrchestrationPlan, + executePlan, materializePlan, normalizeSeedPaths, overlaySeedPaths @@ -137,6 +138,64 @@ test('buildOrchestrationPlan normalizes global and worker seed paths', () => { ]); }); +test('buildOrchestrationPlan rejects worker names that collapse to the same slug', () => { + assert.throws( + () => buildOrchestrationPlan({ + repoRoot: '/tmp/ecc', + sessionName: 'duplicates', + launcherCommand: 'echo run', + workers: [ + { name: 'Docs A', task: 'Fix skill docs' }, + { name: 'Docs/A', task: 'Fix tests' } + ] + }), + /unique slugs/ + ); +}); + +test('buildOrchestrationPlan exposes shell-safe launcher aliases alongside raw defaults', () => { + const repoRoot = path.join('/tmp', 'My Repo'); + const plan = buildOrchestrationPlan({ + repoRoot, + sessionName: 'Spacing Audit', + launcherCommand: 'bash {repo_root_sh}/scripts/orchestrate-codex-worker.sh {task_file_sh} {handoff_file_sh} {status_file_sh} {worker_name_sh} {worker_name}', + workers: [{ name: 'Docs Fixer', task: 'Update docs' }] + }); + const quote = value => `'${String(value).replace(/'/g, `'\\''`)}'`; + const resolvedRepoRoot = plan.workerPlans[0].repoRoot; + + assert.ok( + plan.workerPlans[0].launchCommand.includes(`bash ${quote(resolvedRepoRoot)}/scripts/orchestrate-codex-worker.sh`), + 'repo_root_sh should provide a shell-safe path' + ); + assert.ok( + plan.workerPlans[0].launchCommand.includes(quote(plan.workerPlans[0].taskFilePath)), + 'task_file_sh should provide a shell-safe path' + ); + assert.ok( + plan.workerPlans[0].launchCommand.includes(`${quote(plan.workerPlans[0].workerName)} ${plan.workerPlans[0].workerName}`), + 'raw defaults should remain available alongside shell-safe aliases' + ); +}); + +test('buildOrchestrationPlan shell-quotes the orchestration banner command', () => { + const repoRoot = path.join('/tmp', "O'Hare Repo"); + const plan = buildOrchestrationPlan({ + repoRoot, + sessionName: 'Quote Audit', + launcherCommand: 'echo run', + workers: [{ name: 'Docs', task: 'Update docs' }] + }); + const quote = value => `'${String(value).replace(/'/g, `'\\''`)}'`; + const bannerCommand = plan.tmuxCommands[1].args[3]; + + assert.strictEqual( + bannerCommand, + `printf '%s\\n' ${quote(`Session: ${plan.sessionName}`)} ${quote(`Coordination: ${plan.coordinationDir}`)}`, + 'Banner command should quote coordination paths safely for tmux send-keys' + ); +}); + test('normalizeSeedPaths rejects paths outside the repo root', () => { assert.throws( () => normalizeSeedPaths(['../outside.txt'], '/tmp/ecc'), @@ -229,5 +288,136 @@ test('overlaySeedPaths copies local overlays into the worker worktree', () => { } }); +test('executePlan rolls back partial setup when orchestration fails mid-run', () => { + const plan = { + repoRoot: '/tmp/ecc', + sessionName: 'rollback-test', + coordinationDir: '/tmp/ecc/.orchestration/rollback-test', + replaceExisting: false, + workerPlans: [ + { + workerName: 'Docs', + workerSlug: 'docs', + worktreePath: '/tmp/ecc-rollback-docs', + seedPaths: ['commands/orchestrate.md'], + gitArgs: ['worktree', 'add', '-b', 'orchestrator-rollback-test-docs', '/tmp/ecc-rollback-docs', 'HEAD'], + launchCommand: 'echo run' + } + ] + }; + const calls = []; + const rollbackCalls = []; + + assert.throws( + () => executePlan(plan, { + spawnSync(program, args) { + calls.push({ type: 'spawnSync', program, args }); + if (program === 'tmux' && args[0] === 'has-session') { + return { status: 1, stdout: '', stderr: '' }; + } + throw new Error(`Unexpected spawnSync call: ${program} ${args.join(' ')}`); + }, + runCommand(program, args) { + calls.push({ type: 'runCommand', program, args }); + if (program === 'git' && args[0] === 'rev-parse') { + return { status: 0, stdout: 'true\n', stderr: '' }; + } + if (program === 'tmux' && args[0] === '-V') { + return { status: 0, stdout: 'tmux 3.4\n', stderr: '' }; + } + if (program === 'git' && args[0] === 'worktree') { + return { status: 0, stdout: '', stderr: '' }; + } + throw new Error(`Unexpected runCommand call: ${program} ${args.join(' ')}`); + }, + materializePlan(receivedPlan) { + calls.push({ type: 'materializePlan', receivedPlan }); + }, + overlaySeedPaths() { + throw new Error('overlay failed'); + }, + rollbackCreatedResources(receivedPlan, createdState) { + rollbackCalls.push({ receivedPlan, createdState }); + } + }), + /overlay failed/ + ); + + assert.deepStrictEqual( + rollbackCalls.map(call => call.receivedPlan), + [plan], + 'executePlan should invoke rollback on failure' + ); + assert.deepStrictEqual( + rollbackCalls[0].createdState.workerPlans, + plan.workerPlans, + 'executePlan should only roll back resources created before the failure' + ); + assert.ok( + calls.some(call => call.type === 'runCommand' && call.program === 'git' && call.args[0] === 'worktree'), + 'executePlan should attempt setup before rolling back' + ); +}); + +test('executePlan does not mark pre-existing resources for rollback when worktree creation fails', () => { + const plan = { + repoRoot: '/tmp/ecc', + sessionName: 'rollback-existing', + coordinationDir: '/tmp/ecc/.orchestration/rollback-existing', + replaceExisting: false, + workerPlans: [ + { + workerName: 'Docs', + workerSlug: 'docs', + worktreePath: '/tmp/ecc-existing-docs', + seedPaths: [], + gitArgs: ['worktree', 'add', '-b', 'orchestrator-rollback-existing-docs', '/tmp/ecc-existing-docs', 'HEAD'], + launchCommand: 'echo run', + branchName: 'orchestrator-rollback-existing-docs' + } + ] + }; + const rollbackCalls = []; + + assert.throws( + () => executePlan(plan, { + spawnSync(program, args) { + if (program === 'tmux' && args[0] === 'has-session') { + return { status: 1, stdout: '', stderr: '' }; + } + throw new Error(`Unexpected spawnSync call: ${program} ${args.join(' ')}`); + }, + runCommand(program, args) { + if (program === 'git' && args[0] === 'rev-parse') { + return { status: 0, stdout: 'true\n', stderr: '' }; + } + if (program === 'tmux' && args[0] === '-V') { + return { status: 0, stdout: 'tmux 3.4\n', stderr: '' }; + } + if (program === 'git' && args[0] === 'worktree') { + throw new Error('branch already exists'); + } + throw new Error(`Unexpected runCommand call: ${program} ${args.join(' ')}`); + }, + materializePlan() {}, + rollbackCreatedResources(receivedPlan, createdState) { + rollbackCalls.push({ receivedPlan, createdState }); + } + }), + /branch already exists/ + ); + + assert.deepStrictEqual( + rollbackCalls[0].createdState.workerPlans, + [], + 'Failures before creation should not schedule any worker resources for rollback' + ); + assert.strictEqual( + rollbackCalls[0].createdState.sessionCreated, + false, + 'Failures before tmux session creation should not mark a session for rollback' + ); +}); + console.log(`\n=== Results: ${passed} passed, ${failed} failed ===`); if (failed > 0) process.exit(1); diff --git a/tests/scripts/orchestrate-codex-worker.test.js b/tests/scripts/orchestrate-codex-worker.test.js new file mode 100644 index 00000000..9a945622 --- /dev/null +++ b/tests/scripts/orchestrate-codex-worker.test.js @@ -0,0 +1,63 @@ +'use strict'; + +const assert = require('assert'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); +const { spawnSync } = require('child_process'); + +const SCRIPT = path.join(__dirname, '..', '..', 'scripts', 'orchestrate-codex-worker.sh'); + +console.log('=== Testing orchestrate-codex-worker.sh ===\n'); + +let passed = 0; +let failed = 0; + +function test(desc, fn) { + try { + fn(); + console.log(` ✓ ${desc}`); + passed++; + } catch (error) { + console.log(` ✗ ${desc}: ${error.message}`); + failed++; + } +} + +test('fails fast for an unreadable task file and records failure artifacts', () => { + const tempRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-orch-worker-')); + const handoffFile = path.join(tempRoot, '.orchestration', 'docs', 'handoff.md'); + const statusFile = path.join(tempRoot, '.orchestration', 'docs', 'status.md'); + const missingTaskFile = path.join(tempRoot, '.orchestration', 'docs', 'task.md'); + + try { + spawnSync('git', ['init'], { cwd: tempRoot, stdio: 'ignore' }); + + const result = spawnSync('bash', [SCRIPT, missingTaskFile, handoffFile, statusFile], { + cwd: tempRoot, + encoding: 'utf8' + }); + + assert.notStrictEqual(result.status, 0, 'Script should fail when task file is unreadable'); + assert.ok(fs.existsSync(statusFile), 'Script should still write a status file'); + assert.ok(fs.existsSync(handoffFile), 'Script should still write a handoff file'); + + const statusContent = fs.readFileSync(statusFile, 'utf8'); + const handoffContent = fs.readFileSync(handoffFile, 'utf8'); + + assert.ok(statusContent.includes('- State: failed'), 'Status file should record the failure state'); + assert.ok( + statusContent.includes('task file is missing or unreadable'), + 'Status file should explain the task-file failure' + ); + assert.ok( + handoffContent.includes('Task file is missing or unreadable'), + 'Handoff file should explain the task-file failure' + ); + } finally { + fs.rmSync(tempRoot, { recursive: true, force: true }); + } +}); + +console.log(`\n=== Results: ${passed} passed, ${failed} failed ===`); +if (failed > 0) process.exit(1);