From 2bc295b32d1174ecd43adc7411eb5e9dbb3ae091 Mon Sep 17 00:00:00 2001 From: Colin Johnson Date: Sat, 30 May 2026 17:44:36 -0400 Subject: [PATCH] fix(#464): make phase completion planning writes transactional (#465) * fix(#464): make phase completion planning writes transactional * fix(#464): add phase completion changeset * test(#464): avoid source-grep rollback assertion * fix(#464): address phase completion rollback review * fix(#191): remove retired sdk tsconfig reference --- .changeset/jolly-wolves-hop.md | 5 + .gitignore | 4 + get-shit-done/bin/lib/phase.cjs | 535 ++++++++++-------- get-shit-done/bin/lib/planning-workspace.cjs | 10 +- get-shit-done/bin/lib/state.cjs | 11 + .../4-phase-complete-cjs-regression.test.cjs | 146 +++++ tests/bugs-1656-1657.test.cjs | 16 + tests/planning-workspace.test.cjs | 19 + tsconfig.json | 4 +- 9 files changed, 495 insertions(+), 255 deletions(-) create mode 100644 .changeset/jolly-wolves-hop.md diff --git a/.changeset/jolly-wolves-hop.md b/.changeset/jolly-wolves-hop.md new file mode 100644 index 000000000..afd3cb20e --- /dev/null +++ b/.changeset/jolly-wolves-hop.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 465 +--- +**`phase complete` no longer leaves roadmap and state inconsistent after a failed state publish** — phase completion now publishes planning files from one locked transaction and rolls back earlier writes when a later write fails. diff --git a/.gitignore b/.gitignore index 058828972..b59181191 100644 --- a/.gitignore +++ b/.gitignore @@ -89,3 +89,7 @@ claude-test-command.md .stryker-tmp/ .stryker-incremental.json reports/mutation/ + +# Local Crabbox machine configuration +.crabbox.yaml +.crabbox.local.yaml diff --git a/get-shit-done/bin/lib/phase.cjs b/get-shit-done/bin/lib/phase.cjs index ff0231b50..46a76ccd6 100644 --- a/get-shit-done/bin/lib/phase.cjs +++ b/get-shit-done/bin/lib/phase.cjs @@ -18,7 +18,7 @@ const { escapeRegex, loadConfig, normalizePhaseName, phaseMarkdownRegexSource, c const { platformWriteSync, platformReadSync, platformEnsureDir } = require('./shell-command-projection.cjs'); const { planningDir, withPlanningLock } = require('./planning-workspace.cjs'); const { extractFrontmatter } = require('./frontmatter.cjs'); -const { writeStateMd, readModifyWriteStateMd, stateExtractField, stateReplaceField, stateReplaceFieldWithFallback, updatePerformanceMetricsSection } = require('./state.cjs'); +const { readModifyWriteStateMd, stateExtractField, stateReplaceField, stateReplaceFieldWithFallback, syncStateFrontmatter, withStateLock, updatePerformanceMetricsSection } = require('./state.cjs'); const { formatGsdSlash, resolveRuntime } = require('./runtime-slash.cjs'); // Pure-computation helpers for cmdPhaseComplete (issue #4 fix). const { deriveProgressFromRoadmap, clampPercent } = require('./phase-lifecycle.cjs'); @@ -1207,6 +1207,31 @@ function cmdPhaseRemove(cwd, targetPhase, options, raw) { }, raw); } +function writePlanningFileSet(writes) { + const applied = []; + try { + for (const write of writes) { + if (write.before === write.after) continue; + platformWriteSync(write.filePath, write.after); + applied.push(write); + } + } catch (err) { + for (const write of applied.reverse()) { + try { + platformWriteSync(write.filePath, write.before); + } catch (rollbackErr) { + err.rollbackError = rollbackErr; + err.message += `\nWARNING: rollback failed while restoring ${write.filePath} ` + + `(${rollbackErr.message}). Planning files under .planning/ may be left in an ` + + `inconsistent, partially rolled back state. Inspect ROADMAP.md / REQUIREMENTS.md / ` + + `STATE.md before re-running phase complete.`; + break; + } + } + throw err; + } +} + function cmdPhaseComplete(cwd, phaseNum, raw) { if (!phaseNum) { error('phase number required for phase complete'); @@ -1249,279 +1274,289 @@ function cmdPhaseComplete(cwd, phaseNum, raw) { } } catch {} - // Update ROADMAP.md and REQUIREMENTS.md atomically under lock - if (fs.existsSync(roadmapPath)) { - withPlanningLock(cwd, () => { - let roadmapContent = fs.readFileSync(roadmapPath, 'utf-8'); - - // Checkbox: - [ ] Phase N: → - [x] Phase N: (...completed DATE) - // #3537: padding-tolerant fragment so the caller-resolved padded id - // matches un-padded ROADMAP prose. - const phaseEscaped = phaseMarkdownRegexSource(phaseNum); - const checkboxPattern = new RegExp( - `(-\\s*\\[)[ ](\\]\\s*.*Phase\\s+${phaseEscaped}[:\\s][^\\n]*)`, - 'i' - ); - roadmapContent = roadmapContent.replace(checkboxPattern, `$1x$2 (completed ${today})`); - - // Progress table: update Status to Complete, add date (handles 4 or 5 column tables) - const tableRowPattern = new RegExp( - `^(\\|\\s*${phaseEscaped}\\.?\\s[^|]*(?:\\|[^\\n]*))$`, - 'im' - ); - roadmapContent = roadmapContent.replace(tableRowPattern, (fullRow) => { - const cells = fullRow.split('|').slice(1, -1); - if (cells.length === 5) { - // 5-col: Phase | Milestone | Plans | Status | Completed - cells[2] = ` ${summaryCount}/${planCount} `; - cells[3] = ' Complete '; - cells[4] = ` ${today} `; - } else if (cells.length === 4) { - // 4-col: Phase | Plans | Status | Completed - cells[1] = ` ${summaryCount}/${planCount} `; - cells[2] = ' Complete '; - cells[3] = ` ${today} `; - } - return '|' + cells.join('|') + '|'; - }); - - // Update plan count in phase section. - // Use direct .replace() rather than replaceInCurrentMilestone() so this - // works when the current milestone section is itself inside a
- // block (the standard /gsd:new-project layout). replaceInCurrentMilestone - // scopes to content after the last
, which misses content inside - // the current milestone's own
wrapper (#2005). - // The phase-scoped heading pattern is specific enough to avoid matching - // archived phases (which belong to different milestones). - const planCountPattern = new RegExp( - `(#{2,4}\\s*Phase\\s+${phaseEscaped}[\\s\\S]*?\\*\\*Plans:\\*\\*\\s*)[^\\n]+`, - 'i' - ); - roadmapContent = roadmapContent.replace( - planCountPattern, - `$1${summaryCount}/${planCount} plans complete` - ); - - // Mark completed plan checkboxes (safety net for missed per-plan updates) - // Handles both plain IDs ("- [ ] 01-01-PLAN.md") and bold-wrapped IDs ("- [ ] **01-01**") - for (const summaryFile of phaseInfo.summaries) { - const planId = summaryFile.replace('-SUMMARY.md', '').replace('SUMMARY.md', ''); - if (!planId) continue; - const planEscaped = escapeRegex(planId); - const planCheckboxPattern = new RegExp( - `(-\\s*\\[) (\\]\\s*(?:\\*\\*)?${planEscaped}(?:\\*\\*)?)`, - 'i' - ); - roadmapContent = roadmapContent.replace(planCheckboxPattern, '$1x$2'); - } - - platformWriteSync(roadmapPath, roadmapContent); - - // Update REQUIREMENTS.md traceability for this phase's requirements - const reqPath = path.join(planningDir(cwd), 'REQUIREMENTS.md'); - if (fs.existsSync(reqPath)) { - // Extract the current phase section from roadmap (scoped to avoid cross-phase matching). - // #3537: padding-tolerant fragment so an un-padded `Phase 2.7:` heading - // is found when caller resolved to padded `02.7`. - const phaseEsc = phaseMarkdownRegexSource(phaseNum); - const currentMilestoneRoadmap = extractCurrentMilestone(roadmapContent, cwd); - const phaseSectionMatch = currentMilestoneRoadmap.match( - new RegExp(`(#{2,4}\\s*Phase\\s+${phaseEsc}[:\\s][\\s\\S]*?)(?=#{2,4}\\s*Phase\\s+|$)`, 'i') - ); - - const sectionText = phaseSectionMatch ? phaseSectionMatch[1] : ''; - // Accept all bold/colon variants (#2769) — the previous pattern only - // matched **Requirements:** (colon inside bold) and silently skipped - // **Requirements**: (colon outside), preventing the matching REQ-IDs - // from being ticked off in REQUIREMENTS.md on phase completion. - const reqMatch = sectionText.match(/\*\*Requirements:?\*\*[^\S\n]*:?[^\S\n]*([^\n]+)/i); - - let reqContent = fs.readFileSync(reqPath, 'utf-8'); - - if (reqMatch) { - const reqIds = reqMatch[1].replace(/[\[\]]/g, '').split(/[,\s]+/).map(r => r.trim()).filter(Boolean); - - for (const reqId of reqIds) { - const reqEscaped = escapeRegex(reqId); - // Update checkbox: - [ ] **REQ-ID** → - [x] **REQ-ID** - reqContent = reqContent.replace( - new RegExp(`(-\\s*\\[)[ ](\\]\\s*\\*\\*${reqEscaped}\\*\\*)`, 'gi'), - '$1x$2' - ); - // Update traceability table: | REQ-ID | Phase N | Pending/In Progress | → | REQ-ID | Phase N | Complete | - reqContent = reqContent.replace( - new RegExp(`(\\|\\s*${reqEscaped}\\s*\\|[^|]+\\|)\\s*(?:Pending|In Progress)\\s*(\\|)`, 'gi'), - '$1 Complete $2' - ); - } - } - - // Scan body for all **REQ-ID** patterns, warn about any missing from the Traceability table. - // Always runs regardless of whether the roadmap has a Requirements: line. - const bodyReqIds = []; - const bodyReqPattern = /\*\*([A-Z][A-Z0-9]*-\d+)\*\*/g; - let bodyMatch; - while ((bodyMatch = bodyReqPattern.exec(reqContent)) !== null) { - const id = bodyMatch[1]; - if (!bodyReqIds.includes(id)) bodyReqIds.push(id); - } - - // Collect REQ-IDs present in the Traceability section only, to avoid - // picking up IDs from other tables in the document. - const traceabilityHeadingMatch = reqContent.match(/^#{1,6}\s+Traceability\b/im); - const traceabilitySection = traceabilityHeadingMatch - ? reqContent.slice(traceabilityHeadingMatch.index) - : ''; - const tableReqIds = new Set(); - const tableRowPattern = /^\|\s*([A-Z][A-Z0-9]*-\d+)\s*\|/gm; - let tableMatch; - while ((tableMatch = tableRowPattern.exec(traceabilitySection)) !== null) { - tableReqIds.add(tableMatch[1]); - } - - const unregistered = bodyReqIds.filter(id => !tableReqIds.has(id)); - if (unregistered.length > 0) { - warnings.push( - `REQUIREMENTS.md: ${unregistered.length} REQ-ID(s) found in body but missing from Traceability table: ${unregistered.join(', ')} — add them manually to keep traceability in sync` - ); - } - - platformWriteSync(reqPath, reqContent); - requirementsUpdated = true; - } - }); - } - - // Find next phase — check both filesystem AND roadmap - // Phases may be defined in ROADMAP.md but not yet scaffolded to disk, - // so a filesystem-only scan would incorrectly report is_last_phase:true let nextPhaseNum = null; let nextPhaseName = null; let isLastPhase = true; - try { - const isDirInMilestone = getMilestonePhaseFilter(cwd); - const entries = fs.readdirSync(phasesDir, { withFileTypes: true }); - const dirs = entries.filter(e => e.isDirectory()).map(e => e.name) - .filter(isDirInMilestone) - .sort((a, b) => comparePhaseNum(a, b)); + // Update ROADMAP.md, REQUIREMENTS.md, and STATE.md from one locked snapshot. + // A previous split-lock sequence could publish ROADMAP/REQUIREMENTS and then + // fail before STATE advanced, leaving planning files disagreeing about the + // current phase. + withPlanningLock(cwd, () => { + const runPhaseCompleteTransaction = () => { + const writes = []; + let roadmapContent = null; - // Find the next phase directory after current - // Skip backlog phases (999.x) — they are parked ideas, not sequential work (#2129) - for (const dir of dirs) { - const dm = dir.match(/^(\d+[A-Z]?(?:\.\d+)*)-?(.*)/i); - if (dm) { - if (/^999(?:\.|$)/.test(dm[1])) continue; - if (comparePhaseNum(dm[1], phaseNum) > 0) { - nextPhaseNum = dm[1]; - nextPhaseName = dm[2] || null; - isLastPhase = false; - break; + if (fs.existsSync(roadmapPath)) { + const originalRoadmapContent = fs.readFileSync(roadmapPath, 'utf-8'); + roadmapContent = originalRoadmapContent; + + // Checkbox: - [ ] Phase N: → - [x] Phase N: (...completed DATE) + // #3537: padding-tolerant fragment so the caller-resolved padded id + // matches un-padded ROADMAP prose. + const phaseEscaped = phaseMarkdownRegexSource(phaseNum); + const checkboxPattern = new RegExp( + `(-\\s*\\[)[ ](\\]\\s*.*Phase\\s+${phaseEscaped}[:\\s][^\\n]*)`, + 'i' + ); + roadmapContent = roadmapContent.replace(checkboxPattern, `$1x$2 (completed ${today})`); + + // Progress table: update Status to Complete, add date (handles 4 or 5 column tables) + const tableRowPattern = new RegExp( + `^(\\|\\s*${phaseEscaped}\\.?\\s[^|]*(?:\\|[^\\n]*))$`, + 'im' + ); + roadmapContent = roadmapContent.replace(tableRowPattern, (fullRow) => { + const cells = fullRow.split('|').slice(1, -1); + if (cells.length === 5) { + // 5-col: Phase | Milestone | Plans | Status | Completed + cells[2] = ` ${summaryCount}/${planCount} `; + cells[3] = ' Complete '; + cells[4] = ` ${today} `; + } else if (cells.length === 4) { + // 4-col: Phase | Plans | Status | Completed + cells[1] = ` ${summaryCount}/${planCount} `; + cells[2] = ' Complete '; + cells[3] = ` ${today} `; + } + return '|' + cells.join('|') + '|'; + }); + + // Update plan count in phase section. + // Use direct .replace() rather than replaceInCurrentMilestone() so this + // works when the current milestone section is itself inside a
+ // block (the standard /gsd:new-project layout). replaceInCurrentMilestone + // scopes to content after the last
, which misses content inside + // the current milestone's own
wrapper (#2005). + // The phase-scoped heading pattern is specific enough to avoid matching + // archived phases (which belong to different milestones). + const planCountPattern = new RegExp( + `(#{2,4}\\s*Phase\\s+${phaseEscaped}[\\s\\S]*?\\*\\*Plans:\\*\\*\\s*)[^\\n]+`, + 'i' + ); + roadmapContent = roadmapContent.replace( + planCountPattern, + `$1${summaryCount}/${planCount} plans complete` + ); + + // Mark completed plan checkboxes (safety net for missed per-plan updates) + // Handles both plain IDs ("- [ ] 01-01-PLAN.md") and bold-wrapped IDs ("- [ ] **01-01**") + for (const summaryFile of phaseInfo.summaries) { + const planId = summaryFile.replace('-SUMMARY.md', '').replace('SUMMARY.md', ''); + if (!planId) continue; + const planEscaped = escapeRegex(planId); + const planCheckboxPattern = new RegExp( + `(-\\s*\\[) (\\]\\s*(?:\\*\\*)?${planEscaped}(?:\\*\\*)?)`, + 'i' + ); + roadmapContent = roadmapContent.replace(planCheckboxPattern, '$1x$2'); + } + + writes.push({ filePath: roadmapPath, before: originalRoadmapContent, after: roadmapContent }); + + // Update REQUIREMENTS.md traceability for this phase's requirements + const reqPath = path.join(planningDir(cwd), 'REQUIREMENTS.md'); + if (fs.existsSync(reqPath)) { + // Extract the current phase section from roadmap (scoped to avoid cross-phase matching). + // #3537: padding-tolerant fragment so an un-padded `Phase 2.7:` heading + // is found when caller resolved to padded `02.7`. + const phaseEsc = phaseMarkdownRegexSource(phaseNum); + const currentMilestoneRoadmap = extractCurrentMilestone(roadmapContent, cwd); + const phaseSectionMatch = currentMilestoneRoadmap.match( + new RegExp(`(#{2,4}\\s*Phase\\s+${phaseEsc}[:\\s][\\s\\S]*?)(?=#{2,4}\\s*Phase\\s+|$)`, 'i') + ); + + const sectionText = phaseSectionMatch ? phaseSectionMatch[1] : ''; + // Accept all bold/colon variants (#2769) — the previous pattern only + // matched **Requirements:** (colon inside bold) and silently skipped + // **Requirements**: (colon outside), preventing the matching REQ-IDs + // from being ticked off in REQUIREMENTS.md on phase completion. + const reqMatch = sectionText.match(/\*\*Requirements:?\*\*[^\S\n]*:?[^\S\n]*([^\n]+)/i); + + const originalReqContent = fs.readFileSync(reqPath, 'utf-8'); + let reqContent = originalReqContent; + + if (reqMatch) { + const reqIds = reqMatch[1].replace(/[\[\]]/g, '').split(/[,\s]+/).map(r => r.trim()).filter(Boolean); + + for (const reqId of reqIds) { + const reqEscaped = escapeRegex(reqId); + // Update checkbox: - [ ] **REQ-ID** → - [x] **REQ-ID** + reqContent = reqContent.replace( + new RegExp(`(-\\s*\\[)[ ](\\]\\s*\\*\\*${reqEscaped}\\*\\*)`, 'gi'), + '$1x$2' + ); + // Update traceability table: | REQ-ID | Phase N | Pending/In Progress | → | REQ-ID | Phase N | Complete | + reqContent = reqContent.replace( + new RegExp(`(\\|\\s*${reqEscaped}\\s*\\|[^|]+\\|)\\s*(?:Pending|In Progress)\\s*(\\|)`, 'gi'), + '$1 Complete $2' + ); + } + } + + // Scan body for all **REQ-ID** patterns, warn about any missing from the Traceability table. + // Always runs regardless of whether the roadmap has a Requirements: line. + const bodyReqIds = []; + const bodyReqPattern = /\*\*([A-Z][A-Z0-9]*-\d+)\*\*/g; + let bodyMatch; + while ((bodyMatch = bodyReqPattern.exec(reqContent)) !== null) { + const id = bodyMatch[1]; + if (!bodyReqIds.includes(id)) bodyReqIds.push(id); + } + + // Collect REQ-IDs present in the Traceability section only, to avoid + // picking up IDs from other tables in the document. + const traceabilityHeadingMatch = reqContent.match(/^#{1,6}\s+Traceability\b/im); + const traceabilitySection = traceabilityHeadingMatch + ? reqContent.slice(traceabilityHeadingMatch.index) + : ''; + const tableReqIds = new Set(); + const tableRowPattern = /^\|\s*([A-Z][A-Z0-9]*-\d+)\s*\|/gm; + let tableMatch; + while ((tableMatch = tableRowPattern.exec(traceabilitySection)) !== null) { + tableReqIds.add(tableMatch[1]); + } + + const unregistered = bodyReqIds.filter(id => !tableReqIds.has(id)); + if (unregistered.length > 0) { + warnings.push( + `REQUIREMENTS.md: ${unregistered.length} REQ-ID(s) found in body but missing from Traceability table: ${unregistered.join(', ')} — add them manually to keep traceability in sync` + ); + } + + writes.push({ filePath: reqPath, before: originalReqContent, after: reqContent }); + requirementsUpdated = true; } } - } - } catch { /* intentionally empty */ } - // Fallback: if filesystem found no next phase, check ROADMAP.md - // for phases that are defined but not yet planned (no directory on disk) - if (isLastPhase && fs.existsSync(roadmapPath)) { - try { - const roadmapForPhases = extractCurrentMilestone(fs.readFileSync(roadmapPath, 'utf-8'), cwd); - const phasePattern = /#{2,4}\s*Phase\s+(\d+[A-Z]?(?:\.\d+)*)\s*:\s*([^\n]+)/gi; - let pm; - while ((pm = phasePattern.exec(roadmapForPhases)) !== null) { - if (comparePhaseNum(pm[1], phaseNum) > 0) { - nextPhaseNum = pm[1]; - nextPhaseName = pm[2].replace(/\(INSERTED\)/i, '').trim().toLowerCase().replace(/\s+/g, '-'); - isLastPhase = false; - break; + // Find next phase — check both filesystem AND roadmap + // Phases may be defined in ROADMAP.md but not yet scaffolded to disk, + // so a filesystem-only scan would incorrectly report is_last_phase:true + try { + const isDirInMilestone = getMilestonePhaseFilter(cwd); + const entries = fs.readdirSync(phasesDir, { withFileTypes: true }); + const dirs = entries.filter(e => e.isDirectory()).map(e => e.name) + .filter(isDirInMilestone) + .sort((a, b) => comparePhaseNum(a, b)); + + // Find the next phase directory after current + // Skip backlog phases (999.x) — they are parked ideas, not sequential work (#2129) + for (const dir of dirs) { + const dm = dir.match(/^(\d+[A-Z]?(?:\.\d+)*)-?(.*)/i); + if (dm) { + if (/^999(?:\.|$)/.test(dm[1])) continue; + if (comparePhaseNum(dm[1], phaseNum) > 0) { + nextPhaseNum = dm[1]; + nextPhaseName = dm[2] || null; + isLastPhase = false; + break; + } + } } - } - } catch { /* intentionally empty */ } - } + } catch { /* intentionally empty */ } - // Update STATE.md atomically — hold lock across read-modify-write (#P4.4). - // Previously read outside the lock; a crash between the ROADMAP update - // (locked above) and this write left ROADMAP/STATE inconsistent. - if (fs.existsSync(statePath)) { - readModifyWriteStateMd(statePath, (stateContent) => { - // Update Current Phase — preserve "X of Y (Name)" compound format - const phaseValue = nextPhaseNum || phaseNum; - const existingPhaseField = stateExtractField(stateContent, 'Current Phase') - || stateExtractField(stateContent, 'Phase'); - let newPhaseValue = String(phaseValue); - if (existingPhaseField) { - const totalMatch = existingPhaseField.match(/of\s+(\d+)/); - const nameMatch = existingPhaseField.match(/\(([^)]+)\)/); - if (totalMatch) { - const total = totalMatch[1]; - const nameStr = nextPhaseName ? ` (${nextPhaseName.replace(/-/g, ' ')})` : (nameMatch ? ` (${nameMatch[1]})` : ''); - newPhaseValue = `${phaseValue} of ${total}${nameStr}`; + // Fallback: if filesystem found no next phase, check ROADMAP.md + // for phases that are defined but not yet planned (no directory on disk) + if (isLastPhase && roadmapContent !== null) { + try { + const roadmapForPhases = extractCurrentMilestone(roadmapContent, cwd); + const phasePattern = /#{2,4}\s*Phase\s+(\d+[A-Z]?(?:\.\d+)*)\s*:\s*([^\n]+)/gi; + let pm; + while ((pm = phasePattern.exec(roadmapForPhases)) !== null) { + if (comparePhaseNum(pm[1], phaseNum) > 0) { + nextPhaseNum = pm[1]; + nextPhaseName = pm[2].replace(/\(INSERTED\)/i, '').trim().toLowerCase().replace(/\s+/g, '-'); + isLastPhase = false; + break; + } + } + } catch { /* intentionally empty */ } + } + + // Update STATE.md while the planning lock is still held. + if (fs.existsSync(statePath)) { + const originalStateContent = platformReadSync(statePath) || ''; + let stateContent = originalStateContent; + + // Update Current Phase — preserve "X of Y (Name)" compound format + const phaseValue = nextPhaseNum || phaseNum; + const existingPhaseField = stateExtractField(stateContent, 'Current Phase') + || stateExtractField(stateContent, 'Phase'); + let newPhaseValue = String(phaseValue); + if (existingPhaseField) { + const totalMatch = existingPhaseField.match(/of\s+(\d+)/); + const nameMatch = existingPhaseField.match(/\(([^)]+)\)/); + if (totalMatch) { + const total = totalMatch[1]; + const nameStr = nextPhaseName ? ` (${nextPhaseName.replace(/-/g, ' ')})` : (nameMatch ? ` (${nameMatch[1]})` : ''); + newPhaseValue = `${phaseValue} of ${total}${nameStr}`; + } } - } - stateContent = stateReplaceFieldWithFallback(stateContent, 'Current Phase', 'Phase', newPhaseValue); + stateContent = stateReplaceFieldWithFallback(stateContent, 'Current Phase', 'Phase', newPhaseValue); - // Update Current Phase Name - if (nextPhaseName) { - stateContent = stateReplaceFieldWithFallback(stateContent, 'Current Phase Name', null, nextPhaseName.replace(/-/g, ' ')); - } + // Update Current Phase Name + if (nextPhaseName) { + stateContent = stateReplaceFieldWithFallback(stateContent, 'Current Phase Name', null, nextPhaseName.replace(/-/g, ' ')); + } - // Update Status - stateContent = stateReplaceFieldWithFallback(stateContent, 'Status', null, - isLastPhase ? 'Milestone complete' : 'Ready to plan'); + // Update Status + stateContent = stateReplaceFieldWithFallback(stateContent, 'Status', null, + isLastPhase ? 'Milestone complete' : 'Ready to plan'); - // Update Current Plan - stateContent = stateReplaceFieldWithFallback(stateContent, 'Current Plan', 'Plan', 'Not started'); + // Update Current Plan + stateContent = stateReplaceFieldWithFallback(stateContent, 'Current Plan', 'Plan', 'Not started'); - // Update Last Activity - stateContent = stateReplaceFieldWithFallback(stateContent, 'Last Activity', 'Last activity', today); + // Update Last Activity + stateContent = stateReplaceFieldWithFallback(stateContent, 'Last Activity', 'Last activity', today); - // Update Last Activity Description - stateContent = stateReplaceFieldWithFallback(stateContent, 'Last Activity Description', null, - `Phase ${phaseNum} complete${nextPhaseNum ? `, transitioned to Phase ${nextPhaseNum}` : ''}`); + // Update Last Activity Description + stateContent = stateReplaceFieldWithFallback(stateContent, 'Last Activity Description', null, + `Phase ${phaseNum} complete${nextPhaseNum ? `, transitioned to Phase ${nextPhaseNum}` : ''}`); - // Update Completed Phases counter — derive from ROADMAP instead of blind +1. - // Fix for issue #4: the original code did parseInt(completedRaw, 10) + 1 on every - // call, making phase complete non-idempotent (double-call = double-increment). - // Now we read the freshly-updated ROADMAP to count Complete rows, then use - // deriveProgressFromRoadmap() from phase-lifecycle.generated.cjs (generated from - // sdk/src/query/phase-lifecycle.ts "Root cause 1 fix" block). - // References: issue #4, ADR-3524, gen-phase-lifecycle.mjs. - const completedRaw = stateExtractField(stateContent, 'Completed Phases'); - if (completedRaw !== null) { - // Derive from ROADMAP if available (idempotent); fall back to existing value. - let newCompleted = parseInt(completedRaw, 10); - let derivedTotalPhases = null; - if (fs.existsSync(roadmapPath)) { - try { - const freshRoadmap = fs.readFileSync(roadmapPath, 'utf-8'); - const derived = deriveProgressFromRoadmap(freshRoadmap); + // Update Completed Phases counter — derive from the same ROADMAP snapshot + // that will be published in this transaction, not a separately-read file. + const completedRaw = stateExtractField(stateContent, 'Completed Phases'); + if (completedRaw !== null) { + // Derive from ROADMAP if available (idempotent); fall back to existing value. + let newCompleted = parseInt(completedRaw, 10); + let derivedTotalPhases = null; + if (roadmapContent !== null) { + const derived = deriveProgressFromRoadmap(roadmapContent); if (derived.completedPhases !== null) newCompleted = derived.completedPhases; if (derived.totalPhases !== null) derivedTotalPhases = derived.totalPhases; - } catch { /* fall through to existing value */ } - } - stateContent = stateReplaceField(stateContent, 'Completed Phases', String(newCompleted)) || stateContent; + } + stateContent = stateReplaceField(stateContent, 'Completed Phases', String(newCompleted)) || stateContent; - // Recalculate percent — use clampPercent to prevent >100% (#4 unclamped bug). - const totalRaw = stateExtractField(stateContent, 'Total Phases'); - const totalPhases = derivedTotalPhases - || (totalRaw ? parseInt(totalRaw, 10) : null); - if (totalPhases && totalPhases > 0) { - const newPercent = clampPercent(newCompleted, totalPhases); - stateContent = stateReplaceField(stateContent, 'Progress', `${newPercent}%`) || stateContent; - stateContent = stateContent.replace( - /(percent:\s*)\d+/, - `$1${newPercent}` - ); + // Recalculate percent — use clampPercent to prevent >100% (#4 unclamped bug). + const totalRaw = stateExtractField(stateContent, 'Total Phases'); + const totalPhases = derivedTotalPhases + || (totalRaw ? parseInt(totalRaw, 10) : null); + if (totalPhases && totalPhases > 0) { + const newPercent = clampPercent(newCompleted, totalPhases); + stateContent = stateReplaceField(stateContent, 'Progress', `${newPercent}%`) || stateContent; + stateContent = stateContent.replace( + /(percent:\s*)\d+/, + `$1${newPercent}` + ); + } } + + // Gate 4: Update Performance Metrics section (#1627) + stateContent = updatePerformanceMetricsSection(stateContent, cwd, phaseNum, planCount, summaryCount); + stateContent = syncStateFrontmatter(stateContent, cwd); + + writes.push({ filePath: statePath, before: originalStateContent, after: stateContent }); } - // Gate 4: Update Performance Metrics section (#1627) - stateContent = updatePerformanceMetricsSection(stateContent, cwd, phaseNum, planCount, summaryCount); + writePlanningFileSet(writes); + }; - return stateContent; - }, cwd); - } + if (fs.existsSync(statePath)) { + withStateLock(statePath, runPhaseCompleteTransaction); + } else { + runPhaseCompleteTransaction(); + } + }); // Auto-prune STATE.md on phase boundary when configured (#2087) let autoPruned = false; diff --git a/get-shit-done/bin/lib/planning-workspace.cjs b/get-shit-done/bin/lib/planning-workspace.cjs index f3c2e7f95..5f9bcdcd7 100644 --- a/get-shit-done/bin/lib/planning-workspace.cjs +++ b/get-shit-done/bin/lib/planning-workspace.cjs @@ -100,7 +100,7 @@ function withPlanningLock(cwd, fn, clock) { // Ensure .planning/ exists try { platformEnsureDir(planningDir(cwd)); } catch { /* ok */ } - function runWithHeldLock() { + function acquireLock() { // Atomic create — fails if file exists fs.writeFileSync(lockPath, JSON.stringify({ pid: process.pid, @@ -109,8 +109,9 @@ function withPlanningLock(cwd, fn, clock) { }), { flag: 'wx' }); _heldPlanningLocks.add(lockPath); + } - // Lock acquired — run the function + function runWithHeldLock() { try { return fn(); } finally { @@ -120,12 +121,16 @@ function withPlanningLock(cwd, fn, clock) { } while (clock.now() - start < lockTimeout) { + let lockWasAcquired = false; try { + acquireLock(); + lockWasAcquired = true; return runWithHeldLock(); } catch (err) { // Transient filesystem errors (Docker overlay-fs, NFS, OS signals, AV scanners) // are recoverable — wait and retry rather than propagating. // See PLANNING_LOCK_RETRY_ERRNOS for the full list and rationale. + if (lockWasAcquired) throw err; if (PLANNING_LOCK_RETRY_ERRNOS.has(err.code)) { clock.sleep(100); continue; @@ -150,6 +155,7 @@ function withPlanningLock(cwd, fn, clock) { // Timeout — stale-lock recovery, then re-acquire atomically before entering critical section. try { fs.unlinkSync(lockPath); } catch { /* ok */ } + acquireLock(); return runWithHeldLock(); } diff --git a/get-shit-done/bin/lib/state.cjs b/get-shit-done/bin/lib/state.cjs index 1bac8132f..941096850 100644 --- a/get-shit-done/bin/lib/state.cjs +++ b/get-shit-done/bin/lib/state.cjs @@ -1050,6 +1050,15 @@ function releaseStateLock(lockPath) { try { fs.unlinkSync(lockPath); } catch { /* lock already gone */ } } +function withStateLock(statePath, fn) { + const lockPath = acquireStateLock(statePath); + try { + return fn(); + } finally { + releaseStateLock(lockPath); + } +} + /** * Write STATE.md with synchronized YAML frontmatter. * All STATE.md writes should use this instead of raw writeFileSync. @@ -2001,6 +2010,8 @@ module.exports = { releaseStateLock, writeStateMd, readModifyWriteStateMd, + syncStateFrontmatter, + withStateLock, updatePerformanceMetricsSection, cmdStateLoad, cmdStateGet, diff --git a/tests/4-phase-complete-cjs-regression.test.cjs b/tests/4-phase-complete-cjs-regression.test.cjs index 5b8e6f4d8..a0ca0ae2c 100644 --- a/tests/4-phase-complete-cjs-regression.test.cjs +++ b/tests/4-phase-complete-cjs-regression.test.cjs @@ -41,6 +41,7 @@ const { cmdPhaseComplete } = phaseModule; /** * Creates a minimal fixture project with: * - ROADMAP.md with a 4-column progress table (Phase | Plans | Status | Completed) + * - REQUIREMENTS.md with a phase-scoped REQ-ID and Traceability row * - STATE.md with Completed Phases: 0 and Total Phases: 2 (Progress 0%) * - Phase 01 directory with one plan+summary (to satisfy phase complete guard) * - Phase 02 directory (next phase) @@ -61,6 +62,7 @@ function createFixture(prefix = 'gsd-4-regression-') { '', '### Phase 01: Foundation', '**Goal:** Build the foundation', + '**Requirements:** REQ-1', '**Plans:** 1 plans', '', '### Phase 02: API', @@ -76,6 +78,22 @@ function createFixture(prefix = 'gsd-4-regression-') { ].join('\n'); fs.writeFileSync(path.join(planningDir, 'ROADMAP.md'), roadmap); + const requirements = [ + '# Requirements', + '', + '## Functional Requirements', + '', + '- [ ] **REQ-1** Foundation must be complete.', + '', + '## Traceability', + '', + '| Requirement | Phase | Status |', + '|-------------|-------|--------|', + '| REQ-1 | Phase 01 | Pending |', + '', + ].join('\n'); + fs.writeFileSync(path.join(planningDir, 'REQUIREMENTS.md'), requirements); + // STATE.md: Completed Phases: 0, Total Phases: 2, Progress: 0% // Uses body-field format (bold **Field:** value) so the CJS handler's // stateExtractField/stateReplaceField path is exercised. @@ -111,6 +129,38 @@ function readStateMd(tmpDir) { return fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf8'); } +function roadmapCompletionSnapshot(roadmapContent) { + const snapshot = { + phaseCheckboxes: [], + progressRows: [], + }; + + for (const line of roadmapContent.split(/\r?\n/)) { + let match = line.match(/^- \[([ x])\] Phase ([^:]+): (.*)$/); + if (match) { + snapshot.phaseCheckboxes.push({ + checked: match[1] === 'x', + phase: match[2].trim(), + title: match[3].replace(/\s+\(completed [^)]+\)$/, '').trim(), + }); + continue; + } + + match = line.match(/^\|\s*(\d+[A-Z]?(?:\.\d+)*)\.?\s*([^|]*)\|\s*([^|]*)\|\s*([^|]*)\|\s*([^|]*)\|$/i); + if (match) { + snapshot.progressRows.push({ + phase: match[1].trim(), + title: match[2].trim(), + plans: match[3].trim(), + status: match[4].trim(), + completed: match[5].trim(), + }); + } + } + + return snapshot; +} + function extractField(stateContent, fieldName) { const escaped = fieldName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); const boldMatch = stateContent.match(new RegExp(`\\*\\*${escaped}:\\*\\*[ \\t]*(.+)`, 'i')); @@ -207,6 +257,102 @@ describe('issue #4 (CJS): cmdPhaseComplete — idempotency (blind-increment bug) `STATE after second call (body: ${completedAfter2Body}, fm: ${completedAfter2Fm}):\n${stateAfter2}`, ); }); + + test('rolls back ROADMAP when STATE write fails during phase completion', (t) => { + const roadmapPath = path.join(tmpDir, '.planning', 'ROADMAP.md'); + const reqPath = path.join(tmpDir, '.planning', 'REQUIREMENTS.md'); + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + const originalRoadmap = fs.readFileSync(roadmapPath, 'utf8'); + const originalReq = fs.readFileSync(reqPath, 'utf8'); + const originalState = fs.readFileSync(statePath, 'utf8'); + const originalWriteFileSync = fs.writeFileSync; + + t.mock.method(fs, 'writeFileSync', function injectedStateWriteFailure(target, ...args) { + const targetPath = String(target); + const isStatePublish = targetPath === statePath || targetPath === `${statePath}.tmp.${process.pid}`; + if (isStatePublish) { + const err = new Error('injected STATE.md write failure'); + err.code = 'EIO'; + throw err; + } + return originalWriteFileSync.call(this, target, ...args); + }); + + assert.throws( + () => capturePhaseComplete(tmpDir, '1'), + /injected STATE\.md write failure/, + ); + + const roadmapAfter = fs.readFileSync(roadmapPath, 'utf8'); + const reqAfter = fs.readFileSync(reqPath, 'utf8'); + const stateAfter = fs.readFileSync(statePath, 'utf8'); + + assert.deepEqual( + roadmapCompletionSnapshot(roadmapAfter), + roadmapCompletionSnapshot(originalRoadmap), + 'ROADMAP.md should roll back to its original completion state', + ); + assert.equal(reqAfter, originalReq, 'REQUIREMENTS.md should roll back when STATE.md write fails'); + assert.equal(stateAfter, originalState, 'STATE.md should remain unchanged after injected write failure'); + }); + + test('rolls back ROADMAP when REQUIREMENTS write fails during phase completion', (t) => { + const roadmapPath = path.join(tmpDir, '.planning', 'ROADMAP.md'); + const reqPath = path.join(tmpDir, '.planning', 'REQUIREMENTS.md'); + const originalRoadmap = fs.readFileSync(roadmapPath, 'utf8'); + const originalReq = fs.readFileSync(reqPath, 'utf8'); + const originalWriteFileSync = fs.writeFileSync; + + t.mock.method(fs, 'writeFileSync', function injectedRequirementsWriteFailure(target, ...args) { + const targetPath = String(target); + if (targetPath === reqPath || targetPath === `${reqPath}.tmp.${process.pid}`) { + const err = new Error('injected REQUIREMENTS.md write failure'); + err.code = 'EIO'; + throw err; + } + return originalWriteFileSync.call(this, target, ...args); + }); + + assert.throws( + () => capturePhaseComplete(tmpDir, '1'), + /injected REQUIREMENTS\.md write failure/, + ); + + assert.deepEqual( + roadmapCompletionSnapshot(fs.readFileSync(roadmapPath, 'utf8')), + roadmapCompletionSnapshot(originalRoadmap), + 'ROADMAP.md should roll back when the REQUIREMENTS write fails', + ); + assert.equal(fs.readFileSync(reqPath, 'utf8'), originalReq, 'REQUIREMENTS.md should be unchanged'); + }); + + test('reports rollback failure when restoring an earlier planning file fails', (t) => { + const roadmapPath = path.join(tmpDir, '.planning', 'ROADMAP.md'); + const reqPath = path.join(tmpDir, '.planning', 'REQUIREMENTS.md'); + const originalWriteFileSync = fs.writeFileSync; + let requirementsWriteFailed = false; + + t.mock.method(fs, 'writeFileSync', function injectedRollbackFailure(target, ...args) { + const targetPath = String(target); + if (targetPath === reqPath || targetPath === `${reqPath}.tmp.${process.pid}`) { + requirementsWriteFailed = true; + const err = new Error('injected REQUIREMENTS.md write failure'); + err.code = 'EIO'; + throw err; + } + if (requirementsWriteFailed && (targetPath === roadmapPath || targetPath === `${roadmapPath}.tmp.${process.pid}`)) { + const err = new Error('injected ROADMAP.md rollback failure'); + err.code = 'EIO'; + throw err; + } + return originalWriteFileSync.call(this, target, ...args); + }); + + assert.throws( + () => capturePhaseComplete(tmpDir, '1'), + /injected REQUIREMENTS\.md write failure[\s\S]*WARNING: rollback failed while restoring[\s\S]*injected ROADMAP\.md rollback failure/, + ); + }); }); // ── T2: Progress percent must never exceed 100% ────────────────────────────── diff --git a/tests/bugs-1656-1657.test.cjs b/tests/bugs-1656-1657.test.cjs index d8cdb805f..0964991b3 100644 --- a/tests/bugs-1656-1657.test.cjs +++ b/tests/bugs-1656-1657.test.cjs @@ -90,4 +90,20 @@ describe('#1657 / #191: installer/package metadata retires sdk seam', () => { 'root package.json files must not include sdk paths' ); }); + + test('root tsconfig project references resolve to existing paths (#191)', () => { + const tsconfigPath = path.join(__dirname, '..', 'tsconfig.json'); + if (!fs.existsSync(tsconfigPath)) return; + + const tsconfig = JSON.parse(fs.readFileSync(tsconfigPath, 'utf-8')); + const references = tsconfig.references || []; + for (const ref of references) { + const refPath = String(ref.path || ''); + assert.notEqual(refPath, 'sdk', 'root tsconfig must not reference retired sdk project'); + assert.ok( + fs.existsSync(path.join(__dirname, '..', refPath)), + `root tsconfig reference must exist: ${refPath}` + ); + } + }); }); diff --git a/tests/planning-workspace.test.cjs b/tests/planning-workspace.test.cjs index 9431c46ba..699b0fe73 100644 --- a/tests/planning-workspace.test.cjs +++ b/tests/planning-workspace.test.cjs @@ -125,6 +125,25 @@ describe('planning-workspace: lock seam', () => { fs.rmSync(tmpDir, { recursive: true, force: true }); } }); + + test('does not retry errors thrown by locked work', () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-planning-lock-work-error-')); + let attempts = 0; + try { + assert.throws(() => { + withPlanningLock(tmpDir, () => { + attempts += 1; + const err = new Error('write failed inside critical section'); + err.code = 'EIO'; + throw err; + }); + }, /write failed inside critical section/); + assert.strictEqual(attempts, 1); + assert.ok(!fs.existsSync(path.join(tmpDir, '.planning', '.lock'))); + } finally { + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }); }); describe('core compatibility adapter: planning workspace functions', () => { diff --git a/tsconfig.json b/tsconfig.json index d8563a4ce..c5369eaef 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -1,6 +1,4 @@ { "files": [], - "references": [ - { "path": "sdk" } - ] + "references": [] }