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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* fix: detect files_modified overlap and enforce wave ordering for dependent plans (#1587)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
2026-04-03 12:19:41 -04:00
committed by GitHub
parent 40fc681b28
commit 0b43cfd303
4 changed files with 384 additions and 17 deletions

View File

@@ -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}"`
</step>
<step name="group_into_plans">
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
</step>

View File

@@ -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 `<objective>`. 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="
<objective>
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.
</objective>
<worktree_branch_check>
@@ -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`)
</success_criteria>
"
)
@@ -345,9 +378,21 @@ Execute each selected wave in sequence. Within a wave: parallel if `PARALLELIZAT
</sequential_execution>
```
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:
```
<success_criteria>
- [ ] 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`)
</success_criteria>
```
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 `<checkpoint_handling>`.
8. **Execute checkpoint plans between waves** — see `<checkpoint_handling>`.
7. **Proceed to next wave.**
9. **Proceed to next wave.**
</step>
<step name="checkpoint_handling">

View File

@@ -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]*?<success_criteria>([\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]*?<success_criteria>([\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]*?<success_criteria>([\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]*?<success_criteria>([\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]*?<success_criteria>([\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)'
);
});
});

View File

@@ -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(
/<step name="assign_waves">([\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(
/<step name="assign_waves">([\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'
);
});
});