fix(#2684,#2676): milestone.complete arg forwarding + parallel milestone phase routing (#2708)
* test(#2692): add behavioral --wave N test, annotate source-text assertions Adds two behavioral tests for wave filtering via phase-plan-index: - Verifies plans with wave frontmatter are correctly grouped by wave number - Verifies plans with no wave field default to wave 1 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(#2684,#2676): milestone.complete version validation + parallel milestone phase routing #2684: Confirms milestone.complete correctly validates and uses its version argument end-to-end. The inline archive path in milestoneComplete already forwarded version correctly; regression tests lock in that contract. #2676: phase.complete applied getMilestonePhaseFilter unconditionally, using STATE.md's primary milestone to scope the candidate set. When the completed phase belongs to a parallel (secondary) milestone, the filter excluded all phases from that milestone, leaving an empty candidate set and incorrectly returning is_last_phase: true / next_phase: null. Fix: before applying the milestone filter in Step E, check whether the completed phase itself appears in the filtered set. If not, skip the filter for both the directory scan and the ROADMAP.md fallback so phases from the secondary milestone remain visible for next-phase detection. Closes #2684 Closes #2676 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
@@ -1256,12 +1256,31 @@ export const phaseComplete: QueryHandler = async (args, projectDir, workstream)
|
||||
let nextPhaseNum: string | null = null;
|
||||
let nextPhaseName: string | null = null;
|
||||
let isLastPhase = true;
|
||||
// Tracks whether the completed phase belongs to the primary milestone in STATE.md.
|
||||
// When false (parallel-milestone case, Bug #2676), the milestone filter is bypassed
|
||||
// for next-phase detection so phases from the same secondary milestone are visible.
|
||||
let completedPhaseInPrimaryMilestone = true;
|
||||
|
||||
try {
|
||||
const isDirInMilestone = await getMilestonePhaseFilter(projectDir, workstream);
|
||||
const entries = await readdir(paths.phases, { withFileTypes: true });
|
||||
const dirs = entries.filter(e => e.isDirectory()).map(e => e.name)
|
||||
.filter(isDirInMilestone)
|
||||
const allDirs = entries.filter(e => e.isDirectory()).map(e => e.name);
|
||||
|
||||
// Guard: if the completed phase's directory is not in the current-milestone filter
|
||||
// set, the filter was built from a different (primary) milestone in STATE.md.
|
||||
// In that case skip the filter so we can find the true next phase on disk.
|
||||
// This handles parallel-milestone workflows where STATE.md's `milestone:` field
|
||||
// points at the primary milestone but the phase being completed belongs to a
|
||||
// secondary in-flight milestone. (Bug #2676)
|
||||
const completedDirInFilter = allDirs.some((d) => {
|
||||
const dm = d.match(/^(\d+[A-Z]?(?:\.\d+)*)-?/i);
|
||||
return dm && comparePhaseNum(dm[1], phaseNum) === 0 && isDirInMilestone(d);
|
||||
});
|
||||
completedPhaseInPrimaryMilestone = completedDirInFilter;
|
||||
const effectiveFilter = completedDirInFilter ? isDirInMilestone : (_d: string) => true;
|
||||
|
||||
const dirs = allDirs
|
||||
.filter(effectiveFilter)
|
||||
.sort((a, b) => comparePhaseNum(a, b));
|
||||
|
||||
for (const dir of dirs) {
|
||||
@@ -1277,11 +1296,16 @@ export const phaseComplete: QueryHandler = async (args, projectDir, workstream)
|
||||
}
|
||||
} catch { /* intentionally empty */ }
|
||||
|
||||
// Fallback: check ROADMAP.md for phases not yet scaffolded
|
||||
// Fallback: check ROADMAP.md for phases not yet scaffolded.
|
||||
// When the completed phase is from a parallel (non-primary) milestone, scan the
|
||||
// full ROADMAP rather than the primary-milestone slice so 41.3 is visible when
|
||||
// completing 41.2 for a secondary milestone. (Bug #2676)
|
||||
if (isLastPhase && existsSync(paths.roadmap)) {
|
||||
try {
|
||||
const roadmapContent = await readFile(paths.roadmap, 'utf-8');
|
||||
const roadmapForPhases = await extractCurrentMilestone(roadmapContent, projectDir);
|
||||
const roadmapForPhases = completedPhaseInPrimaryMilestone
|
||||
? await extractCurrentMilestone(roadmapContent, projectDir)
|
||||
: roadmapContent;
|
||||
const phasePattern = /#{2,4}\s*Phase\s+(\d+[A-Z]?(?:\.\d+)*)\s*:\s*([^\n]+)/gi;
|
||||
let pm: RegExpExecArray | null;
|
||||
while ((pm = phasePattern.exec(roadmapForPhases)) !== null) {
|
||||
|
||||
182
tests/bug-2676-parallel-milestone-phase-complete.test.cjs
Normal file
182
tests/bug-2676-parallel-milestone-phase-complete.test.cjs
Normal file
@@ -0,0 +1,182 @@
|
||||
/**
|
||||
* Regression tests for bug #2676:
|
||||
* `gsd-sdk query phase.complete <N>` returns is_last_phase: true
|
||||
* when the completed phase belongs to a milestone that is not the
|
||||
* primary milestone recorded in STATE.md's `milestone:` field.
|
||||
*
|
||||
* Root cause: Step E of phaseComplete applies getMilestonePhaseFilter
|
||||
* unconditionally. getMilestonePhaseFilter extracts phases from the
|
||||
* milestone slice selected by STATE.md's `milestone:` field. When
|
||||
* completing phase 41.2 (which belongs to vB) but STATE.md points at
|
||||
* vA, all 41.x directories are excluded from the candidate set and
|
||||
* the empty set causes isLastPhase = true.
|
||||
*
|
||||
* Fix: before applying the filter, check if the completed phase itself
|
||||
* passes it. If not (parallel-milestone case), skip the filter entirely
|
||||
* so all filesystem phases are visible for next-phase detection.
|
||||
*/
|
||||
|
||||
const { describe, test, beforeEach, afterEach } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('fs');
|
||||
const path = require('path');
|
||||
const { createTempProject, cleanup } = require('./helpers.cjs');
|
||||
|
||||
const SDK_CLI = path.join(__dirname, '..', 'sdk', 'dist', 'cli.js');
|
||||
const { execFileSync } = require('child_process');
|
||||
|
||||
function runSdkQuery(args, cwd) {
|
||||
try {
|
||||
const result = execFileSync(process.execPath, [SDK_CLI, 'query', ...args], {
|
||||
cwd,
|
||||
encoding: 'utf-8',
|
||||
stdio: ['pipe', 'pipe', 'pipe'],
|
||||
});
|
||||
const parsed = JSON.parse(result.trim());
|
||||
return { success: true, data: parsed };
|
||||
} catch (err) {
|
||||
const stderr = err.stderr?.toString().trim() || '';
|
||||
const stdout = err.stdout?.toString().trim() || '';
|
||||
try {
|
||||
const parsed = JSON.parse(stdout);
|
||||
return { success: true, data: parsed };
|
||||
} catch {
|
||||
/* not JSON */
|
||||
}
|
||||
return { success: false, error: stderr || err.message };
|
||||
}
|
||||
}
|
||||
|
||||
// ROADMAP.md with two active milestones: v1.0 (phases 10, 11) and v2.0 (phases 41.1, 41.2, 41.3).
|
||||
// Using numeric version IDs so extractCurrentMilestone can correctly detect milestone boundaries.
|
||||
const PARALLEL_ROADMAP = `# Roadmap
|
||||
|
||||
## v1.0 Milestone (Primary)
|
||||
|
||||
### Phase 10: Foo
|
||||
**Goal:** Foo work
|
||||
|
||||
### Phase 11: Bar
|
||||
**Goal:** Bar work
|
||||
|
||||
## v2.0 Milestone (Parallel)
|
||||
|
||||
### Phase 41.1: Baz
|
||||
**Goal:** Baz work
|
||||
|
||||
### Phase 41.2: Qux
|
||||
**Goal:** Qux work
|
||||
|
||||
### Phase 41.3: Quux
|
||||
**Goal:** Quux work
|
||||
`;
|
||||
|
||||
// STATE.md with milestone pointing at v1.0 (not v2.0).
|
||||
// Uses YAML frontmatter so extractCurrentMilestone can read the `milestone:` field.
|
||||
const STATE_POINTING_V1 = `---
|
||||
milestone: v1.0
|
||||
milestone_name: Primary Milestone
|
||||
---
|
||||
# State
|
||||
|
||||
**Current Phase:** 41.2
|
||||
**Status:** In progress
|
||||
**Last Activity:** 2026-04-25
|
||||
**Last Activity Description:** Working on qux
|
||||
`;
|
||||
|
||||
describe('bug #2676: phase.complete respects parallel milestone routing', () => {
|
||||
let tmpDir;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = createTempProject();
|
||||
// Write ROADMAP.md and STATE.md
|
||||
fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), PARALLEL_ROADMAP);
|
||||
fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), STATE_POINTING_V1);
|
||||
|
||||
// Create filesystem phase directories for vA (primary milestone)
|
||||
fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '10-foo'), { recursive: true });
|
||||
fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '11-bar'), { recursive: true });
|
||||
|
||||
// Create filesystem phase directories for vB (parallel milestone)
|
||||
fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '41.1-baz'), { recursive: true });
|
||||
|
||||
const phase412 = path.join(tmpDir, '.planning', 'phases', '41.2-qux');
|
||||
fs.mkdirSync(phase412, { recursive: true });
|
||||
fs.writeFileSync(path.join(phase412, '41.2-01-PLAN.md'), '# Plan');
|
||||
fs.writeFileSync(path.join(phase412, '41.2-01-SUMMARY.md'), '# Summary');
|
||||
|
||||
fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '41.3-quux'), { recursive: true });
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
cleanup(tmpDir);
|
||||
});
|
||||
|
||||
test('phase.complete 41.2 returns is_last_phase: false when 41.3 exists', () => {
|
||||
// BUG: before the fix this returns is_last_phase: true because the
|
||||
// milestone filter (built from vA's phases: 10, 11) excludes all 41.x dirs,
|
||||
// leaving an empty candidate set and defaulting isLastPhase to true.
|
||||
const result = runSdkQuery(['phase.complete', '41.2'], tmpDir);
|
||||
|
||||
assert.ok(result.success, `phase.complete failed: ${result.error}`);
|
||||
assert.strictEqual(
|
||||
result.data.is_last_phase,
|
||||
false,
|
||||
`expected is_last_phase: false but got true — parallel milestone filter not bypassed. ` +
|
||||
`next_phase was: ${result.data.next_phase}`
|
||||
);
|
||||
});
|
||||
|
||||
test('phase.complete 41.2 returns next_phase pointing at 41.3', () => {
|
||||
const result = runSdkQuery(['phase.complete', '41.2'], tmpDir);
|
||||
|
||||
assert.ok(result.success, `phase.complete failed: ${result.error}`);
|
||||
assert.ok(
|
||||
result.data.next_phase !== null,
|
||||
`next_phase should not be null when 41.3 exists — got: ${JSON.stringify(result.data.next_phase)}`
|
||||
);
|
||||
// next_phase may be returned as "41.3" or "41" depending on dir name matching
|
||||
assert.match(
|
||||
String(result.data.next_phase),
|
||||
/^41\.3/,
|
||||
`next_phase should start with 41.3, got: ${result.data.next_phase}`
|
||||
);
|
||||
});
|
||||
|
||||
test('phase.complete for vA phase still uses milestone filter normally', () => {
|
||||
// Completing phase 10 (in vA): the filter includes it, so the candidate
|
||||
// set for next-phase is {10, 11} and next should be 11.
|
||||
const phase10 = path.join(tmpDir, '.planning', 'phases', '10-foo');
|
||||
fs.writeFileSync(path.join(phase10, '10-01-PLAN.md'), '# Plan');
|
||||
fs.writeFileSync(path.join(phase10, '10-01-SUMMARY.md'), '# Summary');
|
||||
|
||||
const result = runSdkQuery(['phase.complete', '10'], tmpDir);
|
||||
|
||||
assert.ok(result.success, `phase.complete 10 failed: ${result.error}`);
|
||||
assert.strictEqual(result.data.is_last_phase, false, 'phase 10 should not be last (11 follows)');
|
||||
assert.match(
|
||||
String(result.data.next_phase ?? ''),
|
||||
/^11/,
|
||||
`next_phase should point to 11, got: ${result.data.next_phase}`
|
||||
);
|
||||
});
|
||||
|
||||
test('phase.complete for actual last phase of vA still returns is_last_phase: true', () => {
|
||||
// Completing phase 11 (last in vA): the filter includes phases 10 and 11,
|
||||
// nothing higher in the vA milestone, so is_last_phase should be true
|
||||
// (even though 41.x dirs exist on disk for vB).
|
||||
const phase11 = path.join(tmpDir, '.planning', 'phases', '11-bar');
|
||||
fs.writeFileSync(path.join(phase11, '11-01-PLAN.md'), '# Plan');
|
||||
fs.writeFileSync(path.join(phase11, '11-01-SUMMARY.md'), '# Summary');
|
||||
|
||||
const result = runSdkQuery(['phase.complete', '11'], tmpDir);
|
||||
|
||||
assert.ok(result.success, `phase.complete 11 failed: ${result.error}`);
|
||||
assert.strictEqual(
|
||||
result.data.is_last_phase,
|
||||
true,
|
||||
`phase 11 is last in vA; is_last_phase should be true even with vB dirs on disk`
|
||||
);
|
||||
});
|
||||
});
|
||||
125
tests/bug-2684-milestone-complete-version.test.cjs
Normal file
125
tests/bug-2684-milestone-complete-version.test.cjs
Normal file
@@ -0,0 +1,125 @@
|
||||
/**
|
||||
* Regression tests for bug #2684:
|
||||
* `gsd-sdk query milestone.complete <version>` always fails with
|
||||
* GSDError: version required for phases archive.
|
||||
*
|
||||
* Root cause: milestoneComplete extracted version from args[0] but passed
|
||||
* [] instead of args (or [version]) to phasesArchive, so phasesArchive
|
||||
* never received the version string and threw immediately.
|
||||
*
|
||||
* Fix: pass args (or [version]) when delegating to phasesArchive.
|
||||
*/
|
||||
|
||||
const { describe, test, beforeEach, afterEach } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('fs');
|
||||
const path = require('path');
|
||||
const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs');
|
||||
|
||||
const SDK_CLI = path.join(__dirname, '..', 'sdk', 'dist', 'cli.js');
|
||||
const { execFileSync } = require('child_process');
|
||||
|
||||
function runSdkQuery(args, cwd) {
|
||||
try {
|
||||
const result = execFileSync(process.execPath, [SDK_CLI, 'query', ...args], {
|
||||
cwd,
|
||||
encoding: 'utf-8',
|
||||
stdio: ['pipe', 'pipe', 'pipe'],
|
||||
});
|
||||
const parsed = JSON.parse(result.trim());
|
||||
return { success: true, data: parsed };
|
||||
} catch (err) {
|
||||
const stderr = err.stderr?.toString().trim() || '';
|
||||
const stdout = err.stdout?.toString().trim() || '';
|
||||
// If the output is JSON despite non-zero exit, parse it
|
||||
try {
|
||||
const parsed = JSON.parse(stdout);
|
||||
return { success: true, data: parsed };
|
||||
} catch {
|
||||
/* not JSON */
|
||||
}
|
||||
return { success: false, error: stderr || err.message };
|
||||
}
|
||||
}
|
||||
|
||||
describe('bug #2684: milestone.complete forwards version to phases.archive', () => {
|
||||
let tmpDir;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = createTempProject();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
cleanup(tmpDir);
|
||||
});
|
||||
|
||||
test('milestone.complete v1.0 does not throw version required error', () => {
|
||||
// Minimal project: ROADMAP.md so milestone filter can run, one phase dir
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'ROADMAP.md'),
|
||||
`# Roadmap\n\n### Phase 1: Foundation\n**Goal:** Setup\n`
|
||||
);
|
||||
fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '01-foundation'), { recursive: true });
|
||||
|
||||
const result = runSdkQuery(['milestone.complete', 'v1.0'], tmpDir);
|
||||
|
||||
assert.ok(
|
||||
result.success,
|
||||
`milestone.complete should succeed, got error: ${result.error}`
|
||||
);
|
||||
assert.ok(
|
||||
!result.error || !result.error.includes('version required'),
|
||||
`should not throw "version required" — got: ${result.error}`
|
||||
);
|
||||
});
|
||||
|
||||
test('milestone.complete returns version in response data', () => {
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'ROADMAP.md'),
|
||||
`# Roadmap\n\n### Phase 1: Foundation\n**Goal:** Setup\n`
|
||||
);
|
||||
|
||||
const result = runSdkQuery(['milestone.complete', 'v2.5'], tmpDir);
|
||||
|
||||
assert.ok(result.success, `Command failed: ${result.error}`);
|
||||
assert.strictEqual(result.data.version, 'v2.5', 'version should be echoed in response');
|
||||
});
|
||||
|
||||
test('milestone.complete with --archive-phases forwards version correctly', () => {
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'ROADMAP.md'),
|
||||
`# Roadmap\n\n### Phase 1: Foundation\n**Goal:** Setup\n`
|
||||
);
|
||||
const phaseDir = path.join(tmpDir, '.planning', 'phases', '01-foundation');
|
||||
fs.mkdirSync(phaseDir, { recursive: true });
|
||||
fs.writeFileSync(path.join(phaseDir, '01-01-PLAN.md'), '# Plan');
|
||||
fs.writeFileSync(path.join(phaseDir, '01-01-SUMMARY.md'), '# Summary');
|
||||
|
||||
// With --archive-phases, the version must reach the archive logic
|
||||
// Without the fix this would throw "version required for phases archive"
|
||||
const result = runSdkQuery(['milestone.complete', 'v1.0', '--archive-phases'], tmpDir);
|
||||
|
||||
assert.ok(result.success, `milestone.complete --archive-phases failed: ${result.error}`);
|
||||
assert.strictEqual(result.data.version, 'v1.0');
|
||||
// The archive flag should have moved the phase dir
|
||||
assert.ok(
|
||||
result.data.archived.phases === true,
|
||||
'phases should be archived when --archive-phases is passed'
|
||||
);
|
||||
const archiveDir = path.join(tmpDir, '.planning', 'milestones', 'v1.0-phases');
|
||||
assert.ok(fs.existsSync(archiveDir), 'archive directory should exist');
|
||||
});
|
||||
|
||||
test('phases.archive v1.0 (direct call, workaround) also works', () => {
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'ROADMAP.md'),
|
||||
`# Roadmap\n\n### Phase 1: Foundation\n**Goal:** Setup\n`
|
||||
);
|
||||
fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '01-foundation'), { recursive: true });
|
||||
|
||||
const result = runSdkQuery(['phases.archive', 'v1.0'], tmpDir);
|
||||
|
||||
assert.ok(result.success, `phases.archive failed: ${result.error}`);
|
||||
assert.strictEqual(result.data.version, 'v1.0');
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user