diff --git a/get-shit-done/bin/lib/config.cjs b/get-shit-done/bin/lib/config.cjs index cbbaf644..d0294538 100644 --- a/get-shit-done/bin/lib/config.cjs +++ b/get-shit-done/bin/lib/config.cjs @@ -4,7 +4,7 @@ const fs = require('fs'); const path = require('path'); -const { output, error, planningRoot, CONFIG_DEFAULTS, atomicWriteFileSync } = require('./core.cjs'); +const { output, error, planningRoot, withPlanningLock, CONFIG_DEFAULTS, atomicWriteFileSync } = require('./core.cjs'); const { VALID_PROFILES, getAgentToModelMapForProfile, @@ -293,36 +293,38 @@ function cmdConfigEnsureSection(cwd, raw) { function setConfigValue(cwd, keyPath, parsedValue) { const configPath = path.join(planningRoot(cwd), 'config.json'); - // Load existing config or start with empty object - let config = {}; - try { - if (fs.existsSync(configPath)) { - config = JSON.parse(fs.readFileSync(configPath, 'utf-8')); + return withPlanningLock(cwd, () => { + // Load existing config or start with empty object + let config = {}; + try { + if (fs.existsSync(configPath)) { + config = JSON.parse(fs.readFileSync(configPath, 'utf-8')); + } + } catch (err) { + error('Failed to read config.json: ' + err.message); } - } catch (err) { - error('Failed to read config.json: ' + err.message); - } - // Set nested value using dot notation (e.g., "workflow.research") - const keys = keyPath.split('.'); - let current = config; - for (let i = 0; i < keys.length - 1; i++) { - const key = keys[i]; - if (current[key] === undefined || typeof current[key] !== 'object') { - current[key] = {}; + // Set nested value using dot notation (e.g., "workflow.research") + const keys = keyPath.split('.'); + let current = config; + for (let i = 0; i < keys.length - 1; i++) { + const key = keys[i]; + if (current[key] === undefined || typeof current[key] !== 'object') { + current[key] = {}; + } + current = current[key]; } - current = current[key]; - } - const previousValue = current[keys[keys.length - 1]]; // Capture previous value before overwriting - current[keys[keys.length - 1]] = parsedValue; + const previousValue = current[keys[keys.length - 1]]; // Capture previous value before overwriting + current[keys[keys.length - 1]] = parsedValue; - // Write back - try { - atomicWriteFileSync(configPath, JSON.stringify(config, null, 2), 'utf-8'); - return { updated: true, key: keyPath, value: parsedValue, previousValue }; - } catch (err) { - error('Failed to write config.json: ' + err.message); - } + // Write back + try { + atomicWriteFileSync(configPath, JSON.stringify(config, null, 2), 'utf-8'); + return { updated: true, key: keyPath, value: parsedValue, previousValue }; + } catch (err) { + error('Failed to write config.json: ' + err.message); + } + }); } /** diff --git a/get-shit-done/bin/lib/core.cjs b/get-shit-done/bin/lib/core.cjs index e233a272..16957bbd 100644 --- a/get-shit-done/bin/lib/core.cjs +++ b/get-shit-done/bin/lib/core.cjs @@ -27,6 +27,16 @@ const WORKSTREAM_SESSION_ENV_KEYS = [ let cachedControllingTtyToken = null; let didProbeControllingTtyToken = false; +// Track all .planning/.lock files held by this process so they can be removed +// on exit. process.on('exit') fires even on process.exit(1), unlike try/finally +// which is skipped when error() calls process.exit(1) inside a locked region (#1916). +const _heldPlanningLocks = new Set(); +process.on('exit', () => { + for (const lockPath of _heldPlanningLocks) { + try { fs.unlinkSync(lockPath); } catch { /* already gone */ } + } +}); + // ─── Path helpers ──────────────────────────────────────────────────────────── /** Normalize a relative path to always use forward slashes (cross-platform). */ @@ -604,10 +614,15 @@ function withPlanningLock(cwd, fn) { acquired: new Date().toISOString(), }), { flag: 'wx' }); + // Register for exit-time cleanup so process.exit(1) inside a locked region + // cannot leave a stale lock file (#1916). + _heldPlanningLocks.add(lockPath); + // Lock acquired — run the function try { return fn(); } finally { + _heldPlanningLocks.delete(lockPath); try { fs.unlinkSync(lockPath); } catch { /* already released */ } } } catch (err) { diff --git a/get-shit-done/bin/lib/state.cjs b/get-shit-done/bin/lib/state.cjs index ed270b4d..06da9d13 100644 --- a/get-shit-done/bin/lib/state.cjs +++ b/get-shit-done/bin/lib/state.cjs @@ -12,6 +12,16 @@ function getStatePath(cwd) { return planningPaths(cwd).state; } +// Track all lock files held by this process so they can be removed on exit. +// process.on('exit') fires even on process.exit(1), unlike try/finally which is +// skipped when error() calls process.exit(1) inside a locked region (#1916). +const _heldStateLocks = new Set(); +process.on('exit', () => { + for (const lockPath of _heldStateLocks) { + try { require('fs').unlinkSync(lockPath); } catch { /* already gone */ } + } +}); + // Shared helper: extract a field value from STATE.md content. // Supports both **Field:** bold and plain Field: format. function stateExtractField(content, fieldName) { @@ -184,18 +194,22 @@ function cmdStateUpdate(cwd, field, value) { const statePath = planningPaths(cwd).state; try { - let content = fs.readFileSync(statePath, 'utf-8'); - const fieldEscaped = escapeRegex(field); - // Try **Field:** bold format first, then plain Field: format - const boldPattern = new RegExp(`(\\*\\*${fieldEscaped}:\\*\\*\\s*)(.*)`, 'i'); - const plainPattern = new RegExp(`(^${fieldEscaped}:\\s*)(.*)`, 'im'); - if (boldPattern.test(content)) { - content = content.replace(boldPattern, (_match, prefix) => `${prefix}${value}`); - writeStateMd(statePath, content, cwd); - output({ updated: true }); - } else if (plainPattern.test(content)) { - content = content.replace(plainPattern, (_match, prefix) => `${prefix}${value}`); - writeStateMd(statePath, content, cwd); + let updated = false; + readModifyWriteStateMd(statePath, (content) => { + const fieldEscaped = escapeRegex(field); + // Try **Field:** bold format first, then plain Field: format + const boldPattern = new RegExp(`(\\*\\*${fieldEscaped}:\\*\\*\\s*)(.*)`, 'i'); + const plainPattern = new RegExp(`(^${fieldEscaped}:\\s*)(.*)`, 'im'); + if (boldPattern.test(content)) { + updated = true; + return content.replace(boldPattern, (_match, prefix) => `${prefix}${value}`); + } else if (plainPattern.test(content)) { + updated = true; + return content.replace(plainPattern, (_match, prefix) => `${prefix}${value}`); + } + return content; + }, cwd); + if (updated) { output({ updated: true }); } else { output({ updated: false, reason: `Field "${field}" not found in STATE.md` }); @@ -274,55 +288,67 @@ function cmdStateAdvancePlan(cwd, raw) { const statePath = planningPaths(cwd).state; if (!fs.existsSync(statePath)) { output({ error: 'STATE.md not found' }, raw); return; } - let content = fs.readFileSync(statePath, 'utf-8'); const today = new Date().toISOString().split('T')[0]; + let result = null; - // Try legacy separate fields first, then compound "Plan: X of Y" format - const legacyPlan = stateExtractField(content, 'Current Plan'); - const legacyTotal = stateExtractField(content, 'Total Plans in Phase'); - const planField = stateExtractField(content, 'Plan'); + readModifyWriteStateMd(statePath, (content) => { + // Try legacy separate fields first, then compound "Plan: X of Y" format + const legacyPlan = stateExtractField(content, 'Current Plan'); + const legacyTotal = stateExtractField(content, 'Total Plans in Phase'); + const planField = stateExtractField(content, 'Plan'); - let currentPlan, totalPlans; - let useCompoundFormat = false; + let currentPlan, totalPlans; + let useCompoundFormat = false; - if (legacyPlan && legacyTotal) { - currentPlan = parseInt(legacyPlan, 10); - totalPlans = parseInt(legacyTotal, 10); - } else if (planField) { - // Compound format: "2 of 6 in current phase" or "2 of 6" - currentPlan = parseInt(planField, 10); - const ofMatch = planField.match(/of\s+(\d+)/); - totalPlans = ofMatch ? parseInt(ofMatch[1], 10) : NaN; - useCompoundFormat = true; - } + if (legacyPlan && legacyTotal) { + currentPlan = parseInt(legacyPlan, 10); + totalPlans = parseInt(legacyTotal, 10); + } else if (planField) { + // Compound format: "2 of 6 in current phase" or "2 of 6" + currentPlan = parseInt(planField, 10); + const ofMatch = planField.match(/of\s+(\d+)/); + totalPlans = ofMatch ? parseInt(ofMatch[1], 10) : NaN; + useCompoundFormat = true; + } - if (isNaN(currentPlan) || isNaN(totalPlans)) { + if (isNaN(currentPlan) || isNaN(totalPlans)) { + result = { error: true }; + return content; + } + + if (currentPlan >= totalPlans) { + content = stateReplaceFieldWithFallback(content, 'Status', null, 'Phase complete — ready for verification'); + content = stateReplaceFieldWithFallback(content, 'Last Activity', 'Last activity', today); + content = updateCurrentPositionFields(content, { status: 'Phase complete — ready for verification', lastActivity: today }); + result = { advanced: false, reason: 'last_plan', current_plan: currentPlan, total_plans: totalPlans, status: 'ready_for_verification' }; + } else { + const newPlan = currentPlan + 1; + let planDisplayValue; + if (useCompoundFormat) { + // Preserve compound format: "X of Y in current phase" → replace X only + planDisplayValue = planField.replace(/^\d+/, String(newPlan)); + content = stateReplaceField(content, 'Plan', planDisplayValue) || content; + } else { + planDisplayValue = `${newPlan} of ${totalPlans}`; + content = stateReplaceField(content, 'Current Plan', String(newPlan)) || content; + } + content = stateReplaceFieldWithFallback(content, 'Status', null, 'Ready to execute'); + content = stateReplaceFieldWithFallback(content, 'Last Activity', 'Last activity', today); + content = updateCurrentPositionFields(content, { status: 'Ready to execute', lastActivity: today, plan: planDisplayValue }); + result = { advanced: true, previous_plan: currentPlan, current_plan: newPlan, total_plans: totalPlans }; + } + return content; + }, cwd); + + if (!result || result.error) { output({ error: 'Cannot parse Current Plan or Total Plans in Phase from STATE.md' }, raw); return; } - if (currentPlan >= totalPlans) { - content = stateReplaceFieldWithFallback(content, 'Status', null, 'Phase complete — ready for verification'); - content = stateReplaceFieldWithFallback(content, 'Last Activity', 'Last activity', today); - content = updateCurrentPositionFields(content, { status: 'Phase complete — ready for verification', lastActivity: today }); - writeStateMd(statePath, content, cwd); - output({ advanced: false, reason: 'last_plan', current_plan: currentPlan, total_plans: totalPlans, status: 'ready_for_verification' }, raw, 'false'); + if (result.advanced === false) { + output(result, raw, 'false'); } else { - const newPlan = currentPlan + 1; - let planDisplayValue; - if (useCompoundFormat) { - // Preserve compound format: "X of Y in current phase" → replace X only - planDisplayValue = planField.replace(/^\d+/, String(newPlan)); - content = stateReplaceField(content, 'Plan', planDisplayValue) || content; - } else { - planDisplayValue = `${newPlan} of ${totalPlans}`; - content = stateReplaceField(content, 'Current Plan', String(newPlan)) || content; - } - content = stateReplaceFieldWithFallback(content, 'Status', null, 'Ready to execute'); - content = stateReplaceFieldWithFallback(content, 'Last Activity', 'Last activity', today); - content = updateCurrentPositionFields(content, { status: 'Ready to execute', lastActivity: today, plan: planDisplayValue }); - writeStateMd(statePath, content, cwd); - output({ advanced: true, previous_plan: currentPlan, current_plan: newPlan, total_plans: totalPlans }, raw, 'true'); + output(result, raw, 'true'); } } @@ -330,7 +356,6 @@ function cmdStateRecordMetric(cwd, options, raw) { const statePath = planningPaths(cwd).state; if (!fs.existsSync(statePath)) { output({ error: 'STATE.md not found' }, raw); return; } - let content = fs.readFileSync(statePath, 'utf-8'); const { phase, plan, duration, tasks, files } = options; if (!phase || !plan || !duration) { @@ -338,22 +363,29 @@ function cmdStateRecordMetric(cwd, options, raw) { return; } - // Find Performance Metrics section and its table - const metricsPattern = /(##\s*Performance Metrics[\s\S]*?\n\|[^\n]+\n\|[-|\s]+\n)([\s\S]*?)(?=\n##|\n$|$)/i; - const metricsMatch = content.match(metricsPattern); + let recorded = false; + readModifyWriteStateMd(statePath, (content) => { + // Find Performance Metrics section and its table + const metricsPattern = /(##\s*Performance Metrics[\s\S]*?\n\|[^\n]+\n\|[-|\s]+\n)([\s\S]*?)(?=\n##|\n$|$)/i; + const metricsMatch = content.match(metricsPattern); - if (metricsMatch) { - let tableBody = metricsMatch[2].trimEnd(); - const newRow = `| Phase ${phase} P${plan} | ${duration} | ${tasks || '-'} tasks | ${files || '-'} files |`; + if (metricsMatch) { + let tableBody = metricsMatch[2].trimEnd(); + const newRow = `| Phase ${phase} P${plan} | ${duration} | ${tasks || '-'} tasks | ${files || '-'} files |`; - if (tableBody.trim() === '' || tableBody.includes('None yet')) { - tableBody = newRow; - } else { - tableBody = tableBody + '\n' + newRow; + if (tableBody.trim() === '' || tableBody.includes('None yet')) { + tableBody = newRow; + } else { + tableBody = tableBody + '\n' + newRow; + } + + recorded = true; + return content.replace(metricsPattern, (_match, header) => `${header}${tableBody}\n`); } + return content; + }, cwd); - content = content.replace(metricsPattern, (_match, header) => `${header}${tableBody}\n`); - writeStateMd(statePath, content, cwd); + if (recorded) { output({ recorded: true, phase, plan, duration }, raw, 'true'); } else { output({ recorded: false, reason: 'Performance Metrics section not found in STATE.md' }, raw, 'false'); @@ -364,9 +396,7 @@ function cmdStateUpdateProgress(cwd, raw) { const statePath = planningPaths(cwd).state; if (!fs.existsSync(statePath)) { output({ error: 'STATE.md not found' }, raw); return; } - let content = fs.readFileSync(statePath, 'utf-8'); - - // Count summaries across current milestone phases only + // Count summaries across current milestone phases only (outside lock — read-only) const phasesDir = planningPaths(cwd).phases; let totalPlans = 0; let totalSummaries = 0; @@ -389,17 +419,26 @@ function cmdStateUpdateProgress(cwd, raw) { const bar = '\u2588'.repeat(filled) + '\u2591'.repeat(barWidth - filled); const progressStr = `[${bar}] ${percent}%`; - // Try **Progress:** bold format first, then plain Progress: format - const boldProgressPattern = /(\*\*Progress:\*\*\s*).*/i; - const plainProgressPattern = /^(Progress:\s*).*/im; - if (boldProgressPattern.test(content)) { - content = content.replace(boldProgressPattern, (_match, prefix) => `${prefix}${progressStr}`); - writeStateMd(statePath, content, cwd); - output({ updated: true, percent, completed: totalSummaries, total: totalPlans, bar: progressStr }, raw, progressStr); - } else if (plainProgressPattern.test(content)) { - content = content.replace(plainProgressPattern, (_match, prefix) => `${prefix}${progressStr}`); - writeStateMd(statePath, content, cwd); - output({ updated: true, percent, completed: totalSummaries, total: totalPlans, bar: progressStr }, raw, progressStr); + let updated = false; + const _totalPlans = totalPlans; + const _totalSummaries = totalSummaries; + + readModifyWriteStateMd(statePath, (content) => { + // Try **Progress:** bold format first, then plain Progress: format + const boldProgressPattern = /(\*\*Progress:\*\*\s*).*/i; + const plainProgressPattern = /^(Progress:\s*).*/im; + if (boldProgressPattern.test(content)) { + updated = true; + return content.replace(boldProgressPattern, (_match, prefix) => `${prefix}${progressStr}`); + } else if (plainProgressPattern.test(content)) { + updated = true; + return content.replace(plainProgressPattern, (_match, prefix) => `${prefix}${progressStr}`); + } + return content; + }, cwd); + + if (updated) { + output({ updated: true, percent, completed: _totalSummaries, total: _totalPlans, bar: progressStr }, raw, progressStr); } else { output({ updated: false, reason: 'Progress field not found in STATE.md' }, raw, 'false'); } @@ -423,20 +462,26 @@ function cmdStateAddDecision(cwd, options, raw) { if (!summaryText) { output({ error: 'summary required' }, raw); return; } - let content = fs.readFileSync(statePath, 'utf-8'); const entry = `- [Phase ${phase || '?'}]: ${summaryText}${rationaleText ? ` — ${rationaleText}` : ''}`; + let added = false; - // Find Decisions section (various heading patterns) - const sectionPattern = /(###?\s*(?:Decisions|Decisions Made|Accumulated.*Decisions)\s*\n)([\s\S]*?)(?=\n###?|\n##[^#]|$)/i; - const match = content.match(sectionPattern); + readModifyWriteStateMd(statePath, (content) => { + // Find Decisions section (various heading patterns) + const sectionPattern = /(###?\s*(?:Decisions|Decisions Made|Accumulated.*Decisions)\s*\n)([\s\S]*?)(?=\n###?|\n##[^#]|$)/i; + const match = content.match(sectionPattern); - if (match) { - let sectionBody = match[2]; - // Remove placeholders - sectionBody = sectionBody.replace(/None yet\.?\s*\n?/gi, '').replace(/No decisions yet\.?\s*\n?/gi, ''); - sectionBody = sectionBody.trimEnd() + '\n' + entry + '\n'; - content = content.replace(sectionPattern, (_match, header) => `${header}${sectionBody}`); - writeStateMd(statePath, content, cwd); + if (match) { + let sectionBody = match[2]; + // Remove placeholders + sectionBody = sectionBody.replace(/None yet\.?\s*\n?/gi, '').replace(/No decisions yet\.?\s*\n?/gi, ''); + sectionBody = sectionBody.trimEnd() + '\n' + entry + '\n'; + added = true; + return content.replace(sectionPattern, (_match, header) => `${header}${sectionBody}`); + } + return content; + }, cwd); + + if (added) { output({ added: true, decision: entry }, raw, 'true'); } else { output({ added: false, reason: 'Decisions section not found in STATE.md' }, raw, 'false'); @@ -458,18 +503,24 @@ function cmdStateAddBlocker(cwd, text, raw) { if (!blockerText) { output({ error: 'text required' }, raw); return; } - let content = fs.readFileSync(statePath, 'utf-8'); const entry = `- ${blockerText}`; + let added = false; - const sectionPattern = /(###?\s*(?:Blockers|Blockers\/Concerns|Concerns)\s*\n)([\s\S]*?)(?=\n###?|\n##[^#]|$)/i; - const match = content.match(sectionPattern); + readModifyWriteStateMd(statePath, (content) => { + const sectionPattern = /(###?\s*(?:Blockers|Blockers\/Concerns|Concerns)\s*\n)([\s\S]*?)(?=\n###?|\n##[^#]|$)/i; + const match = content.match(sectionPattern); - if (match) { - let sectionBody = match[2]; - sectionBody = sectionBody.replace(/None\.?\s*\n?/gi, '').replace(/None yet\.?\s*\n?/gi, ''); - sectionBody = sectionBody.trimEnd() + '\n' + entry + '\n'; - content = content.replace(sectionPattern, (_match, header) => `${header}${sectionBody}`); - writeStateMd(statePath, content, cwd); + if (match) { + let sectionBody = match[2]; + sectionBody = sectionBody.replace(/None\.?\s*\n?/gi, '').replace(/None yet\.?\s*\n?/gi, ''); + sectionBody = sectionBody.trimEnd() + '\n' + entry + '\n'; + added = true; + return content.replace(sectionPattern, (_match, header) => `${header}${sectionBody}`); + } + return content; + }, cwd); + + if (added) { output({ added: true, blocker: blockerText }, raw, 'true'); } else { output({ added: false, reason: 'Blockers section not found in STATE.md' }, raw, 'false'); @@ -481,27 +532,33 @@ function cmdStateResolveBlocker(cwd, text, raw) { if (!fs.existsSync(statePath)) { output({ error: 'STATE.md not found' }, raw); return; } if (!text) { output({ error: 'text required' }, raw); return; } - let content = fs.readFileSync(statePath, 'utf-8'); + let resolved = false; - const sectionPattern = /(###?\s*(?:Blockers|Blockers\/Concerns|Concerns)\s*\n)([\s\S]*?)(?=\n###?|\n##[^#]|$)/i; - const match = content.match(sectionPattern); + readModifyWriteStateMd(statePath, (content) => { + const sectionPattern = /(###?\s*(?:Blockers|Blockers\/Concerns|Concerns)\s*\n)([\s\S]*?)(?=\n###?|\n##[^#]|$)/i; + const match = content.match(sectionPattern); - if (match) { - const sectionBody = match[2]; - const lines = sectionBody.split('\n'); - const filtered = lines.filter(line => { - if (!line.startsWith('- ')) return true; - return !line.toLowerCase().includes(text.toLowerCase()); - }); + if (match) { + const sectionBody = match[2]; + const lines = sectionBody.split('\n'); + const filtered = lines.filter(line => { + if (!line.startsWith('- ')) return true; + return !line.toLowerCase().includes(text.toLowerCase()); + }); - let newBody = filtered.join('\n'); - // If section is now empty, add placeholder - if (!newBody.trim() || !newBody.includes('- ')) { - newBody = 'None\n'; + let newBody = filtered.join('\n'); + // If section is now empty, add placeholder + if (!newBody.trim() || !newBody.includes('- ')) { + newBody = 'None\n'; + } + + resolved = true; + return content.replace(sectionPattern, (_match, header) => `${header}${newBody}`); } + return content; + }, cwd); - content = content.replace(sectionPattern, (_match, header) => `${header}${newBody}`); - writeStateMd(statePath, content, cwd); + if (resolved) { output({ resolved: true, blocker: text }, raw, 'true'); } else { output({ resolved: false, reason: 'Blockers section not found in STATE.md' }, raw, 'false'); @@ -512,31 +569,33 @@ function cmdStateRecordSession(cwd, options, raw) { const statePath = planningPaths(cwd).state; if (!fs.existsSync(statePath)) { output({ error: 'STATE.md not found' }, raw); return; } - let content = fs.readFileSync(statePath, 'utf-8'); const now = new Date().toISOString(); const updated = []; - // Update Last session / Last Date - let result = stateReplaceField(content, 'Last session', now); - if (result) { content = result; updated.push('Last session'); } - result = stateReplaceField(content, 'Last Date', now); - if (result) { content = result; updated.push('Last Date'); } + readModifyWriteStateMd(statePath, (content) => { + // Update Last session / Last Date + let result = stateReplaceField(content, 'Last session', now); + if (result) { content = result; updated.push('Last session'); } + result = stateReplaceField(content, 'Last Date', now); + if (result) { content = result; updated.push('Last Date'); } - // Update Stopped at - if (options.stopped_at) { - result = stateReplaceField(content, 'Stopped At', options.stopped_at); - if (!result) result = stateReplaceField(content, 'Stopped at', options.stopped_at); - if (result) { content = result; updated.push('Stopped At'); } - } + // Update Stopped at + if (options.stopped_at) { + result = stateReplaceField(content, 'Stopped At', options.stopped_at); + if (!result) result = stateReplaceField(content, 'Stopped at', options.stopped_at); + if (result) { content = result; updated.push('Stopped At'); } + } - // Update Resume file - const resumeFile = options.resume_file || 'None'; - result = stateReplaceField(content, 'Resume File', resumeFile); - if (!result) result = stateReplaceField(content, 'Resume file', resumeFile); - if (result) { content = result; updated.push('Resume File'); } + // Update Resume file + const resumeFile = options.resume_file || 'None'; + result = stateReplaceField(content, 'Resume File', resumeFile); + if (!result) result = stateReplaceField(content, 'Resume file', resumeFile); + if (result) { content = result; updated.push('Resume File'); } + + return content; + }, cwd); if (updated.length > 0) { - writeStateMd(statePath, content, cwd); output({ recorded: true, updated }, raw, 'true'); } else { output({ recorded: false, reason: 'No session fields found in STATE.md' }, raw, 'false'); @@ -805,6 +864,9 @@ function acquireStateLock(statePath) { const fd = fs.openSync(lockPath, fs.constants.O_CREAT | fs.constants.O_EXCL | fs.constants.O_WRONLY); fs.writeSync(fd, String(process.pid)); fs.closeSync(fd); + // Register for exit-time cleanup so process.exit(1) inside a locked region + // cannot leave a stale lock file (#1916). + _heldStateLocks.add(lockPath); return lockPath; } catch (err) { if (err.code === 'EEXIST') { @@ -821,8 +883,7 @@ function acquireStateLock(statePath) { return lockPath; } const jitter = Math.floor(Math.random() * 50); - const start = Date.now(); - while (Date.now() - start < retryDelay + jitter) { /* busy wait */ } + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, retryDelay + jitter); continue; } return lockPath; // non-EEXIST error — proceed without lock @@ -832,6 +893,7 @@ function acquireStateLock(statePath) { } function releaseStateLock(lockPath) { + _heldStateLocks.delete(lockPath); try { fs.unlinkSync(lockPath); } catch { /* lock already gone */ } } @@ -913,96 +975,95 @@ function cmdStateBeginPhase(cwd, phaseNumber, phaseName, planCount, raw) { return; } - let content = fs.readFileSync(statePath, 'utf-8'); const today = new Date().toISOString().split('T')[0]; const updated = []; - // Update Status field - const statusValue = `Executing Phase ${phaseNumber}`; - let result = stateReplaceField(content, 'Status', statusValue); - if (result) { content = result; updated.push('Status'); } + readModifyWriteStateMd(statePath, (content) => { + // Update Status field + const statusValue = `Executing Phase ${phaseNumber}`; + let result = stateReplaceField(content, 'Status', statusValue); + if (result) { content = result; updated.push('Status'); } - // Update Last Activity - result = stateReplaceField(content, 'Last Activity', today); - if (result) { content = result; updated.push('Last Activity'); } + // Update Last Activity + result = stateReplaceField(content, 'Last Activity', today); + if (result) { content = result; updated.push('Last Activity'); } - // Update Last Activity Description if it exists - const activityDesc = `Phase ${phaseNumber} execution started`; - result = stateReplaceField(content, 'Last Activity Description', activityDesc); - if (result) { content = result; updated.push('Last Activity Description'); } + // Update Last Activity Description if it exists + const activityDesc = `Phase ${phaseNumber} execution started`; + result = stateReplaceField(content, 'Last Activity Description', activityDesc); + if (result) { content = result; updated.push('Last Activity Description'); } - // Update Current Phase - result = stateReplaceField(content, 'Current Phase', String(phaseNumber)); - if (result) { content = result; updated.push('Current Phase'); } + // Update Current Phase + result = stateReplaceField(content, 'Current Phase', String(phaseNumber)); + if (result) { content = result; updated.push('Current Phase'); } - // Update Current Phase Name - if (phaseName) { - result = stateReplaceField(content, 'Current Phase Name', phaseName); - if (result) { content = result; updated.push('Current Phase Name'); } - } - - // Update Current Plan to 1 (starting from the first plan) - result = stateReplaceField(content, 'Current Plan', '1'); - if (result) { content = result; updated.push('Current Plan'); } - - // Update Total Plans in Phase - if (planCount) { - result = stateReplaceField(content, 'Total Plans in Phase', String(planCount)); - if (result) { content = result; updated.push('Total Plans in Phase'); } - } - - // Update **Current focus:** body text line (#1104) - const focusLabel = phaseName ? `Phase ${phaseNumber} — ${phaseName}` : `Phase ${phaseNumber}`; - const focusPattern = /(\*\*Current focus:\*\*\s*).*/i; - if (focusPattern.test(content)) { - content = content.replace(focusPattern, (_match, prefix) => `${prefix}${focusLabel}`); - updated.push('Current focus'); - } - - // Update ## Current Position section (#1104, #1365) - // Update individual fields within Current Position instead of replacing the - // entire section, so that Status, Last activity, and Progress are preserved. - const positionPattern = /(##\s*Current Position\s*\n)([\s\S]*?)(?=\n##|$)/i; - const positionMatch = content.match(positionPattern); - if (positionMatch) { - const header = positionMatch[1]; - let posBody = positionMatch[2]; - - // Update or insert Phase line - const newPhase = `Phase: ${phaseNumber}${phaseName ? ` (${phaseName})` : ''} — EXECUTING`; - if (/^Phase:/m.test(posBody)) { - posBody = posBody.replace(/^Phase:.*$/m, newPhase); - } else { - posBody = newPhase + '\n' + posBody; + // Update Current Phase Name + if (phaseName) { + result = stateReplaceField(content, 'Current Phase Name', phaseName); + if (result) { content = result; updated.push('Current Phase Name'); } } - // Update or insert Plan line - const newPlan = `Plan: 1 of ${planCount || '?'}`; - if (/^Plan:/m.test(posBody)) { - posBody = posBody.replace(/^Plan:.*$/m, newPlan); - } else { - posBody = posBody.replace(/^(Phase:.*$)/m, `$1\n${newPlan}`); + // Update Current Plan to 1 (starting from the first plan) + result = stateReplaceField(content, 'Current Plan', '1'); + if (result) { content = result; updated.push('Current Plan'); } + + // Update Total Plans in Phase + if (planCount) { + result = stateReplaceField(content, 'Total Plans in Phase', String(planCount)); + if (result) { content = result; updated.push('Total Plans in Phase'); } } - // Update Status line if present - const newStatus = `Status: Executing Phase ${phaseNumber}`; - if (/^Status:/m.test(posBody)) { - posBody = posBody.replace(/^Status:.*$/m, newStatus); + // Update **Current focus:** body text line (#1104) + const focusLabel = phaseName ? `Phase ${phaseNumber} — ${phaseName}` : `Phase ${phaseNumber}`; + const focusPattern = /(\*\*Current focus:\*\*\s*).*/i; + if (focusPattern.test(content)) { + content = content.replace(focusPattern, (_match, prefix) => `${prefix}${focusLabel}`); + updated.push('Current focus'); } - // Update Last activity line if present - const newActivity = `Last activity: ${today} -- Phase ${phaseNumber} execution started`; - if (/^Last activity:/im.test(posBody)) { - posBody = posBody.replace(/^Last activity:.*$/im, newActivity); + // Update ## Current Position section (#1104, #1365) + // Update individual fields within Current Position instead of replacing the + // entire section, so that Status, Last activity, and Progress are preserved. + const positionPattern = /(##\s*Current Position\s*\n)([\s\S]*?)(?=\n##|$)/i; + const positionMatch = content.match(positionPattern); + if (positionMatch) { + const header = positionMatch[1]; + let posBody = positionMatch[2]; + + // Update or insert Phase line + const newPhase = `Phase: ${phaseNumber}${phaseName ? ` (${phaseName})` : ''} — EXECUTING`; + if (/^Phase:/m.test(posBody)) { + posBody = posBody.replace(/^Phase:.*$/m, newPhase); + } else { + posBody = newPhase + '\n' + posBody; + } + + // Update or insert Plan line + const newPlan = `Plan: 1 of ${planCount || '?'}`; + if (/^Plan:/m.test(posBody)) { + posBody = posBody.replace(/^Plan:.*$/m, newPlan); + } else { + posBody = posBody.replace(/^(Phase:.*$)/m, `$1\n${newPlan}`); + } + + // Update Status line if present + const newStatus = `Status: Executing Phase ${phaseNumber}`; + if (/^Status:/m.test(posBody)) { + posBody = posBody.replace(/^Status:.*$/m, newStatus); + } + + // Update Last activity line if present + const newActivity = `Last activity: ${today} -- Phase ${phaseNumber} execution started`; + if (/^Last activity:/im.test(posBody)) { + posBody = posBody.replace(/^Last activity:.*$/im, newActivity); + } + + content = content.replace(positionPattern, `${header}${posBody}`); + updated.push('Current Position'); } - content = content.replace(positionPattern, `${header}${posBody}`); - updated.push('Current Position'); - } - - if (updated.length > 0) { - writeStateMd(statePath, content, cwd); - } + return content; + }, cwd); output({ updated, phase: phaseNumber, phase_name: phaseName || null, plan_count: planCount || null }, raw, updated.length > 0 ? 'true' : 'false'); } diff --git a/tests/locking-bugs-1909-1916-1925-1927.test.cjs b/tests/locking-bugs-1909-1916-1925-1927.test.cjs new file mode 100644 index 00000000..df4b7afe --- /dev/null +++ b/tests/locking-bugs-1909-1916-1925-1927.test.cjs @@ -0,0 +1,377 @@ +/** + * Regression tests for locking bugs #1909, #1916, #1925, #1927. + * + * These tests are written FIRST (TDD) — they must fail before the fixes are applied + * and pass after. + * + * #1909 — CPU-burning busy-wait in acquireStateLock + * #1916 — Lock files persist after process.exit() + * #1925 — TOCTOU races in 8 state commands (read outside lock) + * #1927 — config.json has no locking in setConfigValue + */ + +const { test, describe, beforeEach, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); +const os = require('os'); +const { execFileSync, execSync } = require('child_process'); +const { promisify } = require('util'); +const { exec } = require('child_process'); + +const { runGsdTools, createTempProject, cleanup, TOOLS_PATH } = require('./helpers.cjs'); + +const execAsync = promisify(exec); + +// ───────────────────────────────────────────────────────────────────────────── +// Helpers +// ───────────────────────────────────────────────────────────────────────────── + +function writeStateMd(tmpDir, content) { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + content, + 'utf-8' + ); +} + +function readStateMd(tmpDir) { + return fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); +} + +function writeConfig(tmpDir, obj) { + const configPath = path.join(tmpDir, '.planning', 'config.json'); + fs.writeFileSync(configPath, JSON.stringify(obj, null, 2), 'utf-8'); +} + +function readConfig(tmpDir) { + const configPath = path.join(tmpDir, '.planning', 'config.json'); + return JSON.parse(fs.readFileSync(configPath, 'utf-8')); +} + +// ───────────────────────────────────────────────────────────────────────────── +// #1909 — CPU-burning busy-wait in acquireStateLock +// Verify the implementation uses Atomics.wait (not a while-loop spin). +// ───────────────────────────────────────────────────────────────────────────── + +describe('#1909 acquireStateLock: no CPU-burning busy-wait', () => { + test('acquireStateLock source code uses Atomics.wait, not a spin-loop', () => { + const stateSrc = fs.readFileSync( + path.join(__dirname, '..', 'get-shit-done', 'bin', 'lib', 'state.cjs'), + 'utf-8' + ); + + // The bug: spin-loop pattern in acquireStateLock + // The fix: use Atomics.wait() for cross-platform sleep, matching withPlanningLock in core.cjs + const spinLoopPattern = /while\s*\(Date\.now\(\)\s*-\s*start\s*<\s*\w+\)\s*\{\s*(?:\/\*[^*]*\*\/)?\s*\}/; + + // Find the acquireStateLock function text + const fnStart = stateSrc.indexOf('function acquireStateLock('); + assert.ok(fnStart !== -1, 'acquireStateLock function must exist'); + + // Extract ~50 lines after the function start to cover the retry logic + const fnSnippet = stateSrc.slice(fnStart, fnStart + 2000); + + assert.ok( + !spinLoopPattern.test(fnSnippet), + 'acquireStateLock must not use a CPU-burning spin-loop (while Date.now()-start < delay)' + ); + + assert.ok( + fnSnippet.includes('Atomics.wait'), + 'acquireStateLock must use Atomics.wait() for sleeping, matching withPlanningLock in core.cjs' + ); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// #1916 — Lock files persist after process.exit() +// Verify that the STATE.md.lock file is removed even when process.exit() is called +// while the lock is held (e.g., via error() inside a locked region). +// ───────────────────────────────────────────────────────────────────────────── + +describe('#1916 lock cleanup on process.exit()', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('STATE.md.lock is removed after a command exits with an error', () => { + // Intentionally trigger an error path: state update with missing STATE.md leaves + // no lock behind (the read-before-lock path returns gracefully, but let's verify + // a command that holds the lock can't accidentally leave the file). + writeStateMd(tmpDir, [ + '# Project State', + '', + '**Status:** Planning', + '**Current Phase:** 01', + ].join('\n') + '\n'); + + // Run a state update — even if it fails, the lock must not remain + runGsdTools('state update Status "In progress"', tmpDir); + + const lockPath = path.join(tmpDir, '.planning', 'STATE.md.lock'); + assert.ok( + !fs.existsSync(lockPath), + 'STATE.md.lock must not persist after any state command terminates' + ); + }); + + test('STATE.md.lock module-level cleanup set is present in source', () => { + // Verify the fix: module-level Set tracks held locks and process.on('exit') cleans them up. + const stateSrc = fs.readFileSync( + path.join(__dirname, '..', 'get-shit-done', 'bin', 'lib', 'state.cjs'), + 'utf-8' + ); + + assert.ok( + stateSrc.includes("process.on('exit'"), + "state.cjs must register process.on('exit', ...) to clean up held lock files" + ); + }); + + test('core.cjs .planning/.lock is removed after a command exits with an error', () => { + // The withPlanningLock in core.cjs also needs exit cleanup. + const coreSrc = fs.readFileSync( + path.join(__dirname, '..', 'get-shit-done', 'bin', 'lib', 'core.cjs'), + 'utf-8' + ); + + assert.ok( + coreSrc.includes("process.on('exit'"), + "core.cjs must register process.on('exit', ...) to clean up held planning lock files" + ); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// #1925 — TOCTOU races in 8 state commands +// Each of the 8 commands reads STATE.md outside the lock, then calls writeStateMd +// (which only locks the write). Two concurrent callers reading the same content +// means the second write clobbers the first. +// +// Fix: migrate all 8 to use readModifyWriteStateMd(). +// Test: call the same command twice concurrently on SEPARATE fields and verify +// both updates survive. +// ───────────────────────────────────────────────────────────────────────────── + +describe('#1925 TOCTOU: state commands use readModifyWriteStateMd', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('state update: both concurrent updates to different fields survive', async () => { + writeStateMd(tmpDir, [ + '# Project State', + '', + '**Status:** Planning', + '**Current Phase:** 01', + '**Current Plan:** 01-01', + '**Last Activity:** 2024-01-01', + ].join('\n') + '\n'); + + const nodeBin = process.execPath; + const cmdA = `"${nodeBin}" "${TOOLS_PATH}" state update Status Executing --cwd "${tmpDir}"`; + const cmdB = `"${nodeBin}" "${TOOLS_PATH}" state update "Current Phase" 02 --cwd "${tmpDir}"`; + + await Promise.all([ + execAsync(cmdA, { encoding: 'utf-8' }).catch(() => {}), + execAsync(cmdB, { encoding: 'utf-8' }).catch(() => {}), + ]); + + const content = readStateMd(tmpDir); + assert.ok( + content.includes('Executing') && content.includes('02'), + 'Both concurrent state update commands must survive (TOCTOU bug: second write clobbers first).\n' + + 'Content:\n' + content + ); + }); + + test('state add-decision: both concurrent calls append different decisions', async () => { + writeStateMd(tmpDir, [ + '# Project State', + '', + '**Current Phase:** 01', + '**Status:** Planning', + '', + '### Decisions', + 'None yet.', + ].join('\n') + '\n'); + + const nodeBin = process.execPath; + const cmdA = `"${nodeBin}" "${TOOLS_PATH}" state add-decision --phase 01 --summary "Use TypeScript" --cwd "${tmpDir}"`; + const cmdB = `"${nodeBin}" "${TOOLS_PATH}" state add-decision --phase 01 --summary "Use PostgreSQL" --cwd "${tmpDir}"`; + + await Promise.all([ + execAsync(cmdA, { encoding: 'utf-8' }).catch(() => {}), + execAsync(cmdB, { encoding: 'utf-8' }).catch(() => {}), + ]); + + const content = readStateMd(tmpDir); + assert.ok( + content.includes('Use TypeScript') && content.includes('Use PostgreSQL'), + 'Both concurrent add-decision calls must survive.\n' + + 'Content:\n' + content + ); + }); + + test('state add-blocker: both concurrent calls append different blockers', async () => { + writeStateMd(tmpDir, [ + '# Project State', + '', + '**Current Phase:** 01', + '', + '### Blockers', + 'None.', + ].join('\n') + '\n'); + + const nodeBin = process.execPath; + const cmdA = `"${nodeBin}" "${TOOLS_PATH}" state add-blocker --text "Need API credentials" --cwd "${tmpDir}"`; + const cmdB = `"${nodeBin}" "${TOOLS_PATH}" state add-blocker --text "Waiting for design review" --cwd "${tmpDir}"`; + + await Promise.all([ + execAsync(cmdA, { encoding: 'utf-8' }).catch(() => {}), + execAsync(cmdB, { encoding: 'utf-8' }).catch(() => {}), + ]); + + const content = readStateMd(tmpDir); + assert.ok( + content.includes('Need API credentials') && content.includes('Waiting for design review'), + 'Both concurrent add-blocker calls must survive.\n' + + 'Content:\n' + content + ); + }); + + test('state commands use readModifyWriteStateMd (source audit)', () => { + const stateSrc = fs.readFileSync( + path.join(__dirname, '..', 'get-shit-done', 'bin', 'lib', 'state.cjs'), + 'utf-8' + ); + + // Each of these functions should NOT contain a bare `fs.readFileSync(...STATE.md...)` + // followed by a `writeStateMd` — they should use readModifyWriteStateMd instead. + // + // We verify this by checking that within the function body we do NOT see the + // TOCTOU pattern: `let content = fs.readFileSync(statePath` (old pattern) + // while also calling `writeStateMd` — except wrapped in readModifyWrite. + + const affectedFunctions = [ + 'cmdStateUpdate', + 'cmdStateAdvancePlan', + 'cmdStateRecordMetric', + 'cmdStateUpdateProgress', + 'cmdStateAddDecision', + 'cmdStateAddBlocker', + 'cmdStateResolveBlocker', + 'cmdStateRecordSession', + 'cmdStateBeginPhase', + ]; + + for (const fnName of affectedFunctions) { + // Find the function in source + const fnIdx = stateSrc.indexOf(`function ${fnName}(`); + assert.ok(fnIdx !== -1, `${fnName} must exist in state.cjs`); + + // Grab the function body (rough heuristic: up to the next top-level function) + const bodyStart = stateSrc.indexOf('{', fnIdx); + // Find end by tracking braces + let depth = 0; + let bodyEnd = bodyStart; + for (let i = bodyStart; i < stateSrc.length; i++) { + if (stateSrc[i] === '{') depth++; + else if (stateSrc[i] === '}') { + depth--; + if (depth === 0) { bodyEnd = i; break; } + } + } + const fnBody = stateSrc.slice(fnIdx, bodyEnd + 1); + + // The function must call readModifyWriteStateMd + assert.ok( + fnBody.includes('readModifyWriteStateMd'), + `${fnName} must use readModifyWriteStateMd() to prevent TOCTOU races` + ); + + // The function must NOT have bare readFileSync for statePath outside the lambda + // (the readFileSync inside readModifyWrite's lambda is fine — that's inside the lock) + // We check for the pre-fix pattern: `let content = fs.readFileSync(statePath` + assert.ok( + !fnBody.match(/let content\s*=\s*fs\.readFileSync\s*\(\s*statePath/), + `${fnName} must not read STATE.md with fs.readFileSync outside readModifyWriteStateMd (TOCTOU)` + ); + } + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// #1927 — config.json has no locking in setConfigValue +// setConfigValue does read-modify-write on config.json without holding any lock. +// Fix: wrap in withPlanningLock. +// ───────────────────────────────────────────────────────────────────────────── + +describe('#1927 config.json: setConfigValue must hold planning lock', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('both concurrent config-set calls persist their values', async () => { + writeConfig(tmpDir, { + model_profile: 'balanced', + workflow: { + research: true, + plan_check: true, + }, + }); + + const nodeBin = process.execPath; + const cmdA = `"${nodeBin}" "${TOOLS_PATH}" config-set model_profile quality --cwd "${tmpDir}"`; + const cmdB = `"${nodeBin}" "${TOOLS_PATH}" config-set workflow.research false --cwd "${tmpDir}"`; + + await Promise.all([ + execAsync(cmdA, { encoding: 'utf-8' }).catch(() => {}), + execAsync(cmdB, { encoding: 'utf-8' }).catch(() => {}), + ]); + + const config = readConfig(tmpDir); + assert.strictEqual( + config.model_profile, + 'quality', + 'config-set model_profile must survive concurrent write' + ); + assert.strictEqual( + config.workflow?.research, + false, + 'config-set workflow.research must survive concurrent write' + ); + }); + + test('config.cjs setConfigValue uses withPlanningLock (source audit)', () => { + const configSrc = fs.readFileSync( + path.join(__dirname, '..', 'get-shit-done', 'bin', 'lib', 'config.cjs'), + 'utf-8' + ); + + // setConfigValue must import/use withPlanningLock + assert.ok( + configSrc.includes('withPlanningLock'), + 'config.cjs must use withPlanningLock in setConfigValue to prevent concurrent write data loss' + ); + }); +});