From 3f6be47db9192739c7c8ac6fb0da1b828a262d61 Mon Sep 17 00:00:00 2001 From: Behruz Nassre Esfahani Date: Fri, 10 Jul 2026 23:57:15 -0700 Subject: [PATCH 1/3] fix(#2137): parse the ROADMAP Progress table by header, not fixed column count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit deriveProgressFromRoadmap read the `## Progress` table with two 4-column-only regexes, so every project past its v1.0 milestone — which uses the 5-column milestone-grouped shape the same template ships (| Phase | Milestone | Plans Complete | Status | Completed |) — parsed to {completedPhases: null, totalPhases: null, totalPlans: null}. Because the body is wrapped in a bare try/catch the failure was silent, and phase.complete flipped the ROADMAP row to Complete but never advanced STATE's progress percent / completed-phase count. Parse the table by header instead: locate the `## Progress` section (line-anchored h2, mirroring the writer's #2012 scoping so an earlier Phase/Status/Completed-shaped table can't hijack it), read the header row to find the Status, Completed, and plans columns by name, then index each data row positionally. One code path now handles both the 4-column greenfield and 5-column milestone-grouped shapes — the same understanding the writer (cmdPhaseComplete, with its `cells.length === 5` branch) already had. Preserves the 999.x backlog exclusion, M/N plan summing, and the prior contract that a count of 0 is reported as null. Regression tests fold into tests/state.test.cjs: the template's own 5-column Progress block parses non-null, both shapes derive identically, 999.x stays excluded in the 5-column shape, and decoy tables (an earlier ## Retrospective table and an ### Progress h3 subheading) do not hijack the scope. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/phase-lifecycle.cts | 114 ++++++++++++++++++++++++++---------- tests/state.test.cjs | 126 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 208 insertions(+), 32 deletions(-) diff --git a/src/phase-lifecycle.cts b/src/phase-lifecycle.cts index 2175cd400..0ff2d4bbb 100644 --- a/src/phase-lifecycle.cts +++ b/src/phase-lifecycle.cts @@ -37,43 +37,93 @@ 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. + 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/state.test.cjs b/tests/state.test.cjs index 94163ec52..71d517cfa 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -9447,6 +9447,132 @@ 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}`); + }); +}); + // ─── Scenario B: state json total_phases via roadmapPhaseCount ─────────────── describe('bug #1445 — state json excludes 999.x phase headings from total_phases', () => { From 3582a45f9e940fe589e5deca8c71de8e70243c6c Mon Sep 17 00:00:00 2001 From: Behruz Nassre Esfahani Date: Sat, 11 Jul 2026 05:52:19 -0700 Subject: [PATCH 2/3] chore(#2137): add changeset Co-Authored-By: Claude Opus 4.8 (1M context) --- .changeset/merry-hawks-swim.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/merry-hawks-swim.md diff --git a/.changeset/merry-hawks-swim.md b/.changeset/merry-hawks-swim.md new file mode 100644 index 000000000..cb63f6436 --- /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. From 7a612941961da1ba8875b3d3b651842082ecef26 Mon Sep 17 00:00:00 2001 From: Behruz Nassre Esfahani Date: Mon, 13 Jul 2026 06:51:21 -0700 Subject: [PATCH 3/3] test(#2137): column-invariance property + boundary tests; mark parser pending #2143 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address @trek-e's review (Option 2, interim tactical fix) on PR #2168: - Add a fast-check property test (tests/derive-progress.property.test.cjs) asserting deriveProgressFromRoadmap's derived counts are invariant to header column ORDER and COUNT — shuffle the header cells and inject unrelated columns, assert counts match the data. This pins the parser's core new capability (header-name lookup vs. the old position-locked regex). Named derive-progress (not phase-lifecycle) so it isn't greedily attributed to the shorter `phase` prod prefix under lint-test-file-count; like the other *.property.test.cjs files it maps to no module and is cap-exempt. - Add boundary regression tests in state.test.cjs: header+separator only (0 rows -> all-null), exactly 1 row, and ragged rows (extra/short cells, handled without throwing). - Mark the adhoc Progress-table scan as pending the ADR-2143 parseMarkdownTable/TABLE_SCHEMAS seam (re-point pending #1372 -> #2143). - Fix the changeset lead-in to the required bold form. Per the maintainer, reader/writer parity and CONTEXT.md updates are explicitly out of scope for this stopgap — Phase 1 (ADR-2143) owns them. Validated: property test 200 runs green; state.test.cjs #2137 block 8/8; eslint clean; lint-test-file-count green; full `npm test` exit 0. Co-Authored-By: Claude Opus 4.8 (1M context) --- .changeset/merry-hawks-swim.md | 2 +- src/phase-lifecycle.cts | 5 + tests/derive-progress.property.test.cjs | 131 ++++++++++++++++++++++++ tests/state.test.cjs | 65 ++++++++++++ 4 files changed, 202 insertions(+), 1 deletion(-) create mode 100644 tests/derive-progress.property.test.cjs diff --git a/.changeset/merry-hawks-swim.md b/.changeset/merry-hawks-swim.md index cb63f6436..9f795dd85 100644 --- a/.changeset/merry-hawks-swim.md +++ b/.changeset/merry-hawks-swim.md @@ -2,4 +2,4 @@ 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. +**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 0ff2d4bbb..9d2dd2507 100644 --- a/src/phase-lifecycle.cts +++ b/src/phase-lifecycle.cts @@ -59,6 +59,11 @@ export function deriveProgressFromRoadmap(roadmapContent: string): RoadmapProgre // 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) { 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 f7696fe62..498f8967b 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -9650,6 +9650,71 @@ describe('#2137 regression: deriveProgressFromRoadmap parses the milestone-group 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 ───────────────