From 6299b9181fa771874577d4218fd0b3eda8578891 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 8 May 2026 08:40:47 -0400 Subject: [PATCH] fix(state): preserve curated progress on body-only updates; correct percent formula (#3242) (#3252) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test: reproduce state.update progress trampling and percent formula (#3242) Two failing regression tests: - Bug A: state.update "Last Activity" tramples curated progress.* frontmatter via readModifyWriteStateMd → syncStateFrontmatter - Bug B: 12 declared ROADMAP phases / 6 realized / 6/6 plans done → percent: 100 instead of 50 (phase-fraction ignored) * fix(state): preserve curated progress on body-only updates; correct percent formula (#3242) Bug A: readModifyWriteStateMd now accepts { resync: false } to preserve existing frontmatter progress.* when only body text changes. cmdStateUpdate passes this flag since it only replaces a body field and must not trample manually-curated cross-milestone counters. Bug B: extract computeProgressPercent() helper — shared by buildStateFrontmatter and cmdStateSync — that applies min(plan_fraction, phase_fraction). When ROADMAP declares more phases than are realized on disk, phase_fraction caps percent so 22/22 plans done with only 6/12 phases gives 50%, not a false 100%. * changeset: pr=3252 for #3242 * fix(test): replace content.includes with structured state json assertion (#3242) --- .changeset/gentle-bears-wave.md | 5 + get-shit-done/bin/lib/state.cjs | 101 ++++- ...242-state-update-progress-trample.test.cjs | 378 ++++++++++++++++++ 3 files changed, 472 insertions(+), 12 deletions(-) create mode 100644 .changeset/gentle-bears-wave.md create mode 100644 tests/bug-3242-state-update-progress-trample.test.cjs diff --git a/.changeset/gentle-bears-wave.md b/.changeset/gentle-bears-wave.md new file mode 100644 index 000000000..5e957585c --- /dev/null +++ b/.changeset/gentle-bears-wave.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3252 +--- +**`state.update ` no longer rebuilds the progress.* block from disk on body-only updates** — manually-curated cross-milestone counters are preserved. Also: progress.percent now reflects the lower of plan-fraction and phase-fraction so milestones with un-planned future phases don't show false 100%. diff --git a/get-shit-done/bin/lib/state.cjs b/get-shit-done/bin/lib/state.cjs index 6f570eed6..7a1e92a61 100644 --- a/get-shit-done/bin/lib/state.cjs +++ b/get-shit-done/bin/lib/state.cjs @@ -13,6 +13,32 @@ const { extractFrontmatter, reconstructFrontmatter } = require('./frontmatter.cj // hasn't changed within the same gsd-tools invocation. const _diskScanCache = new Map(); +/** + * Compute the canonical progress percent for STATE.md frontmatter and body. + * + * Uses min(plan_fraction, phase_fraction) when both denominators are > 0. + * This prevents a false "100%" when ROADMAP declares future phases that have no + * disk dirs yet — all plans summarised only means 100% of *realized* work done, + * not 100% of the declared milestone (#3242 Bug B). + * + * @param {number|null} completedPlans + * @param {number|null} totalPlans + * @param {number|null} completedPhases + * @param {number|null} totalPhases - ROADMAP-declared count (>= realised dirs) + * @returns {number|null} 0–100, or null when there is no data + */ +function computeProgressPercent(completedPlans, totalPlans, completedPhases, totalPhases) { + const hasPlanData = totalPlans !== null && totalPlans > 0 && completedPlans !== null; + const hasPhaseData = totalPhases !== null && totalPhases > 0 && completedPhases !== null; + + if (!hasPlanData && !hasPhaseData) return null; + + const planFraction = hasPlanData ? completedPlans / totalPlans : 1; + const phaseFraction = hasPhaseData ? completedPhases / totalPhases : 1; + + return Math.min(100, Math.round(Math.min(planFraction, phaseFraction) * 100)); +} + /** Shorthand — every state command needs this path */ function getStatePath(cwd) { return planningPaths(cwd).state; @@ -202,6 +228,9 @@ function cmdStateUpdate(cwd, field, value) { const statePath = planningPaths(cwd).state; try { let updated = false; + // resync: false — cmdStateUpdate only replaces a body text line. + // Triggering syncStateFrontmatter would rebuild progress.* from disk, trampling + // manually-curated cross-milestone counters stored in the frontmatter (#3242 Bug A). readModifyWriteStateMd(statePath, (content) => { const fieldEscaped = escapeRegex(field); // Try **Field:** bold format first, then plain Field: format @@ -215,7 +244,7 @@ function cmdStateUpdate(cwd, field, value) { return content.replace(plainPattern, (_match, prefix) => `${prefix}${value}`); } return content; - }, cwd); + }, cwd, { resync: false }); if (updated) { output({ updated: true }); } else { @@ -813,13 +842,12 @@ function buildStateFrontmatter(bodyContent, cwd) { } // Derive percent from disk counts when available (ground truth). - // Only falls back to the body Progress: field when no plan files exist on disk - // (phases directory empty or absent), which means disk has no authoritative data. - // This prevents a stale body "0%" from overriding the real 100% completion state. - let progressPercent = null; - if (totalPlans !== null && totalPlans > 0 && completedPlans !== null) { - progressPercent = Math.min(100, Math.round(completedPlans / totalPlans * 100)); - } else if (progressRaw) { + // Uses min(plan_fraction, phase_fraction) via computeProgressPercent so that + // ROADMAP-declared-but-unrealized future phases cap the reported completion + // instead of a false 100% from plan-only coverage (#3242 Bug B). + // Falls back to the body Progress: field only when no plan files exist on disk. + let progressPercent = computeProgressPercent(completedPlans, totalPlans, completedPhases, totalPhases); + if (progressPercent === null && progressRaw) { const pctMatch = progressRaw.match(/(\d+)%/); if (pctMatch) progressPercent = parseInt(pctMatch[1], 10); } @@ -971,13 +999,43 @@ function writeStateMd(statePath, content, cwd) { * Holds the lock across the entire read -> transform -> write cycle, * preventing the lost-update problem where two agents read the same * content and the second write clobbers the first. + * + * @param {string} statePath + * @param {function} transformFn - (content: string) => string + * @param {string} cwd + * @param {{ resync?: boolean }} [options] + * resync: when true (default) rebuilds the entire frontmatter from disk after + * the transform. Pass { resync: false } for body-only updates (e.g. state.update + * on a single field) that must not trample manually-curated cross-milestone + * progress.* counters in the frontmatter (#3242 Bug A). + * When resync is false, syncStateFrontmatter still runs to maintain/create the + * frontmatter block, but any existing progress.* sub-keys are preserved from + * the pre-transform file rather than being rebuilt from disk. */ -function readModifyWriteStateMd(statePath, transformFn, cwd) { +function readModifyWriteStateMd(statePath, transformFn, cwd, options) { + const resync = !options || options.resync !== false; const lockPath = acquireStateLock(statePath); try { const content = fs.existsSync(statePath) ? fs.readFileSync(statePath, 'utf-8') : ''; + // Snapshot the existing progress block BEFORE the transform so we can + // restore it when resync is false. + const preFm = resync ? null : extractFrontmatter(content); const modified = transformFn(content); - const synced = syncStateFrontmatter(modified, cwd); + let synced = syncStateFrontmatter(modified, cwd); + + if (!resync && preFm && preFm.progress) { + // Re-apply the curated progress block that syncStateFrontmatter just + // overwrote with disk-derived values. Only restore keys that were present + // in the snapshot — this preserves any new non-progress frontmatter fields + // (e.g., status, current_phase) that syncStateFrontmatter legitimately + // derived from the updated body. + const postFm = extractFrontmatter(synced); + postFm.progress = preFm.progress; + const yamlStr = reconstructFrontmatter(postFm); + const body = stripFrontmatter(synced); + synced = `---\n${yamlStr}\n---\n\n${body}`; + } + atomicWriteFileSync(statePath, normalizeMd(synced), 'utf-8'); } finally { releaseStateLock(lockPath); @@ -1449,6 +1507,7 @@ function cmdStateSync(cwd, options, raw) { let totalDiskPlans = 0; let totalDiskSummaries = 0; + let diskCompletedPhases = 0; let highestIncompletePhase = null; let highestIncompletePhaseNum = null; let highestIncompletePhaseplanCount = 0; @@ -1461,6 +1520,7 @@ function cmdStateSync(cwd, options, raw) { const summaries = files.filter(f => f.match(/-SUMMARY\.md$/i)).length; totalDiskPlans += plans; totalDiskSummaries += summaries; + if (plans > 0 && summaries >= plans) diskCompletedPhases++; // Track the highest phase with incomplete plans (or any plans) const phaseMatch = dir.match(/^(\d+[A-Z]?(?:\.\d+)*)/i); @@ -1481,6 +1541,18 @@ function cmdStateSync(cwd, options, raw) { } } + // Determine total phases from ROADMAP (may be larger than realized disk dirs). + // Mirrors the logic in buildStateFrontmatter so both report consistent percents (#3242 Bug B). + let syncTotalPhases = null; + try { + const isDirInMilestone = getMilestonePhaseFilter(cwd); + if (isDirInMilestone.phaseCount > 0) { + syncTotalPhases = Math.max(entries.length, isDirInMilestone.phaseCount); + } else { + syncTotalPhases = entries.length; + } + } catch { /* intentionally empty */ } + // Sync Total Plans in Phase if (highestIncompletePhase) { const currentPlansField = stateExtractField(modified, 'Total Plans in Phase'); @@ -1491,8 +1563,13 @@ function cmdStateSync(cwd, options, raw) { } } - // Sync Progress - const percent = totalDiskPlans > 0 ? Math.min(100, Math.round(totalDiskSummaries / totalDiskPlans * 100)) : 0; + // Sync Progress — use shared helper so formula stays in one place (#3242 Bug B). + // computeProgressPercent applies min(plan_fraction, phase_fraction) so unrealised + // ROADMAP phases cap the reported percent rather than allowing a false 100%. + const percent = (() => { + const p = computeProgressPercent(totalDiskSummaries, totalDiskPlans, diskCompletedPhases, syncTotalPhases); + return p !== null ? p : 0; + })(); const currentProgress = stateExtractField(modified, 'Progress'); if (currentProgress) { const currentPercent = parseInt(currentProgress.replace(/[^\d]/g, ''), 10); diff --git a/tests/bug-3242-state-update-progress-trample.test.cjs b/tests/bug-3242-state-update-progress-trample.test.cjs new file mode 100644 index 000000000..5488c908f --- /dev/null +++ b/tests/bug-3242-state-update-progress-trample.test.cjs @@ -0,0 +1,378 @@ +'use strict'; +// Regression tests for issue #3242 — two distinct bugs in state.cjs: +// +// Bug A: cmdStateUpdate("Last Activity", date) triggers a full disk-derived +// progress.* block rebuild via readModifyWriteStateMd → syncStateFrontmatter → +// buildStateFrontmatter, which tramples manually-curated cross-milestone counters +// in STATE.md frontmatter. A body-only field update must not modify progress.*. +// +// Bug B: buildStateFrontmatter (and the duplicate in cmdStateSync) derives +// progress.percent = completedPlans / totalPlans. When ROADMAP declares more +// phases than have dirs on disk, all plans being summarised gives percent: 100 +// even though half the phases are unrealised. The formula must be +// min(plan_fraction, phase_fraction) to reflect true completion. + +const { describe, test, beforeEach, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs'); + +// ───────────────────────────────────────────────────────────────────────────── +// Helpers +// ───────────────────────────────────────────────────────────────────────────── + +/** + * Parse the STATE.md frontmatter `progress:` block into a numeric map. + * Returns { total_phases, completed_phases, total_plans, completed_plans, percent } + * sourced directly from the persisted frontmatter YAML (not rebuilt from disk). + * + * Only reads the --- block; throws if no closed --- block is found. + * Uses structural parsing (line splitting) rather than regex on the whole file. + */ +function parsePersistedProgress(content) { + const lines = content.split('\n'); + let inFm = false; + let inProgress = false; + const result = {}; + let fmStarted = false; + + for (const line of lines) { + if (!fmStarted) { + if (line.trim() === '---') { inFm = true; fmStarted = true; } + continue; + } + if (line.trim() === '---') break; // end of frontmatter + if (line === 'progress:') { inProgress = true; continue; } + if (inProgress) { + const m = line.match(/^\s{2}([a-z_]+):\s*(\d+)/); + if (m) { + result[m[1]] = parseInt(m[2], 10); + } else if (line.trim() !== '' && !/^\s/.test(line)) { + inProgress = false; // left progress block + } + } + } + return result; +} + +/** + * Build a minimal STATE.md body with frontmatter that has curated progress.*. + * The progress values are cross-milestone aggregates that must NOT be overwritten + * by a body-only field update. + */ +function buildStateWithCuratedProgress(opts) { + const { + completedPlans = 22, + totalPlans = 22, + completedPhases = 6, + totalPhases = 12, + percent = 50, + lastActivity = '2026-01-01', + } = opts || {}; + + return [ + '---', + 'gsd_state_version: 1.0', + 'status: executing', + 'progress:', + ` total_phases: ${totalPhases}`, + ` completed_phases: ${completedPhases}`, + ` total_plans: ${totalPlans}`, + ` completed_plans: ${completedPlans}`, + ` percent: ${percent}`, + '---', + '', + '# GSD State', + '', + '## Configuration', + 'Current Phase: 6', + 'Current Phase Name: test-phase', + 'Total Plans in Phase: 4', + 'Current Plan: 1', + 'Status: Executing Phase 6', + `Last Activity: ${lastActivity}`, + '', + ].join('\n'); +} + +/** + * Write a ROADMAP.md with `numPhases` phase headings (matching `## Phase N:` pattern). + * Only `numRealizedDirs` phase dirs will have plan/summary files on disk. + */ +function buildRoadmap(numPhases) { + const lines = ['# ROADMAP', '', '## Milestone v1.0', '']; + for (let i = 1; i <= numPhases; i++) { + lines.push(`### Phase ${i}: phase-${i}`); + lines.push(''); + } + return lines.join('\n'); +} + +/** + * Create phase dirs with full plan+summary coverage for the first `count` phases. + * Each dir gets 1 PLAN + 1 SUMMARY so the disk-scan treats them as complete. + */ +function createPhaseDirs(phasesDir, count) { + for (let i = 1; i <= count; i++) { + const dir = path.join(phasesDir, String(i).padStart(2, '0')); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, `01-PLAN.md`), `# Plan\n`); + fs.writeFileSync(path.join(dir, `01-SUMMARY.md`), `# Summary\n`); + } +} + +// ───────────────────────────────────────────────────────────────────────────── +// Bug A: state.update must not trample curated progress.* frontmatter +// ───────────────────────────────────────────────────────────────────────────── + +describe('#3242 Bug A: body-only state.update preserves curated progress frontmatter', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('state.update "Last Activity" does not overwrite progress.completed_plans', () => { + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, buildStateWithCuratedProgress({ + completedPlans: 22, + totalPlans: 22, + completedPhases: 6, + totalPhases: 12, + percent: 50, + lastActivity: '2026-01-01', + })); + + // Write 6 phase dirs with full coverage — disk would report 6/6 phases done, + // 6/6 plans done (percent=100 from plans-only formula), but frontmatter says 50%. + const phasesDir = path.join(tmpDir, '.planning', 'phases'); + createPhaseDirs(phasesDir, 6); + + const updateResult = runGsdTools( + ['state', 'update', 'Last Activity', '2026-05-07'], + tmpDir, + ); + assert.ok(updateResult.success, `state update failed: ${updateResult.error}`); + + // Read back the STATE.md file and inspect the persisted frontmatter directly. + // Note: state json always rebuilds from disk (correct for freshness), so we + // must check the file on disk to assert that state.update did not trample + // the curated frontmatter values (#3242 Bug A). + const content = fs.readFileSync(statePath, 'utf-8'); + const progress = parsePersistedProgress(content); + + assert.ok( + Object.keys(progress).length > 0, + 'STATE.md frontmatter must contain a progress: block after state.update', + ); + + // completed_plans must NOT have been trampled to 6 (disk reality) from the + // curated 22 that was stored in the frontmatter before the update. + assert.strictEqual( + progress.completed_plans, + 22, + `state.update "Last Activity" must not overwrite curated progress.completed_plans ` + + `(was 22, got ${progress.completed_plans})`, + ); + + // total_phases must NOT have been trampled to 6 (disk dirs) from curated 12. + assert.strictEqual( + progress.total_phases, + 12, + `state.update "Last Activity" must not overwrite curated progress.total_phases ` + + `(was 12, got ${progress.total_phases})`, + ); + + // percent must NOT have been trampled to 100 (plan-only formula on 6 realized dirs). + assert.strictEqual( + progress.percent, + 50, + `state.update "Last Activity" must not overwrite curated progress.percent ` + + `(was 50, got ${progress.percent})`, + ); + }); + + test('state.update "Last Activity" updates the body field itself', () => { + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, buildStateWithCuratedProgress({ lastActivity: '2026-01-01' })); + + const updateResult = runGsdTools( + ['state', 'update', 'Last Activity', '2026-05-07'], + tmpDir, + ); + assert.ok(updateResult.success, `state update failed: ${updateResult.error}`); + + // Confirm the body field was indeed updated via state json (structured output). + // state json reads last_activity from the body — if the field wasn't updated + // this will return the original '2026-01-01' value. + const jsonResult = runGsdTools('state json', tmpDir); + assert.ok(jsonResult.success, `state json failed: ${jsonResult.error}`); + const fm = JSON.parse(jsonResult.output); + assert.strictEqual( + fm.last_activity, + '2026-05-07', + `state.update should have written '2026-05-07' to the Last Activity body field`, + ); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// Bug B: progress.percent must use min(plan_fraction, phase_fraction) +// ───────────────────────────────────────────────────────────────────────────── + +describe('#3242 Bug B: progress.percent reflects phase fraction when ROADMAP declares future phases', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('12 declared phases / 6 realized / 6/6 plans done → percent is 50, not 100', () => { + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + + // Body: 6 realized phases visible to disk scan. + // Frontmatter: intentionally absent so buildStateFrontmatter runs fresh. + fs.writeFileSync(statePath, [ + '# GSD State', + '', + '## Configuration', + 'Current Phase: 6', + 'Current Phase Name: test-phase-6', + 'Total Plans in Phase: 1', + 'Current Plan: 1', + 'Status: Executing Phase 6', + 'Last Activity: 2026-01-01', + '', + ].join('\n')); + + // ROADMAP with 12 phase headings — only 6 will have dirs on disk + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + buildRoadmap(12), + ); + + // 6 fully-realized phases (all plans have summaries) + const phasesDir = path.join(tmpDir, '.planning', 'phases'); + createPhaseDirs(phasesDir, 6); + + // state json rebuilds frontmatter from disk+body — this exercises buildStateFrontmatter + const jsonResult = runGsdTools('state json', tmpDir); + assert.ok(jsonResult.success, `state json failed: ${jsonResult.error}`); + + const fm = JSON.parse(jsonResult.output); + assert.ok(fm.progress, 'frontmatter must have a progress block'); + + // ROADMAP declares 12 phases; only 6 exist on disk → totalPhases = 12 + assert.strictEqual( + fm.progress.total_phases, + 12, + `total_phases must reflect ROADMAP-declared count (12), got ${fm.progress.total_phases}`, + ); + + // 6 of 12 phases realized → phase_fraction = 50% + // 6/6 plans done → plan_fraction = 100% + // percent = min(100, 50) = 50 + assert.strictEqual( + fm.progress.percent, + 50, + `percent must be 50 (phase fraction), not 100 (plan fraction) — ` + + `6 of 12 ROADMAP phases realized. Got ${fm.progress.percent}`, + ); + }); + + test('all phases realized: percent equals plan fraction (no artificial cap)', () => { + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + + fs.writeFileSync(statePath, [ + '# GSD State', + '', + '## Configuration', + 'Current Phase: 3', + 'Current Phase Name: final-phase', + 'Total Plans in Phase: 1', + 'Current Plan: 1', + 'Status: Executing Phase 3', + 'Last Activity: 2026-01-01', + '', + ].join('\n')); + + // ROADMAP declares 3 phases; all 3 have dirs and full plan+summary coverage + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + buildRoadmap(3), + ); + + const phasesDir = path.join(tmpDir, '.planning', 'phases'); + createPhaseDirs(phasesDir, 3); + + const jsonResult = runGsdTools('state json', tmpDir); + assert.ok(jsonResult.success, `state json failed: ${jsonResult.error}`); + + const fm = JSON.parse(jsonResult.output); + assert.ok(fm.progress, 'frontmatter must have progress block'); + + // 3/3 phases done → phase_fraction = 100% + // 3/3 plans done → plan_fraction = 100% + // percent = min(100, 100) = 100 + assert.strictEqual( + fm.progress.percent, + 100, + `percent must be 100 when all phases are realized and all plans summarized`, + ); + }); + + test('state sync also reflects phase-fraction-capped percent in body Progress field', () => { + // state sync updates the body's Progress: field — it must use the same capped formula + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + + fs.writeFileSync(statePath, [ + '# GSD State', + '', + '## Configuration', + 'Current Phase: 6', + 'Current Phase Name: phase-6', + 'Total Plans in Phase: 1', + 'Current Plan: 1', + 'Status: Executing Phase 6', + 'Last Activity: 2026-01-01', + 'Progress: [░░░░░░░░░░] 0%', + '', + ].join('\n')); + + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + buildRoadmap(12), + ); + + const phasesDir = path.join(tmpDir, '.planning', 'phases'); + createPhaseDirs(phasesDir, 6); + + const syncResult = runGsdTools('state sync', tmpDir); + assert.ok(syncResult.success, `state sync failed: ${syncResult.error}`); + + // Read the body's Progress field via state json (JSON output is authoritative) + const jsonResult = runGsdTools('state json', tmpDir); + assert.ok(jsonResult.success, `state json failed: ${jsonResult.error}`); + + const fm = JSON.parse(jsonResult.output); + assert.ok(fm.progress, 'frontmatter must have progress block'); + + // state sync wrote a Progress: body field; state json re-derives percent from disk. + // Both must agree: 50%, not 100%. + assert.strictEqual( + fm.progress.percent, + 50, + `state sync must cap percent at phase fraction (50%), got ${fm.progress.percent}`, + ); + }); +});