From 3e2c85e6fd1d8d71ac02cfe2b6226b63ebf63151 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 19 Mar 2026 15:05:20 -0400 Subject: [PATCH] refactor: consolidate STATE.md field helpers, fix command injection in isGitIgnored MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- get-shit-done/bin/lib/core.cjs | 6 ++- get-shit-done/bin/lib/milestone.cjs | 22 +++----- get-shit-done/bin/lib/phase.cjs | 84 +++++++++++++---------------- get-shit-done/bin/lib/state.cjs | 32 +++++++---- tests/milestone.test.cjs | 17 ++++++ tests/phase.test.cjs | 25 +++++++++ tests/state.test.cjs | 41 +++++++++++++- 7 files changed, 152 insertions(+), 75 deletions(-) diff --git a/get-shit-done/bin/lib/core.cjs b/get-shit-done/bin/lib/core.cjs index 43b81bc20..c4c7ac707 100644 --- a/get-shit-done/bin/lib/core.cjs +++ b/get-shit-done/bin/lib/core.cjs @@ -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', }); diff --git a/get-shit-done/bin/lib/milestone.cjs b/get-shit-done/bin/lib/milestone.cjs index b105b9f2d..a86584d2f 100644 --- a/get-shit-done/bin/lib/milestone.cjs +++ b/get-shit-done/bin/lib/milestone.cjs @@ -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); } diff --git a/get-shit-done/bin/lib/phase.cjs b/get-shit-done/bin/lib/phase.cjs index 097175e15..5b01f2bbd 100644 --- a/get-shit-done/bin/lib/phase.cjs +++ b/get-shit-done/bin/lib/phase.cjs @@ -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+/, diff --git a/get-shit-done/bin/lib/state.cjs b/get-shit-done/bin/lib/state.cjs index 3a91006b9..a01aafd66 100644 --- a/get-shit-done/bin/lib/state.cjs +++ b/get-shit-done/bin/lib/state.cjs @@ -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, diff --git a/tests/milestone.test.cjs b/tests/milestone.test.cjs index 77b3c8376..c3319d10c 100644 --- a/tests/milestone.test.cjs +++ b/tests/milestone.test.cjs @@ -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'), diff --git a/tests/phase.test.cjs b/tests/phase.test.cjs index a4df0743c..bbc0c004e 100644 --- a/tests/phase.test.cjs +++ b/tests/phase.test.cjs @@ -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'); + }); }); // ───────────────────────────────────────────────────────────────────────────── diff --git a/tests/state.test.cjs b/tests/state.test.cjs index fe3b84670..7f86f25fc 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -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 // ─────────────────────────────────────────────────────────────────────────────