mirror of
https://github.com/glittercowboy/get-shit-done
synced 2026-04-25 17:25:23 +02:00
fix(phase): scan ROADMAP.md entries in next-decimal to prevent collisions (#1877)
next-decimal and insert-phase only scanned directory names in .planning/phases/ when calculating the next available decimal number. When agents added backlog items by writing ROADMAP.md entries and creating directories without calling next-decimal, the function would not see those entries and return a number that was already in use. Both functions now union directory names AND ROADMAP.md phase headers (e.g. ### Phase 999.3: ...) before computing max + 1. This follows the same pattern already used by cmdPhaseComplete (lines 791-834) which scans ROADMAP.md as a fallback for phases defined but not yet scaffolded to disk. Additional hardening: - Use escapeRegex() on normalized phase names in regex construction - Support optional project-code prefix in directory pattern matching - Handle edge cases: missing ROADMAP.md, empty/missing phases dir, leading-zero padded phase numbers in ROADMAP.md Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
Tibsfox
@@ -88,50 +88,49 @@ function cmdPhaseNextDecimal(cwd, basePhase, raw) {
|
||||
const phasesDir = path.join(planningDir(cwd), 'phases');
|
||||
const normalized = normalizePhaseName(basePhase);
|
||||
|
||||
// Check if phases directory exists
|
||||
if (!fs.existsSync(phasesDir)) {
|
||||
output(
|
||||
{
|
||||
found: false,
|
||||
base_phase: normalized,
|
||||
next: `${normalized}.1`,
|
||||
existing: [],
|
||||
},
|
||||
raw,
|
||||
`${normalized}.1`
|
||||
);
|
||||
return;
|
||||
}
|
||||
|
||||
try {
|
||||
const entries = fs.readdirSync(phasesDir, { withFileTypes: true });
|
||||
const dirs = entries.filter(e => e.isDirectory()).map(e => e.name);
|
||||
let baseExists = false;
|
||||
const decimalSet = new Set();
|
||||
|
||||
// Check if base phase exists
|
||||
const baseExists = dirs.some(d => phaseTokenMatches(d, normalized));
|
||||
// Scan directory names for existing decimal phases
|
||||
if (fs.existsSync(phasesDir)) {
|
||||
const entries = fs.readdirSync(phasesDir, { withFileTypes: true });
|
||||
const dirs = entries.filter(e => e.isDirectory()).map(e => e.name);
|
||||
baseExists = dirs.some(d => phaseTokenMatches(d, normalized));
|
||||
|
||||
// Find existing decimal phases for this base
|
||||
const decimalPattern = new RegExp(`^${normalized}\\.(\\d+)`);
|
||||
const existingDecimals = [];
|
||||
|
||||
for (const dir of dirs) {
|
||||
const match = dir.match(decimalPattern);
|
||||
if (match) {
|
||||
existingDecimals.push(`${normalized}.${match[1]}`);
|
||||
const dirPattern = new RegExp(`^(?:[A-Z]{1,6}-)?${escapeRegex(normalized)}\\.(\\d+)`);
|
||||
for (const dir of dirs) {
|
||||
const match = dir.match(dirPattern);
|
||||
if (match) decimalSet.add(parseInt(match[1], 10));
|
||||
}
|
||||
}
|
||||
|
||||
// Sort numerically
|
||||
existingDecimals.sort((a, b) => comparePhaseNum(a, b));
|
||||
// Also scan ROADMAP.md for phase entries that may not have directories yet
|
||||
const roadmapPath = path.join(planningDir(cwd), 'ROADMAP.md');
|
||||
if (fs.existsSync(roadmapPath)) {
|
||||
try {
|
||||
const roadmapContent = fs.readFileSync(roadmapPath, 'utf-8');
|
||||
const phasePattern = new RegExp(
|
||||
`#{2,4}\\s*Phase\\s+0*${escapeRegex(normalized)}\\.(\\d+)\\s*:`, 'gi'
|
||||
);
|
||||
let pm;
|
||||
while ((pm = phasePattern.exec(roadmapContent)) !== null) {
|
||||
decimalSet.add(parseInt(pm[1], 10));
|
||||
}
|
||||
} catch { /* ROADMAP.md read failure is non-fatal */ }
|
||||
}
|
||||
|
||||
// Build sorted list of existing decimals
|
||||
const existingDecimals = Array.from(decimalSet)
|
||||
.sort((a, b) => a - b)
|
||||
.map(n => `${normalized}.${n}`);
|
||||
|
||||
// Calculate next decimal
|
||||
let nextDecimal;
|
||||
if (existingDecimals.length === 0) {
|
||||
if (decimalSet.size === 0) {
|
||||
nextDecimal = `${normalized}.1`;
|
||||
} else {
|
||||
const lastDecimal = existingDecimals[existingDecimals.length - 1];
|
||||
const lastNum = parseInt(lastDecimal.split('.')[1], 10);
|
||||
nextDecimal = `${normalized}.${lastNum + 1}`;
|
||||
nextDecimal = `${normalized}.${Math.max(...decimalSet) + 1}`;
|
||||
}
|
||||
|
||||
output(
|
||||
@@ -416,22 +415,31 @@ function cmdPhaseInsert(cwd, afterPhase, description, raw) {
|
||||
error(`Phase ${afterPhase} not found in ROADMAP.md`);
|
||||
}
|
||||
|
||||
// Calculate next decimal using existing logic
|
||||
// Calculate next decimal by scanning both directories AND ROADMAP.md entries
|
||||
const phasesDir = path.join(planningDir(cwd), 'phases');
|
||||
const normalizedBase = normalizePhaseName(afterPhase);
|
||||
let existingDecimals = [];
|
||||
const decimalSet = new Set();
|
||||
|
||||
try {
|
||||
const entries = fs.readdirSync(phasesDir, { withFileTypes: true });
|
||||
const dirs = entries.filter(e => e.isDirectory()).map(e => e.name);
|
||||
const decimalPattern = new RegExp(`^(?:[A-Z]{1,6}-)?${normalizedBase}\\.(\\d+)`);
|
||||
const decimalPattern = new RegExp(`^(?:[A-Z]{1,6}-)?${escapeRegex(normalizedBase)}\\.(\\d+)`);
|
||||
for (const dir of dirs) {
|
||||
const dm = dir.match(decimalPattern);
|
||||
if (dm) existingDecimals.push(parseInt(dm[1], 10));
|
||||
if (dm) decimalSet.add(parseInt(dm[1], 10));
|
||||
}
|
||||
} catch { /* intentionally empty */ }
|
||||
|
||||
const nextDecimal = existingDecimals.length === 0 ? 1 : Math.max(...existingDecimals) + 1;
|
||||
// Also scan ROADMAP.md content (already loaded) for decimal entries
|
||||
const rmPhasePattern = new RegExp(
|
||||
`#{2,4}\\s*Phase\\s+0*${escapeRegex(normalizedBase)}\\.(\\d+)\\s*:`, 'gi'
|
||||
);
|
||||
let rmMatch;
|
||||
while ((rmMatch = rmPhasePattern.exec(rawContent)) !== null) {
|
||||
decimalSet.add(parseInt(rmMatch[1], 10));
|
||||
}
|
||||
|
||||
const nextDecimal = decimalSet.size === 0 ? 1 : Math.max(...decimalSet) + 1;
|
||||
const _decimalPhase = `${normalizedBase}.${nextDecimal}`;
|
||||
// Optional project code prefix
|
||||
const insertConfig = loadConfig(cwd);
|
||||
|
||||
217
tests/next-decimal-roadmap-scan.test.cjs
Normal file
217
tests/next-decimal-roadmap-scan.test.cjs
Normal file
@@ -0,0 +1,217 @@
|
||||
/**
|
||||
* GSD Tools Tests — phase next-decimal ROADMAP.md scanning
|
||||
*
|
||||
* Covers issue #1865: next-decimal only scanned directory names in
|
||||
* .planning/phases/ to determine the next available decimal number.
|
||||
* It did not check ROADMAP.md entries. When agents add backlog items
|
||||
* by writing ROADMAP.md + creating dirs without calling next-decimal,
|
||||
* collisions occur.
|
||||
*
|
||||
* After the fix, next-decimal unions directory names AND ROADMAP.md
|
||||
* phase headers before computing the next available number.
|
||||
*/
|
||||
|
||||
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');
|
||||
|
||||
describe('phase next-decimal ROADMAP.md scanning (#1865)', () => {
|
||||
let tmpDir;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = createTempProject();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
cleanup(tmpDir);
|
||||
});
|
||||
|
||||
test('directory-only scan still works (existing behavior)', () => {
|
||||
// Create directory-based decimal phases
|
||||
fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '999-backlog'), { recursive: true });
|
||||
fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '999.1-idea-one'), { recursive: true });
|
||||
fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '999.2-idea-two'), { recursive: true });
|
||||
|
||||
const result = runGsdTools('phase next-decimal 999', tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error}`);
|
||||
|
||||
const output = JSON.parse(result.output);
|
||||
assert.strictEqual(output.next, '999.3');
|
||||
assert.deepStrictEqual(output.existing, ['999.1', '999.2']);
|
||||
});
|
||||
|
||||
test('detects ROADMAP.md entries that have no directory', () => {
|
||||
// Only ROADMAP.md has 999.1 and 999.2 — no directories exist
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'ROADMAP.md'),
|
||||
[
|
||||
'# Roadmap',
|
||||
'',
|
||||
'## v1.0',
|
||||
'',
|
||||
'### Phase 999: Backlog',
|
||||
'**Goal:** Parking lot',
|
||||
'',
|
||||
'### Phase 999.1: First idea',
|
||||
'**Goal:** Something',
|
||||
'',
|
||||
'### Phase 999.2: Second idea',
|
||||
'**Goal:** Something else',
|
||||
].join('\n')
|
||||
);
|
||||
|
||||
const result = runGsdTools('phase next-decimal 999', tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error}`);
|
||||
|
||||
const output = JSON.parse(result.output);
|
||||
assert.strictEqual(output.next, '999.3', 'should skip past ROADMAP.md entries');
|
||||
assert.deepStrictEqual(output.existing, ['999.1', '999.2']);
|
||||
});
|
||||
|
||||
test('unions directories and ROADMAP.md entries (no duplicates)', () => {
|
||||
// Directory has 999.1, ROADMAP.md has 999.1 and 999.3
|
||||
fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '999.1-idea-one'), { recursive: true });
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'ROADMAP.md'),
|
||||
[
|
||||
'# Roadmap',
|
||||
'',
|
||||
'### Phase 999.1: First idea',
|
||||
'**Goal:** Something',
|
||||
'',
|
||||
'### Phase 999.3: Third idea',
|
||||
'**Goal:** Something more',
|
||||
].join('\n')
|
||||
);
|
||||
|
||||
const result = runGsdTools('phase next-decimal 999', tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error}`);
|
||||
|
||||
const output = JSON.parse(result.output);
|
||||
assert.strictEqual(output.next, '999.4', 'should be max of union + 1');
|
||||
assert.deepStrictEqual(output.existing, ['999.1', '999.3']);
|
||||
});
|
||||
|
||||
test('ROADMAP.md with higher decimal wins over directories', () => {
|
||||
// Directories: 999.1, 999.2. ROADMAP.md: 999.1, 999.5
|
||||
fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '999.1-one'), { recursive: true });
|
||||
fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '999.2-two'), { recursive: true });
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'ROADMAP.md'),
|
||||
[
|
||||
'# Roadmap',
|
||||
'',
|
||||
'### Phase 999.1: One',
|
||||
'**Goal:** A',
|
||||
'',
|
||||
'### Phase 999.5: Five',
|
||||
'**Goal:** E',
|
||||
].join('\n')
|
||||
);
|
||||
|
||||
const result = runGsdTools('phase next-decimal 999', tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error}`);
|
||||
|
||||
const output = JSON.parse(result.output);
|
||||
assert.strictEqual(output.next, '999.6');
|
||||
assert.deepStrictEqual(output.existing, ['999.1', '999.2', '999.5']);
|
||||
});
|
||||
|
||||
test('handles missing ROADMAP.md gracefully', () => {
|
||||
// No ROADMAP.md, just directories
|
||||
fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '05.1-patch'), { recursive: true });
|
||||
|
||||
const result = runGsdTools('phase next-decimal 5', tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error}`);
|
||||
|
||||
const output = JSON.parse(result.output);
|
||||
assert.strictEqual(output.next, '05.2');
|
||||
});
|
||||
|
||||
test('handles empty phases dir with ROADMAP.md entries', () => {
|
||||
// Empty phases dir but ROADMAP.md has entries
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'ROADMAP.md'),
|
||||
[
|
||||
'# Roadmap',
|
||||
'',
|
||||
'### Phase 10.1: Widget',
|
||||
'**Goal:** Build widget',
|
||||
'',
|
||||
'### Phase 10.2: Gadget',
|
||||
'**Goal:** Build gadget',
|
||||
].join('\n')
|
||||
);
|
||||
|
||||
const result = runGsdTools('phase next-decimal 10', tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error}`);
|
||||
|
||||
const output = JSON.parse(result.output);
|
||||
assert.strictEqual(output.next, '10.3');
|
||||
assert.deepStrictEqual(output.existing, ['10.1', '10.2']);
|
||||
});
|
||||
|
||||
test('handles no phases dir and no ROADMAP.md', () => {
|
||||
// Remove the phases directory entirely
|
||||
fs.rmSync(path.join(tmpDir, '.planning', 'phases'), { recursive: true });
|
||||
|
||||
const result = runGsdTools('phase next-decimal 999', tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error}`);
|
||||
|
||||
const output = JSON.parse(result.output);
|
||||
assert.strictEqual(output.next, '999.1');
|
||||
assert.deepStrictEqual(output.existing, []);
|
||||
});
|
||||
|
||||
test('does not match unrelated phase numbers in ROADMAP.md', () => {
|
||||
// ROADMAP.md has 99.1 and 9.1 — neither should match when querying 999
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'ROADMAP.md'),
|
||||
[
|
||||
'# Roadmap',
|
||||
'',
|
||||
'### Phase 99.1: Close but not 999',
|
||||
'**Goal:** Unrelated',
|
||||
'',
|
||||
'### Phase 9.1: Also unrelated',
|
||||
'**Goal:** Nope',
|
||||
'',
|
||||
'### Phase 9990.1: Prefix mismatch',
|
||||
'**Goal:** Should not match 999',
|
||||
].join('\n')
|
||||
);
|
||||
|
||||
const result = runGsdTools('phase next-decimal 999', tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error}`);
|
||||
|
||||
const output = JSON.parse(result.output);
|
||||
assert.strictEqual(output.next, '999.1', 'should not be confused by unrelated phases');
|
||||
assert.deepStrictEqual(output.existing, []);
|
||||
});
|
||||
|
||||
test('handles leading-zero phase numbers in ROADMAP.md', () => {
|
||||
// ROADMAP.md uses zero-padded phase numbers
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'ROADMAP.md'),
|
||||
[
|
||||
'# Roadmap',
|
||||
'',
|
||||
'### Phase 007.1: Secret agent fix',
|
||||
'**Goal:** Classified',
|
||||
'',
|
||||
'### Phase 007.2: Another fix',
|
||||
'**Goal:** Also classified',
|
||||
].join('\n')
|
||||
);
|
||||
|
||||
const result = runGsdTools('phase next-decimal 7', tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error}`);
|
||||
|
||||
const output = JSON.parse(result.output);
|
||||
// normalizePhaseName('7') pads to '07'
|
||||
assert.strictEqual(output.next, '07.3');
|
||||
assert.deepStrictEqual(output.existing, ['07.1', '07.2']);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user