fix(milestone): deduplicate phase filter and handle empty MILESTONES.md
Two hardening changes to cmdMilestoneComplete: 1. Replace 27 lines of inline isDirInMilestone logic (roadmap parsing, normalization, and matching) with a single call to the shared getMilestonePhaseFilter(cwd) from core.cjs. The inline copy was identical to the core version — deduplicating prevents future drift. 2. Handle empty MILESTONES.md files. Previously, an existing but empty file would fall into the headerMatch branch and produce malformed output. Now an empty file is treated the same as a missing one, writing the standard "# Milestones" header before the entry. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Tibsfox
committed by
Lex Christopherson
Lex Christopherson
parent
15226feb18
commit
8b8d1074b8
@@ -638,9 +638,31 @@ function mergeCodexConfig(configPath, gsdBlock) {
|
||||
|
||||
// Case 2: Has GSD marker — truncate and re-append
|
||||
if (markerIndex !== -1) {
|
||||
const before = existing.substring(0, markerIndex).trimEnd();
|
||||
const newContent = before ? before + '\n\n' + gsdBlock + '\n' : gsdBlock + '\n';
|
||||
fs.writeFileSync(configPath, newContent);
|
||||
let before = existing.substring(0, markerIndex).trimEnd();
|
||||
if (before) {
|
||||
// Strip any GSD-managed sections that leaked above the marker from previous installs
|
||||
before = before.replace(/^\[agents\.gsd-[^\]]+\]\n(?:(?!\[)[^\n]*\n?)*/gm, '');
|
||||
before = before.replace(/^\[agents\]\n(?:(?!\[)[^\n]*\n?)*/m, '');
|
||||
before = before.replace(/\n{3,}/g, '\n\n').trimEnd();
|
||||
|
||||
// Re-inject feature keys if user has [features] above the marker
|
||||
const hasFeatures = /^\[features\]\s*$/m.test(before);
|
||||
if (hasFeatures) {
|
||||
if (!before.includes('multi_agent')) {
|
||||
before = before.replace(/^\[features\]\s*$/m, '[features]\nmulti_agent = true');
|
||||
}
|
||||
if (!before.includes('default_mode_request_user_input')) {
|
||||
before = before.replace(/^\[features\].*$/m, '$&\ndefault_mode_request_user_input = true');
|
||||
}
|
||||
}
|
||||
// Skip [features] from gsdBlock if user already has it
|
||||
const block = hasFeatures
|
||||
? GSD_CODEX_MARKER + '\n' + gsdBlock.substring(gsdBlock.indexOf('[agents]'))
|
||||
: gsdBlock;
|
||||
fs.writeFileSync(configPath, before + '\n\n' + block + '\n');
|
||||
} else {
|
||||
fs.writeFileSync(configPath, gsdBlock + '\n');
|
||||
}
|
||||
return;
|
||||
}
|
||||
|
||||
|
||||
@@ -4,7 +4,7 @@
|
||||
|
||||
const fs = require('fs');
|
||||
const path = require('path');
|
||||
const { escapeRegex, output, error } = require('./core.cjs');
|
||||
const { escapeRegex, getMilestonePhaseFilter, output, error } = require('./core.cjs');
|
||||
const { extractFrontmatter } = require('./frontmatter.cjs');
|
||||
const { writeStateMd } = require('./state.cjs');
|
||||
|
||||
@@ -93,42 +93,10 @@ function cmdMilestoneComplete(cwd, version, options, raw) {
|
||||
// Ensure archive directory exists
|
||||
fs.mkdirSync(archiveDir, { recursive: true });
|
||||
|
||||
// Extract milestone phase numbers from ROADMAP.md to scope stats.
|
||||
// Only phases listed in the current ROADMAP are counted — phases from
|
||||
// prior milestones that remain on disk are excluded.
|
||||
//
|
||||
// Related upstream PRs (getMilestoneInfo, not milestone complete):
|
||||
// #756 — fix(core): detect current milestone correctly in getMilestoneInfo
|
||||
// #783 — fix: getMilestoneInfo() returns wrong version after completion
|
||||
// Those PRs fix *which* milestone is detected; this fix scopes *stats*
|
||||
// and *accomplishments* to only the phases belonging to that milestone.
|
||||
const milestonePhaseNums = new Set();
|
||||
if (fs.existsSync(roadmapPath)) {
|
||||
try {
|
||||
const roadmapContent = fs.readFileSync(roadmapPath, 'utf-8');
|
||||
const phasePattern = /#{2,4}\s*Phase\s+(\d+[A-Z]?(?:\.\d+)*)\s*:/gi;
|
||||
let phaseMatch;
|
||||
while ((phaseMatch = phasePattern.exec(roadmapContent)) !== null) {
|
||||
milestonePhaseNums.add(phaseMatch[1]);
|
||||
}
|
||||
} catch {}
|
||||
}
|
||||
|
||||
// Pre-normalize phase numbers for O(1) lookup — strip leading zeros
|
||||
// and lowercase for case-insensitive matching of letter suffixes (e.g. 3A/3a).
|
||||
const normalizedPhaseNums = new Set(
|
||||
[...milestonePhaseNums].map(num => (num.replace(/^0+/, '') || '0').toLowerCase())
|
||||
);
|
||||
|
||||
// Match a phase directory name to the milestone's phase set.
|
||||
// Handles: "01-foo" → "1", "3A-bar" → "3a", "3.1-baz" → "3.1"
|
||||
// Returns false for non-phase directories (no leading digit).
|
||||
function isDirInMilestone(dirName) {
|
||||
if (normalizedPhaseNums.size === 0) return true; // no scoping
|
||||
const m = dirName.match(/^0*(\d+[A-Za-z]?(?:\.\d+)*)/);
|
||||
if (!m) return false; // not a phase directory
|
||||
return normalizedPhaseNums.has(m[1].toLowerCase());
|
||||
}
|
||||
// Scope stats and accomplishments to only the phases belonging to the
|
||||
// current milestone's ROADMAP. Uses the shared filter from core.cjs
|
||||
// (same logic used by cmdPhasesList and other callers).
|
||||
const isDirInMilestone = getMilestonePhaseFilter(cwd);
|
||||
|
||||
// Gather stats from phases (scoped to current milestone only)
|
||||
let phaseCount = 0;
|
||||
@@ -190,15 +158,20 @@ function cmdMilestoneComplete(cwd, version, options, raw) {
|
||||
|
||||
if (fs.existsSync(milestonesPath)) {
|
||||
const existing = fs.readFileSync(milestonesPath, 'utf-8');
|
||||
// Insert after the header line(s) for reverse chronological order (newest first)
|
||||
const headerMatch = existing.match(/^(#{1,3}\s+[^\n]*\n\n?)/);
|
||||
if (headerMatch) {
|
||||
const header = headerMatch[1];
|
||||
const rest = existing.slice(header.length);
|
||||
fs.writeFileSync(milestonesPath, header + milestoneEntry + rest, 'utf-8');
|
||||
if (!existing.trim()) {
|
||||
// Empty file — treat like new
|
||||
fs.writeFileSync(milestonesPath, `# Milestones\n\n${milestoneEntry}`, 'utf-8');
|
||||
} else {
|
||||
// No recognizable header — prepend the entry
|
||||
fs.writeFileSync(milestonesPath, milestoneEntry + existing, 'utf-8');
|
||||
// Insert after the header line(s) for reverse chronological order (newest first)
|
||||
const headerMatch = existing.match(/^(#{1,3}\s+[^\n]*\n\n?)/);
|
||||
if (headerMatch) {
|
||||
const header = headerMatch[1];
|
||||
const rest = existing.slice(header.length);
|
||||
fs.writeFileSync(milestonesPath, header + milestoneEntry + rest, 'utf-8');
|
||||
} else {
|
||||
// No recognizable header — prepend the entry
|
||||
fs.writeFileSync(milestonesPath, milestoneEntry + existing, 'utf-8');
|
||||
}
|
||||
}
|
||||
} else {
|
||||
fs.writeFileSync(milestonesPath, `# Milestones\n\n${milestoneEntry}`, 'utf-8');
|
||||
|
||||
@@ -365,6 +365,84 @@ describe('mergeCodexConfig', () => {
|
||||
|
||||
assert.strictEqual(first, second, 'idempotent merge');
|
||||
});
|
||||
|
||||
test('case 2 after case 3 with existing [features]: no duplicate sections', () => {
|
||||
const configPath = path.join(tmpDir, 'config.toml');
|
||||
fs.writeFileSync(configPath, '[features]\nother_feature = true\n\n[model]\nname = "o3"\n');
|
||||
mergeCodexConfig(configPath, sampleBlock);
|
||||
|
||||
mergeCodexConfig(configPath, sampleBlock);
|
||||
|
||||
const content = fs.readFileSync(configPath, 'utf8');
|
||||
const featuresCount = (content.match(/^\[features\]\s*$/gm) || []).length;
|
||||
const agentsCount = (content.match(/^\[agents\]\s*$/gm) || []).length;
|
||||
assert.strictEqual(featuresCount, 1, 'exactly one [features] section');
|
||||
assert.strictEqual(agentsCount, 1, 'exactly one [agents] section');
|
||||
assert.ok(content.includes('other_feature = true'), 'preserves user feature keys');
|
||||
assert.ok(content.includes('multi_agent = true'), 'has GSD feature key');
|
||||
assert.ok(content.includes('[agents.gsd-executor]'), 'has agent');
|
||||
});
|
||||
|
||||
test('case 2 re-injects missing feature keys', () => {
|
||||
const configPath = path.join(tmpDir, 'config.toml');
|
||||
const manualContent = '[features]\nother_feature = true\n\n' + GSD_CODEX_MARKER + '\n[agents]\nmax_threads = 4\n';
|
||||
fs.writeFileSync(configPath, manualContent);
|
||||
|
||||
mergeCodexConfig(configPath, sampleBlock);
|
||||
|
||||
const content = fs.readFileSync(configPath, 'utf8');
|
||||
assert.ok(content.includes('multi_agent = true'), 're-injects multi_agent');
|
||||
assert.ok(content.includes('default_mode_request_user_input = true'), 're-injects request_user_input');
|
||||
assert.ok(content.includes('other_feature = true'), 'preserves user feature');
|
||||
});
|
||||
|
||||
test('case 2 strips leaked [agents] from before content', () => {
|
||||
const configPath = path.join(tmpDir, 'config.toml');
|
||||
const brokenContent = [
|
||||
'[features]',
|
||||
'default_mode_request_user_input = true',
|
||||
'multi_agent = true',
|
||||
'child_agents_md = false',
|
||||
'',
|
||||
'[agents]',
|
||||
'max_threads = 4',
|
||||
'max_depth = 2',
|
||||
'',
|
||||
'[agents.gsd-executor]',
|
||||
'description = "old"',
|
||||
'config_file = "agents/gsd-executor.toml"',
|
||||
'',
|
||||
GSD_CODEX_MARKER,
|
||||
'[agents]',
|
||||
'max_threads = 4',
|
||||
'',
|
||||
].join('\n');
|
||||
fs.writeFileSync(configPath, brokenContent);
|
||||
|
||||
mergeCodexConfig(configPath, sampleBlock);
|
||||
|
||||
const content = fs.readFileSync(configPath, 'utf8');
|
||||
const agentsCount = (content.match(/^\[agents\]\s*$/gm) || []).length;
|
||||
assert.strictEqual(agentsCount, 1, 'exactly one [agents] section');
|
||||
assert.ok(content.includes('child_agents_md = false'), 'preserves user feature keys');
|
||||
assert.ok(content.includes('[agents.gsd-executor]'), 'has agent from fresh block');
|
||||
});
|
||||
|
||||
test('case 2 idempotent after case 3 with existing [features]', () => {
|
||||
const configPath = path.join(tmpDir, 'config.toml');
|
||||
fs.writeFileSync(configPath, '[features]\nother_feature = true\n');
|
||||
mergeCodexConfig(configPath, sampleBlock);
|
||||
const first = fs.readFileSync(configPath, 'utf8');
|
||||
|
||||
mergeCodexConfig(configPath, sampleBlock);
|
||||
const second = fs.readFileSync(configPath, 'utf8');
|
||||
|
||||
mergeCodexConfig(configPath, sampleBlock);
|
||||
const third = fs.readFileSync(configPath, 'utf8');
|
||||
|
||||
assert.strictEqual(first, second, 'idempotent after 2nd merge');
|
||||
assert.strictEqual(second, third, 'idempotent after 3rd merge');
|
||||
});
|
||||
});
|
||||
|
||||
// ─── Integration: installCodexConfig ────────────────────────────────────────────
|
||||
|
||||
Reference in New Issue
Block a user