mirror of
https://github.com/glittercowboy/get-shit-done
synced 2026-04-25 17:25:23 +02:00
fix(state): correct TOCTOU races, busy-wait, lock cleanup, and config locking (#1944)
cmdStateUpdateProgress, cmdStateAddDecision, cmdStateAddBlocker,
cmdStateResolveBlocker, cmdStateRecordSession, and cmdStateBeginPhase from
bare readFileSync+writeStateMd to readModifyWriteStateMd, eliminating the
TOCTOU window where two concurrent callers read the same content and the
second write clobbers the first.
Atomics.wait(), matching the pattern already used in withPlanningLock in
core.cjs.
and core.cjs and register a process.on('exit') handler to unlink them on
process exit. The exit event fires even when process.exit(1) is called
inside a locked region, eliminating stale lock files after errors.
read-modify-write body of setConfigValue in a planning lock, preventing
concurrent config-set calls from losing each other's writes.
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
@@ -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);
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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');
|
||||
}
|
||||
|
||||
377
tests/locking-bugs-1909-1916-1925-1927.test.cjs
Normal file
377
tests/locking-bugs-1909-1916-1925-1927.test.cjs
Normal file
@@ -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'
|
||||
);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user