test(#2137): column-invariance property + boundary tests; mark parser pending #2143

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) <noreply@anthropic.com>

.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.

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) {

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('|')}`,
+          );
+        },
+      ),
+    );
+  });
+});

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 ───────────────