fix: address remaining orchestration review comments

tests/lib/orchestration-session.test.js

@@ -109,6 +109,25 @@ test('parseWorkerHandoff also supports bold section headers', () => {
   assert.deepStrictEqual(handoff.remainingRisks, ['No runtime screenshot']);
 });
 
+test('parseWorkerHandoff accepts legacy verification and follow-up headings', () => {
+  const handoff = parseWorkerHandoff([
+    '# Handoff',
+    '',
+    '## Summary',
+    '- Worker completed successfully',
+    '',
+    '## Tests / Verification',
+    '- Ran tests',
+    '',
+    '## Follow-ups',
+    '- Re-run screenshots after deploy'
+  ].join('\n'));
+
+  assert.deepStrictEqual(handoff.summary, ['Worker completed successfully']);
+  assert.deepStrictEqual(handoff.validation, ['Ran tests']);
+  assert.deepStrictEqual(handoff.remainingRisks, ['Re-run screenshots after deploy']);
+});
+
 test('loadWorkerSnapshots reads coordination worker directories', () => {
   const tempRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-orch-session-'));
   const coordinationDir = path.join(tempRoot, 'coordination');
@@ -237,5 +256,52 @@ test('resolveSnapshotTarget normalizes plan session names and defaults to the re
   }
 });
 
+test('resolveSnapshotTarget rejects malformed plan files and invalid config fields', () => {
+  const tempRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-orch-target-'));
+  const repoRoot = path.join(tempRoot, 'repo');
+  fs.mkdirSync(repoRoot, { recursive: true });
+
+  const invalidJsonPath = path.join(repoRoot, 'invalid-json.json');
+  const invalidSessionNamePath = path.join(repoRoot, 'invalid-session.json');
+  const invalidRepoRootPath = path.join(repoRoot, 'invalid-repo-root.json');
+  const invalidCoordinationRootPath = path.join(repoRoot, 'invalid-coordination-root.json');
+
+  fs.writeFileSync(invalidJsonPath, '{not valid json');
+  fs.writeFileSync(invalidSessionNamePath, JSON.stringify({
+    sessionName: '',
+    repoRoot
+  }));
+  fs.writeFileSync(invalidRepoRootPath, JSON.stringify({
+    sessionName: 'workflow',
+    repoRoot: ['not-a-string']
+  }));
+  fs.writeFileSync(invalidCoordinationRootPath, JSON.stringify({
+    sessionName: 'workflow',
+    repoRoot,
+    coordinationRoot: '   '
+  }));
+
+  try {
+    assert.throws(
+      () => resolveSnapshotTarget(invalidJsonPath, repoRoot),
+      /Invalid orchestration plan JSON/
+    );
+    assert.throws(
+      () => resolveSnapshotTarget(invalidSessionNamePath, repoRoot),
+      /sessionName must be a non-empty string/
+    );
+    assert.throws(
+      () => resolveSnapshotTarget(invalidRepoRootPath, repoRoot),
+      /repoRoot must be a non-empty string/
+    );
+    assert.throws(
+      () => resolveSnapshotTarget(invalidCoordinationRootPath, repoRoot),
+      /coordinationRoot must be a non-empty string/
+    );
+  } finally {
+    fs.rmSync(tempRoot, { recursive: true, force: true });
+  }
+});
+
 console.log(`\n=== Results: ${passed} passed, ${failed} failed ===`);
 if (failed > 0) process.exit(1);

tests/lib/tmux-worktree-orchestrator.test.js

@@ -137,6 +137,44 @@ 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 placeholders with raw aliases', () => {
+  const repoRoot = path.join('/tmp', 'My Repo');
+  const plan = buildOrchestrationPlan({
+    repoRoot,
+    sessionName: 'Spacing Audit',
+    launcherCommand: 'bash {repo_root}/scripts/orchestrate-codex-worker.sh {task_file} {handoff_file} {status_file} {worker_name} {worker_name_raw}',
+    workers: [{ name: 'Docs Fixer', task: 'Update docs' }]
+  });
+
+  assert.ok(
+    plan.workerPlans[0].launchCommand.includes(`bash '${repoRoot}'/scripts/orchestrate-codex-worker.sh`),
+    'repo_root should be shell-safe by default'
+  );
+  assert.ok(
+    plan.workerPlans[0].launchCommand.includes(`'${plan.workerPlans[0].taskFilePath}'`),
+    'task_file should be shell-safe by default'
+  );
+  assert.ok(
+    plan.workerPlans[0].launchCommand.includes(`'${plan.workerPlans[0].workerName}' ${plan.workerPlans[0].workerName}`),
+    'worker_name_raw should preserve the unquoted value when explicitly requested'
+  );
+});
+
 test('normalizeSeedPaths rejects paths outside the repo root', () => {
   assert.throws(
     () => normalizeSeedPaths(['../outside.txt'], '/tmp/ecc'),
@@ -170,11 +208,18 @@ test('materializePlan keeps worker instructions inside the worktree boundary', (
     materializePlan(plan);
 
     const taskFile = fs.readFileSync(plan.workerPlans[0].taskFilePath, 'utf8');
+    const handoffFile = fs.readFileSync(plan.workerPlans[0].handoffFilePath, 'utf8');
 
     assert.ok(
       taskFile.includes('Report results in your final response.'),
       'Task file should tell the worker to report in stdout'
     );
+    assert.ok(
+      taskFile.includes('## Summary') &&
+      taskFile.includes('## Validation') &&
+      taskFile.includes('## Remaining Risks'),
+      'Task file should require parser-compatible headings'
+    );
     assert.ok(
       taskFile.includes('Do not spawn subagents or external agents for this task.'),
       'Task file should keep nested workers single-session'
@@ -187,6 +232,18 @@ test('materializePlan keeps worker instructions inside the worktree boundary', (
       !taskFile.includes('Update `'),
       'Task file should not instruct the nested worker to update orchestration status files'
     );
+    assert.ok(
+      handoffFile.includes('## Summary') &&
+      handoffFile.includes('## Validation') &&
+      handoffFile.includes('## Remaining Risks'),
+      'Handoff placeholder should seed parser-compatible headings'
+    );
+    assert.ok(
+      !handoffFile.includes('## Files Changed') &&
+      !handoffFile.includes('## Tests / Verification') &&
+      !handoffFile.includes('## Follow-ups'),
+      'Handoff placeholder should not use legacy headings'
+    );
   } finally {
     fs.rmSync(tempRoot, { recursive: true, force: true });
   }