mirror of
https://github.com/glittercowboy/get-shit-done
synced 2026-04-25 17:25:23 +02:00
refactor: consolidate STATE.md field helpers, fix command injection in isGitIgnored
Refactoring: - Extract stateReplaceFieldWithFallback() to state.cjs as single source of truth for the try-primary-then-fallback pattern that was duplicated inline across phase.cjs, milestone.cjs, and state.cjs - Replace all inline bold-only regex patterns in cmdPhaseComplete with shared stateReplaceField/stateExtractField helpers — now supports both **Bold:** and plain Field: STATE.md formats (fixes the same bug as #924) - Replace inline bold-only regex patterns in cmdMilestoneComplete with shared helpers - Replace inline bold-only regex for Completed Phases/Total Phases/Progress counters in cmdPhaseComplete with stateExtractField/stateReplaceField - Replace inline bold-only Total Phases regex in cmdPhaseRemove with shared helpers Security: - Fix command injection surface in isGitIgnored (core.cjs): replace execSync with string concatenation with execFileSync using array arguments — prevents shell interpretation of special characters in file paths Tests (7 new): - 5 tests for stateReplaceFieldWithFallback: primary field, fallback, neither, preference, and plain format - 1 regression test: phase complete with plain-format STATE.md fields - 1 regression test: milestone complete with plain-format STATE.md fields 854 tests pass (was 847). No behavioral regressions.
This commit is contained in:
Tom Boucher
@@ -4,7 +4,7 @@
|
||||
|
||||
const fs = require('fs');
|
||||
const path = require('path');
|
||||
const { execSync, spawnSync } = require('child_process');
|
||||
const { execSync, execFileSync, spawnSync } = require('child_process');
|
||||
const { MODEL_PROFILES } = require('./model-profiles.cjs');
|
||||
|
||||
// ─── Path helpers ────────────────────────────────────────────────────────────
|
||||
@@ -131,7 +131,9 @@ function isGitIgnored(cwd, targetPath) {
|
||||
// Without it, git check-ignore returns "not ignored" for tracked files even when
|
||||
// .gitignore explicitly lists them — a common source of confusion when .planning/
|
||||
// was committed before being added to .gitignore.
|
||||
execSync('git check-ignore -q --no-index -- ' + targetPath.replace(/[^a-zA-Z0-9._\-/]/g, ''), {
|
||||
// Use execFileSync (array args) to prevent shell interpretation of special characters
|
||||
// in file paths — avoids command injection via crafted path names.
|
||||
execFileSync('git', ['check-ignore', '-q', '--no-index', '--', targetPath], {
|
||||
cwd,
|
||||
stdio: 'pipe',
|
||||
});
|
||||
|
||||
@@ -6,7 +6,7 @@ const fs = require('fs');
|
||||
const path = require('path');
|
||||
const { escapeRegex, getMilestonePhaseFilter, extractOneLinerFromBody, normalizeMd, planningPaths, output, error } = require('./core.cjs');
|
||||
const { extractFrontmatter } = require('./frontmatter.cjs');
|
||||
const { writeStateMd } = require('./state.cjs');
|
||||
const { writeStateMd, stateReplaceFieldWithFallback } = require('./state.cjs');
|
||||
|
||||
function cmdRequirementsMarkComplete(cwd, reqIdsRaw, raw) {
|
||||
if (!reqIdsRaw || reqIdsRaw.length === 0) {
|
||||
@@ -194,21 +194,15 @@ function cmdMilestoneComplete(cwd, version, options, raw) {
|
||||
fs.writeFileSync(milestonesPath, normalizeMd(`# Milestones\n\n${milestoneEntry}`), 'utf-8');
|
||||
}
|
||||
|
||||
// Update STATE.md
|
||||
// Update STATE.md — use shared helpers that handle both **bold:** and plain Field: formats
|
||||
if (fs.existsSync(statePath)) {
|
||||
let stateContent = fs.readFileSync(statePath, 'utf-8');
|
||||
stateContent = stateContent.replace(
|
||||
/(\*\*Status:\*\*\s*).*/,
|
||||
`$1${version} milestone complete`
|
||||
);
|
||||
stateContent = stateContent.replace(
|
||||
/(\*\*Last Activity:\*\*\s*).*/,
|
||||
`$1${today}`
|
||||
);
|
||||
stateContent = stateContent.replace(
|
||||
/(\*\*Last Activity Description:\*\*\s*).*/,
|
||||
`$1${version} milestone completed and archived`
|
||||
);
|
||||
|
||||
stateContent = stateReplaceFieldWithFallback(stateContent, 'Status', null, `${version} milestone complete`);
|
||||
stateContent = stateReplaceFieldWithFallback(stateContent, 'Last Activity', 'Last activity', today);
|
||||
stateContent = stateReplaceFieldWithFallback(stateContent, 'Last Activity Description', null,
|
||||
`${version} milestone completed and archived`);
|
||||
|
||||
writeStateMd(statePath, stateContent, cwd);
|
||||
}
|
||||
|
||||
|
||||
@@ -6,7 +6,7 @@ const fs = require('fs');
|
||||
const path = require('path');
|
||||
const { escapeRegex, loadConfig, normalizePhaseName, comparePhaseNum, findPhaseInternal, getArchivedPhaseDirs, generateSlugInternal, getMilestonePhaseFilter, stripShippedMilestones, extractCurrentMilestone, replaceInCurrentMilestone, toPosixPath, output, error } = require('./core.cjs');
|
||||
const { extractFrontmatter } = require('./frontmatter.cjs');
|
||||
const { writeStateMd } = require('./state.cjs');
|
||||
const { writeStateMd, stateExtractField, stateReplaceField, stateReplaceFieldWithFallback } = require('./state.cjs');
|
||||
|
||||
function cmdPhasesList(cwd, options, raw) {
|
||||
const phasesDir = path.join(cwd, '.planning', 'phases');
|
||||
@@ -685,12 +685,11 @@ function cmdPhaseRemove(cwd, targetPhase, options, raw) {
|
||||
const statePath = path.join(cwd, '.planning', 'STATE.md');
|
||||
if (fs.existsSync(statePath)) {
|
||||
let stateContent = fs.readFileSync(statePath, 'utf-8');
|
||||
// Update "Total Phases" field
|
||||
const totalPattern = /(\*\*Total Phases:\*\*\s*)(\d+)/;
|
||||
const totalMatch = stateContent.match(totalPattern);
|
||||
if (totalMatch) {
|
||||
const oldTotal = parseInt(totalMatch[2], 10);
|
||||
stateContent = stateContent.replace(totalPattern, `$1${oldTotal - 1}`);
|
||||
// Update "Total Phases" field — supports both bold and plain formats
|
||||
const totalRaw = stateExtractField(stateContent, 'Total Phases');
|
||||
if (totalRaw) {
|
||||
const oldTotal = parseInt(totalRaw, 10);
|
||||
stateContent = stateReplaceField(stateContent, 'Total Phases', String(oldTotal - 1)) || stateContent;
|
||||
}
|
||||
// Update "Phase: X of Y" pattern
|
||||
const ofPattern = /(\bof\s+)(\d+)(\s*(?:\(|phases?))/i;
|
||||
@@ -882,67 +881,58 @@ function cmdPhaseComplete(cwd, phaseNum, raw) {
|
||||
} catch { /* intentionally empty */ }
|
||||
}
|
||||
|
||||
// Update STATE.md
|
||||
// Update STATE.md — use shared helpers that handle both **bold:** and plain Field: formats
|
||||
if (fs.existsSync(statePath)) {
|
||||
let stateContent = fs.readFileSync(statePath, 'utf-8');
|
||||
|
||||
// Update Current Phase
|
||||
stateContent = stateContent.replace(
|
||||
/(\*\*Current Phase:\*\*\s*).*/,
|
||||
`$1${nextPhaseNum || phaseNum}`
|
||||
);
|
||||
// Update Current Phase — preserve "X of Y (Name)" compound format
|
||||
const phaseValue = nextPhaseNum || phaseNum;
|
||||
const existingPhaseField = stateExtractField(stateContent, 'Current Phase')
|
||||
|| stateExtractField(stateContent, 'Phase');
|
||||
let newPhaseValue = String(phaseValue);
|
||||
if (existingPhaseField) {
|
||||
const totalMatch = existingPhaseField.match(/of\s+(\d+)/);
|
||||
const nameMatch = existingPhaseField.match(/\(([^)]+)\)/);
|
||||
if (totalMatch) {
|
||||
const total = totalMatch[1];
|
||||
const nameStr = nextPhaseName ? ` (${nextPhaseName.replace(/-/g, ' ')})` : (nameMatch ? ` (${nameMatch[1]})` : '');
|
||||
newPhaseValue = `${phaseValue} of ${total}${nameStr}`;
|
||||
}
|
||||
}
|
||||
stateContent = stateReplaceFieldWithFallback(stateContent, 'Current Phase', 'Phase', newPhaseValue);
|
||||
|
||||
// Update Current Phase Name
|
||||
if (nextPhaseName) {
|
||||
stateContent = stateContent.replace(
|
||||
/(\*\*Current Phase Name:\*\*\s*).*/,
|
||||
`$1${nextPhaseName.replace(/-/g, ' ')}`
|
||||
);
|
||||
stateContent = stateReplaceFieldWithFallback(stateContent, 'Current Phase Name', null, nextPhaseName.replace(/-/g, ' '));
|
||||
}
|
||||
|
||||
// Update Status
|
||||
stateContent = stateContent.replace(
|
||||
/(\*\*Status:\*\*\s*).*/,
|
||||
`$1${isLastPhase ? 'Milestone complete' : 'Ready to plan'}`
|
||||
);
|
||||
stateContent = stateReplaceFieldWithFallback(stateContent, 'Status', null,
|
||||
isLastPhase ? 'Milestone complete' : 'Ready to plan');
|
||||
|
||||
// Update Current Plan
|
||||
stateContent = stateContent.replace(
|
||||
/(\*\*Current Plan:\*\*\s*).*/,
|
||||
`$1Not started`
|
||||
);
|
||||
stateContent = stateReplaceFieldWithFallback(stateContent, 'Current Plan', 'Plan', 'Not started');
|
||||
|
||||
// Update Last Activity
|
||||
stateContent = stateContent.replace(
|
||||
/(\*\*Last Activity:\*\*\s*).*/,
|
||||
`$1${today}`
|
||||
);
|
||||
stateContent = stateReplaceFieldWithFallback(stateContent, 'Last Activity', 'Last activity', today);
|
||||
|
||||
// Update Last Activity Description
|
||||
stateContent = stateContent.replace(
|
||||
/(\*\*Last Activity Description:\*\*\s*).*/,
|
||||
`$1Phase ${phaseNum} complete${nextPhaseNum ? `, transitioned to Phase ${nextPhaseNum}` : ''}`
|
||||
);
|
||||
stateContent = stateReplaceFieldWithFallback(stateContent, 'Last Activity Description', null,
|
||||
`Phase ${phaseNum} complete${nextPhaseNum ? `, transitioned to Phase ${nextPhaseNum}` : ''}`);
|
||||
|
||||
// Increment Completed Phases counter (#956)
|
||||
const completedMatch = stateContent.match(/\*\*Completed Phases:\*\*\s*(\d+)/);
|
||||
if (completedMatch) {
|
||||
const newCompleted = parseInt(completedMatch[1], 10) + 1;
|
||||
stateContent = stateContent.replace(
|
||||
/(\*\*Completed Phases:\*\*\s*)\d+/,
|
||||
`$1${newCompleted}`
|
||||
);
|
||||
const completedRaw = stateExtractField(stateContent, 'Completed Phases');
|
||||
if (completedRaw) {
|
||||
const newCompleted = parseInt(completedRaw, 10) + 1;
|
||||
stateContent = stateReplaceField(stateContent, 'Completed Phases', String(newCompleted)) || stateContent;
|
||||
|
||||
// Recalculate percent based on completed / total (#956)
|
||||
const totalMatch = stateContent.match(/\*\*Total Phases:\*\*\s*(\d+)/);
|
||||
if (totalMatch) {
|
||||
const totalPhases = parseInt(totalMatch[1], 10);
|
||||
const totalRaw = stateExtractField(stateContent, 'Total Phases');
|
||||
if (totalRaw) {
|
||||
const totalPhases = parseInt(totalRaw, 10);
|
||||
if (totalPhases > 0) {
|
||||
const newPercent = Math.round((newCompleted / totalPhases) * 100);
|
||||
stateContent = stateContent.replace(
|
||||
/(\*\*Progress:\*\*\s*)\d+%/,
|
||||
`$1${newPercent}%`
|
||||
);
|
||||
stateContent = stateReplaceField(stateContent, 'Progress', `${newPercent}%`) || stateContent;
|
||||
// Also update percent field if it exists separately
|
||||
stateContent = stateContent.replace(
|
||||
/(percent:\s*)\d+/,
|
||||
|
||||
@@ -201,6 +201,22 @@ function stateReplaceField(content, fieldName, newValue) {
|
||||
return null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Replace a STATE.md field with fallback field name support.
|
||||
* Tries `primary` first, then `fallback` (if provided), returns content unchanged
|
||||
* if neither matches. This consolidates the replaceWithFallback pattern that was
|
||||
* previously duplicated inline across phase.cjs, milestone.cjs, and state.cjs.
|
||||
*/
|
||||
function stateReplaceFieldWithFallback(content, primary, fallback, value) {
|
||||
let result = stateReplaceField(content, primary, value);
|
||||
if (result) return result;
|
||||
if (fallback) {
|
||||
result = stateReplaceField(content, fallback, value);
|
||||
if (result) return result;
|
||||
}
|
||||
return content;
|
||||
}
|
||||
|
||||
function cmdStateAdvancePlan(cwd, raw) {
|
||||
const statePath = planningPaths(cwd).state;
|
||||
if (!fs.existsSync(statePath)) { output({ error: 'STATE.md not found' }, raw); return; }
|
||||
@@ -232,16 +248,9 @@ function cmdStateAdvancePlan(cwd, raw) {
|
||||
return;
|
||||
}
|
||||
|
||||
const replaceField = (c, primary, fallback, value) => {
|
||||
let r = stateReplaceField(c, primary, value);
|
||||
if (r) return r;
|
||||
if (fallback) { r = stateReplaceField(c, fallback, value); if (r) return r; }
|
||||
return c;
|
||||
};
|
||||
|
||||
if (currentPlan >= totalPlans) {
|
||||
content = replaceField(content, 'Status', null, 'Phase complete — ready for verification');
|
||||
content = replaceField(content, 'Last Activity', 'Last activity', today);
|
||||
content = stateReplaceFieldWithFallback(content, 'Status', null, 'Phase complete — ready for verification');
|
||||
content = stateReplaceFieldWithFallback(content, 'Last Activity', 'Last activity', today);
|
||||
writeStateMd(statePath, content, cwd);
|
||||
output({ advanced: false, reason: 'last_plan', current_plan: currentPlan, total_plans: totalPlans, status: 'ready_for_verification' }, raw, 'false');
|
||||
} else {
|
||||
@@ -253,8 +262,8 @@ function cmdStateAdvancePlan(cwd, raw) {
|
||||
} else {
|
||||
content = stateReplaceField(content, 'Current Plan', String(newPlan)) || content;
|
||||
}
|
||||
content = replaceField(content, 'Status', null, 'Ready to execute');
|
||||
content = replaceField(content, 'Last Activity', 'Last activity', today);
|
||||
content = stateReplaceFieldWithFallback(content, 'Status', null, 'Ready to execute');
|
||||
content = stateReplaceFieldWithFallback(content, 'Last Activity', 'Last activity', today);
|
||||
writeStateMd(statePath, content, cwd);
|
||||
output({ advanced: true, previous_plan: currentPlan, current_plan: newPlan, total_plans: totalPlans }, raw, 'true');
|
||||
}
|
||||
@@ -906,6 +915,7 @@ function cmdSignalResume(cwd, raw) {
|
||||
module.exports = {
|
||||
stateExtractField,
|
||||
stateReplaceField,
|
||||
stateReplaceFieldWithFallback,
|
||||
writeStateMd,
|
||||
cmdStateLoad,
|
||||
cmdStateGet,
|
||||
|
||||
@@ -476,6 +476,23 @@ describe('milestone complete command', () => {
|
||||
);
|
||||
});
|
||||
|
||||
test('updates STATE.md with plain format fields', () => {
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'ROADMAP.md'),
|
||||
`# Roadmap v1.0\n`
|
||||
);
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'STATE.md'),
|
||||
`# State\n\nStatus: In progress\nLast Activity: 2025-01-01\nLast Activity Description: Working\n`
|
||||
);
|
||||
|
||||
const result = runGsdTools('milestone complete v1.0 --name Test', tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error}`);
|
||||
|
||||
const state = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8');
|
||||
assert.ok(state.includes('v1.0 milestone complete'), 'plain Status field should be updated');
|
||||
});
|
||||
|
||||
test('handles empty phases directory', () => {
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'ROADMAP.md'),
|
||||
|
||||
@@ -1508,6 +1508,31 @@ describe('phase complete command', () => {
|
||||
assert.strictEqual(cells[1], 'v1.0', 'Milestone column should be preserved');
|
||||
assert.ok(cells[3].includes('Complete'), 'Status column should be Complete');
|
||||
});
|
||||
|
||||
test('updates STATE.md with plain format fields (no bold)', () => {
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'ROADMAP.md'),
|
||||
`# Roadmap\n\n### Phase 1: Only\n**Goal:** Test\n`
|
||||
);
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'STATE.md'),
|
||||
`# State\n\nPhase: 1 of 1 (Only)\nStatus: In progress\nPlan: 01-01\nLast Activity: 2025-01-01\nLast Activity Description: Working\n`
|
||||
);
|
||||
|
||||
const p1 = path.join(tmpDir, '.planning', 'phases', '01-only');
|
||||
fs.mkdirSync(p1, { recursive: true });
|
||||
fs.writeFileSync(path.join(p1, '01-01-PLAN.md'), '# Plan');
|
||||
fs.writeFileSync(path.join(p1, '01-01-SUMMARY.md'), '# Summary');
|
||||
|
||||
const result = runGsdTools('phase complete 1', tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error}`);
|
||||
|
||||
const state = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8');
|
||||
assert.ok(state.includes('Milestone complete'), 'plain Status field should be updated');
|
||||
assert.ok(state.includes('Not started'), 'plain Plan field should be updated');
|
||||
// Verify compound format preserved
|
||||
assert.ok(state.match(/Phase:.*of\s+1/), 'should preserve "of N" in compound Phase format');
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
@@ -506,7 +506,7 @@ describe('STATE.md frontmatter sync', () => {
|
||||
// stateExtractField and stateReplaceField helpers
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
const { stateExtractField, stateReplaceField } = require('../get-shit-done/bin/lib/state.cjs');
|
||||
const { stateExtractField, stateReplaceField, stateReplaceFieldWithFallback } = require('../get-shit-done/bin/lib/state.cjs');
|
||||
|
||||
describe('stateExtractField and stateReplaceField helpers', () => {
|
||||
// stateExtractField tests
|
||||
@@ -585,6 +585,45 @@ describe('stateExtractField and stateReplaceField helpers', () => {
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// stateReplaceFieldWithFallback — consolidated fallback helper
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
describe('stateReplaceFieldWithFallback', () => {
|
||||
test('replaces primary field when present', () => {
|
||||
const content = '# State\n\n**Status:** Old\n';
|
||||
const result = stateReplaceFieldWithFallback(content, 'Status', null, 'New');
|
||||
assert.ok(result.includes('**Status:** New'));
|
||||
});
|
||||
|
||||
test('falls back to secondary field when primary not found', () => {
|
||||
const content = '# State\n\nLast activity: 2024-01-01\n';
|
||||
const result = stateReplaceFieldWithFallback(content, 'Last Activity', 'Last activity', '2025-03-19');
|
||||
assert.ok(result.includes('Last activity: 2025-03-19'), 'should update fallback field');
|
||||
});
|
||||
|
||||
test('returns content unchanged when neither field matches', () => {
|
||||
const content = '# State\n\n**Phase:** 3\n';
|
||||
const result = stateReplaceFieldWithFallback(content, 'Status', 'state', 'New');
|
||||
assert.strictEqual(result, content, 'content should be unchanged');
|
||||
});
|
||||
|
||||
test('prefers primary over fallback when both exist', () => {
|
||||
const content = '# State\n\n**Status:** Old\nStatus: Also old\n';
|
||||
const result = stateReplaceFieldWithFallback(content, 'Status', 'Status', 'New');
|
||||
// Bold format is tried first by stateReplaceField
|
||||
assert.ok(result.includes('**Status:** New'), 'should replace bold (primary) format');
|
||||
});
|
||||
|
||||
test('works with plain format fields', () => {
|
||||
const content = '# State\n\nPhase: 1 of 3 (Foundation)\nStatus: In progress\nPlan: 01-01\n';
|
||||
let updated = stateReplaceFieldWithFallback(content, 'Status', null, 'Complete');
|
||||
assert.ok(updated.includes('Status: Complete'), 'should update plain Status');
|
||||
updated = stateReplaceFieldWithFallback(updated, 'Current Plan', 'Plan', 'Not started');
|
||||
assert.ok(updated.includes('Plan: Not started'), 'should fall back to Plan field');
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// cmdStateLoad, cmdStateGet, cmdStatePatch, cmdStateUpdate CLI tests
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
Reference in New Issue
Block a user