diff --git a/.changeset/1257-state-pipe-table-planned-begin.md b/.changeset/1257-state-pipe-table-planned-begin.md new file mode 100644 index 000000000..a15e59fe8 --- /dev/null +++ b/.changeset/1257-state-pipe-table-planned-begin.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 1260 +--- +**`state planned-phase` now advances the pipe-table `Status` cell (and frontmatter `status`), and `state begin-phase` now updates the Current Position `| Phase |` / `| Plan |` cells instead of prepending stray inline lines.** Systemic follow-up to #1255: `planned-phase` ran its body-field replacements on the full file content, so the YAML frontmatter `status:` line was matched before the body `| Status | … |` cell and the status never reached `Ready to execute`; and `begin-phase` had pipe-table branches only for `Status`/`Last activity`, so for pipe-table `STATE.md` the `Phase`/`Plan` rows were left stale while a spurious inline `Phase: N — EXECUTING` line was prepended. Both handlers now strip frontmatter before body-field replacement and update pipe-table cells in place, matching the inline-format behaviour. (#1257) diff --git a/src/state.cts b/src/state.cts index 26c52970c..c1f54a902 100644 --- a/src/state.cts +++ b/src/state.cts @@ -1833,7 +1833,12 @@ function cmdStateBeginPhase(cwd: string, phaseNumber: string | number, phaseName if (/^Phase:/m.test(posBody)) { posBody = posBody.replace(/^Phase:.*$/m, newPhase); } else { - posBody = newPhase + '\n' + posBody; + // Pipe-table format in Current Position (#1257): update the | Phase | … | + // cell rather than prepending a spurious inline `Phase:` line (which left + // the table cell stale). Mirrors the Status/Last-activity table branches. + const phaseValue = `${phaseNumber}${phaseName ? ` (${phaseName})` : ''} — EXECUTING`; + const replaced = stateReplaceField(posBody, 'Phase', phaseValue); + if (replaced !== null) posBody = replaced; } // Update or insert Plan line @@ -1841,7 +1846,11 @@ function cmdStateBeginPhase(cwd: string, phaseNumber: string | number, phaseName if (/^Plan:/m.test(posBody)) { posBody = posBody.replace(/^Plan:.*$/m, newPlan); } else { - posBody = posBody.replace(/^(Phase:.*$)/m, `$1\n${newPlan}`); + // Pipe-table format in Current Position (#1257): update the | Plan | … | + // cell rather than appending after a prepended inline line. + const planValue = `1 of ${planCount || '?'}`; + const replaced = stateReplaceField(posBody, 'Plan', planValue); + if (replaced !== null) posBody = replaced; } // Update Status line if present @@ -2010,36 +2019,49 @@ function cmdStatePlannedPhase(cwd: string, phaseNumber: string | number, planCou // doing so tramples curated/known-good counters. Route through the body-only // write contract (resync:false), the same guard state.update uses. (#500 RC1) readModifyWriteStateMd(statePath, (content) => { + // Bug #1257: all body-field replacements must operate on the body only + // (frontmatter stripped), not on the full content. When the full content is + // passed to stateReplaceFieldIfTemplate the YAML `status: planning` key matches + // the plain-text pattern (`^Status:\s*`) before the body pipe-table row, so the + // pipe-table `| Status | Planning |` cell is never updated and syncStateFrontmatter + // re-derives 'planning' from the unchanged body — the status never advances. + // (Mirrors the begin/complete-phase fix from #1255/#1256.) + const existingFm = extractFrontmatter(content) as Record; + const hasFrontmatter = Object.keys(existingFm).length > 0; + let body = stripFrontmatter(content); + const reassemble = (b: string) => + hasFrontmatter ? `---\n${reconstructFrontmatter(existingFm as unknown as Frontmatter)}\n---\n\n${b}` : b; + // Update Status — only when the existing value is a known template default // (Knuth invariant: preserve executor-authored values). - const newContent = stateReplaceFieldIfTemplate(content, 'Status', statusDefaults, 'Ready to execute'); - if (newContent !== content) { content = newContent; updated.push('Status'); } + const newBody = stateReplaceFieldIfTemplate(body, 'Status', statusDefaults, 'Ready to execute'); + if (newBody !== body) { body = newBody; updated.push('Status'); } // Update Total Plans in Phase if (planCount !== null && planCount !== undefined) { - const result = stateReplaceField(content, 'Total Plans in Phase', String(planCount)); - if (result) { content = result; updated.push('Total Plans in Phase'); } + const result = stateReplaceField(body, 'Total Plans in Phase', String(planCount)); + if (result) { body = result; updated.push('Total Plans in Phase'); } } // Update Last Activity — only when the existing value is a known template default { - const after = stateReplaceFieldIfTemplate(content, 'Last Activity', lastActivityDefaults, today); - if (after !== content) { content = after; updated.push('Last Activity'); } + const after = stateReplaceFieldIfTemplate(body, 'Last Activity', lastActivityDefaults, today); + if (after !== body) { body = after; updated.push('Last Activity'); } } // Update Last Activity Description { - const result = stateReplaceField(content, 'Last Activity Description', `Phase ${phaseNumber} planning complete — ${planCount || '?'} plans ready`); - if (result) { content = result; updated.push('Last Activity Description'); } + const result = stateReplaceField(body, 'Last Activity Description', `Phase ${phaseNumber} planning complete — ${planCount || '?'} plans ready`); + if (result) { body = result; updated.push('Last Activity Description'); } } // Update Current Position section - content = updateCurrentPositionFields(content, { + body = updateCurrentPositionFields(body, { status: 'Ready to execute', lastActivity: `${today} -- Phase ${phaseNumber} planning complete`, }); - return content; + return reassemble(body); }, cwd, { resync: false }); output({ updated, phase: phaseNumber, plan_count: planCount }, raw, updated.length > 0 ? 'true' : 'false'); diff --git a/tests/state.test.cjs b/tests/state.test.cjs index 687383d4d..4c7098306 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -3960,3 +3960,209 @@ Last activity: 2026-06-01 -- Roadmap created } }); }); + +// #1257 — planned-phase + begin-phase pipe-table regressions +// ───────────────────────────────────────────────────────────────────────────── + +/** + * Regression tests for bug #1257. + * + * Finding 1 (INFERRED — reproduced here empirically): + * `cmdStatePlannedPhase` calls `stateReplaceFieldIfTemplate(content, 'Status', …)` + * on the FULL file content (including YAML frontmatter). The plain-text pattern + * `^Status:\s*(.+)` (case-insensitive) matches the YAML frontmatter `status: planning` + * line BEFORE reaching the body pipe-table row `| Status | Planning |`. The pipe-table + * cell is never updated. `syncStateFrontmatter` re-derives from the unchanged body + * and the #1230 delta heuristic preserves the original frontmatter value, so the + * status never advances to 'Ready to execute'. + * Smoking-gun: src/state.cts:2015 — `stateReplaceFieldIfTemplate(content, 'Status', …)` + * where `content` is the full file (frontmatter + body), not stripped body. + * + * Finding 2 (OBSERVED): + * `cmdStateBeginPhase`'s `## Current Position` update block only has pipe-table + * else-branches for Status (#1255) and Last-activity (#1255), NOT for Phase or Plan. + * For a pipe-table STATE.md, the `| Phase | … |` and `| Plan | … |` rows are silently + * ignored: the else-branch instead INSERTS a new inline `Phase: N — EXECUTING` text + * line prepended to the section body (leaving the old table cells stale). + * Smoking-gun: src/state.cts:1833–1844 — `^Phase:` / `^Plan:` plain-text checks with + * else-branches that prepend text rather than calling stateReplaceField on the table. + */ + +// STATE.md fixture for #1257 — pipe-table format with frontmatter status: planning +// After planned-phase the body Status should become 'Ready to execute' and +// frontmatter status should advance accordingly. +const TABLE_STATUS_PLANNING_1257 = `--- +gsd_state_version: '1.0' +status: planning +--- + +# Project State + +## Configuration + +| Current Phase | 1 | +| Current Phase Name | setup | +| Total Plans in Phase | 3 | +| Current Plan | 1 | +| Status | Planning | +| Last Activity | 2026-06-01 | +| Last Activity Description | Roadmap created | + +## Current Position + +| Phase | 1 (setup) | +| Plan | 1 of 3 | +| Status | Planning | +| Last activity | 2026-06-01 | +`; + +function make1257TempProject(stateContent) { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-1257-')); + const planningDir = path.join(dir, '.planning'); + fs.mkdirSync(planningDir, { recursive: true }); + // Minimal ROADMAP so phase resolution can proceed + fs.writeFileSync(path.join(planningDir, 'ROADMAP.md'), [ + '# ROADMAP', + '', + '## Phase 1: setup:', + '- [ ] Step 1', + '', + ].join('\n'), 'utf8'); + fs.writeFileSync(path.join(planningDir, 'STATE.md'), stateContent, 'utf8'); + return dir; +} + +describe('#1257 — planned-phase and begin-phase pipe-table regressions', () => { + + // ── Finding 1 ─────────────────────────────────────────────────────────────── + + test('Finding 1: planned-phase advances Configuration pipe-table Status cell to Ready to execute', () => { + // planned-phase should update the Configuration-section body | Status | … | cell. + // Smoking-gun: state.cts:2015 calls stateReplaceFieldIfTemplate on full content, + // so the frontmatter `status:` key shadows the body table cell and the cell is + // never updated. (updateCurrentPositionFields at line 2037 does correctly update + // the Current Position table cell — this test specifically targets the Configuration + // table cell, which has no pipe-table else-branch in planned-phase.) + const dir = make1257TempProject(TABLE_STATUS_PLANNING_1257); + try { + const result = runGsdTools( + ['state', 'planned-phase', '--phase', '1', '--plans', '3'], + dir + ); + assert.ok(result.success, `planned-phase failed: ${result.error || result.output}`); + + const after = fs.readFileSync(path.join(dir, '.planning', 'STATE.md'), 'utf8'); + + // Extract the ## Configuration section (stops before ## Current Position) + // to avoid false-positive from the Current Position table (which IS updated + // by updateCurrentPositionFields). + const cfgMatch = after.match(/##\s*Configuration\s*\r?\n([\s\S]*?)(?=\r?\n##|$)/i); + assert.ok(cfgMatch, '## Configuration section must exist'); + const cfgSection = cfgMatch[1]; + + // The Configuration section's pipe-table Status cell must be updated + assert.ok( + /\|\s*Status\s*\|\s*Ready to execute\s*\|/i.test(cfgSection), + `Configuration pipe-table Status cell must be 'Ready to execute' after planned-phase; got Configuration:\n${cfgSection}` + ); + } finally { + cleanup(dir); + } + }); + + test('Finding 1: planned-phase advances frontmatter status to executing when body Status is pipe-table', () => { + // The frontmatter status must advance after planned-phase sets Status to 'Ready to execute'. + // (syncStateFrontmatter maps 'ready to execute' → 'executing'.) + const dir = make1257TempProject(TABLE_STATUS_PLANNING_1257); + try { + runGsdTools( + ['state', 'planned-phase', '--phase', '1', '--plans', '3'], + dir + ); + const after = fs.readFileSync(path.join(dir, '.planning', 'STATE.md'), 'utf8'); + + const fmMatch = after.match(/^---\r?\n([\s\S]*?)\r?\n---/); + assert.ok(fmMatch, 'STATE.md must have YAML frontmatter after planned-phase'); + const fm = fmMatch[1]; + // syncStateFrontmatter maps 'Ready to execute' → 'executing' in normalizeStateStatus + assert.ok( + /^status:\s*executing\s*$/m.test(fm), + `frontmatter status must be 'executing' after planned-phase on pipe-table STATUS; got frontmatter:\n${fm}` + ); + } finally { + cleanup(dir); + } + }); + + // ── Finding 2 ─────────────────────────────────────────────────────────────── + + test('Finding 2: begin-phase updates Current Position pipe-table Phase cell (not prepend inline)', () => { + // begin-phase must update the | Phase | … | cell in ## Current Position. + // Smoking-gun: state.cts:1833 checks `^Phase:` (plain-text pattern) which + // never matches a pipe-table row, so the else-branch at 1836 PREPENDS a new + // inline `Phase: N — EXECUTING` line to the section instead of updating the cell. + const dir = make1257TempProject(TABLE_STATUS_PLANNING_1257); + try { + const result = runGsdTools( + ['state', 'begin-phase', '--phase', '1', '--name', 'setup', '--plans', '3'], + dir + ); + assert.ok(result.success, `begin-phase failed: ${result.error || result.output}`); + + const after = fs.readFileSync(path.join(dir, '.planning', 'STATE.md'), 'utf8'); + + // Extract ## Current Position section only + const cpMatch = after.match(/##\s*Current Position\s*\r?\n([\s\S]*?)(?=\r?\n##|$)/i); + assert.ok(cpMatch, '## Current Position section must exist'); + const cpSection = cpMatch[1]; + + // The pipe-table Phase cell must be updated to reflect the executing phase + assert.ok( + /\|\s*Phase\s*\|[^|]*1[^|]*EXECUTING[^|]*\|/i.test(cpSection), + `Current Position pipe-table Phase cell must contain phase 1 EXECUTING; got Current Position:\n${cpSection}` + ); + + // Must NOT have a spurious prepended inline `Phase: …` text line + assert.ok( + !/^Phase:\s+\d/m.test(cpSection), + `Current Position must NOT have a spuriously prepended inline 'Phase: N' text line; got Current Position:\n${cpSection}` + ); + } finally { + cleanup(dir); + } + }); + + test('Finding 2: begin-phase updates Current Position pipe-table Plan cell (not prepend inline)', () => { + // begin-phase must update the | Plan | … | cell in ## Current Position. + // Smoking-gun: state.cts:1841 checks `^Plan:` which never matches a pipe-table row, + // so the else-branch at 1843 replaces the (newly-prepended) inline Phase line with + // Phase\nPlan, neither touching the existing table | Plan | cell. + const dir = make1257TempProject(TABLE_STATUS_PLANNING_1257); + try { + runGsdTools( + ['state', 'begin-phase', '--phase', '1', '--name', 'setup', '--plans', '3'], + dir + ); + const after = fs.readFileSync(path.join(dir, '.planning', 'STATE.md'), 'utf8'); + + // Extract ## Current Position section only + const cpMatch = after.match(/##\s*Current Position\s*\r?\n([\s\S]*?)(?=\r?\n##|$)/i); + assert.ok(cpMatch, '## Current Position section must exist'); + const cpSection = cpMatch[1]; + + // The pipe-table Plan cell must be updated to '1 of 3' + assert.ok( + /\|\s*Plan\s*\|\s*1 of 3\s*\|/i.test(cpSection), + `Current Position pipe-table Plan cell must be '1 of 3'; got Current Position:\n${cpSection}` + ); + + // Must NOT have a spurious prepended inline `Plan: …` text line + assert.ok( + !/^Plan:\s+\d/m.test(cpSection), + `Current Position must NOT have a spuriously prepended inline 'Plan: N' text line; got Current Position:\n${cpSection}` + ); + } finally { + cleanup(dir); + } + }); +});