From 0b43cfd303a0fef3e06048c5ef9fa1d8ead8ddde Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 3 Apr 2026 12:19:41 -0400 Subject: [PATCH] fix: detect files_modified overlap and enforce wave ordering for dependent plans (#1587) (#1600) * fix: correct STATE.md progress counter fields during plan/phase completion (#1589) Co-Authored-By: Claude Sonnet 4.6 * ci: re-run CI with Windows pointer lifecycle fix in main * fix: orchestrator owns STATE.md/ROADMAP.md writes in parallel worktree mode (#1571) Co-Authored-By: Claude Sonnet 4.6 * fix: detect files_modified overlap and enforce wave ordering for dependent plans (#1587) Co-Authored-By: Claude Sonnet 4.6 * fix: trim gsd-planner.md below 50000-char prompt-injection limit The assign_waves section added in this branch pushed agents/gsd-planner.md to 50271 chars, triggering the security scanner's prompt-stuffing check on all CI platforms. Condense prose while preserving all logic and validation rules; file is now 49754 chars. Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Claude Sonnet 4.6 --- agents/gsd-planner.md | 15 +- get-shit-done/workflows/execute-phase.md | 95 +++++++++-- .../execute-phase-worktree-artifacts.test.cjs | 134 +++++++++++++++ tests/parallel-dependent-plans.test.cjs | 157 ++++++++++++++++++ 4 files changed, 384 insertions(+), 17 deletions(-) create mode 100644 tests/execute-phase-worktree-artifacts.test.cjs create mode 100644 tests/parallel-dependent-plans.test.cjs diff --git a/agents/gsd-planner.md b/agents/gsd-planner.md index 228f45df4..f61e5e649 100644 --- a/agents/gsd-planner.md +++ b/agents/gsd-planner.md @@ -1216,13 +1216,26 @@ for each plan in plan_order: else: plan.wave = max(waves[dep] for dep in plan.depends_on) + 1 waves[plan.id] = plan.wave + +# Implicit dependency: files_modified overlap forces a later wave. +for each plan B in plan_order: + for each earlier plan A where A != B: + if any file in B.files_modified is also in A.files_modified: + B.wave = max(B.wave, A.wave + 1) + waves[B.id] = B.wave ``` + +**Rule:** Any two plans in the same wave MUST have zero `files_modified` overlap — even if +no explicit `depends_on` was set. After computing all wave numbers, verify every wave group: +collect the union of `files_modified` per wave; if any file appears in two or more plans +within the same wave, bump the later plan to the next wave and repeat until clean. +Log each bump: `"Plan {B} moved to wave {N+1}: files_modified overlap with Plan {A} on {file}"` Rules: 1. Same-wave tasks with no file conflicts → parallel plans -2. Shared files → same plan or sequential plans +2. Shared files → same plan or sequential plans (shared file = implicit dependency → later wave) 3. Checkpoint tasks → `autonomous: false` 4. Each plan: 2-3 tasks, single concern, ~50% context target diff --git a/get-shit-done/workflows/execute-phase.md b/get-shit-done/workflows/execute-phase.md index d3a2690ba..888a7ba30 100644 --- a/get-shit-done/workflows/execute-phase.md +++ b/get-shit-done/workflows/execute-phase.md @@ -218,7 +218,41 @@ Execute each selected wave in sequence. Within a wave: parallel if `PARALLELIZAT **For each wave:** -1. **Describe what's being built (BEFORE spawning):** +1. **Intra-wave files_modified overlap check (BEFORE spawning):** + + Before spawning any agents for this wave, inspect the `files_modified` list of all plans + in the wave. Check every pair of plans in the wave — if any two plans share even one file + in their `files_modified` lists, those plans have an implicit dependency and MUST NOT run + in parallel. + + **Detection algorithm (pseudocode):** + ``` + seen_files = {} + overlapping_plans = [] + for each plan in wave_plans: + for each file in plan.files_modified: + if file in seen_files: + overlapping_plans.add(plan, seen_files[file]) # both plans overlap on this file + else: + seen_files[file] = plan + ``` + + **If overlap is detected:** + - Warn the user: + ``` + ⚠ Intra-wave files_modified overlap detected in Wave {N}: + Plan {A} and Plan {B} both modify {file} + Running these plans sequentially to avoid parallel worktree conflicts. + ``` + - Override `PARALLELIZATION` to `false` for this wave only — run all plans in the wave + sequentially regardless of the global parallelization setting. + - This is a safety net for plans that were incorrectly assigned to the same wave. + The planner should have caught this; flag it as a planning defect so the user can + replan the phase if desired. + + **If no overlap:** proceed normally (parallel if `PARALLELIZATION=true`). + +2. **Describe what's being built (BEFORE spawning):** Read each plan's ``. Extract what's being built and why. @@ -236,7 +270,7 @@ Execute each selected wave in sequence. Within a wave: parallel if `PARALLELIZAT - Bad: "Executing terrain generation plan" - Good: "Procedural terrain generator using Perlin noise — creates height maps, biome zones, and collision meshes. Required before vehicle physics can interact with ground." -2. **Spawn executor agents:** +3. **Spawn executor agents:** Pass paths only — executors read files themselves with their fresh context window. For 200k models, this keeps orchestrator context lean (~10-15%). @@ -258,7 +292,8 @@ Execute each selected wave in sequence. Within a wave: parallel if `PARALLELIZAT prompt=" Execute plan {plan_number} of phase {phase_number}-{phase_name}. - Commit each task atomically. Create SUMMARY.md. Update STATE.md and ROADMAP.md. + Commit each task atomically. Create SUMMARY.md. + Do NOT update STATE.md or ROADMAP.md — the orchestrator owns those writes after all worktree agents in the wave complete. @@ -327,8 +362,6 @@ Execute each selected wave in sequence. Within a wave: parallel if `PARALLELIZAT - [ ] All tasks executed - [ ] Each task committed individually - [ ] SUMMARY.md created in plan directory - - [ ] STATE.md updated with position and decisions - - [ ] ROADMAP.md updated with plan progress (via `roadmap update-plan-progress`) " ) @@ -345,9 +378,21 @@ Execute each selected wave in sequence. Within a wave: parallel if `PARALLELIZAT ``` + The sequential mode Task prompt uses the same structure as worktree mode but with these differences in success_criteria — since there is only one agent writing at a time, there are no shared-file conflicts: + + ``` + + - [ ] All tasks executed + - [ ] Each task committed individually + - [ ] SUMMARY.md created in plan directory + - [ ] STATE.md updated with position and decisions + - [ ] ROADMAP.md updated with plan progress (via `roadmap update-plan-progress`) + + ``` + When worktrees are disabled, execute plans **one at a time within each wave** (sequential) regardless of the `PARALLELIZATION` setting — multiple agents writing to the same working tree concurrently would cause conflicts. -3. **Wait for all agents in wave to complete.** +4. **Wait for all agents in wave to complete.** **Completion signal fallback (Copilot and runtimes where Task() may not return):** @@ -361,17 +406,17 @@ Execute each selected wave in sequence. Within a wave: parallel if `PARALLELIZAT ``` **If SUMMARY.md exists AND commits are found:** The agent completed successfully — - treat as done and proceed to step 4. Log: `"✓ {Plan ID} completed (verified via spot-check — completion signal not received)"` + treat as done and proceed to step 5. Log: `"✓ {Plan ID} completed (verified via spot-check — completion signal not received)"` **If SUMMARY.md does NOT exist after a reasonable wait:** The agent may still be running or may have failed silently. Check `git log --oneline -5` for recent activity. If commits are still appearing, wait longer. If no activity, report - the plan as failed and route to the failure handler in step 5. + the plan as failed and route to the failure handler in step 6. **This fallback applies automatically to all runtimes.** Claude Code's Task() normally returns synchronously, but the fallback ensures resilience if it doesn't. -4. **Post-wave hook validation (parallel mode only):** +5. **Post-wave hook validation (parallel mode only):** When agents committed with `--no-verify`, run pre-commit hooks once after the wave: ```bash @@ -381,7 +426,7 @@ Execute each selected wave in sequence. Within a wave: parallel if `PARALLELIZAT ``` If hooks fail: report the failure and ask "Fix hook issues now?" or "Continue to next wave?" -4.5. **Worktree cleanup (when `isolation="worktree"` was used):** +5.5. **Worktree cleanup (when `isolation="worktree"` was used):** When executor agents ran in worktree isolation, their commits land on temporary branches in separate working trees. After the wave completes, merge these changes back and clean up: @@ -414,7 +459,25 @@ Execute each selected wave in sequence. Within a wave: parallel if `PARALLELIZAT **If no worktrees found:** Skip silently — agents may have been spawned without worktree isolation. -5. **Report completion — spot-check claims first:** +5.6. **Post-wave shared artifact update (worktree mode only):** + + When executor agents ran with `isolation="worktree"`, they skipped STATE.md and ROADMAP.md updates to avoid last-merge-wins overwrites. The orchestrator is the single writer for these files. After worktrees are merged back, update shared artifacts once: + + ```bash + # Update ROADMAP.md for each completed plan in this wave + for PLAN_ID in ${WAVE_PLAN_IDS}; do + node "$HOME/.claude/get-shit-done/bin/gsd-tools.cjs" roadmap update-plan-progress "${PHASE_NUMBER}" "${PLAN_ID}" completed + done + + # Update STATE.md position to reflect the last completed plan in this wave + node "$HOME/.claude/get-shit-done/bin/gsd-tools.cjs" state update-position --phase "${PHASE_NUMBER}" --plan "${LAST_PLAN_ID}" + ``` + + Where `WAVE_PLAN_IDS` is the space-separated list of plan IDs that completed in this wave, and `LAST_PLAN_ID` is the last plan ID in the wave (used to set current position). + + **If `workflow.use_worktrees` is `false`:** Sequential agents already updated STATE.md and ROADMAP.md themselves — skip this step. + +6. **Report completion — spot-check claims first:** For each SUMMARY.md: - Verify first 2 files from `key-files.created` exist on disk @@ -439,13 +502,13 @@ Execute each selected wave in sequence. Within a wave: parallel if `PARALLELIZAT - Bad: "Wave 2 complete. Proceeding to Wave 3." - Good: "Terrain system complete — 3 biome types, height-based texturing, physics collision meshes. Vehicle physics (Wave 3) can now reference ground surfaces." -5. **Handle failures:** +7. **Handle failures:** - **Known Claude Code bug (classifyHandoffIfNeeded):** If an agent reports "failed" with error containing `classifyHandoffIfNeeded is not defined`, this is a Claude Code runtime bug — not a GSD or agent issue. The error fires in the completion handler AFTER all tool calls finish. In this case: run the same spot-checks as step 4 (SUMMARY.md exists, git commits present, no Self-Check: FAILED). If spot-checks PASS → treat as **successful**. If spot-checks FAIL → treat as real failure below. + **Known Claude Code bug (classifyHandoffIfNeeded):** If an agent reports "failed" with error containing `classifyHandoffIfNeeded is not defined`, this is a Claude Code runtime bug — not a GSD or agent issue. The error fires in the completion handler AFTER all tool calls finish. In this case: run the same spot-checks as step 5 (SUMMARY.md exists, git commits present, no Self-Check: FAILED). If spot-checks PASS → treat as **successful**. If spot-checks FAIL → treat as real failure below. For real failures: report which plan failed → ask "Continue?" or "Stop?" → if continue, dependent plans may also fail. If stop, partial completion report. -5b. **Pre-wave dependency check (waves 2+ only):** +7b. **Pre-wave dependency check (waves 2+ only):** Before spawning wave N+1, for each plan in the upcoming wave: ```bash @@ -466,9 +529,9 @@ Execute each selected wave in sequence. Within a wave: parallel if `PARALLELIZAT Key-links referencing files in the CURRENT (upcoming) wave are skipped. -6. **Execute checkpoint plans between waves** — see ``. +8. **Execute checkpoint plans between waves** — see ``. -7. **Proceed to next wave.** +9. **Proceed to next wave.** diff --git a/tests/execute-phase-worktree-artifacts.test.cjs b/tests/execute-phase-worktree-artifacts.test.cjs new file mode 100644 index 000000000..0e986e9d6 --- /dev/null +++ b/tests/execute-phase-worktree-artifacts.test.cjs @@ -0,0 +1,134 @@ +/** + * Execute-phase worktree shared artifact ownership tests + * + * Guards against bug #1571: worktree executor agents independently writing + * STATE.md and ROADMAP.md, causing last-merge-wins overwrites. + * + * Fix: In parallel worktree mode, remove STATE.md/ROADMAP.md update requirements + * from the executor agent success_criteria. The orchestrator owns those writes + * after each wave via single-writer post-wave commands. + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert'); +const fs = require('fs'); +const path = require('path'); + +const WORKFLOW_PATH = path.join(__dirname, '..', 'get-shit-done', 'workflows', 'execute-phase.md'); + +describe('execute-phase worktree: shared artifact ownership (#1571)', () => { + test('workflow file exists', () => { + assert.ok(fs.existsSync(WORKFLOW_PATH), 'workflows/execute-phase.md should exist'); + }); + + test('worktree executor agent success_criteria does NOT include STATE.md update', () => { + const content = fs.readFileSync(WORKFLOW_PATH, 'utf-8'); + + // Extract the worktree Task() block (between "Worktree mode" and "Sequential mode") + const worktreeMatch = content.match( + /\*\*Worktree mode\*\*[\s\S]*?([\s\S]*?)<\/success_criteria>/ + ); + assert.ok(worktreeMatch, 'should find success_criteria inside the worktree mode Task block'); + + const criteria = worktreeMatch[1]; + assert.ok( + !criteria.includes('STATE.md'), + 'worktree executor success_criteria must NOT reference STATE.md (orchestrator owns this write)' + ); + }); + + test('worktree executor agent success_criteria does NOT include ROADMAP.md update', () => { + const content = fs.readFileSync(WORKFLOW_PATH, 'utf-8'); + + // Extract the worktree Task() block + const worktreeMatch = content.match( + /\*\*Worktree mode\*\*[\s\S]*?([\s\S]*?)<\/success_criteria>/ + ); + assert.ok(worktreeMatch, 'should find success_criteria inside the worktree mode Task block'); + + const criteria = worktreeMatch[1]; + assert.ok( + !criteria.includes('ROADMAP.md'), + 'worktree executor success_criteria must NOT reference ROADMAP.md (orchestrator owns this write)' + ); + }); + + test('worktree executor agent success_criteria includes SUMMARY.md creation', () => { + const content = fs.readFileSync(WORKFLOW_PATH, 'utf-8'); + + // SUMMARY.md is plan-local and safe for worktree agents to create + const worktreeMatch = content.match( + /\*\*Worktree mode\*\*[\s\S]*?([\s\S]*?)<\/success_criteria>/ + ); + assert.ok(worktreeMatch, 'should find success_criteria inside the worktree mode Task block'); + + const criteria = worktreeMatch[1]; + assert.ok( + criteria.includes('SUMMARY.md'), + 'worktree executor success_criteria should still require SUMMARY.md creation' + ); + }); + + test('post-wave orchestrator runs roadmap update-plan-progress for each completed plan', () => { + const content = fs.readFileSync(WORKFLOW_PATH, 'utf-8'); + assert.ok( + content.includes('roadmap update-plan-progress'), + 'post-wave section should contain orchestrator-owned roadmap update-plan-progress command' + ); + // Confirm it is in a post-wave context, not only inside an agent prompt + const postWaveIdx = content.indexOf('roadmap update-plan-progress'); + const worktreeAgentStart = content.indexOf('isolation="worktree"'); + const worktreeAgentEnd = content.indexOf('**Sequential mode**'); + assert.ok( + postWaveIdx < worktreeAgentStart || postWaveIdx > worktreeAgentEnd, + 'roadmap update-plan-progress must appear outside the worktree agent prompt (orchestrator-owned)' + ); + }); + + test('post-wave orchestrator runs state update-position after completing a wave in worktree mode', () => { + const content = fs.readFileSync(WORKFLOW_PATH, 'utf-8'); + assert.ok( + content.includes('state update-position'), + 'post-wave section should contain orchestrator-owned state update-position command' + ); + // Confirm it appears after the worktree agent block + const stateUpdateIdx = content.lastIndexOf('state update-position'); + const worktreeAgentEnd = content.indexOf('**Sequential mode**'); + assert.ok( + stateUpdateIdx > worktreeAgentEnd, + 'state update-position must appear after the worktree agent block (orchestrator-owned, post-wave)' + ); + }); + + test('sequential mode executor agent success_criteria still includes STATE.md update', () => { + const content = fs.readFileSync(WORKFLOW_PATH, 'utf-8'); + + // Extract the sequential mode Task() block + const seqMatch = content.match( + /\*\*Sequential mode\*\*[\s\S]*?([\s\S]*?)<\/success_criteria>/ + ); + assert.ok(seqMatch, 'should find success_criteria inside the sequential mode Task block'); + + const criteria = seqMatch[1]; + assert.ok( + criteria.includes('STATE.md'), + 'sequential executor success_criteria should still require STATE.md update (no conflict risk)' + ); + }); + + test('sequential mode executor agent success_criteria still includes ROADMAP.md update', () => { + const content = fs.readFileSync(WORKFLOW_PATH, 'utf-8'); + + // Extract the sequential mode Task() block + const seqMatch = content.match( + /\*\*Sequential mode\*\*[\s\S]*?([\s\S]*?)<\/success_criteria>/ + ); + assert.ok(seqMatch, 'should find success_criteria inside the sequential mode Task block'); + + const criteria = seqMatch[1]; + assert.ok( + criteria.includes('ROADMAP.md'), + 'sequential executor success_criteria should still require ROADMAP.md update (no conflict risk)' + ); + }); +}); diff --git a/tests/parallel-dependent-plans.test.cjs b/tests/parallel-dependent-plans.test.cjs new file mode 100644 index 000000000..255c507ab --- /dev/null +++ b/tests/parallel-dependent-plans.test.cjs @@ -0,0 +1,157 @@ +/** + * Tests for bug #1587: parallel agents for dependent plans + * + * Validates that: + * 1. gsd-planner.md assign_waves step explicitly checks files_modified overlap + * and mandates a later wave for any plan that shares files with a prior plan. + * 2. execute-phase.md has a pre-spawn intra-wave files_modified overlap check + * and directs sequential execution when overlap is detected. + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert'); +const fs = require('fs'); +const path = require('path'); + +const PLANNER_AGENT_PATH = path.join(__dirname, '..', 'agents', 'gsd-planner.md'); +const EXECUTE_PHASE_PATH = path.join( + __dirname, + '..', + 'get-shit-done', + 'workflows', + 'execute-phase.md' +); + +// --------------------------------------------------------------------------- +// gsd-planner.md — wave assignment must account for files_modified overlap +// --------------------------------------------------------------------------- + +describe('gsd-planner agent: files_modified wave ordering', () => { + test('planner agent file exists', () => { + assert.ok(fs.existsSync(PLANNER_AGENT_PATH), 'agents/gsd-planner.md should exist'); + }); + + test('assign_waves step checks files_modified overlap', () => { + const content = fs.readFileSync(PLANNER_AGENT_PATH, 'utf-8'); + // The assign_waves step must mention files_modified overlap as a wave-bumping condition + assert.ok( + content.includes('files_modified'), + 'assign_waves step should reference files_modified' + ); + // Must state that overlap forces a later wave (not just "same plan or sequential") + assert.ok( + content.includes('files_modified overlap') || + content.includes('files_modified') && + (content.includes('later wave') || content.includes('strictly later wave')), + 'assign_waves step should explicitly require a later wave when files_modified overlap exists' + ); + }); + + test('assign_waves step contains explicit overlap → later-wave rule', () => { + const content = fs.readFileSync(PLANNER_AGENT_PATH, 'utf-8'); + // Look for the assign_waves step block + const assignWavesMatch = content.match( + /([\s\S]*?)<\/step>/ + ); + assert.ok(assignWavesMatch, 'assign_waves step should exist in gsd-planner.md'); + + const stepContent = assignWavesMatch[1]; + + // Must mention files_modified as a wave-ordering factor inside the step + assert.ok( + stepContent.includes('files_modified'), + 'assign_waves step body must reference files_modified as a wave-assignment factor' + ); + }); + + test('assign_waves step treats files_modified overlap same as depends_on dependency', () => { + const content = fs.readFileSync(PLANNER_AGENT_PATH, 'utf-8'); + const assignWavesMatch = content.match( + /([\s\S]*?)<\/step>/ + ); + assert.ok(assignWavesMatch, 'assign_waves step should exist'); + + const stepContent = assignWavesMatch[1]; + + // The step must bump the wave when files_modified overlap exists + assert.ok( + stepContent.includes('overlap') || stepContent.includes('shared file'), + 'assign_waves step must handle file overlap as a wave-bumping condition' + ); + }); + + test('planner has validation step or quality gate for wave/files_modified consistency', () => { + const content = fs.readFileSync(PLANNER_AGENT_PATH, 'utf-8'); + // Either a validation step or the quality_gate checklist must assert no same-wave overlap + const hasValidationStep = + content.includes('validate_waves') || + content.includes('wave_validation') || + content.includes('files_modified overlap'); + const hasQualityGateCheck = + content.includes('files_modified') && content.includes('same wave'); + assert.ok( + hasValidationStep || hasQualityGateCheck, + 'planner should validate that no two plans in the same wave share files_modified entries' + ); + }); +}); + +// --------------------------------------------------------------------------- +// execute-phase.md — pre-spawn intra-wave overlap safety net +// --------------------------------------------------------------------------- + +describe('execute-phase workflow: intra-wave files_modified overlap check', () => { + test('execute-phase workflow file exists', () => { + assert.ok(fs.existsSync(EXECUTE_PHASE_PATH), 'workflows/execute-phase.md should exist'); + }); + + test('execute_waves step contains intra-wave files_modified overlap check', () => { + const content = fs.readFileSync(EXECUTE_PHASE_PATH, 'utf-8'); + // The workflow must mention checking files_modified overlap before spawning + assert.ok( + content.includes('files_modified') && + (content.includes('overlap') || content.includes('intra-wave')), + 'execute-phase workflow should check for files_modified overlap within a wave before spawning' + ); + }); + + test('overlap detection is placed before agent spawning', () => { + const content = fs.readFileSync(EXECUTE_PHASE_PATH, 'utf-8'); + // Overlap check keyword must appear before the Task( spawn call + const overlapIdx = content.indexOf('intra-wave') !== -1 + ? content.indexOf('intra-wave') + : content.indexOf('files_modified overlap'); + const spawnIdx = content.indexOf('Spawn executor agents'); + assert.ok(overlapIdx !== -1, 'overlap check text should exist in execute-phase.md'); + assert.ok(spawnIdx !== -1, '"Spawn executor agents" heading should exist'); + assert.ok( + overlapIdx < spawnIdx, + 'overlap check should appear before the "Spawn executor agents" section' + ); + }); + + test('workflow warns and switches to sequential when overlap detected', () => { + const content = fs.readFileSync(EXECUTE_PHASE_PATH, 'utf-8'); + // Must log a warning and force sequential execution for overlapping plans + assert.ok( + content.includes('sequentially') || content.includes('sequential'), + 'workflow should direct sequential execution when overlap is detected' + ); + assert.ok( + content.includes('overlap') && content.includes('warn'), + 'workflow should log a warning when files_modified overlap is detected in a wave' + ); + }); + + test('overlap check covers all plans in the wave, not just adjacent pairs', () => { + const content = fs.readFileSync(EXECUTE_PHASE_PATH, 'utf-8'); + // Must describe comparing all plans in the wave (set-intersection language) + assert.ok( + content.includes('all plans in') || + content.includes('all plans within') || + content.includes('each pair') || + content.includes('any two plans'), + 'overlap check should cover all plan pairs in the wave, not just adjacent ones' + ); + }); +});