From 8b8d1074b80e660875359b6f07c94346f46d8716 Mon Sep 17 00:00:00 2001 From: Tibsfox Date: Tue, 3 Mar 2026 01:48:47 -0800 Subject: [PATCH] fix(milestone): deduplicate phase filter and handle empty MILESTONES.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- bin/install.js | 28 +++++++++-- get-shit-done/bin/lib/milestone.cjs | 63 +++++++---------------- tests/codex-config.test.cjs | 78 +++++++++++++++++++++++++++++ 3 files changed, 121 insertions(+), 48 deletions(-) diff --git a/bin/install.js b/bin/install.js index aa62fd27c..962ea2eb3 100755 --- a/bin/install.js +++ b/bin/install.js @@ -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; } diff --git a/get-shit-done/bin/lib/milestone.cjs b/get-shit-done/bin/lib/milestone.cjs index 9021d2bea..71dc918de 100644 --- a/get-shit-done/bin/lib/milestone.cjs +++ b/get-shit-done/bin/lib/milestone.cjs @@ -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'); diff --git a/tests/codex-config.test.cjs b/tests/codex-config.test.cjs index e9cf8ad32..758eedc84 100644 --- a/tests/codex-config.test.cjs +++ b/tests/codex-config.test.cjs @@ -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 ────────────────────────────────────────────