golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

Merge pull request #2168 from behruznassre/fix/2137-derive-progress-header-driven

Browse Source 
fix(#2137): parse the ROADMAP Progress table by header, not fixed column count
This commit is contained in:
Tom Boucher
2026-07-13 16:38:06 -04:00
committed by GitHub
parent 02120b81ad f89740d53f
commit 9fda4ed755
4 changed files with 414 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
.changeset/merry-hawks-swim.md Normal file
View File

@@ -0,0 +1,5 @@
---
type: Fixed
pr: 2168
---
**phase complete now updates STATE progress on milestone-grouped roadmaps** — deriveProgressFromRoadmap parses the ## Progress table by header (column-by-name) instead of a fixed 4-column layout, so the 5-column milestone-grouped shape is no longer silently unparsed.

119
src/phase-lifecycle.cts
View File

@@ -37,43 +37,98 @@ export function deriveProgressFromRoadmap(roadmapContent: string): RoadmapProgre
let totalPlans: number | null = null;
try {
// Count Complete rows in the progress table (Status column = "Complete").
// Pattern: row where the phase cell starts with a digit (data row, not header),
// followed by any cell content, then a "Complete" status cell.
// Handles both short form ("| 4. |") and long form ("| 01. Foundation |").
// See phase-lifecycle.ts ~line 1655 for the original SDK pattern.
const tableCompletePattern = /\|\s*\d+[^|]*\|\s*[^|]*\|\s*Complete\s*\|/gi;
const completeMatches = roadmapContent.match(tableCompletePattern);
completedPhases = completeMatches ? completeMatches.length : null;
// Parse the Progress table by HEADER, not by a fixed column count. The
// writer (cmdPhaseComplete) already branches on `cells.length === 5`, so it
// understands both the 4-column greenfield table
// | Phase | Plans Complete | Status | Completed |
// and the 5-column milestone-grouped table the same template ships
// | Phase | Milestone | Plans Complete | Status | Completed |
// The reader used two 4-column-only regexes, so every project past its v1.0
// milestone (5-column shape) parsed to all-null and phase.complete silently
// skipped the STATE progress write. Reading column indices by NAME keeps the
// reader and writer in agreement across both shapes and any future column
// (#2137). The table is located by its header row rather than a `## Progress`
// heading because some callers pass a milestone slice with no heading (#1445).
//
// When a `## Progress` heading IS present, scope the search to that section
// (mirroring the writer's #2012 scoping in cmdPhaseComplete) so the reader
// cannot bind to an earlier Phase/Status/Completed-shaped table elsewhere in
// the roadmap. Callers that pass a headingless milestone slice fall back to
// scanning the whole input.
// Line-anchored h2 match — `indexOf('## Progress')` would also match inside
// an h3 `### Progress` (the `## Progress` substring starts at the 2nd hash),
// letting a decoy subheading hijack the slice.
// Case-insensitive to match the case-insensitive header-cell comparison below.
//
// allow-adhoc-markdown: line-based Progress-table scan (header lookup +
// positional cell indexing); table parsing is out of the markdown-sectionizer
// seam's scope. Superseded by the ADR-2143 parseMarkdownTable/TABLE_SCHEMAS
// seam; pending #2143.
const progressMatch = roadmapContent.match(/^##[ \t]+Progress\b/im);
let scoped = roadmapContent;
if (progressMatch && progressMatch.index !== undefined) {
// Slice from `## Progress` to the next h1/h2 heading (or end); h3+ headings
// inside the section do not terminate it. The heading sits at index 0 of
// this slice with no leading newline, so the `\n#` search cannot match it.
const afterHeading = roadmapContent.slice(progressMatch.index);
const nextHeading = afterHeading.search(/\n#{1,2}[ \t]/);
scoped = nextHeading >= 0 ? afterHeading.slice(0, nextHeading) : afterHeading;
}
const lines = scoped.split('\n');
// Count total phase rows in the progress table.
// Identify the table by looking for Phase|...|Status|...|Completed header.
const progressTableMatch = roadmapContent.match(
// allow-adhoc-markdown: table-scoped regex with heading lookahead as stop; table parsing, out of seam scope; pending #1372
/\|\s*Phase\s*\|[^|]*\|[^|]*Status[^|]*\|[^|]*Completed[^|]*\|[\s\S]*?(?=\n\n|\n##|$)/i,
);
if (progressTableMatch) {
const tableText = progressTableMatch[0];
// Count data rows (rows starting with pipe then a phase number),
// excluding 999.x backlog phases. Mirrors init.cts /^999(?:\.|$)/ filter.
const dataRowPattern = /^\|\s*(\d+[^|]*)\|/gm;
let dataRowCount = 0;
let drm: RegExpExecArray | null;
while ((drm = dataRowPattern.exec(tableText)) !== null) {
if (/^999\b/.test(drm[1].trim())) continue;
dataRowCount++;
// Split a markdown table row into trimmed cells — the same
// `split('|').slice(1, -1)` boundary the writer uses (phase.cts).
const rowCells = (line: string): string[] =>
line.split('|').slice(1, -1).map((c) => c.trim());
const isTableRow = (line: string): boolean => line.trim().startsWith('|');
const isSeparatorRow = (cells: string[]): boolean =>
cells.length > 0 && cells.every((c) => /^:?-+:?$/.test(c));
let headerLine = -1;
let phaseIdx = -1;
let statusIdx = -1;
let plansIdx = -1;
for (let i = 0; i < lines.length; i++) {
if (!isTableRow(lines[i])) continue;
const lc = rowCells(lines[i]).map((c) => c.toLowerCase());
const p = lc.indexOf('phase');
const s = lc.indexOf('status');
const c = lc.indexOf('completed');
if (p >= 0 && s >= 0 && c >= 0) {
headerLine = i;
phaseIdx = p;
statusIdx = s;
plansIdx = lc.findIndex((h) => h.includes('plans'));
break;
}
totalPhases = dataRowCount > 0 ? dataRowCount : null;
}
// Sum plan counts from M/N columns in progress table
let totalPlansSum = 0;
const planCellPattern = /\|\s*\d+[^|]*\|\s*(\d+)\/(\d+)\s*\|/gi;
let pm: RegExpExecArray | null;
while ((pm = planCellPattern.exec(roadmapContent)) !== null) {
totalPlansSum += parseInt(pm[2], 10);
if (headerLine >= 0) {
let phaseCount = 0;
let completedCount = 0;
let plansSum = 0;
// Walk the contiguous rows after the header; a markdown table ends at the
// first non-`|` line.
for (let i = headerLine + 1; i < lines.length; i++) {
if (!isTableRow(lines[i])) break;
const cells = rowCells(lines[i]);
if (isSeparatorRow(cells)) continue;
const phaseToken = (cells[phaseIdx] ?? '').trim();
if (!/^\d/.test(phaseToken)) continue; // not a data row
if (/^999\b/.test(phaseToken)) continue; // 999.x backlog sentinel (#1445)
phaseCount++;
if ((cells[statusIdx] ?? '').toLowerCase() === 'complete') completedCount++;
if (plansIdx >= 0) {
const mn = (cells[plansIdx] ?? '').match(/^(\d+)\/(\d+)$/);
if (mn) plansSum += parseInt(mn[2], 10);
}
}
// Preserve the prior contract: a count of 0 is reported as null (absent),
// so the consumer leaves the existing STATE value untouched.
completedPhases = completedCount > 0 ? completedCount : null;
totalPhases = phaseCount > 0 ? phaseCount : null;
totalPlans = plansSum > 0 ? plansSum : null;
}
if (totalPlansSum > 0) totalPlans = totalPlansSum;
} catch { /* intentionally empty — fall through to existing values */ }
return { completedPhases, totalPhases, totalPlans };

131
tests/derive-progress.property.test.cjs Normal file
View File

@@ -0,0 +1,131 @@
'use strict';
/**
* Property-based tests for deriveProgressFromRoadmap column-invariance (#2137).
*
* Module: gsd-core/bin/lib/phase-lifecycle.cjs
* Exported: deriveProgressFromRoadmap(roadmapContent)
*
* The #2137 fix re-reads the `## Progress` table by HEADER NAME (locate the
* `Phase` / `Status` / `Completed` / plans columns by their header cell, index
* data rows positionally) instead of by a fixed 4-column layout. The core new
* capability is therefore invariance to column ORDER and column COUNT: the same
* data must derive the same {completedPhases, totalPhases, totalPlans} no matter
* where the columns sit or how many unrelated columns are interleaved.
*
* Property tested:
* Header-cell permutation + injection invariance — for any set of phase rows,
* shuffling the header columns and injecting arbitrary unrelated columns leaves
* the derived counts identical to the counts computed directly from the data.
* This is the property that distinguishes header-driven parsing from the old
* position-locked regex, and it is the finding the reviewer held firm on.
*
* Lives in a standalone *.property.test.cjs file (the established property-test
* convention). Its effective prefix `derive-progress.property` matches no
* production module prefix, so — like the other *.property.test.cjs files — it
* maps to no module and does not count against the per-module test-file cap
* (lint-test-file-count.cjs). It is named `derive-progress` rather than
* `phase-lifecycle` so it does not get greedily attributed to the shorter
* `phase` prod prefix. The unit/regression fixtures live in state.test.cjs
* alongside the other deriveProgressFromRoadmap cases.
*/
const { describe, test } = require('node:test');
const assert = require('node:assert/strict');
const fc = require('./helpers/fast-check-setup.cjs');
const { deriveProgressFromRoadmap } = require('../gsd-core/bin/lib/phase-lifecycle.cjs');
// A header name for an INJECTED (unrelated) column. It must not collide with the
// reader's column-name lookups: not exactly `phase` / `status` / `completed`
// (indexOf exact match) and not containing `plans` (findIndex substring match).
// Restricted to letters + spaces so it never introduces a `|` that would break
// cell splitting.
const injectedNameArb = fc
.stringMatching(/^[A-Za-z][A-Za-z ]{0,11}$/)
.filter((s) => {
const l = s.trim().toLowerCase();
return l !== '' && l !== 'phase' && l !== 'status' && l !== 'completed' && !l.includes('plans');
});
// One phase row's underlying data. `num` stays ≤ 900 so it never trips the 999.x
// backlog sentinel; `plans` stays ≥ 1 so the plans denominator sum is always > 0
// (deterministic — the reader reports a 0 sum as null, which we avoid here to
// keep the expected value exact). `name` is letters/spaces only (no `|`).
const rowArb = fc.record({
num: fc.integer({ min: 1, max: 900 }),
name: fc.stringMatching(/^[A-Za-z][A-Za-z ]{0,8}$/),
complete: fc.boolean(),
plans: fc.integer({ min: 1, max: 9 }),
});
// Render the cell for a given logical column key from a row datum.
function cellFor(key, row) {
switch (key) {
case 'phase':
return `${row.num}. ${row.name}`.trim();
case 'plans':
return `${row.plans}/${row.plans}`;
case 'status':
return row.complete ? 'Complete' : 'Not started';
case 'completed':
return row.complete ? '2026-01-01' : '-';
default:
return 'x'; // injected column placeholder value
}
}
describe('#2137 deriveProgressFromRoadmap — column-order / column-count invariance', () => {
test('shuffled headers + injected columns derive the same counts as the data', () => {
fc.assert(
fc.property(
fc.array(rowArb, { minLength: 1, maxLength: 6 }),
fc.uniqueArray(injectedNameArb, { maxLength: 3, selector: (s) => s.trim().toLowerCase() }),
fc.integer({ min: 0, max: 1_000_000 }),
(rows, injected, permSeed) => {
// The four real columns plus any injected unrelated columns.
const columns = [
{ key: 'phase', header: 'Phase' },
{ key: 'plans', header: 'Plans Complete' },
{ key: 'status', header: 'Status' },
{ key: 'completed', header: 'Completed' },
...injected.map((h, i) => ({ key: `inj${i}`, header: h })),
];
// Deterministic Fisher–Yates permutation driven by permSeed (an LCG),
// so column ORDER varies across runs without needing Math.random.
let s = (permSeed % 2147483647) + 1;
const nextRand = () => {
s = (s * 48271) % 2147483647;
return (s - 1) / 2147483646;
};
for (let i = columns.length - 1; i > 0; i--) {
const j = Math.floor(nextRand() * (i + 1));
[columns[i], columns[j]] = [columns[j], columns[i]];
}
const headerRow = `| ${columns.map((c) => c.header).join(' | ')} |`;
const sepRow = `| ${columns.map(() => '---').join(' | ')} |`;
const dataRows = rows.map((r) => `| ${columns.map((c) => cellFor(c.key, r)).join(' | ')} |`);
const roadmap = ['## Progress', '', headerRow, sepRow, ...dataRows].join('\n');
const expectedCompleted = rows.filter((r) => r.complete).length;
const expectedPlans = rows.reduce((acc, r) => acc + r.plans, 0);
const result = deriveProgressFromRoadmap(roadmap);
assert.deepEqual(
result,
{
completedPhases: expectedCompleted > 0 ? expectedCompleted : null,
totalPhases: rows.length,
totalPlans: expectedPlans, // ≥ 1 per row, so always > 0 → non-null
},
`derived counts must be invariant to column order/count. columns=${columns
.map((c) => c.header)
.join('|')}`,
);
},
),
);
});
});

191
tests/state.test.cjs
View File

@@ -9557,6 +9557,197 @@ describe('bug #1445 — deriveProgressFromRoadmap excludes 999.x rows', () => {
});
});
// ─── #2137: header-driven parse handles the milestone-grouped (5-col) table ──
//
// Regression for #2137: deriveProgressFromRoadmap read the `## Progress` table
// with two 4-column-only regexes. Every project past its v1.0 milestone uses the
// 5-column milestone-grouped shape the same template ships, so the reader (which
// only understood 4 columns) returned { null, null, null } while the writer
// (cmdPhaseComplete, with its explicit `cells.length === 5` branch) happily wrote
// it — and phase.complete then silently skipped the STATE progress update. The
// fix reads columns by NAME, so both shapes parse identically. These tests would
// fail against the pre-fix 4-column regexes (which returned all-null for 5-col).
describe('#2137 regression: deriveProgressFromRoadmap parses the milestone-grouped 5-column table', () => {
test("the template's own 5-column milestone-grouped Progress block parses non-null", () => {
// Byte-identical to gsd-core/templates/roadmap.md's "Milestone-Grouped
// Roadmap" Progress block — the exact shape that silently returned all-null.
const roadmap = [
'## Progress',
'',
'| Phase | Milestone | Plans Complete | Status | Completed |',
'|-------|-----------|----------------|--------|-----------|',
'| 1. Foundation | v1.0 | 3/3 | Complete | YYYY-MM-DD |',
'| 2. Features | v1.0 | 2/2 | Complete | YYYY-MM-DD |',
'| 5. Security | v1.1 | 0/2 | Not started | - |',
].join('\n');
const result = deriveProgressFromRoadmap(roadmap);
assert.equal(result.totalPhases, 3, `totalPhases must be 3 (5-col table must parse). Got ${result.totalPhases}`);
assert.equal(result.completedPhases, 2, `completedPhases must be 2 (Status is column 4 in the 5-col shape). Got ${result.completedPhases}`);
assert.equal(result.totalPlans, 7, `totalPlans must be 3+2+2=7 (Plans is column 3 in the 5-col shape). Got ${result.totalPlans}`);
});
test('the 4-column greenfield and 5-column milestone-grouped shapes derive the same progress', () => {
// The reader must agree with the writer on both shapes the template ships.
const fiveCol = [
'## Progress',
'| Phase | Milestone | Plans Complete | Status | Completed |',
'| --- | --- | --- | --- | --- |',
'| 1. Foundation | v1.0 | 3/3 | Complete | 2026-01-01 |',
'| 2. Features | v1.0 | 2/2 | Complete | 2026-01-02 |',
].join('\n');
const fourCol = [
'## Progress',
'| Phase | Plans Complete | Status | Completed |',
'| --- | --- | --- | --- |',
'| 1. Foundation | 3/3 | Complete | 2026-01-01 |',
'| 2. Features | 2/2 | Complete | 2026-01-02 |',
].join('\n');
assert.deepEqual(
deriveProgressFromRoadmap(fiveCol),
deriveProgressFromRoadmap(fourCol),
'the milestone-grouped and greenfield shapes must derive identical progress',
);
assert.deepEqual(deriveProgressFromRoadmap(fiveCol), {
completedPhases: 2,
totalPhases: 2,
totalPlans: 5,
});
});
test('999.x backlog rows stay excluded in the 5-column shape', () => {
const roadmap = [
'## Progress',
'| Phase | Milestone | Plans Complete | Status | Completed |',
'| --- | --- | --- | --- | --- |',
'| 1. Alpha | v1.0 | 2/2 | Complete | 2026-01-01 |',
'| 2. Beta | v1.0 | 1/1 | Complete | 2026-01-02 |',
'| 999.1 Future | v2.0 | 0/0 | Backlog | - |',
].join('\n');
const result = deriveProgressFromRoadmap(roadmap);
assert.equal(result.totalPhases, 2, `999.1 backlog row must be excluded in the 5-col shape too. Got ${result.totalPhases}`);
assert.equal(result.completedPhases, 2, `completedPhases must be 2. Got ${result.completedPhases}`);
});
test('binds to the ## Progress table, not an earlier Phase/Status/Completed-shaped table', () => {
// A decoy table under a different heading shares the Phase/Status/Completed
// header shape. The reader must scope to ## Progress (mirroring the writer's
// #2012 scoping) rather than binding to the first matching table it sees.
const roadmap = [
'## Retrospective',
'',
'| Phase | Owner | Status | Completed |',
'| --- | --- | --- | --- |',
'| 1. Old | jo | Complete | 2025-01-01 |',
'',
'## Progress',
'',
'| Phase | Milestone | Plans Complete | Status | Completed |',
'| --- | --- | --- | --- | --- |',
'| 1. Foundation | v1.0 | 3/3 | Complete | 2026-01-01 |',
'| 2. Features | v1.0 | 2/2 | Complete | 2026-01-02 |',
'| 3. Security | v1.1 | 0/2 | Not started | - |',
'',
'## Next',
].join('\n');
const result = deriveProgressFromRoadmap(roadmap);
assert.equal(result.totalPhases, 3, `must count the 3 rows of the ## Progress table, not the 1-row decoy. Got ${result.totalPhases}`);
assert.equal(result.completedPhases, 2, `must count Complete rows in ## Progress (2), not the decoy's 1. Got ${result.completedPhases}`);
assert.equal(result.totalPlans, 7, `must sum the ## Progress plans (3+2+2=7). Got ${result.totalPlans}`);
});
test('an h3 ### Progress decoy does not hijack the h2 ## Progress scope', () => {
// Heading detection must be line-anchored to h2: "### Progress".indexOf("## Progress")
// is 1, so a substring scan would start the slice inside the h3 subheading and
// miss the real table below.
const roadmap = [
'### Progress notes',
'',
'Some prose about progress, no table here.',
'',
'## Progress',
'',
'| Phase | Milestone | Plans Complete | Status | Completed |',
'| --- | --- | --- | --- | --- |',
'| 1. Foundation | v1.0 | 3/3 | Complete | 2026-01-01 |',
'| 2. Features | v1.0 | 2/2 | Complete | 2026-01-02 |',
].join('\n');
const result = deriveProgressFromRoadmap(roadmap);
assert.equal(result.totalPhases, 2, `h2 ## Progress table must be found past the h3 decoy. Got ${result.totalPhases}`);
assert.equal(result.completedPhases, 2, `completedPhases must be 2. Got ${result.completedPhases}`);
});
// ── Boundary conditions (#2137 review) ──────────────────────────────────────
// The header-driven walk terminates at the first non-`|` line and skips the
// separator row, so these edges must not throw and must honour the "0 → null"
// contract that lets the consumer leave the existing STATE value untouched.
test('header + separator only (0 data rows) derives all-null', () => {
const roadmap = [
'## Progress',
'',
'| Phase | Milestone | Plans Complete | Status | Completed |',
'| --- | --- | --- | --- | --- |',
].join('\n');
const result = deriveProgressFromRoadmap(roadmap);
assert.deepEqual(
result,
{ completedPhases: null, totalPhases: null, totalPlans: null },
`an empty table must report all-null (0 counts → null), got ${JSON.stringify(result)}`,
);
});
test('exactly one data row derives that single row', () => {
const roadmap = [
'## Progress',
'',
'| Phase | Milestone | Plans Complete | Status | Completed |',
'| --- | --- | --- | --- | --- |',
'| 1. Foundation | v1.0 | 4/4 | Complete | 2026-01-01 |',
].join('\n');
const result = deriveProgressFromRoadmap(roadmap);
assert.deepEqual(
result,
{ completedPhases: 1, totalPhases: 1, totalPlans: 4 },
`a single Complete row must derive {1,1,4}, got ${JSON.stringify(result)}`,
);
});
test('ragged rows (more/fewer cells than the header) are handled without throwing', () => {
// The reader indexes cells positionally by header name, so an EXTRA trailing
// column is ignored and a SHORT row simply has absent Status/Plans cells
// (`cells[idx] ?? ''`) — neither should throw or corrupt the well-formed row.
const roadmap = [
'## Progress',
'',
'| Phase | Milestone | Plans Complete | Status | Completed |',
'| --- | --- | --- | --- | --- |',
'| 1. Alpha | v1.0 | 2/2 | Complete | 2026-01-01 | stray-extra-column |', // 6 cells (extra)
'| 2. Beta | v1.0 |', // 2 cells (short: Plans/Status/Completed absent)
].join('\n');
let result;
assert.doesNotThrow(() => {
result = deriveProgressFromRoadmap(roadmap);
}, 'ragged rows must not throw');
// Row 1: extra column ignored → counted, Complete, +2 plans.
// Row 2: short row → counted as a phase, but Status/Plans cells are absent so
// it is neither Complete nor plan-bearing.
assert.deepEqual(
result,
{ completedPhases: 1, totalPhases: 2, totalPlans: 2 },
`ragged rows must degrade gracefully to {1,2,2}, got ${JSON.stringify(result)}`,
);
});
});
// ─── Scenario B: state json total_phases via roadmapPhaseCount ───────────────
describe('bug #1445 — state json excludes 999.x phase headings from total_phases', () => {