diff --git a/.changeset/merry-hawks-swim.md b/.changeset/merry-hawks-swim.md new file mode 100644 index 000000000..9f795dd85 --- /dev/null +++ b/.changeset/merry-hawks-swim.md @@ -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. diff --git a/src/phase-lifecycle.cts b/src/phase-lifecycle.cts index 2175cd400..9d2dd2507 100644 --- a/src/phase-lifecycle.cts +++ b/src/phase-lifecycle.cts @@ -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 }; diff --git a/tests/derive-progress.property.test.cjs b/tests/derive-progress.property.test.cjs new file mode 100644 index 000000000..3eb0f2b12 --- /dev/null +++ b/tests/derive-progress.property.test.cjs @@ -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('|')}`, + ); + }, + ), + ); + }); +}); diff --git a/tests/state.test.cjs b/tests/state.test.cjs index 5257bb23d..93140a33f 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -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', () => {