From af318b8f04f0ed52826926ecbf4c63e85066af0a Mon Sep 17 00:00:00 2001 From: Affaan Mustafa Date: Thu, 12 Mar 2026 15:34:05 -0700 Subject: [PATCH] fix: address remaining orchestration review comments --- .agents/skills/crosspost/SKILL.md | 12 ++-- .agents/skills/exa-search/SKILL.md | 2 +- scripts/lib/orchestration-session.js | 64 ++++++++++++++++--- scripts/lib/tmux-worktree-orchestrator.js | 38 ++++++++--- scripts/orchestrate-codex-worker.sh | 8 +-- scripts/orchestrate-worktrees.js | 7 ++- skills/configure-ecc/SKILL.md | 1 + skills/crosspost/SKILL.md | 12 ++-- skills/dmux-workflows/SKILL.md | 4 +- skills/exa-search/SKILL.md | 8 +-- tests/lib/orchestration-session.test.js | 66 ++++++++++++++++++++ tests/lib/tmux-worktree-orchestrator.test.js | 57 +++++++++++++++++ 12 files changed, 242 insertions(+), 37 deletions(-) diff --git a/.agents/skills/crosspost/SKILL.md b/.agents/skills/crosspost/SKILL.md index c20e03ec..fe021365 100644 --- a/.agents/skills/crosspost/SKILL.md +++ b/.agents/skills/crosspost/SKILL.md @@ -8,14 +8,16 @@ origin: ECC Distribute content across multiple social platforms with platform-native adaptation. -## When to Activate +## When to Use - User wants to post content to multiple platforms - Publishing announcements, launches, or updates across social media - Repurposing a post from one platform to others - User says "crosspost", "post everywhere", "share on all platforms", or "distribute this" -## Core Rules +## How It Works + +### Core Rules 1. **Never post identical content cross-platform.** Each platform gets a native adaptation. 2. **Primary platform first.** Post to the main platform, then adapt for others. @@ -23,7 +25,7 @@ Distribute content across multiple social platforms with platform-native adaptat 4. **One idea per post.** If the source content has multiple ideas, split across posts. 5. **Attribution matters.** If crossposting someone else's content, credit the source. -## Platform Specifications +### Platform Specifications | Platform | Max Length | Link Handling | Hashtags | Media | |----------|-----------|---------------|----------|-------| @@ -32,7 +34,7 @@ Distribute content across multiple social platforms with platform-native adaptat | Threads | 500 chars | Separate link attachment | None typical | Images, video | | Bluesky | 300 chars | Via facets (rich text) | None (use feeds) | Images | -## Workflow +### Workflow ### Step 1: Create Source Content @@ -87,7 +89,7 @@ Post adapted versions to remaining platforms: - Stagger timing (not all at once — 30-60 min gaps) - Include cross-platform references where appropriate ("longer thread on X" etc.) -## Content Adaptation Examples +## Examples ### Source: Product Launch diff --git a/.agents/skills/exa-search/SKILL.md b/.agents/skills/exa-search/SKILL.md index 4e26a6b7..16001637 100644 --- a/.agents/skills/exa-search/SKILL.md +++ b/.agents/skills/exa-search/SKILL.md @@ -27,7 +27,7 @@ Exa MCP server must be configured. Add to `~/.claude.json`: "args": [ "-y", "exa-mcp-server", - "tools=web_search_exa,web_search_advanced_exa,get_code_context_exa,crawling_exa,company_research_exa,linkedin_search_exa,deep_researcher_start,deep_researcher_check" + "tools=web_search_exa,web_search_advanced_exa,get_code_context_exa,crawling_exa,company_research_exa,people_search_exa,deep_researcher_start,deep_researcher_check" ], "env": { "EXA_API_KEY": "YOUR_EXA_API_KEY_HERE" } } diff --git a/scripts/lib/orchestration-session.js b/scripts/lib/orchestration-session.js index 1eff6340..0f0f60d6 100644 --- a/scripts/lib/orchestration-session.js +++ b/scripts/lib/orchestration-session.js @@ -106,11 +106,31 @@ function parseWorkerTask(content) { }; } +function parseFirstSection(content, headings) { + for (const heading of headings) { + const section = parseSection(content, heading); + if (section) { + return section; + } + } + + return ''; +} + function parseWorkerHandoff(content) { return { - summary: parseBullets(parseSection(content, 'Summary')), - validation: parseBullets(parseSection(content, 'Validation')), - remainingRisks: parseBullets(parseSection(content, 'Remaining Risks')) + summary: parseBullets(parseFirstSection(content, ['Summary'])), + validation: parseBullets(parseFirstSection(content, [ + 'Validation', + 'Tests / Verification', + 'Tests', + 'Verification' + ])), + remainingRisks: parseBullets(parseFirstSection(content, [ + 'Remaining Risks', + 'Follow-ups', + 'Follow Ups' + ])) }; } @@ -250,18 +270,48 @@ function buildSessionSnapshot({ sessionName, coordinationDir, panes }) { }; } +function readPlanConfig(absoluteTarget) { + let config; + + try { + config = JSON.parse(fs.readFileSync(absoluteTarget, 'utf8')); + } catch (error) { + throw new Error(`Invalid orchestration plan JSON: ${absoluteTarget}`); + } + + if (!config || Array.isArray(config) || typeof config !== 'object') { + throw new Error(`Invalid orchestration plan: expected a JSON object (${absoluteTarget})`); + } + + return config; +} + +function readPlanString(config, key, absoluteTarget) { + const value = config[key]; + + if (value === undefined) { + return undefined; + } + + if (typeof value !== 'string' || value.trim().length === 0) { + throw new Error(`Invalid orchestration plan: ${key} must be a non-empty string (${absoluteTarget})`); + } + + return value.trim(); +} + function resolveSnapshotTarget(targetPath, cwd = process.cwd()) { const absoluteTarget = path.resolve(cwd, targetPath); if (fs.existsSync(absoluteTarget) && fs.statSync(absoluteTarget).isFile()) { - const config = JSON.parse(fs.readFileSync(absoluteTarget, 'utf8')); - const repoRoot = path.resolve(config.repoRoot || cwd); + const config = readPlanConfig(absoluteTarget); + const repoRoot = path.resolve(readPlanString(config, 'repoRoot', absoluteTarget) || cwd); const sessionName = normalizeSessionName( - config.sessionName || path.basename(repoRoot), + readPlanString(config, 'sessionName', absoluteTarget) || path.basename(repoRoot), 'session' ); const coordinationRoot = path.resolve( - config.coordinationRoot || path.join(repoRoot, '.orchestration') + readPlanString(config, 'coordinationRoot', absoluteTarget) || path.join(repoRoot, '.orchestration') ); return { diff --git a/scripts/lib/tmux-worktree-orchestrator.js b/scripts/lib/tmux-worktree-orchestrator.js index 902ddbe1..0aa89c97 100644 --- a/scripts/lib/tmux-worktree-orchestrator.js +++ b/scripts/lib/tmux-worktree-orchestrator.js @@ -34,6 +34,18 @@ 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] = quotedValue; + accumulator[`${key}_raw`] = stringValue; + accumulator[`${key}_sh`] = quotedValue; + return accumulator; + }, {}); +} + function normalizeSeedPaths(seedPaths, repoRoot) { const resolvedRepoRoot = path.resolve(repoRoot); const entries = Array.isArray(seedPaths) ? seedPaths : []; @@ -126,6 +138,13 @@ function buildWorkerArtifacts(workerPlan) { '## Completion', 'Do not spawn subagents or external agents for this task.', 'Report results in your final response.', + 'Respond with these exact sections so orchestration parsing can succeed:', + '## Summary', + '- ...', + '## Validation', + '- ...', + '## Remaining Risks', + '- ...', `The worker launcher captures your response in \`${workerPlan.handoffFilePath}\` automatically.`, `The worker launcher updates \`${workerPlan.statusFilePath}\` automatically.` ].join('\n') @@ -138,13 +157,10 @@ function buildWorkerArtifacts(workerPlan) { '## Summary', '- Pending', '', - '## Files Changed', + '## Validation', '- Pending', '', - '## Tests / Verification', - '- Pending', - '', - '## Follow-ups', + '## Remaining Risks', '- Pending' ].join('\n') }, @@ -180,6 +196,7 @@ function buildOrchestrationPlan(config = {}) { throw new Error('buildOrchestrationPlan requires at least one worker'); } + const seenWorkerSlugs = new Map(); const workerPlans = workers.map((worker, index) => { if (!worker || typeof worker.task !== 'string' || worker.task.trim().length === 0) { throw new Error(`Worker ${index + 1} is missing a task`); @@ -187,6 +204,13 @@ function buildOrchestrationPlan(config = {}) { const workerName = worker.name || `worker-${index + 1}`; const workerSlug = slugify(workerName, `worker-${index + 1}`); + if (seenWorkerSlugs.has(workerSlug)) { + const firstWorkerName = seenWorkerSlugs.get(workerSlug); + throw new Error( + `Worker names must map to unique slugs: ${workerSlug} (${firstWorkerName}, ${workerName})` + ); + } + seenWorkerSlugs.set(workerSlug, workerName); const branchName = `orchestrator-${sessionName}-${workerSlug}`; const worktreePath = path.join(worktreeRoot, `${repoName}-${sessionName}-${workerSlug}`); const workerCoordinationDir = path.join(coordinationDir, workerSlug); @@ -196,7 +220,7 @@ function buildOrchestrationPlan(config = {}) { const launcherCommand = worker.launcherCommand || defaultLauncher; const workerSeedPaths = normalizeSeedPaths(worker.seedPaths, repoRoot); const seedPaths = normalizeSeedPaths([...globalSeedPaths, ...workerSeedPaths], repoRoot); - const templateVariables = { + const templateVariables = buildTemplateVariables({ branch_name: branchName, handoff_file: handoffFilePath, repo_root: repoRoot, @@ -206,7 +230,7 @@ function buildOrchestrationPlan(config = {}) { worker_name: workerName, worker_slug: workerSlug, worktree_path: worktreePath - }; + }); if (!launcherCommand) { throw new Error(`Worker ${workerName} is missing a launcherCommand`); diff --git a/scripts/orchestrate-codex-worker.sh b/scripts/orchestrate-codex-worker.sh index 5461596b..5b115964 100755 --- a/scripts/orchestrate-codex-worker.sh +++ b/scripts/orchestrate-codex-worker.sh @@ -51,11 +51,11 @@ Rules: - Report progress and final results in stdout only. - Do not write handoff or status files yourself; the launcher manages those artifacts. - If you change code or docs, keep the scope narrow and defensible. -- In your final response, include exactly these sections: +- In your final response, include these exact sections: 1. Summary - 2. Files Changed - 3. Validation - 4. Remaining Risks + 2. Validation + 3. Remaining Risks +- You may include Files Changed if useful, but keep the three sections above exact. Task file: $task_file diff --git a/scripts/orchestrate-worktrees.js b/scripts/orchestrate-worktrees.js index 0368825f..c6af18d0 100644 --- a/scripts/orchestrate-worktrees.js +++ b/scripts/orchestrate-worktrees.js @@ -17,8 +17,11 @@ function usage() { ' node scripts/orchestrate-worktrees.js [--write-only]', '', 'Placeholders supported in launcherCommand:', - ' {worker_name} {worker_slug} {session_name} {repo_root}', - ' {worktree_path} {branch_name} {task_file} {handoff_file} {status_file}', + ' Shell-safe defaults: {worker_name} {worker_slug} {session_name} {repo_root}', + ' Shell-safe defaults: {worktree_path} {branch_name} {task_file} {handoff_file} {status_file}', + ' Raw variants: {worker_name_raw} {worker_slug_raw} {session_name_raw} {repo_root_raw}', + ' Raw variants: {worktree_path_raw} {branch_name_raw} {task_file_raw} {handoff_file_raw} {status_file_raw}', + ' Explicit shell-safe aliases also exist with the _sh suffix.', '', 'Without flags the script prints a dry-run plan only.' ].join('\n')); diff --git a/skills/configure-ecc/SKILL.md b/skills/configure-ecc/SKILL.md index 110d82e9..7a5e5ee4 100644 --- a/skills/configure-ecc/SKILL.md +++ b/skills/configure-ecc/SKILL.md @@ -90,6 +90,7 @@ Options: - "Framework & Language" — "Django, Spring Boot, Go, Python, Java, Frontend, Backend patterns" - "Database" — "PostgreSQL, ClickHouse, JPA/Hibernate patterns" - "Workflow & Quality" — "TDD, verification, learning, security review, compaction" + - "Business & Content" — "Article writing, content engine, market research, investor materials, outreach" - "Research & APIs" — "Deep research, Exa search, Claude API patterns" - "Social & Content Distribution" — "X/Twitter API, crossposting alongside content-engine" - "Media Generation" — "fal.ai image/video/audio alongside VideoDB" diff --git a/skills/crosspost/SKILL.md b/skills/crosspost/SKILL.md index 937b0f0e..5d30a3eb 100644 --- a/skills/crosspost/SKILL.md +++ b/skills/crosspost/SKILL.md @@ -8,14 +8,16 @@ origin: ECC Distribute content across multiple social platforms with platform-native adaptation. -## When to Activate +## When to Use - User wants to post content to multiple platforms - Publishing announcements, launches, or updates across social media - Repurposing a post from one platform to others - User says "crosspost", "post everywhere", "share on all platforms", or "distribute this" -## Core Rules +## How It Works + +### Core Rules 1. **Never post identical content cross-platform.** Each platform gets a native adaptation. 2. **Primary platform first.** Post to the main platform, then adapt for others. @@ -23,7 +25,7 @@ Distribute content across multiple social platforms with platform-native adaptat 4. **One idea per post.** If the source content has multiple ideas, split across posts. 5. **Attribution matters.** If crossposting someone else's content, credit the source. -## Platform Specifications +### Platform Specifications | Platform | Max Length | Link Handling | Hashtags | Media | |----------|-----------|---------------|----------|-------| @@ -32,7 +34,7 @@ Distribute content across multiple social platforms with platform-native adaptat | Threads | 500 chars | Separate link attachment | None typical | Images, video | | Bluesky | 300 chars | Via facets (rich text) | None (use feeds) | Images | -## Workflow +### Workflow ### Step 1: Create Source Content @@ -87,7 +89,7 @@ Post adapted versions to remaining platforms: - Stagger timing (not all at once — 30-60 min gaps) - Include cross-platform references where appropriate ("longer thread on X" etc.) -## Content Adaptation Examples +## Examples ### Source: Product Launch diff --git a/skills/dmux-workflows/SKILL.md b/skills/dmux-workflows/SKILL.md index 6e6c5544..35644bd5 100644 --- a/skills/dmux-workflows/SKILL.md +++ b/skills/dmux-workflows/SKILL.md @@ -150,7 +150,7 @@ Example `plan.json`: { "sessionName": "skill-audit", "baseRef": "HEAD", - "launcherCommand": "codex exec --cwd {worktree_path} --task-file {task_file}", + "launcherCommand": "codex exec --cwd {worktree_path_sh} --task-file {task_file_sh}", "workers": [ { "name": "docs-a", "task": "Fix skills 1-4 and write handoff notes." }, { "name": "docs-b", "task": "Fix skills 5-8 and write handoff notes." } @@ -176,7 +176,7 @@ Use `seedPaths` when workers need access to dirty or untracked local files that "scripts/lib/tmux-worktree-orchestrator.js", ".claude/plan/workflow-e2e-test.json" ], - "launcherCommand": "bash {repo_root}/scripts/orchestrate-codex-worker.sh {task_file} {handoff_file} {status_file}", + "launcherCommand": "bash {repo_root_sh}/scripts/orchestrate-codex-worker.sh {task_file_sh} {handoff_file_sh} {status_file_sh}", "workers": [ { "name": "seed-check", "task": "Verify seeded files are present before starting work." } ] diff --git a/skills/exa-search/SKILL.md b/skills/exa-search/SKILL.md index 8a8059b8..45a7ee01 100644 --- a/skills/exa-search/SKILL.md +++ b/skills/exa-search/SKILL.md @@ -27,7 +27,7 @@ Exa MCP server must be configured. Add to `~/.claude.json`: "args": [ "-y", "exa-mcp-server", - "tools=web_search_exa,web_search_advanced_exa,get_code_context_exa,crawling_exa,company_research_exa,linkedin_search_exa,deep_researcher_start,deep_researcher_check" + "tools=web_search_exa,web_search_advanced_exa,get_code_context_exa,crawling_exa,company_research_exa,people_search_exa,deep_researcher_start,deep_researcher_check" ], "env": { "EXA_API_KEY": "YOUR_EXA_API_KEY_HERE" } } @@ -103,11 +103,11 @@ company_research_exa(companyName: "Anthropic", numResults: 5) | `companyName` | string | required | Company name | | `numResults` | number | 5 | Number of results | -### linkedin_search_exa -Find professional profiles and company-adjacent people research. +### people_search_exa +Find professional profiles and bios. ``` -linkedin_search_exa(query: "AI safety researchers at Anthropic", numResults: 5) +people_search_exa(query: "AI safety researchers at Anthropic", numResults: 5) ``` ### crawling_exa diff --git a/tests/lib/orchestration-session.test.js b/tests/lib/orchestration-session.test.js index 4bd4b340..be196b7b 100644 --- a/tests/lib/orchestration-session.test.js +++ b/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); diff --git a/tests/lib/tmux-worktree-orchestrator.test.js b/tests/lib/tmux-worktree-orchestrator.test.js index c6a657eb..cedaf085 100644 --- a/tests/lib/tmux-worktree-orchestrator.test.js +++ b/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 }); }